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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2020, 17:51 UTC
Message-ID: <xmqq5zfaixun.fsf@gitster.c.googlers.com>
In-Reply-To: <20200310110008.GA3122@szeder.dev>

SZEDER Gábor <szeder.dev@gmail.com> writes:

Show 42 quoted lines

>   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-<.

Show 7 quoted lines

> 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'.