Re: [PATCH] bisect: fix quoting TRIED revs when "bad" commit is also "skip"ped

[Christian Couder <chriscool@tuxfamily.org>]

Le jeudi 26 février 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Before this patch, when the "bad" commit was also "skip"ped and
> > when more than one commit was skipped, the "filter_skipped" function
> > would have printed something like:
>
> Everybody knows that the problem description that comes at the beginning
> of the commit log message talks about the state of the code before the
> patch is applied.
>
> Try reading the first sentence without "Before this patch, ".  It still
> makes perfect sense and more importantly, it is much more readable.

Ok, I will drop that.

> > bisect_rev=<hash1>|<hash2>
> >
> > (where <hash1> and <hash2> are hexadecimal sha1 hashes)
> >
> > and this would have been evaled later as piping "bisect_rev=<hash1>"
> > into "<hash2>", which would have failed.
>
> I am a bit worried why this "would have failed" was not noticed.

Perhaps because people do not often use "skip" and "bad" on the same commit.
There is an eval error printed on STDERR when this happens.

> > So this patch makes the "filter_skipped" function properly quote
> > what it outputs, so that it will print something like:
> >
> > bisect_rev="<hash1>|<hash2>"
> >
> > which will be properly evaled later.
> >
> > A test case is added to the test suite.
>
> Thanks.  Fixes before the 1.6.2 release are very much welcomed.
>
> > And while at it, we also remove a spurious space where the
> > "exit_if_skipped_commits" function is defined.
>
> Looking at:
>
> $ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- '*.sh' |
>   sed -e 's/^[^ (]*/X/' | sort | uniq -c
>
> and then doing the same for only git-bisect.sh, i.e.
>
> $ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- 'git-bisect.sh' |
>   sed -e 's/^[^ (]*/X/' | sort | uniq -c
>
> you will notice that git-bisect is an odd-man out that uses 3 "func () {"
> and 13 "func() {".  Majority of functions have one SP after the function
> name.
>
> If we were to standardize on one style, we should consistently have SP
> there.

Ok, I dropped the related change in the series I will send just after this 
email.

> > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> > index dd7eac8..b5da16f 100755
> > --- a/t/t6030-bisect-porcelain.sh
> > +++ b/t/t6030-bisect-porcelain.sh
> > @@ -224,6 +224,31 @@ test_expect_success 'bisect skip: cannot tell
> > between 2 commits' ' fi
> >  '
> >
> > +# $HASH1 is good, $HASH4 is both skipped and bad, we skip $HASH3
> > +# and $HASH2 is good,
> > +# so we should not be able to tell the first bad commit
> > +# among $HASH3 and $HASH4
> > +test_expect_success 'bisect skip: with commit both bad and skipped' '
> > +	git bisect start &&
> > +	git bisect skip &&
> > +	git bisect bad &&
> > +	git bisect good $HASH1 &&
> > +	git bisect skip &&
> > +	if git bisect good > my_bisect_log.txt
>
> An unpatched "git-bisect" seems to say "32a594a3 was both good and bad"
> in its my_bisect_log.txt .

Yes, but there is also an error printed on STDERR.

> This makes me suspect that we are forgetting 
> to check return status after we eval the output from filter_skipped
> function. Shouldn't the function should string its commands together with
> "&&" to protect it from a breakage like this?

Right, that would be an improvement. I put it in another patch though, 
because it is not really needed to fix a breakage.

> Also, VARS, FOUND and TRIED are not initialized anywhere.  We should
> protect ourselves from environment variables the user may have with these
> names before starting bisect to break the logic of this code.

Yeah I noticed that, I put that change in the first patch.

> Back to the test script.
>
> > +		grep "first bad commit could be any of" my_bisect_log.txt &&
> > +		test_must_fail grep $HASH1 my_bisect_log.txt &&
> > +		test_must_fail grep $HASH2 my_bisect_log.txt &&
>
> These two are easier to read with ! not test_must_fail; we do not expect
> grep to be buggy and dump core ;-)

Ok, there is now "!" in the first patch.

> So perhaps the big loop should be better written like this...

Yeah, it will look more or less like that.

Thanks,
Christian.