| Message ID | 20200228215833.319691-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pull: warn if the user didn't say whether to rebase or to merge | expand |
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; > }
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
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
"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.
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; }
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(+)