Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

From: Björn Steinbrink <b.steinbrink@gmx.de>
Date: Nov 6, 2007, 06:59 UTC
Message-ID: <20071106065941.GA6423@atjola.homenet>
In-Reply-To: <Pine.LNX.4.64.0711052317170.4362@racer.site>

On 2007.11.05 23:18:36 +0000, Johannes Schindelin wrote:

Show 24 quoted lines

> Hi,
> 
> On Mon, 5 Nov 2007, Bj?rn Steinbrink wrote:
> 
> > On 2007.11.05 13:57:53 -0500, Kristian H?gsberg wrote:
> >
> > > The shell script just has
> > > 
> > > case "$all,$interactive,$also,$#" in
> > > *t,*t,*)
> > >         die "Cannot use -a, --interactive or -i at the same time." ;;
> > > 
> > > which doesn't seem to care about the value of $also.  As far as I 
> > > understand git commit, it doesn't make sense to pass any of -a, -i, -o 
> > > or --interactive at the same time so I guess I could join the checks
> > 
> > Note that there are only two commas. The asterisks catch everything and
> > $# won't be "t", so that catches anything with at least two t's.
> 
> So shouldn't it be
> 
> 	if (!!all + !!interactive + !!also > 1)
> 
> Hmm?

Ah, yeah, that's the short and sweet version, I always forget about the conversion to bool giving you 0/1 values... ;-)

Note though, that Kristian had a similar check at the end of his email, that included "only" (but lacked the bool conversion). The original reason why I thought that it would be better was that for example "git commit --all --only foo" didn't care about "only" at all. But that actually was because the --all + paths usage check was broken. So the fixed version actually refuses to use accept that, but with a (IMHO) not so good error message:

$ git commit -a -o file Paths with -a does not make sense.

Given that some people are used to just pass -a all the time, they might just automatically pass it together with -o. And I think that we actually want to tell them that -a + -o makes no sense instead. Just like we do for -a + -i, which is kind of the complementary usage error.

So I'd go for a correct version of Kristian's suggestion:

if (!!also + !!only + !!all + !!interactive > 1)
	die("Only one of --include/--only/--all/--interactive can be used.");

Björn