Re: [PATCH] contrib/subtree bugfix: Can't `add` annotated tag

James Denholm [off-list ref] writes:

cmd_add_commit() is passed FETCH_HEAD by cmd_add_repository, which is
then rev-parsed into an object ID. However, if the user is fetching a
tag rather than a branch HEAD, such as by executing:

$ git subtree add -P oldGit https://github.com/git/git.git tags/v1.8.0

The object ID is a tag and is never peeled, and the git commit-tree call
(line 561) slaps us in the face because it doesn't handle tag IDs.

The "rev" (not "revs") seems to be used by more things than the
final commit-tree state.  Are we losing some useful information by
peeling it too early like this patch does?  The reason why we
stopped peeling when writing FETCH_HEAD was because we wanted to
record the fact that we merged a tag (and use the GPG signature if
found in it) when constructing the log message for the merge, and
peeling the tag too early and recording the commit in FETCH_HEAD
would make it impossible to do, and I am wondering if this change is
making the same kind of mistake here.

I see that add_msg does not use anything useful from latest_new, so
with the current state of the code, it does not make that much
difference (except that it says "from commit '$latest_new'", and by
peeling, the fact that the user wanted to use a tag is lost from the
result).

On a side note, if merging a tag without --squash, git merge recognises
that it's a tag and adds a note to the merge commit body. It may be
worth mimicking this when using "subtree merge --squash" or
"subtree add".

Yes, and this change makes such a change harder to implement on top,
I suspect.

Would it be sufficient to do

	git commit-tree $tree $headp -p "$rev^0"

in that "not squashing" codepath instead?

Junio C Hamano [off-list ref] wrote:

The "rev" (not "revs") seems to be used by more things than the
final commit-tree state.  Are we losing some useful information by
peeling it too early like this patch does? (...)

You're not wrong, actually, peeling at the last minute (or at least
later) would be a better choice. I'd suggest that we aren't losing
currently-useful information (as it'd be rare-if-ever that a user would
look at a hash in their commit logs and think "Oh, that's that tag!"),
but certainly with future development in mind it's more ideal.

I see that add_msg does not use anything useful from latest_new, so
with the current state of the code, it does not make that much
difference (except that it says "from commit '$latest_new'", and by
peeling, the fact that the user wanted to use a tag is lost from the
result).

Yeah, that might be a worthy thing to porcelain-up in the future with
logging the tag name rather than, or in addition to, the hash, as well
as a similar change in add_squashed_msg.

Would it be sufficient to do

        git commit-tree $tree $headp -p "$rev^0"

in that "not squashing" codepath instead?

On line 561, sure. Do you want me to do a re-roll?

On Fri, May 09, 2014 at 05:36:15PM +1000, James Denholm wrote:

Junio C Hamano [off-list ref] wrote:

quoted

Would it be sufficient to do

        git commit-tree $tree $headp -p "$rev^0"

in that "not squashing" codepath instead?

On line 561, sure. Do you want me to do a re-roll?

Sorry to bump, but do you want a reroll on this?

---
Regards,
James Denholm.