Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
config.) This is just some initial thoughts about a possible config format. A more important point though, is that we don't really need to add anything to core Git to support this. All we need to do is to implement a set of "dispatcher" hooks that read the relevant configuration and perform the job accordingly. Although these "dispatcher" hooks could certainly be developed as a separate project - more or less independent from git.git, I do believe there would be considerable value in distributing them along with Git and easily enabling them (maybe even enabling them by default, as without the config options they would just be no-ops). Otherwise, it would be hard to make them used/accepted widely enough to actually replace current ad hoc solutions. ...Johan -- Johan Herland, www.herland.net -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] commit: Add -f, --fixes commit option to add Fixes: line
and perform the job accordingly. Although these dispatcher hooks could certainly be developed as a separate project - more or less independent from git.git, I do believe there would be considerable value in distributing them along with Git and easily enabling them (maybe even enabling them by default, as without the config options they would just be no-ops). Otherwise, it would be hard to make them used/accepted widely enough to actually replace current ad hoc solutions. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Tue, Oct 29, 2013 at 3:08 AM, Jeff King wrote:
> On Mon, Oct 28, 2013 at 12:29:32PM +0100, Johan Herland wrote:
>> > A hook-based solution could do this. But a built-in "all-purpose"
>> > handler like "footer.Fixes.arg=commit", which was intended to be
>> > reusable, wouldn't be able to do such footer-specific extra work without
>> > having to create new special cases in git each time.
>>
>> Which begs the question (posed to all, not specifically to you): Why
>> would we want solve this issue in config instead of in hooks? The
>> hooks will always be more flexible and less dependent on making
>> changes in git.git. (...a suitably flexible hook could even use the
>> config options discussed above as input...) In both cases, we need the
>> user to actively enable the functionality (either installing hooks, or
>> setting up config), and in both cases we could bundle Git with
>> defaults that solve the common cases, so that is not a useful
>> differentiator between the two approaches. I would even venture to
>> ask: If we end up solving this problem in config and not in hooks,
>> then why do we bother having hooks in the first place?
>
> One thing that is much nicer with config vs hooks is that you can manage
> config for all of your repositories by tweaking ~/.gitconfig (and that
> is where I would expect this type of config to go).
Actually, I believe the use of footers are more often guided by
project conventions/rules, so I wouldn't expect such config to go into
~/.gitconfig. I would rather expect to find it in an in-project config
that was included from the repo config...
> Managing hooks globally means having each repo symlink to a central hook
> area, and having the forethought to set up the symlink farm and use
> init.templatedir before cloning any repos. We could probably make this
> friendlier by reading from ~/.githooks and defining some semantics for
> multiple hooks. E.g., fall back to ~/.githooks if the repo hook is not
> executable, or possibly run them both (or even allow multiple instances
> of a hook in ~/.githooks, which can help organization), and consider the
> hook a failure if any of them fail.
Yes, we do lack a good infrastructure for managing Git hooks from
multiple sources. It makes people afraid to use them, because they
might conflict with hooks from another source. There are (off the top
of my head):
- "personal" hooks ("I want this behaviour in my repo(s)")
- "project" hooks ("In this project we follow these conventions")
- "system" hooks ("This host runs gitolite (or whatever) which needs
these hooks...")
- "default" hooks (Some of the core Git code could have be
implemented as hooks (e.g. "--signoff"), but is instead put into core
Git)
Maybe if we solved that problem, we could actually make use of hooks
instead of adding "code" to our git configs (by which I mean config
directives that are flexible enough to encode all kinds of semantics
and behaviors that are probably better expressed in real code...).
...Johan
--
Johan Herland,
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] commit: Add -f, --fixes commit option to add Fixes: line
On Tue, Oct 29, 2013 at 3:08 AM, Jeff King p...@peff.net wrote: On Mon, Oct 28, 2013 at 12:29:32PM +0100, Johan Herland wrote: A hook-based solution could do this. But a built-in all-purpose handler like footer.Fixes.arg=commit, which was intended to be reusable, wouldn't be able to do such footer-specific extra work without having to create new special cases in git each time. Which begs the question (posed to all, not specifically to you): Why would we want solve this issue in config instead of in hooks? The hooks will always be more flexible and less dependent on making changes in git.git. (...a suitably flexible hook could even use the config options discussed above as input...) In both cases, we need the user to actively enable the functionality (either installing hooks, or setting up config), and in both cases we could bundle Git with defaults that solve the common cases, so that is not a useful differentiator between the two approaches. I would even venture to ask: If we end up solving this problem in config and not in hooks, then why do we bother having hooks in the first place? One thing that is much nicer with config vs hooks is that you can manage config for all of your repositories by tweaking ~/.gitconfig (and that is where I would expect this type of config to go). Actually, I believe the use of footers are more often guided by project conventions/rules, so I wouldn't expect such config to go into ~/.gitconfig. I would rather expect to find it in an in-project config that was included from the repo config... Managing hooks globally means having each repo symlink to a central hook area, and having the forethought to set up the symlink farm and use init.templatedir before cloning any repos. We could probably make this friendlier by reading from ~/.githooks and defining some semantics for multiple hooks. E.g., fall back to ~/.githooks if the repo hook is not executable, or possibly run them both (or even allow multiple instances of a hook in ~/.githooks, which can help organization), and consider the hook a failure if any of them fail. Yes, we do lack a good infrastructure for managing Git hooks from multiple sources. It makes people afraid to use them, because they might conflict with hooks from another source. There are (off the top of my head): - personal hooks (I want this behaviour in my repo(s)) - project hooks (In this project we follow these conventions) - system hooks (This host runs gitolite (or whatever) which needs these hooks...) - default hooks (Some of the core Git code could have be implemented as hooks (e.g. --signoff), but is instead put into core Git) Maybe if we solved that problem, we could actually make use of hooks instead of adding code to our git configs (by which I mean config directives that are flexible enough to encode all kinds of semantics and behaviors that are probably better expressed in real code...). ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Mon, Oct 28, 2013 at 10:02 AM, Michael Haggerty wrote: > On 10/27/2013 08:14 AM, Josh Triplett wrote: >> On Sun, Oct 27, 2013 at 06:42:44AM +0100, Michael Haggerty wrote: >>> On 10/27/2013 02:34 AM, Josh Triplett wrote: >>> I wonder if the two features could >>> be combined in some way? >>> >>> The main difference between the two features is how they are intended to >>> be used: --fixup is to fix a commit that hasn't been pushed yet (where >>> the user intends to squash the commits together), whereas --fixes is to >>> mark a commit as a fix to a commit that has already been pushed (where >>> the commits will remain separate). But there seems to be a common >>> concept here. >>> >>> For example, what happens if a --fixes commit is "rebase -i"ed at the >>> same time as the commit that it fixes? It might make sense to do the >>> autosquash thing just like with a --fixup/--squash commit. (Otherwise >>> the SHA-1 in the "Fixes:" line will become invalid anyway.) >> >> Most definitely not, no, at least not without an explicit option to >> enable that. Consider the case of backporting a series of patches and >> preserving the relative history of those patches, to make it easier to >> match up a set of patches. At most, it might be a good idea for >> cherry-pick or similar to provide an updated Fixes tag for the new hash >> of the older commit. Personally, I'd argue against doing this even with >> --autosquash. I could see the argument for an --autosquash-fixes, but I >> can't think of a real-world scenario where what would come up. >> >> Generally, if history is still editable, you should just squash in the >> fix to the original commit, and if history is no longer editable (which >> is the use case for "Fixes:" lines), the squash case simply won't come >> up, offering little point to adding special support for that case. > > In your last paragraph you explain exactly why these two features are > similar and why it is thinkable to make the way that they are handled > depend on the context. Exactly because one would never rebase a > "Fixes:" commit and the commit it is fixing at the same time, they would > never be squashed together. And ISTM that in most cases whenever they > *are* being rebased at the same time, then one would want to squash them > together. So it might be possible to mark both types of commits the > same way and then squash/not squash them depending on the context and > the --autosquash option. In general, we should be careful with introducing features that exhibit different consequences based on the context in which they are used, but in this case, I believe I agree with you. The existence of "Fixes:" in a commit should be a just as valid hint to --autosquash as a commit message starting with "fixup!" or "squash!" (obviously, the "Fixes:" commit should be handled like a "squash!" and not like a "fixup!", so that we don't haphazardly discard the commit message accompanying "Fixes:"). >>> I see that there a consistency check that the --fixes argument is a >>> valid commit. But is there/should there be a check that it is an >>> ancestor of the commit being created? Is there/should there be a check >>> that both of these facts remain true if the the commit containing it is >>> rebased, cherry-picked, etc? >> >> That sounds like a nice future enhancement, sure. I don't have any plans to >> add such a check myself, though. Also note that --fixup and --squash >> don't have such a check either; if you want to add one, you should add >> it for all three options at once. > > A hook-based solution could do this. But a built-in "all-purpose" > handler like "footer.Fixes.arg=commit", which was intended to be > reusable, wouldn't be able to do such footer-specific extra work without > having to create new special cases in git each time. Which begs the question (posed to all, not specifically to you): Why would we want solve this issue in config instead of in hooks? The hooks will always be more flexible and less dependent on making changes in git.git. (...a suitably flexible hook could even use the config options discussed above as input...) In both cases, we need the user to actively enable the functionality (either installing hooks, or setting up config), and in both cases we could bundle Git with defaults that solve the common cases, so that is not a useful differentiator between the two approaches. I would even venture to ask: If we end up solving this problem in config and not in hooks, then why do we bother having hooks in the first place? ...Johan -- Johan Herland, www.herland.net -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] commit: Add -f, --fixes commit option to add Fixes: line
On Mon, Oct 28, 2013 at 10:02 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 10/27/2013 08:14 AM, Josh Triplett wrote: On Sun, Oct 27, 2013 at 06:42:44AM +0100, Michael Haggerty wrote: On 10/27/2013 02:34 AM, Josh Triplett wrote: I wonder if the two features could be combined in some way? The main difference between the two features is how they are intended to be used: --fixup is to fix a commit that hasn't been pushed yet (where the user intends to squash the commits together), whereas --fixes is to mark a commit as a fix to a commit that has already been pushed (where the commits will remain separate). But there seems to be a common concept here. For example, what happens if a --fixes commit is rebase -ied at the same time as the commit that it fixes? It might make sense to do the autosquash thing just like with a --fixup/--squash commit. (Otherwise the SHA-1 in the Fixes: line will become invalid anyway.) Most definitely not, no, at least not without an explicit option to enable that. Consider the case of backporting a series of patches and preserving the relative history of those patches, to make it easier to match up a set of patches. At most, it might be a good idea for cherry-pick or similar to provide an updated Fixes tag for the new hash of the older commit. Personally, I'd argue against doing this even with --autosquash. I could see the argument for an --autosquash-fixes, but I can't think of a real-world scenario where what would come up. Generally, if history is still editable, you should just squash in the fix to the original commit, and if history is no longer editable (which is the use case for Fixes: lines), the squash case simply won't come up, offering little point to adding special support for that case. In your last paragraph you explain exactly why these two features are similar and why it is thinkable to make the way that they are handled depend on the context. Exactly because one would never rebase a Fixes: commit and the commit it is fixing at the same time, they would never be squashed together. And ISTM that in most cases whenever they *are* being rebased at the same time, then one would want to squash them together. So it might be possible to mark both types of commits the same way and then squash/not squash them depending on the context and the --autosquash option. In general, we should be careful with introducing features that exhibit different consequences based on the context in which they are used, but in this case, I believe I agree with you. The existence of Fixes: in a commit should be a just as valid hint to --autosquash as a commit message starting with fixup! or squash! (obviously, the Fixes: commit should be handled like a squash! and not like a fixup!, so that we don't haphazardly discard the commit message accompanying Fixes:). I see that there a consistency check that the --fixes argument is a valid commit. But is there/should there be a check that it is an ancestor of the commit being created? Is there/should there be a check that both of these facts remain true if the the commit containing it is rebased, cherry-picked, etc? That sounds like a nice future enhancement, sure. I don't have any plans to add such a check myself, though. Also note that --fixup and --squash don't have such a check either; if you want to add one, you should add it for all three options at once. A hook-based solution could do this. But a built-in all-purpose handler like footer.Fixes.arg=commit, which was intended to be reusable, wouldn't be able to do such footer-specific extra work without having to create new special cases in git each time. Which begs the question (posed to all, not specifically to you): Why would we want solve this issue in config instead of in hooks? The hooks will always be more flexible and less dependent on making changes in git.git. (...a suitably flexible hook could even use the config options discussed above as input...) In both cases, we need the user to actively enable the functionality (either installing hooks, or setting up config), and in both cases we could bundle Git with defaults that solve the common cases, so that is not a useful differentiator between the two approaches. I would even venture to ask: If we end up solving this problem in config and not in hooks, then why do we bother having hooks in the first place? ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Sun, Oct 27, 2013 at 8:04 PM, Christian Couder wrote: > On Sun, Oct 27, 2013 at 2:30 PM, Johan Herland wrote: >> On Sun, Oct 27, 2013 at 1:30 PM, Christian Couder >> 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 >>> Reviewed-by: >>> Tested-by: >>> Signed-off-by: Myself >>> >>> (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 (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 " 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/com
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Sun, Oct 27, 2013 at 10:20 AM, Josh Triplett wrote: > On Sun, Oct 27, 2013 at 09:09:32AM +0100, Thomas Rast wrote: >> Josh Triplett writes: >> > On Sun, Oct 27, 2013 at 06:42:44AM +0100, Michael Haggerty wrote: >> >> But I don't think that this feature should be given the "-f" short >> >> option, as (a) -f often means "force"; (b) it will increase the >> >> confusion with --fixup; (c) it just doesn't strike me as being likely to >> >> be such a frequently-used option (though if this changes over time the >> >> "-f" option could always be granted to it later). >> > >> > (a) -n often means --dry-run, but for commit it means --no-verify. >> > Different commands have different options, and commit doesn't have a >> > --force to abbreviate as -f. >> > >> > (b) If anything, I think the existence of a short option will make the >> > distinction more obvious, since -f and --fixup are much less similar >> > than --fixes and --fixup. Most users will never type --fixes, making >> > confusion unlikely. >> > >> > (c) Short option letters tend to be first-come first-serve unless >> > there's a strong reason to do otherwise. Why reserve 'f' for some >> > hypothetical future option that doesn't exist yet? >> >> No, lately the direction in Git has been to avoid giving options a >> one-letter shorthand until they have proven so useful that people using >> it in the wild start to suggest that it should have one. >> >> See e.g. >> >> http://article.gmane.org/gmane.comp.version-control.git/233998 >> http://article.gmane.org/gmane.comp.version-control.git/168748 > > Fair enough; easy enough to drop -f if that's the consensus. However... > >> A much better argument would be if it was already clear from the specs >> laid out for Fixes that n% of the kernel commits will end up having this >> footer, and thus kernel hackers will spend x amount of time spelling out >> --fixes and/or confusing it with --fixup to much headache. > > ...good suggestion: > > ~/src/linux$ git log --grep='stable@' --oneline --since='1 year ago' | wc -l > 2769 > ~/src/linux$ git log --grep='stable@' --oneline --since='1 year ago' > --pretty=format:%an | sort -u | wc -l > 839 > > Several thousand commits per year by hundreds of unique people seems > like enough to justify a short option. I think this can be solved just as well (if not better) using a combination of a commit message template (or a prepare-commit-msg hook) and a commit-msg hook. The former appends a section of commonly-used RFC822-style headers (with empty values) to the bottom of the commit message, e.g. some variation on this: Fixes: Reported-by: Suggested-by: Improved-by: Acked-by: Reviewed-by: Tested-by: Signed-off-by: Then the user (in addition to writing the commit message above this block) may choose to fill in one or more values in this "form", e.g. like this: My commit subject This is the commit message body. Fixes: 1234beef Reported-by: Joe User Suggested-by: Improved-by: Joe Hacker Acked-by: Reviewed-by: Tested-by: Joe Tester Signed-off-by: Myself Then, the commit-msg hook can clean up and transform this into the final commit message: My commit subject This is the commit message body. Fixes: 1234beef56 (Commit message summmary) Reported-by: Joe User Improved-by: Joe Hacker Tested-by: Joe Tester Signed-off-by: Myself Here, the commit-msg hook removes the fields that were not filled in, and performs additional filtering on the "Fixes" line (Adding commit message summary). The filtering could also resolve ref names, so that if you had refs/tags/security-bug pointing at the buggy commit, then: Fixes: security-bug would be expanded/DWIMed into: Fixes: 1234beef56 (Commit message summmary) Obviously, any other fancy processing you want to do into in the commit-msg hook can be done as well, adding footnotes, checking that commits are present in the ancestry, etc, etc. Three good reasons to go this way: 1. If the user forgets to supply command-line options like -s, --fixes, etc, there is a nice reminder in the supplied form. 2. No need to add any command-line options to Git. 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. ...Johan -- Johan Herland, www.herland.net -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] commit: Add -f, --fixes commit option to add Fixes: line
On Sun, Oct 27, 2013 at 10:20 AM, Josh Triplett j...@joshtriplett.org wrote: On Sun, Oct 27, 2013 at 09:09:32AM +0100, Thomas Rast wrote: Josh Triplett j...@joshtriplett.org writes: On Sun, Oct 27, 2013 at 06:42:44AM +0100, Michael Haggerty wrote: But I don't think that this feature should be given the -f short option, as (a) -f often means force; (b) it will increase the confusion with --fixup; (c) it just doesn't strike me as being likely to be such a frequently-used option (though if this changes over time the -f option could always be granted to it later). (a) -n often means --dry-run, but for commit it means --no-verify. Different commands have different options, and commit doesn't have a --force to abbreviate as -f. (b) If anything, I think the existence of a short option will make the distinction more obvious, since -f and --fixup are much less similar than --fixes and --fixup. Most users will never type --fixes, making confusion unlikely. (c) Short option letters tend to be first-come first-serve unless there's a strong reason to do otherwise. Why reserve 'f' for some hypothetical future option that doesn't exist yet? No, lately the direction in Git has been to avoid giving options a one-letter shorthand until they have proven so useful that people using it in the wild start to suggest that it should have one. See e.g. http://article.gmane.org/gmane.comp.version-control.git/233998 http://article.gmane.org/gmane.comp.version-control.git/168748 Fair enough; easy enough to drop -f if that's the consensus. However... A much better argument would be if it was already clear from the specs laid out for Fixes that n% of the kernel commits will end up having this footer, and thus kernel hackers will spend x amount of time spelling out --fixes and/or confusing it with --fixup to much headache. ...good suggestion: ~/src/linux$ git log --grep='stable@' --oneline --since='1 year ago' | wc -l 2769 ~/src/linux$ git log --grep='stable@' --oneline --since='1 year ago' --pretty=format:%an | sort -u | wc -l 839 Several thousand commits per year by hundreds of unique people seems like enough to justify a short option. I think this can be solved just as well (if not better) using a combination of a commit message template (or a prepare-commit-msg hook) and a commit-msg hook. The former appends a section of commonly-used RFC822-style headers (with empty values) to the bottom of the commit message, e.g. some variation on this: Fixes: Reported-by: Suggested-by: Improved-by: Acked-by: Reviewed-by: Tested-by: Signed-off-by: Then the user (in addition to writing the commit message above this block) may choose to fill in one or more values in this form, e.g. like this: My commit subject This is the commit message body. Fixes: 1234beef Reported-by: Joe User j.u...@example.com Suggested-by: Improved-by: Joe Hacker j.hac...@example.com Acked-by: Reviewed-by: Tested-by: Joe Tester j.tes...@example.com Signed-off-by: Myself mys...@example.com Then, the commit-msg hook can clean up and transform this into the final commit message: My commit subject This is the commit message body. Fixes: 1234beef56 (Commit message summmary) Reported-by: Joe User j.u...@example.com Improved-by: Joe Hacker j.hac...@example.com Tested-by: Joe Tester j.tes...@example.com Signed-off-by: Myself mys...@example.com Here, the commit-msg hook removes the fields that were not filled in, and performs additional filtering on the Fixes line (Adding commit message summary). The filtering could also resolve ref names, so that if you had refs/tags/security-bug pointing at the buggy commit, then: Fixes: security-bug would be expanded/DWIMed into: Fixes: 1234beef56 (Commit message summmary) Obviously, any other fancy processing you want to do into in the commit-msg hook can be done as well, adding footnotes, checking that commits are present in the ancestry, etc, etc. Three good reasons to go this way: 1. If the user forgets to supply command-line options like -s, --fixes, etc, there is a nice reminder in the supplied form. 2. No need to add any command-line options to Git. 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. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] commit: Add -f, --fixes commit option to add Fixes: line
On Sun, Oct 27, 2013 at 8:04 PM, Christian Couder christian.cou...@gmail.com wrote: On Sun, Oct 27, 2013 at 2:30 PM, Johan Herland jo...@herland.net wrote: On Sun, Oct 27, 2013 at 1:30 PM, Christian Couder christian.cou...@gmail.com 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 p...@peff.net Reviewed-by: Tested-by: Signed-off-by: Myself mys...@example.com (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 p...@peff.net (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 p...@peff.net 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
