Re: [PATCH v2 06/14] pull: move default warning
- From
Elijah Newren <newren@gmail.com>
- Date
- Dec 4, 2020, 23:18 UTC
- Message-ID
- <CABPp-BHH0baiECvtvVOoHR82upJ1C+0hy-ukS9Hi2ZFHh19nOg@mail.gmail.com>
- In-Reply-To
- <20201204061623.1170745-7-felipe.contreras@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. :-)
Show 87 quoted lines
>
> 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
>