* [PATCH v7 0/1] Reject non-ff pulls by default @ 2020-11-23 22:46 Felipe Contreras 2020-11-23 22:46 ` [PATCH v7 1/1] pull: add ff-only option Felipe Contreras 0 siblings, 1 reply; 11+ messages in thread From: Felipe Contreras @ 2020-11-23 22:46 UTC (permalink / raw) To: git Cc: Vít Ondruch, Alex Henrie, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King, Felipe Contreras This is an attempt to revive the old patch series [1], except the only thing it's doing is the important change: adding the option to reject non-ff pulls. Following this patch the next one should add a new default mode that warns instead of fail, and the next one change the default to ff-only. This leaves the interface in an inconsistent state, IMO, since I believe these should work: * git pull --rebase * git pull --merge * git -c pull.mode=rebase pull * git -c pull.mode=merge pull * git -c pull.mode=ff-only pull But that can be fixed later. Cheers. [1] https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/ Felipe Contreras (1): pull: add ff-only option Documentation/config/branch.txt | 2 ++ Documentation/config/pull.txt | 2 ++ builtin/pull.c | 6 +++++- rebase.c | 2 ++ rebase.h | 3 ++- t/t5520-pull.sh | 36 +++++++++++++++++++++++++++++++++ 6 files changed, 49 insertions(+), 2 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v7 1/1] pull: add ff-only option 2020-11-23 22:46 [PATCH v7 0/1] Reject non-ff pulls by default Felipe Contreras @ 2020-11-23 22:46 ` Felipe Contreras 2020-11-23 23:02 ` Felipe Contreras 0 siblings, 1 reply; 11+ messages in thread From: Felipe Contreras @ 2020-11-23 22:46 UTC (permalink / raw) To: git Cc: Vít Ondruch, Alex Henrie, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King, Felipe Contreras It is very typical for Git newcomers to inadvertently create merges and worse; inadvertently pushing them. This is one of the reasons many experienced users prefer to avoid 'git pull', and recommend newcomers to avoid it as well. To avoid these problems, and keep 'git pull' useful, it has been suggested that 'git pull' barfs by default if the merge is non-fast-forward, which unfortunately would break backwards compatibility. This patch leaves everything in place to enable this new mode, but it only gets enabled if the user specifically configures it; pull.rebase = ff-only. Later on this mode can be enabled by default. For the full discussion you can read: https://lore.kernel.org/git/5363BB9F.40102@xiplink.com/ Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Documentation/config/branch.txt | 2 ++ Documentation/config/pull.txt | 2 ++ builtin/pull.c | 6 +++++- rebase.c | 2 ++ rebase.h | 3 ++- t/t5520-pull.sh | 36 +++++++++++++++++++++++++++++++++ 6 files changed, 49 insertions(+), 2 deletions(-) diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt index cc5f3249fc..715987c759 100644 --- a/Documentation/config/branch.txt +++ b/Documentation/config/branch.txt @@ -81,6 +81,8 @@ branch.<name>.rebase:: "git pull" is run. See "pull.rebase" for doing this in a non branch-specific manner. + +When `ff-only` (or just 'f'), the rebase will only succeed if it's fast-forward. ++ When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase' so that the local merge commits are included in the rebase (see linkgit:git-rebase[1] for details). diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt index 5404830609..060bacc63c 100644 --- a/Documentation/config/pull.txt +++ b/Documentation/config/pull.txt @@ -14,6 +14,8 @@ pull.rebase:: pull" is run. See "branch.<name>.rebase" for setting this on a per-branch basis. + +When `ff-only` (or just 'f'), the rebase will only succeed if it's fast-forward. ++ When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase' so that the local merge commits are included in the rebase (see linkgit:git-rebase[1] for details). diff --git a/builtin/pull.c b/builtin/pull.c index 17aa63cd35..ba2777c401 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1033,8 +1033,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix) } free_commit_list(list); } - if (!ran_ff) + if (!ran_ff) { + if (opt_rebase == REBASE_FF_ONLY) + die(_("The pull was not fast-forward, please either merge or rebase.\n" + "If unsure, run 'git pull --merge'.")); ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point); + } if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON || recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)) diff --git a/rebase.c b/rebase.c index f8137d859b..eba1253552 100644 --- a/rebase.c +++ b/rebase.c @@ -20,6 +20,8 @@ enum rebase_type rebase_parse_value(const char *value) return REBASE_FALSE; else if (v > 0) return REBASE_TRUE; + else if (!strcmp(value, "ff-only") || !strcmp(value, "f")) + return REBASE_FF_ONLY; else if (!strcmp(value, "preserve") || !strcmp(value, "p")) return REBASE_PRESERVE; else if (!strcmp(value, "merges") || !strcmp(value, "m")) diff --git a/rebase.h b/rebase.h index cc723d4748..127febeb7c 100644 --- a/rebase.h +++ b/rebase.h @@ -7,7 +7,8 @@ enum rebase_type { REBASE_TRUE, REBASE_PRESERVE, REBASE_MERGES, - REBASE_INTERACTIVE + REBASE_INTERACTIVE, + REBASE_FF_ONLY }; enum rebase_type rebase_parse_value(const char *value); diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 9fae07cdfa..d96bdc90cc 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -819,4 +819,40 @@ test_expect_success 'git pull --rebase against local branch' ' test_cmp expect file2 ' +test_expect_success 'git pull fast-forward (ff-only)' ' + test_when_finished "git checkout master && git branch -D other test" && + test_config pull.rebase 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 +' + +test_expect_success 'git pull non-fast-forward (ff-only)' ' + test_when_finished "git checkout master && git branch -D other test" && + test_config pull.rebase 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 && + test_must_fail git pull +' + +test_expect_success 'git pull non-fast-forward with merge (ff-only)' ' + test_when_finished "git checkout master && git branch -D other test" && + test_config pull.rebase 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 --no-rebase +' + test_done -- 2.29.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-23 22:46 ` [PATCH v7 1/1] pull: add ff-only option Felipe Contreras @ 2020-11-23 23:02 ` Felipe Contreras 2020-11-23 23:22 ` Alex Henrie 0 siblings, 1 reply; 11+ messages in thread From: Felipe Contreras @ 2020-11-23 23:02 UTC (permalink / raw) To: Git Cc: Vít Ondruch, Alex Henrie, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King On Mon, Nov 23, 2020 at 4:46 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > It is very typical for Git newcomers to inadvertently create merges and > worse; inadvertently pushing them. This is one of the reasons many > experienced users prefer to avoid 'git pull', and recommend newcomers to > avoid it as well. > > To avoid these problems, and keep 'git pull' useful, it has been > suggested that 'git pull' barfs by default if the merge is > non-fast-forward, which unfortunately would break backwards > compatibility. > > This patch leaves everything in place to enable this new mode, but it > only gets enabled if the user specifically configures it; > > pull.rebase = ff-only. > > Later on this mode can be enabled by default. > > For the full discussion you can read: > > https://lore.kernel.org/git/5363BB9F.40102@xiplink.com/ Note: Philip Oakley's address was wrong (fixed now). -- Felipe Contreras ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-23 23:02 ` Felipe Contreras @ 2020-11-23 23:22 ` Alex Henrie 2020-11-23 23:32 ` Junio C Hamano 2020-11-24 0:08 ` Felipe Contreras 0 siblings, 2 replies; 11+ messages in thread From: Alex Henrie @ 2020-11-23 23:22 UTC (permalink / raw) To: Felipe Contreras Cc: Git, Vít Ondruch, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > This patch leaves everything in place to enable this new mode, but it > only gets enabled if the user specifically configures it; > > pull.rebase = ff-only. Why not use the existing pull.ff=only option instead of adding a new one? -Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-23 23:22 ` Alex Henrie @ 2020-11-23 23:32 ` Junio C Hamano 2020-11-23 23:51 ` Felipe Contreras 2020-11-24 0:08 ` Felipe Contreras 1 sibling, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2020-11-23 23:32 UTC (permalink / raw) To: Alex Henrie Cc: Felipe Contreras, Git, Vít Ondruch, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King Alex Henrie <alexhenrie24@gmail.com> writes: > On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: >> >> This patch leaves everything in place to enable this new mode, but it >> only gets enabled if the user specifically configures it; >> >> pull.rebase = ff-only. > > Why not use the existing pull.ff=only option instead of adding a new one? If you have pull.rebase=false, "git -c pull.ff=only pull" would fail as desired upon a non-fast-forward. But if you have pull.rebase=true, does it fail the same way (not a rhetorical question; I didn't try)? If so, I agree we do not need a new one. Otherwise, I am on two minds. Having just a single variable would be easier to manage, so pull.rebase=ff-only that is equivalent to pull.ff=only might be claimed to be UI improvement. On the other hand, it looks quite funny for that single variable that controls the way how pull works, whether rebase or merge is used, is pull.REBASE. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-23 23:32 ` Junio C Hamano @ 2020-11-23 23:51 ` Felipe Contreras 2020-11-24 1:45 ` Jeff King 0 siblings, 1 reply; 11+ messages in thread From: Felipe Contreras @ 2020-11-23 23:51 UTC (permalink / raw) To: Junio C Hamano Cc: Alex Henrie, Git, Vít Ondruch, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King On Mon, Nov 23, 2020 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > >> > >> This patch leaves everything in place to enable this new mode, but it > >> only gets enabled if the user specifically configures it; > >> > >> pull.rebase = ff-only. > > > > Why not use the existing pull.ff=only option instead of adding a new one? > > If you have pull.rebase=false, "git -c pull.ff=only pull" would fail > as desired upon a non-fast-forward. But if you have > pull.rebase=true, does it fail the same way (not a rhetorical > question; I didn't try)? If so, I agree we do not need a new one. No. It attempts the rebase, because whatever is set in pull.ff affects only the merge mode. Also, I don't think there's any way to tell git rebase to fail if it's not fast-forward (not that we should attempt the rebase anyway). > On the other hand, it looks quite funny for that single variable > that controls the way how pull works, whether rebase or merge is > used, is pull.REBASE. Which is precisely why I wanted to rename it to pull.mode. In my option git pull should have three main modes: 1. fast-forward only 2. merge 3. rebase The fast-forward only mode can be considered a merge, or a rebase, doesn't matter. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-23 23:51 ` Felipe Contreras @ 2020-11-24 1:45 ` Jeff King 0 siblings, 0 replies; 11+ messages in thread From: Jeff King @ 2020-11-24 1:45 UTC (permalink / raw) To: Felipe Contreras Cc: Junio C Hamano, Alex Henrie, Git, Vít Ondruch, Theodore Y . Ts'o, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King On Mon, Nov 23, 2020 at 05:51:42PM -0600, Felipe Contreras wrote: > > On the other hand, it looks quite funny for that single variable > > that controls the way how pull works, whether rebase or merge is > > used, is pull.REBASE. > > Which is precisely why I wanted to rename it to pull.mode. > > In my option git pull should have three main modes: > > 1. fast-forward only > 2. merge > 3. rebase > > The fast-forward only mode can be considered a merge, or a rebase, > doesn't matter. I agree that is much nicer. I'd worry a bit though that it may be confusing to users to have this new mode option _and_ the existing pull.rebase and pull.ff options, which overlap (but not completely). It might be something we could solve with good documentation, though. -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-23 23:22 ` Alex Henrie 2020-11-23 23:32 ` Junio C Hamano @ 2020-11-24 0:08 ` Felipe Contreras 2020-11-24 0:32 ` Raymond E. Pasco 1 sibling, 1 reply; 11+ messages in thread From: Felipe Contreras @ 2020-11-24 0:08 UTC (permalink / raw) To: Alex Henrie Cc: Git, Vít Ondruch, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King On Mon, Nov 23, 2020 at 5:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > This patch leaves everything in place to enable this new mode, but it > > only gets enabled if the user specifically configures it; > > > > pull.rebase = ff-only. > > Why not use the existing pull.ff=only option instead of adding a new one? Because that option is only for the merge. I'd have no objection in refactoring this option so it's used for both, but this breaks existing behavior. A user that has "pull.ff=only", and does a "git pull --rebase" might expect it to work. My main interest is what happens when the user has not configured anything, which I defined with a "push.mode=default", any option (e.g. --rebase/--merge) would override that default. -- Felipe Contreras ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-24 0:08 ` Felipe Contreras @ 2020-11-24 0:32 ` Raymond E. Pasco 2020-11-24 1:51 ` Alex Henrie 0 siblings, 1 reply; 11+ messages in thread From: Raymond E. Pasco @ 2020-11-24 0:32 UTC (permalink / raw) To: Felipe Contreras, Alex Henrie Cc: Git, Vít Ondruch, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King On Mon Nov 23, 2020 at 7:08 PM EST, Felipe Contreras wrote: > I'd have no objection in refactoring this option so it's used for > both, but this breaks existing behavior. A user that has > "pull.ff=only", and does a "git pull --rebase" might expect it to > work. > > My main interest is what happens when the user has not configured > anything, which I defined with a "push.mode=default", any option (e.g. > --rebase/--merge) would override that default. I feel like the parsimonious change to make is simply defaulting the existing "pull.ff" to "only". I think someone who has set "pull.rebase" expects pull --rebase behavior just as much as someone who passes --rebase on the command line. The issue in question is what someone who has not made any changes to the settings expects to happen with a plain "git pull", and I certainly agree that people who are not power users expect a fast-forward (I try not to force my opinions on workflow or style when onboarding people to Git, but I do always recommend "pull.ff=only" because I know this is a perennial pitfall). The problem is that, as it stands now, this would just leave the user with a cryptic error message ("Not possible to fast-forward, aborting") when they wanted to see remote changes. I think this might warrant an even more expanded message than the short one in this patch, but I'm not sure exactly what it should say - there are a few things the user might expect, but an error message isn't the best place for a crash course. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-24 0:32 ` Raymond E. Pasco @ 2020-11-24 1:51 ` Alex Henrie 2020-11-24 3:45 ` Felipe Contreras 0 siblings, 1 reply; 11+ messages in thread From: Alex Henrie @ 2020-11-24 1:51 UTC (permalink / raw) To: Raymond E. Pasco Cc: Felipe Contreras, Git, Vít Ondruch, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King On Mon, Nov 23, 2020 at 5:52 PM Raymond E. Pasco <ray@ameretat.dev> wrote: > > I feel like the parsimonious change to make is simply defaulting the > existing "pull.ff" to "only". I think someone who has set "pull.rebase" > expects pull --rebase behavior just as much as someone who passes > --rebase on the command line. The issue in question is what someone who > has not made any changes to the settings expects to happen with a plain > "git pull", and I certainly agree that people who are not power users > expect a fast-forward (I try not to force my opinions on workflow or > style when onboarding people to Git, but I do always recommend > "pull.ff=only" because I know this is a perennial pitfall). > > The problem is that, as it stands now, this would just leave the user > with a cryptic error message ("Not possible to fast-forward, aborting") > when they wanted to see remote changes. I think this might warrant an > even more expanded message than the short one in this patch, but I'm > not sure exactly what it should say - there are a few things the user > might expect, but an error message isn't the best place for a crash > course. Another problem is that when pull.ff=only but you really do want to allow merging if fast-forwarding is impossible, having to type `git pull --ff` to get that behavior is *very* counterintuitive :-( -Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v7 1/1] pull: add ff-only option 2020-11-24 1:51 ` Alex Henrie @ 2020-11-24 3:45 ` Felipe Contreras 0 siblings, 0 replies; 11+ messages in thread From: Felipe Contreras @ 2020-11-24 3:45 UTC (permalink / raw) To: Alex Henrie Cc: Raymond E. Pasco, Git, Vít Ondruch, Theodore Y . Ts'o, Jeff King, Andreas Krey, John Keeping, Richard Hansen, Philip Oakley, Brian M. Carlson, W. Trevor King On Mon, Nov 23, 2020 at 7:51 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > On Mon, Nov 23, 2020 at 5:52 PM Raymond E. Pasco <ray@ameretat.dev> wrote: > > > > I feel like the parsimonious change to make is simply defaulting the > > existing "pull.ff" to "only". I think someone who has set "pull.rebase" > > expects pull --rebase behavior just as much as someone who passes > > --rebase on the command line. The issue in question is what someone who > > has not made any changes to the settings expects to happen with a plain > > "git pull", and I certainly agree that people who are not power users > > expect a fast-forward (I try not to force my opinions on workflow or > > style when onboarding people to Git, but I do always recommend > > "pull.ff=only" because I know this is a perennial pitfall). > > > > The problem is that, as it stands now, this would just leave the user > > with a cryptic error message ("Not possible to fast-forward, aborting") > > when they wanted to see remote changes. I think this might warrant an > > even more expanded message than the short one in this patch, but I'm > > not sure exactly what it should say - there are a few things the user > > might expect, but an error message isn't the best place for a crash > > course. > > Another problem is that when pull.ff=only but you really do want to > allow merging if fast-forwarding is impossible, having to type `git > pull --ff` to get that behavior is *very* counterintuitive :-( Indeed. It would be much more intuitive to type "git pull --merge" which would override some default (e.g. do not merge unless it's a fast-forward). -- Felipe Contreras ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-11-24 3:47 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-23 22:46 [PATCH v7 0/1] Reject non-ff pulls by default Felipe Contreras 2020-11-23 22:46 ` [PATCH v7 1/1] pull: add ff-only option Felipe Contreras 2020-11-23 23:02 ` Felipe Contreras 2020-11-23 23:22 ` Alex Henrie 2020-11-23 23:32 ` Junio C Hamano 2020-11-23 23:51 ` Felipe Contreras 2020-11-24 1:45 ` Jeff King 2020-11-24 0:08 ` Felipe Contreras 2020-11-24 0:32 ` Raymond E. Pasco 2020-11-24 1:51 ` Alex Henrie 2020-11-24 3:45 ` Felipe Contreras
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).