Thread (1 message) 1 message, 1 author, 2020-03-11

Re: [Breakage] t0410 - subtests report unable to remove non-existent file.

From: Junio C Hamano <hidden>
Date: 2020-03-11 17:51:36

SZEDER Gábor [off-list ref] writes:
  promise_and_delete () {
          HASH=$(git -C repo rev-parse "$1") &&
          <...>
          git -C repo rev-parse my_annotated_tag | pack_as_from_promisor &&
          <...>
          delete_object repo "$HASH"
  }

The failing 'rm' is in the 'delete_object_repo' helper function.
The 'pack_as_from_promisor' does the following:

  pack_as_from_promisor () {
          HASH=$(git -C repo pack-objects .git/objects/pack/pack) &&
          >repo/.git/objects/pack/pack-$HASH.promisor &&
          echo $HASH
  }

Notice that both 'promise_and_delete' and 'pack_as_from_promisor' set
the $HASH variable.  This is usually not an issue, because
'pack_as_from_promisor' is invoked in a pipe, and thus most shells
execute it in a subshell environment.

However, apparently 'ksh' doesn't run this helper function in a
subshell environment, and the value set in 'pack_as_from_promisor'
overwrites the value set in its caller, thus 'promise_and_delete' ends
up trying to delete a non-existing object (it's the SHA1 of a
packfile).
...
Note, however, that 'ksh' is not utterly wrong in doing so, because
POSIX does allow this behavior: POSIX 2.12 Shell Execution Environment,
the last paragraph of the section:

  "each command of a multi-command pipeline is in a subshell
   environment; as an extension, however, any or all commands in a
   pipeline may be executed in the current environment."

   https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_12

So apparently 'ksh' implements this extension.

The trivial fix would be to mark $HASH as 'local' in both helper
functions,
Or
	git -C repo rev-parse my_annotated_tag |
	(pack_as_from_promisor) &&

that would not require local?
but this would not help 'ksh', of course, as it doesn't
support 'local'.  However, since we use more and more 'local's in our
testsuite, 'ksh' might be considered a lost cause anyway.
That's somewhat sad, especially since "local" is outside POSIX, but
probably re-whipping our test suite into shape to be usable with
POSIX shells is too much effort for little benefit X-<.
Or we could rename these HASH variables to something more specific to
prevent this name collision, e.g. PACK_HASH in 'pack_as_from_promisor'.

Note that there are tests in t0410 that set and use the $HASH variable
outside of these helper function, and, worse, there is a test that uses
the $HASH variable set in the previous test.  Luckily, none of those
tests use 'promise_and_delete' or 'pack_as_from_promisor'.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help