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

[Junio C Hamano <junkio@cox.net>]

"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).