amend warnings with no changes staged

amend warnings with no changes staged

[Lukas Gross]

Hi,

I have occasionally used git commit --amend without staging any
changes or modifying the commit message (--no-edit). Since this is
often done unintentionally, could amend warn when it is being used in
this way?

Thanks!

Re: amend warnings with no changes staged

[Jonathan Nieder]

Hi,

Lukas Gross wrote:

> I have occasionally used git commit --amend without staging any
> changes or modifying the commit message (--no-edit). Since this is
> often done unintentionally, could amend warn when it is being used in
> this way?

Can you say more about the context?  What were you trying to do when
you performed this operation?  What happened instead?

Thanks,
Jonathan

Re: amend warnings with no changes staged

[Lukas Gross]

Hi Jonathan,

I had intended to stage commits but forgot to do so. Git responded
with a normal commit creation message, so I pushed to the remote to
begin a CI build. When the build failed for the same reason, I
realized I had forgotten to stage the changes. An additional line in
the response to the effect of “Warning: did you mean to amend with no
changes?” would be very helpful to shorten this feedback loop.

Lukas

On 8/5/19, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Lukas Gross wrote:
>
>> I have occasionally used git commit --amend without staging any
>> changes or modifying the commit message (--no-edit). Since this is
>> often done unintentionally, could amend warn when it is being used in
>> this way?
>
> Can you say more about the context?  What were you trying to do when
> you performed this operation?  What happened instead?
>
> Thanks,
> Jonathan
>


-- 
*Lukas Gross*
B.S. Computer Science | McCormick School of Engineering
Northwestern University | Class of 2020
(224) 522-9067

Re: amend warnings with no changes staged

[Jonathan Nieder]

(administrivia: please don't top-post)
Lukas Gross wrote:

> I had intended to stage commits but forgot to do so. Git responded
> with a normal commit creation message, so I pushed to the remote to
> begin a CI build. When the build failed for the same reason, I
> realized I had forgotten to stage the changes. An additional line in
> the response to the effect of “Warning: did you mean to amend with no
> changes?” would be very helpful to shorten this feedback loop.

Ah, I see.  You passed --no-edit so you didn't see the usual --dry-run
style output that "git commit" shows.  You forgot to run "git add"
before amending, and this is what you'd like commit to assist you
with.

That said, this is sometimes an operation people perform
intentionally.

Ideas:

* Can the documentation do a better job of steering people away
  from --no-edit?  The hints shown when "git commit --amend" (and
  --no-amend, for that matter) open a text editor tend to be very
  helpful for understanding what is going to happen.  If any
  documentation is leading people to forgo that tool, we should fix
  it.

* Should "git commit --no-edit" say a little more about what it did,
  since it knows that the user didn't get to see the text editor?

* Should we have a special option (e.g. "git commit --amend
  --refresh") for a no-op amend?  That ways, when a user doesn't pass
  that option, it would be more clear that it is a mistake, allowing
  a warning or even an error.

Of these three, the first seems most compelling to me.  Others may
have other ideas.

Thoughts?

Thanks,
Jonathan

Re: amend warnings with no changes staged

[Jonathan Nieder]

Lukas Gross wrote:

> I had intended to stage commits but forgot to do so. Git responded
> with a normal commit creation message, so I pushed to the remote to
> begin a CI build. When the build failed for the same reason, I
> realized I had forgotten to stage the changes. An additional line in
> the response to the effect of “Warning: did you mean to amend with no
> changes?” would be very helpful to shorten this feedback loop.

On second thought:

	$ git commit --amend --no-edit
	[detached HEAD 33a3db8805] Git 2.23-rc1
	 Author: Junio C Hamano <gitster@pobox.com>
	 Date: Fri Aug 2 13:12:24 2019 -0700
	 2 files changed, 2 insertions(+), 1 deletion(-)
	$

Some non-judgemental descriptive output like

	$ git commit --amend --no-edit
	No changes.
	$

would address this case, without bothering people who are doing it
intentionally.  So I think there's room for a simple improvement here.

Care to take a stab at it?  builtin/commit.c would be the place to
start.

Thanks and sorry for the roller-coaster,
Jonathan

Re: amend warnings with no changes staged

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Lukas Gross wrote:
>
>> I had intended to stage commits but forgot to do so. Git responded
>> with a normal commit creation message, so I pushed to the remote to
>> begin a CI build. When the build failed for the same reason, I
>> realized I had forgotten to stage the changes. An additional line in
>> the response to the effect of “Warning: did you mean to amend with no
>> changes?” would be very helpful to shorten this feedback loop.
>
> On second thought:
>
> 	$ git commit --amend --no-edit
> 	[detached HEAD 33a3db8805] Git 2.23-rc1
> 	 Author: Junio C Hamano <gitster@pobox.com>
> 	 Date: Fri Aug 2 13:12:24 2019 -0700
> 	 2 files changed, 2 insertions(+), 1 deletion(-)
> 	$
>
> Some non-judgemental descriptive output like
>
> 	$ git commit --amend --no-edit
> 	No changes.
> 	$
>
> would address this case, without bothering people who are doing it
> intentionally.  So I think there's room for a simple improvement here.

I do that to refresh the committer timestamp.  Do I now have to say
something silly like

	$ GIT_EDITOR=true git commit --amend

to defeat the above (mis)feature, or is there a cleaner workaround?

Obviously I'd prefer to see a solution that does not force existing
users to work around the new behaviour ;-)

> Care to take a stab at it?  builtin/commit.c would be the place to
> start.

I'd suggest not to start it before we know what we want.  I am not
yet convinced that "--amend --no-edit will become a no-op" is the
final solution we want.

Re: amend warnings with no changes staged

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Lukas Gross wrote:

>>> I had intended to stage commits but forgot to do so. Git responded
>>> with a normal commit creation message, so I pushed to the remote to
>>> begin a CI build. When the build failed for the same reason, I
>>> realized
[...]
>> 	$ git commit --amend --no-edit
>> 	[detached HEAD 33a3db8805] Git 2.23-rc1
>> 	 Author: Junio C Hamano <gitster@pobox.com>
>> 	 Date: Fri Aug 2 13:12:24 2019 -0700
>> 	 2 files changed, 2 insertions(+), 1 deletion(-)
>> 	$
>>
>> Some non-judgemental descriptive output like
>>
>> 	$ git commit --amend --no-edit
>> 	No changes.
>> 	$
>>
>> would address this case, without bothering people who are doing it
>> intentionally.  So I think there's room for a simple improvement here.
>
> I do that to refresh the committer timestamp.

I do, too.  The proposal is, paraphrasing,

	$ git commit --amend --no-edit
	Ah, I see that you want me to refresh the committer timestamp.
	Done, as requested.
	$

In other words:

[...]
>                                                           I am not
> yet convinced that "--amend --no-edit will become a no-op" is the
> final solution we want.

Not this.

Hoping that clarifies,
Jonathan

Re: amend warnings with no changes staged

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>>> Some non-judgemental descriptive output like
>>>
>>> 	$ git commit --amend --no-edit
>>> 	No changes.
>>> 	$
>>>
>>> would address this case, without bothering people who are doing it
>>> intentionally.  So I think there's room for a simple improvement here.
>>
>> I do that to refresh the committer timestamp.
>
> I do, too.  The proposal is, paraphrasing,
>
> 	$ git commit --amend --no-edit
> 	Ah, I see that you want me to refresh the committer timestamp.
> 	Done, as requested.
> 	$

Ah, OK then.  I somehow misread "No changes." as an error message.

Re: amend warnings with no changes staged

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>>> Some non-judgemental descriptive output like
>>>>
>>>> 	$ git commit --amend --no-edit
>>>> 	No changes.
>>>> 	$
>>>>
>>>> would address this case, without bothering people who are doing it
>>>> intentionally.  So I think there's room for a simple improvement here.
>>>
>>> I do that to refresh the committer timestamp.
>>
>> I do, too.  The proposal is, paraphrasing,
>>
>> 	$ git commit --amend --no-edit
>> 	Ah, I see that you want me to refresh the committer timestamp.
>> 	Done, as requested.
>> 	$
>
> Ah, OK then.  I somehow misread "No changes." as an error message.

Well, on second thought, I think "fatal: no changes" that exits with
non-zero, with "--force" as an escape hatch for those who want to
refresh the committer timestamp, would probably be more in line with
the expectation Lukas had when this thread was started, and I further
suspect that it might be a bit more end-user friendly.

It is a backward incompatible behaviour, but I suspect that if I
were inventing "commit --amend" today, unlike 8588452c ("git-commit
--amend: allow empty commit.", 2006-03-04), I probably would design
it that way.  After all, failing and stopping is always a safer
option than going ahead with or without a report.

I am not sure which one between "go ahead anyway but report" and
"fail by default but allow forcing" I would prefer more.  At least
not yet.  But I won't rule the latter out at this point.

Thanks.

Re: amend warnings with no changes staged

[Jeff King]

On Mon, Aug 05, 2019 at 07:16:18PM -0700, Jonathan Nieder wrote:

> Lukas Gross wrote:
> 
> > I had intended to stage commits but forgot to do so. Git responded
> > with a normal commit creation message, so I pushed to the remote to
> > begin a CI build. When the build failed for the same reason, I
> > realized I had forgotten to stage the changes. An additional line in
> > the response to the effect of “Warning: did you mean to amend with no
> > changes?” would be very helpful to shorten this feedback loop.
> 
> On second thought:
> 
> 	$ git commit --amend --no-edit
> 	[detached HEAD 33a3db8805] Git 2.23-rc1
> 	 Author: Junio C Hamano <gitster@pobox.com>
> 	 Date: Fri Aug 2 13:12:24 2019 -0700
> 	 2 files changed, 2 insertions(+), 1 deletion(-)
> 	$
> 
> Some non-judgemental descriptive output like
> 
> 	$ git commit --amend --no-edit
> 	No changes.
> 	$
> 
> would address this case, without bothering people who are doing it
> intentionally.  So I think there's room for a simple improvement here.
> 
> Care to take a stab at it?  builtin/commit.c would be the place to
> start.

I'm not clear on the situation from your second change. There are two
sets of changes to talk about here: the changes between the new commit
and its parent, and the changes between the original commit and the
amended version.

The output in your first example is showing the differences to the
parent. Do you mean in the second example that there are no changes to
the parent, and thus we say "No changes"? If not, then what happened to
that output? :)

And if so, then I don't think it is solving Lukas's problem. I imagine
the issue to be (because I have done this myself many times):

  git commit -m 'buggy commit'
  echo fix >>file.c
  git commit --amend ;# oops, should have been "-a"
  git push

But perhaps that gets to the heart of the matter. Could we perhaps be
providing a more detailed summary of what happened for an --amend? I.e.,
to summarize _both_ sets of changes (and if one set is empty, say so)?

I'm not quite sure how to make that readable. Something like:

  $ git commit --amend
  [master 5787bce] some commit subject
   Date: Tue Aug 6 00:03:28 2019 -0400
  Changes introduced by this commit (diff HEAD^):
   1 file changed, 1 insertion(+)
   create mode 100644 added-by-broken-commit
  Changes from the amend commit (diff HEAD@{1}):
   1 file changed, 1 insertion(+)
   create mode 100644 added-during-amend

is pretty ugly. And anyway, because it's just the diffstat total, it's
hard to see whether it contains your changes or not (i.e., would you
notice if you forgot to stage 3 lines from some random file). OTOH, if
the common case we care about is just "you didn't stage anything as part
of the amend", then it would be enough to let you know (without making a
judgement about whether it's an error, since it may well be that you
were simply rewording the commit message).

-Peff

Re: amend warnings with no changes staged

[Phillip Wood]

On 06/08/2019 04:53, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>>>> Some non-judgemental descriptive output like
>>>>>
>>>>> 	$ git commit --amend --no-edit
>>>>> 	No changes.
>>>>> 	$
>>>>>
>>>>> would address this case, without bothering people who are doing it
>>>>> intentionally.  So I think there's room for a simple improvement here.
>>>>
>>>> I do that to refresh the committer timestamp.
>>>
>>> I do, too.  The proposal is, paraphrasing,
>>>
>>> 	$ git commit --amend --no-edit
>>> 	Ah, I see that you want me to refresh the committer timestamp.
>>> 	Done, as requested.
>>> 	$
>>
>> Ah, OK then.  I somehow misread "No changes." as an error message.
> 
> Well, on second thought, I think "fatal: no changes" that exits with
> non-zero, with "--force" as an escape hatch for those who want to
> refresh the committer timestamp, would probably be more in line with
> the expectation Lukas had when this thread was started, and I further
> suspect that it might be a bit more end-user friendly.

I agree that this would be the must user friendly way of doing it. I 
think refreshing the timestamp is probably a niche use of '--amend' (out 
of interest what the the motivation for doing that?) although it does 
seem to popular with the other contributors to this thread. We could 
always have a transition period with an opt-in config variable. I find 
the current behavior quite annoying when I've forgotten to stage 
anything as there is no indication the the commit's content is 
unchanged. I've been using a wrapper script that errors out if there are 
no new changes staged with --amend --no-edit and found it very helpful 
(as is a proper --reword option rather than having to give --amend --only)

Best Wishes

Phillip

> 
> It is a backward incompatible behaviour, but I suspect that if I
> were inventing "commit --amend" today, unlike 8588452c ("git-commit
> --amend: allow empty commit.", 2006-03-04), I probably would design
> it that way.  After all, failing and stopping is always a safer
> option than going ahead with or without a report.
> 
> I am not sure which one between "go ahead anyway but report" and
> "fail by default but allow forcing" I would prefer more.  At least
> not yet.  But I won't rule the latter out at this point.
> 
> Thanks.
> 

Re: amend warnings with no changes staged

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   git commit -m 'buggy commit'
>   echo fix >>file.c
>   git commit --amend ;# oops, should have been "-a"
>   git push
>
> But perhaps that gets to the heart of the matter. Could we perhaps be
> providing a more detailed summary of what happened for an --amend? I.e.,
> to summarize _both_ sets of changes (and if one set is empty, say so)?
> ...
> judgement about whether it's an error, since it may well be that you
> were simply rewording the commit message).

Perhaps "git range-diff HEAD@{1}...HEAD" being an empty is a sign
that either the user intentionally or accidentally did not do
anything other than "touch"ing the commit.

"git commit --amend --[no-]range-diff" that shows what you changed
with the amending may be an interesting possibility; I am not yet
ready to seriously encourage anybody to explore it, though, because
"git diff HEAD@{1}" is much easier to see what code got changed, but
one (and probably only) downside is that it does not cover the
change in the log message.

Re: amend warnings with no changes staged

[Jeff King]

On Tue, Aug 06, 2019 at 12:11:53PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >   git commit -m 'buggy commit'
> >   echo fix >>file.c
> >   git commit --amend ;# oops, should have been "-a"
> >   git push
> >
> > But perhaps that gets to the heart of the matter. Could we perhaps be
> > providing a more detailed summary of what happened for an --amend? I.e.,
> > to summarize _both_ sets of changes (and if one set is empty, say so)?
> > ...
> > judgement about whether it's an error, since it may well be that you
> > were simply rewording the commit message).
> 
> Perhaps "git range-diff HEAD@{1}...HEAD" being an empty is a sign
> that either the user intentionally or accidentally did not do
> anything other than "touch"ing the commit.
> 
> "git commit --amend --[no-]range-diff" that shows what you changed
> with the amending may be an interesting possibility; I am not yet
> ready to seriously encourage anybody to explore it, though, because
> "git diff HEAD@{1}" is much easier to see what code got changed, but
> one (and probably only) downside is that it does not cover the
> change in the log message.

I hadn't even thought of range-diff.

Showing the commit message diff would be a big improvement. A range-diff
would also show an update to the author (e.g., if you used --reset-author).

We don't really need the full power of range diff, though. After all, we
know there are exactly two patches to compare, so we don't care about it
trying to figure out which ones correlate (and it might even be a bad
thing for it to decide that two entries don't match). So I think we'd
probably want our own custom thing.

-Peff