Re: [PATCH] commit: Add -f, --fixes <commit> 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:

quoted

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.

[...]

quoted

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.

[...]

quoted

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.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/