[PATCH v2] commit, status: #comment diff output in verbose mode

[PATCH v2] commit, status: #comment diff output in verbose mode

[Ian Ward Comfort]

By historical accident, diffs included in commit templates and status
output when the "-v" option is given are not prefixed with the # comment
character, as other advice and status information is. Stripping these
lines is thus a best-effort operation, as it is not always possible to
tell which lines were generated by "-v" and which were inserted by the
user.

Improve this situation by adding the # prefix to diff output along with
all other status output in these cases. The change is simply made thanks
to a3c158d (Add a prefix output callback to diff output, 2010-05-26). The
prefixed diff can be stripped (or not, as configured) by the standard
cleanup code, so our special verbose-mode heuristic can be removed.

Documentation and a few tests which rely on the old "-v" format are
updated to match. One known breakage is fixed in t7507.

Signed-off-by: Ian Ward Comfort <icomfort@stanford.edu>
---
Resending this patch from the "commit notes workflow" thread ($gmane/168387)
since I didn't see it in "What's cooking". v2 changes only the placement of
sed in t4030, to match surrounding tests a little better.

(If there's no interest in this change, I'll drop it.)

 Documentation/git-commit.txt |    3 +--
 builtin/commit.c             |    7 -------
 t/t4030-diff-textconv.sh     |    2 +-
 t/t7502-commit.sh            |    4 ++--
 t/t7507-commit-verbose.sh    |    4 ++--
 wt-status.c                  |   12 ++++++++++++
 6 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 8f89f6f..792f993 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -233,8 +233,7 @@ configuration variable documented in linkgit:git-config[1].
 --verbose::
 	Show unified diff between the HEAD commit and what
 	would be committed at the bottom of the commit message
-	template.  Note that this diff output doesn't have its
-	lines prefixed with '#'.
+	template.
 
 -q::
 --quiet::
diff --git a/builtin/commit.c b/builtin/commit.c
index 355b2cb..efecac3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1381,13 +1381,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		die("could not read commit message: %s", strerror(saved_errno));
 	}
 
-	/* Truncate the message just before the diff, if any. */
-	if (verbose) {
-		p = strstr(sb.buf, "\ndiff --git ");
-		if (p != NULL)
-			strbuf_setlen(&sb, p - sb.buf + 1);
-	}
-
 	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
 	if (message_is_empty(&sb) && !allow_empty_message) {
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 88c5619..8d82faa 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -78,7 +78,7 @@ test_expect_success 'format-patch produces binary' '
 
 test_expect_success 'status -v produces text' '
 	git reset --soft HEAD^ &&
-	git status -v >diff &&
+	git status -v | sed -e "s/^# //" >diff &&
 	find_diff <diff >actual &&
 	test_cmp expect.text actual &&
 	git reset --soft HEAD@{1}
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 50da034..a916001 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -151,8 +151,8 @@ test_expect_success 'verbose' '
 
 	echo minus >negative &&
 	git add negative &&
-	git status -v | sed -ne "/^diff --git /p" >actual &&
-	echo "diff --git a/negative b/negative" >expect &&
+	git status -v | sed -ne "/^# diff --git /p" >actual &&
+	echo "# diff --git a/negative b/negative" >expect &&
 	test_cmp expect actual
 
 '
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index da5bd3b..5b21bbb 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -5,7 +5,7 @@ test_description='verbose commit template'
 
 cat >check-for-diff <<EOF
 #!$SHELL_PATH
-exec grep '^diff --git' "\$1"
+exec grep '^# diff --git' "\$1"
 EOF
 chmod +x check-for-diff
 test_set_editor "$PWD/check-for-diff"
@@ -65,7 +65,7 @@ test_expect_success 'diff in message is retained without -v' '
 	check_message diff
 '
 
-test_expect_failure 'diff in message is retained with -v' '
+test_expect_success 'diff in message is retained with -v' '
 	git commit --amend -F diff -v &&
 	check_message diff
 '
diff --git a/wt-status.c b/wt-status.c
index a82b11d..fc0063e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -32,6 +32,12 @@ static const char *color(int slot, struct wt_status *s)
 	return c;
 }
 
+static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)
+{
+	assert(data);
+	return (struct strbuf *)data;
+}
+
 void wt_status_prepare(struct wt_status *s)
 {
 	unsigned char sha1[20];
@@ -588,6 +594,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;
+	struct strbuf diff_output_prefix = STRBUF_INIT;
 
 	init_revisions(&rev, NULL);
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
@@ -596,10 +603,14 @@ static void wt_status_print_verbose(struct wt_status *s)
 	opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;
 	setup_revisions(0, NULL, &rev, &opt);
 
+	strbuf_addstr(&diff_output_prefix, "# ");
+
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.file = s->fp;
 	rev.diffopt.close_file = 0;
+	rev.diffopt.output_prefix = diff_output_prefix_callback;
+	rev.diffopt.output_prefix_data = &diff_output_prefix;
 	/*
 	 * If we're not going to stdout, then we definitely don't
 	 * want color, since we are going to the commit message
@@ -609,6 +620,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 	if (s->fp != stdout)
 		DIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);
 	run_diff_index(&rev, 1);
+	strbuf_release(&diff_output_prefix);
 }
 
 static void wt_status_print_tracking(struct wt_status *s)
-- 
1.7.3.2

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Jeff King]

On Thu, Mar 10, 2011 at 11:59:00AM -0800, Ian Ward Comfort wrote:

> By historical accident, diffs included in commit templates and status
> output when the "-v" option is given are not prefixed with the # comment
> character, as other advice and status information is. Stripping these
> lines is thus a best-effort operation, as it is not always possible to
> tell which lines were generated by "-v" and which were inserted by the
> user.
> 
> Improve this situation by adding the # prefix to diff output along with
> all other status output in these cases. The change is simply made thanks
> to a3c158d (Add a prefix output callback to diff output, 2010-05-26). The
> prefixed diff can be stripped (or not, as configured) by the standard
> cleanup code, so our special verbose-mode heuristic can be removed.
> 
> Documentation and a few tests which rely on the old "-v" format are
> updated to match. One known breakage is fixed in t7507.

One reason to keep the existing behavior is that editors will tend to
syntax-highlight the diff portion without much extra effort (in vim, at
least, the syntax highlighting just includes the diff syntax
highlighting for that section). I have no idea if this would make things
much harder for that case or not.

Even if it does make things harder there, I am not sure that the
increased robustness isn't more important, anyway. But I thought I would
point it out.

-Peff

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[SZEDER Gábor]

On Thu, Mar 10, 2011 at 05:52:33PM -0500, Jeff King wrote:
> On Thu, Mar 10, 2011 at 11:59:00AM -0800, Ian Ward Comfort wrote:
> 
> > By historical accident, diffs included in commit templates and status
> > output when the "-v" option is given are not prefixed with the # comment
> > character, as other advice and status information is. Stripping these
> > lines is thus a best-effort operation, as it is not always possible to
> > tell which lines were generated by "-v" and which were inserted by the
> > user.
> > 
> > Improve this situation by adding the # prefix to diff output along with
> > all other status output in these cases. The change is simply made thanks
> > to a3c158d (Add a prefix output callback to diff output, 2010-05-26). The
> > prefixed diff can be stripped (or not, as configured) by the standard
> > cleanup code, so our special verbose-mode heuristic can be removed.
> > 
> > Documentation and a few tests which rely on the old "-v" format are
> > updated to match. One known breakage is fixed in t7507.
> 
> One reason to keep the existing behavior is that editors will tend to
> syntax-highlight the diff portion without much extra effort (in vim, at
> least, the syntax highlighting just includes the diff syntax
> highlighting for that section). I have no idea if this would make things
> much harder for that case or not.
> 
> Even if it does make things harder there, I am not sure that the
> increased robustness isn't more important, anyway. But I thought I would
> point it out.

We had robustness issues with 'git commit -v' in the past, and Junio's
suggestion was to use "# Everything under this line is deleted." at
the beginning of the commit message template, and do so after the
editor exits.  That would work with syntax highlighting (well, at
least with vim), and perhaps isn't any less robust than prefixing the
diff output with #.

http://thread.gmane.org/gmane.comp.version-control.git/100525/focus=100655


Best,
Gábor

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> One reason to keep the existing behavior is that editors will tend to
> syntax-highlight the diff portion without much extra effort (in vim, at
> least, the syntax highlighting just includes the diff syntax
> highlighting for that section).

Hmm, thanks for pointing it out; it indeed is a valid concern.

Although I usually strongly resist changes in order to keep the user
experience stable, I didn't think about this one, as I don't let the
editor syntax highlight anything.

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Jeff King]

On Thu, Mar 10, 2011 at 04:45:14PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > One reason to keep the existing behavior is that editors will tend to
> > syntax-highlight the diff portion without much extra effort (in vim, at
> > least, the syntax highlighting just includes the diff syntax
> > highlighting for that section).
> 
> Hmm, thanks for pointing it out; it indeed is a valid concern.
> 
> Although I usually strongly resist changes in order to keep the user
> experience stable, I didn't think about this one, as I don't let the
> editor syntax highlight anything.

I like the proposal for:

  # Lines below this one will be removed.
  diff --git ...

which seems to have the best of both worlds, robust and easy for editors
to recognize as a diff. For that matter, we could also do "# Lines below
this one..." for _all_ of the git-status template, but I don't think
it's necessary. Those lines are already clearly marked with a delimiter,
and I don't think anybody is complaining about them (and the "Lines
below this one..." line adds just one more line of cruft).

-Peff

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Jeff King]

On Thu, Mar 10, 2011 at 08:23:18PM -0500, Jeff King wrote:

> I like the proposal for:
> 
>   # Lines below this one will be removed.
>   diff --git ...
> 
> which seems to have the best of both worlds, robust and easy for editors
> to recognize as a diff. For that matter, we could also do "# Lines below
> this one..." for _all_ of the git-status template, but I don't think
> it's necessary. Those lines are already clearly marked with a delimiter,
> and I don't think anybody is complaining about them (and the "Lines
> below this one..." line adds just one more line of cruft).

Hmm, actually the proposal that Gábor mentioned here:

  http://thread.gmane.org/gmane.comp.version-control.git/100525/focus=100655

was to mark the whole status template as "everything below this line is
uninteresting". And I was wrong that it would add one more line of
cruft; we already have a line saying "lines with '#' will be ignored",
so it would be replacing it.

I do still think I prefer the "#" as comment lines, though. Editors
understand that concept pretty well. For example, one thing that happens
to me a lot is that I write a paragraph, then edit it, then ask the
editor to re-wrap it. Inevitably it buts against the "#" lines, and
those get re-wrapped, too. I could fix it, of course, but I don't bother
because the editor knows that the stuff on "#" lines should remain on
"#" lines. So as it is now, the git-status output gets scrambled, but I
don't have to care. With a special "# Lines below this one..." line, I
will have mangled it and get extra cruft in my commit message.

But I admit that this is one pretty bizarre personal anecdote and might
not affect anyone else.

-Peff

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Michael J Gruber]

Jeff King venit, vidit, dixit 11.03.2011 06:31:
> On Thu, Mar 10, 2011 at 08:23:18PM -0500, Jeff King wrote:
> 
>> I like the proposal for:
>>
>>   # Lines below this one will be removed.
>>   diff --git ...
>>
>> which seems to have the best of both worlds, robust and easy for editors
>> to recognize as a diff. For that matter, we could also do "# Lines below
>> this one..." for _all_ of the git-status template, but I don't think
>> it's necessary. Those lines are already clearly marked with a delimiter,
>> and I don't think anybody is complaining about them (and the "Lines
>> below this one..." line adds just one more line of cruft).
> 
> Hmm, actually the proposal that Gábor mentioned here:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/100525/focus=100655
> 
> was to mark the whole status template as "everything below this line is
> uninteresting". And I was wrong that it would add one more line of
> cruft; we already have a line saying "lines with '#' will be ignored",
> so it would be replacing it.
> 
> I do still think I prefer the "#" as comment lines, though. Editors
> understand that concept pretty well. For example, one thing that happens
> to me a lot is that I write a paragraph, then edit it, then ask the
> editor to re-wrap it. Inevitably it buts against the "#" lines, and
> those get re-wrapped, too. I could fix it, of course, but I don't bother
> because the editor knows that the stuff on "#" lines should remain on
> "#" lines. So as it is now, the git-status output gets scrambled, but I
> don't have to care. With a special "# Lines below this one..." line, I
> will have mangled it and get extra cruft in my commit message.

As long as we match for the first n characters of that line with n<60 or
so the rewrapping will do no harm (assuming you leave it to start a new
paragraph, i.e. "^#Lines..." stays "^#Lines...").

> 
> But I admit that this is one pretty bizarre personal anecdote and might
> not affect anyone else.

What affects me more is when when I track files in a different encoding
(latin1, say), the diff triggers that encoding for vim and I end up with
encoding issues for the commit message (which is supposed to be utf8)...

Michael

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Piotr Krukowiecki]

On Fri, Mar 11, 2011 at 2:23 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 10, 2011 at 04:45:14PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > One reason to keep the existing behavior is that editors will tend to
>> > syntax-highlight the diff portion without much extra effort (in vim, at
>> > least, the syntax highlighting just includes the diff syntax
>> > highlighting for that section).
>>
>> Hmm, thanks for pointing it out; it indeed is a valid concern.
>>
>> Although I usually strongly resist changes in order to keep the user
>> experience stable, I didn't think about this one, as I don't let the
>> editor syntax highlight anything.

/me too - I find syntax highlighting a nice feature and would prefer it to
stay as it is over using #commented out diff


> I like the proposal for:
>
>  # Lines below this one will be removed.
>  diff --git ...
>
> which seems to have the best of both worlds, robust and easy for editors
> to recognize as a diff. For that matter, we could also do "# Lines below
> this one..." for _all_ of the git-status template, but I don't think
> it's necessary. Those lines are already clearly marked with a delimiter,
> and I don't think anybody is complaining about them

The advantage of using such line is that it's more unique - IMO it's less likely
someone writes a commit message with "# Lines below ..." etc then with
"diff --git".

It also makes possible to remove this line and thus include git diff output in
commit message.

The downside is probably the need to support i18n for "# Lines below ..."

Less magic formats (or formats less magic) is better IMO.

-- 
Piotr Krukowiecki

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Jeff King]

On Fri, Mar 11, 2011 at 09:49:30AM +0100, Michael J Gruber wrote:

> > I do still think I prefer the "#" as comment lines, though. Editors
> > understand that concept pretty well. For example, one thing that happens
> > to me a lot is that I write a paragraph, then edit it, then ask the
> > editor to re-wrap it. Inevitably it buts against the "#" lines, and
> > those get re-wrapped, too. I could fix it, of course, but I don't bother
> > because the editor knows that the stuff on "#" lines should remain on
> > "#" lines. So as it is now, the git-status output gets scrambled, but I
> > don't have to care. With a special "# Lines below this one..." line, I
> > will have mangled it and get extra cruft in my commit message.
> 
> As long as we match for the first n characters of that line with n<60 or
> so the rewrapping will do no harm (assuming you leave it to start a new
> paragraph, i.e. "^#Lines..." stays "^#Lines...").

Yeah, that would work in my case.

> > But I admit that this is one pretty bizarre personal anecdote and might
> > not affect anyone else.
> 
> What affects me more is when when I track files in a different encoding
> (latin1, say), the diff triggers that encoding for vim and I end up with
> encoding issues for the commit message (which is supposed to be utf8)...

Yuck. You may be literally feeding different charsets into a single
buffer of the editor. The best you could do is something like:

  au BufNewFile,BufRead COMMIT_EDITMSG set fenc=utf-8

and then for an empty commit message, vim will read in the latin1, and
then convert it to utf-8 on output. You will not have munged the "diff"
line, so git will still recognize it and remove everything after. But if
you are amending, then you will feed it a utf-8 commit message along
with a latin1 diff. And vim will screw that up when reading it in.

-Peff

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Michael J Gruber]

Jeff King venit, vidit, dixit 17.03.2011 08:37:
> On Fri, Mar 11, 2011 at 09:49:30AM +0100, Michael J Gruber wrote:
> 
>>> I do still think I prefer the "#" as comment lines, though. Editors
>>> understand that concept pretty well. For example, one thing that happens
>>> to me a lot is that I write a paragraph, then edit it, then ask the
>>> editor to re-wrap it. Inevitably it buts against the "#" lines, and
>>> those get re-wrapped, too. I could fix it, of course, but I don't bother
>>> because the editor knows that the stuff on "#" lines should remain on
>>> "#" lines. So as it is now, the git-status output gets scrambled, but I
>>> don't have to care. With a special "# Lines below this one..." line, I
>>> will have mangled it and get extra cruft in my commit message.
>>
>> As long as we match for the first n characters of that line with n<60 or
>> so the rewrapping will do no harm (assuming you leave it to start a new
>> paragraph, i.e. "^#Lines..." stays "^#Lines...").
> 
> Yeah, that would work in my case.
> 
>>> But I admit that this is one pretty bizarre personal anecdote and might
>>> not affect anyone else.
>>
>> What affects me more is when when I track files in a different encoding
>> (latin1, say), the diff triggers that encoding for vim and I end up with
>> encoding issues for the commit message (which is supposed to be utf8)...
> 
> Yuck. You may be literally feeding different charsets into a single
> buffer of the editor. The best you could do is something like:
> 
>   au BufNewFile,BufRead COMMIT_EDITMSG set fenc=utf-8
> 
> and then for an empty commit message, vim will read in the latin1, and
> then convert it to utf-8 on output. You will not have munged the "diff"
> line, so git will still recognize it and remove everything after. But if
> you are amending, then you will feed it a utf-8 commit message along
> with a latin1 diff. And vim will screw that up when reading it in.

I resorted to using

git config diff.latin1.textconv "iconv -f latin1"

and setting a latin1 attribute on my (php) files (which use
institute-wide templates enforcing latin1).

This gives me better diffs also, of course. I just hadn't bothered so far.

Though I'm wondering whether we should do something about it in general
(we assume utf8 commit messages, don't we), at least in doc. Your
suggestion above does make things safer (so that you don't screw up the
commit message accidentally) and would make a good patch to vim's
filetype.vim

Michael

Re: [PATCH v2] commit, status: #comment diff output in verbose mode

[Jeff King]

On Thu, Mar 17, 2011 at 09:01:33AM +0100, Michael J Gruber wrote:

> > Yuck. You may be literally feeding different charsets into a single
> > buffer of the editor. The best you could do is something like:
> > 
> >   au BufNewFile,BufRead COMMIT_EDITMSG set fenc=utf-8
>
> [...]
>
> Though I'm wondering whether we should do something about it in general
> (we assume utf8 commit messages, don't we), at least in doc. Your
> suggestion above does make things safer (so that you don't screw up the
> commit message accidentally) and would make a good patch to vim's
> filetype.vim

Being perfectly content with ASCII for my native language, I am not a
good person to judge. But I wonder how common your situation is. That
is, your files are all in latin1, but you continue to use utf8 for your
commit messages. Perhaps an easier solution would just be to tell git
that you want to make commit messages in latin1?

-Peff