Re: [PATCH] pull: warn if the user didn't say whether to rebase or to merge

[Junio C Hamano <gitster@pobox.com>]

Alex Henrie <alexhenrie24@gmail.com> writes:

> Most Git users are contributors, not maintainers, and want to use
> `git pull` to rebase their changes before sending a pull request to the
> upstream project. However, novice Git users often forget to specify the
> --rebase option when pulling, leading to an unwanted merge commit.

The logic above looks somewhat twisted.  Even if most Git users were
integrators who would not want "pull --rebase", everything you said
after "However" still holds true.

    Often novice Git users forget to say "pull --rebase" and ends up
    with an unnecessary merge from upstream.  What they usually want
    is either "pull --rebase" in the simpler cases, or "pull
    --ff-only" to update the copy of main integration branches, and
    rebase their work separately.  The pull.rebase configuration
    variable exists to help them in the simpler cases, but there is
    no mechanism to make these users aware of it.

> To
> avoid that situation, Git should require users to explicitly specify
> whether their primary workflow is a contributor/rebasing workflow or a
> maintainer/merging workflow.

There is nothing Git "should" do.  There are things we wish Git did,
and we give orders to the codebase to do so in our proposed log
message.  Perhaps like:

    Issue a warning message when no --[no-]rebase option from the
    command line and no pull.rebase configuration variable is given.
    This will inconvenience those who never want to "pull --rebase",
    who haven't had to do anything special, but the cost of the
    inconvenience is paid only once per user, which should be
    reasonable cost to help number of new users.



>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  builtin/pull.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 3e624d1e00..8ec8e6f5d3 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -327,6 +327,13 @@ static enum rebase_type config_get_rebase(void)
>  	if (!git_config_get_value("pull.rebase", &value))
>  		return parse_config_rebase("pull.rebase", value, 1);

This helper function is called only when opt_rebase < 0 in the
caller, which means there were no --[no-]rebase on the command line.
That's why we called this function to learn what the configuration
say.  So it is the right place to add the new check.

Luckily for us, the caller also made sure opt_ff is already set up
by calling config_get_ff() first when there was no --ff related
command line option ;-)

>  
> +	if (strcmp(opt_ff, "--ff-only") != 0) {

Style.  Do not write "!= 0" in the condition.  Just

	if (strcmp(opt_ff, "--ff-only")) {

is enough.

> +		warning(_("Pulling without specifying whether to rebase or to merge is discouraged\n"

I briefly wondered if this wants to be an advice instead, but the
way to squelch the message is already built into this codepath so
there is no need to ;-)

> +			"and will be disallowed in a future Git release.\n"
> +			"Next time, run `git config pull.rebase (true|false)` first\n"
> +			"or specify --rebase or --no-rebase on the command line.\n"));
> +	}
> +
>  	return REBASE_FALSE;
>  }