Re: [PATCH] commit: Add -f, --fixes <commit> option to add Fixes: line

[Johan Herland]

On Sun, Oct 27, 2013 at 8:04 PM, Christian Couder
<christian.couder@xxxxxxxxx> wrote:
> On Sun, Oct 27, 2013 at 2:30 PM, Johan Herland <johan@xxxxxxxxxxx> wrote:
>> On Sun, Oct 27, 2013 at 1:30 PM, Christian Couder <christian.couder@xxxxxxxxx> wrote:
>>>
>>> Your suggestion is very good, and it is not incompatible with command
>>> line options.
>>> So both could be implemented and even work together.
>>>
>>> For example if "-f ack:Peff" was passed to the command line, "git commit" could
>>> lookup in the commit message template and see if there is one
>>> RFC822-style header
>>> that starts with or contains "ack" (discarding case) and it could look
>>> in some previous commits if
>>> there is an author whose name contains "Peff" (discarding case)
>>
>> ...may be cheaper to (first) look at the .mailmap?
>
> Ok. I haven't really had a look at how it could best be done.
>
>>> and if it is the case
>>> it could append the following to the bottom of the commit message:
>>>
>>> Fixes:
>>> Reported-by:
>>> Suggested-by:
>>> Improved-by:
>>> Acked-by: Jeff King <peff@xxxxxxxx>
>>> Reviewed-by:
>>> Tested-by:
>>> Signed-off-by: Myself <myself@xxxxxxxxxxx>
>>>
>>> (I suppose that the sob is automatically added.)
>>>
>>> It would work also with "-f fix:security-bug" and would put something
>>> like what you suggested:
>>>
>>> Fixes: 1234beef56 (Commit message summmary)
>>
>> Even better: Imagine "-f" (or whatever is decided) as a general
>> mechanism for forwarding parameters to the prepare-commit-msg hook.
>> When you run "git commit -f ack:Peff -f fix:security-bug", the -f
>> arguments will be forwarded to prepare-commit-msg (as additional
>> command-line args, or on stdin), and then the prepare-commit-msg hook
>> can do whatever it wants with them (e.g. the things you describe
>> above).
>
> If "git commit" processes these arguments and puts the result in the
> commit message file that is passed to the
> prepare-commit-msg hook, then this hook can still get them from the
> file and process them however it wants.
>
> And in most cases the processing could be the same as what is done by
> the commit-msg hook when the user changes the "Fixes: xxx" and
> "Stuffed-by: yyy" lines in the editor.
>
> So it would probably be easier for people customizing the
> prepare-commit-msg and commit-msg if "git commit" processes the
> arguments instead of just passing them to the prepare-commit-msg hook.
>
> And it will be better for people who don't set up any *commit-msg hook.
> Even if there is no commit template, "-f Acked-by:Peff" and "-f
> Fixes:security-bug" could still work.
> I suspect most users don't setup any hook or commit template.

Hmm. I'm not sure what you argue about which part of the system should
perform which function. Let's examine the above options in more
detail. Roughly, the flow of events look like this

  git commit -f ack:Peff -f fix:security-bug
    |
    v
  builtin/commit.c (i.e. inside "git commit")
    |
    v
  prepare-commit-msg hook
    |
    v
  commit message template:
    Fixes: security-bug
    Acked-by: Peff
    |
    v
  user edits commit message (may or may not change Fixes/Acked-by lines)
    |
    v
  commit-msg hook
    |
    v
  commit message:
    Fixes: 1234beef56 (Commit message summmary)
    Acked-by: Jeff King <peff@xxxxxxxx>

(The above is even a bit simplified, but I believe it's sufficient for
the current discussion.) So, there are several expansions happening
between the initial "git commit" and the final commit message. They
are:

 1. "fix" -> "Fixes: "
 2. "security-bug" -> "1234beef56 (Commit message summmary)"
 3. "ack" -> "Acked-by: "
 4. "Peff" -> "Jeff King <peff@xxxxxxxx>"

First, I think we both agree that expansions #2 and #4 MUST be done by
the commit-msg hook. The reason for this is two-fold: (a) the
expansion must be done (at least) after the user has edited the commit
message (since the values entered by the user might require the same
expansion), and (b) how (and whether) to perform the expansion is a
project-specific policy question, and not something that Git can
dictate. Obviously, common functionality can be made available in the
default hook shipped by Git, but it's up to each project to enable
and/or customize this.

Second, there is #1 and #3, the expansion of "ack" -> "Acked-by:" and
"fix" -> "Fixes:". Is this expansion performed by the
prepare-commit-msg hook, or directly inside builtin/commit.c?

If you are arguing for the latter (and I'm not sure that you are), we
would need to add a dictionary to "git commit" that maps shorthand
field names ("ack") to the RFC822 -style equivalent ("Acked-by: ").

I would instead argue for the former, i.e. simply forwarding "ack" and
"fix" as-is to the prepare-commit-msg hook, and let it deal with the
appropriate expansion. The main reason for this is that if a project
wants to add another shorthand expansion (e.g. "bug" ->
"Related-Bugzilla-Id: "), they can do so without hacking
builtin/commit.c.

Certainly, we could ship a default prepare-commit-msg hook that knows
how to expand the usual suspects (like "ack" and "fix"), but
hardcoding this inside "git commit" is not optimal, IMHO.

>> The reason I like this, is that we can now support project-specific
>> conventions/rules without having to encode those directly in "git
>> commit" itself. The only thing "git commit" needs to know, is how to
>> forward the appropriate information to the project-specific hook.
>
> Supporting project specific conventions/rules would still be possible
> by processing lines in the commit message file without changing "git
> commit".
>
> If "git commit" is already able to do some processing, it only adds
> power to what can be done by people writing hooks.
>
> We could even have git plumbing commands used by git commit to process
> the -f (or whatever option) arguments and they could be reused by the
> *commit-msg hooks if they find them useful.

Can you walk through an example of such reusable functionality? ISTM
that you want to add quite a lot of infrastructure to git for very
small gains (preparing and cleaning up commit messages), when that
infrastructure could instead be added by those (few?) projects that
need it without complicating Git itself (it is e.g. trivially easy to
share code between the prepare-commit-msg and commit-msg hooks...)

>> One such project-specific convention/rule is the Signed-off-by line
>> (although it has certainly spread to quite a lot of projects). I am
>> not 100% comfortable with encoding this convention directly into "git
>> commit", because it serves as a "slippery slope" to encode even more
>> project-specific conventions/rules directly into "git commit" (the
>> proposals to add command-line options for the "Change-Id" and "Fixes"
>> headers are the two most recent examples), and the more we add, the
>> more we bloat the "git commit" command-line interface.
>
> I don't think we would bloat the "git commit" command line interface.
> We just add one option that could help a lot of people, even those who
> want something very special.
>
>>>>  2. No need to add any command-line options to Git.
>>>
>>> This is not a good reason. If many users prefer a command line option,
>>> why not let them use that?
>>
>> True. As explained above, what I don't want is to add another
>> command-line option _every_time_ there is a useful project-specific
>> convention. Adding _one_ option to rule them all is much more
>> acceptable to me. :)
>
> Great!
>
>>>>  3. The whole mechanism is controlled by the project. The kernel folks
>>>> can do whatever they want in their templates/hooks without needing
>>>> changes to the Git project.
>>>
>>> The Git project already manages sob lines. It would be a good thing if
>>> it could manage
>>> more of this stuff to help users in a generic way while taking care of
>>> user preferences.
>>
>> Yes, but the key word here is _generic_. It's impossible to make "git
>> commit" able to solve all problems for all projects, but we can make a
>> generic mechanism that helps projects solve their own problems more
>> easily.
>
> Yeah, but as you said in your initial message, the generic mechanism
> already exists with commit templates and *commit-msg hooks. The only
> question left is how an additional command line option can best help.
> And if this option does nearly nothing, it will not help much.

But I still don't see exactly what this option should do (inside "git
commit") that would end up being useful across most/all projects, and
not just something that could more easily be implemented in the
*commit-msg hooks for relevant projects.


...Johan

-- 
Johan Herland, <johan@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/