{"thread":{"id":"42959","subject":"Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","startedAt":"2016-07-29T09:17:22Z","lastAt":"2016-09-09T23:31:07Z","messageCount":18,"participants":["Dakota Hawkins","Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"292504","messageId":"CAG0BQXnVAYdpk9EM_uiD+=UKSKmK=z1YEar5MresTr5XfDCxHw@mail.gmail.com","threadId":"42959","inReplyTo":null,"subject":"Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Dakota Hawkins","fromEmail":"dakotahawkins@gmail.com","sentAt":"2016-07-29T09:17:15Z","receivedAt":"2016-07-29T09:17:22Z","isPatch":false,"sender":{"key":"dakotahawkins@gmail.com","avatar":"https://gravatar.com/avatar/6283c977565cfcecb284e62d5713a91a6a8df0a3b06f27439edaab4220ae143d?d=mp&s=160"},"body":"Hello,\n\nI have a question which may be a bug (I'm a bit skeptical), but here goes:\n\nIn my global .gitconfig, I have \"user.useConfigOnly = true\" and\nuser.email isn't set there (I prefer to be forced to set it on a\nper-repo basis, as I use different emails for work and personal\nrepos). I ALSO have \"pull.rebase = preserve\" set.\n\nAn example of the problem I have is with tools like golang (I filed an\nissue there, they closed it and suggested the problem is with git or\nmy config: https://github.com/golang/go/issues/16516#issuecomment-235800085)\nthat use git to pull in package repos without any real user\ninteraction. When something like that runs a git pull for me (to\nupdate a package repo) my global config makes it try to rebase, which\nfails because git doesn't know who I am.\n\nWith golang, at least, there's no help setting-up all of its cloned\nrepos with my user information in local configs, it's a manual step\nand I have to repeat it anytime anything decided to clone a new repo\nto use as a package, so it's at least persistently frustrating. Hence\nthe golang bug, which I think is still a bug because they should\nhandle this better.\n\nIn those cases specifically, I never have local commits that differ\nfrom the remote, so a \"pull --ff-only\" should leave me in the same\nstate as a \"pull --rebase\".\n\nIs this a case of rebase trying to make sure it has enough information\nfor me to be a committer before knowing whether I even need to rewrite\nany commits, and could/should that be avoided? Alternatively (or also)\ncould/should rebase detect that a fast-forward is possible and prefer\nto do that instead?\n\nI would not be surprised to learn that there's something I don't know\nor haven't considered that explains why those things aren't really\ndo-able, but if that's the case I'd be interested to know what they\nare (I'd love to go back to the golang people and tell them to re-open\nmy bug because there's nothing wrong with git).\n\nThanks for your time, and please let me know if you'd like any\nadditional information.\n\n- Dakota Hawkins\n"},{"id":"292539","messageId":"xmqqoa5gnxow.fsf@gitster.mtv.corp.google.com","threadId":"42959","inReplyTo":"CAG0BQXnVAYdpk9EM_uiD+=UKSKmK=z1YEar5MresTr5XfDCxHw@mail.gmail.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-29T17:47:27Z","receivedAt":"2016-07-29T17:47:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dakota Hawkins writes:\n\n> I have a question which may be a bug (I'm a bit skeptical), but here goes:\n>\n> In my global .gitconfig, I have \"user.useConfigOnly = true\" and\n> user.email isn't set there (I prefer to be forced to set it on a\n> per-repo basis, as I use different emails for work and personal\n> repos). I ALSO have \"pull.rebase = preserve\" set.\n>\n> An example of the problem I have is with tools like golang (I filed an\n> issue there, they closed it and suggested the problem is with git or\n> my config: https://github.com/golang/go/issues/16516#issuecomment-235800085)\n> that use git to pull in package repos without any real user\n> interaction. When something like that runs a git pull for me (to\n> update a package repo) my global config makes it try to rebase, which\n> fails because git doesn't know who I am.\n\nIt's an interesting chicken-and-egg problem that user.useConfigOnly\nintroduces. It seems that the design of that configuration variable\nis not perfect and has room for improvement.\n\n> In those cases specifically, I never have local commits that differ\n> from the remote, so a \"pull --ff-only\" should leave me in the same\n> state as a \"pull --rebase\".\n>\n> Is this a case of rebase trying to make sure it has enough information\n> for me to be a committer before knowing whether I even need to rewrite\n> any commits, and could/should that be avoided? Alternatively (or also)\n> could/should rebase detect that a fast-forward is possible and prefer\n> to do that instead?\n\nI think that is a reasonable argument, but to solve this for a more\ngeneral case, shouldn't we be discussing a solution that would also\nwork when rebase _does_ need to create a new commit? And when the\nlatter is solved, I would imagine that \"this rebase happens to be\nfast-forward, and not having an ident shouldn't be an issue for this\nspecial case\" would become moot.\n"},{"id":"292543","messageId":"20160729181121.GB14953@sigill.intra.peff.net","threadId":"42959","inReplyTo":"xmqqoa5gnxow.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-29T18:11:22Z","receivedAt":"2016-07-29T18:11:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2016 at 10:47:27AM -0700, Junio C Hamano wrote:\n\n> > In those cases specifically, I never have local commits that differ\n> > from the remote, so a \"pull --ff-only\" should leave me in the same\n> > state as a \"pull --rebase\".\n> >\n> > Is this a case of rebase trying to make sure it has enough information\n> > for me to be a committer before knowing whether I even need to rewrite\n> > any commits, and could/should that be avoided? Alternatively (or also)\n> > could/should rebase detect that a fast-forward is possible and prefer\n> > to do that instead?\n> \n> I think that is a reasonable argument, but to solve this for a more\n> general case, shouldn't we be discussing a solution that would also\n> work when rebase _does_ need to create a new commit? And when the\n> latter is solved, I would imagine that \"this rebase happens to be\n> fast-forward, and not having an ident shouldn't be an issue for this\n> special case\" would become moot.\n\nWouldn't it be wrong to create a commit with non-config ident when\nuser.useConfigOnly is set, though? That is the exact point when it\nshould kick in, to tell the user \"you thought it would not matter here,\nbut in this case we _do_ need your real ident; what should we do?\"\n\nIf the user is doing a one-off thing where they do not care if their\ncrappy, fake ident makes it into a commit object, then the right thing\nis:\n\n git -c user.useConfigOnly=false pull --rebase\n\nor even:\n\n git -c user.email=fake-but-ok@example.com pull --rebase\n\nAnd they can do that preemptively for commands like the golang example\nhere. They shouldn't _have_ to do that, though, if the command wouldn't\nactually create a commit. So I do think there may be a bug to be fixed,\nbut it is simply commands being over-eager to make sure we have an\nident when they might not need it.\n\n-Peff\n"},{"id":"292546","messageId":"xmqqa8h0nw5a.fsf@gitster.mtv.corp.google.com","threadId":"42959","inReplyTo":"xmqqoa5gnxow.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-29T18:20:49Z","receivedAt":"2016-07-29T18:20:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Dakota Hawkins writes:\n>\n>> In those cases specifically, I never have local commits that differ\n>> from the remote, so a \"pull --ff-only\" should leave me in the same\n>> state as a \"pull --rebase\".\n>>\n>> Is this a case of rebase trying to make sure it has enough information\n>> for me to be a committer before knowing whether I even need to rewrite\n>> any commits, and could/should that be avoided? Alternatively (or also)\n>> could/should rebase detect that a fast-forward is possible and prefer\n>> to do that instead?\n>\n> I think that is a reasonable argument,...\n\nThere is one that still wants to know who you are, I think. The\nreflog entries record who moved the tip of the ref and when, and\nobviously a fast-forward is also recorded.\n\nI _think_ our intention was to allow a bogus ident in reflog entries\n(even though we want to avoid a bogus ident in commits and tags), so\nperhaps additional code/logic for user.useConfigOnly may need to know\nabout that (I didn't dig)?\n\n"},{"id":"292550","messageId":"20160729183143.GB9877@sigill.intra.peff.net","threadId":"42959","inReplyTo":"xmqqa8h0nw5a.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-29T18:31:43Z","receivedAt":"2016-07-29T18:31:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2016 at 11:20:49AM -0700, Junio C Hamano wrote:\n\n> There is one that still wants to know who you are, I think. The\n> reflog entries record who moved the tip of the ref and when, and\n> obviously a fast-forward is also recorded.\n> \n> I _think_ our intention was to allow a bogus ident in reflog entries\n> (even though we want to avoid a bogus ident in commits and tags), so\n> perhaps additional code/logic for user.useConfigOnly may need to know\n> about that (I didn't dig)?\n\nI think we handle this case OK, or you would not even be able to \"git\nfetch\" into a repository.\n\nIt works because the check is predicated on the \"strict\" flag, and the\nreflog writer passes IDENT_NO_STRICT.\n\n-Peff\n"},{"id":"292551","messageId":"xmqq60ronvlj.fsf@gitster.mtv.corp.google.com","threadId":"42959","inReplyTo":"20160729181121.GB14953@sigill.intra.peff.net","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-29T18:32:40Z","receivedAt":"2016-07-29T18:32:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > Is this a case of rebase trying to make sure it has enough information\n>> > for me to be a committer before knowing whether I even need to rewrite\n>> > any commits, and could/should that be avoided? Alternatively (or also)\n>> > could/should rebase detect that a fast-forward is possible and prefer\n>> > to do that instead?\n>> \n>> I think that is a reasonable argument, but to solve this for a more\n>> general case, shouldn't we be discussing a solution that would also\n>> work when rebase _does_ need to create a new commit? And when the\n>> latter is solved, I would imagine that \"this rebase happens to be\n>> fast-forward, and not having an ident shouldn't be an issue for this\n>> special case\" would become moot.\n>\n> Wouldn't it be wrong to create a commit with non-config ident when\n> user.useConfigOnly is set, though?\n\nThat is exactly what I was getting at.\n\n> If the user is doing a one-off thing where they do not care if their\n> crappy, fake ident makes it into a commit object, then the right thing\n> is:\n>\n> git -c user.useConfigOnly=false pull --rebase\n>\n> or even:\n>\n> git -c user.email=fake-but-ok@example.com pull --rebase\n\nHmm, I somehow had an impression that these git commands are not\nwhat the end-user runs from the command line, but wrapper tools like\n\"go get\" has a hardcoded invocation of \"git pull\".\n\nIf a user sets useconfigonly globally, each repository must have\nident the user wants to use in it configured, so I would think that\na solution should be something that makes it easy to do so.\n\n"},{"id":"292553","messageId":"xmqq1t2cnvco.fsf@gitster.mtv.corp.google.com","threadId":"42959","inReplyTo":"20160729181121.GB14953@sigill.intra.peff.net","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-29T18:37:59Z","receivedAt":"2016-07-29T18:38:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... So I do think there may be a bug to be fixed,\n> but it is simply commands being over-eager to make sure we have an\n> ident when they might not need it.\n\n36267854 (pull: fast-forward \"pull --rebase=true\", 2016-06-29) may\nbe a part of a good solution for that, perhaps?\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Wed, 29 Jun 2016 10:22:31 -0700\nSubject: [PATCH] pull: fast-forward \"pull --rebase=true\"\n\nSigned-off-by: Junio C Hamano \n---\n builtin/pull.c | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex bf3fd3f..2a41d41 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (merge_heads.nr > 1)\n \t\t\tdie(_(\"Cannot merge multiple branches into empty head.\"));\n \t\treturn pull_into_void(*merge_heads.sha1, curr_head);\n-\t} else if (opt_rebase) {\n-\t\tif (merge_heads.nr > 1)\n-\t\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n+\t}\n+\tif (opt_rebase && merge_heads.nr > 1)\n+\t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n+\n+\tif (opt_rebase) {\n+\t\tstruct commit_list *list = NULL;\n+\t\tstruct commit *merge_head, *head;\n+\n+\t\thead = lookup_commit_reference(orig_head);\n+\t\tcommit_list_insert(head, &list);\n+\t\tmerge_head = lookup_commit_reference(merge_heads.sha1[0]);\n+\t\tif (is_descendant_of(merge_head, list)) {\n+\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\topt_ff = \"--ff-only\";\n+\t\t\treturn run_merge();\n+\t\t}\n \t\treturn run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);\n-\t} else\n+\t} else {\n \t\treturn run_merge();\n+\t}\n }\n-- \n2.9.2-685-g483c9ea\n\n"},{"id":"292554","messageId":"20160729183911.GA10268@sigill.intra.peff.net","threadId":"42959","inReplyTo":"xmqq60ronvlj.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-29T18:39:12Z","receivedAt":"2016-07-29T18:39:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2016 at 11:32:40AM -0700, Junio C Hamano wrote:\n\n> > Wouldn't it be wrong to create a commit with non-config ident when\n> > user.useConfigOnly is set, though?\n> \n> That is exactly what I was getting at.\n\nAh, OK, I thought you were trying to explore the opposite direction.\n\n> > If the user is doing a one-off thing where they do not care if their\n> > crappy, fake ident makes it into a commit object, then the right thing\n> > is:\n> >\n> > git -c user.useConfigOnly=false pull --rebase\n> >\n> > or even:\n> >\n> > git -c user.email=fake-but-ok@example.com pull --rebase\n> \n> Hmm, I somehow had an impression that these git commands are not\n> what the end-user runs from the command line, but wrapper tools like\n> \"go get\" has a hardcoded invocation of \"git pull\".\n\nYeah, the right person or entity to set those options is the one who\nknows \"the operation I am doing is OK even with bogus ident\". I had\nassumed if \"go get\" fell under that category, that it should be the one\nto tell it to git (via the config above).\n\nBut I am not really sure that is the case. In general \"go get\" shouldn't\nmake commits if you aren't doing active work on the repo (AFAIK), and it\nshould just work.\n\nFrom my limited testing, \"git pull --rebase\" is perfectly fine. The\nculprit is \"--rebase=preverse\", which complains even if it would be a\nfast-forward.\n\n-Peff\n"},{"id":"292555","messageId":"20160729185211.GA5591@sigill.intra.peff.net","threadId":"42959","inReplyTo":"20160729183911.GA10268@sigill.intra.peff.net","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-29T18:52:11Z","receivedAt":"2016-07-29T18:52:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2016 at 02:39:11PM -0400, Jeff King wrote:\n\n> From my limited testing, \"git pull --rebase\" is perfectly fine. The\n> culprit is \"--rebase=preverse\", which complains even if it would be a\n> fast-forward.\n\nThat should be preserve, of course. :)\n\nAnd I think I see what is happening. \"preserve\" implies\ninteractive-rebase, which makes an early check that we have valid\ncommitter info, even though we might not actually write any new commits.\n\nSo doing this:\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex ded4595..f0f4777 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -1180,9 +1180,6 @@ To continue rebase after editing, run:\n \t;;\n esac\n \n-git var GIT_COMMITTER_IDENT >/dev/null ||\n-\tdie \"$(gettext \"You need to set your committer info first\")\"\n-\n comment_for_reflog start\n \n if test ! -z \"$switch_to\"\n\nfixes it for me. I can't figure out if that would have any bad side\neffects, though. That check comes from Dscho's original 1b1dce4 (Teach\nrebase an interactive mode, 2007-06-25), so there's not much comment on\nwhy it was added specifically.\n\nWe would notice the bogus ident later when we actually do try to create\na commit object, but I can guess that this up-front check might give us\na better error message. You get warned up-front, rather than something\nlike:\n\n Rebasing (1/1)\n *** Please tell me who you are.\n [...]\n fatal: no name was given and auto-detection is disabled\n Could not pick 8ebea123853128ca2411b2b449f76a1a4b0d026c\n\nand dumped in the middle of an interactive rebase that you cannot\ncomplete. OTOH, that is how a regular non-interactive merge works. And\nif your next step is to set up your ident, then it's natural to do:\n\n git config user.email whatever\n git rebase --continue\n\nSo I'd lean towards dropping it, but maybe there are other hidden\ngotchas.\n\n-Peff\n"},{"id":"292567","messageId":"20160729223134.GA22591@sigill.intra.peff.net","threadId":"42959","inReplyTo":"xmqq1t2cnvco.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-29T22:31:35Z","receivedAt":"2016-07-29T22:31:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2016 at 11:37:59AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... So I do think there may be a bug to be fixed,\n> > but it is simply commands being over-eager to make sure we have an\n> > ident when they might not need it.\n> \n> 36267854 (pull: fast-forward \"pull --rebase=true\", 2016-06-29) may\n> be a part of a good solution for that, perhaps?\n\nHmm. So the solution I came up with is below, and I think is the \"most\ncorrect\" in the sense that it leaves the check to the low-level code\nwhich actually creates commits, and so we know that we will absolutely\nonly ever complain when we actually need an ident.\n\nBut I could also see an argument along the lines of: if you do not have\nan ident set up, you don't really have any business running \"git rebase\n-i\" in the first place, and it is OK for us to complain even there's a\nchance your rebase might be a noop. So we should prefer consistency over\nabsolute flexibility in rebase, and just fix the common case of invoking\n\"rebase\" at all when we know it is a fast-forward.\n\nWhich is what your patch is doing. I can buy that argument in general\nfor rebase, though it is a little funny that \"git pull --rebase\" would\nthen behave differently than \"git fetch && git rebase\".\n\nThere's a middle ground, too, which goes something like: it is OK to\ninvoke \"rebase\" (interactive or not) without a valid ident if you have\nno commits to rebase. But as soon as we know there are commits to pick,\nwe should do the ident check and bail, canceling the rebase entirely.\n\nThat seems to be what happens in the non-interactive case (we call \"git\nam --rebasing\" and it bails immediately without leaving any state).\nImplementing that would mean shifting rebase--interactive's ident check\nto the right spot (after seeing the insn sheet has entries) and bailing\nappropriately.\n\nTBH, I'm not sure anybody cares that much between the three. Even before\nuser.useConfigOnly, this could be an issue on machines where the ident\ncould not be auto-configured, and it seems like nobody ran across it.\nIt's only the funny interaction with pull.rebase that makes it likely to\ncome up, so as long as that code path is fixed (one way or another), I\ndoubt anybody would bring it up again.\n\nAnyway, here's my patch.\n\n-- >8 --\nSubject: rebase-interactive: drop early check for valid ident\n\nSince the very inception of interactive-rebase in 1b1dce4\n(Teach rebase an interactive mode, 2007-06-25), there has\nbeen a preemptive check, before looking at any commits, to\nsee whether the user has a valid name/email combination.\n\nThis is convenient, because it means that we abort the\noperation before even beginning (rather than just\ncomplaining that we are unable to pick a particular commit).\n\nHowever, it does the wrong thing when the rebase does not\nactually need to generate any new commits (e.g., a\nfast-forward with no commits to pick, or one where the base\nstays the same, and we just pick the same commits without\nrewriting anything). In this case it may complain about the\nlack of ident, even though one would not be needed to\ncomplete the operation.\n\nThis may seem like mere nit-picking, but because interactive\nrebase underlies the \"preserve-merges\" rebase, somebody who\nhas set \"pull.rebase\" to \"preserve\" cannot make even a\nfast-forward pull without a valid ident, as we bail before\neven realizing the fast-forward nature.\n\nThis commit drops the extra ident check entirely. This means\nwe rely on individual commands that generate commit objects\nto complain. So we will continue to notice and prevent cases\nthat actually do create commits, but with one important\ndifference: we fail while actually executing the \"pick\"\noperations, and leave the rebase in a conflicted, half-done\nstate.\n\nIn some ways this is less convenient, but in some ways it is\nmore so; the user can then manually commit or even \"git\nrebase --continue\" after setting up their ident (or\nproviding it as a one-off on the command line).\n\nReported-by: Dakota Hawkins \nSigned-off-by: Jeff King \n---\n git-rebase--interactive.sh | 3 ---\n t/t7517-per-repo-email.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex ded4595..f0f4777 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -1180,9 +1180,6 @@ To continue rebase after editing, run:\n \t;;\n esac\n \n-git var GIT_COMMITTER_IDENT >/dev/null ||\n-\tdie \"$(gettext \"You need to set your committer info first\")\"\n-\n comment_for_reflog start\n \n if test ! -z \"$switch_to\"\ndiff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh\nindex 337e6e3..2a22fa7 100755\n--- a/t/t7517-per-repo-email.sh\n+++ b/t/t7517-per-repo-email.sh\n@@ -36,4 +36,51 @@ test_expect_success 'succeeds cloning if global email is not set' '\n \tgit clone . clone\n '\n \n+test_expect_success 'set up rebase scenarios' '\n+\t# temporarily enable an actual ident for this setup\n+\ttest_config user.email foo@example.com &&\n+\ttest_commit new &&\n+\tgit branch side-without-commit HEAD^ &&\n+\tgit checkout -b side-with-commit HEAD^ &&\n+\ttest_commit side\n+'\n+\n+test_expect_success 'fast-forward rebase does not care about ident' '\n+\tgit checkout -B tmp side-without-commit &&\n+\tgit rebase master\n+'\n+\n+test_expect_success 'non-fast-forward rebase refuses to write commits' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -B tmp side-with-commit &&\n+\ttest_must_fail git rebase master\n+'\n+\n+test_expect_success 'fast-forward rebase does not care about ident (interactive)' '\n+\tgit checkout -B tmp side-without-commit &&\n+\tgit rebase -i master\n+'\n+\n+test_expect_success 'non-fast-forward rebase refuses to write commits (interactive)' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -B tmp side-with-commit &&\n+\ttest_must_fail git rebase -i master\n+'\n+\n+test_expect_success 'noop interactive rebase does not care about ident' '\n+\tgit checkout -B tmp side-with-commit &&\n+\tgit rebase -i HEAD^\n+'\n+\n+test_expect_success 'fast-forward rebase does not care about ident (preserve)' '\n+\tgit checkout -B tmp side-without-commit &&\n+\tgit rebase -p master\n+'\n+\n+test_expect_success 'non-fast-forward rebase refuses to write commits (preserve)' '\n+\ttest_when_finished \"git rebase --abort || true\" &&\n+\tgit checkout -B tmp side-with-commit &&\n+\ttest_must_fail git rebase -p master\n+'\n+\n test_done\n-- \n2.9.2.666.g67a7da4\n\n"},{"id":"292568","messageId":"xmqqwpk4kqr4.fsf@gitster.mtv.corp.google.com","threadId":"42959","inReplyTo":"20160729223134.GA22591@sigill.intra.peff.net","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-29T22:45:35Z","receivedAt":"2016-07-29T22:45:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> TBH, I'm not sure anybody cares that much between the three. Even before\n> user.useConfigOnly, this could be an issue on machines where the ident\n> could not be auto-configured, and it seems like nobody ran across it.\n> It's only the funny interaction with pull.rebase that makes it likely to\n> come up, so as long as that code path is fixed (one way or another), I\n> doubt anybody would bring it up again.\n\nYup, I do not think the choice among the three would make all that\nmuch difference in practice. If I really have to pick one of them,\nI think the one in your message I am responding to would make the\nmost sense.\n\nThe one I sent, which I wrote as a response to some end-user request\non the list back then, has been sitting on 'pu' for quite a while\nbecause I didn't see a real use or positive support for it, and the\nonly reason why I sent it is because this might be that one\nreal use it wanted to see.\n\n\n> In some ways this is less convenient, but in some ways it is\n> more so; the user can then manually commit or even \"git\n> rebase --continue\" after setting up their ident (or\n> providing it as a one-off on the command line).\n\nYup, that is the controvercial bit, and I suspect Dscho's original\nwas siding for the \"set up ident first, as you will need it anyway\neventually\", so I'll let others with viewpoints different from us to\nchime in first before picking it up.\n\nThanks.\n"},{"id":"292569","messageId":"20160729224944.GA23242@sigill.intra.peff.net","threadId":"42959","inReplyTo":"xmqqwpk4kqr4.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-29T22:49:44Z","receivedAt":"2016-07-29T22:49:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 29, 2016 at 03:45:35PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > TBH, I'm not sure anybody cares that much between the three. Even before\n> > user.useConfigOnly, this could be an issue on machines where the ident\n> > could not be auto-configured, and it seems like nobody ran across it.\n> > It's only the funny interaction with pull.rebase that makes it likely to\n> > come up, so as long as that code path is fixed (one way or another), I\n> > doubt anybody would bring it up again.\n> \n> Yup, I do not think the choice among the three would make all that\n> much difference in practice. If I really have to pick one of them,\n> I think the one in your message I am responding to would make the\n> most sense.\n> \n> The one I sent, which I wrote as a response to some end-user request\n> on the list back then, has been sitting on 'pu' for quite a while\n> because I didn't see a real use or positive support for it, and the\n> only reason why I sent it is because this might be that one\n> real use it wanted to see.\n\nBTW, I didn't actually test yours, but if we do go that route I suspect\nyou can reuse the tests I posted by just replacing \"git rebase\" with\n\"git pull --rebase= . master\".\n\n> > In some ways this is less convenient, but in some ways it is\n> > more so; the user can then manually commit or even \"git\n> > rebase --continue\" after setting up their ident (or\n> > providing it as a one-off on the command line).\n> \n> Yup, that is the controvercial bit, and I suspect Dscho's original\n> was siding for the \"set up ident first, as you will need it anyway\n> eventually\", so I'll let others with viewpoints different from us to\n> chime in first before picking it up.\n\nVery sensible. Thanks.\n\n-Peff\n"},{"id":"292609","messageId":"CAG0BQXmF8Ro5j9oTPMCZwO4gfA+9VdGq3qQZwHJWSCuF_7h3Pg@mail.gmail.com","threadId":"42959","inReplyTo":"20160729224944.GA23242@sigill.intra.peff.net","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Dakota Hawkins","fromEmail":"dakotahawkins@gmail.com","sentAt":"2016-07-30T16:41:04Z","receivedAt":"2016-07-30T16:41:13Z","isPatch":false,"sender":{"key":"dakotahawkins@gmail.com","avatar":"https://gravatar.com/avatar/6283c977565cfcecb284e62d5713a91a6a8df0a3b06f27439edaab4220ae143d?d=mp&s=160"},"body":"On Fri, Jul 29, 2016 at 6:49 PM, Jeff King wrote:\n> On Fri, Jul 29, 2016 at 03:45:35PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>\n>> > TBH, I'm not sure anybody cares that much between the three. Even before\n>> > user.useConfigOnly, this could be an issue on machines where the ident\n>> > could not be auto-configured, and it seems like nobody ran across it.\n>> > It's only the funny interaction with pull.rebase that makes it likely to\n>> > come up, so as long as that code path is fixed (one way or another), I\n>> > doubt anybody would bring it up again.\n>>\n>> Yup, I do not think the choice among the three would make all that\n>> much difference in practice. If I really have to pick one of them,\n>> I think the one in your message I am responding to would make the\n>> most sense.\n>>\n>> The one I sent, which I wrote as a response to some end-user request\n>> on the list back then, has been sitting on 'pu' for quite a while\n>> because I didn't see a real use or positive support for it, and the\n>> only reason why I sent it is because this might be that one\n>> real use it wanted to see.\n>\n> BTW, I didn't actually test yours, but if we do go that route I suspect\n> you can reuse the tests I posted by just replacing \"git rebase\" with\n> \"git pull --rebase= . master\".\n>\n>> > In some ways this is less convenient, but in some ways it is\n>> > more so; the user can then manually commit or even \"git\n>> > rebase --continue\" after setting up their ident (or\n>> > providing it as a one-off on the command line).\n>>\n>> Yup, that is the controvercial bit, and I suspect Dscho's original\n>> was siding for the \"set up ident first, as you will need it anyway\n>> eventually\", so I'll let others with viewpoints different from us to\n>> chime in first before picking it up.\n>\n> Very sensible. Thanks.\n>\n> -Peff\n\nAll of the options sounds OK to me. I do like the idea of being able\nto set it and --continue what I was doing.\n\nEven more convenient than that would be an optional \"user.prompt=true\"\nso the ident check could get what it needs from a simple terminal\nprompt, set it in the local config, and try again before returning.\n\nWhile that would be nice, I've been OK with the current system for my\nworking repositories for a while. I'm used to having to go set it and\nrepeat whatever I was trying to do, as it seems like I forget to set\nmy user.email about half of the time :)\n\n-Dakota\n"},{"id":"298797","messageId":"xmqqvaz7x6vv.fsf@gitster.mtv.corp.google.com","threadId":"42959","inReplyTo":"20160729223134.GA22591@sigill.intra.peff.net","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-11T22:44:52Z","receivedAt":"2016-08-11T22:45:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier, Peff sent this patch (slightly buried in a discussion) on\n\"rebase -i\" in <20160729223134.GA22591@sigill.intra.peff.net>.\n\n> Subject: rebase-interactive: drop early check for valid ident\n>\n> Since the very inception of interactive-rebase in 1b1dce4\n> (Teach rebase an interactive mode, 2007-06-25), there has\n> been a preemptive check, before looking at any commits, to\n> see whether the user has a valid name/email combination.\n>\n> This is convenient, because it means that we abort the\n> operation before even beginning (rather than just\n> complaining that we are unable to pick a particular commit).\n>\n> However, it does the wrong thing when the rebase does not\n> actually need to generate any new commits (e.g., a\n> fast-forward with no commits to pick, or one where the base\n> stays the same, and we just pick the same commits without\n> rewriting anything). In this case it may complain about the\n> lack of ident, even though one would not be needed to\n> complete the operation.\n>\n> This may seem like mere nit-picking, but because interactive\n> rebase underlies the \"preserve-merges\" rebase, somebody who\n> has set \"pull.rebase\" to \"preserve\" cannot make even a\n> fast-forward pull without a valid ident, as we bail before\n> even realizing the fast-forward nature.\n>\n> This commit drops the extra ident check entirely. This means\n> we rely on individual commands that generate commit objects\n> to complain. So we will continue to notice and prevent cases\n> that actually do create commits, but with one important\n> difference: we fail while actually executing the \"pick\"\n> operations, and leave the rebase in a conflicted, half-done\n> state.\n>\n> In some ways this is less convenient, but in some ways it is\n> more so; the user can then manually commit or even \"git\n> rebase --continue\" after setting up their ident (or\n> providing it as a one-off on the command line).\n>\n> Reported-by: Dakota Hawkins \n> Signed-off-by: Jeff King \n> ---\n\nTo which, I responded (referring to the last paragraph):\n\n Yup, that is the controvercial bit, and I suspect Dscho's original\n was siding for the \"set up ident first, as you will need it anyway\n eventually\", so I'll let others with viewpoints different from us to\n chime in first before picking it up.\n\nDo you have a preference either way to help us decide if we want to\ntake this change or not?\n\nThanks.\n\n> git-rebase--interactive.sh | 3 ---\n> t/t7517-per-repo-email.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 47 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index ded4595..f0f4777 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -1180,9 +1180,6 @@ To continue rebase after editing, run:\n> \t;;\n> esac\n> \n> -git var GIT_COMMITTER_IDENT >/dev/null ||\n> -\tdie \"$(gettext \"You need to set your committer info first\")\"\n> -\n> comment_for_reflog start\n> \n> if test ! -z \"$switch_to\"\n> diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh\n> index 337e6e3..2a22fa7 100755\n> --- a/t/t7517-per-repo-email.sh\n> +++ b/t/t7517-per-repo-email.sh\n> @@ -36,4 +36,51 @@ test_expect_success 'succeeds cloning if global email is not set' '\n> \tgit clone . clone\n> '\n> \n> +test_expect_success 'set up rebase scenarios' '\n> +\t# temporarily enable an actual ident for this setup\n> +\ttest_config user.email foo@example.com &&\n> +\ttest_commit new &&\n> +\tgit branch side-without-commit HEAD^ &&\n> +\tgit checkout -b side-with-commit HEAD^ &&\n> +\ttest_commit side\n> +'\n> +\n> +test_expect_success 'fast-forward rebase does not care about ident' '\n> +\tgit checkout -B tmp side-without-commit &&\n> +\tgit rebase master\n> +'\n> +\n> +test_expect_success 'non-fast-forward rebase refuses to write commits' '\n> +\ttest_when_finished \"git rebase --abort || true\" &&\n> +\tgit checkout -B tmp side-with-commit &&\n> +\ttest_must_fail git rebase master\n> +'\n> +\n> +test_expect_success 'fast-forward rebase does not care about ident (interactive)' '\n> +\tgit checkout -B tmp side-without-commit &&\n> +\tgit rebase -i master\n> +'\n> +\n> +test_expect_success 'non-fast-forward rebase refuses to write commits (interactive)' '\n> +\ttest_when_finished \"git rebase --abort || true\" &&\n> +\tgit checkout -B tmp side-with-commit &&\n> +\ttest_must_fail git rebase -i master\n> +'\n> +\n> +test_expect_success 'noop interactive rebase does not care about ident' '\n> +\tgit checkout -B tmp side-with-commit &&\n> +\tgit rebase -i HEAD^\n> +'\n> +\n> +test_expect_success 'fast-forward rebase does not care about ident (preserve)' '\n> +\tgit checkout -B tmp side-without-commit &&\n> +\tgit rebase -p master\n> +'\n> +\n> +test_expect_success 'non-fast-forward rebase refuses to write commits (preserve)' '\n> +\ttest_when_finished \"git rebase --abort || true\" &&\n> +\tgit checkout -B tmp side-with-commit &&\n> +\ttest_must_fail git rebase -p master\n> +'\n> +\n> test_done\n"},{"id":"301517","messageId":"alpine.DEB.2.20.1609091731540.129229@virtualbox","threadId":"42959","inReplyTo":"xmqqvaz7x6vv.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T15:32:28Z","receivedAt":"2016-09-09T15:32:45Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 11 Aug 2016, Junio C Hamano wrote:\n\n> Earlier, Peff sent this patch (slightly buried in a discussion) on\n> \"rebase -i\" in <20160729223134.GA22591@sigill.intra.peff.net>.\n> \n> > Subject: rebase-interactive: drop early check for valid ident\n> >\n> > Since the very inception of interactive-rebase in 1b1dce4\n> > (Teach rebase an interactive mode, 2007-06-25), there has\n> > been a preemptive check, before looking at any commits, to\n> > see whether the user has a valid name/email combination.\n> >\n> > This is convenient, because it means that we abort the\n> > operation before even beginning (rather than just\n> > complaining that we are unable to pick a particular commit).\n> >\n> > However, it does the wrong thing when the rebase does not\n> > actually need to generate any new commits (e.g., a\n> > fast-forward with no commits to pick, or one where the base\n> > stays the same, and we just pick the same commits without\n> > rewriting anything). In this case it may complain about the\n> > lack of ident, even though one would not be needed to\n> > complete the operation.\n> >\n> > This may seem like mere nit-picking, but because interactive\n> > rebase underlies the \"preserve-merges\" rebase, somebody who\n> > has set \"pull.rebase\" to \"preserve\" cannot make even a\n> > fast-forward pull without a valid ident, as we bail before\n> > even realizing the fast-forward nature.\n> >\n> > This commit drops the extra ident check entirely. This means\n> > we rely on individual commands that generate commit objects\n> > to complain. So we will continue to notice and prevent cases\n> > that actually do create commits, but with one important\n> > difference: we fail while actually executing the \"pick\"\n> > operations, and leave the rebase in a conflicted, half-done\n> > state.\n> >\n> > In some ways this is less convenient, but in some ways it is\n> > more so; the user can then manually commit or even \"git\n> > rebase --continue\" after setting up their ident (or\n> > providing it as a one-off on the command line).\n> >\n> > Reported-by: Dakota Hawkins \n> > Signed-off-by: Jeff King \n> > ---\n> \n> To which, I responded (referring to the last paragraph):\n> \n> Yup, that is the controvercial bit, and I suspect Dscho's original\n> was siding for the \"set up ident first, as you will need it anyway\n> eventually\", so I'll let others with viewpoints different from us to\n> chime in first before picking it up.\n> \n> Do you have a preference either way to help us decide if we want to\n> take this change or not?\n\nI have no strong preference. I guess that it does not hurt to go with the\npatch, and it would probably help in a few cases.\n\nCiao,\nDscho\n"},{"id":"301520","messageId":"xmqq7faldpgp.fsf@gitster.mtv.corp.google.com","threadId":"42959","inReplyTo":"alpine.DEB.2.20.1609091731540.129229@virtualbox","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T16:09:58Z","receivedAt":"2016-09-09T16:10:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 11 Aug 2016, Junio C Hamano wrote:\n>> \n>> Do you have a preference either way to help us decide if we want to\n>> take this change or not?\n>\n> I have no strong preference. I guess that it does not hurt to go with the\n> patch, and it would probably help in a few cases.\n\nOK. Let me dig the change back and how well it still fits ;-)\n\nThanks.\n"},{"id":"301541","messageId":"xmqqk2ekc2zl.fsf@gitster.mtv.corp.google.com","threadId":"42959","inReplyTo":"xmqq7faldpgp.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T19:00:46Z","receivedAt":"2016-09-09T19:01:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> On Thu, 11 Aug 2016, Junio C Hamano wrote:\n>>> \n>>> Do you have a preference either way to help us decide if we want to\n>>> take this change or not?\n>>\n>> I have no strong preference. I guess that it does not hurt to go with the\n>> patch, and it would probably help in a few cases.\n>\n> OK. Let me dig the change back and how well it still fits ;-)\n\nAh, I already had it in my tree lest I forget. Let me mark it for\nmerging down to 'master'.\n\nThanks.\n"},{"id":"301582","messageId":"CAG0BQXmNyh-5pcCudobRpG=2bTHibDYkXJEBdWBx4HUgH4Ysqg@mail.gmail.com","threadId":"42959","inReplyTo":"xmqqk2ekc2zl.fsf@gitster.mtv.corp.google.com","subject":"Re: Issue with global config defaults \"user.useConfigOnly = true\" + \"pull.rebase = preserve\" - \"user.email\"","fromName":"Dakota Hawkins","fromEmail":"dakotahawkins@gmail.com","sentAt":"2016-09-09T23:31:01Z","receivedAt":"2016-09-09T23:31:07Z","isPatch":false,"sender":{"key":"dakotahawkins@gmail.com","avatar":"https://gravatar.com/avatar/6283c977565cfcecb284e62d5713a91a6a8df0a3b06f27439edaab4220ae143d?d=mp&s=160"},"body":"You guys are the best, I'm really impressed with all of the responses\nto this issue! Thank you all for all of your hard work!\n\nDakota\n\nOn Fri, Sep 9, 2016 at 3:00 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Johannes Schindelin writes:\n>>\n>>> On Thu, 11 Aug 2016, Junio C Hamano wrote:\n>>>>\n>>>> Do you have a preference either way to help us decide if we want to\n>>>> take this change or not?\n>>>\n>>> I have no strong preference. I guess that it does not hurt to go with the\n>>> patch, and it would probably help in a few cases.\n>>\n>> OK. Let me dig the change back and how well it still fits ;-)\n>\n> Ah, I already had it in my tree lest I forget. Let me mark it for\n> merging down to 'master'.\n>\n> Thanks.\n"}]}