Re: [PATCH] git-request-pull: add --stat option

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2014, 17:46 UTC
Message-ID: <xmqqbnvqwrgu.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1398331763-601-1-git-send-email-jslaby@suse.cz>

Jiri Slaby <jslaby@suse.cz> writes:

Show 5 quoted lines

> Which is passed on to git diff. I very need this option instead of
> changing the terminal size.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---

Interesting. I wonder if that suggests perhaps the default may be better if it were --stat=80 regardless of your terminal width.

Oh, wait. That is the default we use when the output is not connected to the terminal.

Initially, I thought that the motivation behind this is that you got complaint from the recipient of your request that was generated to fit the width of _your_ taste (which is a lot wider than the standard 80) because you run the command in a wide terminal. But that does not sound like it, as sending out a request would go more like:

    $ git request-pull ... >request.txt
    $ edit request.txt
    $ mua send request.txt

and you would be getting 80-column output in that workflow.

What are you using the output of the script for, and why do you "very need this instead of changing the terminal size"?

I am puzzled.

Perhaps is it the case that "--stat" output with full width of the terminal does *not* suit _your_ taste (not just the recipient's), and that is not limited to the request-pull output, but shared across "log -p --stat", "diff --stat", and friends? I wonder if it would be a better solution for you and those in the same situation to set diff.statgraphwidth or something so that all these output are limited to a reasonable width, if that is the case?

Perhaps that diff.statgraphwidth that only specifies the graph part is too unwieldy and having a diff.statwidth or something that allows you to customize that "80 or terminal width" in a more direct way is needed?

Regardless, having a way to pass thru an option, like this patch does, is independently a good thing, I would tend to think. But "I need it instead of changing the terminal size" does not look like a sufficient and readable justification that describes why.

Show 29 quoted lines

>  git-request-pull.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/git-request-pull.sh b/git-request-pull.sh
> index 5c1599752314..a23f03fddec0 100755
> --- a/git-request-pull.sh
> +++ b/git-request-pull.sh
> @@ -13,6 +13,7 @@ OPTIONS_STUCKLONG=
>  OPTIONS_SPEC='git request-pull [options] start url [end]
>  --
>  p    show patch text as well
> +stat= specify stat output (see man git-diff for details)
>  '
>  
>  . git-sh-setup
> @@ -21,11 +22,16 @@ GIT_PAGER=
>  export GIT_PAGER
>  
>  patch=
> +stat=--stat
>  while	case "$#" in 0) break ;; esac
>  do
>  	case "$1" in
>  	-p)
>  		patch=-p ;;
> +	--stat)
> +		stat="$1=$2"
> +		shift
> +		;;

If somebody did not want to give diffstat output for whatever reason, wouldn't it be natural to want to say

	request-pull --stat= ...other options...

rather than having to say it with an explicit empty string, i.e.

	request-pull --stat "" ...other options...

In other words, I think the patch should also add

	--stat=*)
        	stat="$1"
		;;

Show 9 quoted lines

>  	--)
>  		shift; break ;;
>  	-*)
> @@ -152,6 +158,6 @@ then
>  fi &&
>  
>  git shortlog ^$baserev $headrev &&
> -git diff -M --stat --summary $patch $merge_base..$headrev || status=1
> +git diff -M $stat --summary $patch $merge_base..$headrev || status=1

This would not let the command notice a user error on the command line of request-pull, e.g.

	request-pull --stat='30 bar baz' ...other options...

because it will end up passing "--stat=30", "bar" and "baz" as separate options to it, no?

	diff -M ${stat+="$stat"} ...

perhaps?

>  
>  exit $status