Re: [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote

[Junio C Hamano <gitster@pobox.com>]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So this relaxes the remote matching, and allows using the "local:remote" 
> syntax to say that the local branch is differently named from the remote 
> one.
>
> It is probably worth folding it into the previous patch if you think this 
> whole approach is workable.

Haven't thought enough to decide on that part, yet.

>  # $3 must be a symbolic ref, a unique ref, or
> -# a SHA object expression
> +# a SHA object expression. It can also be of
> +# the format 'local-name:remote-name'.
>  #
> -head=$(git symbolic-ref -q "${3-HEAD}")
> -head=${head:-$(git show-ref "${3-HEAD}" | cut -d' ' -f2)}
> -head=${head:-$(git rev-parse --quiet --verify "$3")}
> +local=${3%:*}
> +local=${local:-HEAD}
> +remote=${3#*:}
> +head=$(git symbolic-ref -q "$local")
> +head=${head:-$(git show-ref --heads --tags "$local" | cut -d' ' -f2)}
> +head=${head:-$(git rev-parse --quiet --verify "$local")}
>  
>  # None of the above? Bad.
> -test -z "$head" && die "fatal: Not a valid revision: $3"
> +test -z "$head" && die "fatal: Not a valid revision: $local"
>  
>  # This also verifies that the resulting head is unique:

I am not sure if it is a good idea to hand-craft "resulting head is
unique" constraint here.  We already have disambiguation rules (and
warning mechanism) we use in other places---this part should use the
same rule, I think.

A short experiment tells me that we are almost there:

  $ git init && git commit --allow-empty -m initial
  $ git branch other && git tag master
  $ git -c core.warnambiguousrefs=true \
      rev-parse --symbolic-full-name other
  refs/heads/other
  $ git -c core.warnambiguousrefs=true \
      rev-parse --symbolic-full-name master; echo $?
  warning: refname 'master' is ambiguous.
  error: refname 'master' is ambiguous
  0

We say "error" but we do not actually error out, but that shouldn't
be too hard to fix.

>  # "git show-ref" could have shown multiple matching refs..
>  headrev=$(git rev-parse --verify --quiet "$head"^0)
> -test -z "$headrev" && die "fatal: Ambiguous revision: $3"
> +test -z "$headrev" && die "fatal: Ambiguous revision: $local"
>  
>  # Was it a branch with a description?
>  branch_name=${head#refs/heads/}
> @@ -69,9 +73,6 @@ then
>  	branch_name=
>  fi
>  
> -prettyhead=${head#refs/}
> -prettyhead=${prettyhead#heads/}
> -
>  merge_base=$(git merge-base $baserev $headrev) ||
>  die "fatal: No commits in common between $base and $head"
>  
> @@ -81,30 +82,37 @@ die "fatal: No commits in common between $base and $head"
>  #
>  # Otherwise find a random ref that matches $headrev.
>  find_matching_ref='
> +	my ($head,$headrev) = (@ARGV);
> +	my ($found);
> +
>  	while (<STDIN>) {
> +		chomp;
>  		my ($sha1, $ref, $deref) = /^(\S+)\s+([^^]+)(\S*)$/;
> +		my ($pattern);
> +		next unless ($sha1 eq $headrev);
> +
> +		$pattern="/$head\$";

I think $head is constant inside the loop, so lift it outside?

> +		if ($ref eq $head) {
> +			$found = $ref;
> +		}
> +		if ($ref =~ /$pattern/) {
> +			$found = $ref;
>  		}
> +		if ($sha1 eq $head) {

I think this is $headrev ($head may be $remote or HEAD), but then
anything that does not point at $headrev has already been rejected
at the beginning of this loop, so...?

>  			$found = $sha1;
>  		}
>  	}
> +	if ($found) {
>  		print "$found\n";
>  	}
>  '

I somehow feel that this is inadequate to catch the "delayed
propagation" error in the opposite direction.  The publish
repository may have an unrelated ref pointing at the $headrev and we
may guess that is the ref to be fetched by the integrator based on
that, but by the time the integrator fetches from the repository,
the ref may have been updated to its new value that does not match
$headrev.  But I do not think of a way to solve that one.

In any case, shouldn't we be catching duplicate matches here, if the
real objective is to make it less likely for the users to make
mistakes?  Otherwise, if there are two 'master' over there
(e.g. refs/heads/master and refs/remotes/origin/master), it seems to
me that $ref =~ /$pattern/ will trigger twice in your loop and ends
up reporting the last match.

In other words,

	my (@found) = ();
        my (@guessed) = ();
	while (<STDIN>) {
		next unless ($sha1 eq $headrev);
        	...
                if ($ref =~ /$pattern/) {
                	push @found, $ref;
		} else {
                	push @guessed, $ref;
		}
	}
        if (@found == 1) {
        	print "$found[0]\n";
	} else if (@guessed == 1) {
        	print "$guessed[0]\n";
        }

or something like that?

> -ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" "$head" "$headrev")
> +ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" "${remote:-HEAD}" "$headrev")

There are three cases as far as ${remote:-HEAD} aka $head in the
script is concerned:

 1. The user said $3=local:remote; we do not want to guess, and we
    pass it thru, e.g. 'master:for-linus' will give 'for-linus',
    likely to mean 'refs/heads/for-linus' or 'refs/tags/for-linus'.
    The loop must find it if it is there and is a unique match.

 2. The user said $3=branch.

   a) It may have been pushed to the branch of the same name over
      there.  The loop must find it if it is there and is a unique
      match.

   b) It may have been pushed out as 'for-linus' branch or tag over
      there.  The loop must find it based on $headrev.

 3. The user said $3="", implying HEAD.

    Passing HEAD does not sound like a good way to handle this
    case, as the loop in find_matching_ref script would try to use
    it literally.

    With "git symbolic-ref HEAD | sed -e 's|^refs/heads/||" or an
    equivalent of it is used instead, we can reuse the logic in the
    second case fully, no?

It is too early to include in the discussion, but Peff's B@{publish}
notation may play a role in the future to convert 'local' to 'remote'
without the user having to say 'local:remote'.

Thanks.