[PATCH v3] request-pull: state what commit to expect

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2011, 19:04 UTC
Message-ID: <7viposfgvd.fsf_-_@alter.siamese.dyndns.org>
In-Reply-To: <7vobynui8a.fsf@alter.siamese.dyndns.org>

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

Show 15 quoted lines

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> I think that would probably be a good idea, although I'd actually
>> prefer you to be more verbose, and more human-friendly, and actually
>> talk about the commit in a readable way. Get rid of the *horrible*
>> BRANCH-NOT-VERIFIED message...
>>
>>  Top commit 1f51b001cccf: "Merge branches 'cns3xxx/fixes',
>>  'omap/fixes' and 'davinci/fixes' into fixes"
>>
>>  and at *that* point you might have a "UNVERIFIED" notice for people
>> to check if they forgot to push.
>
> That UNVERIFIED thing was neither my favorite nor my idea, and I'd happily
> rip it out in any second ;-)

So this is the third round.

-- >8 -- The message gives a detailed explanation of the commit the requester based the changes on, but lacks information that is necessary for the person who performs a fetch & merge in order to verify that the correct branch was fetched when responding to the pull request.

Add a few more lines to describe the commit at the tip expected to be fetched to the same level of detail as the base commit.

Also update the warning message slightly when the script notices that the commit may not have been pushed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

A UI wart that we cannot fix without breaking backward compatibility is that the "end" parameter (which defaults to HEAD and is assigned to $head variable in the script) the requestor uses from the command line names a commit (often the name of a local branch), but for the purpose of telling which ref to pull from the public repository, that is a _wrong_ thing to give to the recipient.

Because the act of generating a request-pull message and the act of pushing to the public repository are not linked in any way, the script does not know _how_ the requestor caused (or intends to cause) the commit to sit at the tip of which branch. There is no guarantee that a lazy "git push" that relies on the configured refspec will be (or have been) used, so even parsing the output from "git push -n --porcelain -v $there" would not tell the script which branch the commit to be pulled is to be pushed out to, or if the branch is consistent with the request message.

The use of "git ls-remote" in the script and picking one of the refs that matches the commit object at random from its output is unsatisfactory, but that is unfortunately the best this script could do without correcting the design mistake and redefining what the "end" parameter means.

If we can break the backward compatibility and redefine that the "end" parameter now means the name of the branch at the public repository, it would make the operation a lot more robust. We could then:

 - $branch is what is given by the end user (it is an error not to give
   the "end" parameter);
 - run "git ls-remote $url $head" to find $headrev;
 - generate the message and shortlog using the information obtained from
   $url; and
 - get rid of "did you forget to push" message.

We could allow adding yet another argument which names a commit object locally, and make sure if the $headrev observed by ls-remote does not match it.

---
 git-request-pull.sh     |   34 +++++++++++++++++++---------------
 t/t5150-request-pull.sh |    6 ++++++
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index afb75e8..438e7eb 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -35,7 +35,7 @@ do
 	shift
 done
 
-base=$1 url=$2 head=${3-HEAD}
+base=$1 url=$2 head=${3-HEAD} status=0
 
 test -n "$base" && test -n "$url" || usage
 baserev=$(git rev-parse --verify "$base"^0) &&
@@ -51,25 +51,29 @@ find_matching_branch="/^$headrev	"'refs\/heads\//{
 }'
 branch=$(git ls-remote "$url" | sed -n -e "$find_matching_branch")
 url=$(git ls-remote --get-url "$url")
-if test -z "$branch"
-then
-	echo "warn: No branch of $url is at:" >&2
-	git log --max-count=1 --pretty='tformat:warn:   %h: %s' $headrev >&2
-	echo "warn: Are you sure you pushed $head there?" >&2
-	echo >&2
-	echo >&2
-	branch=..BRANCH.NOT.VERIFIED..
-	status=1
-fi
 
 git show -s --format='The following changes since commit %H:
 
   %s (%ci)
 
-are available in the git repository at:' $baserev &&
-echo "  $url $branch" &&
-echo &&
+are available in the git repository at:
+' $baserev &&
+echo "  $url${branch+ $branch}" &&
+git show -s --format='
+for you to fetch changes up to %H:
+
+  %s (%ci)
+
+----------------------------------------------------------------' $headrev &&
 
 git shortlog ^$baserev $headrev &&
-git diff -M --stat --summary $patch $merge_base..$headrev || exit
+git diff -M --stat --summary $patch $merge_base..$headrev || status=1
+
+if test -z "$branch"
+then
+	echo "warn: No branch of $url is at:" >&2
+	git show -s --format='warn:   %h: %s' $headrev >&2
+	echo "warn: Are you sure you pushed '$head' there?" >&2
+	status=1
+fi
 exit $status
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 9cc0a42..5bd1682 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -193,8 +193,14 @@ test_expect_success 'pull request format' '
 	  SUBJECT (DATE)
 
 	are available in the git repository at:
+
 	  URL BRANCH
 
+	for you to fetch changes up to OBJECT_NAME:
+
+	  SUBJECT (DATE)
+
+	----------------------------------------------------------------
 	SHORTLOG
 
 	DIFFSTAT

-- 
1.7.7.rc1.3.g559357