| Message ID | 20250205030642.95252-1-ben.knoble+github@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pull: allow branch.<name>.rebase to override pull.ff=only | expand |
"D. Ben Knoble" <ben.knoble+github@gmail.com> writes: > When running "git pull" with the following configuration options, we > fail to merge divergent branches: > > - pull.ff=only > - pull.rebase (unset) > - branch.<current_branch>.rebase=true > > Yet it seems that the user intended to make rebase the default for the > current branch while using --ff-only for non-rebase pulls. Since this > case appears uncovered by existing tests, changing the behavior here > might be safe: it makes what was an error into a successful rebase. Hmph, to me it looks more like with pull.ff, the user, no matter what other variables say and which mode between merge and rebase a pull consolidates the histories, wanted to make sure they will never accept anything other than fast-forwarding of the history, because the end-user expects that they will pull only after they push out everything, i.e., the expectation is that the other side is a strict fast-forward or the user wants to examine the situation before making further damage to the local history. With that understanding, I am not sure "even though pull.ff tells us to stop unless the other side is a descendant of our history, if we are rebasing, it is OK if they have something we have never seen" is a good thing to do. So, I dunno.
On Wed, Feb 5, 2025 at 8:09 AM Junio C Hamano <gitster@pobox.com> wrote: > > "D. Ben Knoble" <ben.knoble+github@gmail.com> writes: > > > When running "git pull" with the following configuration options, we > > fail to merge divergent branches: > > > > - pull.ff=only > > - pull.rebase (unset) > > - branch.<current_branch>.rebase=true > > > > Yet it seems that the user intended to make rebase the default for the > > current branch while using --ff-only for non-rebase pulls. Since this > > case appears uncovered by existing tests, changing the behavior here > > might be safe: it makes what was an error into a successful rebase. > > Hmph, to me it looks more like with pull.ff, the user, no matter > what other variables say and which mode between merge and rebase a > pull consolidates the histories, wanted to make sure they will never > accept anything other than fast-forwarding of the history, because > the end-user expects that they will pull only after they push out > everything, i.e., the expectation is that the other side is a strict > fast-forward or the user wants to examine the situation before > making further damage to the local history. That's certainly one way to understand --ff-only, but I can't find it supported by existing docs (though it's what current code says, excepting lack of test for interaction with branch.name.merge). For example, `git help pull`: --ff-only Only update to the new history if there is no divergent local history. This is the default when no method for reconciling divergent histories is provided (via the --rebase=* flags). and `git help config`: pull.ff By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the tip of the current branch is fast-forwarded. When set to false, this variable tells Git to create an extra merge commit in such a case (equivalent to giving the --no-ff option from the command line). When set to only, only such fast-forward merges are allowed (equivalent to giving the --ff-only option from the command line). This setting overrides merge.ff when pulling. […] branch.autoSetupRebase When a new branch is created with git branch, git switch or git checkout that tracks another branch, this variable tells Git to set up pull to rebase instead of merge (see "branch.<name>.rebase"). When never, rebase is never automatically set to true. When local, rebase is set to true for tracked branches of other local branches. When remote, rebase is set to true for tracked branches of remote-tracking branches. When always, rebase will be set to true for all tracking branches. See "branch.autoSetupMerge" for details on how to set up a branch to track another branch. This option defaults to never. […] branch.<name>.rebase When true, rebase the branch <name> on top of the fetched branch, instead of merging the default branch from the default remote when "git pull" is run. See "pull.rebase" for doing this in a non branch-specific manner. [snip] NOTE: this is a possibly dangerous operation; do not use it unless you understand the implications (see git-rebase(1) for details). So I would tend to read branch.name.rebase as "you opted in to this, you know what you're doing" and let it override --ff-only. Granted, it's not clear just from reading the various git-config files which sections and variables override which, so I'm perhaps overly-reliant on the documentation to understand when those overrides happen (see "notes" in original post). > > With that understanding, I am not sure "even though pull.ff tells > us to stop unless the other side is a descendant of our history, if > we are rebasing, it is OK if they have something we have never seen" > is a good thing to do. > > So, I dunno. Agreed that if pull.ff=only is supposed to override all other options (except those on the command-line), this might be wrong. And `git pull --rebase` works in the scenario I described. I think that `pull.ff=only` + `branch.name.rebase=true` is a useful combination to say "unless I'm asking to rebase [via --rebase or branch settings], only permit fast-forward pulls." For example, my main or master branch is typically fast-forward only, while I want my topic branches to be rebased; preferably, all of those things happen for just "git pull." But maybe the intended way to accomplish what I want is pull.ff=true (the default?), which doesn't prevent accidental merges in the cases I want it to without setting branch.name.mergeOptions for each branch I want to protect from accidental pull-merges. (I'm in the habit of using fetch + merge as needed and mostly use pull to shortcut things when I'm confident, and obviously I can undo the accidental merge… but not having it in the first place is nice, too.) LMK if something in my position is not clear—my overreliance on parentheticals can be confusing.
"D. Ben Knoble" <ben.knoble+github@gmail.com> writes: >> So, I dunno. > > Agreed that if pull.ff=only is supposed to override all other options > (except those on the command-line), this might be wrong. And `git pull > --rebase` works in the scenario I described. Yeah, I view --ff-only as a safety measure for the user to say "my workflow is to make sure I do not have anything locally cooking on my branch when integrating with the other side, and stop me if I somehow made a mistake". If rebase or other options override, the folks in the rebasing camp, unlike in the merging camp, cannot benefit from such safety measure, which worries me.
On Wed, Feb 5, 2025 at 12:42 PM Junio C Hamano <gitster@pobox.com> wrote: > > "D. Ben Knoble" <ben.knoble+github@gmail.com> writes: > > >> So, I dunno. > > > > Agreed that if pull.ff=only is supposed to override all other options > > (except those on the command-line), this might be wrong. And `git pull > > --rebase` works in the scenario I described. > > Yeah, I view --ff-only as a safety measure for the user to say "my > workflow is to make sure I do not have anything locally cooking on > my branch when integrating with the other side, and stop me if I > somehow made a mistake". If rebase or other options override, the > folks in the rebasing camp, unlike in the merging camp, cannot > benefit from such safety measure, which worries me. Is there, then, an existing combination that means roughly to treat `git pull` with no other options like this: - if not rebasing, forbid merging and be equivalent to --ff-only - if rebasing is requested (because of branch.name.rebase or --rebase or …?), allow it In other words, something like a pull.merge=ff (or ff-only) meaning to apply the rules I've attempted to describe, in which case I would leave pull.ff unset? I suppose pull.rebase=true is close, but is not quite the same for me (I'd like to be warned when this would imply a non-fast-forward for a main branch, though the "rebasing" logs might be sufficient)…
On Tue, Feb 4, 2025 at 8:11 PM D. Ben Knoble <ben.knoble+github@gmail.com> wrote: > > When running "git pull" with the following configuration options, we > fail to merge divergent branches: > > - pull.ff=only > - pull.rebase (unset) > - branch.<current_branch>.rebase=true > > Yet it seems that the user intended to make rebase the default for the > current branch while using --ff-only for non-rebase pulls. You make an interesting point. The idea is that more specific options override less specific options. In this case, "fast-forward only" is more specific than "rebase" (because rebasing might or might not fast-forward), but "my branch" is also more specific than "all branches". So which option should win?
On Thu, Feb 6, 2025 at 9:36 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > On Tue, Feb 4, 2025 at 8:11 PM D. Ben Knoble > <ben.knoble+github@gmail.com> wrote: > > > > When running "git pull" with the following configuration options, we > > fail to merge divergent branches: > > > > - pull.ff=only > > - pull.rebase (unset) > > - branch.<current_branch>.rebase=true > > > > Yet it seems that the user intended to make rebase the default for the > > current branch while using --ff-only for non-rebase pulls. > > You make an interesting point. The idea is that more specific options > override less specific options. In this case, "fast-forward only" is > more specific than "rebase" (because rebasing might or might not > fast-forward), but "my branch" is also more specific than "all > branches". So which option should win?
On Mon, Feb 10, 2025 at 1:26 PM D. Ben Knoble <ben.knoble+github@gmail.com> wrote: > > On Thu, Feb 6, 2025 at 9:36 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > > > On Tue, Feb 4, 2025 at 8:11 PM D. Ben Knoble > > <ben.knoble+github@gmail.com> wrote: > > > > > > When running "git pull" with the following configuration options, we > > > fail to merge divergent branches: > > > > > > - pull.ff=only > > > - pull.rebase (unset) > > > - branch.<current_branch>.rebase=true > > > > > > Yet it seems that the user intended to make rebase the default for the > > > current branch while using --ff-only for non-rebase pulls. > > > > You make an interesting point. The idea is that more specific options > > override less specific options. In this case, "fast-forward only" is > > more specific than "rebase" (because rebasing might or might not > > fast-forward), but "my branch" is also more specific than "all > > branches". So which option should win?
diff --git a/builtin/pull.c b/builtin/pull.c index 9c4a00620a..c30f233dcc 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -326,13 +326,13 @@ static const char *config_get_ff(void) } /** - * Returns the default configured value for --rebase. It first looks for the + * Returns the default configured value for --rebase. It looks for the * value of "branch.$curr_branch.rebase", where $curr_branch is the current * branch, and if HEAD is detached or the configuration key does not exist, - * looks for the value of "pull.rebase". If both configuration keys do not - * exist, returns REBASE_FALSE. + * considers the result unspecified. Follow up by checking + * config_get_rebase_pull. */ -static enum rebase_type config_get_rebase(int *rebase_unspecified) +static enum rebase_type config_get_rebase_branch(int *rebase_unspecified) { struct branch *curr_branch = branch_get("HEAD"); const char *value; @@ -349,11 +349,22 @@ static enum rebase_type config_get_rebase(int *rebase_unspecified) free(key); } + *rebase_unspecified = 1; + return REBASE_INVALID; +} + +/* + * Looks for the value of "pull.rebase". If it does not exist, returns + * REBASE_FALSE. + */ +static enum rebase_type config_get_rebase_pull(int *rebase_unspecified) +{ + const char *value; + if (!git_config_get_value("pull.rebase", &value)) return parse_config_rebase("pull.rebase", value, 1); *rebase_unspecified = 1; - return REBASE_FALSE; } @@ -1026,7 +1037,7 @@ int cmd_pull(int argc, * are relying on the next if-condition happening before * the config_get_rebase() call so that an explicit * "--rebase" can override a config setting of - * pull.ff=only. + * pull.ff=only. [continued…] */ if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) { free(opt_ff); @@ -1034,8 +1045,20 @@ int cmd_pull(int argc, } } - if (opt_rebase < 0) - opt_rebase = config_get_rebase(&rebase_unspecified); + if (opt_rebase < 0) { + /* + * […continued] But, if the config requests rebase *for this + * branch*, override --ff-only, which otherwise takes precedence + * over pull.rebase=true. + */ + opt_rebase = config_get_rebase_branch(&rebase_unspecified); + if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only")) { + free(opt_ff); + opt_ff = xstrdup("--ff"); + } else { + opt_rebase = config_get_rebase_pull(&rebase_unspecified); + } + } if (repo_read_index_unmerged(the_repository)) die_resolve_conflict("pull"); diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 199a1d5db3..fd99f46aad 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -113,6 +113,14 @@ test_grep ! "You have divergent branches" err ' +test_expect_success 'pull.rebase not set and pull.ff=only and branch.<name>.rebase=true (not-fast-forward)' ' + git reset --hard c2 && + test_config pull.ff only && + git switch -c bc2 && + test_config branch.bc2.rebase true && + git pull . c1 +' + test_expect_success 'pull.rebase not set and --rebase given (not-fast-forward)' ' git reset --hard c2 && git pull --rebase . c1 2>err &&
When running "git pull" with the following configuration options, we fail to merge divergent branches: - pull.ff=only - pull.rebase (unset) - branch.<current_branch>.rebase=true Yet it seems that the user intended to make rebase the default for the current branch while using --ff-only for non-rebase pulls. Since this case appears uncovered by existing tests, changing the behavior here might be safe: it makes what was an error into a successful rebase. Add a test for the behavior and make it pass: this requires knowing from where the rebase was requested. Previous commits (e4dc25ed49 (pull: since --ff-only overrides, handle it first, 2021-07-22), adc27d6a93 (pull: make --rebase and --no-rebase override pull.ff=only, 2021-07-22)) took care to differentiate that --rebase overrides pull.ff=only, but don't distinguish which config setting requests the rebase. Split config_get_rebase into 2 parts so that we know where the rebase comes from, since we only want to allow branch-config to override pull.ff=only (like --rebase does); pull.rebase should still be overridden by pull.ff=only or --ff-only. Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com> --- Notes: - I also looked at ea1954af77 (pull: should be noop when already-up-to-date, 2021-11-17) when trying to understand how some options override others, but it didnt' seem germane to the final version. - I think I've got the right test script, since it's the one that started failing before I added the "else" branch to the new code (which also confirms that it's necessary to preserve current behavior); the only new behavior should be the one mentioned by the new test. - A possible #leftoverbits: it would be good to document more clearly the interplay of --ff[-only], --rebase, pull.ff, pull.rebase, and branch.<name>.rebase, particularly when they override each other. Confusingly, branch.<name>.merge has nothing to do with whether pull will merge or rebase ;) lest you think I'd forgotten something that _looks_ parallel to pull.rebase. builtin/pull.c | 39 ++++++++++++++++++++++++++++-------- t/t7601-merge-pull-config.sh | 8 ++++++++ 2 files changed, 39 insertions(+), 8 deletions(-)