Re: [PATCH v2 06/14] pull: move default warning

[Elijah Newren <newren@gmail.com>]

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
>