Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Alex Henrie <alexhenrie24@gmail.com>
Cc: 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
Date: Fri, 28 Feb 2020 15:16:01 -0800
Message-ID: <xmqqeeuecngu.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20200228215833.319691-1-alexhenrie24@gmail.com> (Alex Henrie's message of "Fri, 28 Feb 2020 14:58:33 -0700")

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

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-28 21:58 Alex Henrie
2020-02-28 23:16 ` Junio C Hamano [this message]
2020-02-29  3:03   ` Theodore Y. Ts'o
2020-02-29 16:16     ` Randall S. Becker
2020-02-29 16:33     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqeeuecngu.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=alexhenrie24@gmail.com \
    --cc=annulen@yandex.ru \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    --cc=rcdailey.lists@gmail.com \
    --cc=rsbecker@nexbridge.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git