{"thread":{"id":"36604","subject":"[PATCH] contrib/subtree bugfix: Can't `add` annotated tag","startedAt":"2014-05-08T01:04:39Z","lastAt":"2014-05-12T07:29:47Z","messageCount":4,"participants":["James Denholm","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"240982","messageId":"1399511079-1994-1-git-send-email-nod.helm@gmail.com","threadId":"36604","inReplyTo":null,"subject":"[PATCH] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"James Denholm","fromEmail":"nod.helm@gmail.com","sentAt":"2014-05-08T01:04:39Z","receivedAt":"2014-05-08T01:04:39Z","isPatch":true,"sender":{"key":"nod.helm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1721189?v=4"},"body":"cmd_add_commit() is passed FETCH_HEAD by cmd_add_repository, which is\nthen rev-parsed into an object ID. However, if the user is fetching a\ntag rather than a branch HEAD, such as by executing:\n\n$ git subtree add -P oldGit https://github.com/git/git.git tags/v1.8.0\n\nThe object ID is a tag and is never peeled, and the git commit-tree call\n(line 561) slaps us in the face because it doesn't handle tag IDs.\n\nBecause peeling a committish ID doesn't do anything if it's already a\ncommit, fix by peeling[1] the object ID before assigning it to $rev, as\nper the patch.\n\n[*1*]: Via peel_committish(), from git:git-sh-setup.sh\n\nReported-by: Kevin Cagle \nDiagnosed-by: Junio C Hamano \nSigned-off-by: James Denholm \n---\nNB: This bug doesn't surface when using --squash, as $rev is reassigned\nto the squash commit via new_squash_commit before git commit-tree sees\nit (though for simplicity, new_squash_commit now also sees the peeled\nID).\n\nAlso doesn't surface when using \"git subtree merge\", as git merge can\nhandle tag objects.\n\nOn a side note, if merging a tag without --squash, git merge recognises\nthat it's a tag and adds a note to the merge commit body. It may be\nworth mimicking this when using \"subtree merge --squash\" or\n\"subtree add\".\n\n contrib/subtree/git-subtree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex dc59a91..9453dae 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -538,7 +538,7 @@ cmd_add_commit()\n {\n \trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n \tset -- $revs\n-\trev=\"$1\"\n+\trev=$(peel_committish \"$1\")\n \t\n \tdebug \"Adding $dir as '$rev'...\"\n \tgit read-tree --prefix=\"$dir\" $rev || exit $?\n-- \n1.9.2\n"},{"id":"241015","messageId":"xmqqoaz841d3.fsf@gitster.dls.corp.google.com","threadId":"36604","inReplyTo":"1399511079-1994-1-git-send-email-nod.helm@gmail.com","subject":"Re: [PATCH] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-08T17:38:32Z","receivedAt":"2014-05-08T17:38:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Denholm writes:\n\n> cmd_add_commit() is passed FETCH_HEAD by cmd_add_repository, which is\n> then rev-parsed into an object ID. However, if the user is fetching a\n> tag rather than a branch HEAD, such as by executing:\n>\n> $ git subtree add -P oldGit https://github.com/git/git.git tags/v1.8.0\n>\n> The object ID is a tag and is never peeled, and the git commit-tree call\n> (line 561) slaps us in the face because it doesn't handle tag IDs.\n\nThe \"rev\" (not \"revs\") seems to be used by more things than the\nfinal commit-tree state. Are we losing some useful information by\npeeling it too early like this patch does? The reason why we\nstopped peeling when writing FETCH_HEAD was because we wanted to\nrecord the fact that we merged a tag (and use the GPG signature if\nfound in it) when constructing the log message for the merge, and\npeeling the tag too early and recording the commit in FETCH_HEAD\nwould make it impossible to do, and I am wondering if this change is\nmaking the same kind of mistake here.\n\nI see that add_msg does not use anything useful from latest_new, so\nwith the current state of the code, it does not make that much\ndifference (except that it says \"from commit '$latest_new'\", and by\npeeling, the fact that the user wanted to use a tag is lost from the\nresult).\n\n> On a side note, if merging a tag without --squash, git merge recognises\n> that it's a tag and adds a note to the merge commit body. It may be\n> worth mimicking this when using \"subtree merge --squash\" or\n> \"subtree add\".\n\nYes, and this change makes such a change harder to implement on top,\nI suspect.\n\nWould it be sufficient to do\n\n\tgit commit-tree $tree $headp -p \"$rev^0\"\n\nin that \"not squashing\" codepath instead?\n\n> contrib/subtree/git-subtree.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index dc59a91..9453dae 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -538,7 +538,7 @@ cmd_add_commit()\n> {\n> \trevs=$(git rev-parse $default --revs-only \"$@\") || exit $?\n> \tset -- $revs\n> -\trev=\"$1\"\n> +\trev=$(peel_committish \"$1\")\n> \t\n> \tdebug \"Adding $dir as '$rev'...\"\n> \tgit read-tree --prefix=\"$dir\" $rev || exit $?\n"},{"id":"241112","messageId":"CAHYYfeFfo=xVDezAGFyCvuhx=bkzMF6KyDCAjZNKNoGnytXDWA@mail.gmail.com","threadId":"36604","inReplyTo":"xmqqoaz841d3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"James Denholm","fromEmail":"nod.helm@gmail.com","sentAt":"2014-05-09T07:36:15Z","receivedAt":"2014-05-09T07:36:15Z","isPatch":true,"sender":{"key":"nod.helm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1721189?v=4"},"body":"Junio C Hamano wrote:\n> The \"rev\" (not \"revs\") seems to be used by more things than the\n> final commit-tree state. Are we losing some useful information by\n> peeling it too early like this patch does? (...)\n\nYou're not wrong, actually, peeling at the last minute (or at least\nlater) would be a better choice. I'd suggest that we aren't losing\ncurrently-useful information (as it'd be rare-if-ever that a user would\nlook at a hash in their commit logs and think \"Oh, that's that tag!\"),\nbut certainly with future development in mind it's more ideal.\n\n> I see that add_msg does not use anything useful from latest_new, so\n> with the current state of the code, it does not make that much\n> difference (except that it says \"from commit '$latest_new'\", and by\n> peeling, the fact that the user wanted to use a tag is lost from the\n> result).\n\nYeah, that might be a worthy thing to porcelain-up in the future with\nlogging the tag name rather than, or in addition to, the hash, as well\nas a similar change in add_squashed_msg.\n\n> Would it be sufficient to do\n>\n> git commit-tree $tree $headp -p \"$rev^0\"\n>\n> in that \"not squashing\" codepath instead?\n\nOn line 561, sure. Do you want me to do a re-roll?\n"},{"id":"241276","messageId":"20140512072947.GA18519@debian","threadId":"36604","inReplyTo":"CAHYYfeFfo=xVDezAGFyCvuhx=bkzMF6KyDCAjZNKNoGnytXDWA@mail.gmail.com","subject":"Re: [PATCH] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"James Denholm","fromEmail":"nod.helm@gmail.com","sentAt":"2014-05-12T07:29:47Z","receivedAt":"2014-05-12T07:29:47Z","isPatch":true,"sender":{"key":"nod.helm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1721189?v=4"},"body":"On Fri, May 09, 2014 at 05:36:15PM +1000, James Denholm wrote:\n> Junio C Hamano wrote:\n> > Would it be sufficient to do\n> >\n> > git commit-tree $tree $headp -p \"$rev^0\"\n> >\n> > in that \"not squashing\" codepath instead?\n> \n> On line 561, sure. Do you want me to do a re-roll?\n\nSorry to bump, but do you want a reroll on this?\n\n---\nRegards,\nJames Denholm.\n"}]}