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

11 messages, 5 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:21

Jeff King [off-list ref] writes:
  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:
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?

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

From: Jeff King <hidden>
Date: 2016-06-15 22:46:22

On Sun, Mar 08, 2009 at 12:45:55PM -0700, Junio C Hamano wrote:
If this part from your analysis is true for a shell:
quoted
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?
OK, good points. I was just hoping not to cause people on FreeBSD undue
pain. What is the best way to make such a declaration? I can think of:

  1. A mention in the release notes.

  2. A test in the Makefile similar to the $(:) test.

  3. Getting in touch with the freebsd ports maintainer for git and
     suggesting a dependency on bash (and/or seeing if he wants to push
     through a fix for /bin/sh).

     I don't know if the same problem exists on other BSD-influenced systems,
     or how closely they share the ports collection (it's been quite a
     while since I've really admin'd a freebsd box). For that matter, I
     wonder if this is also a problem on OS X. Can somebody with an OS X
     box try:

       $ /bin/sh
       $ eval 'false

         '
       $ echo $?

     It should print '1'; if it prints '0', the shell is broken.

-Peff

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

From: Tomas Carnecky <hidden>
Date: 2016-06-15 22:46:22

On Mar 10, 2009, at 7:17 PM, Jeff King wrote:
On Sun, Mar 08, 2009 at 12:45:55PM -0700, Junio C Hamano wrote:
quoted
If this part from your analysis is true for a shell:
quoted
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?
OK, good points. I was just hoping not to cause people on FreeBSD  
undue
pain. What is the best way to make such a declaration? I can think of:

1. A mention in the release notes.

2. A test in the Makefile similar to the $(:) test.

3. Getting in touch with the freebsd ports maintainer for git and
   suggesting a dependency on bash (and/or seeing if he wants to push
   through a fix for /bin/sh).

   I don't know if the same problem exists on other BSD-influenced  
systems,
   or how closely they share the ports collection (it's been quite a
   while since I've really admin'd a freebsd box). For that matter, I
   wonder if this is also a problem on OS X. Can somebody with an OS X
   box try:

     $ /bin/sh
     $ eval 'false

       '
     $ echo $?

   It should print '1'; if it prints '0', the shell is broken.
prints '1' here (10.5.6)

tom

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

From: Tomas Carnecky <hidden>
Date: 2016-06-15 22:46:22

On Mar 10, 2009, at 7:17 PM, Jeff King wrote:
On Sun, Mar 08, 2009 at 12:45:55PM -0700, Junio C Hamano wrote:
quoted
If this part from your analysis is true for a shell:
quoted
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?
OK, good points. I was just hoping not to cause people on FreeBSD  
undue
pain. What is the best way to make such a declaration? I can think of:

 1. A mention in the release notes.

 2. A test in the Makefile similar to the $(:) test.

 3. Getting in touch with the freebsd ports maintainer for git and
    suggesting a dependency on bash (and/or seeing if he wants to push
    through a fix for /bin/sh).

    I don't know if the same problem exists on other BSD-influenced  
systems,
    or how closely they share the ports collection (it's been quite a
    while since I've really admin'd a freebsd box). For that matter, I
    wonder if this is also a problem on OS X. Can somebody with an  
OS X
    box try:

      $ /bin/sh
      $ eval 'false

        '
      $ echo $?

    It should print '1'; if it prints '0', the shell is broken.
prints '1' here (10.5.6)

tom

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

From: Chris Johnsen <hidden>
Date: 2016-06-15 22:46:22

On 2009 Mar 10, at 13:17, Jeff King wrote:
Can somebody with an OS X box try:

  $ /bin/sh
  $ eval 'false

    '
  $ echo $?

It should print '1'; if it prints '0', the shell is broken.
I wrote t3505 on a Mac OS X 10.4.11 system. On that system, /bin/sh  
is a copy of bash v2.05b. Your test code prints 1 here.

-- 
Chris

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

From: Jeff King <hidden>
Date: 2016-06-15 22:46:22

On Tue, Mar 10, 2009 at 06:57:55PM -0500, Chris Johnsen wrote:
On 2009 Mar 10, at 13:17, Jeff King wrote:
quoted
Can somebody with an OS X box try:

  $ /bin/sh
  $ eval 'false

    '
  $ echo $?

It should print '1'; if it prints '0', the shell is broken.
I wrote t3505 on a Mac OS X 10.4.11 system. On that system, /bin/sh is a 
copy of bash v2.05b. Your test code prints 1 here.
OK, then nothing to worry about there. I have no idea which shell
OpenBSD and NetBSD use these days, and I don't have access to a box.
Anybody?

-Peff

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

From: Mike Ralphson <hidden>
Date: 2016-06-15 22:46:22

2009/3/11 Jeff King [off-list ref]:
OK, then nothing to worry about there. I have no idea which shell
OpenBSD and NetBSD use these days, and I don't have access to a box.
Anybody?
OpenBSD uses pdksh in Bourne shell mode for non-root shells (ksh mode
for root) [1].

NetBSD >=4 uses a Bourne shell but I don't know the exact provenance.
[2] "A sh command appeared in Version 1 AT&T UNIX.  It was, however,
unmaintainable so we wrote this one."

[1] http://www.openbsd.org/faq/faq10.html#ksh
[2] http://www.netbsd.org/docs/misc/index.html#shells

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

From: Mike Ralphson <hidden>
Date: 2016-06-15 22:46:22

2009/3/11 Mike Ralphson [off-list ref]
2009/3/11 Jeff King [off-list ref]:
quoted
OK, then nothing to worry about there. I have no idea which shell
OpenBSD and NetBSD use these days, and I don't have access to a box.
Anybody?
OpenBSD uses pdksh in Bourne shell mode for non-root shells (ksh mode
for root)
... and isn't broken in this instance (OpenBSD v4.1)

Weird test failures though, so now I'm looking at that 8-)

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

From: Jeff King <hidden>
Date: 2016-06-15 22:46:26

[this is a follow-up on the "eval 'false\n\n'" returns 0 issue on
FreeBSD]

On Wed, Mar 11, 2009 at 11:08:06AM +0000, Mike Ralphson wrote:
2009/3/11 Jeff King [off-list ref]:
quoted
OK, then nothing to worry about there. I have no idea which shell
OpenBSD and NetBSD use these days, and I don't have access to a box.
Anybody?
OpenBSD uses pdksh in Bourne shell mode for non-root shells (ksh mode
for root) [1].

NetBSD >=4 uses a Bourne shell but I don't know the exact provenance.
[2] "A sh command appeared in Version 1 AT&T UNIX.  It was, however,
unmaintainable so we wrote this one."

[1] http://www.openbsd.org/faq/faq10.html#ksh
[2] http://www.netbsd.org/docs/misc/index.html#shells
Thanks for looking this up, Mike. It sounds like FreeBSD is probably the
only problematic one. I confirmed that the problem still exists in
FreeBSD 7.1, and I've mailed the git ports maintainer off-list to
make him aware of the issue. So we'll see what happens.

Junio, do you want to put anything in the release notes warning people
who build from source that this is a potential issue? Do you want
something in the Makefile detecting that the shell is broken?

-Peff

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:27

Jeff King [off-list ref] writes:
[this is a follow-up on the "eval 'false\n\n'" returns 0 issue on
FreeBSD]
Thanks for keeping track of this one.
Thanks for looking this up, Mike. It sounds like FreeBSD is probably the
only problematic one. I confirmed that the problem still exists in
FreeBSD 7.1, and I've mailed the git ports maintainer off-list to
make him aware of the issue. So we'll see what happens.

Junio, do you want to put anything in the release notes warning people
who build from source that this is a potential issue? Do you want
something in the Makefile detecting that the shell is broken?
A sentence or two in INSTALL will not hurt.

I would not worry too much about the test scripts, but I would worry more
about getting phantom bug reports for our shell script Porcelains that get
hit by this.  Earlier I mentioned bisect is the only heavy user, but the
issue is more severe with filter-branch that is designed to eval end user
scripts (calls to 'eval "$filter_frotz"' check the exit status and die on
failure---with trailing blank lines the failure the filter reports will
not get caught).

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

From: Jeff King <hidden>
Date: 2016-06-15 22:46:27

On Sun, Mar 22, 2009 at 02:58:35PM -0700, Junio C Hamano wrote:
quoted
Junio, do you want to put anything in the release notes warning people
who build from source that this is a potential issue? Do you want
something in the Makefile detecting that the shell is broken?
A sentence or two in INSTALL will not hurt.

I would not worry too much about the test scripts, but I would worry more
about getting phantom bug reports for our shell script Porcelains that get
hit by this.  Earlier I mentioned bisect is the only heavy user, but the
issue is more severe with filter-branch that is designed to eval end user
scripts (calls to 'eval "$filter_frotz"' check the exit status and die on
failure---with trailing blank lines the failure the filter reports will
not get caught).
Agreed. The good news is that the /bin/sh people are treating it like a
bug:

  http://lists.freebsd.org/pipermail/freebsd-standards/2009-March/001721.html

so it will hopefully be fixed soon. It might still be worth warning
users of older releases in INSTALL, though.

-Peff
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help