Re: git mailinfo strips important context from patch subjects

Jeff King [off-list ref] writes:

On Sun, Jun 28, 2009 at 08:38:58PM +0100, Roger Leigh wrote:

quoted

In most of the projects I work on, the git commit message has
the affected subsystem or component in square brackets, such as

  [foo] change bar to baz

[...]

The [sbuild] prefix has been dropped from the Subject, so an
important bit of context about the patch has been lost.

It's a bit of a bug that you can't round trip from a git-format-patch
to import with git-am and then not be able to produce the exact same
patch set with git-format-patch again (assuming preparing and applying
to the same point, of course).

As an immediate solution, you probably want to use "-k" when generating
the patch (not to add the [PATCH] munging) and "-k" when reading the
patch via "git am" (which will avoid trying to strip any munging).

However:

quoted

Would it be possible to change the git-mailinfo logic to use a less
greedy pattern match so it leaves everything after
([PATCH( [0-9/])+])+ in the subject?  AFAICT this is cleanup_subject in
builtin-mailinfo.c?  Could this rather complex function not just do a
simple regex match which can also take care of stripping ([Rr]e:) ?

Yes, I think in the long run it makes sense to strip just the _first_
set of brackets. I don't think we want to be more specific than that in
the match, because we allow arbitrary cruft inside the brackets (like
"[RFC/PATCH]", etc). But if format-patch always puts exactly one set of
brackets, and am strips exactly one set, then that should retain your
subject in practice, even if it starts with [foo].

I think it may still make sense to insist that PATCH appears somewhere in
the first set of brackets, but I have stop and wonder if it is even
necessary.

Because git removes [sbuild] at the beginning, Roger is unhappy.

 * Is he happy that git removes [PATCH]?  In E-mail based workflow it is
   a good practice to mark messages that are patches clearly so that they
   can be quickly found among the discussions that lead to them, and it is
   plausible that his project accepted that as an established practice
   supported well by git.

 * Is he happy that git treats the first paragraph of the commit message
   specially from the rest of the message?  In a project with many
   commits, it is essential that people write good commit summaries that
   fits on a single line so that tools like shortlog and gitweb can be
   used to get a bird-eye view of what happened recently.  Perhaps his
   project picked it up as the best current practice supported well by
   git.

 * Is he happy that git takes "---" as the end of message marker, so that
   any other commentary can be added to the message to facilitate the
   communication without adding noise to the commits?  Perhaps he is and
   his project picked it up as a good practice supported well by git.

There are many other conventions in git that does not have anything to do
with what the underlying git datastructure supports, but conventions can
always be seen as "don't do that, instead do it this way", limitations,
and to some of them Roger may not be happy.  Where would we draw a line?

_An_ established (note that I did not say _the_ nor _best current_)
practice supported well by git to note the area being affected in a
project of nontrivial size is to prefix the single line summary with the
name of the area followed by a colon.  There is no difference between
"[sbuild] foo" and "sbuild: foo" at the information content point-of-view,
but the latter has an advantage of being one letter shorter and less
distracting in MUA.  He does not have a very strong reason to choose
something different only to make his life harder, does he?

Users can take advantage of this established practice when running
shortlog with "--grep=^area:" to limit the birds-eye-view to a specific
area.  If this turns out to be useful, we could even add an option to "git
log --area=name" that limits this kind of match to the first paragraph of
the commit log message, for example.

Supporting a slightly different convention may seem to be accomodating and
nice, but if there is no real technical difference between the two (and
again, "area:" is one letter shorter ;-), letting people run with
different convention longer, when they can switch easily to another
convention that is already well supported, may actually hurt them in the
long run.  "[sbuild]" will not match "--area=sbuild" that will internally
become "--grep-only-first-line=sbuild:" so either he will miss out
benefiting from the new feature, or the implementation of the new feature
unnecessarily needs more code.

It is not about discouraging a wrong workflow or practice, because there
is nothing _wrong_ per-se in [sbuild] prefix.  It is just that it makes
things harder in the long run.  In this particular case, it is only very
slightly harder, but these things tend to add up from different fronts.

Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Sun, Jun 28, 2009 at 08:38:58PM +0100, Roger Leigh wrote:

quoted

In most of the projects I work on, the git commit message has
the affected subsystem or component in square brackets, such as

  [foo] change bar to baz

[...]

The [sbuild] prefix has been dropped from the Subject, so an
important bit of context about the patch has been lost.

It's a bit of a bug that you can't round trip from a git-format-patch
to import with git-am and then not be able to produce the exact same
patch set with git-format-patch again (assuming preparing and applying
to the same point, of course).

As an immediate solution, you probably want to use "-k" when generating
the patch (not to add the [PATCH] munging) and "-k" when reading the
patch via "git am" (which will avoid trying to strip any munging).

However:

quoted

Would it be possible to change the git-mailinfo logic to use a less
greedy pattern match so it leaves everything after
([PATCH( [0-9/])+])+ in the subject?  AFAICT this is cleanup_subject in
builtin-mailinfo.c?  Could this rather complex function not just do a
simple regex match which can also take care of stripping ([Rr]e:) ?

Yes, I think in the long run it makes sense to strip just the _first_
set of brackets. I don't think we want to be more specific than that in
the match, because we allow arbitrary cruft inside the brackets (like
"[RFC/PATCH]", etc). But if format-patch always puts exactly one set of
brackets, and am strips exactly one set, then that should retain your
subject in practice, even if it starts with [foo].

I think it may still make sense to insist that PATCH appears somewhere in
the first set of brackets, but I have stop and wonder if it is even
necessary.

Because git removes [sbuild] at the beginning, Roger is unhappy.

[ and a lot more ]

_An_ established (note that I did not say _the_ nor _best current_)
practice supported well by git to note the area being affected in a
project of nontrivial size is to prefix the single line summary with the
name of the area followed by a colon.  There is no difference between
"[sbuild] foo" and "sbuild: foo" at the information content point-of-view,
but the latter has an advantage of being one letter shorter and less
distracting in MUA.  He does not have a very strong reason to choose
something different only to make his life harder, does he?

True, but it seems wrong to have am remove more of the subject than
format-patch prepends. Imagine a commit subject looking like this:
  "Allow [ and ] in the blurble.foostuff table".

Should am strip the subject all the way up to the last ']'? I think
not, and I'd be very vexed if it did.

Users can take advantage of this established practice when running
shortlog with "--grep=^area:" to limit the birds-eye-view to a specific
area.  If this turns out to be useful, we could even add an option to "git
log --area=name" that limits this kind of match to the first paragraph of
the commit log message, for example.

Supporting a slightly different convention may seem to be accomodating and
nice, but if there is no real technical difference between the two (and
again, "area:" is one letter shorter ;-), letting people run with
different convention longer, when they can switch easily to another
convention that is already well supported, may actually hurt them in the
long run.  "[sbuild]" will not match "--area=sbuild" that will internally
become "--grep-only-first-line=sbuild:" so either he will miss out
benefiting from the new feature, or the implementation of the new feature
unnecessarily needs more code.

It is not about discouraging a wrong workflow or practice, because there
is nothing _wrong_ per-se in [sbuild] prefix.  It is just that it makes
things harder in the long run.  In this particular case, it is only very
slightly harder, but these things tend to add up from different fronts.

Agreed, but there are valid use-cases orthogonal to subsystem naming to
place [] in the patch subject. I still feel that since format-patch only
adds one set, am (mailinfo) should really only remove one set, too. It's
what makes sense, really.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

git-format-patch prepends patches with a [PATCH x/n] prefix, but
mailinfo used to remove any number of square-bracket pairs and
the content between them. This prevents one from using a commit
subject like this:

  [ and ] must be allowed as input

Removing the square bracket pair from this rather clumsily
constructed subject line loses important information, so we must
take care not to.

This patch causes the subject stripping to stop after it has
encountered one pair of square brackets.

One possible downside of this patch is that the patch-handling
programs will now fail at removing author-added square-brackets
to be removed, such as

  [RFC][PATCH x/n]

However, since format-patch only adds one set of square brackets,
this behaviour is quite easily undesrstood and defended while the
previous behaviour is not.

Signed-off-by: Andreas Ericsson <redacted>
---
 builtin-mailinfo.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 92637ac..fb5ad70 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c

@@ -221,6 +221,8 @@ static void cleanup_subject(struct strbuf *subject)
 {
 	char *pos;
 	size_t remove;
+	int brackets_removed = 0;
+
 	while (subject->len) {
 		switch (*subject->buf) {
 		case 'r': case 'R':

@@ -235,10 +237,15 @@ static void cleanup_subject(struct strbuf *subject)
 			strbuf_remove(subject, 0, 1);
 			continue;
 		case '[':
+			/* remove only one set of square brackets */
+			if (brackets_removed)
+				break;
+
 			if ((pos = strchr(subject->buf, ']'))) {
 				remove = pos - subject->buf;
 				if (remove <= (subject->len - remove) * 2) {
 					strbuf_remove(subject, 0, remove + 1);
+					brackets_removed = 1;
 					continue;
 				}
 			} else

-- 
1.6.3.3.354.gfb24

Andreas Ericsson [off-list ref] writes:

git-format-patch prepends patches with a [PATCH x/n] prefix, but
mailinfo used to remove any number of square-bracket pairs and
the content between them. This prevents one from using a commit
subject like this:

  [ and ] must be allowed as input

Removing the square bracket pair from this rather clumsily
constructed subject line loses important information, so we must
take care not to.

This patch causes the subject stripping to stop after it has
encountered one pair of square brackets.

One possible downside of this patch is that the patch-handling
programs will now fail at removing author-added square-brackets
to be removed, such as

  [RFC][PATCH x/n]

However, since format-patch only adds one set of square brackets,
this behaviour is quite easily undesrstood and defended while the
previous behaviour is not.

Signed-off-by: Andreas Ericsson <redacted>
---

All good points, and I like this one, including its Subject: line.

Use a regular expression to match text after "Re:" or any text in the
first pair of square brackets such as "[PATCH n/m]".  This replaces
the complex hairy string munging with a simple single  pattern match.

Signed-off-by: Roger Leigh <redacted>
---
 builtin-mailinfo.c |   61 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 92637ac..6d19046 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c

@@ -219,35 +219,42 @@ static int is_multipart_boundary(const struct strbuf *line)
 
 static void cleanup_subject(struct strbuf *subject)
 {
-	char *pos;
-	size_t remove;
-	while (subject->len) {
-		switch (*subject->buf) {
-		case 'r': case 'R':
-			if (subject->len <= 3)
-				break;
-			if (!memcmp(subject->buf + 1, "e:", 2)) {
-				strbuf_remove(subject, 0, 3);
-				continue;
-			}
-			break;
-		case ' ': case '\t': case ':':
-			strbuf_remove(subject, 0, 1);
-			continue;
-		case '[':
-			if ((pos = strchr(subject->buf, ']'))) {
-				remove = pos - subject->buf;
-				if (remove <= (subject->len - remove) * 2) {
-					strbuf_remove(subject, 0, remove + 1);
-					continue;
-				}
-			} else
-				strbuf_remove(subject, 0, 1);
-			break;
-		}
+	int status;
+	regex_t regex;
+	regmatch_t match[4];
+
+	/* Strip off 'Re:' and/or the first text in square brackets, such as
+	   '[PATCH]' at the start of the mail Subject. */
+	status = regcomp(&regex,
+			 "^([Rr]e:)?([^]]*\\[[^]]+\\])(.*)$",
+			 REG_EXTENDED);
+
+	if (status) {
+		/* Compiling the regex failed.  Find out why and tell
+		   the user.  This is always a bug in the code. */
+		int esize = regerror(status, &regex, NULL, 0);
+		struct strbuf etext = STRBUF_INIT;
+
+		strbuf_grow(&etext, esize);
+		regerror(status, &regex, etext.buf, esize);
+		fprintf (stderr,
+			 "Error compiling regular expression: %s\n",
+			 etext.buf);
+		strbuf_release(&etext);
+		exit(1);
+	}
+
+	/* Store any matches in match. */
+	status = regexec(&regex, subject->buf, 4, match, 0);
+
+	/* If there was a match for \3 in the regex, trim the subject
+	   to this match. */
+	if (!status && match[3].rm_so > 0) {
+		strbuf_remove(subject, 0, match[3].rm_so);
 		strbuf_trim(subject);
-		return;
 	}
+
+	return;
 }
 
 static void cleanup_space(struct strbuf *sb)

-- 
1.6.3.3

Roger Leigh [off-list ref] writes:

Use a regular expression to match text after "Re:" or any text in the
first pair of square brackets such as "[PATCH n/m]".  This replaces
the complex hairy string munging with a simple single  pattern match.

[...]

+	/* Strip off 'Re:' and/or the first text in square brackets, such as
+	   '[PATCH]' at the start of the mail Subject. */
+	status = regcomp(&regex,
+			 "^([Rr]e:)?([^]]*\\[[^]]+\\])(.*)$",
+			 REG_EXTENDED);

Sidenote: it probably didn't worked before either, but there are some
broken mail readers in the wold (*cough* MS Outlook *cough*), that
misinterpret RFCs and use translated form of "Re:" e.g. "Odp:" (Polish),
or not strip "Re:" when replying resulting in string of "Re: Re: Re: ...",
or use capitalized form of "Re:", i.e. "RE:", or use yet another form 
e.g. compact form of repeated "Re: Re: Re: ..." in form of "Re(3):".

But I guess it didn't worked before either.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Signed-off-by: Roger Leigh <redacted>
---
 builtin-mailinfo.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 6d19046..6559c37 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c

@@ -254,6 +254,8 @@ static void cleanup_subject(struct strbuf *subject)
 		strbuf_trim(subject);
 	}
 
+	regfree(&regex);
+
 	return;
 }
 

-- 
1.6.3.3

On Sun, Jun 28, 2009 at 04:04:37PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Sun, Jun 28, 2009 at 08:38:58PM +0100, Roger Leigh wrote:

quoted

In most of the projects I work on, the git commit message has
the affected subsystem or component in square brackets, such as

  [foo] change bar to baz

[...]

The [sbuild] prefix has been dropped from the Subject, so an
important bit of context about the patch has been lost.

It's a bit of a bug that you can't round trip from a git-format-patch
to import with git-am and then not be able to produce the exact same
patch set with git-format-patch again (assuming preparing and applying
to the same point, of course).

As an immediate solution, you probably want to use "-k" when generating
the patch (not to add the [PATCH] munging) and "-k" when reading the
patch via "git am" (which will avoid trying to strip any munging).

However:

quoted

Would it be possible to change the git-mailinfo logic to use a less
greedy pattern match so it leaves everything after
([PATCH( [0-9/])+])+ in the subject?  AFAICT this is cleanup_subject in
builtin-mailinfo.c?  Could this rather complex function not just do a
simple regex match which can also take care of stripping ([Rr]e:) ?

Yes, I think in the long run it makes sense to strip just the _first_
set of brackets. I don't think we want to be more specific than that in
the match, because we allow arbitrary cruft inside the brackets (like
"[RFC/PATCH]", etc). But if format-patch always puts exactly one set of
brackets, and am strips exactly one set, then that should retain your
subject in practice, even if it starts with [foo].

I think it may still make sense to insist that PATCH appears somewhere in
the first set of brackets, but I have stop and wonder if it is even
necessary.

I imagine not.  I've submitted a patch separately which implements
this behaviour (more on that below).

Because git removes [sbuild] at the beginning, Roger is unhappy.

 * Is he happy that git removes [PATCH]?  In E-mail based workflow it is
   a good practice to mark messages that are patches clearly so that they
   can be quickly found among the discussions that lead to them, and it is
   plausible that his project accepted that as an established practice
   supported well by git.

I'm perfectly happy that [PATCH] is removed.  My requirement is that
the commit created by "git am" is identical to the commit represented
in the patch created with "git format-patch".  The removal of this
is IMO correct, and I agree that it's presence is useful in an email-
based workflow.

 * Is he happy that git treats the first paragraph of the commit message
   specially from the rest of the message?  In a project with many
   commits, it is essential that people write good commit summaries that
   fits on a single line so that tools like shortlog and gitweb can be
   used to get a bird-eye view of what happened recently.  Perhaps his
   project picked it up as the best current practice supported well by
   git.

I'm also happy with this, and make use of it.  As for the previous
paragraph, I would like the commit message to be preserved correctly
so that the message committed by "git am" matches the original
commit message exactly.

 * Is he happy that git takes "---" as the end of message marker, so that
   any other commentary can be added to the message to facilitate the
   communication without adding noise to the commits?  Perhaps he is and
   his project picked it up as a good practice supported well by git.

This sounds just fine, though I have not yet had the need to use it.

_An_ established (note that I did not say _the_ nor _best current_)
practice supported well by git to note the area being affected in a
project of nontrivial size is to prefix the single line summary with the
name of the area followed by a colon.  There is no difference between
"[sbuild] foo" and "sbuild: foo" at the information content point-of-view,
but the latter has an advantage of being one letter shorter and less
distracting in MUA.  He does not have a very strong reason to choose
something different only to make his life harder, does he?

Well, I sometimes use the format

  [foo] bar: baz

but my more general point was not my specific usage but that the
existing behaviour was causing loss of information.  I think it
would be preferable to guarantee that data from the original
commit is not lost and is preserved exactly if at all possible.

Supporting a slightly different convention may seem to be accomodating and
nice, but if there is no real technical difference between the two (and
again, "area:" is one letter shorter ;-), letting people run with
different convention longer, when they can switch easily to another
convention that is already well supported, may actually hurt them in the
long run.  "[sbuild]" will not match "--area=sbuild" that will internally
become "--grep-only-first-line=sbuild:" so either he will miss out
benefiting from the new feature, or the implementation of the new feature
unnecessarily needs more code.

This is a nice feature I wasn't aware of, so thanks for pointing it
out.  It might be useful to alter my workflow to allow it to be used,
or alternatively customisation to allow a custom regex stored e.g.
in .git/config would allow me to match both forms?

The patch I sent to the list separately replaces the existing
cleanup_subject string munging (which is rather complex and
hairy), with a single regular expression to match the bits of
the string we don't want such as '^Re:' and the first set of
square brackets.  We then just keep the remainder.  I initally
went with the following extended regex:

  ^([Rr]e: )?(.*PATCH[^]]*\\] )(.*)$

but as per your comments above about removing the first set of
brackets whatever the contents, chose the following more
general expression:

  ^([Rr]e:)?([^]]*\\[[^]]+\\])(.*)$

This should be rather more maintainable and flexible than the
existing code, because one can just tweak the regex rather than
fiddling with hairy string offsets.  This preserves the
existing behaviour with the exception of matching the first []
pair only rather than being "greedy" and removing everything up
to the last "]".


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.

On Mon, Jun 29, 2009 at 02:26:45PM -0700, Jakub Narebski wrote:

Roger Leigh [off-list ref] writes:

quoted

Use a regular expression to match text after "Re:" or any text in the
first pair of square brackets such as "[PATCH n/m]".  This replaces
the complex hairy string munging with a simple single  pattern match.

[...]

quoted

+	/* Strip off 'Re:' and/or the first text in square brackets, such as
+	   '[PATCH]' at the start of the mail Subject. */
+	status = regcomp(&regex,
+			 "^([Rr]e:)?([^]]*\\[[^]]+\\])(.*)$",
+			 REG_EXTENDED);

Sidenote: it probably didn't worked before either, but there are some
broken mail readers in the wold (*cough* MS Outlook *cough*), that
misinterpret RFCs and use translated form of "Re:" e.g. "Odp:" (Polish),
or not strip "Re:" when replying resulting in string of "Re: Re: Re: ...",
or use capitalized form of "Re:", i.e. "RE:", or use yet another form 
e.g. compact form of repeated "Re: Re: Re: ..." in form of "Re(3):".

But I guess it didn't worked before either.

One could update the regex to cope with that easily enough such as

  "^([Rr]e:[[:space:]]*)*([^]]*\\[[^]]+\\])(.*)$"

for the "Re: Re: Re:" case, though I can't say I've seen anything
except "Re:" for years.  Maybe I just don't get mail and patches
from Outlook users ;-)


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.

On Mon, Jun 29, 2009 at 11:55:51AM +0200, Andreas Ericsson wrote:

git-format-patch prepends patches with a [PATCH x/n] prefix, but
mailinfo used to remove any number of square-bracket pairs and
the content between them. This prevents one from using a commit
subject like this:

  [ and ] must be allowed as input

Removing the square bracket pair from this rather clumsily
constructed subject line loses important information, so we must
take care not to.

This patch causes the subject stripping to stop after it has
encountered one pair of square brackets.

I think this is a definite improvement, though I would be much more
convinced that the does the right thing if there were some tests. :)

One possible downside of this patch is that the patch-handling
programs will now fail at removing author-added square-brackets
to be removed, such as

  [RFC][PATCH x/n]

However, since format-patch only adds one set of square brackets,
this behaviour is quite easily undesrstood and defended while the
previous behaviour is not.

Agreed. And I think Junio raised a good point elsewhere: there are
certain formatting conventions that are part of format-patch output. So
I think we do need to address "this subject munging is totally idiot
proof and will always reproduce the input patch text exactly". But
rather "is this a sane and useful way to do the munging?". And I think
it is a useful convention.

This is a user-visible change that might impact people's workflows (if
only slightly), though, so it should probably get a good mention in the
release notes.

-Peff