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

Paul Campbell [off-list ref] writes:

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 <redacted>
---

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.

On Thu, Feb 28, 2013 at 12:20 AM, Junio C Hamano [off-list ref] wrote:

Paul Campbell [off-list ref] writes:

quoted

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 <redacted>
---

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.

Thanks for the feedback.

Checking for a ':' or a '*' in the refspec should stop the storage
name and wildcards getting through.

Rerolling the patch with new test.

-- 
Paul [W] Campbell