git commit -v does not removes the patch

Hi *,

  $subject since:

4f672ad (wt-status: load diff ui config, 2008-10-26)

I tried to make a test case, but failed. I think because it is a bit tricky the fake_editor/stdin/stdout stuff, so at the end I bisected it by hand

Regards, Santi

On Mon, Nov 10, 2008 at 04:20:51PM +0100, Santi Béjar wrote:

Sorry, I don't quite understand what the problem is. From reading your subject line, I expected that "git commit -v" would show the diff in your editor, but then accidentally also include it in the final commit message. But I can't seem to reproduce that.

Can you describe the problem in more detail?

-Peff

On Mon, Nov 10, 2008 at 7:10 PM, Jeff King <peff@peff.net> wrote:

It is exactly as you described. I'll try in other systems.

Santi

"Santi Béjar" <santi@agolina.net> writes:

Guess in the dark... by any chance are you enabling color unconditionally?

On Mon, Nov 10, 2008 at 03:27:18PM -0800, Junio C Hamano wrote:

I thought I covered that case explicitly...

...yes, I can't reproduce with color set to "always". And indeed, looking at the diff for 4f672ad shows that we always turn off color for the message that goes to the editor (since we can't trust our isatty setting _anyway_, since we're not going to the tty).

So that's not it.

-Peff

On Tue, Nov 11, 2008 at 1:07 AM, Jeff King <peff@peff.net> wrote:

Almost! I have diff.mnemonicprefix=true, if I unset it everything works.

Santi

On Tue, Nov 11, 2008 at 08:56:34AM +0100, Santi Béjar wrote:

Ah, indeed. The obvious fix is just loosening our match a little bit:

On Tue, Nov 11, 2008 at 11:29 AM, Jeff King <peff@peff.net> wrote:

It fixes it, thanks.

The false positive/negative were already possible, it was just a little more narrow. So not worth worrying about. Also, if you really want a diff in the commit message you can use the --cleanup option.

A more robust solution could be to have a:

# Everything after this line will be removed from the commit message

But, again, not worth.

Santi

Jeff King <peff@peff.net> writes:

Actually, I recall (from my googling quite some time ago) people noticing this, but they learnt to live with it.

We may want to change this. We can say "# Everything under this line is deleted." at the beginning of the "#" block we produce in the commit log message editor, replacing the "Lines starting with '#' will be ignored, " we currently have. When reading back the editor result, make "git commit -v" scan for the "# Everything ..." line. We remove it and everything that follows, but we do not touch anything above that line (including the ones that begin with "diff" or "#") except the usual trailing whitespace removal. That way, people can leave a sample shell session with root prompt, and sample diff, in their message.

If we do not see "# Everything ..." when we read it back, we can do what we currently do as a fallback.

On Tue, Nov 11, 2008 at 09:13:18AM -0800, Junio C Hamano wrote:

I am little hesitant to do this, because I think people have scripted minorly around the template format (at the very least, for syntax highlighting). I think some people may have pre-written templates, as well, though I guess your "If we do not see..." paragraph is meant to address that. Though that brings some confusion itself, because now the parsing rules for what is kept change if I delete that line.

Here's a patch series that at least improves the situation by turning off the diff-stripping if we never put in a diff in the first place. That way only people who actually _use_ "-v" will have to pay for it. It has the fix I sent to Santi earlier, as well as some related cleanups.

    1/5: define empty tree sha1 as a macro
    2/5: wt-status: refactor initial commit printing
    3/5: status: show "-v" diff even for initial commit
    4/5: commit: loosen pattern for matching "-v" diff
    5/5: commit: only strip diff from message in verbose mode

-Peff

This can potentially be used in a few places, so let's make it available to all parts of the code.

Signed-off-by: Jeff King <peff@peff.net>
---
I use the _HEX version in further patches, but since it is such a magic
number, I thought it made sense to keep all definitions in the same
place.

 cache.h     |    6 ++++++
 sha1_file.c |    4 +---
 2 files changed, 7 insertions(+), 3 deletions(-)

When we showed the initial commit, we had no reference to diff against, so we went through the cache manually. Nowadays, however, we have a virtual empty tree commit, so we can simply diff against that to get the same results.

Signed-off-by: Jeff King <peff@peff.net>
---
I ran across this when I realized I could do the same trick for the
verbose diff (which I will do in the next patch). I think it is a
worthwhile cleanup. Not only does it remove a lot of lines, but it gives
a single codepath for printing, which will be helpful if that codepath
ever changes (e.g., with the alternate status formats we talked about a
few weeks ago).

This could be made even simpler by setting s->reference to the empty tree when we are on the initial commit, but I think that is a little messy. The caller may have set s->reference to an arbitrary pointer, including one which needs free()d, and we would be overwriting that. As it happens, with the current callers there is no problem, but it seems like a bad interface.

 wt-status.c |   28 +++-------------------------
 1 files changed, 3 insertions(+), 25 deletions(-)

Since we can use the same "diff against empty tree" trick as we do for the non-initial case, it is trivial to make this work.

Signed-off-by: Jeff King <peff@peff.net>
---
I ran across this while writing my test cases for the later patches. I
don't see any reason why it shouldn't behave this way, and I suspect the
reason was just that the original author saw how we did
"wt_status_print_initial" and didn't want to have to do the equivalent
for generating diff output.

 t/t7507-commit-verbose.sh |   29 +++++++++++++++++++++++++++++
 wt-status.c               |    5 +++--
 2 files changed, 32 insertions(+), 2 deletions(-)
 create mode 100755 t/t7507-commit-verbose.sh

When a user asks to see the diff to be applied via "git commit -v", the diff ends up in the commit template, and we must later remove it.

To detect the start of the included diff, we used to search for a line beginning with "diff --git a/". However, in the face of diff.mnemonicprefix, that will actually be "diff --git i/".

So let's just loosen the pattern a bit to handle either case.

Signed-off-by: Jeff King <peff@peff.net>
---
And this is the fix from before, with a test case.

 builtin-commit.c          |    2 +-
 t/t7507-commit-verbose.sh |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)

When the "-v" option is given, we put diff of what is to be committed into the commit template, and then strip it back out again after the user has edited it.

We guess at the location of the diff by searching for the "diff --git" header. This means that if the user puts their own diff in the message (e.g., as a sample output), then we will accidentally trigger the pattern, removing part of their output.

We can avoid doing this stripping altogether if the user didn't use "-v" in the first place, so we know that any match we find will be a false positive.

Signed-off-by: Jeff King <peff@peff.net>
---
Obviously this doesn't solve the problem entirely, but it at least makes
a reasonable rule: if we accidentally eat your sample diff output with
"-v", then try without it. :)

But more importantly, I suspect "-v" is used infrequently, so in practice this should fix most cases.

 builtin-commit.c          |    8 +++++---
 t/t7507-commit-verbose.sh |   21 +++++++++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

On Wed, Nov 12, 2008 at 03:25:52AM -0500, Jeff King wrote:

I put in this expect_failure to remind us that we could be doing better. The solution you proposed would fix this test. Another idea I had was to use a bogus diff header, and match on that. Something like:

  diff --git --verbose-commit a/file b/file

and searching for "diff --git --verbose-commit", which would not trigger on any diffs (unless, of course, you were trying to explain how the verbose mode worked in your commit message :) ).

But that doesn't cover any other special input (i.e., lines beginning with '#'), and would probably require an ugly hack to the diff machinery (some diff option for "add these bogus diff command line options").

-Peff

Jeff King <peff@peff.net> writes:

Makes sense. Thanks, will queue for 'master' with minor futzing to lift minimum fix to 'maint'.

Hi,

On Wed, Nov 12, 2008 at 03:16:09AM -0500, Jeff King wrote:

I always want to see the diff while editing a commit message, even if it's the commit message of a merge, a revert, a squash in rebase. Since these commands do not have an option equivalent to 'git commit -v', I wrote a little prepare-commit-msg hook to always append the appropriate diff to the end of the appropriate commit message file. This worked pretty well for the last couple of months, until 5/5 got into maint.

So, what is the/is there a preferred way to always include the diff in the commit message template and get it removed automatically? Are there any workarounds other than revert that commit locally?

Thanks, Gábor

On Thu, Nov 20, 2008 at 02:09:28PM +0100, SZEDER Gábor wrote:

Hmm. I am sad that this change has broken somebody's existing workflow. OTOH, I'm not convinced that workflow wasn't a little crazy to begin with, depending on this undocumented munging.

How about:

    $ cat .git/hooks/prepare-commit-msg
    #!/bin/sh
    git diff --cached >>"$1"

    $ cat .git/hooks/commit-msg
    #!/bin/sh
    sed -i '/^diff --git/Q' "$1"

which is more or less the original behavior. The only downside I see is that "--no-verify" will turn off the commit-msg hook, but not the prepare-commit-msg hook.

-Peff

Hi Jeff,

On Thu, Nov 20, 2008 at 10:20:16AM -0500, Jeff King wrote:

Well, the docs didn't say that the patch is only removed in verbose mode (; But I agree with you that this is (was) an undocumented "feature" that I should not have relied upon.

Thanks for the tip. Junio's suggestion about '# Everything under this line is deleted.' could be implemented this way fairly easily (just an additional echo in prepare-commit-msg, and other pattern in commit-msg). But 'git commit --no-verify' is indeed a problem. Although I never use it explicitly, 'git rebase -i' does so.

However.

'git rebase -i' uses 'git commit --no-verify' since c5b09feb (Avoid update hook during git-rebase --interactive, 2007-12-19) to avoid squashing commits together when the pre-commit hook fails because of whitespace errors. But note that at that time the 'git commit' calls looked like this (after that patch):

  $USE_OUTPUT git commit --no-verify -F "$MSG" $EDIT_COMMIT
  git commit --no-verify -F "$DOTEST"/message -e

Witness that it's not checked whether 'git commit' succeeded or failed. However, nowadays those lines look like this:

  $USE_OUTPUT git commit --no-verify $MSG_OPT "$MSG_FILE" $EDIT_COMMIT || failed=t
  git commit --no-verify -F "$DOTEST"/message -e || { ...

There is a check for failed 'git commit', so if the pre-commit hook finds whitespace errors, then 'git commit' will error out, and, consequently, 'git rebase -i' will error out, too, and commits won't be squashed accidentaly.

So, I wonder whether those '--no-verify' options are still required in 'git rebase -i'.

Any thoughts?

Thanks, Gábor

On Sat, Nov 22, 2008 at 04:55:41PM +0100, SZEDER Gábor wrote:

One other option would be to munge the editor script instead of using hooks. I.e., something like this:

  $ cat ~/local/bin/git-editor
  #!/bin/sh
  echo '# Everything under this line is deleted' >>"$1"
  git diff --cached >>"$1"
  $EDITOR "$1" || exit $?
  sed -i '/^# Everything under this line is deleted$/Q' "$1"

  $ git config core.editor ~/local/bin/git-editor

Of course, then you end up with the munging for things like "git rebase -i" picking commits, but it should be harmless (since we end up removing it anyway).

-Peff

SZEDER Gábor <szeder@ira.uka.de> writes:

The use of --no-verify there does not have anything to do with "whitespace errors". It is to override _any_ validation the users want to do when using "git commit" in their interactive workflow. It so happens that the example hook we ship demonstrates how you hunt for whitespace errors, but you have to remember that it is just an example.

We may want to disable the checking of exit status from commit-msg hook while still calling the hook itself, though. The primary purpose of the hook is to allow users to reformat (say, passing "fmt -64") the message, but it is allowed to interfere and that was meant to happen when using "git commit" but we probably do not want it when rebasing.

Further, we also may want to make the use of --no-verify overridable from the command line when running rebase. The primary purpose of the rebase command is to transplant a sequence of commits to someplace else without molesting its contents and message unnecessarily, and --no-verify is a good thing to use in general for that reason, but people may sometimes want to use it as a way to clean up the changes.