| Message ID | 20210627000855.530985-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pull: abort by default if fast-forwarding is impossible | expand |
On Sat, Jun 26, 2021 at 5:10 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > The behavior of warning but merging anyway is difficult to explain to > users because it sends mixed signals. End the confusion by firmly > requiring the user to specify whether they want a merge or a rebase. I would use some of Junio's wording from https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/: Given how annoying the "loudly warn but still go ahead" behaviour for git pull's default behavior when fast-forwarding is not possible (which was also displayed even when fast-forwarding was possible until c525de335e ("pull: display default warning only when non-ff", 2020-12-12)), and how easy it is for existing users to have squelched the annoyance by choosing between rebase and merge already, turn the warning into an error. As an added bonus, the previous behavior of git pull was difficult to explain, and making this change helps us simplify it. > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > Documentation/git-pull.txt | 14 +++++++++----- > builtin/pull.c | 32 +++++++++++++++----------------- > 2 files changed, 24 insertions(+), 22 deletions(-) > > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt > index 5c3fb67c01..0fb185811e 100644 > --- a/Documentation/git-pull.txt > +++ b/Documentation/git-pull.txt > @@ -16,13 +16,17 @@ DESCRIPTION > ----------- > > Incorporates changes from a remote repository into the current > -branch. In its default mode, `git pull` is shorthand for > -`git fetch` followed by `git merge FETCH_HEAD`. > +branch. `git pull` is shorthand for `git fetch` followed by I'd replace "shorthand for" with "shorthand for either" > +`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`. > > More precisely, 'git pull' runs 'git fetch' with the given > -parameters and calls 'git merge' to merge the retrieved branch > -heads into the current branch. > -With `--rebase`, it runs 'git rebase' instead of 'git merge'. > +parameters and then, by default, attempts to fast-forward the > +current branch to the remote branch head. If fast-forwarding > +is not possible because the local and remote branches have Good to here. > +diverged, the `--rebase` option causes 'git rebase' to be... > +called to rebase the local branch upon the remote branch, and > +the `--no-rebase` option causes 'git merge' to be called to > +merge the retrieved branch heads into the current branch. This is okay, but can we just simplify it to: ....diverged, then the user must specify whether to rebase or merge (see the --rebase and --no-rebase flags). While at it, we may also want to change the documentation for the --no-rebase flag. Instead of --no-rebase:: Override earlier --rebase. which can be confusing since --rebase takes so many different choices, instead: --no-rebase:: This is shorthand for --rebase=false > <repository> should be the name of a remote repository as > passed to linkgit:git-fetch[1]. <refspec> can name an > diff --git a/builtin/pull.c b/builtin/pull.c > index e8927fc2ff..21eaebc463 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_ > return ret; > } > > -static void show_advice_pull_non_ff(void) > +static void die_pull_non_ff(void) > { > - advise(_("Pulling without specifying how to reconcile divergent branches is\n" > - "discouraged. You can squelch this message by running one of the following\n" > - "commands sometime before your next pull:\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. You can also pass --rebase, --no-rebase,\n" > - "or --ff-only on the command line to override the configured default per\n" > - "invocation.\n")); > + die(_("Pulling without specifying how to reconcile divergent branches is not\n" > + "possible. You can squelch this message by running one of the following\n" > + "commands sometime before your next pull:\n" > + "\n" > + " git config pull.rebase false # merge\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. You can also pass --rebase, --no-rebase,\n" > + "or --ff-only on the command line to override the configured default per\n" > + "invocation.\n")); > } > > int cmd_pull(int argc, const char **argv, const char *prefix) > @@ -1047,10 +1047,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > > can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); > > - if (rebase_unspecified && !opt_ff && !can_ff) { > - if (opt_verbosity >= 0) > - show_advice_pull_non_ff(); > - } > + if (rebase_unspecified && !opt_ff && !can_ff) > + die_pull_non_ff(); > > if (opt_rebase) { > int ret = 0; > -- > 2.32.0.95.g4a22da5c5a The code changes look good, but you'll need to add several test changes as well, or this code change will cause test failures. You'll need to go through the relevant `git pull` invocations in the testsuite and add the --no-rebase flag. Thanks for working on this.
Alex Henrie wrote: > The behavior of warning but merging anyway is difficult to explain to > users because it sends mixed signals. End the confusion by firmly > requiring the user to specify whether they want a merge or a rebase. When were users warned about this change? Generally before making such big UI changes to core commands there needs to be a deprecation period before pulling the trigger. Second, supposing this was a proposal to add such warning, we would need a configuration so the user can decide to retain the old behavior, or move to the new one. What is that configuration? How can a user choose to enable the new behavior in v2.32 (or any other version)? Also, a bunch of tests are broken after this change: t4013-diff-various.sh t5521-pull-options.sh t5524-pull-msg.sh t5520-pull.sh t5553-set-upstream.sh t5604-clone-reference.sh t6409-merge-subtree.sh t6402-merge-rename.sh t6417-merge-ours-theirs.sh t7601-merge-pull-config.sh t7603-merge-reduce-heads.sh If you didn't mean this patch to be applied then perhaps add the RFC prefix. > --- a/Documentation/git-pull.txt > +++ b/Documentation/git-pull.txt > @@ -16,13 +16,17 @@ DESCRIPTION > ----------- > > Incorporates changes from a remote repository into the current > -branch. In its default mode, `git pull` is shorthand for > -`git fetch` followed by `git merge FETCH_HEAD`. > +branch. `git pull` is shorthand for `git fetch` followed by > +`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`. Is it? If I have a pull.rebase=merges is `git pull` the same as doing `git rebase FETCH_HEAD`? > More precisely, 'git pull' runs 'git fetch' with the given > -parameters and calls 'git merge' to merge the retrieved branch > -heads into the current branch. > -With `--rebase`, it runs 'git rebase' instead of 'git merge'. > +parameters and then, by default, attempts to fast-forward the > +current branch to the remote branch head. OK. > +If fast-forwarding > +is not possible because the local and remote branches have > +diverged, the `--rebase` option causes 'git rebase' to be > +called to rebase the local branch upon the remote branch, Does --rebase only work when a fast-forward isn't possible. > +and > +the `--no-rebase` option causes 'git merge' to be called to > +merge the retrieved branch heads into the current branch. Isn't merge called when a fast-forward is possible? > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_ > return ret; > } > > -static void show_advice_pull_non_ff(void) > +static void die_pull_non_ff(void) > { > - advise(_("Pulling without specifying how to reconcile divergent branches is\n" > - "discouraged. You can squelch this message by running one of the following\n" > - "commands sometime before your next pull:\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. You can also pass --rebase, --no-rebase,\n" > - "or --ff-only on the command line to override the configured default per\n" > - "invocation.\n")); > + die(_("Pulling without specifying how to reconcile divergent branches is not\n" > + "possible. You can squelch this message by running one of the following\n" > + "commands sometime before your next pull:\n" Is squelching this message really what we are aming for? > + " git config pull.rebase false # merge\n" > + " git config pull.rebase true # rebase\n" > + " git config pull.ff only # fast-forward only\n" `git config pull.ff only` will get rid of this messge, but the pull would still fail, only that it will be a different message: fatal: Not possible to fast-forward, aborting. Doesn't seem very user-friendly. > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > + "or --ff-only on the command line to override the configured default per\n" > + "invocation.\n")); Can I? git -c pull.rebase=true pull --ff-only `--ff-only` doesn't seem to be overriding the configuration. In addition to all the issues I've already pointed out, the message seems rather big for a die(). I think this is close to the end-goal we should be aiming for, but there's a lot that is missing before we should even consider flipping the switch. Cheers.
Elijah Newren wrote: > The code changes look good, but you'll need to add several test > changes as well, or this code change will cause test failures. Wouldn't you consider sending a patch without running 'make test' "cavalier"? > Thanks for working on this. Such a completely different tone for a "cavalier" patch depending 100% on the person who sent it. Weird.
On Sat, Jun 26, 2021 at 9:16 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Elijah Newren wrote: > > > The code changes look good, but you'll need to add several test > > changes as well, or this code change will cause test failures. > > Wouldn't you consider sending a patch without running 'make test' > "cavalier"? > > > Thanks for working on this. > > Such a completely different tone for a "cavalier" patch depending 100% > on the person who sent it. Weird. First of all, attacking a random bystander like Alex is rather uncalled for. As far as we know, Alex is trying to help out and made a mistake or oversight, but will likely correct the problem if kindly pointed out. This email is egregiously far from constructive criticism for Alex. Second, why do you grossly mischaracterize folks (not just me) and try so hard to insinuate unfairness? I don't get it. Do you really not understand? Let me attempt to explain... Not running the tests is a problem, sure. I pointed it out. It could have been a mistake or oversight on Alex's part. Now, if Alex were to state it was not accidental and then repeatedly defend submitting patches without running tests, I would absolutely start calling out both his submission and his professed level of reasonable due diligence. He hasn't done that though. People deserve a chance to respond and correct things first. We gave you that same opportunity. It was pointed out to you rather politely that you had neglected to act on a report that a patch caused a segfault[1]; in fact, you re-submitted the old patch as-is and suggested it might be a good default[2]. You had a chance to point out that what you had done was just a mistake or oversight. Instead, you confirmed that you saw the report and just didn't bother acting on it[3]. And then you defended your misbehavior[4]. Over[5] and over[6] and over[7] again. And you also defended your related patches[8] instead of double checking or trying to correct them -- even when prompted to double check[9] -- despite those additional patches having severe problems of their own[10]. So, yes, I used terms like negligent, reckless, careless, and cavalier in my critiques with you, but I very carefully always used those terms to describe either the patches you submitted or the amount of due diligence that you repeatedly defended. I never applied those terms to you. And those terms didn't apply to you, as you could have chosen to change and use a different level of due diligence. So, your accusations of personal attacks, harassment, and ad hominem arguments that you made against me earlier[11], and your insinuations of unfairness both then and now, are all false. This, of course, isn't your first time grossly mischaracterizing events and trying to claim unfairness. Let me give you a second example of why there was no unfairness in case the first is hard for you to understand. Let's use [12] as another example, and explain the lead-up to that time: I politely gave you a chance to correct a misleading claim[13], which you then instead admitted to being no mistake[14], and didn't bother to correct. The original misleading claim was a minor issue. If it had been corrected (even if it had been intentional), it could have been no big deal. The reason I drew significant attention to it[15] was due to your response. I pointed out quite clearly when I drew attention to it, that it was your *response* to the error that was problematic, and even reiterated it in a follow-up. I do not see why you ignored that, mischaracterized my objections as being entirely about the initial error, hand picked an example that didn't even do what you said it did, all to lead up to your "Could it be that this has absolutely nothing to do with the action, and everything to do with the person doing the action?" The answer is quite simply "Of course not." I would also call out other people if they stated that misleading claims of theirs were intentionally submitted and did not try to correct them. I just haven't ever seen anyone else do that. (Let alone follow it up again a short time later quite similarly[16], except that at least you corrected it.) Unfortunately, these aren't isolated incidents of claims of unfairness, and it's not just me you hurl these accusations against. In fact, in other cases with other people you go much further overboard. For example, you recently used the phrase "petty personal animus"[17] with Junio. Perhaps you don't see the problem, so let me state it in a way you might recognize: Could it be that you always use these perjorative phrases in questions just to provide plausible deniability that you are directly accusing folks of severe personal failings? Just as I shouldn't ask the above question (I only did so to try to highlight how your questions come across), you shouldn't be asking yours. It's inappropriate. Putting such negative words in a question format might make the phrases less objectionable, but the questions are still way out of line. If you react negatively to my example sentence above, hopefully that helps you see why that is inappropriate. Please correct this pattern. And all of this hardly even touches on your interactions with reviewers, which sadly is following the same arc as in 2013 and 2014[18]. In short, please stop: * Please stop attacking random bystanders like Alex (or other folks[19]) * Please stop defending shockingly inadequate levels of due diligence, and adopt a higher one. * Please stop accusing others of bias, unfairness, and other shortcomings. Learn to recognize why your behavior often results in others changing theirs. * And please find ways to stop burning out reviewers, especially since they are the rate-limiting resource in determining git's overall velocity Please know that I really do think you are talented. I tried really hard to help you change and improve because I thought you could have been a great addition to the community. I'm nowhere near as talented as Michael in expressing it (again, [18]), but I really wish you had chosen to change. I agree with his sentiments back then. [1] https://lore.kernel.org/git/YMYnVWSEgxvKRU9j@coredump.intra.peff.net/ [2] https://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/ [3] https://lore.kernel.org/git/60c647c1d9b5c_41f452089@natae.notmuch/ [4] https://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/ [5] https://lore.kernel.org/git/60c86ff87d598_e6332085b@natae.notmuch/ [6] https://lore.kernel.org/git/60c85bfd112a9_e633208d5@natae.notmuch/ [7] https://lore.kernel.org/git/60cb7f3e66cfd_1305720822@natae.notmuch/ [8] https://lore.kernel.org/git/60c85bfd112a9_e633208d5@natae.notmuch/ [9] https://lore.kernel.org/git/CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com/ [10] https://lore.kernel.org/git/CABPp-BEXtJtkkh9Diuo4e1K1ci=vggGxkLRDfkpOH12LM8TCfA@mail.gmail.com/ [11] https://lore.kernel.org/git/60cb5a02f1b31_1259c2086f@natae.notmuch/ [12] https://lore.kernel.org/git/60cb7f3e66cfd_1305720822@natae.notmuch/ [13] https://lore.kernel.org/git/CABPp-BGstXDbzxpySw7q_jn22HD05MsrZeHNv+kXFHOFS2_WCQ@mail.gmail.com/ [14] https://lore.kernel.org/git/60c887f678c88_e63320846@natae.notmuch/ [15] https://lore.kernel.org/git/CABPp-BG53Kd7MhzE3hdq5fjBQVV2Ew3skcUCAuTfM5daP2wmZA@mail.gmail.com/ [16] https://lore.kernel.org/git/CABPp-BF1noWhiJadHzjJmnGo8hdZj6Fk7XnZ=u6BVVSGfHE7og@mail.gmail.com/ [17] https://lore.kernel.org/git/60d289c84fadf_312208dc@natae.notmuch/ [18] https://lore.kernel.org/git/53709788.2050201@alum.mit.edu/ [19] "if you eyes have trouble seeing", from https://lore.kernel.org/git/60839422353fc_10cb9208c7@natae.notmuch/ https://lore.kernel.org/git/87fszfafkh.fsf@angela.anarc.at/
Elijah Newren wrote: > On Sat, Jun 26, 2021 at 9:16 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Elijah Newren wrote: > > > > > The code changes look good, but you'll need to add several test > > > changes as well, or this code change will cause test failures. > > > > Wouldn't you consider sending a patch without running 'make test' > > "cavalier"? > > > > > Thanks for working on this. > > > > Such a completely different tone for a "cavalier" patch depending 100% > > on the person who sent it. Weird. > > First of all, attacking a random bystander like Alex is rather > uncalled for. I did not attack Alex, I simply pointed out the fact that his patch breaks the test suite, which you did too. I asked *you* if you didn't consider this "cavalier". Your reaction seems inconsistent. This question has nothing to do with Alex. I don't throw personal attacks, nor do I chastise contributors for attempting to improve the project. That's your department. I focus on the ball, not the player.
On Sat, Jun 26, 2021 at 7:34 PM Elijah Newren <newren@gmail.com> wrote: > > Thanks for working on this. Thanks for all the feedback. I will incorporate your suggestions about the commit message and documentation into the next revision, whenever that is. On Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Also, a bunch of tests are broken after this change: > > t4013-diff-various.sh > t5521-pull-options.sh > t5524-pull-msg.sh > t5520-pull.sh > t5553-set-upstream.sh > t5604-clone-reference.sh > t6409-merge-subtree.sh > t6402-merge-rename.sh > t6417-merge-ours-theirs.sh > t7601-merge-pull-config.sh > t7603-merge-reduce-heads.sh > > If you didn't mean this patch to be applied then perhaps add the RFC > prefix. I actually did run `make test` before sending the patch, but when so many seemingly unrelated tests failed, I foolishly assumed that they were pre-existing failures. I should have run the tests on master for comparison, sorry. Or at least put "RFC" in the subject instead of "PATCH" as you suggest. I sincerely apologize for my lack of due diligence and I know that I need to do better at self-reviewing patches before sending them. > Alex Henrie wrote: > > > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > > + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > > + "or --ff-only on the command line to override the configured default per\n" > > + "invocation.\n")); > > Can I? > > git -c pull.rebase=true pull --ff-only > > `--ff-only` doesn't seem to be overriding the configuration. Ahh, so /that's/ what you meant by "3. Fix all the wrong behavior with --ff, --no-ff, and -ff-only". That does seem like something worth trying to fix before making the switch. On Sat, Jun 26, 2021 at 10:16 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Elijah Newren wrote: > > > The code changes look good, but you'll need to add several test > > changes as well, or this code change will cause test failures. > > Wouldn't you consider sending a patch without running 'make test' > "cavalier"? > > > Thanks for working on this. > > Such a completely different tone for a "cavalier" patch depending 100% > on the person who sent it. Weird. I'm trying to make things better, as I'm sure you are as well, and I know that I make a lot of mistakes and need the help of more experienced contributors like you. So please be nice, even if others are mean to you, because regardless of whether these comments were directed at me, this kind of comment just makes me feel like giving up. On Sun, Jun 27, 2021 at 10:46 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > I don't throw personal attacks, nor do I chastise contributors for > attempting to improve the project. That's your department. "That's your department" is a personal attack. How about we all go spend some time enjoying the weather, and then get back to working on Git problems later this week. -Alex
Alex Henrie wrote: > On Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Also, a bunch of tests are broken after this change: > > > > t4013-diff-various.sh > > t5521-pull-options.sh > > t5524-pull-msg.sh > > t5520-pull.sh > > t5553-set-upstream.sh > > t5604-clone-reference.sh > > t6409-merge-subtree.sh > > t6402-merge-rename.sh > > t6417-merge-ours-theirs.sh > > t7601-merge-pull-config.sh > > t7603-merge-reduce-heads.sh > > > > If you didn't mean this patch to be applied then perhaps add the RFC > > prefix. > > I actually did run `make test` before sending the patch, but when so > many seemingly unrelated tests failed, I foolishly assumed that they > were pre-existing failures. I should have run the tests on master for > comparison, sorry. Or at least put "RFC" in the subject instead of > "PATCH" as you suggest. I sincerely apologize for my lack of due > diligence and I know that I need to do better at self-reviewing > patches before sending them. I personally don't see any need for apologies, we all make mistakes, just keep it in mind for the future. Personally I prefer to run prove instead, because the output is less verbose, and there's a nice summary at the end: prove t[0-9][0-9][0-9][0-9]-*.sh > > Alex Henrie wrote: > > > > > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > > > + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > > > + "or --ff-only on the command line to override the configured default per\n" > > > + "invocation.\n")); > > > > Can I? > > > > git -c pull.rebase=true pull --ff-only > > > > `--ff-only` doesn't seem to be overriding the configuration. > > Ahh, so /that's/ what you meant by "3. Fix all the wrong behavior with > --ff, --no-ff, and -ff-only". That does seem like something worth > trying to fix before making the switch. That is just one of the inconsistencies, there's many others. Consider all these: git pull git pull --ff-only git -c pull.ff=only pull git -c pull.ff=only pull --ff-only Do you see any good reason why the error message should be different for any of these? How about these? git pull git pull --ff Why does --ff make the second command work? Even worse when you start considering more combinations, like "--no-ff --rebase" which is obviously nonsense, but it depends on the configuration. I proposed a solution for some of these inconsistencies with --ff*, but was ignored [1]. > On Sat, Jun 26, 2021 at 10:16 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Elijah Newren wrote: > > > > > The code changes look good, but you'll need to add several test > > > changes as well, or this code change will cause test failures. > > > > Wouldn't you consider sending a patch without running 'make test' > > "cavalier"? > > > > > Thanks for working on this. > > > > Such a completely different tone for a "cavalier" patch depending 100% > > on the person who sent it. Weird. > > I'm trying to make things better, as I'm sure you are as well, and I > know that I make a lot of mistakes and need the help of more > experienced contributors like you. So please be nice, even if others > are mean to you, because regardless of whether these comments were > directed at me, this kind of comment just makes me feel like giving > up. My mail was directed towards Elijah, not you. I do appreciate all contributions, especially if they were done pro bono. > On Sun, Jun 27, 2021 at 10:46 AM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > I don't throw personal attacks, nor do I chastise contributors for > > attempting to improve the project. That's your department. > > "That's your department" is a personal attack. How about we all go > spend some time enjoying the weather, and then get back to working on > Git problems later this week. Stating facts about what a person did is not a personal attack, it's just stating facts. Elijah did attacked me personally, that's a fact [2]: If I were in charge, at this point I would drop all of Felipe's patches on the floor (not just the ones from these threads), and not accept any further ones. I am not in charge, though, and you have more patience than me. But I will not be reviewing or responding to Felipe's patches further. Please do not trust future submissions from him that have an "Acked-by" or "Reviewed-by" from me, including resubmissions of past patches. And he did so after I made a "mistake" (in his view) that was much much less serious than the one you did. I am merely pointing out the inconsistency, that's all. This has absolutely nothing to do with you, again... I appreciate you used some of your free time to try to improve git pull. Whatever beef Elijah has with me is an entirely different subject, that I suggest you leave on the table. I don't think anything constructive can come from trying to defend what he did. Let's just focus on the patches. Cheers. [1] https://lore.kernel.org/git/5fdd154264baf_130e182082b@natae.notmuch/ [2] https://lore.kernel.org/git/CABPp-BG53Kd7MhzE3hdq5fjBQVV2Ew3skcUCAuTfM5daP2wmZA@mail.gmail.com/
On Sun, Jun 27 2021, Felipe Contreras wrote: > Alex Henrie wrote: > >> On Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras >> <felipe.contreras@gmail.com> wrote: >> > >> > Also, a bunch of tests are broken after this change: >> > >> > t4013-diff-various.sh >> > t5521-pull-options.sh >> > t5524-pull-msg.sh >> > t5520-pull.sh >> > t5553-set-upstream.sh >> > t5604-clone-reference.sh >> > t6409-merge-subtree.sh >> > t6402-merge-rename.sh >> > t6417-merge-ours-theirs.sh >> > t7601-merge-pull-config.sh >> > t7603-merge-reduce-heads.sh >> > >> > If you didn't mean this patch to be applied then perhaps add the RFC >> > prefix. >> >> I actually did run `make test` before sending the patch, but when so >> many seemingly unrelated tests failed, I foolishly assumed that they >> were pre-existing failures. I should have run the tests on master for >> comparison, sorry. Or at least put "RFC" in the subject instead of >> "PATCH" as you suggest. I sincerely apologize for my lack of due >> diligence and I know that I need to do better at self-reviewing >> patches before sending them. > I personally don't see any need for apologies, we all make mistakes, > just keep it in mind for the future. Yes, for someone joining the project it's not obvious what the status of the tests is. No problem. Alex: To elaborate, our exists tests should pass, and should pass on every commit (both as a matter of fact and future coding practice). We also have CI, so if you e.g. have a fork of git/git and push to your fork you'll find that CI is run for you on several platforms. See below for a one-liner to possibly speed up the testing for you. > Personally I prefer to run prove instead, because the output is less > verbose, and there's a nice summary at the end: > > prove t[0-9][0-9][0-9][0-9]-*.sh I also like "prove" better (well, I added the support for it, so ...). It's generally better to use e.g.: make test DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="--jobs $(nproc)" Since we do some basic checking via the Makefile that effectively form a part of our tests. FWIW for your one-liner it can be just: prove t[0-9]*.sh Alex: You might also find that if you specify --root as the path to a ramdisk the tests are much faster, e.g. on my Linux boxes I set --root=/run/user/`id -u`/git.
Ævar Arnfjörð Bjarmason wrote: > > On Sun, Jun 27 2021, Felipe Contreras wrote: > > Personally I prefer to run prove instead, because the output is less > > verbose, and there's a nice summary at the end: > > > > prove t[0-9][0-9][0-9][0-9]-*.sh > > I also like "prove" better (well, I added the support for it, so > ...). It's generally better to use e.g.: > > make test DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="--jobs $(nproc)" Or just add "-j$(nproc)" to ~/.proverc so you don't need to specify GIT_PROVE_OPTS every time. > Since we do some basic checking via the Makefile that effectively form a > part of our tests. > > FWIW for your one-liner it can be just: > > prove t[0-9]*.sh > > Alex: You might also find that if you specify --root as the path to a > ramdisk the tests are much faster, e.g. on my Linux boxes I set > --root=/run/user/`id -u`/git. Or TEST_OUTPUT_DIRECTORY=/tmp/git.
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt index 5c3fb67c01..0fb185811e 100644 --- a/Documentation/git-pull.txt +++ b/Documentation/git-pull.txt @@ -16,13 +16,17 @@ DESCRIPTION ----------- Incorporates changes from a remote repository into the current -branch. In its default mode, `git pull` is shorthand for -`git fetch` followed by `git merge FETCH_HEAD`. +branch. `git pull` is shorthand for `git fetch` followed by +`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`. More precisely, 'git pull' runs 'git fetch' with the given -parameters and calls 'git merge' to merge the retrieved branch -heads into the current branch. -With `--rebase`, it runs 'git rebase' instead of 'git merge'. +parameters and then, by default, attempts to fast-forward the +current branch to the remote branch head. If fast-forwarding +is not possible because the local and remote branches have +diverged, the `--rebase` option causes 'git rebase' to be +called to rebase the local branch upon the remote branch, and +the `--no-rebase` option causes 'git merge' to be called to +merge the retrieved branch heads into the current branch. <repository> should be the name of a remote repository as passed to linkgit:git-fetch[1]. <refspec> can name an diff --git a/builtin/pull.c b/builtin/pull.c index e8927fc2ff..21eaebc463 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_ return ret; } -static void show_advice_pull_non_ff(void) +static void die_pull_non_ff(void) { - advise(_("Pulling without specifying how to reconcile divergent branches is\n" - "discouraged. You can squelch this message by running one of the following\n" - "commands sometime before your next pull:\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. You can also pass --rebase, --no-rebase,\n" - "or --ff-only on the command line to override the configured default per\n" - "invocation.\n")); + die(_("Pulling without specifying how to reconcile divergent branches is not\n" + "possible. You can squelch this message by running one of the following\n" + "commands sometime before your next pull:\n" + "\n" + " git config pull.rebase false # merge\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. You can also pass --rebase, --no-rebase,\n" + "or --ff-only on the command line to override the configured default per\n" + "invocation.\n")); } int cmd_pull(int argc, const char **argv, const char *prefix) @@ -1047,10 +1047,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix) can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); - if (rebase_unspecified && !opt_ff && !can_ff) { - if (opt_verbosity >= 0) - show_advice_pull_non_ff(); - } + if (rebase_unspecified && !opt_ff && !can_ff) + die_pull_non_ff(); if (opt_rebase) { int ret = 0;
The behavior of warning but merging anyway is difficult to explain to users because it sends mixed signals. End the confusion by firmly requiring the user to specify whether they want a merge or a rebase. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- Documentation/git-pull.txt | 14 +++++++++----- builtin/pull.c | 32 +++++++++++++++----------------- 2 files changed, 24 insertions(+), 22 deletions(-)