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

On Mon, Oct 28, 2013 at 3:46 AM, Johan Herland [off-list ref] wrote:

On Sun, Oct 27, 2013 at 8:04 PM, Christian Couder [off-list ref] wrote:

quoted

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

Here it could already be:

     Fixes: 1234beef56 (Commit message summmary)
     Acked-by: Jeff King [off-list ref]

Because builtin/commit.c hook could already have expanded everything.

    |
    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 [off-list ref]

(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 [off-list ref]"

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.

I don't agree. Git doesn't need to dictate anything to be able to do
these expansions.
Git only needs some hints to do these expansions properly and it could
just look at the commit template, or the config, to get those hints.

For example, if there is a "Acked-by:" line in the commit template,
then Git might decide that "ack" means "Acked-by", and then that "-by"
means that "Peff" should be related to an author, and then that it is
probably "Jeff King [off-list ref]".

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: ").

Yes, I am arguing that builtin/commit.c, or better a plumbing command
launched by builtin/commit.c, should do it.

And I don't think there is an absolute need for a dictionary.
There could be such a dictionary in the config (as Junio proposed).
But if there isn't, the plumbing command launched by builtin/commit.c
could look at the commit template to decide that "ack" means
"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.

I agree that there should be no need to hack builtin/commit.c.
For example if "Bugzilla-Id:" is in the commit template, the plumbing
command launched by builtin/commit.c would decide that "bug" means
"Bugzilla-Id:" without any hack.

Of course this suppose that there is no other "Bugtracker:" or "Bug:"
in the commit template.
But even in this case it would mean that users have to use "-f
bugz:XXX" (or "--trailer bugz=XXX") instead of just "-f bug:XXX".

quoted

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?

Ok, let's call the new plumbing command "git interpret-trailers".
And let's suppose that "git commit" is passed "-f ack:Peff -f
fix:security-bug" (or "--trailer ack=Peff --trailer
fix=security-bug").

"git commit" would then call something like:

git interpret-trailers --file commit_message_template.txt 'ack:Peff'
'fix:security-bug'

And this command would output:

------------------
<<<upper part of commit_message_template.txt>>>

Fixes: 1234beef56 (Commit message summmary)
Reported-by:
Suggested-by:
Improved-by:
Acked-by: Jeff King <redacted>
Reviewed-by:
Tested-by:
Signed-off-by: Myself <redacted>
------------------

Because it would have looked at the commit template it is passed and
filled in the blanks it could fill using the arguments it is also
passed.

"git commit" would then put the above lines in the file that it passes
to the prepare-commit-msg hook.

Then the prepare-commit-msg could just do nothing.

After the user has edited the commit message, the commit-msg hook
could just call:

git interpret-trailers --trim-empty --file commit_message.txt

so that what the user changed is interpreted again.

For example if the user changed the "Reviewed-by:" line to
"Reviewed-by: Johan", then the output would be:

------------------
<<<upper part of commit_message.txt>>>

Fixes: 1234beef56 (Commit message summmary)
Acked-by: Jeff King <redacted>
Reviewed-by: Johan Herland <redacted>
Signed-off-by: Myself <redacted>
------------------

And that would be the final commit message in most cases.

Thanks,
Christian.