{"thread":{"id":"38782","subject":"git commit --amend safety check?","startedAt":"2015-03-11T04:31:40Z","lastAt":"2015-03-11T17:56:20Z","messageCount":8,"participants":["Shawn Pearce","Junio C Hamano","Jeff King","Peter Krefting","Mike Hommey"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"257533","messageId":"CAJo=hJuwdOzNZSVBRAPDz97Gdoi6JWvYxE0ufY+Hd9K8jjCqfA@mail.gmail.com","threadId":"38782","inReplyTo":null,"subject":"git commit --amend safety check?","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2015-03-11T04:31:40Z","receivedAt":"2015-03-11T04:31:40Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"We keep seeing reports of Gerrit Code Review users who incorrectly do\nsomething like:\n\n git clone URL foo\n cd foo\n git commit --amend -m \"My first change!\" -a\n git push URL HEAD:refs/for/master\n\nStep #3 is where they get into trouble. They just amended the\npublished tip commit and pushed it back to the server. That is... lets\njust say not good.\n\nHg is known to be more user friendly. One way its user friendly is it\nby default refuses to let you amend a change set that the client has\nreasonable assertion to believe was already published through a remote\nrepository.\n\nFor Git that would mean `commit --amend` refusing to amend (by\ndefault) if the commit is reachable from a refs/remotes/... tracking\nbranch.\n\nThoughts?\n"},{"id":"257534","messageId":"xmqq8uf46ru8.fsf@gitster.dls.corp.google.com","threadId":"38782","inReplyTo":"CAJo=hJuwdOzNZSVBRAPDz97Gdoi6JWvYxE0ufY+Hd9K8jjCqfA@mail.gmail.com","subject":"Re: git commit --amend safety check?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-11T05:11:43Z","receivedAt":"2015-03-11T05:11:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> We keep seeing reports of Gerrit Code Review users who incorrectly do\n> something like:\n>\n> git clone URL foo\n> cd foo\n> git commit --amend -m \"My first change!\" -a\n> git push URL HEAD:refs/for/master\n>\n> Step #3 is where they get into trouble. They just amended the\n> published tip commit and pushed it back to the server. That is... lets\n> just say not good.\n>\n> Hg is known to be more user friendly. One way its user friendly is it\n> by default refuses to let you amend a change set that the client has\n> reasonable assertion to believe was already published through a remote\n> repository.\n\nWell, it is not Git that is less user friendly, but Gerrit is the\nproblem. More specifically, the last line:\n\n> git push URL HEAD:refs/for/master\n\nis what catches this non-fast-forward in usual workflow with Git.\nWouldn't the real problem that the refs/for/master magic accepts\nanything, even a non-fast-forward?\n\nHaving said that, disabling --amend and forcing to use --force or\nsomething when it is clear that the user is attempting something\nunusual is a good idea. But I am not sure what the definition of\nunusual should be. In a non-Gerrit central repository workflow, the\nrule might be \"HEAD must not be reachable from @{upstream}\"\n(otherwise you are rewriting what you got from elsewhere), or it may\nbe \"HEAD must not be reachable from @{publish}'s remote tracking\nbranch\", or perhaps both, as these two could be different in\ntriangular workflow.\n\nBut I am not sure what the sensible rules are when the user prepares\nthe commit, planning to push it to a ref like refs/for/master that\ndoes not have a counterpart on our side.\n"},{"id":"257536","messageId":"CAJo=hJtxeZGyP=VxLSdDzoMOtVZTFNsNTqPpNUTXUOBDZKzY9Q@mail.gmail.com","threadId":"38782","inReplyTo":"xmqq8uf46ru8.fsf@gitster.dls.corp.google.com","subject":"Re: git commit --amend safety check?","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2015-03-11T06:00:42Z","receivedAt":"2015-03-11T06:00:42Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, Mar 10, 2015 at 10:11 PM, Junio C Hamano wrote:\n> Shawn Pearce writes:\n>\n>> We keep seeing reports of Gerrit Code Review users who incorrectly do\n>> something like:\n>>\n>> git clone URL foo\n>> cd foo\n>> git commit --amend -m \"My first change!\" -a\n>> git push URL HEAD:refs/for/master\n>>\n>> Step #3 is where they get into trouble. They just amended the\n>> published tip commit and pushed it back to the server. That is... lets\n>> just say not good.\n>>\n>> Hg is known to be more user friendly. One way its user friendly is it\n>> by default refuses to let you amend a change set that the client has\n>> reasonable assertion to believe was already published through a remote\n>> repository.\n>\n> Well, it is not Git that is less user friendly, but Gerrit is the\n> problem. More specifically, the last line:\n>\n>> git push URL HEAD:refs/for/master\n>\n> is what catches this non-fast-forward in usual workflow with Git.\n\nOK, replace that Gerrit-specific push syntax with:\n\n git send-email\n\n:)\n\nEven in mailing list based workflows we ask Git users to use \"git\ncommit\" to make their new commit and \"git commit --amend\" to polish it\nup. New users are often confused and cargo-culting their Git usage\nbecause they cannot be bothered to actually learn the tools they are\nusing; they are too busy learning all of the other tools they need,\nlike their programming language or the latest jQuery plugin.\n\n> Wouldn't the real problem that the refs/for/master magic accepts\n> anything, even a non-fast-forward?\n\nI see your argument. But this was a conscious design choice.\n\nIt is already untenable to ask users sending you patches on git ML to\nfirst fetch and rebase *their* working tree against your latest master\nbefore they run git send-email. The async nature of when you publish\nyour tree vs. when they prepared their commit makes it likely that at\nleast some of the patches you receive were based on a different tip\nthan what you most recently published.\n\nOn very fast moving development repositories that have hundreds of\ndevelopers working against them during the bulk of the working day the\ntip can be advancing more rapidly than users have the patience to run:\n\n while ! ( git pull --rebase && git push origin HEAD:refs/for/master ); do\n echo Retrying ...\n done\n\n> Having said that, disabling --amend and forcing to use --force or\n> something when it is clear that the user is attempting something\n> unusual is a good idea. But I am not sure what the definition of\n> unusual should be. In a non-Gerrit central repository workflow, the\n> rule might be \"HEAD must not be reachable from @{upstream}\"\n> (otherwise you are rewriting what you got from elsewhere), or it may\n> be \"HEAD must not be reachable from @{publish}'s remote tracking\n> branch\", or perhaps both, as these two could be different in\n> triangular workflow.\n>\n> But I am not sure what the sensible rules are when the user prepares\n> the commit, planning to push it to a ref like refs/for/master that\n> does not have a counterpart on our side.\n\nTrue.\n\nAnother way users get into a bind is they pull someone else's branch\n(so they can build on top of her work), then `git commit --amend -a`\nby mistake instead of making a new commit. Now they mixed their work\nwith her last commit. Then they push this.\n\nCurrently Gerrit Code Review accepts that as an intentional squash to\nfurther polish her proposed change. And why not? I can take a patch\nproposed by Peff from the mailing list, `commit --amend` additional\npolishing, and resend with attribution^Wblame to Peff.\n\n\nPerhaps Gerrit Code Review should reject this by default unless the\nuser has some sort of \"Yes I know what I am doing\" bit toggled on her\nuser account.\n\nThat only mildly helps the poor newbie who has no idea how to\n\"unstick\" their Git repository. Most users just rm -rf the directory,\nreclone, and start over from scratch, meanwhile complaining that \"Git\nlost hours of their work\".\n\nNevermind that one could probably get out of such a jam with something like:\n\n git diff HEAD@{1}..HEAD >P\n git reset --hard HEAD@{1}\n git apply --index P ; rm P\n\nIf only they knew how to use their tools. :(\n"},{"id":"257537","messageId":"CAPc5daVMec1okdBW3Wo_gEr7W3FwRwmH5pmiiMaAgGoN7MGa_A@mail.gmail.com","threadId":"38782","inReplyTo":"CAJo=hJtxeZGyP=VxLSdDzoMOtVZTFNsNTqPpNUTXUOBDZKzY9Q@mail.gmail.com","subject":"Re: git commit --amend safety check?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-11T06:18:45Z","receivedAt":"2015-03-11T06:18:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Mar 10, 2015 at 11:00 PM, Shawn Pearce wrote:\n>\n> OK, replace that Gerrit-specific push syntax with:\n>\n> git send-email\n>\n> :)\n\nHeh don't be too defensive; I was merely pulling your leg.\n\n>> ... But I am not sure what the definition of\n>> unusual should be. In a non-Gerrit central repository workflow, the\n>> rule might be \"HEAD must not be reachable from @{upstream}\"\n>> (otherwise you are rewriting what you got from elsewhere), or it may\n>> be \"HEAD must not be reachable from @{publish}'s remote tracking\n>> branch\", or perhaps both, as these two could be different in\n>> triangular workflow.\n>>\n>> But I am not sure what the sensible rules are when the user prepares\n>> the commit, planning to push it to a ref like refs/for/master that\n>> does not have a counterpart on our side.\n>\n> True.\n\nWith a more concrete example, I'd imagine you have something like\nthis to wok on branch \"master\"\n\n[remote \"origin\"]\n url = ...\n pushurl = ...\n fetch = +refs/heads/*:refs/remotes/origin/*\n push = refs/heads/*:refs/for/*\n\n[branch \"master\"]\n merge = refs/heads/master\n\nEven though we cannot prevent the user from rewriting what _he_\nalready pushed out to refs/for/master (as we do not have the record\nof what the last thing we pushed there and its history via a reflog),\nwe could at least detect when he attempts to rewrite what he\nobtained directly from the upstream by noticing where origin/master\nis. If HEAD is _at_ that commit, or its ancestor, then he is trying to\nrewrite what he got from elsewhere.\n\nIt would catch your original \"commit --amend -m 'my first'\" scenario.\nRun is_ancestor(HEAD, @{upstream}) we can notice. That may be\nbetter than nothing, but I do not offhand know if that is sufficient.\n\n> Another way users get into a bind is they pull someone else's branch\n> (so they can build on top of her work), then `git commit --amend -a`\n> by mistake instead of making a new commit.\n\nOne thing we already do is to give an extra \"Author: \" line in the\ncomment when the user edits the log message, so that it is clear\nthat what is being edited is not their own work but hers. We obviously\ncan add the extra warning, when the is_ancestor() thing triggers, to\nsay YOU ARE REWRITING PUBLISHED HISTORY in blinking red\nbold letters there.\n\nBut the symptom indicates that they are not reading these warning\ncomment. Perhaps it is necessary to introduce a training wheel mode\nwhere you cannot use \"--amend\" and \"-m\" options from the command\nline until you ask nicely to override it?\n"},{"id":"257550","messageId":"20150311063335.GA20713@glandium.org","threadId":"38782","inReplyTo":"CAPc5daVMec1okdBW3Wo_gEr7W3FwRwmH5pmiiMaAgGoN7MGa_A@mail.gmail.com","subject":"Re: git commit --amend safety check?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-03-11T06:33:35Z","receivedAt":"2015-03-11T06:33:35Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Mar 10, 2015 at 11:18:45PM -0700, Junio C Hamano wrote:\n> One thing we already do is to give an extra \"Author: \" line in the\n> comment when the user edits the log message, so that it is clear\n> that what is being edited is not their own work but hers. We obviously\n> can add the extra warning, when the is_ancestor() thing triggers, to\n> say YOU ARE REWRITING PUBLISHED HISTORY in blinking red\n> bold letters there.\n> \n> But the symptom indicates that they are not reading these warning\n> comment.\n\nMaybe they would if the warnings were a big paragraph *before* the\ncommit message (but brains are easily trained to recognize patterns and\nskip them, so that would need to be done for very specific warnings,\nnot for everything that's printed in a git commit editor).\n\nMike\n"},{"id":"257548","messageId":"20150311081329.GA11087@peff.net","threadId":"38782","inReplyTo":"CAPc5daVMec1okdBW3Wo_gEr7W3FwRwmH5pmiiMaAgGoN7MGa_A@mail.gmail.com","subject":"Re: git commit --amend safety check?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-11T08:13:29Z","receivedAt":"2015-03-11T08:13:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 10, 2015 at 11:18:45PM -0700, Junio C Hamano wrote:\n\n> Even though we cannot prevent the user from rewriting what _he_\n> already pushed out to refs/for/master (as we do not have the record\n> of what the last thing we pushed there and its history via a reflog),\n> we could at least detect when he attempts to rewrite what he\n> obtained directly from the upstream by noticing where origin/master\n> is. If HEAD is _at_ that commit, or its ancestor, then he is trying to\n> rewrite what he got from elsewhere.\n> \n> It would catch your original \"commit --amend -m 'my first'\" scenario.\n> Run is_ancestor(HEAD, @{upstream}) we can notice. That may be\n> better than nothing, but I do not offhand know if that is sufficient.\n\nI think rebase basically suffers the same problem, too. Perhaps it\nhappens less there because we choose @{upstream} as the default fork\npoint. But rewriting commits is a potential problem any time they are\nreferenced somewhere else. For example, if you do this:\n\n git checkout -b topic origin/master\n ... commit commit commit ...\n git checkout -b subtopic\n ... commit commit commit ...\n git checkout topic\n git rebase ;# or git pull --rebase\n\nyou are left with doppelganger commits in \"subtopic\", which you probably\nwant to handle by rebasing it (with --fork-point).\n\nI kind of wonder if the check should just be \"is the commit you are\nrewriting mentioned in _any_ ref except the current HEAD?\".\n\nIn theory we could even give advice based on the command and the ref we\nfind that contains the commit (e.g., if it's another local branch,\nsuggest rebasing that branch. If it's in @{upstream}, suggest \"commit\"\nwithout \"--amend\" if that was the command given). But I'm not at all\nconfident that we could cover all cases thoroughly enough to do more\ngood than harm.\n\n> > Another way users get into a bind is they pull someone else's branch\n> > (so they can build on top of her work), then `git commit --amend -a`\n> > by mistake instead of making a new commit.\n> \n> One thing we already do is to give an extra \"Author: \" line in the\n> comment when the user edits the log message, so that it is clear\n> that what is being edited is not their own work but hers. We obviously\n> can add the extra warning, when the is_ancestor() thing triggers, to\n> say YOU ARE REWRITING PUBLISHED HISTORY in blinking red\n> bold letters there.\n> \n> But the symptom indicates that they are not reading these warning\n> comment. Perhaps it is necessary to introduce a training wheel mode\n> where you cannot use \"--amend\" and \"-m\" options from the command\n> line until you ask nicely to override it?\n\nIt's entirely possible to me that the \"Author\" line is too subtle, and a\nbigger warning might do the trick.\n\nAt the very least printing a warning that can be suppressed with\nadvice.* would match our usual technique for dealing with possible\nmistakes like this (as opposed to blocking the action entirely). And we\ncan always bump the severity of the warning (or introduce blocking) if\nit doesn't have an effect.\n\nI dunno. It feels like such a warning would probably trigger as a false\npositive way too often and get in people's way. But then, I am not\nexactly the target audience for the warning, so my perspective is a bit\nskewed. I do think it has a reasonable chance of irritating old-timers,\neven with an escape hatch. Were you thinking that training-wheel mode\nwould have to be turned on explicitly?\n\n-Peff\n"},{"id":"257549","messageId":"alpine.DEB.2.00.1503110931100.26355@ds9.cixit.se","threadId":"38782","inReplyTo":"xmqq8uf46ru8.fsf@gitster.dls.corp.google.com","subject":"Re: git commit --amend safety check?","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2015-03-11T08:37:09Z","receivedAt":"2015-03-11T08:37:09Z","isPatch":false,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Junio C Hamano:\n\n> Having said that, disabling --amend and forcing to use --force or \n> something when it is clear that the user is attempting something \n> unusual is a good idea. But I am not sure what the definition of \n> unusual should be.\n\nFor commit --amend, I would say it would refuse to amend if the commit \nyou are trying to amend\n\n 1. was not authored by yourself (and --reset-author was not given), or\n 2. is reachable (or is the tip?) from an upstream branch.\n\nOf course, you should be able to do the amend commit, but then you \nwould need to say something like \"git commit --amend --force\" to \nindicate that you really want to do that.\n\nAt least (1) would have saved myself from mistakes that take time and \neffort to clean up (I have used Git for eight years or so already, and \nI *still* do that kind of mistake every now and then).\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"257560","messageId":"xmqq1tkv770b.fsf@gitster.dls.corp.google.com","threadId":"38782","inReplyTo":"alpine.DEB.2.00.1503110931100.26355@ds9.cixit.se","subject":"Re: git commit --amend safety check?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-11T17:56:20Z","receivedAt":"2015-03-11T17:56:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Krefting writes:\n\n> For commit --amend, I would say it would refuse to amend if the commit\n> you are trying to amend\n>\n> 1. was not authored by yourself (and --reset-author was not given), or\n> 2. is reachable (or is the tip?) from an upstream branch.\n\nI agree that 2. is a safe check without too much risk to trigger a\nfalse positive (and the tip of origin/master is reachable from\norigin/master, so we do not have to single out \"is the tip\").\n\nOn the other hand, 1. may be good in training wheel mode, but once\nyou start allowing amends and rebases, I do not see why it should be\nconsidered possibly bad as long as check 2. says it is OK to rewrite.\n\n> At least (1) would have saved myself from mistakes that take time and\n> effort to clean up (I have used Git for eight years or so already, and\n> I *still* do that kind of mistake every now and then).\n\nIsn't your friend reflog helping you to clean things up? The\ndifference between the state before you started amending and the\ncurrent state is what you did since then, so...?\n"}]}