Re: [PATCH v2 5/6] stash: default listing to "--cc --simplify-combined-diff"

From: Jeff King <peff@peff.net>
Date: Jul 31, 2014, 00:09 UTC
Message-ID: <20140731000907.GA22297@peff.net>
In-Reply-To: <xmqqtx5yy6nm.fsf@gitster.dls.corp.google.com>

On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> > "git log --cc" is one of the things I wanted for a long time to fix.
> > When the user explicitly asks "--cc", we currently ignore it, but
> > because we know the user wants to view combined diff, we should turn
> > "-p" on automatically.  And the change this patch introduces will be
> > broken when we fix "log --cc" ("stash list" will end up always
> > showing the patch, without a way to disable it).
> >
> > Can you make this conditional?  Do this only when <options> are
> > given to "git stash list" command and that includes "-p" or
> > something?

I'm definitely sympathetic. Using "--cc" with the diff family _does_ imply "-p" already, so it would be nice to do the same for the log family.

Show 24 quoted lines

> Perhaps something along this line?
> 
>  git-stash.sh | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/git-stash.sh b/git-stash.sh
> index ae73ba4..0db1b19 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -297,8 +297,15 @@ have_stash () {
>  
>  list_stash () {
>  	have_stash || return 0
> -	git log --format="%gd: %gs" -g --cc --simplify-combined-diff \
> -		"$@" $ref_stash --
> +	case " $* " in
> +	*' --cc '*)
> +		;; # the user knows what she is doing
> +	*' -p '* | *' -U'*)
> +		set x "--cc" "--simplify-combined-diff" "$@"
> +		shift
> +		;;
> +	esac
> +	git log --format="%gd: %gs" -g "$@" $ref_stash --

Ugh. I'd generally like to avoid this sort of ad-hoc command line parsing, as it is easy to get confused by option arguments or even non-options. At least we do not have to worry about pathspecs here, as we already do an explicit "--" (so we might be confused by them, but they are broken anyway).

Still, I wonder if a cleaner solution is to provide alternate "default to this" options for the revision.c option parser. We already have "--default" to pick the default starting point. Could we do something like "--default-merge-handling=cc" or something?

There's a similar ad-hoc parsing case in "stash show", where we add "--stat" if no arguments are given, but we have no clue if a diff format was given (so "git stash show --color" accidentally turns on patch format!). Something like "--default-diff-format=stat" would be more robust.

I dunno. Maybe it is over-engineering. I just hate to see solutions that work most of the time and crumble in weird corner cases, even if people don't hit those corner cases very often.

-Peff