On 05/05/2012 08:03 AM, Michael Haggerty wrote:
On 05/05/2012 12:30 AM, Junio C Hamano wrote:There are a handful of places where we call check_refname_format() on a substring after "refs/" of a refname we are going to use, and filter out a valid match with "refs/stash" with such a pathspec. Not sending a stash may arguably be a feature (as stash is inherently a local workflow element), but the code in the transport layer is oblivious to this filtering performed by the lower layer of the code, and complains that the other side did not send all the objects that needs to complete refs/stash at the end, even though the code will not write refs/stash out anyway, and making the whole command fail. This is an attempt to "fix" it by using check_refname_format() on the whole "refs/....." string and allowing refs/stash to be also copied. Signed-off-by: Junio C Hamano<gitster@xxxxxxxxx> --- * With this patch: $ git checkout HEAD^0 ;# make sure we are on detached HEAD $ git fetch $somewhere +refs/*:refs/* and victim$ git config receive.denyCurrentBranch warn master$ git push victim +refs/*:refs/* should work. builtin/fetch-pack.c | 2 +- builtin/receive-pack.c | 2 +- remote.c | 2 +- t/t5516-fetch-push.sh | 30 ++++++++++++++++++++++++++++++ 4 files changed, 33 insertions(+), 3 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 6207ecd..a3e3fa3 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match) for (ref = *refs; ref; ref = next) { next = ref->next; if (!memcmp(ref->name, "refs/", 5)&& - check_refname_format(ref->name + 5, 0)) + check_refname_format(ref->name, 0))The patch looks fine to me. This combination "!memcmp(ref->name, "refs/", 5) && check_refname_format(ref->name, 0)" is the reason that I suggested adding a REFNAME_FULL option [1], in which case it could be written "check_refname_format(ref->name, REFNAME_FULL)". However, now I think that the options should be constructed a little differently: flags==0: Require refname to start with "refs/" flags==REFNAME_ALLOW_SPECIAL: Also accept single-level ALL_CAPS refnames. flags==REFNAME_ALLOW_PARTIAL: Don't check the namespace or require '/'). This could be used for checking partial names like "master" as shorthand for "refs/master". Does this sound reasonable to you?
Oops; I just realized that this particular example doesn't check "starts with "refs/" and valid refname" as expected. In fact it checks "starts with "refs/" and INVALID refname". The proposed REFNAME_FULL option would of course be equivalent to the former. (Why the latter is needed here is something that needs a larger investment of time to figure out.)
The other examples that you patched do the expected combination of checks. Michael -- Michael Haggerty mhagger@xxxxxxxxxxxx http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html