Re: git mailinfo strips important context from patch subjects

[Andreas Ericsson <ae@op5.se>]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Sun, Jun 28, 2009 at 08:38:58PM +0100, Roger Leigh wrote:
>>
>>> 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:
>>
>>> 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.