{"thread":{"id":"56011","subject":"[PATCH] pull: abort by default if fast-forwarding is impossible","startedAt":"2021-06-27T00:11:02Z","lastAt":"2021-06-28T17:39:48Z","messageCount":10,"participants":["Alex Henrie","Elijah Newren","Felipe Contreras","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"428534","messageId":"20210627000855.530985-1-alexhenrie24@gmail.com","threadId":"56011","inReplyTo":null,"subject":"[PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-06-27T00:08:55Z","receivedAt":"2021-06-27T00:11:02Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The behavior of warning but merging anyway is difficult to explain to\nusers because it sends mixed signals. End the confusion by firmly\nrequiring the user to specify whether they want a merge or a rebase.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n Documentation/git-pull.txt | 14 +++++++++-----\n builtin/pull.c             | 32 +++++++++++++++-----------------\n 2 files changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex 5c3fb67c01..0fb185811e 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -16,13 +16,17 @@ DESCRIPTION\n -----------\n \n Incorporates changes from a remote repository into the current\n-branch.  In its default mode, `git pull` is shorthand for\n-`git fetch` followed by `git merge FETCH_HEAD`.\n+branch.  `git pull` is shorthand for `git fetch` followed by\n+`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`.\n \n More precisely, 'git pull' runs 'git fetch' with the given\n-parameters and calls 'git merge' to merge the retrieved branch\n-heads into the current branch.\n-With `--rebase`, it runs 'git rebase' instead of 'git merge'.\n+parameters and then, by default, attempts to fast-forward the\n+current branch to the remote branch head.  If fast-forwarding\n+is not possible because the local and remote branches have\n+diverged, the `--rebase` option causes 'git rebase' to be\n+called to rebase the local branch upon the remote branch, and\n+the `--no-rebase` option causes 'git merge' to be called to\n+merge the retrieved branch heads into the current branch.\n \n <repository> should be the name of a remote repository as\n passed to linkgit:git-fetch[1].  <refspec> can name an\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex e8927fc2ff..21eaebc463 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_\n \treturn ret;\n }\n \n-static void show_advice_pull_non_ff(void)\n+static void die_pull_non_ff(void)\n {\n-\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n-\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n-\t\t \"commands sometime before your next pull:\\n\"\n-\t\t \"\\n\"\n-\t\t \"  git config pull.rebase false  # merge (the default strategy)\\n\"\n-\t\t \"  git config pull.rebase true   # rebase\\n\"\n-\t\t \"  git config pull.ff only       # fast-forward only\\n\"\n-\t\t \"\\n\"\n-\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n-\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n-\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n-\t\t \"invocation.\\n\"));\n+\tdie(_(\"Pulling without specifying how to reconcile divergent branches is not\\n\"\n+\t      \"possible. You can squelch this message by running one of the following\\n\"\n+\t      \"commands sometime before your next pull:\\n\"\n+\t      \"\\n\"\n+\t      \"  git config pull.rebase false  # merge\\n\"\n+\t      \"  git config pull.rebase true   # rebase\\n\"\n+\t      \"  git config pull.ff only       # fast-forward only\\n\"\n+\t      \"\\n\"\n+\t      \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n+\t      \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n+\t      \"or --ff-only on the command line to override the configured default per\\n\"\n+\t      \"invocation.\\n\"));\n }\n \n int cmd_pull(int argc, const char **argv, const char *prefix)\n@@ -1047,10 +1047,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n \n-\tif (rebase_unspecified && !opt_ff && !can_ff) {\n-\t\tif (opt_verbosity >= 0)\n-\t\t\tshow_advice_pull_non_ff();\n-\t}\n+\tif (rebase_unspecified && !opt_ff && !can_ff)\n+\t\tdie_pull_non_ff();\n \n \tif (opt_rebase) {\n \t\tint ret = 0;\n-- \n2.32.0.95.g4a22da5c5a\n\n"},{"id":"428536","messageId":"CABPp-BGko7BP6ZMyRKwKrv0xz7FM9ehe67HSAtzgHF9eV2TD3A@mail.gmail.com","threadId":"56011","inReplyTo":"20210627000855.530985-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-27T01:34:28Z","receivedAt":"2021-06-27T01:34:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jun 26, 2021 at 5:10 PM Alex Henrie <alexhenrie24@gmail.com> wrote:\n>\n> The behavior of warning but merging anyway is difficult to explain to\n> users because it sends mixed signals. End the confusion by firmly\n> requiring the user to specify whether they want a merge or a rebase.\n\nI would use some of Junio's wording from\nhttps://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/:\n\nGiven how annoying the \"loudly warn but still go ahead\" behaviour for\ngit pull's default behavior when fast-forwarding is not possible\n(which was also displayed even when fast-forwarding was possible until\nc525de335e (\"pull: display default warning only when non-ff\",\n2020-12-12)), and how easy it is for existing users to have squelched\nthe annoyance by choosing between rebase and merge already, turn the\nwarning into an error.  As an added bonus, the previous behavior of\ngit pull was difficult to explain, and making this change helps us\nsimplify it.\n\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>  Documentation/git-pull.txt | 14 +++++++++-----\n>  builtin/pull.c             | 32 +++++++++++++++-----------------\n>  2 files changed, 24 insertions(+), 22 deletions(-)\n>\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> index 5c3fb67c01..0fb185811e 100644\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -16,13 +16,17 @@ DESCRIPTION\n>  -----------\n>\n>  Incorporates changes from a remote repository into the current\n> -branch.  In its default mode, `git pull` is shorthand for\n> -`git fetch` followed by `git merge FETCH_HEAD`.\n> +branch.  `git pull` is shorthand for `git fetch` followed by\n\nI'd replace \"shorthand for\" with \"shorthand for either\"\n\n> +`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`.\n>\n>  More precisely, 'git pull' runs 'git fetch' with the given\n> -parameters and calls 'git merge' to merge the retrieved branch\n> -heads into the current branch.\n> -With `--rebase`, it runs 'git rebase' instead of 'git merge'.\n> +parameters and then, by default, attempts to fast-forward the\n> +current branch to the remote branch head.  If fast-forwarding\n> +is not possible because the local and remote branches have\n\nGood to here.\n\n> +diverged, the `--rebase` option causes 'git rebase' to be...\n> +called to rebase the local branch upon the remote branch, and\n> +the `--no-rebase` option causes 'git merge' to be called to\n> +merge the retrieved branch heads into the current branch.\n\nThis is okay, but can we just simplify it to:\n\n....diverged, then the user must specify whether to rebase or merge\n(see the --rebase and --no-rebase flags).\n\n\n\nWhile at it, we may also want to change the documentation for the\n--no-rebase flag.  Instead of\n\n--no-rebase::\n    Override earlier --rebase.\n\nwhich can be confusing since --rebase takes so many different choices, instead:\n\n--no-rebase::\n    This is shorthand for --rebase=false\n\n\n>  <repository> should be the name of a remote repository as\n>  passed to linkgit:git-fetch[1].  <refspec> can name an\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index e8927fc2ff..21eaebc463 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_\n>         return ret;\n>  }\n>\n> -static void show_advice_pull_non_ff(void)\n> +static void die_pull_non_ff(void)\n>  {\n> -       advise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n> -                \"discouraged. You can squelch this message by running one of the following\\n\"\n> -                \"commands sometime before your next pull:\\n\"\n> -                \"\\n\"\n> -                \"  git config pull.rebase false  # merge (the default strategy)\\n\"\n> -                \"  git config pull.rebase true   # rebase\\n\"\n> -                \"  git config pull.ff only       # fast-forward only\\n\"\n> -                \"\\n\"\n> -                \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> -                \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> -                \"or --ff-only on the command line to override the configured default per\\n\"\n> -                \"invocation.\\n\"));\n> +       die(_(\"Pulling without specifying how to reconcile divergent branches is not\\n\"\n> +             \"possible. You can squelch this message by running one of the following\\n\"\n> +             \"commands sometime before your next pull:\\n\"\n> +             \"\\n\"\n> +             \"  git config pull.rebase false  # merge\\n\"\n> +             \"  git config pull.rebase true   # rebase\\n\"\n> +             \"  git config pull.ff only       # fast-forward only\\n\"\n> +             \"\\n\"\n> +             \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> +             \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> +             \"or --ff-only on the command line to override the configured default per\\n\"\n> +             \"invocation.\\n\"));\n>  }\n>\n>  int cmd_pull(int argc, const char **argv, const char *prefix)\n> @@ -1047,10 +1047,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>\n>         can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n>\n> -       if (rebase_unspecified && !opt_ff && !can_ff) {\n> -               if (opt_verbosity >= 0)\n> -                       show_advice_pull_non_ff();\n> -       }\n> +       if (rebase_unspecified && !opt_ff && !can_ff)\n> +               die_pull_non_ff();\n>\n>         if (opt_rebase) {\n>                 int ret = 0;\n> --\n> 2.32.0.95.g4a22da5c5a\n\nThe code changes look good, but you'll need to add several test\nchanges as well, or this code change will cause test failures.  You'll\nneed to go through the relevant `git pull` invocations in the\ntestsuite and add the --no-rebase flag.\n\nThanks for working on this.\n"},{"id":"428543","messageId":"60d7fbb770397_b8dfe2087d@natae.notmuch","threadId":"56011","inReplyTo":"CABPp-BGko7BP6ZMyRKwKrv0xz7FM9ehe67HSAtzgHF9eV2TD3A@mail.gmail.com","subject":"Re: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-27T04:16:55Z","receivedAt":"2021-06-27T04:17:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n\n> The code changes look good, but you'll need to add several test\n> changes as well, or this code change will cause test failures.\n\nWouldn't you consider sending a patch without running 'make test'\n\"cavalier\"?\n\n> Thanks for working on this.\n\nSuch a completely different tone for a \"cavalier\" patch depending 100%\non the person who sent it. Weird.\n\n-- \nFelipe Contreras\n"},{"id":"428544","messageId":"60d7faaf5311a_b8dfe208bd@natae.notmuch","threadId":"56011","inReplyTo":"20210627000855.530985-1-alexhenrie24@gmail.com","subject":"RE: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-27T04:12:31Z","receivedAt":"2021-06-27T04:17:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Alex Henrie wrote:\n> The behavior of warning but merging anyway is difficult to explain to\n> users because it sends mixed signals. End the confusion by firmly\n> requiring the user to specify whether they want a merge or a rebase.\n\nWhen were users warned about this change? Generally before making such\nbig UI changes to core commands there needs to be a deprecation period\nbefore pulling the trigger.\n\nSecond, supposing this was a proposal to add such warning, we would need\na configuration so the user can decide to retain the old behavior, or\nmove to the new one. What is that configuration?\n\nHow can a user choose to enable the new behavior in v2.32 (or any other\nversion)?\n\nAlso, a bunch of tests are broken after this change:\n\n  t4013-diff-various.sh\n  t5521-pull-options.sh\n  t5524-pull-msg.sh\n  t5520-pull.sh\n  t5553-set-upstream.sh\n  t5604-clone-reference.sh\n  t6409-merge-subtree.sh\n  t6402-merge-rename.sh\n  t6417-merge-ours-theirs.sh\n  t7601-merge-pull-config.sh\n  t7603-merge-reduce-heads.sh\n\nIf you didn't mean this patch to be applied then perhaps add the RFC\nprefix.\n\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -16,13 +16,17 @@ DESCRIPTION\n>  -----------\n>  \n>  Incorporates changes from a remote repository into the current\n> -branch.  In its default mode, `git pull` is shorthand for\n> -`git fetch` followed by `git merge FETCH_HEAD`.\n> +branch.  `git pull` is shorthand for `git fetch` followed by\n> +`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`.\n\nIs it?\n\nIf I have a pull.rebase=merges is `git pull` the same as doing `git\nrebase FETCH_HEAD`?\n\n>  More precisely, 'git pull' runs 'git fetch' with the given\n> -parameters and calls 'git merge' to merge the retrieved branch\n> -heads into the current branch.\n> -With `--rebase`, it runs 'git rebase' instead of 'git merge'.\n> +parameters and then, by default, attempts to fast-forward the\n> +current branch to the remote branch head.\n\nOK.\n\n> +If fast-forwarding\n> +is not possible because the local and remote branches have\n> +diverged, the `--rebase` option causes 'git rebase' to be\n> +called to rebase the local branch upon the remote branch,\n\nDoes --rebase only work when a fast-forward isn't possible.\n\n> +and\n> +the `--no-rebase` option causes 'git merge' to be called to\n> +merge the retrieved branch heads into the current branch.\n\nIsn't merge called when a fast-forward is possible?\n\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_\n>  \treturn ret;\n>  }\n>  \n> -static void show_advice_pull_non_ff(void)\n> +static void die_pull_non_ff(void)\n>  {\n> -\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n> -\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n> -\t\t \"commands sometime before your next pull:\\n\"\n> -\t\t \"\\n\"\n> -\t\t \"  git config pull.rebase false  # merge (the default strategy)\\n\"\n> -\t\t \"  git config pull.rebase true   # rebase\\n\"\n> -\t\t \"  git config pull.ff only       # fast-forward only\\n\"\n> -\t\t \"\\n\"\n> -\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> -\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> -\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n> -\t\t \"invocation.\\n\"));\n> +\tdie(_(\"Pulling without specifying how to reconcile divergent branches is not\\n\"\n> +\t      \"possible. You can squelch this message by running one of the following\\n\"\n> +\t      \"commands sometime before your next pull:\\n\"\n\nIs squelching this message really what we are aming for?\n\n> +\t      \"  git config pull.rebase false  # merge\\n\"\n> +\t      \"  git config pull.rebase true   # rebase\\n\"\n> +\t      \"  git config pull.ff only       # fast-forward only\\n\"\n\n`git config pull.ff only` will get rid of this messge, but the pull\nwould still fail, only that it will be a different message:\n\n  fatal: Not possible to fast-forward, aborting.\n\nDoesn't seem very user-friendly.\n\n> +\t      \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> +\t      \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> +\t      \"or --ff-only on the command line to override the configured default per\\n\"\n> +\t      \"invocation.\\n\"));\n\nCan I?\n\n  git -c pull.rebase=true pull --ff-only\n\n`--ff-only` doesn't seem to be overriding the configuration.\n\n\nIn addition to all the issues I've already pointed out, the message\nseems rather big for a die().\n\nI think this is close to the end-goal we should be aiming for, but\nthere's a lot that is missing before we should even consider flipping\nthe switch.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"428547","messageId":"CABPp-BGXQ4vbFf6Gx21X-Ms+1VQsZfod1waNJZQxVmB7b-gD7Q@mail.gmail.com","threadId":"56011","inReplyTo":"60d7fbb770397_b8dfe2087d@natae.notmuch","subject":"Re: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-27T07:49:45Z","receivedAt":"2021-06-27T07:50:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jun 26, 2021 at 9:16 PM Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n>\n> Elijah Newren wrote:\n>\n> > The code changes look good, but you'll need to add several test\n> > changes as well, or this code change will cause test failures.\n>\n> Wouldn't you consider sending a patch without running 'make test'\n> \"cavalier\"?\n>\n> > Thanks for working on this.\n>\n> Such a completely different tone for a \"cavalier\" patch depending 100%\n> on the person who sent it. Weird.\n\nFirst of all, attacking a random bystander like Alex is rather\nuncalled for.  As far as we know, Alex is trying to help out and made\na mistake or oversight, but will likely correct the problem if kindly\npointed out.  This email is egregiously far from constructive\ncriticism for Alex.\n\nSecond, why do you grossly mischaracterize folks (not just me) and try\nso hard to insinuate unfairness?  I don't get it.  Do you really not\nunderstand?\n\nLet me attempt to explain...\n\n\n\nNot running the tests is a problem, sure.  I pointed it out.  It could\nhave been a mistake or oversight on Alex's part.  Now, if Alex were to\nstate it was not accidental and then repeatedly defend submitting\npatches without running tests, I would absolutely start calling out\nboth his submission and his professed level of reasonable due\ndiligence.  He hasn't done that though.  People deserve a chance to\nrespond and correct things first.\n\nWe gave you that same opportunity.  It was pointed out to you rather\npolitely that you had neglected to act on a report that a patch caused\na segfault[1]; in fact, you re-submitted the old patch as-is and\nsuggested it might be a good default[2].  You had a chance to point\nout that what you had done was just a mistake or oversight.  Instead,\nyou confirmed that you saw the report and just didn't bother acting on\nit[3].  And then you defended your misbehavior[4].  Over[5] and\nover[6] and over[7] again.  And you also defended your related\npatches[8] instead of double checking or trying to correct them --\neven when prompted to double check[9] -- despite those additional\npatches having severe problems of their own[10].\n\nSo, yes, I used terms like negligent, reckless, careless, and cavalier\nin my critiques with you, but I very carefully always used those terms\nto describe either the patches you submitted or the amount of due\ndiligence that you repeatedly defended.  I never applied those terms\nto you.  And those terms didn't apply to you, as you could have chosen\nto change and use a different level of due diligence.  So, your\naccusations of personal attacks, harassment, and ad hominem arguments\nthat you made against me earlier[11], and your insinuations of\nunfairness both then and now, are all false.\n\nThis, of course, isn't your first time grossly mischaracterizing\nevents and trying to claim unfairness.  Let me give you a second\nexample of why there was no unfairness in case the first is hard for\nyou to understand.  Let's use [12] as another example, and explain the\nlead-up to that time: I politely gave you a chance to correct a\nmisleading claim[13], which you then instead admitted to being no\nmistake[14], and didn't bother to correct.  The original misleading\nclaim was a minor issue.  If it had been corrected (even if it had\nbeen intentional), it could have been no big deal.  The reason I drew\nsignificant attention to it[15] was due to your response.  I pointed\nout quite clearly when I drew attention to it, that it was your\n*response* to the error that was problematic, and even reiterated it\nin a follow-up.  I do not see why you ignored that, mischaracterized\nmy objections as being entirely about the initial error, hand picked\nan example that didn't even do what you said it did, all to lead up to\nyour \"Could it be that this has absolutely nothing to do with the\naction, and everything to do with the person doing the action?\"  The\nanswer is quite simply \"Of course not.\"  I would also call out other\npeople if they stated that misleading claims of theirs were\nintentionally submitted and did not try to correct them.  I just\nhaven't ever seen anyone else do that.  (Let alone follow it up again\na short time later quite similarly[16], except that at least you\ncorrected it.)\n\nUnfortunately, these aren't isolated incidents of claims of\nunfairness, and it's not just me you hurl these accusations against.\nIn fact, in other cases with other people you go much further\noverboard.  For example, you recently used the phrase \"petty personal\nanimus\"[17] with Junio.  Perhaps you don't see the problem, so let me\nstate it in a way you might recognize:\n     Could it be that you always use these perjorative phrases in\nquestions just to provide plausible deniability that you are directly\naccusing folks of severe personal failings?\nJust as I shouldn't ask the above question (I only did so to try to\nhighlight how your questions come across), you shouldn't be asking\nyours.  It's inappropriate.  Putting such negative words in a question\nformat might make the phrases less objectionable, but the questions\nare still way out of line.  If you react negatively to my example\nsentence above, hopefully that helps you see why that is\ninappropriate.  Please correct this pattern.\n\nAnd all of this hardly even touches on your interactions with\nreviewers, which sadly is following the same arc as in 2013 and\n2014[18].\n\nIn short, please stop:\n  * Please stop attacking random bystanders like Alex (or other folks[19])\n  * Please stop defending shockingly inadequate levels of due\ndiligence, and adopt a higher one.\n  * Please stop accusing others of bias, unfairness, and other\nshortcomings.  Learn to recognize why your behavior often results in\nothers changing theirs.\n  * And please find ways to stop burning out reviewers, especially\nsince they are the rate-limiting resource in determining git's overall\nvelocity\n\n\nPlease know that I really do think you are talented.  I tried really\nhard to help you change and improve because I thought you could have\nbeen a great addition to the community.  I'm nowhere near as talented\nas Michael in expressing it (again, [18]), but I really wish you had\nchosen to change.  I agree with his sentiments back then.\n\n\n[1] https://lore.kernel.org/git/YMYnVWSEgxvKRU9j@coredump.intra.peff.net/\n[2] https://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/\n[3] https://lore.kernel.org/git/60c647c1d9b5c_41f452089@natae.notmuch/\n[4] https://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/\n[5] https://lore.kernel.org/git/60c86ff87d598_e6332085b@natae.notmuch/\n[6] https://lore.kernel.org/git/60c85bfd112a9_e633208d5@natae.notmuch/\n[7] https://lore.kernel.org/git/60cb7f3e66cfd_1305720822@natae.notmuch/\n\n[8] https://lore.kernel.org/git/60c85bfd112a9_e633208d5@natae.notmuch/\n[9] https://lore.kernel.org/git/CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com/\n[10] https://lore.kernel.org/git/CABPp-BEXtJtkkh9Diuo4e1K1ci=vggGxkLRDfkpOH12LM8TCfA@mail.gmail.com/\n\n[11] https://lore.kernel.org/git/60cb5a02f1b31_1259c2086f@natae.notmuch/\n[12] https://lore.kernel.org/git/60cb7f3e66cfd_1305720822@natae.notmuch/\n[13] https://lore.kernel.org/git/CABPp-BGstXDbzxpySw7q_jn22HD05MsrZeHNv+kXFHOFS2_WCQ@mail.gmail.com/\n[14] https://lore.kernel.org/git/60c887f678c88_e63320846@natae.notmuch/\n[15] https://lore.kernel.org/git/CABPp-BG53Kd7MhzE3hdq5fjBQVV2Ew3skcUCAuTfM5daP2wmZA@mail.gmail.com/\n[16] https://lore.kernel.org/git/CABPp-BF1noWhiJadHzjJmnGo8hdZj6Fk7XnZ=u6BVVSGfHE7og@mail.gmail.com/\n\n[17] https://lore.kernel.org/git/60d289c84fadf_312208dc@natae.notmuch/\n\n[18] https://lore.kernel.org/git/53709788.2050201@alum.mit.edu/\n\n[19] \"if you eyes have trouble seeing\", from\nhttps://lore.kernel.org/git/60839422353fc_10cb9208c7@natae.notmuch/\n       https://lore.kernel.org/git/87fszfafkh.fsf@angela.anarc.at/\n"},{"id":"428570","messageId":"60d8ab7fe0761_ba5a2208b@natae.notmuch","threadId":"56011","inReplyTo":"CABPp-BGXQ4vbFf6Gx21X-Ms+1VQsZfod1waNJZQxVmB7b-gD7Q@mail.gmail.com","subject":"Re: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-27T16:46:55Z","receivedAt":"2021-06-27T16:47:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Sat, Jun 26, 2021 at 9:16 PM Felipe Contreras\n> <felipe.contreras@gmail.com> wrote:\n> >\n> > Elijah Newren wrote:\n> >\n> > > The code changes look good, but you'll need to add several test\n> > > changes as well, or this code change will cause test failures.\n> >\n> > Wouldn't you consider sending a patch without running 'make test'\n> > \"cavalier\"?\n> >\n> > > Thanks for working on this.\n> >\n> > Such a completely different tone for a \"cavalier\" patch depending 100%\n> > on the person who sent it. Weird.\n> \n> First of all, attacking a random bystander like Alex is rather\n> uncalled for.\n\nI did not attack Alex, I simply pointed out the fact that his patch\nbreaks the test suite, which you did too.\n\nI asked *you* if you didn't consider this \"cavalier\". Your reaction\nseems inconsistent. This question has nothing to do with Alex.\n\nI don't throw personal attacks, nor do I chastise contributors for\nattempting to improve the project. That's your department.\n\nI focus on the ball, not the player.\n\n-- \nFelipe Contreras\n"},{"id":"428574","messageId":"CAMMLpeQaY0z_baVQspJhAX0wanaJwneFoM5rG=xwFUmLEE-QaQ@mail.gmail.com","threadId":"56011","inReplyTo":"60d8ab7fe0761_ba5a2208b@natae.notmuch","subject":"Re: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-06-27T19:54:16Z","receivedAt":"2021-06-27T19:54:37Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Sat, Jun 26, 2021 at 7:34 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> Thanks for working on this.\n\nThanks for all the feedback. I will incorporate your suggestions about\nthe commit message and documentation into the next revision, whenever\nthat is.\n\nOn Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n>\n> Also, a bunch of tests are broken after this change:\n>\n>   t4013-diff-various.sh\n>   t5521-pull-options.sh\n>   t5524-pull-msg.sh\n>   t5520-pull.sh\n>   t5553-set-upstream.sh\n>   t5604-clone-reference.sh\n>   t6409-merge-subtree.sh\n>   t6402-merge-rename.sh\n>   t6417-merge-ours-theirs.sh\n>   t7601-merge-pull-config.sh\n>   t7603-merge-reduce-heads.sh\n>\n> If you didn't mean this patch to be applied then perhaps add the RFC\n> prefix.\n\nI actually did run `make test` before sending the patch, but when so\nmany seemingly unrelated tests failed, I foolishly assumed that they\nwere pre-existing failures. I should have run the tests on master for\ncomparison, sorry. Or at least put \"RFC\" in the subject instead of\n\"PATCH\" as you suggest. I sincerely apologize for my lack of due\ndiligence and I know that I need to do better at self-reviewing\npatches before sending them.\n\n> Alex Henrie wrote:\n>\n> > +           \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> > +           \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> > +           \"or --ff-only on the command line to override the configured default per\\n\"\n> > +           \"invocation.\\n\"));\n>\n> Can I?\n>\n>   git -c pull.rebase=true pull --ff-only\n>\n> `--ff-only` doesn't seem to be overriding the configuration.\n\nAhh, so /that's/ what you meant by \"3. Fix all the wrong behavior with\n--ff, --no-ff, and -ff-only\". That does seem like something worth\ntrying to fix before making the switch.\n\nOn Sat, Jun 26, 2021 at 10:16 PM Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n>\n> Elijah Newren wrote:\n>\n> > The code changes look good, but you'll need to add several test\n> > changes as well, or this code change will cause test failures.\n>\n> Wouldn't you consider sending a patch without running 'make test'\n> \"cavalier\"?\n>\n> > Thanks for working on this.\n>\n> Such a completely different tone for a \"cavalier\" patch depending 100%\n> on the person who sent it. Weird.\n\nI'm trying to make things better, as I'm sure you are as well, and I\nknow that I make a lot of mistakes and need the help of more\nexperienced contributors like you. So please be nice, even if others\nare mean to you, because regardless of whether these comments were\ndirected at me, this kind of comment just makes me feel like giving\nup.\n\nOn Sun, Jun 27, 2021 at 10:46 AM Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n>\n> I don't throw personal attacks, nor do I chastise contributors for\n> attempting to improve the project. That's your department.\n\n\"That's your department\" is a personal attack. How about we all go\nspend some time enjoying the weather, and then get back to working on\nGit problems later this week.\n\n-Alex\n"},{"id":"428576","messageId":"60d8edbb3803f_70e20886@natae.notmuch","threadId":"56011","inReplyTo":"CAMMLpeQaY0z_baVQspJhAX0wanaJwneFoM5rG=xwFUmLEE-QaQ@mail.gmail.com","subject":"Re: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-27T21:29:31Z","receivedAt":"2021-06-27T21:29:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Alex Henrie wrote:\n\n> On Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras\n> <felipe.contreras@gmail.com> wrote:\n> >\n> > Also, a bunch of tests are broken after this change:\n> >\n> >   t4013-diff-various.sh\n> >   t5521-pull-options.sh\n> >   t5524-pull-msg.sh\n> >   t5520-pull.sh\n> >   t5553-set-upstream.sh\n> >   t5604-clone-reference.sh\n> >   t6409-merge-subtree.sh\n> >   t6402-merge-rename.sh\n> >   t6417-merge-ours-theirs.sh\n> >   t7601-merge-pull-config.sh\n> >   t7603-merge-reduce-heads.sh\n> >\n> > If you didn't mean this patch to be applied then perhaps add the RFC\n> > prefix.\n> \n> I actually did run `make test` before sending the patch, but when so\n> many seemingly unrelated tests failed, I foolishly assumed that they\n> were pre-existing failures. I should have run the tests on master for\n> comparison, sorry. Or at least put \"RFC\" in the subject instead of\n> \"PATCH\" as you suggest. I sincerely apologize for my lack of due\n> diligence and I know that I need to do better at self-reviewing\n> patches before sending them.\n\nI personally don't see any need for apologies, we all make mistakes,\njust keep it in mind for the future.\n\nPersonally I prefer to run prove instead, because the output is less\nverbose, and there's a nice summary at the end:\n\n  prove t[0-9][0-9][0-9][0-9]-*.sh\n\n> > Alex Henrie wrote:\n> >\n> > > +           \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> > > +           \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> > > +           \"or --ff-only on the command line to override the configured default per\\n\"\n> > > +           \"invocation.\\n\"));\n> >\n> > Can I?\n> >\n> >   git -c pull.rebase=true pull --ff-only\n> >\n> > `--ff-only` doesn't seem to be overriding the configuration.\n> \n> Ahh, so /that's/ what you meant by \"3. Fix all the wrong behavior with\n> --ff, --no-ff, and -ff-only\". That does seem like something worth\n> trying to fix before making the switch.\n\nThat is just one of the inconsistencies, there's many others.\n\nConsider all these:\n\n\tgit pull\n\tgit pull --ff-only\n\tgit -c pull.ff=only pull\n\tgit -c pull.ff=only pull --ff-only\n\nDo you see any good reason why the error message should be different for\nany of these?\n\nHow about these?\n\n\tgit pull\n\tgit pull --ff\n\nWhy does --ff make the second command work?\n\nEven worse when you start considering more combinations, like\n\"--no-ff --rebase\" which is obviously nonsense, but it depends on the\nconfiguration.\n\nI proposed a solution for some of these inconsistencies with --ff*, but\nwas ignored [1].\n\n> On Sat, Jun 26, 2021 at 10:16 PM Felipe Contreras\n> <felipe.contreras@gmail.com> wrote:\n> >\n> > Elijah Newren wrote:\n> >\n> > > The code changes look good, but you'll need to add several test\n> > > changes as well, or this code change will cause test failures.\n> >\n> > Wouldn't you consider sending a patch without running 'make test'\n> > \"cavalier\"?\n> >\n> > > Thanks for working on this.\n> >\n> > Such a completely different tone for a \"cavalier\" patch depending 100%\n> > on the person who sent it. Weird.\n> \n> I'm trying to make things better, as I'm sure you are as well, and I\n> know that I make a lot of mistakes and need the help of more\n> experienced contributors like you. So please be nice, even if others\n> are mean to you, because regardless of whether these comments were\n> directed at me, this kind of comment just makes me feel like giving\n> up.\n\nMy mail was directed towards Elijah, not you.\n\nI do appreciate all contributions, especially if they were done pro\nbono.\n\n> On Sun, Jun 27, 2021 at 10:46 AM Felipe Contreras\n> <felipe.contreras@gmail.com> wrote:\n> >\n> > I don't throw personal attacks, nor do I chastise contributors for\n> > attempting to improve the project. That's your department.\n> \n> \"That's your department\" is a personal attack. How about we all go\n> spend some time enjoying the weather, and then get back to working on\n> Git problems later this week.\n\nStating facts about what a person did is not a personal attack, it's\njust stating facts. Elijah did attacked me personally, that's a fact [2]:\n\n  If I were in charge, at this point I would drop all of Felipe's\n  patches on the floor (not just the ones from these threads), and not\n  accept any further ones.  I am not in charge, though, and you have\n  more patience than me.  But I will not be reviewing or responding to\n  Felipe's patches further.  Please do not trust future submissions from\n  him that have an \"Acked-by\" or \"Reviewed-by\" from me, including\n  resubmissions of past patches.\n\nAnd he did so after I made a \"mistake\" (in his view) that was much much\nless serious than the one you did.\n\nI am merely pointing out the inconsistency, that's all.\n\nThis has absolutely nothing to do with you, again... I appreciate you\nused some of your free time to try to improve git pull.\n\nWhatever beef Elijah has with me is an entirely different subject, that\nI suggest you leave on the table. I don't think anything constructive\ncan come from trying to defend what he did.\n\nLet's just focus on the patches.\n\nCheers.\n\n[1] https://lore.kernel.org/git/5fdd154264baf_130e182082b@natae.notmuch/\n[2] https://lore.kernel.org/git/CABPp-BG53Kd7MhzE3hdq5fjBQVV2Ew3skcUCAuTfM5daP2wmZA@mail.gmail.com/\n\n-- \nFelipe Contreras\n"},{"id":"428602","messageId":"87k0megtlo.fsf@evledraar.gmail.com","threadId":"56011","inReplyTo":"60d8edbb3803f_70e20886@natae.notmuch","subject":"Re: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-28T10:31:52Z","receivedAt":"2021-06-28T10:40:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jun 27 2021, Felipe Contreras wrote:\n\n> Alex Henrie wrote:\n>\n>> On Sat, Jun 26, 2021 at 10:12 PM Felipe Contreras\n>> <felipe.contreras@gmail.com> wrote:\n>> >\n>> > Also, a bunch of tests are broken after this change:\n>> >\n>> >   t4013-diff-various.sh\n>> >   t5521-pull-options.sh\n>> >   t5524-pull-msg.sh\n>> >   t5520-pull.sh\n>> >   t5553-set-upstream.sh\n>> >   t5604-clone-reference.sh\n>> >   t6409-merge-subtree.sh\n>> >   t6402-merge-rename.sh\n>> >   t6417-merge-ours-theirs.sh\n>> >   t7601-merge-pull-config.sh\n>> >   t7603-merge-reduce-heads.sh\n>> >\n>> > If you didn't mean this patch to be applied then perhaps add the RFC\n>> > prefix.\n>> \n>> I actually did run `make test` before sending the patch, but when so\n>> many seemingly unrelated tests failed, I foolishly assumed that they\n>> were pre-existing failures. I should have run the tests on master for\n>> comparison, sorry. Or at least put \"RFC\" in the subject instead of\n>> \"PATCH\" as you suggest. I sincerely apologize for my lack of due\n>> diligence and I know that I need to do better at self-reviewing\n>> patches before sending them.\n> I personally don't see any need for apologies, we all make mistakes,\n> just keep it in mind for the future.\n\nYes, for someone joining the project it's not obvious what the status of\nthe tests is. No problem.\n\nAlex: To elaborate, our exists tests should pass, and should pass on\nevery commit (both as a matter of fact and future coding practice). We\nalso have CI, so if you e.g. have a fork of git/git and push to your\nfork you'll find that CI is run for you on several platforms.\n\nSee below for a one-liner to possibly speed up the testing for you.\n\n> Personally I prefer to run prove instead, because the output is less\n> verbose, and there's a nice summary at the end:\n>\n>   prove t[0-9][0-9][0-9][0-9]-*.sh\n\nI also like \"prove\" better (well, I added the support for it, so\n...). It's generally better to use e.g.:\n\n    make test DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\"--jobs $(nproc)\"\n\nSince we do some basic checking via the Makefile that effectively form a\npart of our tests.\n\nFWIW for your one-liner it can be just:\n\n    prove t[0-9]*.sh\n\nAlex: You might also find that if you specify --root as the path to a\nramdisk the tests are much faster, e.g. on my Linux boxes I set\n--root=/run/user/`id -u`/git.\n"},{"id":"428617","messageId":"60da095a5ce4_1a78f20847@natae.notmuch","threadId":"56011","inReplyTo":"87k0megtlo.fsf@evledraar.gmail.com","subject":"Re: [PATCH] pull: abort by default if fast-forwarding is impossible","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-28T17:39:38Z","receivedAt":"2021-06-28T17:39:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sun, Jun 27 2021, Felipe Contreras wrote:\n\n> > Personally I prefer to run prove instead, because the output is less\n> > verbose, and there's a nice summary at the end:\n> >\n> >   prove t[0-9][0-9][0-9][0-9]-*.sh\n> \n> I also like \"prove\" better (well, I added the support for it, so\n> ...). It's generally better to use e.g.:\n> \n>     make test DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\"--jobs $(nproc)\"\n\nOr just add \"-j$(nproc)\" to ~/.proverc so you don't need to specify\nGIT_PROVE_OPTS every time.\n\n> Since we do some basic checking via the Makefile that effectively form a\n> part of our tests.\n> \n> FWIW for your one-liner it can be just:\n> \n>     prove t[0-9]*.sh\n> \n> Alex: You might also find that if you specify --root as the path to a\n> ramdisk the tests are much faster, e.g. on my Linux boxes I set\n> --root=/run/user/`id -u`/git.\n\nOr TEST_OUTPUT_DIRECTORY=/tmp/git.\n\n-- \nFelipe Contreras"}]}