SZEDER Gábor <szeder.dev@xxxxxxxxx> 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'.
