Re: [BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 28, 2013, 00:20 UTC
Message-ID: <7vwqtti91k.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALeLG_ndDWyPWRBQtKV4WswZUKRpNQzCAXmB0f0yoKGsAYMUig@mail.gmail.com>

Paul Campbell <pcampbell@kemitix.net> writes:

Show 17 quoted lines

> cmd_add() attempts to check for the validity of refspec for the repository
> it is about to add as a subtree. It tries to do so before contacting the
> repository. If the refspec happens to exist locally (say 'master') then
> the test passes and the repo is fetched. If the refspec doesn't exist
> locally then the test fails and the remote repo is never contacted.
>
> Removing the tests still works as the git fetch command fails with the
> perfectly accurate error:
>
>   fatal: Couldn't find remote ref <refspec>
>
> Signed-off-by: Paul Campbell <pcampbell@carnegiecollege.ac.uk>
> ---
>
> I must confess to not understanding the comment preceding the
> rev-parse test. Given that it is against the rev-parse and not the
> cmd_add_repository call I'm assuming it can be removed.

This is contrib/ material and I do not use the command, so anything I say should be taken with a moderate amount of salt, but I think the comment is talking about _not_ accepting a full storing refspec, or worse yet wildcard, e.g.

	refs/heads/topic:refs/remotes/origin/topic
	refs/heads/*:refs/remotes/origin/*

which will not make sense given that you are only adding a single commit via "cmd_add". Also, if you have already fetched from the remote, "rev-parse $2^{commit}" at this point will protect against a typo by the end user.

As you noticed, checking if $2 is a commit using rev-parse _before_ fetching $2 from the remote repository is not a valid way to check against such errors. So I tend to agree that removing the "die" will be a good idea.

Typing "tipoc" when the user meant "topic" will error out at the "git fetch" done in cmd_add_repository, but that fetch will happily fetch and store whatever a refspec specifies, so you might want to replace the bogus "rev-parse before fetching" check to "reject refspec" with something else.

Show 24 quoted lines

>  contrib/subtree/git-subtree.sh | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 8a23f58..9a38b18 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -503,14 +503,6 @@ cmd_add()
>
>             "cmd_add_commit" "$@"
>         elif [ $# -eq 2 ]; then
> -           # Technically we could accept a refspec here but we're
> -           # just going to turn around and add FETCH_HEAD under the
> -           # specified directory.  Allowing a refspec might be
> -           # misleading because we won't do anything with any other
> -           # branches fetched via the refspec.
> -           git rev-parse -q --verify "$2^{commit}" >/dev/null ||
> -           die "'$2' does not refer to a commit"
> -
>             "cmd_add_repository" "$@"
>         else
>             say "error: parameters were '$@'"
> --
> 1.8.2.rc1