Re: git commit --amend safety check?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 10, 2015 at 10:11 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Shawn Pearce <spearce@xxxxxxxxxxx> 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

> 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. :(
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]