amend warnings with no changes staged

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!

Hi,

Lukas Gross wrote:

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

Thanks, Jonathan

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:

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

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

Lukas Gross wrote:

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

Jonathan Nieder <jrnieder@gmail.com> writes:

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 ;-)

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.

Junio C Hamano wrote:

[...]

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:

[...]

Not this.

Hoping that clarifies, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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

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.

On 06/08/2019 04:53, Junio C Hamano wrote:

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

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

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

Jeff King <peff@peff.net> writes:

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.

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

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