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

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

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 22 Jan 2014 15:23:48 -0800
Subject: [PATCH] Make request-pull able to take a refspec of form local:remote

This allows a user to say that a local branch has a different name on
the remote server, using the same syntax that "git push" uses to create
that situation.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

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.

 git-request-pull.sh | 50 +++++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index 659a412155d8..c8ab0e912011 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -47,19 +47,23 @@ fi
 
 #
 # $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:
 # "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 ($exact,$found);
+	my ($head,$headrev) = (@ARGV);
+	my ($found);
+
 	while (<STDIN>) {
+		chomp;
 		my ($sha1, $ref, $deref) = /^(\S+)\s+([^^]+)(\S*)$/;
-		next unless ($sha1 eq $ARGV[1]);
-		if ($ref eq $ARGV[0]) {
-			$exact = $ref;
+		my ($pattern);
+		next unless ($sha1 eq $headrev);
+
+		$pattern="/$head\$";
+		if ($ref eq $head) {
+			$found = $ref;
+		}
+		if ($ref =~ /$pattern/) {
+			$found = $ref;
 		}
-		if ($sha1 eq $ARGV[0]) {
+		if ($sha1 eq $head) {
 			$found = $sha1;
 		}
 	}
-	if ($exact) {
-		print "$exact\n";
-	} elsif ($found) {
+	if ($found) {
 		print "$found\n";
 	}
 '
 
-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")
 
 if test -z "$ref"
 then
-	echo "warn: No match for $prettyhead found at $url" >&2
-	echo "warn: Are you sure you pushed '$prettyhead' there?" >&2
+	echo "warn: No match for commit $headrev found at $url" >&2
+	echo "warn: Are you sure you pushed '${remote:-HEAD}' there?" >&2
 	status=1
 fi
 
@@ -116,7 +124,7 @@ git show -s --format='The following changes since commit %H:
 
 are available in the git repository at:
 ' $merge_base &&
-echo "  $url $prettyhead" &&
+echo "  $url $remote" &&
 git show -s --format='
 for you to fetch changes up to %H:
 
-- 
1.9.rc0.10.gf0799f9.dirty

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

[Junio C Hamano]

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.

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

[Linus Torvalds]

On Thu, Jan 23, 2014 at 11:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

If you can fix that, then yes, that would be lovely. As it is, I
couldn't find any easily scriptable way to do that.

>>  #
>>  # 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?

Yes. I'm not really a perl person, and this came from me trying to
make the code more readable (and it used to do that magic quoting
thing inside the loop, I just used a helper pattern variable).

>> +             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...?

No, this is for when "head" ends up not being a ref, but a SHA1 expression.

IOW, for when you do something odd like

    git request-pull HEAD^^ origin HEAD^

when hacking things together. It doesn't actually generate the right
request-pull message (because there's no valid branch name), but it
*works* in the sense that you can get the diffstat etc and edit things
manually.

It's not a big deal - it has never really "worked", and I actually
broke that when I then used "$remote" that doesn't actually have the
SHA1 any more.

>> +     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.

Yes, so you'll get a warning (or, if you get a partial match, maybe
not even that), but the important part about all these changes is that
it DOESN'T MATTER.

Why? Because it no longer re-writes the target branch name based on
that match or non-match. So the pull request will be fine.

In other words, the really fundamental change here i that the "oops, I
couldn't find things on the remote" no longer affects the output. It
only affects the warning. And I think that's important.

It used to be that the remote matching actually changed the output of
the request-pull, and *THAT* was the fundamental problem.

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

It would be good, yes. But my perl-fu is weak, and I really didn't
want to worry about it. Also, as above: my primary issue was to not
screw up the output, so the remote matching actually has become much
less important, and now the warning about it is purely about being
helpful, it no longer fundamentally alters any semantics.

So I agree that there is room for improvement, but that's kind of
separate from the immediate problem I was trying to solve.

              Linus

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

[Junio C Hamano]

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

> Yes, so you'll get a warning (or, if you get a partial match, maybe
> not even that), but the important part about all these changes is that
> it DOESN'T MATTER.
>
> Why? Because it no longer re-writes the target branch name based on
> that match or non-match. So the pull request will be fine.

Will be fine, provided if they always use local:remote syntax, I'd
agree.

> In other words, the really fundamental change here i that the "oops, I
> couldn't find things on the remote" no longer affects the output. It
> only affects the warning. And I think that's important.
>
> It used to be that the remote matching actually changed the output of
> the request-pull, and *THAT* was the fundamental problem.

The fingers of users can be retrained to use the local:remote syntax
after we apologize for this change in the Release Notes, I see only
one issue (we seem to lose the message from the annotated/signed tag
when asking to pull it) remaining, after looking at what updates are
needed for t5150.

Thanks.

-- >8 --
Subject: [PATCH] pull-request: test updates

This illustrates behaviour changes that result from the recent
change by Linus.  Most shows good changes, but there may be
usability regressions:

 - The command continues to fail when the user forgot to push out
   before running the command, but the wording of the message has
   been slightly changed.

 - The command no longer guesses when asked to request the commit at
   the HEAD be pulled after pushing it to a branch 'for-upstream',
   even when that branch points at the correct commit.  The user
   must ask the command with the new "master:for-upstream" syntax.

 - The command no longer favours a tag that peels to the requested
   commit over a branch that points at the same commit.  This needs
   to be asked explicitly by specifying the tag object, not the
   commit.  But somehow this does not see to work (yet); somewhere
   the "tag-ness" of the requested ref seems to be lost.

The new behaviour needs to be documented in any case, but we need to
agree what the new behaviour should be before doing so, so...

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5150-request-pull.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 1afa0d5..412ee4f 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -86,7 +86,7 @@ test_expect_success 'setup: two scripts for reading pull requests' '
 	s/[-0-9]\{10\} [:0-9]\{8\} [-+][0-9]\{4\}/DATE/g
 	s/        [^ ].*/        SUBJECT/g
 	s/  [^ ].* (DATE)/  SUBJECT (DATE)/g
-	s/for-upstream/BRANCH/g
+	s|tags/full|BRANCH|g
 	s/mnemonic.txt/FILENAME/g
 	s/^version [0-9]/VERSION/
 	/^ FILENAME | *[0-9]* [-+]*\$/ b diffstat
@@ -127,7 +127,7 @@ test_expect_success 'pull request when forgot to push' '
 		test_must_fail git request-pull initial "$downstream_url" \
 			2>../err
 	) &&
-	grep "No branch of.*is at:\$" err &&
+	grep "No match for commit .*" err &&
 	grep "Are you sure you pushed" err
 
 '
@@ -141,7 +141,7 @@ test_expect_success 'pull request after push' '
 		git checkout initial &&
 		git merge --ff-only master &&
 		git push origin master:for-upstream &&
-		git request-pull initial origin >../request
+		git request-pull initial origin master:for-upstream >../request
 	) &&
 	sed -nf read-request.sed <request >digest &&
 	cat digest &&
@@ -160,7 +160,7 @@ test_expect_success 'pull request after push' '
 
 '
 
-test_expect_success 'request names an appropriate branch' '
+test_expect_success 'request asks HEAD to be pulled' '
 
 	rm -fr downstream.git &&
 	git init --bare downstream.git &&
@@ -179,11 +179,11 @@ test_expect_success 'request names an appropriate branch' '
 		read repository &&
 		read branch
 	} <digest &&
-	test "$branch" = tags/full
+	test -z "$branch"
 
 '
 
-test_expect_success 'pull request format' '
+test_expect_failure 'pull request format' '
 
 	rm -fr downstream.git &&
 	git init --bare downstream.git &&
@@ -212,8 +212,8 @@ test_expect_success 'pull request format' '
 		cd local &&
 		git checkout initial &&
 		git merge --ff-only master &&
-		git push origin master:for-upstream &&
-		git request-pull initial "$downstream_url" >../request
+		git push origin tags/full &&
+		git request-pull initial "$downstream_url" tags/full >../request
 	) &&
 	<request sed -nf fuzz.sed >request.fuzzy &&
 	test_i18ncmp expect request.fuzzy
@@ -229,7 +229,7 @@ test_expect_success 'request-pull ignores OPTIONS_KEEPDASHDASH poison' '
 		git checkout initial &&
 		git merge --ff-only master &&
 		git push origin master:for-upstream &&
-		git request-pull -- initial "$downstream_url" >../request
+		git request-pull -- initial "$downstream_url" master:for-upstream >../request
 	)
 
 '
-- 
1.9-rc0-250-ge2d8c96

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

[Linus Torvalds]

On Thu, Jan 23, 2014 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Will be fine, provided if they always use local:remote syntax, I'd
> agree.

Why? No sane user should actually need to use the local:remote syntax.

The normal situation should be that you create the correctly named
branch or tag locally, and then push it out under that name.

So I don't actually think anybody should need to be retrained, or
"always use the local:remote" syntax. The local:remote syntax exists
only for that special insane case where you used (the same)
local:remote syntax to push out a branch under a different name.

[ And yeah, maybe that behavior is more common than I think, but even
if it is, such behavior would always be among people who are *very*
aware of the whole "local branch vs remote branch name is different"
situation. ]

                   Linus

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

[Junio C Hamano]

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

> So I don't actually think anybody should need to be retrained, or
> "always use the local:remote" syntax. The local:remote syntax exists
> only for that special insane case where you used (the same)
> local:remote syntax to push out a branch under a different name.
>
> [ And yeah, maybe that behavior is more common than I think, but even
> if it is, such behavior would always be among people who are *very*
> aware of the whole "local branch vs remote branch name is different"
> situation. ]

As the new default for "git push" would push to the same name, I
agree that people who are now forced to use local:remote syntax
would be the ones who know what they are doing [*1*].

So there are two remaining items, I think.

 - After creating a tags/for-linus signed tag and pushing it to
   tags/for-linus, asking request-pull to request that tag to be
   pulled seems to lose the tag message from the output.

 - Docs.


[Footnote]

*1* Not that it is always acceptable to break the existing users as
    long as they are clueful ones and they are given an escape hatch.
    But this time I know I won't be in the middle of firestorm like
    the one we had immediately after 1.6.0, as long as I keep the
    URL of the message I am responding to in the list archive ;-)

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So there are two remaining items, I think.
>
>  - After creating a tags/for-linus signed tag and pushing it to
>    tags/for-linus, asking request-pull to request that tag to be
>    pulled seems to lose the tag message from the output.
>
>  - Docs.
>
> [Footnote]
>
> *1* Not that it is always acceptable to break the existing users as
>     long as they are clueful ones and they are given an escape hatch.
>     But this time I know I won't be in the middle of firestorm like
>     the one we had immediately after 1.6.0, as long as I keep the
>     URL of the message I am responding to in the list archive ;-)

I am not yet doing the docs, but here is a minimal (and I think is
the most sensible) fix to the "If I asked a tag to be pulled, I used
to get the message from the tag in the output---the updated code no
longer does so" problem.

With this fix, the updates to t5150 I queued on top of the two
patches can lose a "test_expect_failure".

I would not be surprised if there are other regressions, though
[*1*].  I am worried about regressions when the user explicitly asks
a ref to be pulled---e.g the command refuses to produce output and
instead fails (perhaps because the ambiguity check is overly
stricter than it should be), or the command produces output that is
different from what we used to produce (this patch is a fix to the
problem in that latter category, but there may be other differences
the existing tests are not covering).

[Footnote]

*1* No, I do not count "I used to be able to ask 'master' (or
implicitly 'HEAD' that I happen to be sitting on) to be pulled and
rely on that the command figures out that I have that commit on
'for-linus' in my publish repository, but that feature was removed"
as a regression.  Removing that cleverness is the point of this
series.

-- >8 --
Subject: [PATCH] request-pull: pick up tag message as before

The previous two steps were meant to stop promoting the explicit
refname the user gave to the command to a different ref that points
at it.  Most notably, we no longer substitute a branch name the user
used with a name of the tqag that points at the commit at the tip of
the branch.

However, they also lost the code that included the message in a
tag when the user _did_ ask the tag to be pulled.  Resurrect it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-request-pull.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index c8ab0e9..93b4135 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -132,6 +132,14 @@ for you to fetch changes up to %H:
 
 ----------------------------------------------------------------' $headrev &&
 
+if test $(git cat-file -t "$head") = tag
+then
+	git cat-file tag "$head" |
+	sed -n -e '1,/^$/d' -e '/^-----BEGIN PGP /q' -e p
+	echo
+	echo "----------------------------------------------------------------"
+fi &&
+
 if test -n "$branch_name"
 then
 	echo "(from the branch description for $branch_name local branch)"
-- 
1.9-rc1-183-g614c158

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

On Wed, Jan 29, 2014 at 03:34:32PM -0800, Junio C Hamano wrote:
> The previous two steps were meant to stop promoting the explicit
> refname the user gave to the command to a different ref that points
> at it.  Most notably, we no longer substitute a branch name the user
> used with a name of the tqag that points at the commit at the tip of

s/tqag/tag/


-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Linus Torvalds]

On Wed, Jan 29, 2014 at 3:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I am not yet doing the docs, but here is a minimal (and I think is
> the most sensible) fix to the "If I asked a tag to be pulled, I used
> to get the message from the tag in the output---the updated code no
> longer does so" problem.

That was a complete oversight/bug on my part, due to just removing the
tag_name special cases, not thinking about the tag message.

Thinking some more about the tag_name issue, I realize that the other
patch ("Make request-pull able to take a refspec of form
local:remote") broke another thing.

The first patch pretty-printed the local branch-name, removing "refs/"
and possibly "heads/" from the local refname. So for a branch, it
would ask people to just pull from the branch-name, and for a tag it
would ask people to pull from "tags/name", which is good policy. So if
you had a tag called "for-linus", it would say so (using
"tags/for-linus").

But the local:remote syntax thing ends up breaking that nice feature.
The old find_matching_refs would actually cause us to show the "tags"
part if it existed on the remote, but that had become pointless and
counter-productive with the first patch. But with the second patch,
maybe we should reinstate that logic..

                Linus

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

[Junio C Hamano]

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

> Thinking some more about the tag_name issue, I realize that the other
> patch ("Make request-pull able to take a refspec of form
> local:remote") broke another thing.
>
> The first patch pretty-printed the local branch-name, removing "refs/"
> and possibly "heads/" from the local refname. So for a branch, it
> would ask people to just pull from the branch-name, and for a tag it
> would ask people to pull from "tags/name", which is good policy. So if
> you had a tag called "for-linus", it would say so (using
> "tags/for-linus").
>
> But the local:remote syntax thing ends up breaking that nice feature.
> The old find_matching_refs would actually cause us to show the "tags"
> part if it existed on the remote, but that had become pointless and
> counter-productive with the first patch. But with the second patch,
> maybe we should reinstate that logic..

Sorry for back-burnering this topic so long.

I think the following does what you suggested in the message I am
responding to.

Now, hopefully the only thing we need is a documentation update and
the series should be ready to go.

-- >8 --
Subject: request-pull: resurrect "pretty refname" feature

When asking to fetch/pull a branch whose name is B or a tag whose
name is T, we used to show the command to run as:

	git pull $URL B
        git pull $URL tags/T
	
even when B and T were spelled in a more qualified way in order to
disambiguate, e.g. heads/B or refs/tags/T, but the recent update
lost this feature.  Resurrect it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-request-pull.sh     | 4 +++-
 t/t5150-request-pull.sh | 8 +++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index 93b4135..b67513a 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -53,6 +53,8 @@ fi
 local=${3%:*}
 local=${local:-HEAD}
 remote=${3#*:}
+pretty_remote=${remote#refs/}
+pretty_remote=${pretty_remote#heads/}
 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")}
@@ -124,7 +126,7 @@ git show -s --format='The following changes since commit %H:
 
 are available in the git repository at:
 ' $merge_base &&
-echo "  $url $remote" &&
+echo "  $url $pretty_remote" &&
 git show -s --format='
 for you to fetch changes up to %H:
 
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 2622057..75d6b38 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -216,8 +216,14 @@ test_expect_success 'pull request format' '
 		git request-pull initial "$downstream_url" tags/full >../request
 	) &&
 	<request sed -nf fuzz.sed >request.fuzzy &&
-	test_i18ncmp expect request.fuzzy
+	test_i18ncmp expect request.fuzzy &&
 
+	(
+		cd local &&
+		git request-pull initial "$downstream_url" tags/full:refs/tags/full
+	) >request &&
+	sed -nf fuzz.sed <request >request.fuzzy &&
+	test_i18ncmp expect request.fuzzy
 '
 
 test_expect_success 'request-pull ignores OPTIONS_KEEPDASHDASH poison' '

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sorry for back-burnering this topic so long.
>
> I think the following does what you suggested in the message I am
> responding to.
>
> Now, hopefully the only thing we need is a documentation update and
> the series should be ready to go.

... and here it is, to be sanity checked before I queue the whole
thing in 'next'.

-- >8 --
Subject: [PATCH] request-pull: documentation updates

The original description talked only about what it does.  Instead,
start it with the purpose of the command, i.e. what it is used for,
and then mention what it does to achieve that goal.

Clarify what <start>, <url> and <end> means in the context of the
overall purpose of the command.

Describe the extended syntax of <end> parameter that is used when
the local branch name is different from the branch name at the
repository the changes are published.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-request-pull.txt | 55 +++++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-request-pull.txt b/Documentation/git-request-pull.txt
index b99681c..57bc1f5 100644
--- a/Documentation/git-request-pull.txt
+++ b/Documentation/git-request-pull.txt
@@ -13,22 +13,65 @@ SYNOPSIS
 DESCRIPTION
 -----------
 
-Summarizes the changes between two commits to the standard output, and includes
-the given URL in the generated summary.
+Prepare a request to your upstream project to pull your changes to
+their tree to the standard output, by summarizing your changes and
+showing where your changes can be pulled from.
+
+The upstream project is expected to have the commit named by
+`<start>` and the output asks it to integrate the changes you made
+since that commit, up to the commit named by `<end>`, by visiting
+the repository named by `<url>`.
+
 
 OPTIONS
 -------
 -p::
-	Show patch text
+	Include patch text in the output.
 
 <start>::
-	Commit to start at.
+	Commit to start at.  This names a commit that is already in
+	the upstream history.
 
 <url>::
-	URL to include in the summary.
+	The repository URL to be pulled from.
 
 <end>::
-	Commit to end at; defaults to HEAD.
+	Commit to end at (defaults to HEAD).  This names the commit
+	at the tip of the history you are asking to be pulled.
++
+When the repository named by `<url>` has the commit at a tip of a
+ref that is different from the ref you have it locally, you can use
+the `<local>:<remote>` syntax, to have its local name, a colon `:`,
+and its remote name.
+
+
+EXAMPLE
+-------
+
+Imagine that you built your work on your `master` branch on top of
+the `v1.0` release, and want it to be integrated to the project.
+First you push that change to your public repository for others to
+see:
+
+	git push https://git.ko.xz/project master
+
+Then, you run this command:
+
+	git request-pull v1.0 https://git.ko.xz/project master
+
+which will produce a request to the upstream, summarizing the
+changes between the `v1.0` release and your `master`, to pull it
+from your public repository.
+
+If you pushed your change to a branch whose name is different from
+the one you have locally, e.g.
+
+	git push https://git.ko.xz/project master:for-linus
+
+then you can ask that to be pulled with
+
+	git request-pull v1.0 https://git.ko.xz/project master:for-linus
+
 
 GIT
 ---
-- 
1.9.0-293-gd838d6f

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

[Eric Sunshine]

On Wed, Mar 12, 2014 at 2:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] request-pull: documentation updates
>
> The original description talked only about what it does.  Instead,
> start it with the purpose of the command, i.e. what it is used for,
> and then mention what it does to achieve that goal.
>
> Clarify what <start>, <url> and <end> means in the context of the
> overall purpose of the command.
>
> Describe the extended syntax of <end> parameter that is used when
> the local branch name is different from the branch name at the
> repository the changes are published.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-request-pull.txt | 55 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-request-pull.txt b/Documentation/git-request-pull.txt
> index b99681c..57bc1f5 100644
> --- a/Documentation/git-request-pull.txt
> +++ b/Documentation/git-request-pull.txt
> @@ -13,22 +13,65 @@ SYNOPSIS
>  DESCRIPTION
>  -----------
>
> -Summarizes the changes between two commits to the standard output, and includes
> -the given URL in the generated summary.
> +Prepare a request to your upstream project to pull your changes to
> +their tree to the standard output, by summarizing your changes and
> +showing where your changes can be pulled from.

Perhaps splitting this into two sentence (and using fewer to's) would
make it a bit easier to grok? Something like:

    Generate a request asking your upstream project to pull changes
    into their tree. The request, printed to standard output,
    summarizes the changes and indicates from where they can be
    pulled.

> +The upstream project is expected to have the commit named by
> +`<start>` and the output asks it to integrate the changes you made
> +since that commit, up to the commit named by `<end>`, by visiting
> +the repository named by `<url>`.
> +
>
>  OPTIONS
>  -------
>  -p::
> -       Show patch text
> +       Include patch text in the output.
>
>  <start>::
> -       Commit to start at.
> +       Commit to start at.  This names a commit that is already in
> +       the upstream history.
>
>  <url>::
> -       URL to include in the summary.
> +       The repository URL to be pulled from.
>
>  <end>::
> -       Commit to end at; defaults to HEAD.
> +       Commit to end at (defaults to HEAD).  This names the commit
> +       at the tip of the history you are asking to be pulled.
> ++
> +When the repository named by `<url>` has the commit at a tip of a
> +ref that is different from the ref you have it locally, you can use

Did you want to drop "it" from this sentence? Or did you mean to say
"the ref as you have it locally"?

> +the `<local>:<remote>` syntax, to have its local name, a colon `:`,
> +and its remote name.
> +
> +
> +EXAMPLE
> +-------
> +
> +Imagine that you built your work on your `master` branch on top of
> +the `v1.0` release, and want it to be integrated to the project.
> +First you push that change to your public repository for others to
> +see:
> +
> +       git push https://git.ko.xz/project master
> +
> +Then, you run this command:
> +
> +       git request-pull v1.0 https://git.ko.xz/project master
> +
> +which will produce a request to the upstream, summarizing the
> +changes between the `v1.0` release and your `master`, to pull it
> +from your public repository.
> +
> +If you pushed your change to a branch whose name is different from
> +the one you have locally, e.g.
> +
> +       git push https://git.ko.xz/project master:for-linus
> +
> +then you can ask that to be pulled with
> +
> +       git request-pull v1.0 https://git.ko.xz/project master:for-linus
> +
>
>  GIT
>  ---
> --
> 1.9.0-293-gd838d6f

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +Prepare a request to your upstream project to pull your changes to
>> +their tree to the standard output, by summarizing your changes and
>> +showing where your changes can be pulled from.
>
> Perhaps splitting this into two sentence (and using fewer to's) would
> make it a bit easier to grok? Something like:
>
>     Generate a request asking your upstream project to pull changes
>     into their tree. The request, printed to standard output,
>     summarizes the changes and indicates from where they can be
>     pulled.

Thanks.

>> +When the repository named by `<url>` has the commit at a tip of a
>> +ref that is different from the ref you have it locally, you can use
>
> Did you want to drop "it" from this sentence? Or did you mean to say
> "the ref as you have it locally"?

Thanks for your careful reading.  Will drop "it".