{"thread":{"id":"36644","subject":"[PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag","startedAt":"2014-05-13T04:08:58Z","lastAt":"2014-05-14T22:50:01Z","messageCount":7,"participants":["James Denholm","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"241330","messageId":"1399954138-2807-1-git-send-email-nod.helm@gmail.com","threadId":"36644","inReplyTo":null,"subject":"[PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"James Denholm","fromEmail":"nod.helm@gmail.com","sentAt":"2014-05-13T04:08:58Z","receivedAt":"2014-05-13T04:08:58Z","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, pre-existing\ndependency of git-subtree.\n\nReported-by: Kevin Cagle \nHelped-by: Junio C Hamano \nSigned-off-by: James Denholm \n---\nI felt that defining revp would be a little more self-documenting than\nusing $rev^0.\n\n contrib/subtree/git-subtree.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex dc59a91..eefd720 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -557,8 +557,9 @@ cmd_add_commit()\n \t\tcommit=$(add_squashed_msg \"$rev\" \"$dir\" |\n \t\t\t git commit-tree $tree $headp -p \"$rev\") || exit $?\n \telse\n+\t\trevp=$(peel_committish \"$rev\")\n \t\tcommit=$(add_msg \"$dir\" \"$headrev\" \"$rev\" |\n-\t\t\t git commit-tree $tree $headp -p \"$rev\") || exit $?\n+\t\t\t git commit-tree $tree $headp -p \"$revp\") || exit $?\n \tfi\n \tgit reset \"$commit\" || exit $?\n \t\n-- \n1.9.2\n"},{"id":"241394","messageId":"xmqqa9alo4lm.fsf@gitster.dls.corp.google.com","threadId":"36644","inReplyTo":"1399954138-2807-1-git-send-email-nod.helm@gmail.com","subject":"Re: [PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-13T19:34:13Z","receivedAt":"2014-05-13T19:34:13Z","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>\n> Because peeling a committish ID doesn't do anything if it's already a\n> commit, fix by peeling[1] the object ID before assigning it to $rev, as\n> per the patch.\n>\n> [*1*]: Via peel_committish(), from git:git-sh-setup.sh, pre-existing\n> dependency of git-subtree.\n>\n> Reported-by: Kevin Cagle \n> Helped-by: Junio C Hamano \n> Signed-off-by: James Denholm \n> ---\n> I felt that defining revp would be a little more self-documenting than\n> using $rev^0.\n\nThat is a good decision, but as long as we are attempting to peel,\ndon't we want to stop the damage when it does not peel to a commit?\n\nI'll tentatively queue this. Thanks.\n\n-- >8 --\nFrom: James Denholm \nDate: Tue, 13 May 2014 14:08:58 +1000\nSubject: [PATCH] contrib/subtree: allow adding an annotated tag\n\ncmd_add_commit() is passed FETCH_HEAD by cmd_add_repository, which\nis then rev-parsed into an object name. However, if the user is\nfetching a 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\nthe object name refers to a tag and is never peeled, and the git\ncommit-tree call (line 561) slaps us in the face because it doesn't\npeel tags to commits.\n\nBecause peeling a committish doesn't do anything if it's already a\ncommit, fix by peeling the object name before assigning it to $rev,\nusing peel_committish() from git:git-sh-setup.sh, a pre-existing\ndependency of git-subtree.\n\nReported-by: Kevin Cagle \nHelped-by: Junio C Hamano \nSigned-off-by: James Denholm \nSigned-off-by: Junio C Hamano \n---\n contrib/subtree/git-subtree.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex db925ca..fa1a583 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -558,8 +558,9 @@ cmd_add_commit()\n \t\tcommit=$(add_squashed_msg \"$rev\" \"$dir\" |\n \t\t\t git commit-tree $tree $headp -p \"$rev\") || exit $?\n \telse\n+\t\trevp=$(peel_committish \"$rev\") &&\n \t\tcommit=$(add_msg \"$dir\" \"$headrev\" \"$rev\" |\n-\t\t\t git commit-tree $tree $headp -p \"$rev\") || exit $?\n+\t\t\t git commit-tree $tree $headp -p \"$revp\") || exit $?\n \tfi\n \tgit reset \"$commit\" || exit $?\n \t\n-- \n2.0.0-rc3-404-gb0be553\n"},{"id":"241468","messageId":"20140513230201.GA32562@debian","threadId":"36644","inReplyTo":"xmqqa9alo4lm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"James Denholm","fromEmail":"nod.helm@gmail.com","sentAt":"2014-05-13T23:02:01Z","receivedAt":"2014-05-13T23:02:01Z","isPatch":true,"sender":{"key":"nod.helm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1721189?v=4"},"body":"On Tue, May 13, 2014 at 12:34:13PM -0700, Junio C Hamano wrote:\n> James Denholm writes:\n> > I felt that defining revp would be a little more self-documenting than\n> > using $rev^0.\n> \n> That is a good decision, but as long as we are attempting to peel,\n> don't we want to stop the damage when it does not peel to a commit?\n\nI'm not sure that can actually happen - peel_committish is essentially\nimplemented as `rev-parse $arg^0` (though with a bit of bling, of\ncourse), and to my understanding FETCH_HEAD will always parse to a\ncommittish - I could have missed something, of course.\n\nsubtree Will need error-catching at some point, of course, triggering\nresets or at least suggesting instructions to the user, but I think\nthat's a touch out of the scope of a bugfix at this point (and, to be\nhonest, I personally can't allocate the time to that for about a month\ndue to the dark shadow of academic exams). Indeed, what to do in those\ncases is probably worth (re-)discussing the overall design and aims of\nsubtree for, and so I'm not confident that I currently know the best way\nto do that.\n\n> I'll tentatively queue this. Thanks.\n\nAwesome, thanks again for this and the feedback.\n\n---\nRegards,\nJames Denholm.\n"},{"id":"241473","messageId":"xmqqy4y5l1c7.fsf@gitster.dls.corp.google.com","threadId":"36644","inReplyTo":"20140513230201.GA32562@debian","subject":"Re: [PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-13T23:12:56Z","receivedAt":"2014-05-13T23:12:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Denholm writes:\n\n> I'm not sure that can actually happen - peel_committish is essentially\n> implemented as `rev-parse $arg^0` (though with a bit of bling, of\n> course), and to my understanding FETCH_HEAD will always parse to a\n> committish - I could have missed something, of course.\n\n $ git fetch git://repo.or.cz/alt-git junio-gpg-pub\n $ git rev-parse FETCH_HEAD^0\n"},{"id":"241583","messageId":"20140514213206.GA12228@debian","threadId":"36644","inReplyTo":"xmqqy4y5l1c7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"James Denholm","fromEmail":"nod.helm@gmail.com","sentAt":"2014-05-14T21:32:06Z","receivedAt":"2014-05-14T21:32:06Z","isPatch":true,"sender":{"key":"nod.helm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1721189?v=4"},"body":"On Tue, May 13, 2014 at 04:12:56PM -0700, Junio C Hamano wrote:\n> James Denholm writes:\n> \n> > I'm not sure that can actually happen - peel_committish is essentially\n> > implemented as `rev-parse $arg^0` (though with a bit of bling, of\n> > course), and to my understanding FETCH_HEAD will always parse to a\n> > committish - I could have missed something, of course.\n> \n> $ git fetch git://repo.or.cz/alt-git junio-gpg-pub\n> $ git rev-parse FETCH_HEAD^0\n\nThat would be a problem... Sadly I doubt I'll have time to develop a\nsolution into subtree's overall design before the end of June. As that\neventual change would probably involve altering the inclusions of this\nfix, and that users have a workaround in adding either squashed commits\nor referencing lightweight tags, would you rather drop the patch and\nwait for that?\n\n---\nRegards,\nJames Denholm.\n"},{"id":"241585","messageId":"xmqqtx8shwel.fsf@gitster.dls.corp.google.com","threadId":"36644","inReplyTo":"20140514213206.GA12228@debian","subject":"Re: [PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-14T21:40:02Z","receivedAt":"2014-05-14T21:40:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Denholm writes:\n\n> On Tue, May 13, 2014 at 04:12:56PM -0700, Junio C Hamano wrote:\n>> James Denholm writes:\n>> \n>> > I'm not sure that can actually happen - peel_committish is essentially\n>> > implemented as `rev-parse $arg^0` (though with a bit of bling, of\n>> > course), and to my understanding FETCH_HEAD will always parse to a\n>> > committish - I could have missed something, of course.\n>> \n>> $ git fetch git://repo.or.cz/alt-git junio-gpg-pub\n>> $ git rev-parse FETCH_HEAD^0\n>\n> That would be a problem... Sadly I doubt I'll have time to develop a\n> solution into subtree's overall design before the end of June. As that\n> eventual change would probably involve altering the inclusions of this\n> fix, and that users have a workaround in adding either squashed commits\n> or referencing lightweight tags, would you rather drop the patch and\n> wait for that?\n\nSorry, I am lost. What would be a problem exactly?\n\nA FETCH_HEAD can be pointing at an object that is not committish,\nand users involved, both at the originating end who controls the\nrepository you fetched from and at the receiving end who wanted to\nfetch the object, are *not* expeting to be able to make a merge of\nsuch an object anyway. My suggestion was not to ask you to come up\nwith a sane behaviour when the user told us to add a single blob\nwith \"subtree add\"; it was merely to detect such unintended use as\nan error.\n\nTo me, it looks like all that is necessary is to accept your patch\nbut with a three-byte tightening to detect such a pathological case\nand signal an error, which is what \" &&\", which I added to your new\nline that sets revp=$(peel_committish ...), is about.\n\nThis patch, with or without these extra \" &&\" three bytes, will not\nbe part of the upcoming 2.0 release anyway, so we have enough time\nto iron it out.\n"},{"id":"241630","messageId":"20140514225001.GA5850@debian","threadId":"36644","inReplyTo":"xmqqtx8shwel.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag","fromName":"James Denholm","fromEmail":"nod.helm@gmail.com","sentAt":"2014-05-14T22:50:01Z","receivedAt":"2014-05-14T22:50:01Z","isPatch":true,"sender":{"key":"nod.helm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1721189?v=4"},"body":"Junio C Hamano wrote:\n> To me, it looks like all that is necessary is to accept your patch\n> but with a three-byte tightening to detect such a pathological case\n> and signal an error, which is what \" &&\", which I added to your new\n> line that sets revp=$(peel_committish ...), is about.\n> \n> This patch, with or without these extra \" &&\" three bytes, will not\n> be part of the upcoming 2.0 release anyway, so we have enough time\n> to iron it out.\n\nAh, right, sorry, somehow I missed the \" &&\" in the amended patch. I\ndon't know how I overlooked that. That indeed would be plenty.\n\n> Sorry, I am lost. What would be a problem exactly?\n> \n> A FETCH_HEAD can be pointing at an object that is not committish,\n> and users involved, both at the originating end who controls the\n> repository you fetched from and at the receiving end who wanted to\n> fetch the object, are *not* expeting to be able to make a merge of\n> such an object anyway. My suggestion was not to ask you to come up\n> with a sane behaviour when the user told us to add a single blob\n> with \"subtree add\"; it was merely to detect such unintended use as\n> an error.\n\nYeah, what I meant by problem was the possibility of finding a way to\npre-empt the case of a tag pointing to a blob and handle it as\ngracefully as possible, and try to leave the user with their working\ntree in the state from before the subtree call.\n\nThe reason I'd suggest it might be worth handling at some point is that\n(in the far flung future) it may not be out of the scope of subtree to\npull not only other repos to a local subtree, but also specific trees\n(or perhaps blobs) to a local subtree. Conceptually, I'd argue that a\nsensible future functionality in order to allow subtree users to deal\nwith upstreams that don't split their projects up sanely (cough splutter\nFacebook's internal). Hence, working out a way to determine tag types,\npossibly before doing the fetch somehow, would be a boon to that\nmethinks.\n\nOf course, this is something I haven't yet thought enough about and the\nidea is likely full of holes, but hey, I'm nothing if not impractically\nidealist.\n\n---\nRegards,\nJames Denholm.\n"}]}