| Message ID | 20201205040644.1259845-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | experiment: pull: change --ff-only and default mode | expand |
On Fri, Dec 4, 2020 at 8:06 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: This commit message should say something more than just "change x", it should have some words about what it is change from or to. I think the thrust of the patch is allowing --ff-only, --merge, and --rebase to countermand an earlier otherwise-conflicting option. Perhaps a commit message of the form: In git, we sometimes allow conflicting command line options with the last one winning, e.g. git log --patch --no-patch git log --no-patch --patch other times we just error out when conflicting options are given, e.g. git checkout -b --orphan NEWBRANCH git checkout --orphan -b NEWBRANCH Previously, we did neither with --no-ff, --merge, and --rebase. Change these options to have a last-one-wins behavior. (Although, after writing this out, I wonder if we want to die with a conflict message instead of going the route I suggested yesterday. I'm not certain if one is better than the other, but worth considering. Thoughts?) > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/pull.c | 40 ++++++++++++++++++++++++++-------------- > t/t5520-pull.sh | 23 +++++++++++++++++++++++ > 2 files changed, 49 insertions(+), 14 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index e389ffcdc3..95ecbdaad5 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -62,6 +62,7 @@ static int parse_opt_rebase(const struct option *opt, const char *arg, int unset > *value = parse_config_rebase("--rebase", arg, 0); > else > *value = unset ? REBASE_FALSE : REBASE_TRUE; > + if (*value != REBASE_INVALID) default_mode = 0; > return *value == REBASE_INVALID ? -1 : 0; > } > > @@ -114,6 +115,23 @@ static int opt_show_forced_updates = -1; > static char *set_upstream; > static struct strvec opt_fetch = STRVEC_INIT; > > +static int parse_opt_ff_only(const struct option *opt, const char *arg, int unset) > +{ > + char **value = opt->value; > + opt_rebase = REBASE_FALSE; > + *value = "--ff-only"; > + return 0; > +} > + > +static int parse_opt_merge(const struct option *opt, const char *arg, int unset) > +{ > + char **value = opt->value; > + opt_ff = NULL; > + *value = REBASE_FALSE; > + default_mode = 0; > + return 0; > +} > + > static struct option pull_options[] = { > /* Shared options */ > OPT__VERBOSITY(&opt_verbosity), > @@ -131,8 +149,9 @@ static struct option pull_options[] = { > "(false|true|merges|preserve|interactive)", > N_("incorporate changes by rebasing rather than merging"), > PARSE_OPT_OPTARG, parse_opt_rebase), > - OPT_SET_INT('m', "merge", &opt_rebase, > - N_("incorporate changes by merging"), 0), > + OPT_CALLBACK_F('m', "merge", &opt_rebase, NULL, > + N_("incorporate changes by merging"), > + PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_merge), > OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL, > N_("do not show a diffstat at the end of the merge"), > PARSE_OPT_NOARG | PARSE_OPT_NONEG), > @@ -161,9 +180,9 @@ static struct option pull_options[] = { > OPT_PASSTHRU(0, "ff", &opt_ff, NULL, > N_("allow fast-forward"), > PARSE_OPT_NOARG), > - OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL, > + OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL, > N_("abort if fast-forward is not possible"), > - PARSE_OPT_NOARG | PARSE_OPT_NONEG), > + PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_ff_only), > OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL, > N_("verify that the named commit has a valid GPG signature"), > PARSE_OPT_NOARG), > @@ -924,6 +943,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > > git_config(git_pull_config, NULL); > > + opt_ff = xstrdup_or_null(config_get_ff()); > + opt_rebase = config_get_rebase(); > + > argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0); > > if (cleanup_arg) > @@ -935,12 +957,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > > parse_repo_refspecs(argc, argv, &repo, &refspecs); > > - if (!opt_ff) > - opt_ff = xstrdup_or_null(config_get_ff()); > - > - if (opt_rebase < 0) > - opt_rebase = config_get_rebase(); > - > if (read_cache_unmerged()) > die_resolve_conflict("pull"); > > @@ -1037,10 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > } > } > > - /* Disable --ff-only when --merge is specified */ > - if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only")) > - opt_ff = NULL; > - > if (opt_rebase) { > int ret = 0; > if ((recurse_submodules == RECURSE_SUBMODULES_ON || > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh > index f48d0f8d50..c0cfde54e1 100755 > --- a/t/t5520-pull.sh > +++ b/t/t5520-pull.sh > @@ -881,4 +881,27 @@ test_expect_success 'git pull non-fast-forward error message' ' > grep -q "The pull was not fast-forward" error > ' > > +test_expect_success 'git pull --merge overrides --ffonly' ' You're missing a hyphen there; it should be --ff-only. > + test_when_finished "git checkout master && git branch -D other test" && > + test_config pull.ff only && > + git checkout -b other master^ && > + >new && > + git add new && > + git commit -m new && > + git checkout -b test -t other && > + git reset --hard master && > + git pull --ff-only --merge > +' > + > +test_expect_success 'git pull --ff-only overrides --merge' ' > + test_when_finished "git checkout master && git branch -D other test" && > + git checkout -b other master^ && > + >new && > + git add new && > + git commit -m new && > + git checkout -b test -t other && > + git reset --hard master && > + test_must_fail git pull --merge --ff-only > +' What about --ff-only and --rebase? Are there already tests for those?
On Sat, Dec 5, 2020 at 11:29 AM Elijah Newren <newren@gmail.com> wrote: > > On Fri, Dec 4, 2020 at 8:06 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > This commit message should say something more than just "change x", it > should have some words about what it is change from or to. I think > the thrust of the patch is allowing --ff-only, --merge, and --rebase > to countermand an earlier otherwise-conflicting option. Perhaps a > commit message of the form: It's not a real commit, it's just a preview at what the eventual diffstat towards v3 might look like.
Elijah Newren <newren@gmail.com> writes: > On Fri, Dec 4, 2020 at 8:06 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > This commit message should say something more than just "change x", it > should have some words about what it is change from or to. I think > the thrust of the patch is allowing --ff-only, --merge, and --rebase > to countermand an earlier otherwise-conflicting option. Perhaps a > commit message of the form: > > In git, we sometimes allow conflicting command line options with the > last one winning, e.g. > git log --patch --no-patch > git log --no-patch --patch > other times we just error out when conflicting options are given, e.g. > git checkout -b --orphan NEWBRANCH > git checkout --orphan -b NEWBRANCH > Previously, we did neither with --no-ff, --merge, and --rebase. > Change these options to have a last-one-wins behavior. If the choices are clearly among --ff-only, --merge and --rebase, I am fine with the last-one-wins change, and your explanation does make sense. I however am not sure how --no-ff and other --ff=<how> friends should interact with the primary two choices. --ff=no is used by those who want to merge, so "--merge --ff=no" combination is a sensible thing to accept. The same can be said for "--merge --ff" (fast-forward when able). It is interesting to realize that it would not be wrong to say "--rebase --ff" even. When their history we are rebasing our own work on top is a descendant of our history, such a rebase operation ends up in fast-forwarding to their tip.
diff --git a/builtin/pull.c b/builtin/pull.c index e389ffcdc3..95ecbdaad5 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -62,6 +62,7 @@ static int parse_opt_rebase(const struct option *opt, const char *arg, int unset *value = parse_config_rebase("--rebase", arg, 0); else *value = unset ? REBASE_FALSE : REBASE_TRUE; + if (*value != REBASE_INVALID) default_mode = 0; return *value == REBASE_INVALID ? -1 : 0; } @@ -114,6 +115,23 @@ static int opt_show_forced_updates = -1; static char *set_upstream; static struct strvec opt_fetch = STRVEC_INIT; +static int parse_opt_ff_only(const struct option *opt, const char *arg, int unset) +{ + char **value = opt->value; + opt_rebase = REBASE_FALSE; + *value = "--ff-only"; + return 0; +} + +static int parse_opt_merge(const struct option *opt, const char *arg, int unset) +{ + char **value = opt->value; + opt_ff = NULL; + *value = REBASE_FALSE; + default_mode = 0; + return 0; +} + static struct option pull_options[] = { /* Shared options */ OPT__VERBOSITY(&opt_verbosity), @@ -131,8 +149,9 @@ static struct option pull_options[] = { "(false|true|merges|preserve|interactive)", N_("incorporate changes by rebasing rather than merging"), PARSE_OPT_OPTARG, parse_opt_rebase), - OPT_SET_INT('m', "merge", &opt_rebase, - N_("incorporate changes by merging"), 0), + OPT_CALLBACK_F('m', "merge", &opt_rebase, NULL, + N_("incorporate changes by merging"), + PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_merge), OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL, N_("do not show a diffstat at the end of the merge"), PARSE_OPT_NOARG | PARSE_OPT_NONEG), @@ -161,9 +180,9 @@ static struct option pull_options[] = { OPT_PASSTHRU(0, "ff", &opt_ff, NULL, N_("allow fast-forward"), PARSE_OPT_NOARG), - OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL, + OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL, N_("abort if fast-forward is not possible"), - PARSE_OPT_NOARG | PARSE_OPT_NONEG), + PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_ff_only), OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL, N_("verify that the named commit has a valid GPG signature"), PARSE_OPT_NOARG), @@ -924,6 +943,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) git_config(git_pull_config, NULL); + opt_ff = xstrdup_or_null(config_get_ff()); + opt_rebase = config_get_rebase(); + argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0); if (cleanup_arg) @@ -935,12 +957,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix) parse_repo_refspecs(argc, argv, &repo, &refspecs); - if (!opt_ff) - opt_ff = xstrdup_or_null(config_get_ff()); - - if (opt_rebase < 0) - opt_rebase = config_get_rebase(); - if (read_cache_unmerged()) die_resolve_conflict("pull"); @@ -1037,10 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix) } } - /* Disable --ff-only when --merge is specified */ - if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only")) - opt_ff = NULL; - if (opt_rebase) { int ret = 0; if ((recurse_submodules == RECURSE_SUBMODULES_ON || diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index f48d0f8d50..c0cfde54e1 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -881,4 +881,27 @@ test_expect_success 'git pull non-fast-forward error message' ' grep -q "The pull was not fast-forward" error ' +test_expect_success 'git pull --merge overrides --ffonly' ' + test_when_finished "git checkout master && git branch -D other test" && + test_config pull.ff only && + git checkout -b other master^ && + >new && + git add new && + git commit -m new && + git checkout -b test -t other && + git reset --hard master && + git pull --ff-only --merge +' + +test_expect_success 'git pull --ff-only overrides --merge' ' + test_when_finished "git checkout master && git branch -D other test" && + git checkout -b other master^ && + >new && + git add new && + git commit -m new && + git checkout -b test -t other && + git reset --hard master && + test_must_fail git pull --merge --ff-only +' + test_done
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/pull.c | 40 ++++++++++++++++++++++++++-------------- t/t5520-pull.sh | 23 +++++++++++++++++++++++ 2 files changed, 49 insertions(+), 14 deletions(-)