git commit --amend safety check?

git commit --amend safety check?

[Shawn Pearce]

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.

For Git that would mean `commit --amend` refusing to amend (by
default) if the commit is reachable from a refs/remotes/... tracking
branch.

Thoughts?

Re: git commit --amend safety check?

[Junio C Hamano]

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.
Wouldn't the real problem that the refs/for/master magic accepts
anything, even a non-fast-forward?

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.

Re: git commit --amend safety check?

[Shawn Pearce]

On Tue, Mar 10, 2015 at 10:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

> 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. :(

Re: git commit --amend safety check?

[Junio C Hamano]

On Tue, Mar 10, 2015 at 11:00 PM, Shawn Pearce <spearce@spearce.org> wrote:
>
> OK, replace that Gerrit-specific push syntax with:
>
>  git send-email
>
> :)

Heh don't be too defensive; I was merely pulling your leg.

>> ...  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.

With a more concrete example, I'd imagine you have something like
this to wok on branch "master"

[remote "origin"]
  url = ...
  pushurl = ...
  fetch = +refs/heads/*:refs/remotes/origin/*
  push = refs/heads/*:refs/for/*

[branch "master"]
  merge = refs/heads/master

Even though we cannot prevent the user from rewriting what _he_
already pushed out to refs/for/master (as we do not have the record
of what the last thing we pushed there and its history via a reflog),
we could at least detect when he attempts to rewrite what he
obtained directly from the upstream by noticing where origin/master
is. If HEAD is _at_ that commit, or its ancestor, then he is trying to
rewrite what he got from elsewhere.

It would catch your original "commit --amend -m 'my first'" scenario.
Run is_ancestor(HEAD, @{upstream}) we can notice. That may be
better than nothing, but I do not offhand know if that is sufficient.

> 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.

One thing we already do is to give an extra "Author: " line in the
comment when the user edits the log message, so that it is clear
that what is being edited is not their own work but hers. We obviously
can add the extra warning, when the is_ancestor() thing triggers, to
say YOU ARE REWRITING PUBLISHED HISTORY in blinking red
bold letters there.

But the symptom indicates that they are not reading these warning
comment. Perhaps it is necessary to introduce a training wheel mode
where you cannot use "--amend" and "-m" options from the command
line until you ask nicely to override it?

Re: git commit --amend safety check?

[Mike Hommey]

On Tue, Mar 10, 2015 at 11:18:45PM -0700, Junio C Hamano wrote:
> One thing we already do is to give an extra "Author: " line in the
> comment when the user edits the log message, so that it is clear
> that what is being edited is not their own work but hers. We obviously
> can add the extra warning, when the is_ancestor() thing triggers, to
> say YOU ARE REWRITING PUBLISHED HISTORY in blinking red
> bold letters there.
> 
> But the symptom indicates that they are not reading these warning
> comment.

Maybe they would if the warnings were a big paragraph *before* the
commit message (but brains are easily trained to recognize patterns and
skip them, so that would need to be done for very specific warnings,
not for everything that's printed in a git commit editor).

Mike

Re: git commit --amend safety check?

[Jeff King]

On Tue, Mar 10, 2015 at 11:18:45PM -0700, Junio C Hamano wrote:

> Even though we cannot prevent the user from rewriting what _he_
> already pushed out to refs/for/master (as we do not have the record
> of what the last thing we pushed there and its history via a reflog),
> we could at least detect when he attempts to rewrite what he
> obtained directly from the upstream by noticing where origin/master
> is. If HEAD is _at_ that commit, or its ancestor, then he is trying to
> rewrite what he got from elsewhere.
> 
> It would catch your original "commit --amend -m 'my first'" scenario.
> Run is_ancestor(HEAD, @{upstream}) we can notice. That may be
> better than nothing, but I do not offhand know if that is sufficient.

I think rebase basically suffers the same problem, too. Perhaps it
happens less there because we choose @{upstream} as the default fork
point. But rewriting commits is a potential problem any time they are
referenced somewhere else. For example, if you do this:

  git checkout -b topic origin/master
  ... commit commit commit ...
  git checkout -b subtopic
  ... commit commit commit ...
  git checkout topic
  git rebase ;# or git pull --rebase

you are left with doppelganger commits in "subtopic", which you probably
want to handle by rebasing it (with --fork-point).

I kind of wonder if the check should just be "is the commit you are
rewriting mentioned in _any_ ref except the current HEAD?".

In theory we could even give advice based on the command and the ref we
find that contains the commit (e.g., if it's another local branch,
suggest rebasing that branch. If it's in @{upstream}, suggest "commit"
without "--amend" if that was the command given). But I'm not at all
confident that we could cover all cases thoroughly enough to do more
good than harm.

> > 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.
> 
> One thing we already do is to give an extra "Author: " line in the
> comment when the user edits the log message, so that it is clear
> that what is being edited is not their own work but hers. We obviously
> can add the extra warning, when the is_ancestor() thing triggers, to
> say YOU ARE REWRITING PUBLISHED HISTORY in blinking red
> bold letters there.
> 
> But the symptom indicates that they are not reading these warning
> comment. Perhaps it is necessary to introduce a training wheel mode
> where you cannot use "--amend" and "-m" options from the command
> line until you ask nicely to override it?

It's entirely possible to me that the "Author" line is too subtle, and a
bigger warning might do the trick.

At the very least printing a warning that can be suppressed with
advice.* would match our usual technique for dealing with possible
mistakes like this (as opposed to blocking the action entirely). And we
can always bump the severity of the warning (or introduce blocking) if
it doesn't have an effect.

I dunno. It feels like such a warning would probably trigger as a false
positive way too often and get in people's way. But then, I am not
exactly the target audience for the warning, so my perspective is a bit
skewed. I do think it has a reasonable chance of irritating old-timers,
even with an escape hatch. Were you thinking that training-wheel mode
would have to be turned on explicitly?

-Peff

Re: git commit --amend safety check?

[Peter Krefting]

Junio C Hamano:

> 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.

For commit --amend, I would say it would refuse to amend if the commit 
you are trying to amend

  1. was not authored by yourself (and --reset-author was not given), or
  2. is reachable (or is the tip?) from an upstream branch.

Of course, you should be able to do the amend commit, but then you 
would need to say something like "git commit --amend --force" to 
indicate that you really want to do that.

At least (1) would have saved myself from mistakes that take time and 
effort to clean up (I have used Git for eight years or so already, and 
I *still* do that kind of mistake every now and then).

-- 
\\// Peter - http://www.softwolves.pp.se/

Re: git commit --amend safety check?

[Junio C Hamano]

Peter Krefting <peter@softwolves.pp.se> writes:

> For commit --amend, I would say it would refuse to amend if the commit
> you are trying to amend
>
>  1. was not authored by yourself (and --reset-author was not given), or
>  2. is reachable (or is the tip?) from an upstream branch.

I agree that 2. is a safe check without too much risk to trigger a
false positive (and the tip of origin/master is reachable from
origin/master, so we do not have to single out "is the tip").

On the other hand, 1. may be good in training wheel mode, but once
you start allowing amends and rebases, I do not see why it should be
considered possibly bad as long as check 2. says it is OK to rewrite.

> At least (1) would have saved myself from mistakes that take time and
> effort to clean up (I have used Git for eight years or so already, and
> I *still* do that kind of mistake every now and then).

Isn't your friend reflog helping you to clean things up?  The
difference between the state before you started amending and the
current state is what you did since then, so...?