Re: [PATCH] request-pull: avoid mentioning that the start point is a single commit

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 29, 2010, 07:33 UTC
Message-ID: <7vfx5pjrgm.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100129011817.GT12429@genesis.frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 30 quoted lines

> Previously we ran shortlog on the start commit which always printed
> "(1)" after the start commit, which gives no information, but makes the
> output less easy to read. Avoid doing so.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>
> So for example the 'git request-pull master~2 . master' output diff is
> the following here:
>
> 	 The following changes since commit 68186857a9bb0a71e9456155623e02d398a5b817:
> 	-  Junio C Hamano (1):
> 	-        Merge branch 'il/maint-colon-address'
> 	+  Junio C Hamano: Merge branch 'il/maint-colon-address'
>
> 	 are available in the git repository at:
>
>  git-request-pull.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-request-pull.sh b/git-request-pull.sh
> index 630cedd..8475919 100755
> --- a/git-request-pull.sh
> +++ b/git-request-pull.sh
> @@ -66,7 +66,7 @@ if [ -z "$branch" ]; then
>  fi
>  
>  echo "The following changes since commit $baserev:"
> -git shortlog --max-count=1 $baserev | sed -e 's/^\(.\)/  \1/'
> +git log --max-count=1 --pretty=format:"  %an: %s%n%n" $baserev

A few comments:

 - Modernising implementation by using tools different from what the
   original used (i.e. shortlog -> log) is fine, but I'd recommend doing
   so even more thoroughly.  Use "show -s" instead of "log -1" and
   "--format=" instead of "--pretty=format:", for example.

 - If the stated goal of the change is to remove " (1)" which is
   distracting with no useful information, remove that and only that,
   without changing anything else in the output.

 - On the other hand, if you find that "AuthorName: " part is less useful
   in identifying the commit than its title to help the requestee, change
   the whole thing to make it even more useful.

So I'd suggest either:

	git show -s --format="  %an:%n        %s" $baserev

to be conservative, or

	git show -s --format="  %s (%an)" $baserev

or even to this:

	git show -s --format="  %s (%ci)" $baserev

I suspect that the last one would be the easiest for the requestee to judge the freshness of the branch.

Why isn't the "The following changes..." line not part of the --format thing, by the way? From the POV of readability of the code (not necessarily the output), doing it this way might be the cleanest:

-- >8 -- git show -s --format='The following changes since %H

    %s (%ci)

are available in the git repository at:' $baserev echo " $url $branch" -- 8< --