Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2009, 19:45 UTC
Message-ID: <7v8wnflrws.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20090308144240.GA30794@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

>   1. Declare appended newline a forbidden style, fix all existing cases
>      in the test suite, and be on the lookout for new ones.
>
>      The biggest problem with this option is that we have no automated
>      way of policing. Such tests will just silently pass on the broken
>      platform.
>
>   2. Have test_run_ canonicalize the snippet by removing trailing
>      newlines.
>
>   3. Declare FreeBSD's /bin/sh unfit for git consumption, and require
>      bash for the test suite.
>
> I think (2) is the most reasonable option of those choices.
>
> We could also try to convince FreeBSD that it's a bug, but that doesn't
> change the fact that the tests are broken on every existing version.

If this part from your analysis is true for a shell:

Show 8 quoted lines

> eval 'false
>
> '
> echo status is $?
>
> generates:
> ...
>   status is 0

I would be very tempted to declare that shell is unfit for any serious use, not just for test suite. Removing the empty line at the end of a scriptlet that such a broken shell misinterprets as an empty command that is equivalent to ":" (or "true") might hide breakages in the test suite, but

 (1) eval "$string" is used outside of test suite, most notably "am" and
     "bisect".  I think "am"'s use is safe, but I wouldn't be surprised if
     the scriptlet "bisect" internally creates has empty lines if only for
     debuggability; and more importantly

 (2) who knows what _other_ things may be broken in such a shell?