Re: [PATCH] contrib/subtree bugfix: Can't `add` annotated tag
On Fri, May 09, 2014 at 05:36:15PM +1000, James Denholm wrote: Junio C Hamano gits...@pobox.com wrote: 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/subtree bugfix: Can't `add` annotated tag
Junio C Hamano gits...@pobox.com 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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/subtree bugfix: Can't `add` annotated tag
James Denholm nod.h...@gmail.com 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?
contrib/subtree/git-subtree.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index dc59a91..9453dae 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -538,7 +538,7 @@ cmd_add_commit()
{
revs=$(git rev-parse $default --revs-only $@) || exit $?
set -- $revs
- rev=$1
+ rev=$(peel_committish $1)
debug Adding $dir as '$rev'...
git read-tree --prefix=$dir $rev || exit $?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] contrib/subtree bugfix: Can't `add` annotated tag
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.
Because peeling a committish ID doesn't do anything if it's already a
commit, fix by peeling[1] the object ID before assigning it to $rev, as
per the patch.
[*1*]: Via peel_committish(), from git:git-sh-setup.sh
Reported-by: Kevin Cagle kca...@micron.com
Diagnosed-by: Junio C Hamano gits...@pobox.com
Signed-off-by: James Denholm nod.h...@gmail.com
---
NB: This bug doesn't surface when using --squash, as $rev is reassigned
to the squash commit via new_squash_commit before git commit-tree sees
it (though for simplicity, new_squash_commit now also sees the peeled
ID).
Also doesn't surface when using git subtree merge, as git merge can
handle tag objects.
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.
contrib/subtree/git-subtree.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index dc59a91..9453dae 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -538,7 +538,7 @@ cmd_add_commit()
{
revs=$(git rev-parse $default --revs-only $@) || exit $?
set -- $revs
- rev=$1
+ rev=$(peel_committish $1)
debug Adding $dir as '$rev'...
git read-tree --prefix=$dir $rev || exit $?
--
1.9.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
