On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
>
> > Eventually we want to be able to display the warning only when
> > fast-forward merges are not possible.
> >
> > In order to do so we need to move the default warning up to the point
> > where we can check if we can fast-forward or not.
>
> Makes sense.
>
> > Additionally, config_get_rebase() was probably never its true home.
>
> I agree with this point. I've always found it suboptimal.
>
> > This requires a temporary variable to check if we are in the
> > "default mode" (no --rebase or --no-rebase specified).
>
> Two points:
>
> - "mode" is so overused a word; a more focused word is preferrable.
There's literally only one instance in the file, and it's a call to an
external function.
Plus, it refers to "git pull" without any --merge or --rebase (that's
the default mode in my book).
> - by introducing a local variable in cmd_pull() and passing a
> pointer to it to config_get_rebase(), we can easily avoid having
> to rely on an extra global variable.
>
> I'd suggest addressing the above along the following lines.
...
> to possibly cause it to set to true, and use that instead of the
> global variable to decide if we want to give the help text.
Yeah, there's only 38 global variables. We wouldn't want another one
which makes the code pretty straightforward.
> > @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> > if (opt_rebase && merge_heads.nr > 1)
> > die(_("Cannot rebase onto multiple branches."));
>
> And this is the point where we finish various error checks and
> starts to run either rebase or merge. It is as late as we could
> delay the "non-ff and you are not configured" message. In other
> words, the place chosen in cmd_pull() to move this code to is
> optimal.
Which is right before the fork between rebase and merge.
> > + if (default_mode && opt_verbosity >= 0 && !opt_ff) {
> > + advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> > + "discouraged. You can squelch this message by running one of the following\n"
> > + "commands sometime before your next pull:\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. You can also pass --rebase, --no-rebase,\n"
> > + "or --ff-only on the command line to override the configured default per\n"
> > + "invocation.\n"));
> > + }
>
> Either as a part of this step, as a part of the next step, or a
> separate follow-up patch, we should
>
> - create a single-purpose helper function that only calls advise()
> with the message and returns; name it show_advice_pull_non_ff().
If we are going for low-hanging fruit there were many in v4 of this
series, which actually improve behavior, not just code organization,
but OK.
> - correct the if() statement above, so that regardless of verbosity
> level, we can do _something_ common when the history does not
> fast-forward. I.e.
>
> if (rebase_unspecified && !opt_ff) {
> if (opt_verbosity >= 0)
> show_advice_pull_non_ff();
> }
>
> These would allow us to further turn the logic to
>
> if (rebase_unspecified && !opt_ff) {
> if (opt_verbosity >= 0 && advice_pull_non_ff)
> show_advice_pull_non_ff();
> die("not a fast-forward; must merge or rebase");
> }
Should actually be something like:
if (rebase_unspecified && !can_ff)
die("Not a fast-forward; must either merge or rebase");
But yeah.
--
Felipe Contreras
