| Message ID | 20210613045949.255090-2-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pull: obvious fixes | expand |
On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Currently "git pull --rebase" takes a shortcut in the case a > fast-forward merge is possible; run_merge() is called with --ff-only. > > However, "git merge" didn't have an --autostash option, so, when "git > pull --rebase --autostash" was called *and* the fast-forward merge > shortcut was taken, then the pull failed. > > This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash > works in dirty repo, 2017-06-01) by simply skipping the fast-forward > merge shortcut. > > Later on "git merge" learned the --autostash option [a03b55530a > (merge: teach --autostash option, 2020-04-07)], and so did "git pull" > [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)]. > > Therefore it's not necessary to skip the fast-forward merge shortcut > anymore when called with --rebase --autostash. > > Let's always take the fast-forward merge shortcut by essentially > reverting f15e7cf5cc. > > Reviewed-by: Elijah Newren <newren@gmail.com> I think you are basing the Reviewed-by on https://lore.kernel.org/git/CABPp-BEsQWsHMAmwc3gmJnXcS+aR-FtoMJxBRQ=BpARP49-L-Q@mail.gmail.com/; is that correct? Messages from folks that they seem to like the patch or believe it looks good should be translated into an Acked-by rather than a Reviewed-by; from Documentation/SubmittingPatches: * `Reviewed-by:`, unlike the other tags, can only be offered by the reviewer and means that she is completely satisfied that the patch is ready for application. It is usually offered only after a detailed review. Sorry for not catching this when you posted v3 & v4 of your earlier series. When your series exploded in size and seemed to just be accumulating additional changes you wanted to make in the area that weren't in response to reviewer feedback (I wasn't sure why the new patches in subsequent rerolls weren't just separate series), I didn't have the bandwidth to keep up and review them, so I just missed it. > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/pull.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index e8927fc2ff..a22293b7db 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > struct oid_array merge_heads = OID_ARRAY_INIT; > struct object_id orig_head, curr_head; > struct object_id rebase_fork_point; > - int autostash; > int rebase_unspecified = 0; > int can_ff; > > @@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > if (get_oid("HEAD", &orig_head)) > oidclr(&orig_head); > > - autostash = config_autostash; > if (opt_rebase) { > + int autostash = config_autostash; > if (opt_autostash != -1) > autostash = opt_autostash; > > @@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) && > submodule_touches_in_range(the_repository, &upstream, &curr_head)) > die(_("cannot rebase with locally recorded submodule modifications")); > - if (!autostash) { > - if (can_ff) { > - /* we can fast-forward this without invoking rebase */ > - opt_ff = "--ff-only"; > - ran_ff = 1; > - ret = run_merge(); > - } > + > + if (can_ff) { > + /* we can fast-forward this without invoking rebase */ > + opt_ff = "--ff-only"; > + ran_ff = 1; > + ret = run_merge(); > } > if (!ran_ff) > ret = run_rebase(&newbase, &upstream); > -- > 2.32.0
Elijah Newren wrote: > On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Currently "git pull --rebase" takes a shortcut in the case a > > fast-forward merge is possible; run_merge() is called with --ff-only. > > > > However, "git merge" didn't have an --autostash option, so, when "git > > pull --rebase --autostash" was called *and* the fast-forward merge > > shortcut was taken, then the pull failed. > > > > This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash > > works in dirty repo, 2017-06-01) by simply skipping the fast-forward > > merge shortcut. > > > > Later on "git merge" learned the --autostash option [a03b55530a > > (merge: teach --autostash option, 2020-04-07)], and so did "git pull" > > [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)]. > > > > Therefore it's not necessary to skip the fast-forward merge shortcut > > anymore when called with --rebase --autostash. > > > > Let's always take the fast-forward merge shortcut by essentially > > reverting f15e7cf5cc. > > > > Reviewed-by: Elijah Newren <newren@gmail.com> > > I think you are basing the Reviewed-by on > https://lore.kernel.org/git/CABPp-BEsQWsHMAmwc3gmJnXcS+aR-FtoMJxBRQ=BpARP49-L-Q@mail.gmail.com/; > is that correct? No, more like: [1] https://lore.kernel.org/git/20201205195313.1557473-5-felipe.contreras@gmail.com/ > Messages from folks that they seem to like the patch > or believe it looks good should be translated into an Acked-by rather > than a Reviewed-by; from Documentation/SubmittingPatches: To me an acknowledgment means something entirely different, and must be expressly given. > * `Reviewed-by:`, unlike the other tags, can only be offered by the > reviewer and means that she is completely satisfied that the patch > is ready for application. It is usually offered only after a > detailed review. Yeah, I read that after I sent v3. In this series I simply cherry-picked it from a previous series. I guess I'll just avoid both.
Carlo Arenas wrote: > On Sat, Jun 12, 2021 at 10:11 PM Felipe Contreras < > felipe.contreras@gmail.com> wrote: > > > - autostash = config_autostash; > > if (opt_rebase) { > > + int autostash = config_autostash; > > if (opt_autostash != -1) > > autostash = opt_autostash; > > > > since you are reducing the scope of the autostash variable anyway, why > not refactor it additionally for clarity with (something like): > > int autostash = (opt_autostash != -1) ? opt_autostash : config_autostash; Because I had like 15 branches on top of this, and wanted 1) to minimize modifications, and 2) to minimize the chance of the patch being rejected, and this is how the code was before f15e7cf5cc (pull: ff --rebase --autostash works in dirty repo, 2017-06-01), so in theory nobody could object. But yeah, that version makes sense, and in fact I probably had such cleanup in some branch.
diff --git a/builtin/pull.c b/builtin/pull.c index e8927fc2ff..a22293b7db 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix) struct oid_array merge_heads = OID_ARRAY_INIT; struct object_id orig_head, curr_head; struct object_id rebase_fork_point; - int autostash; int rebase_unspecified = 0; int can_ff; @@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (get_oid("HEAD", &orig_head)) oidclr(&orig_head); - autostash = config_autostash; if (opt_rebase) { + int autostash = config_autostash; if (opt_autostash != -1) autostash = opt_autostash; @@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix) recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) && submodule_touches_in_range(the_repository, &upstream, &curr_head)) die(_("cannot rebase with locally recorded submodule modifications")); - if (!autostash) { - if (can_ff) { - /* we can fast-forward this without invoking rebase */ - opt_ff = "--ff-only"; - ran_ff = 1; - ret = run_merge(); - } + + if (can_ff) { + /* we can fast-forward this without invoking rebase */ + opt_ff = "--ff-only"; + ran_ff = 1; + ret = run_merge(); } if (!ran_ff) ret = run_rebase(&newbase, &upstream);