| Message ID | 20201204061623.1170745-7-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pull: default warning improvements | expand |
On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Up to the point where can check if we can fast-forward or not. Seem to be missing some subjects in that sentence. ;-) Perhaps: Move the default warning to the point where we can check if we can fast-forward or not. > No functional changes. You didn't explain the reasoning for the change here, though I suspect it makes it easier to change the default to ff-only later. However, looking over the patch and pulling up the code, I actually find it pretty odd that this warning was in a function named config_get_rebase(). The warning is not rebase-specific, and so clearly does not belong there. And for such a function name, the only kinds of warnings I'd expect are ones where the user configured some option but set it to a value that cannot make sense. So it all around seems like the wrong place to me, and I find your patch to be a good cleanup. It would benefit from a slightly improved commit message though. :-) > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/pull.c | 39 +++++++++++++++++++++++---------------- > 1 file changed, 23 insertions(+), 16 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 8daba7539c..f82e214fc8 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -27,6 +27,8 @@ > #include "commit-reach.h" > #include "sequencer.h" > > +static int default_mode; > + > /** > * Parses the value of --rebase. If value is a false value, returns > * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is > @@ -344,21 +346,7 @@ static enum rebase_type config_get_rebase(void) > if (!git_config_get_value("pull.rebase", &value)) > return parse_config_rebase("pull.rebase", value, 1); > > - if (opt_verbosity >= 0 && !opt_ff) { > - advise(_("Pulling without specifying how to reconcile divergent branches is\n" > - "discouraged; you need to specify if you want a merge, or a rebase.\n" > - "You can squelch this message by running one of the following commands:\n" > - "\n" > - " git config pull.rebase false # merge (the default strategy)\n" > - " git config pull.rebase true # rebase\n" > - " git config pull.ff only # fast-forward only\n" > - "\n" > - "You can replace \"git config\" with \"git config --global\" to set a default\n" > - "preference for all repositories.\n" > - "If unsure, run \"git pull --no-rebase\".\n" > - "Read \"git pull --help\" for more information." > - )); > - } > + default_mode = 1; > > return REBASE_FALSE; > } > @@ -927,6 +915,7 @@ 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 can_ff; > > if (!getenv("GIT_REFLOG_ACTION")) > set_reflog_message(argc, argv); > @@ -1022,6 +1011,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > if (opt_rebase && merge_heads.nr > 1) > die(_("Cannot rebase onto multiple branches.")); > > + can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); > + > + if (default_mode && opt_verbosity >= 0 && !opt_ff) { > + advise(_("Pulling without specifying how to reconcile divergent branches is\n" > + "discouraged; you need to specify if you want a merge, or a rebase.\n" > + "You can squelch this message by running one of the following commands:\n" > + "\n" > + " git config pull.rebase false # merge (the default strategy)\n" > + " git config pull.rebase true # rebase\n" > + " git config pull.ff only # fast-forward only\n" > + "\n" > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > + "preference for all repositories.\n" > + "If unsure, run \"git pull --no-rebase\".\n" > + "Read \"git pull --help\" for more information." > + )); > + } > + > if (opt_rebase) { > int ret = 0; > if ((recurse_submodules == RECURSE_SUBMODULES_ON || > @@ -1029,7 +1036,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head)) > die(_("cannot rebase with locally recorded submodule modifications")); > > - if (get_can_ff(&orig_head, &merge_heads.oid[0])) { > + if (can_ff) { > /* we can fast-forward this without invoking rebase */ > opt_ff = "--ff-only"; > ret = run_merge(); > -- > 2.29.2 >
On Fri, Dec 4, 2020 at 5:18 PM Elijah Newren <newren@gmail.com> wrote: > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Up to the point where can check if we can fast-forward or not. > > Seem to be missing some subjects in that sentence. ;-) Perhaps: > > Move the default warning to the point where we can check if we can > fast-forward or not. Yes, I missed a "we". I usually continue what I said in the subject, but lately while reviewing my own patches in lore.kernel.org I've found it hard to understand the first paragraph since it's not easy to find the subject. Maybe I should stop doing that. > > No functional changes. > > You didn't explain the reasoning for the change here, though I suspect > it makes it easier to change the default to ff-only later. Right, I noticed that after the nth time I reviewed the series. It's not actually to change the default to ff-only, it's mainly to display the annoying warning only when a non-fast-forward pull is being attempted. I'll add that to the commit message. > However, > looking over the patch and pulling up the code, I actually find it > pretty odd that this warning was in a function named > config_get_rebase(). The warning is not rebase-specific, and so > clearly does not belong there. And for such a function name, the only > kinds of warnings I'd expect are ones where the user configured some > option but set it to a value that cannot make sense. So it all around > seems like the wrong place to me, and I find your patch to be a good > cleanup. It would benefit from a slightly improved commit message > though. :-) Indeed. The only reason it's there is that it's the only place we know the user has not done either --rebase, or --merge (--no-rebase), or the config equivalents. I fixed that by introducing a default_mode flag, but in other patch series I replace default_mode with a check for REBASE_DEFAULT (which doesn't exist yet). Cheers.
diff --git a/builtin/pull.c b/builtin/pull.c index 8daba7539c..f82e214fc8 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -27,6 +27,8 @@ #include "commit-reach.h" #include "sequencer.h" +static int default_mode; + /** * Parses the value of --rebase. If value is a false value, returns * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is @@ -344,21 +346,7 @@ static enum rebase_type config_get_rebase(void) if (!git_config_get_value("pull.rebase", &value)) return parse_config_rebase("pull.rebase", value, 1); - if (opt_verbosity >= 0 && !opt_ff) { - advise(_("Pulling without specifying how to reconcile divergent branches is\n" - "discouraged; you need to specify if you want a merge, or a rebase.\n" - "You can squelch this message by running one of the following commands:\n" - "\n" - " git config pull.rebase false # merge (the default strategy)\n" - " git config pull.rebase true # rebase\n" - " git config pull.ff only # fast-forward only\n" - "\n" - "You can replace \"git config\" with \"git config --global\" to set a default\n" - "preference for all repositories.\n" - "If unsure, run \"git pull --no-rebase\".\n" - "Read \"git pull --help\" for more information." - )); - } + default_mode = 1; return REBASE_FALSE; } @@ -927,6 +915,7 @@ 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 can_ff; if (!getenv("GIT_REFLOG_ACTION")) set_reflog_message(argc, argv); @@ -1022,6 +1011,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase && merge_heads.nr > 1) die(_("Cannot rebase onto multiple branches.")); + can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); + + if (default_mode && opt_verbosity >= 0 && !opt_ff) { + advise(_("Pulling without specifying how to reconcile divergent branches is\n" + "discouraged; you need to specify if you want a merge, or a rebase.\n" + "You can squelch this message by running one of the following commands:\n" + "\n" + " git config pull.rebase false # merge (the default strategy)\n" + " git config pull.rebase true # rebase\n" + " git config pull.ff only # fast-forward only\n" + "\n" + "You can replace \"git config\" with \"git config --global\" to set a default\n" + "preference for all repositories.\n" + "If unsure, run \"git pull --no-rebase\".\n" + "Read \"git pull --help\" for more information." + )); + } + if (opt_rebase) { int ret = 0; if ((recurse_submodules == RECURSE_SUBMODULES_ON || @@ -1029,7 +1036,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head)) die(_("cannot rebase with locally recorded submodule modifications")); - if (get_can_ff(&orig_head, &merge_heads.oid[0])) { + if (can_ff) { /* we can fast-forward this without invoking rebase */ opt_ff = "--ff-only"; ret = run_merge();
Up to the point where can check if we can fast-forward or not. No functional changes. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/pull.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)