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

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

[Alex Henrie]

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. 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.

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);
 
+	if (strcmp(opt_ff, "--ff-only") != 0) {
+		warning(_("Pulling without specifying whether to rebase or to merge is discouraged\n"
+			"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;
 }
 
-- 
2.25.1

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

[Junio C Hamano]

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;
>  }

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

[Theodore Y. Ts'o]

On Fri, Feb 28, 2020 at 03:16:01PM -0800, Junio C Hamano wrote:
> > 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:

I'd also note that there are some workflows that assume that --rebase
is *never* a good thing, even for contributors.  We can decide whether
we want to bias the git man page in favor of one workflow as opposed
to another, for the sake of new git users, but I don't think it's
accurate to say (or even imply) that there are only two workflows:
contributor/rebasing and maintainer/merging.

							- Ted

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

[Randall S. Becker]

On February 28, 2020 10:04 PM, Theodore Y. Ts'o wrote:
> To: Junio C Hamano <gitster@pobox.com>
> Cc: Alex Henrie <alexhenrie24@gmail.com>; git@vger.kernel.org;
> rcdailey.lists@gmail.com; newren@gmail.com; rsbecker@nexbridge.com;
> annulen@yandex.ru
> Subject: Re: [PATCH] pull: warn if the user didn't say whether to rebase
or to
> merge
> 
> On Fri, Feb 28, 2020 at 03:16:01PM -0800, Junio C Hamano wrote:
> > > 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:
> 
> I'd also note that there are some workflows that assume that --rebase is
> *never* a good thing, even for contributors.  We can decide whether we
> want to bias the git man page in favor of one workflow as opposed to
> another, for the sake of new git users, but I don't think it's accurate to
say
> (or even imply) that there are only two workflows:
> contributor/rebasing and maintainer/merging.

I second this sentiment. The repositories my community (outside my company)
has are typically large (3-5Gb of sources) with 10K-100K individual files.
They all use a */merging paradigm.

Randall

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

[Junio C Hamano]

"Theodore Y. Ts'o" <tytso@mit.edu> writes:

> I'd also note that there are some workflows that assume that --rebase
> is *never* a good thing, even for contributors.  We can decide whether
> we want to bias the git man page in favor of one workflow as opposed
> to another, for the sake of new git users, but I don't think it's
> accurate to say (or even imply) that there are only two workflows:
> contributor/rebasing and maintainer/merging.

Thanks for raising this.