Re: git commit --amend safety check?

From: Shawn Pearce <spearce@spearce.org>
Date: Mar 11, 2015, 06:00 UTC
Message-ID: <CAJo=hJtxeZGyP=VxLSdDzoMOtVZTFNsNTqPpNUTXUOBDZKzY9Q@mail.gmail.com>
In-Reply-To: <xmqq8uf46ru8.fsf@gitster.dls.corp.google.com>

On Tue, Mar 10, 2015 at 10:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

> Shawn Pearce <spearce@spearce.org> writes:
>
>> We keep seeing reports of Gerrit Code Review users who incorrectly do
>> something like:
>>
>>   git clone URL foo
>>   cd foo
>>   git commit --amend -m "My first change!" -a
>>   git push URL HEAD:refs/for/master
>>
>> Step #3 is where they get into trouble. They just amended the
>> published tip commit and pushed it back to the server. That is... lets
>> just say not good.
>>
>> Hg is known to be more user friendly. One way its user friendly is it
>> by default refuses to let you amend a change set that the client has
>> reasonable assertion to believe was already published through a remote
>> repository.
>
> Well, it is not Git that is less user friendly, but Gerrit is the
> problem.  More specifically, the last line:
>
>>   git push URL HEAD:refs/for/master
>
> is what catches this non-fast-forward in usual workflow with Git.

OK, replace that Gerrit-specific push syntax with:

 git send-email

:)

Even in mailing list based workflows we ask Git users to use "git commit" to make their new commit and "git commit --amend" to polish it up. New users are often confused and cargo-culting their Git usage because they cannot be bothered to actually learn the tools they are using; they are too busy learning all of the other tools they need, like their programming language or the latest jQuery plugin.

> Wouldn't the real problem that the refs/for/master magic accepts
> anything, even a non-fast-forward?

I see your argument. But this was a conscious design choice.

It is already untenable to ask users sending you patches on git ML to first fetch and rebase *their* working tree against your latest master before they run git send-email. The async nature of when you publish your tree vs. when they prepared their commit makes it likely that at least some of the patches you receive were based on a different tip than what you most recently published.

On very fast moving development repositories that have hundreds of developers working against them during the bulk of the working day the tip can be advancing more rapidly than users have the patience to run:

  while ! ( git pull --rebase && git push origin HEAD:refs/for/master ); do
    echo Retrying ...
  done

Show 13 quoted lines

> Having said that, disabling --amend and forcing to use --force or
> something when it is clear that the user is attempting something
> unusual is a good idea.  But I am not sure what the definition of
> unusual should be.  In a non-Gerrit central repository workflow, the
> rule might be "HEAD must not be reachable from @{upstream}"
> (otherwise you are rewriting what you got from elsewhere), or it may
> be "HEAD must not be reachable from @{publish}'s remote tracking
> branch", or perhaps both, as these two could be different in
> triangular workflow.
>
> But I am not sure what the sensible rules are when the user prepares
> the commit, planning to push it to a ref like refs/for/master that
> does not have a counterpart on our side.

True.

Another way users get into a bind is they pull someone else's branch (so they can build on top of her work), then `git commit --amend -a` by mistake instead of making a new commit. Now they mixed their work with her last commit. Then they push this.

Currently Gerrit Code Review accepts that as an intentional squash to further polish her proposed change. And why not? I can take a patch proposed by Peff from the mailing list, `commit --amend` additional polishing, and resend with attribution^Wblame to Peff.

Perhaps Gerrit Code Review should reject this by default unless the user has some sort of "Yes I know what I am doing" bit toggled on her user account.

That only mildly helps the poor newbie who has no idea how to "unstick" their Git repository. Most users just rm -rf the directory, reclone, and start over from scratch, meanwhile complaining that "Git lost hours of their work".

Nevermind that one could probably get out of such a jam with something like:

  git diff HEAD@{1}..HEAD >P
  git reset --hard HEAD@{1}
  git apply --index P ; rm P

If only they knew how to use their tools. :(