Re: git commit -v does not removes the patch

"Santi Béjar" [off-list ref] writes:

On Mon, Nov 10, 2008 at 7:10 PM, Jeff King [off-list ref] wrote:

quoted

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

quoted

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

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?

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

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:

quoted

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

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

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 [off-list ref] wrote:

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

quoted

quoted

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

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

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.

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:

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

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

diff --git a/builtin-commit.c b/builtin-commit.c
index 93ca496..a721990 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c

@@ -1015,7 +1015,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Truncate the message just before the diff, if any. */
-	p = strstr(sb.buf, "\ndiff --git a/");
+	p = strstr(sb.buf, "\ndiff --git ");
 	if (p != NULL)
 		strbuf_setlen(&sb, p - sb.buf + 1);
 

But I have to wonder if there is some more robust solution. It seems
like this can have false positives if you include diff output in your
commit message, and a potential false negative if you delete the newline
(e.g., delete everything up to "diff --git", making it the first line).

But I guess we haven't seen a lot of complaints, so maybe those
conditions aren't worth worrying about.

-Peff

On Tue, Nov 11, 2008 at 11:29 AM, Jeff King [off-list ref] wrote:

It fixes it, thanks.

But I have to wonder if there is some more robust solution. It seems
like this can have false positives if you include diff output in your
commit message, and a potential false negative if you delete the newline
(e.g., delete everything up to "diff --git", making it the first line).

But I guess we haven't seen a lot of complaints, so maybe those
conditions aren't worth worrying about.

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

-Peff