* Re: [PATCH] git-request-pull: add --stat option
2014-04-24 9:29 [PATCH] git-request-pull: add --stat option Jiri Slaby
@ 2014-04-24 17:46 ` Junio C Hamano
0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2014-04-24 17:46 UTC (permalink / raw)
To: Jiri Slaby; +Cc: git
Jiri Slaby <jslaby@suse.cz> writes:
> 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.
> 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"
;;
> --)
> 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
^ permalink raw reply [flat|nested] 2+ messages in thread