| Message ID | b379fea097d65a28f1791f7f2f9432b6689a977f.1626536508.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Handle pull option precedence | expand |
Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > Fix the last few precedence tests failing in t7601 by now implementing > the logic to have --[no-]rebase override a pull.ff=only config setting. This is a semantic change that breaks current behavior. Now this does something different: git -c pull.ff=only pull --merge *And* the new behavior is not documented anywhere.
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > Fix the last few precedence tests failing in t7601 by now implementing > the logic to have --[no-]rebase override a pull.ff=only config setting. Because we only have opt_rebase and opt_ff, unless we are in a very early part of the code, we cannot tell if "opt_rebase >= 0" we see came from the command line or from the configuration, and because pull.rebase, pull.ff, --rebase=<kind>, and <--ff|--ff-only|--no-ff> have intricate interactions among them, we'd probably need to behave differently depending on where "opt_rebase >= 0" came from. The new code works only because we haven't called config_get_rebase() yet in this part. If we swapped the order of "if ff is not set, then figure it out from the configuration" (the code you are extending) and "if rebase is not set, read from the configuration", then the logic would not work. The resulting code looks correct, but it is subtle and may deserve an in-code comment. > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > builtin/pull.c | 5 ++++- > t/t7601-merge-pull-config.sh | 4 ++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 5ba376a7487..7e7c90f6a10 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -966,8 +966,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > > parse_repo_refspecs(argc, argv, &repo, &refspecs); > > - if (!opt_ff) > + if (!opt_ff) { > opt_ff = xstrdup_or_null(config_get_ff()); /* * Do we have --rebase=<kind> or --no-rebase from the * command line (note that we haven't read pull.rebase yet)? * If so, defeat the configured pull.ff=only; instead of * failing to integrate divergent histories, we want the * rebase or merge to happen. */ > + if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) > + opt_ff = "--ff"; > + } Thanks.
Junio C Hamano wrote: > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > Fix the last few precedence tests failing in t7601 by now implementing > > the logic to have --[no-]rebase override a pull.ff=only config setting. > > Because we only have opt_rebase and opt_ff, unless we are in a very > early part of the code, we cannot tell if "opt_rebase >= 0" we see > came from the command line or from the configuration, and because > pull.rebase, pull.ff, --rebase=<kind>, and <--ff|--ff-only|--no-ff> > have intricate interactions among them, we'd probably need to behave > differently depending on where "opt_rebase >= 0" came from. Where is that backwards-incompatible change explained in the documentation?
diff --git a/builtin/pull.c b/builtin/pull.c index 5ba376a7487..7e7c90f6a10 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -966,8 +966,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix) parse_repo_refspecs(argc, argv, &repo, &refspecs); - if (!opt_ff) + if (!opt_ff) { opt_ff = xstrdup_or_null(config_get_ff()); + if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) + opt_ff = "--ff"; + } if (opt_rebase < 0) opt_rebase = config_get_rebase(&rebase_unspecified); diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 23a1fbc17b3..12787d07289 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -244,11 +244,11 @@ test_expect_success '--ff-only takes precedence over pull.rebase=false' ' test_attempts_fast_forward -c pull.rebase=false pull --ff-only ' -test_expect_failure '--no-rebase overrides pull.ff=only' ' +test_expect_success '--no-rebase overrides pull.ff=only' ' test_does_need_full_merge -c pull.ff=only pull --no-rebase ' -test_expect_failure '--rebase takes precedence over pull.ff=only' ' +test_expect_success '--rebase takes precedence over pull.ff=only' ' test_does_rebase -c pull.ff=only pull --rebase '