[PATCH] Improve request-pull to handle non-rebased branches

[PATCH] Improve request-pull to handle non-rebased branches

[Shawn O. Pearce]

This is actually a few different changes to request-pull,
making it slightly smarter:

 1) Compute the merge-base between the two revisions upfront
    and reuse that selected merge-base for all log and diff
    operations.

 2) Disable the PAGER when running shortlog and diff, as these
    would otherwise activate the pager for each command when
    git-request-pull is run on a tty.  Users can get the entire
    output paged (if desired) using `git -p request-pull`.

 3) Use shortlog rather than `git log | git shortlog` now that
    recent shortlog versions are able to run the revision listing
    internally.

 4) Attempt to resolve the input URL using the user's configured
    remotes.  This is useful if the URL you want the recipient to
    see is also the one you used to push your changes.  If not a
    config-file remote could easily be setup for the public URL
    and request-pull can be passed that instead.

 5) Minor cleanup of revision->base variable names, making it
    follow the head/headrev naming convention that is already
    in use.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git-request-pull.sh |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index 4eacc3a..367b262 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -9,25 +9,27 @@ LONG_USAGE='Summarizes the changes since <commit> to the standard output,
 and includes <url> in the message generated.'
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
+. git-parse-remote
 
-revision=$1
+base=$1
 url=$2
 head=${3-HEAD}
 
-[ "$revision" ] || usage
+[ "$base" ] || usage
 [ "$url" ] || usage
 
-baserev=`git-rev-parse --verify "$revision"^0` &&
+baserev=`git-merge-base "$base" "$head"` &&
 headrev=`git-rev-parse --verify "$head"^0` || exit
 
+PAGER=
+export PAGER
 echo "The following changes since commit $baserev:"
-git log --max-count=1 --pretty=short "$baserev" |
-git-shortlog | sed -e 's/^\(.\)/  \1/'
+git shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'
 
 echo "are found in the git repository at:" 
 echo
-echo "  $url"
+echo "  `get_remote_url "$url"`"
 echo
 
-git log  $baserev..$headrev | git-shortlog ;
-git diff -M --stat --summary $baserev..$headrev
+git shortlog ^$baserev $headrev
+git diff -M --stat --summary $baserev $headrev
-- 
1.5.1.1.135.gf948

Re: [PATCH] Improve request-pull to handle non-rebased branches

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> -revision=$1
> +base=$1
>  url=$2
>  head=${3-HEAD}
>  
> -[ "$revision" ] || usage
> +[ "$base" ] || usage
>  [ "$url" ] || usage
>  
> -baserev=`git-rev-parse --verify "$revision"^0` &&
> +baserev=`git-merge-base "$base" "$head"` &&
>  headrev=`git-rev-parse --verify "$head"^0` || exit
>  
> +PAGER=
> +export PAGER
>  echo "The following changes since commit $baserev:"
> -git log --max-count=1 --pretty=short "$baserev" |
> -git-shortlog | sed -e 's/^\(.\)/  \1/'
> +git shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'

I know the message says "since commit X", so taking merge base
feels technically correct from that point of view.  But I am not
convinced this is a good change.

It is acceptable to send a pull request from somewhat stale side
branch.  git projects (not necessarily git.git itself, but the
project with distributed management style that git aims to
support well) are inherently peer-to-peer, not hub-and-spoke, so
subsystem people should not be forced to rebase to get included.

However, I think the common courtesy _should_ be for pull
requestor to be confident that what s/he is requesting to be
pulled would result in sensible merge results, before sending
such a pull request.

And one way to show that you did your side of due diligence is
to say "This would merge to your tip".  Taking merge-base and
saying only "I forked from you this long time ago" makes the
output convey a wrong message.  I would certainly be suspicious
of a message that says "Here are fixes since v1.3.0", even if
the changes are isolated to the files that never changed since
v1.3.0.

>  echo "are found in the git repository at:" 
>  echo
> -echo "  $url"
> +echo "  `get_remote_url "$url"`"
>  echo

If you can verify headrev is at the tip of a branch (and that is
what you are asking to be pulled), I think we would want to see
the branch name at the end.  A recent example from the kernel
list:

   On Thu, 19 Apr 2007, Avi Kivity wrote:
   > 
   > Please pull from the 'linus' branch of
   > 
   >   git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm.git

   *please* put the branch-name after the git repo, so that I can 
   cut-and-paste without noticing only afterwards that the diffstat doesn't 
   match what it was supposed to, and I got the wrong commits, and have to 
   undo and re-do the pull..

   Yeah, I didn't do that mistake this time, and in fact, I seldom do, but 
   every time the branch is mentioned somewhere else than with the actual 
   repo to pull, I get nervous. 

   So I've said this a million times before, but I'll say it one more 
   time: if it's not the main branch, the message should be

      Please pull from the 'linus' branch of

        git://git.kernel.org/pub/scm/linux/kernel/git/avi/kvm.git linus

      to get the following changes: ...

   so that when I cut-and-paste the URL (you didn't think I'd try to type it 
   in by hand, did you?) I get the branch automatically.

> -git log  $baserev..$headrev | git-shortlog ;
> -git diff -M --stat --summary $baserev..$headrev
> +git shortlog ^$baserev $headrev
> +git diff -M --stat --summary $baserev $headrev

Use of the merge base revision for the last diff makes sense.
There should not be any difference for the shortlog if there is
only one merge base (and if there is more than one, then using
only one of them is probably buggy), so I think you would want
to say "shortlog $base..$headrev" instead (iow keep the original
for this shortlog).

[PATCH] Improve request-pull to handle non-rebased branches

[Shawn O. Pearce]

This is actually a few different changes to request-pull,
making it slightly smarter:

 1) Minor cleanup of revision->base variable names, making it
    follow the head/headrev naming convention that is already
    in use.

 2) Compute the merge-base between the two revisions upfront
    and reuse that selected merge-base to create the diffstat.

 3) Refuse to generate a pull request for branches that have no
    existing relationship.  These aren't very common and would mess
    up our diffstat generation.

 4) Disable the PAGER when running shortlog and diff, as these
    would otherwise activate the pager for each command when
    git-request-pull is run on a tty.  Instead users can get the
    entire output paged (if desired) using `git -p request-pull`.

 5) Use shortlog rather than `git log | git shortlog` now that
    recent shortlog versions are able to run the revision listing
    internally.

 6) Attempt to resolve the input URL using the user's configured
    remotes.  This is useful if the URL you want the recipient to
    see is also the one you used to push your changes.  If not a
    config-file remote could easily be setup for the public URL
    and request-pull could be passed that name instead.

 7) Automatically guess and include the remote branch name in the
    body of the message.  We list the branch name immediately after
    the URL, making it easy for the recipient to copy and paste
    the entire line onto a `git pull` command line.  Rumor has it
    Linus likes this format, for exactly that reason.

    If multiple branches at the remote match $headrev we take the
    first one returned by peek-remote and assume it is suitable.

    If no branches are available we warn the user about the problem,
    but insert a static string that is not a valid branch name
    and would be obvious to anyone reading the message as being
    totally incorrect.  This allows users to still generate a
    template message without network access (for example) and
    hand-correct the bits that cannot be verified.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 This is attempt #2, after including your recent comments.  Thoughts?

 git-request-pull.sh |   41 ++++++++++++++++++++++++++++++++---------
 1 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index 4eacc3a..ba577d4 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -9,25 +9,48 @@ LONG_USAGE='Summarizes the changes since <commit> to the standard output,
 and includes <url> in the message generated.'
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
+. git-parse-remote
 
-revision=$1
+base=$1
 url=$2
 head=${3-HEAD}
 
-[ "$revision" ] || usage
+[ "$base" ] || usage
 [ "$url" ] || usage
 
-baserev=`git-rev-parse --verify "$revision"^0` &&
+baserev=`git-rev-parse --verify "$base"^0` &&
 headrev=`git-rev-parse --verify "$head"^0` || exit
 
+merge_base=`git merge-base $baserev $headrev` ||
+die "fatal: No commits in common between $base and $head"
+
+url="`get_remote_url "$url"`"
+branch=`git peek-remote "$url" \
+	| sed -n -e "/^$headrev	refs.heads./{
+		s/^.*	refs.heads.//
+		p
+		q
+	}"`
+if [ -z "$branch" ]; then
+	echo "warn: No branch of $url is at:" >&2
+	git log --max-count=1 --pretty='format: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
+
+PAGER=
+export PAGER
 echo "The following changes since commit $baserev:"
-git log --max-count=1 --pretty=short "$baserev" |
-git-shortlog | sed -e 's/^\(.\)/  \1/'
+git shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'
 
-echo "are found in the git repository at:" 
+echo "are available in the git repository at:"
 echo
-echo "  $url"
+echo "  $url $branch"
 echo
 
-git log  $baserev..$headrev | git-shortlog ;
-git diff -M --stat --summary $baserev..$headrev
+git shortlog ^$baserev $headrev
+git diff -M --stat --summary $merge_base $headrev
+exit $status
-- 
1.5.1.1.135.gf948

[PATCH] Improve request-pull to handle non-rebased branches

[Shawn O. Pearce]

This is actually a few different changes to request-pull,
making it slightly smarter:

 1) Minor cleanup of revision->base variable names, making it
    follow the head/headrev naming convention that is already
    in use.

 2) Compute the merge-base between the two revisions upfront
    and reuse that selected merge-base to create the diffstat.

 3) Refuse to generate a pull request for branches that have no
    existing relationship.  These aren't very common and would mess
    up our diffstat generation.

 4) Disable the PAGER when running shortlog and diff, as these
    would otherwise activate the pager for each command when
    git-request-pull is run on a tty.  Instead users can get the
    entire output paged (if desired) using `git -p request-pull`.

 5) Use shortlog rather than `git log | git shortlog` now that
    recent shortlog versions are able to run the revision listing
    internally.

 6) Attempt to resolve the input URL using the user's configured
    remotes.  This is useful if the URL you want the recipient to
    see is also the one you used to push your changes.  If not a
    config-file remote could easily be setup for the public URL
    and request-pull could be passed that name instead.

 7) Automatically guess and include the remote branch name in the
    body of the message.  We list the branch name immediately after
    the URL, making it easy for the recipient to copy and paste
    the entire line onto a `git pull` command line.  Rumor has it
    Linus likes this format, for exactly that reason.

    If multiple branches at the remote match $headrev we take the
    first one returned by peek-remote and assume it is suitable.

    If no branches are available we warn the user about the problem,
    but insert a static string that is not a valid branch name
    and would be obvious to anyone reading the message as being
    totally incorrect.  This allows users to still generate a
    template message without network access (for example) and
    hand-correct the bits that cannot be verified.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 This is a resend, as it appears as though you haven't merged it yet?

 git-request-pull.sh |   41 ++++++++++++++++++++++++++++++++---------
 1 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index 4eacc3a..ba577d4 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -9,25 +9,48 @@ LONG_USAGE='Summarizes the changes since <commit> to the standard output,
 and includes <url> in the message generated.'
 SUBDIRECTORY_OK='Yes'
 . git-sh-setup
+. git-parse-remote
 
-revision=$1
+base=$1
 url=$2
 head=${3-HEAD}
 
-[ "$revision" ] || usage
+[ "$base" ] || usage
 [ "$url" ] || usage
 
-baserev=`git-rev-parse --verify "$revision"^0` &&
+baserev=`git-rev-parse --verify "$base"^0` &&
 headrev=`git-rev-parse --verify "$head"^0` || exit
 
+merge_base=`git merge-base $baserev $headrev` ||
+die "fatal: No commits in common between $base and $head"
+
+url="`get_remote_url "$url"`"
+branch=`git peek-remote "$url" \
+	| sed -n -e "/^$headrev	refs.heads./{
+		s/^.*	refs.heads.//
+		p
+		q
+	}"`
+if [ -z "$branch" ]; then
+	echo "warn: No branch of $url is at:" >&2
+	git log --max-count=1 --pretty='format: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
+
+PAGER=
+export PAGER
 echo "The following changes since commit $baserev:"
-git log --max-count=1 --pretty=short "$baserev" |
-git-shortlog | sed -e 's/^\(.\)/  \1/'
+git shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'
 
-echo "are found in the git repository at:" 
+echo "are available in the git repository at:"
 echo
-echo "  $url"
+echo "  $url $branch"
 echo
 
-git log  $baserev..$headrev | git-shortlog ;
-git diff -M --stat --summary $baserev..$headrev
+git shortlog ^$baserev $headrev
+git diff -M --stat --summary $merge_base $headrev
+exit $status
-- 
1.5.2.rc1.687.gcb389