{"thread":{"id":"33037","subject":"[BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present","startedAt":"2013-02-27T23:51:13Z","lastAt":"2013-02-28T21:37:00Z","messageCount":3,"participants":["Paul Campbell","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"210415","messageId":"CALeLG_ndDWyPWRBQtKV4WswZUKRpNQzCAXmB0f0yoKGsAYMUig@mail.gmail.com","threadId":"33037","inReplyTo":null,"subject":"[BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present","fromName":"Paul Campbell","fromEmail":"pcampbell@kemitix.net","sentAt":"2013-02-27T23:51:13Z","receivedAt":"2013-02-27T23:51:13Z","isPatch":true,"sender":{"key":"pcampbell@kemitix.net","avatar":"https://gravatar.com/avatar/57584e05501b694929004e43fcd7308f4ad64df2eb0474cd8c7e5f93662bb0f1?d=mp&s=160"},"body":"cmd_add() attempts to check for the validity of refspec for the repository\nit is about to add as a subtree. It tries to do so before contacting the\nrepository. If the refspec happens to exist locally (say 'master') then\nthe test passes and the repo is fetched. If the refspec doesn't exist\nlocally then the test fails and the remote repo is never contacted.\n\nRemoving the tests still works as the git fetch command fails with the\nperfectly accurate error:\n\n  fatal: Couldn't find remote ref <refspec>\n\nSigned-off-by: Paul Campbell <pcampbell@carnegiecollege.ac.uk>\n---\n\nI must confess to not understanding the comment preceding the\nrev-parse test. Given that it is against the rev-parse and not the\ncmd_add_repository call I'm assuming it can be removed.\n\n contrib/subtree/git-subtree.sh | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 8a23f58..9a38b18 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -503,14 +503,6 @@ cmd_add()\n\n            \"cmd_add_commit\" \"$@\"\n        elif [ $# -eq 2 ]; then\n-           # Technically we could accept a refspec here but we're\n-           # just going to turn around and add FETCH_HEAD under the\n-           # specified directory.  Allowing a refspec might be\n-           # misleading because we won't do anything with any other\n-           # branches fetched via the refspec.\n-           git rev-parse -q --verify \"$2^{commit}\" >/dev/null ||\n-           die \"'$2' does not refer to a commit\"\n-\n            \"cmd_add_repository\" \"$@\"\n        else\n            say \"error: parameters were '$@'\"\n--\n1.8.2.rc1\n\n\n-- \nPaul [W] Campbell\n"},{"id":"210417","messageId":"7vwqtti91k.fsf@alter.siamese.dyndns.org","threadId":"33037","inReplyTo":"CALeLG_ndDWyPWRBQtKV4WswZUKRpNQzCAXmB0f0yoKGsAYMUig@mail.gmail.com","subject":"Re: [BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-28T00:20:07Z","receivedAt":"2013-02-28T00:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Campbell <pcampbell@kemitix.net> writes:\n\n> cmd_add() attempts to check for the validity of refspec for the repository\n> it is about to add as a subtree. It tries to do so before contacting the\n> repository. If the refspec happens to exist locally (say 'master') then\n> the test passes and the repo is fetched. If the refspec doesn't exist\n> locally then the test fails and the remote repo is never contacted.\n>\n> Removing the tests still works as the git fetch command fails with the\n> perfectly accurate error:\n>\n>   fatal: Couldn't find remote ref <refspec>\n>\n> Signed-off-by: Paul Campbell <pcampbell@carnegiecollege.ac.uk>\n> ---\n>\n> I must confess to not understanding the comment preceding the\n> rev-parse test. Given that it is against the rev-parse and not the\n> cmd_add_repository call I'm assuming it can be removed.\n\nThis is contrib/ material and I do not use the command, so anything\nI say should be taken with a moderate amount of salt, but I think\nthe comment is talking about _not_ accepting a full storing refspec,\nor worse yet wildcard, e.g.\n\n\trefs/heads/topic:refs/remotes/origin/topic\n\trefs/heads/*:refs/remotes/origin/*\n\nwhich will not make sense given that you are only adding a single\ncommit via \"cmd_add\".  Also, if you have already fetched from the\nremote, \"rev-parse $2^{commit}\" at this point will protect against\na typo by the end user.\n\nAs you noticed, checking if $2 is a commit using rev-parse _before_\nfetching $2 from the remote repository is not a valid way to check\nagainst such errors.  So I tend to agree that removing the \"die\"\nwill be a good idea.\n\nTyping \"tipoc\" when the user meant \"topic\" will error out at the\n\"git fetch\" done in cmd_add_repository, but that fetch will happily\nfetch and store whatever a refspec specifies, so you might want to\nreplace the bogus \"rev-parse before fetching\" check to \"reject\nrefspec\" with something else.\n\n>  contrib/subtree/git-subtree.sh | 8 --------\n>  1 file changed, 8 deletions(-)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 8a23f58..9a38b18 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -503,14 +503,6 @@ cmd_add()\n>\n>             \"cmd_add_commit\" \"$@\"\n>         elif [ $# -eq 2 ]; then\n> -           # Technically we could accept a refspec here but we're\n> -           # just going to turn around and add FETCH_HEAD under the\n> -           # specified directory.  Allowing a refspec might be\n> -           # misleading because we won't do anything with any other\n> -           # branches fetched via the refspec.\n> -           git rev-parse -q --verify \"$2^{commit}\" >/dev/null ||\n> -           die \"'$2' does not refer to a commit\"\n> -\n>             \"cmd_add_repository\" \"$@\"\n>         else\n>             say \"error: parameters were '$@'\"\n> --\n> 1.8.2.rc1\n"},{"id":"210439","messageId":"CALeLG_mryWOE_KrFDAf6Z7=w6kdaXii4rHzPYBfRbsjt4TZTAw@mail.gmail.com","threadId":"33037","inReplyTo":"7vwqtti91k.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present","fromName":"Paul Campbell","fromEmail":"pcampbell@kemitix.net","sentAt":"2013-02-28T21:37:00Z","receivedAt":"2013-02-28T21:37:00Z","isPatch":true,"sender":{"key":"pcampbell@kemitix.net","avatar":"https://gravatar.com/avatar/57584e05501b694929004e43fcd7308f4ad64df2eb0474cd8c7e5f93662bb0f1?d=mp&s=160"},"body":"On Thu, Feb 28, 2013 at 12:20 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Paul Campbell <pcampbell@kemitix.net> writes:\n>\n>> cmd_add() attempts to check for the validity of refspec for the repository\n>> it is about to add as a subtree. It tries to do so before contacting the\n>> repository. If the refspec happens to exist locally (say 'master') then\n>> the test passes and the repo is fetched. If the refspec doesn't exist\n>> locally then the test fails and the remote repo is never contacted.\n>>\n>> Removing the tests still works as the git fetch command fails with the\n>> perfectly accurate error:\n>>\n>>   fatal: Couldn't find remote ref <refspec>\n>>\n>> Signed-off-by: Paul Campbell <pcampbell@carnegiecollege.ac.uk>\n>> ---\n>>\n>> I must confess to not understanding the comment preceding the\n>> rev-parse test. Given that it is against the rev-parse and not the\n>> cmd_add_repository call I'm assuming it can be removed.\n>\n> This is contrib/ material and I do not use the command, so anything\n> I say should be taken with a moderate amount of salt, but I think\n> the comment is talking about _not_ accepting a full storing refspec,\n> or worse yet wildcard, e.g.\n>\n>         refs/heads/topic:refs/remotes/origin/topic\n>         refs/heads/*:refs/remotes/origin/*\n>\n> which will not make sense given that you are only adding a single\n> commit via \"cmd_add\".  Also, if you have already fetched from the\n> remote, \"rev-parse $2^{commit}\" at this point will protect against\n> a typo by the end user.\n>\n> As you noticed, checking if $2 is a commit using rev-parse _before_\n> fetching $2 from the remote repository is not a valid way to check\n> against such errors.  So I tend to agree that removing the \"die\"\n> will be a good idea.\n>\n> Typing \"tipoc\" when the user meant \"topic\" will error out at the\n> \"git fetch\" done in cmd_add_repository, but that fetch will happily\n> fetch and store whatever a refspec specifies, so you might want to\n> replace the bogus \"rev-parse before fetching\" check to \"reject\n> refspec\" with something else.\n\nThanks for the feedback.\n\nChecking for a ':' or a '*' in the refspec should stop the storage\nname and wildcards getting through.\n\nRerolling the patch with new test.\n\n-- \nPaul [W] Campbell\n"}]}