threads / patch / 26705

v2commit, status: #comment diff output in verbose mode

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

## tl;dr

11 messages between Mar 10, 2011 and Mar 17, 2011. Diffs are folded; open one to read it.

replies: 10people: 6as markdown or json

Ian Ward Comfort· Mar 10, 2011, 19:59 UTC · lore

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(-)
Show changes to 6 files +18 −14

Documentation/git-commit.txt, builtin/commit.c, t/t4030-diff-textconv.sh, t/t7502-commit.sh, t/t7507-commit-verbose.sh, wt-status.c

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
Jeff King· Mar 10, 2011, 22:52 UTC · re: Ian Ward Comfort · lore

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

On Thu, Mar 10, 2011 at 11:59:00AM -0800, Ian Ward Comfort wrote:
Show 15 quoted lines
> 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
SZEDER Gábor· Mar 10, 2011, 23:57 UTC · re: Jeff King · lore

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

On Thu, Mar 10, 2011 at 05:52:33PM -0500, Jeff King wrote:
Show 27 quoted lines
> 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

Junio C Hamano· Mar 11, 2011, 00:45 UTC · re: Jeff King · lore

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

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.

Jeff King· Mar 11, 2011, 01:23 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 10, 2011 at 04:45:14PM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> 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
Jeff King· Mar 11, 2011, 05:31 UTC · re: Jeff King · lore

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

On Thu, Mar 10, 2011 at 08:23:18PM -0500, Jeff King wrote:
Show 11 quoted lines
> 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
Michael J Gruber· Mar 11, 2011, 08:49 UTC · re: Jeff King · lore

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

Jeff King venit, vidit, dixit 11.03.2011 06:31:
Show 32 quoted lines
> 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
Jeff King· Mar 17, 2011, 07:37 UTC · re: Michael J Gruber · lore

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

On Fri, Mar 11, 2011 at 09:49:30AM +0100, Michael J Gruber wrote:
Show 13 quoted lines
> > 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.
Show 6 quoted 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)...

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
Michael J Gruber· Mar 17, 2011, 08:01 UTC · re: Jeff King · lore

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

Jeff King venit, vidit, dixit 17.03.2011 08:37:
Show 35 quoted lines
> 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
Jeff King· Mar 17, 2011, 19:41 UTC · re: Michael J Gruber · lore

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

On Thu, Mar 17, 2011 at 09:01:33AM +0100, Michael J Gruber wrote:
Show 12 quoted lines
> > 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
Piotr Krukowiecki· Mar 13, 2011, 18:34 UTC · re: Jeff King · lore

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

On Fri, Mar 11, 2011 at 2:23 AM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> 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

Show 10 quoted lines
> 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

← back to recent threads