Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Thu, Oct 31, 2013 at 6:20 PM, Junio C Hamano wrote: > Duy Nguyen writes: >> OK how about, if $GIT_DIR/hooks/something is a directory, then the >> directory must contain a file named "index", listing all the hooks of >> type "something". All the hooks in "index" will be executed in the >> listing order. > > Hooks that take arbitrary amount of information from the body read > their standard input. How are your multiple hooks supposed to > interact? As an example, at $dayjob we have a "dispatcher" post-receive hook running on our Git server that captures the current environment, and reads all of stdin. It then iterates through a (configurable) sequence of "subhooks" providing them each with a copy of the data that was passed to it. The "subhooks" may perform duties such as notifying automated build and test systems, triggering updates of mirrors, updating bug trackers, formatting and sending commit emails to mailing lists, etc. Some of them are run synchronously (redirecting their output back to the push client), and some are run asynchronously (redirecting their output to logs). The nice thing is that each of the "subhooks" use the same post-receive hook interface, and is therefore a fully capable stand-alone hook by itself (often implemented in different languages, some of them are not even written by us), and also fully independent of the other "subhooks". It is therefore relatively straightforward to add, remove and mix hooks. > Hooks that prevent you from doing something stupid signal allow/deny > with their exit code. Do you fail a commit if any of your pre-commit > hook fails, or is it OK to commit as long as one of them says so? > If the former, do all the hooks described in the index still run, or > does the first failure short-cut the remainder? This clearly needs to be configurable, as there are valid use cases for all the behaviors you mention. That said, I believe that a sane default would be for a single hook failure to cause the entire chain-of-hooks to fail, including short-cutting the remainder of the hooks (at least for the hooks where the exit code determines the outcome of the entire operation). For example, one could envision a sequence of pre-commit hooks being configured something like this: [hook "pre-commit.check-whitespace"] run = /path/to/whitespace-checker on-error = fail-later [hook "pre-commit.check-valid-ident"] run = /path/to/ident-checker on-error = fail-later [hook "pre-commit.run-testsuite"] run = "/path/to/testsuite --with --arguments" on-error = fail-later The hooks would be run in sequence. The hook.pre-commit.*.run variable specifies how to execute the hook (it is assumed that each of the configured hooks behaves according to the pre-commit hook interface). The hook.pre-commit.*.on-error variable specifies how to handle a non-zero exit code from the hook. Possible values would be "abort" (abort the remaining hooks and return failure immediately), "fail-later" (keep running the remainder of the hooks, but make sure we do return failure in the end), or "ignore" (always pretend the hook returns successfully). The default on-error behavior should IMHO be "abort", but in this case, we don't want to abort on the first failure, as we'd rather report errors from _multiple_ hooks to the user in a single go. Similarly, a sequence of post-receive hooks could be configure like this: [hook "post-receive.trigger-buildbot"] run = /path/to/buildbot-trigger-hook [hook "post-receive.update-bugtracker"] run = /path/to/bugtracker-update-hook [hook "post-receive.trigger-mirror-update"] run = /path/to/mirror-update-hook async = true redirect-output = /var/log/mirror-update-hook.log [hook "post-receive.send-commit-emails"] run = /path/to/commit-emailer async = true Here, the .on-error variable is probably less than useful, since post-receive hooks cannot affect the outcome of the push operation (and having one post-receive hook abort the running of another is probably uncommon). Instead, the .async variable (default: false) is used to indicate which hooks should be run asynchronously (i.e. the client does not have to wait for these hooks to complete). On a server with many repos, you could even store the above in the global git config, to have the hooks available to all repos, and then use hook.post-receive.*.enabled = true/false to turn hooks on/off for individual repos. (A nice side-effect of putting this stuff in the config is that it makes is easy to add/remove/manage hooks through our Gitolite setup - which already has support for managing per-repo config options in the Gitolite 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
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Fri, Nov 1, 2013 at 12:20 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> OK how about, if $GIT_DIR/hooks/something is a directory, then the >> directory must contain a file named "index", listing all the hooks of >> type "something". All the hooks in "index" will be executed in the >> listing order. > > Hooks that take arbitrary amount of information from the body read > their standard input. How are your multiple hooks supposed to > interact? If each only needs to read a few lines from stdin, they can do so in order. If two hooks need to read till the end of stdin, they are incompatible. If we support some sort of hook signature, we could warn the user when they combine the two. If not, the second's failing (because stdin is already closed) may show the incompatibility. "git hook" should support dry-run mode to test out new combinations. > Hooks that prevent you from doing something stupid signal allow/deny > with their exit code. Do you fail a commit if any of your pre-commit > hook fails, or is it OK to commit as long as one of them says so? > If the former, do all the hooks described in the index still run, or > does the first failure short-cut the remainder? One failed hook fails the commit and stops the remaining from executing. You can skip the hook if you want with --exclude-hooks. -- Duy -- 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
Duy Nguyen writes: > OK how about, if $GIT_DIR/hooks/something is a directory, then the > directory must contain a file named "index", listing all the hooks of > type "something". All the hooks in "index" will be executed in the > listing order. Hooks that take arbitrary amount of information from the body read their standard input. How are your multiple hooks supposed to interact? Hooks that prevent you from doing something stupid signal allow/deny with their exit code. Do you fail a commit if any of your pre-commit hook fails, or is it OK to commit as long as one of them says so? If the former, do all the hooks described in the index still run, or does the first failure short-cut the remainder? -- 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 Thu, Oct 31, 2013 at 1:12 AM, Johan Herland wrote:
> 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...).
OK how about, if $GIT_DIR/hooks/something is a directory, then the
directory must contain a file named "index", listing all the hooks of
type "something". All the hooks in "index" will be executed in the
listing order. There could be directories inside .git/hooks/something
to help categorize the scripts, so project hooks stay in "project"
subdirectory and so on.
With this we could provide "git hook" command to manipulate hooks and
test out the new combination of hooks. We could even select what
scripts not to run for a particular run, say you don't want the s-o-b
hook active when you commit this thing, you could run
git commit --exclude-hooks=pre-commit-msg/s-o-b
You could exclude hooks by pattern as well
git commit --exclude-hooks="pre-commit-msg/projects/*"
Or run an unsinstalled hook just one time
git commit --include-hooks=/path/to/my/hook
Hooks like "Fixes" may need input from the user. The hook could bail
out if the required input is not given. But it maybe a good idea for
git to check and reject before running hooks, if the input is not
specified (e.g. from command line). I guess those extra info has to be
in .git/config and be added to .git/config by "git hook" command,
unless we have some convention to express those without running the hook.
For old Git versions that does not support this scheme, as directories
usually have u+x, the hook directory should be mistaken as an
executable and rejected when executed (permission denied in my test),
which gives user a good warning that this repo should not be used with
this git version.
--
Duy
--
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 option to add Fixes: line
On Sat, Oct 26, 2013 at 6:34 PM, Josh Triplett wrote:
> + format_commit_message(commit, "Fixes: %h ('%s')\n", sb, );
What is the value of double wrapping the commit message inside '...'
and then ('...')?
-Tony
--
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
Jeff King writes: > We could probably make this friendlier by reading from ~/.githooks > and defining some semantics for multiple hooks. I'd be all for it, except I'd call this ~/.config/git/hooks/* (or $XDG_CONFIG_HOME if set). > E.g., fall back to ~/.githooks if the repo hook is not > executable, or possibly run them both I think running them both would be the best option. Otherwise, adding a (possibly trivial) hook to a repo would disable the user-wide one, that'd feel weird. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 11:10:13PM +0100, Thomas Rast wrote: > * In your list > > > Fixes: > > Reported-by: > > Suggested-by: > > Improved-by: > > Acked-by: > > Reviewed-by: > > Tested-by: > > Signed-off-by: > > and I might add > > Cherry-picked-from: > Reverts: > > if one were to phrase that as a footer/pseudoheader, observe that > there are only two kinds of these: footers that contain identities, > and footers that contain references to commits. I think people put other things in, too. For example, cross-referencing bug-tracker ids. In fact, if I saw "fixes: XXX", I would expect the latter to be a tracker id. People do this a lot with GitHub issues, because GitHub will auto-close issue 123 if a commit with "fixes #123" is pushed to master. Because of the "#", no pseudo-header is needed, but I have also seen people use the footer style (I don't have any examples on-hand, though). That being said, in your examples: > So why not support these use-cases? We could have something like > footer.foo.* configuration, e.g. > > [footer "fixes"] > type = commit > suggest = true > [footer "acked-by"] > type = identity you could easily have "type=text" to handle arbitrary text. -Peff -- 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 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). 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. -Peff -- 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: [ksummit-attendees] [PATCH] commit: Add -f, --fixes option to add Fixes: line
[ I've removed Junio and the git mailing list form the -cc ] On Mon, Oct 28, 2013 at 11:41:17PM +, Russell King - ARM Linux wrote: > > I agree too. This whole thread seems to be about noise, and I too > thought there was something about not cross-posting between this list > and any other list. We talked about not including patches on a theoretical "git subtree maintainers list", for noise control purposes, and I think it would be a good to extend this to the ksummit-* mailing lists as well. I'm not sure there's an easy way to make mailman reject e-mails that contain patches, but I'd request that people please honor this restriction. If you must, send the patch to the appropriate mailing list, and then include a lkml.org or some other mail archive link to the ksummit-* lists. - Ted -- 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: [ksummit-attendees] [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Tue, Oct 29, 2013 at 10:09:53AM +1100, Benjamin Herrenschmidt wrote: > On Mon, 2013-10-28 at 09:59 +0100, Christoph Hellwig wrote: > > Btw, can we please take away this discussion from ksummit-attendees? It's > > got > > absolutely nothing to do with kernel summit and is getting fairly annoying. > > Ack. Additionally, iirc, we had decided that > > - We don't cross post multiple lists > > - We drop the annoying subject tags > > As is, all I see is some attempt at doing an lkml dup, which is > pointless I agree too. This whole thread seems to be about noise, and I too thought there was something about not cross-posting between this list and any other list. -- 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: [ksummit-attendees] [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Tue, Oct 29, 2013 at 10:09:53AM +1100, Benjamin Herrenschmidt wrote: > On Mon, 2013-10-28 at 09:59 +0100, Christoph Hellwig wrote: > > Btw, can we please take away this discussion from ksummit-attendees? It's > > got > > absolutely nothing to do with kernel summit and is getting fairly annoying. > > Ack. Additionally, iirc, we had decided that > > - We don't cross post multiple lists > > - We drop the annoying subject tags > > As is, all I see is some attempt at doing an lkml dup, which is > pointless I agree too. This whole thread seems to be about noise, and I too thought there was something about not cross-posting between this list and any other list. -- 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: [ksummit-attendees] [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Mon, 2013-10-28 at 09:59 +0100, Christoph Hellwig wrote: > Btw, can we please take away this discussion from ksummit-attendees? It's got > absolutely nothing to do with kernel summit and is getting fairly annoying. Ack. Additionally, iirc, we had decided that - We don't cross post multiple lists - We drop the annoying subject tags As is, all I see is some attempt at doing an lkml dup, which is pointless Ben. -- 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
Johan Herland writes:
> 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.
[Ok, admittedly I don't really know what to quote from your message,
since I'm mostly responding to the overall concept.]
I like the idea of putting all that in hooks, but I have two
observations:
* Signed-off-by: is already such a case (and was probably also added for
the kernel?) that _could_ have been dealt with using {prepare-,}commit-msg,
but has its own support in various git tools.
* In your list
> Fixes:
> Reported-by:
> Suggested-by:
> Improved-by:
> Acked-by:
> Reviewed-by:
> Tested-by:
> Signed-off-by:
and I might add
Cherry-picked-from:
Reverts:
if one were to phrase that as a footer/pseudoheader, observe that
there are only two kinds of these: footers that contain identities,
and footers that contain references to commits.
So why not support these use-cases? We could have something like
footer.foo.* configuration, e.g.
[footer "fixes"]
type = commit
suggest = true
[footer "acked-by"]
type = identity
where 'suggest' (please suggest a better name) means that git-commit
will put a blank one in the commit message template for you to fill in.
'commit' and 'identity' can have some elementary expansion and
validation tied to them. Some easy extensiblity (hooks?) might not
hurt, but then as you point out, the existing hooks already cover that.
Perhaps we could also have, for Gerrit (cf. [1]):
[footer "change-id"]
type = uuid
though admittedly I haven't investigated if it's okay to just put a
random string there, or it needs to have a specific value.
[1] http://thread.gmane.org/gmane.comp.version-control.git/236429
--
Thomas Rast
t...@thomasrast.ch
--
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 option to add Fixes: line
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: >> [...] >> First of all, let me show my ignorance. How formalized is the use of >> metadata lines at the end of a commit message? I don't remember seeing >> documentation about such lines in general (as opposed to documentation >> about particular types of lines). Is the format defined well enough >> that tools that don't know about a particular line could nonetheless >> preserve it correctly? Is there/should there be a standard recommended >> order of metadata lines? (For example, should "Fixes:" lines always >> appear before "Signed-off-by" lines, or vice versa?) If so, is it >> documented somewhere and preserved by tools when such lines are >> added/modified? Should there be support for querying such lines? > > While it isn't very well documented in git itself, metadata lines are > quite standardized. See Documentation/SubmittingPatches and > Documentation/development-process/5.Posting in the Linux kernel, for an > explanation of "Reported-by:", "Tested-by:", "Reviewed-by:", > "Suggested-by:", and "Acked-by:". And git itself looks for a very > specific format; the has_conforming_footer function looks for a footer > consisting exclusively of rfc2822-style (email-style) header lines to > decide whether to append "Signed-off-by:" (and now "Fixes:") directly to > that block or to create a new block. It would be nice to document exactly what "rfc2822-style" means in this context (e.g., are line breaks supported? Encoding changes? etc.) so that (1) new inventors of trailer lines can make sure that they conform to what Git expects and (2) Git could someday add some generic facilities for handling these fields (e.g., adding/removing/tidying them in a commit-msg hook; grepping through them by name) and be relatively sure that it is not breaking somebody's metadata. I'm not saying that it's your job; only that it would be helpful for ideas like yours. > [...] >> 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. > [...] >> 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,
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
Junio C Hamano writes:
> There are unbound number of kinds of trailers people would want to
> add, depending on their projects' needs. We should not have to add
> a specific support for a tailer like this one, before thinking
> through to see if we can add generic support for adding arbitrary
> trailers to avoid code and interface bloat.
>
> Think of the existing --signoff as a historical mistake. Such a
> generic "adding arbitrary trailers" support, when done properly,
> should be able to express what "--signoff" does, and we should be
> able to redo "--signoff" as a special case of that generic "adding
> arbitrary trailers" support, and at that point, "Fixes:" trailer the
> kernel project wants to use should fall out as a natural consequence.
Thinking aloud further, what I had in mind was along the lines of
the following.
* The most generic external interface would be spelled as
--trailer [=]
where can be things like "signoff", "closes", "acked-by",
"change-id", "fixes", etc.; they can be taken from an unbounded
set. The historical "--signoff" can become a short-hand for
"--trailer signoff". More than one "--trailer" option can be
given on a single command line.
* The token is used to look into the configuration, e.g.,
[commitTrailer "signoff"]
style = append-norepeat
trailer = Signed-off-by
command = echo "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"'
[commitTrailer "change-id"]
style = append-only-if-missing
trailer = Change-Id
command = 'git hash-object -t commit --stdin <$GIT_PROTO_COMMIT'
[commitTrailer "fixes"]
style = overwrite
trailer = Fixes
command = 'git log -1 --oneline --format="%h (%s)" --abbrev-commit=14
$ARG'
where
- "commitTrailer..style" defines the interaction with
existing trailer of the same kind (e.g. S-o-b: accumulates by
appending, but we try not to repeat the same sign-off twice
which would show you forwarding your own message you are the
last person in the Sign-off chain; Fixes: if there is already
one will remove the old one and replaces; etc.);
- "commitTrailer..trailer" defines the trailer label at
the beginning of the trailer line;
- "commitTrailer..command" gives the command to run to
obtain the payload after the "trailer" label. A handful
obvious and useful variables are exported for the command to
use, and is exported as $ARG, if present.
With the most generic syntax, with the above commitTrailer.fixes.*
configuration, I would imagine that you can say something like:
git commit --trailer fixes="v2.6.12^{/^i386: tweak frobnitz}"
to say that the first commit you find traversing the history of
v2.6.12 whose title is "i386: tweak frobnitz" was faulty, and you
are creating a commit that corrects its mistake.
Giving some default configuration to often used trailer types
(e.g. configuration for "--trailer signoff") and promoting some
commonly used ones into a separate built-in option (e.g. an option
"--signoff" that does not have to say "--trailer signoff") are
entirely separate issues, and only time can nudge us into evaluating
individual types of trailers.
--
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: [ksummit-attendees] [PATCH] commit: Add -f, --fixes option to add Fixes: line
Btw, can we please take away this discussion from ksummit-attendees? It's got absolutely nothing to do with kernel summit and is getting fairly annoying. Thanks! -- 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 10/28/2013 08:16 AM, Josh Triplett wrote:
> On Sun, Oct 27, 2013 at 06:52:18PM -0700, Junio C Hamano wrote:
>> There are unbound number of kinds of trailers people would want to
>> add, depending on their projects' needs. We should not have to add
>> a specific support for a tailer like this one, before thinking
>> through to see if we can add generic support for adding arbitrary
>> trailers to avoid code and interface bloat.
>>
>> Think of the existing --signoff as a historical mistake. Such a
>> generic "adding arbitrary trailers" support, when done properly,
>> should be able to express what "--signoff" does, and we should be
>> able to redo "--signoff" as a special case of that generic "adding
>> arbitrary trailers" support, and at that point, "Fixes:" trailer the
>> kernel project wants to use should fall out as a natural consequence.
>
> Well, the add_signoff_extra function I added makes it easy to add any
> kind of trailing data you want to a commit; the question just becomes
> what the UI looks like to drive that.
>
> Would you be OK with a solution that pushes the specific supported
> footer lines into git's configuration, and then supplies default
> configuration for common cases such as Fixes? The option could become
> -f/--footer, and the configuration would specify how to parse various
> arguments of -f and turn them into something. For example:
>
> [footer "Fixes"]
> abbrev = f
> arg = commit
> format = %h ('%s')
It could be even more decoupled, for example like this:
[footer "Fixes"]
type = pipe
cmd = awk '{ print $1 }' | git log --stdin --no-walk --abbrev=12
--pretty=format:\"Fixes: %h ('%s')\"
Note that the command is written to be idempotent; that way git could
re-pipe the old value(s) of the footer though the command if necessary.
And it can handle multiple lines, since some callback scripts might
want to see all of them at once.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 06:52:18PM -0700, Junio C Hamano wrote:
> There are unbound number of kinds of trailers people would want to
> add, depending on their projects' needs. We should not have to add
> a specific support for a tailer like this one, before thinking
> through to see if we can add generic support for adding arbitrary
> trailers to avoid code and interface bloat.
>
> Think of the existing --signoff as a historical mistake. Such a
> generic "adding arbitrary trailers" support, when done properly,
> should be able to express what "--signoff" does, and we should be
> able to redo "--signoff" as a special case of that generic "adding
> arbitrary trailers" support, and at that point, "Fixes:" trailer the
> kernel project wants to use should fall out as a natural consequence.
Well, the add_signoff_extra function I added makes it easy to add any
kind of trailing data you want to a commit; the question just becomes
what the UI looks like to drive that.
Would you be OK with a solution that pushes the specific supported
footer lines into git's configuration, and then supplies default
configuration for common cases such as Fixes? The option could become
-f/--footer, and the configuration would specify how to parse various
arguments of -f and turn them into something. For example:
[footer "Fixes"]
abbrev = f
arg = commit
format = %h ('%s')
git commit -f Cc:sta...@vger.kernel.org -f f:bad-commit ...
The Cc line there would go unparsed since there's no specific support
for it, while the 'f:bad-commit' would get expanded by the configuration
above to parse bad-commit as a committish and format it using the
specified pretty format.
Look reasonable? I could start out by adding support for footer lines
that take commits as arguments and format them using arbitrary pretty
strings, and leave room for future expansion to support footers that
reference idents (given some way to expand idents from some shorter
form, otherwise there's no point).
- Josh Triplett
--
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/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
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
There are unbound number of kinds of trailers people would want to add, depending on their projects' needs. We should not have to add a specific support for a tailer like this one, before thinking through to see if we can add generic support for adding arbitrary trailers to avoid code and interface bloat. Think of the existing --signoff as a historical mistake. Such a generic "adding arbitrary trailers" support, when done properly, should be able to express what "--signoff" does, and we should be able to redo "--signoff" as a special case of that generic "adding arbitrary trailers" support, and at that point, "Fixes:" trailer the kernel project wants to use should fall out as a natural consequence. -- 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 10/26/13 18:34, Josh Triplett wrote:
Linux Kernel ... "Fixes:" line ... containing an abbreviated commit hash
This helps people (or automated tools) determine how far to backport
I beg pardon if I'm rehearsing an old debate, but it seems to me it
would be better and worthwhile to bring more of git to bear by adding
`reference` links as follows from considering this proposed sequence:
# ...G---B---...history-with-bug-at-B
Gprime=`git commit-tree --reference G`
Bprime=`git commit-tree --reference B -p $Gprime`
# ...G---B---... history-with-bug-at-B
# : : # <-- `:`'s are `reference` links
# G'--B'$Bprime is a mergeable cherry-pick for B
`reference` links have no enforced semantics. Teach all current logic to
ignore them (fetch doesn't fetch through them, fsck doesn't care, etc.).
Elaborating some of the good parts:
* If the author and committer data are left untouched when
`commit-tree`'s tree and message arguments are defaulted, as above, to
the referenced commit's tree and message, the resulting commit is unique.
* Bullet-proof cherry-pick creation becomes easy and idempotent:
git-make-cherry-pick() {
local picked=$1
set -- `git rev-list --parents $picked^!`
shift
local parents
local parent
local p2
for parent; do
p2="$p2 -p `git commit-tree --reference $parent`"
done
git commit-tree --reference $picked $parents`
}
* Which makes the created commit id a fully-implemented _change-id_ for
the referenced commit:
git merge $(git-make-cherry-pick $B)
can be done from anywhere, merge won't have to rely on patch-id's
to detect cherry-picks done this way.
* A bugged commit gets fixed by fixing its reference commit and merging
normally, worry-free:
...G---B ... -F Merge fix X for a bug in B
: : /
G'--B'---X X's commit message is the `Fixes:` equivalent
Bugfix commit X can be safely merged anywhere. Worst case, `git
merge -s ours --no-commit X` and do whatever you would have done otherwise.
`merge` might usefully be updated to warn about merging from a commit
with only a reference parent, I think merging from `G'` would probably
be a mistake.
---
So, this is as far as I've gotten with this, is there reason to think it
should or shouldn't be pursued?
--
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
[Sorry I already sent the reply below to Johan only instead of everyone.] Hi Johan, On Sun, Oct 27, 2013 at 11:59 AM, Johan Herland wrote: > On Sun, Oct 27, 2013 at 10:20 AM, Josh Triplett wrote: >> >> ...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. 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) 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) > 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. Yeah, the commit message hook could do some more processing if the user adds or changes stuff. > 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. Great! > 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? > 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. Best regards, Christian. -- 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 10/27/2013 05:30 PM, Thomas Rast wrote:
> Stefan Beller writes:
>
>> I assembled an overview table, which plots the long options of
>> git commands by the short letters.
> [...]
>> (In case thunderbird messes it up, here it is again
>> http://pastebin.com/raw.php?i=JBci2Krx)
>>
>> As you can see, f is always --force except for git-config, where it is --file
>
> Woah! Impressive work. Did you autogenerate this? If so, can we have
> it as a small make target somewhere? If not, can you send a patch to
> put your table in Documentation somewhere?
>
I thought about generating it by parsing the man pages,
but I felt it would not be reliable enough and quite time consuming
to come up with a parser. Parsing the C sources however also seemed time
consuming,
so I decided to come up with this patch:
--8<--
Subject: [PATCH] parse-options: print all options having short and long form
and exit
This patch basically only prints all options which have a long and a short form
and then aborts the program. A typical output looks like this:
./git-add
add, n, dry-run
add, v, verbose
add, i, interactive
add, p, patch
add, e, edit
add, f, force
add, u, update
add, N, intent-to-add
add, A, all
---
parse-options.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/parse-options.c b/parse-options.c
index 62e9b1c..b356ca9 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -500,6 +500,12 @@ int parse_options(int argc, const char **argv, const char
*prefix,
{
struct parse_opt_ctx_t ctx;
+ for (; options->type != OPTION_END; options++) {
+ if (options->long_name && options->short_name)
+ printf("%s, %c, %s\n", argv[0], options->short_name,
options->long_name);
+ }
+ exit(1);
+
parse_options_start(, argc, argv, prefix, options, flags);
switch (parse_options_step(, options, usagestr)) {
case PARSE_OPT_HELP:
--
1.8.4.1.605.g23c6912
Unfortunately we can only check git commands, which are written in C.
You'll notice all the perl/shell written commands are missing (rebase, etc).
Also a few commands written in C cannot easily be picked up, as they do stuff
before calling parse_options. [typically something like "if (argc != 4)
print_usage();"]
These commands are also not contained.
The generation of the table however was just a little python:
--8<--
#!/usr/bin/python
cmds="""git-add
git-apply
git-archive
git-branch
git-check-attr
git-check-ignore
git-check-mailmap
git-checkout
git-checkout-index
git-cherry
git-cherry-pick
git-clean
git-clone
git-column
git-commit
git-config
git-count-objects
git-credential-cache
git-credential-store
git-describe
git-fetch
git-fmt-merge-msg
git-for-each-ref
git-format-patch
git-fsck
git-fsck-objects
git-gc
git-grep
git-hash-object
git-help
git-init
git-init-db
git-log
git-ls-files
git-ls-tree
git-merge
git-merge-base
git-merge-file
git-merge-ours
git-mktree
git-mv
git-name-rev
git-notes
git-pack-objects
git-pack-refs
git-prune
git-prune-packed
git-push
git-read-tree
git-reflog
git-remote
git-repack
git-replace
git-rerere
git-reset
git-revert
git-rev-parse
git-rm
git-show
git-show-branch
git-show-ref
git-stage
git-status
git-symbolic-ref
git-tag
git-update-index
git-update-ref
git-update-server-info
git-verify-pack
git-verify-tag
git-whatchanged
git-write-tree"""
import subprocess
shorts={}
cmdoptions={}
for cmd in cmds.split("\n"):
p = subprocess.Popen("./"+cmd, stdout=subprocess.PIPE)
p.wait()
lines = p.stdout.read()
for line in lines.split("\n"):
if not len(line):
continue
name, short, long = line.split(",")
if not short in shorts:
shorts[short] = len(long)
else:
shorts[short] = max(shorts[short], len(long))
if not name in cmdoptions:
cmdoptions[name] = {}
cmdoptions[name][short] = long
longest_cmd = 0
for cmd in cmdoptions:
longest_cmd = max(longest_cmd, len(cmd))
print " "*(longest_cmd-len("Name\\short")), "Name\\short",
for short in shorts:
print "|" + " "*(1+shorts[short]-len(short)) + short,
print
for cmd in cmdoptions:
print " "*(longest_cmd-len(cmd)), cmd,
for short in shorts:
s = ""
if short in cmdoptions[cmd]:
s = cmdoptions[cmd][short]
print "|" + " "*(1+shorts[short]-len(s)) + s,
print " ", cmd
--8<--
I am not sure if we should add such code to the git code base, as it would need
some cleanup.
The existing table however would become outdated fast?
So I do not have a good idea, how such a table could be easily incorporated and
kept up to date.
Thanks,
Stefan
--
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
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
Stefan Beller writes: > I assembled an overview table, which plots the long options of > git commands by the short letters. [...] > (In case thunderbird messes it up, here it is again > http://pastebin.com/raw.php?i=JBci2Krx) > > As you can see, f is always --force except for git-config, where it is --file Woah! Impressive work. Did you autogenerate this? If so, can we have it as a small make target somewhere? If not, can you send a patch to put your table in Documentation somewhere? -- Thomas Rast t...@thomasrast.ch -- 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 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 option to add Fixes: line
On 10/27/2013 09:09 AM, 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 > > 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. > I assembled an overview table, which plots the long options of git commands by the short letters. Here it is: (Best viewed with a *large* screen and monospace font) Name\short | C | B | A | G | F |E | H | O | N |L | S |R | P | W |X | c | b | a | g | f |e |d | k |i | o | n | m | l | s |r | q | p |w | v | u | t | z |x | 3 | 2 status || || || | | || | | | | | | | branch | | || | | | | | | | | short | ||| | verbose | untracked-files | | null | | | status help || || || | | || | | | | | | | | all | guides || | | | info | | | man | | | ||| web | | | | | | | help show || || || | | || | | | | | | | | | || | | | | | | | | | | quiet || | | | | | | | show revert || || || | | || | | | | | strategy-option | | | | || edit | | | | | no-commit | mainline | | signoff | ||| | |
Re: [Ksummit-2013-discuss] [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Sun, Oct 27, 2013 at 01:03:47AM -0700, Michel Lespinasse wrote: > On Sun, Oct 27, 2013 at 12:14 AM, Josh Triplett wrote: > >> > +-f :: > >> > +--fixes=:: > >> > + Add Fixes line for the specified commit at the end of the commit > >> > + log message. This line includes an abbreviated commit hash for > >> > + the specified commit; the `core.abbrev` option determines the > >> > + length of the abbreviated commit hash used, with a minimum length > >> > + of 12 hex digits. > >> > >> You might also mention that the "Fixes:" line includes the old commit's > >> subject line. > > > > I only mentioned the abbreviated commit hash because it was necessary to > > explain the factors affecting hash length. -s, above, doesn't mention > > that the Signed-off-by line includes the name and email address of the > > committer. > > I do wonder, if we're going to bake into git the idea that too-short > abbreviated sha1s don't make sense, why don't we just change the > core.abbrev default to 12 everywhere rather than just in this one > command ? You won't get any argument from me on that one. I personally would have argued for making the hashes 40 characters always, but in any case bumping up the default (and minimum) for core.abbrev seems entirely sensible. - Josh Triplett -- 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 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. - Josh Triplett -- 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 03:33:19PM +0700, Duy Nguyen wrote:
> On Sun, Oct 27, 2013 at 8:34 AM, Josh Triplett wrote:
> > Add a command line option for git commit to automatically construct the
> > "Fixes:" line for a commit. This avoids the need to manually construct
> > that line by copy-pasting the commit hash and subject.
>
> But you still have to copy/paste the hash in command line. I wonder if
> we should approach it differently: the user writes "Fixes: " in
> the commit message, then git detects these lines and expands them
Then you have to copy/paste the hash into the commit message; either way
you're not getting around that. However, note that you can pass a ref
instead of a commit hash, if you happen to have saved a tag pointing to
the broken ref. (Or, for instance, if you have it from a bisection.)
I could imagine supporting that approach in addition (via a commit-msg
hook, for instance), but I'd still like to have the command-line option
to git commit.
> using a user-configured format. For the kernel circle, the format
> would be "%h ('%s')" (I'll need to think how to let the user say
> "minimum 12 chars").
I considered making the format configurable, and that's easy enough to
do, but I wanted to start out with the simplest patch that achieved the
goal, on the theory that it's easy to add configurability later if
anyone actually needs it.
> Other projects need to refer to old commits sometimes in commit
> messages too and this could be extended further to expand inline
> abbrev sha-1s, but to not break the text alignment badly, maybe
> footnotes will be created to store subjects and stuff, rather than do
> inline expansion. For example,
>
> commit 1232343 breaks something.
>
> becomes
>
> comit 1232343 [1] breaks something
>
> [1] 123234332131 (do something wrong - at this date)
Easily done via a commit-msg hook, if you want that.
- Josh Triplett
--
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:34 AM, Josh Triplett wrote:
> Add a command line option for git commit to automatically construct the
> "Fixes:" line for a commit. This avoids the need to manually construct
> that line by copy-pasting the commit hash and subject.
But you still have to copy/paste the hash in command line. I wonder if
we should approach it differently: the user writes "Fixes: " in
the commit message, then git detects these lines and expands them
using a user-configured format. For the kernel circle, the format
would be "%h ('%s')" (I'll need to think how to let the user say
"minimum 12 chars").
Other projects need to refer to old commits sometimes in commit
messages too and this could be extended further to expand inline
abbrev sha-1s, but to not break the text alignment badly, maybe
footnotes will be created to store subjects and stuff, rather than do
inline expansion. For example,
commit 1232343 breaks something.
becomes
comit 1232343 [1] breaks something
[1] 123234332131 (do something wrong - at this date)
--
Duy
--
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
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 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. -- Thomas Rast t...@thomasrast.ch -- 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: [Ksummit-2013-discuss] [PATCH] commit: Add -f, --fixes option to add Fixes: line
On Sun, Oct 27, 2013 at 12:14 AM, Josh Triplett wrote: >> > +-f :: >> > +--fixes=:: >> > + Add Fixes line for the specified commit at the end of the commit >> > + log message. This line includes an abbreviated commit hash for >> > + the specified commit; the `core.abbrev` option determines the >> > + length of the abbreviated commit hash used, with a minimum length >> > + of 12 hex digits. >> >> You might also mention that the "Fixes:" line includes the old commit's >> subject line. > > I only mentioned the abbreviated commit hash because it was necessary to > explain the factors affecting hash length. -s, above, doesn't mention > that the Signed-off-by line includes the name and email address of the > committer. I do wonder, if we're going to bake into git the idea that too-short abbreviated sha1s don't make sense, why don't we just change the core.abbrev default to 12 everywhere rather than just in this one command ? -- Michel "Walken" Lespinasse A program is never fully debugged until the last user dies. -- 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 06:42:44AM +0100, Michael Haggerty wrote: > On 10/27/2013 02:34 AM, Josh Triplett wrote: > > Linux Kernel Summit 2013 decided on a commit message convention to > > identify commits containing bugs fixed by a commit: a "Fixes:" line, > > included in the standard commit footer (along with "Signed-off-by:" if > > present), containing an abbreviated commit hash (at least 12 characters > > to keep it valid for a long time) and the subject of the commit (for > > human readers). This helps people (or automated tools) determine how > > far to backport a commit. > > > > Add a command line option for git commit to automatically construct the > > "Fixes:" line for a commit. This avoids the need to manually construct > > that line by copy-pasting the commit hash and subject. > > > > Also works with --amend to modify an existing commit's message. To add > > a Fixes line to an earlier commit in a series, use rebase -i and add the > > following line after the existing commit: > > x git commit --amend --no-edit -f $commit_containing_bug > > > > Generalize append_signoff to support appending arbitrary extra lines to > > a commit in the signoff block; this avoids duplicating the logic to find > > or construct that block. > > I have a few comments and questions about the design of this feature: > > First of all, let me show my ignorance. How formalized is the use of > metadata lines at the end of a commit message? I don't remember seeing > documentation about such lines in general (as opposed to documentation > about particular types of lines). Is the format defined well enough > that tools that don't know about a particular line could nonetheless > preserve it correctly? Is there/should there be a standard recommended > order of metadata lines? (For example, should "Fixes:" lines always > appear before "Signed-off-by" lines, or vice versa?) If so, is it > documented somewhere and preserved by tools when such lines are > added/modified? Should there be support for querying such lines? While it isn't very well documented in git itself, metadata lines are quite standardized. See Documentation/SubmittingPatches and Documentation/development-process/5.Posting in the Linux kernel, for an explanation of "Reported-by:", "Tested-by:", "Reviewed-by:", "Suggested-by:", and "Acked-by:". And git itself looks for a very specific format; the has_conforming_footer function looks for a footer consisting exclusively of rfc2822-style (email-style) header lines to decide whether to append "Signed-off-by:" (and now "Fixes:") directly to that block or to create a new block. I do think there should be additional support for such lines in git, such as a git commit option to add "Cc:" lines (via a --cc-cmd like get_maintainer.pl run at commit time), or fast options in rebase -i to append arbitrary footer lines to a commit. > Too bad your proposed new option sounds so similar to --fixup, which > does something conceptually similar albeit very different in effect. > This will likely lead to confusion. Given that the line is named "Fixes:", I don't think the name of the option will extend the confusion any further. :) > 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. > Conversely, I suppose one could ask whether there should be some way to > prevent "fixup!" or "squash!" commits from being pushed, at least > without some kind of --force option. This could of course be enforced > by a hook but it might be nice to
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
One of the uses of the Fixes commit line is so that when we fix a security bug that has been in mainline for a while, it can be tricky to determine whether it should be backported in to the various stable branches. For example, let's suppose the security bug (or any bug, but one of the contexts where this came up was for security fixes) was introduced in 3.5, and backported into the 3.2.x kernel series, but couldn't be applied into the 3.2.0 kernel series. The security fix was introduced in 3.12, and so it would be obvious that it should be backported to the 3.10 kernel series, but it might not be so obvious that it would also be required for the 3.2.x long-term stable series. So the inclusion of the Fixes: line provides this critical bit of information. It's also useful not just for the long-term stable tree maintainers, but the maintainers of distro kernels would also find it to be very useful. > 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? > > In workflows that make more use of cherry-picking, it could be that the > original buggy commit was cherry-picked to a different branch. In this > case the user would probably want to cherry-pick the fixing commit to > the other branch, too. But then the commit that it would be fixing > would have a different SHA-1 than it did on the original branch. A > check that the "Fixes:" line refers to an ancestor of the current commit > could warn against such errors. (In some cases it might be possible to > use cherry-pick's "-x" lines to figure out how to rewrite the "Fixes:" > line, but I doubt that would work often enough to be worthwhile.) I believe that in the discussions we had, it was assumed that the Fixes: line would reference the commit in the mainline kernel tree. i.e., it would always reference the commit which introduced the bug in 3.5, even if the commit-id after the buggy commit was backported to 3.2.x would obviously be different. Presumably the distro kernel maintainer would be able to find the commit in Linus's tree and then try to find the corresponding commit in the distro kernel git tree, probably by doing string searches over "git log". We could actually do a much more elegant job if we did have the concept of commit identity (i.e., ChangeID's) baked into git. That way, there would be a constant ChangeID that would remain constant not only across revisions of a patch under development, but also when the commit is cherry picked into stable branches. If we had that, then instead of doing string searches on git log output, we could imagine a web and/or command line interface where given a ChangeID, it would tell you which branches or which tags contained the same semantic patch. Of course, as soon as you do that, then if the multiple commits get squashed together, you might need to have to support multiple ChangeID's associated with one commit, at which point it becomes incompatible with Gerrit's use of this feature. So we could add all sorts of complexity, but it's not obvious to me that it's worth it. > First of all, let me show my ignorance. How formalized is the use of > metadata lines at the end of a commit message? I don't remember seeing > documentation about such lines in general (as opposed to documentation > about particular types of lines). Is the format defined well enough > that tools that don't know about a particular line could nonetheless > preserve it correctly? Is there/should there be a standard recommended > order of metadata lines? (For example, should "Fixes:" lines always > appear before "Signed-off-by" lines, or vice versa?) If so, is it > documented somewhere and preserved by tools when such lines are > added/modified? Should there be support for querying such lines? Internally inside Google, we have tools that will assist in forward porting local changes from a 3.x based kernel to a 3.y kernel, to make sure that all local changes are properly accounted for and none are accidentally dropped during the rebase operation. So we have various new metadata lines that we add internally, for example: Upstream-3.x-SHA1: for commits in newer kernels that have been backported Origin-3.x-SHA1: to indicate the commit-id of a patch that was forward ported as part of a rebase operation from 3.x to 3.9 Upstream-Dropped-3.x-SHA1: As part of an empty commit to indicate that a patch that was originally in our tree, has since been pushed upstream, so we can drop it as part of the rebase to the 3.y kernel. etc. Other projects have various metadata lines to reference a bug-tracker id number; folks may have seen commits with various metadata id's in public git repositories such as: Google-Bug-Id:
Re: [PATCH] commit: Add -f, --fixes option to add Fixes: line
On 10/27/2013 02:34 AM, Josh Triplett wrote: > Linux Kernel Summit 2013 decided on a commit message convention to > identify commits containing bugs fixed by a commit: a "Fixes:" line, > included in the standard commit footer (along with "Signed-off-by:" if > present), containing an abbreviated commit hash (at least 12 characters > to keep it valid for a long time) and the subject of the commit (for > human readers). This helps people (or automated tools) determine how > far to backport a commit. > > Add a command line option for git commit to automatically construct the > "Fixes:" line for a commit. This avoids the need to manually construct > that line by copy-pasting the commit hash and subject. > > Also works with --amend to modify an existing commit's message. To add > a Fixes line to an earlier commit in a series, use rebase -i and add the > following line after the existing commit: > x git commit --amend --no-edit -f $commit_containing_bug > > Generalize append_signoff to support appending arbitrary extra lines to > a commit in the signoff block; this avoids duplicating the logic to find > or construct that block. I have a few comments and questions about the design of this feature: First of all, let me show my ignorance. How formalized is the use of metadata lines at the end of a commit message? I don't remember seeing documentation about such lines in general (as opposed to documentation about particular types of lines). Is the format defined well enough that tools that don't know about a particular line could nonetheless preserve it correctly? Is there/should there be a standard recommended order of metadata lines? (For example, should "Fixes:" lines always appear before "Signed-off-by" lines, or vice versa?) If so, is it documented somewhere and preserved by tools when such lines are added/modified? Should there be support for querying such lines? There is another thread [1] proposing the addition of a "Change-Id:" metadata line, so maybe now would be a good time to discuss such lines in general. Too bad your proposed new option sounds so similar to --fixup, which does something conceptually similar albeit very different in effect. This will likely lead to confusion. 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.) Conversely, I suppose one could ask whether there should be some way to prevent "fixup!" or "squash!" commits from being pushed, at least without some kind of --force option. This could of course be enforced by a hook but it might be nice to have some protection by default. 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? In workflows that make more use of cherry-picking, it could be that the original buggy commit was cherry-picked to a different branch. In this case the user would probably want to cherry-pick the fixing commit to the other branch, too. But then the commit that it would be fixing would have a different SHA-1 than it did on the original branch. A check that the "Fixes:" line refers to an ancestor of the current commit could warn against such errors. (In some cases it might be possible to use cherry-pick's "-x" lines to figure out how to rewrite the "Fixes:" line, but I doubt that would work often enough to be worthwhile.) > Signed-off-by: Josh Triplett > --- > Documentation/git-commit.txt | 12 ++-- > builtin/commit.c | 29 +++-- > sequencer.c | 31 +++ > sequencer.h | 3 +++ > t/t7502-commit.sh| 39 ++- > 5 files changed, 101 insertions(+), 13 deletions(-) > > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt > index 1a7616c..fcc6ed2 100644 > --- a/Documentation/git-commit.txt > +++ b/Documentation/git-commit.txt > @@ -8,8 +8,8 @@ git-commit - Record changes to the repository > SYNOPSIS > > [verse] > -'git commit' [-a | --interactive | --patch] [-s] [-v] [-u] [--amend] > -[--dry-run] [(-c | -C | --fixup | --squash) ] > +'git commit' [-a |
[PATCH] commit: Add -f, --fixes option to add Fixes: line
Linux Kernel Summit 2013 decided on a commit message convention to
identify commits containing bugs fixed by a commit: a "Fixes:" line,
included in the standard commit footer (along with "Signed-off-by:" if
present), containing an abbreviated commit hash (at least 12 characters
to keep it valid for a long time) and the subject of the commit (for
human readers). This helps people (or automated tools) determine how
far to backport a commit.
Add a command line option for git commit to automatically construct the
"Fixes:" line for a commit. This avoids the need to manually construct
that line by copy-pasting the commit hash and subject.
Also works with --amend to modify an existing commit's message. To add
a Fixes line to an earlier commit in a series, use rebase -i and add the
following line after the existing commit:
x git commit --amend --no-edit -f $commit_containing_bug
Generalize append_signoff to support appending arbitrary extra lines to
a commit in the signoff block; this avoids duplicating the logic to find
or construct that block.
Signed-off-by: Josh Triplett
---
Documentation/git-commit.txt | 12 ++--
builtin/commit.c | 29 +++--
sequencer.c | 31 +++
sequencer.h | 3 +++
t/t7502-commit.sh| 39 ++-
5 files changed, 101 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 1a7616c..fcc6ed2 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -8,8 +8,8 @@ git-commit - Record changes to the repository
SYNOPSIS
[verse]
-'git commit' [-a | --interactive | --patch] [-s] [-v] [-u] [--amend]
- [--dry-run] [(-c | -C | --fixup | --squash) ]
+'git commit' [-a | --interactive | --patch] [-s] [-f ] [-v] [-u]
+ [--amend] [--dry-run] [(-c | -C | --fixup | --squash) ]
[-F | -m ] [--reset-author] [--allow-empty]
[--allow-empty-message] [--no-verify] [-e] [--author=]
[--date=] [--cleanup=] [--[no-]status]
@@ -156,6 +156,14 @@ OPTIONS
Add Signed-off-by line by the committer at the end of the commit
log message.
+-f ::
+--fixes=::
+ Add Fixes line for the specified commit at the end of the commit
+ log message. This line includes an abbreviated commit hash for
+ the specified commit; the `core.abbrev` option determines the
+ length of the abbreviated commit hash used, with a minimum length
+ of 12 hex digits.
+
-n::
--no-verify::
This option bypasses the pre-commit and commit-msg hooks.
diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..9bbcd8a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -123,6 +123,7 @@ static int use_editor = 1, include_status = 1;
static int show_ignored_in_status, have_option_m;
static const char *only_include_assumed;
static struct strbuf message = STRBUF_INIT;
+static struct strbuf fixes = STRBUF_INIT;
static enum status_format {
STATUS_FORMAT_NONE = 0,
@@ -133,6 +134,28 @@ static enum status_format {
STATUS_FORMAT_UNSPECIFIED
} status_format = STATUS_FORMAT_UNSPECIFIED;
+static int opt_parse_f(const struct option *opt, const char *arg, int unset)
+{
+ struct strbuf *sb = opt->value;
+ if (unset) {
+ strbuf_setlen(sb, 0);
+ } else {
+ struct pretty_print_context ctx = {0};
+ struct commit *commit;
+
+ commit = lookup_commit_reference_by_name(arg);
+ if (!commit)
+ die(_("could not lookup commit %s"), arg);
+ ctx.output_encoding = get_commit_output_encoding();
+ ctx.abbrev = DEFAULT_ABBREV;
+ if (ctx.abbrev < 12)
+ ctx.abbrev = 12;
+ format_commit_message(commit, "Fixes: %h ('%s')\n", sb, );
+ }
+
+ return 0;
+}
+
static int opt_parse_m(const struct option *opt, const char *arg, int unset)
{
struct strbuf *buf = opt->value;
@@ -718,7 +741,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
if (clean_message_contents)
stripspace(, 0);
- if (signoff) {
+ if (signoff || fixes.len) {
/*
* See if we have a Conflicts: block at the end. If yes, count
* its size, so we can ignore it.
@@ -742,7 +765,8 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
previous = eol;
}
- append_signoff(, ignore_footer, 0);
+ append_signoff_extra(, ignore_footer,
+signoff ? 0 : APPEND_EXTRA_ONLY, );
}
if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
@@ -1463,6 +1487,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
