Problems with t6011

Problems with t6011

[Torsten Bögershausen]

Hej Patrick,
in case you have a second:
the mv command here needs a "-f" to overwrite
read-only files:

--- a/t/t6011-rev-list-with-bad-commit.sh
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -39,7 +39,7 @@ test_expect_success 'corrupt second commit object' '
         for p in .git/objects/pack/*.pack
         do
                 sed "s/second commit/socond commit/" "$p" >"$p.munged" &&
-               mv "$p.munged" "$p" ||
+               mv -f "$p.munged" "$p" ||
                 return 1

Re: Problems with t6011

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Hej Patrick,
> in case you have a second:
> the mv command here needs a "-f" to overwrite
> read-only files:
>
> --- a/t/t6011-rev-list-with-bad-commit.sh
> +++ b/t/t6011-rev-list-with-bad-commit.sh
> @@ -39,7 +39,7 @@ test_expect_success 'corrupt second commit object' '
>         for p in .git/objects/pack/*.pack
>         do
>                 sed "s/second commit/socond commit/" "$p" >"$p.munged" &&
> -               mv "$p.munged" "$p" ||
> +               mv -f "$p.munged" "$p" ||
>                 return 1

Looking at the remainder of cdbdc6bf (t: refactor tests depending on
Perl substitution operator, 2025-04-03), the commit that introduced
these lines, it seems that the prevailing pattern was:

	chmod +w "$packfile" &&
	perl -pe "regexp" "$packfile" >"$packfile.munged" &&
	mv "packfile.munged" $packfile"

but the original for this loop used "perl -i.bak -pe" that dealt
with read-only input just fine, wihtout the need for a separate
"mv".

So the lack of "-f" indeed is a bug in that "refactor" commit.

I would be more worried about using "sed" on clearly non-text files,
which technically is undefined operation, and I strongly suspect
that it was the reason why we used Perl to munge files that are
clearly binary, like the packfiles, in the first place.

[PATCH] t6011: fix misconversion from perl to sed

[Junio C Hamano]

No, this is not about a quiz on regexp compatibility between Perl
and sed.

Back when cdbdc6bf (t: refactor tests depending on Perl substitution
operator, 2025-04-03) rewrite many use of perl with sed, the general
pattern of the original scripts were

    chmod +w some_read_only_file &&
    perl -p -e "regexp to munge" some_read_only_file >some_tmp &&
    mv some_tmp some_read_only_file

persumably because the author new replacing some_read_only_file with
"mv" at the last step would not work without "mv -f" in some
environments (GNU does not seem to give any prompt when not running
interactively, which is what happens when running t/ scripts).
Replacing perl with sed would be fine as long as sed with updated
regexp does the equivalent munging.

But one place used to use a different construct in the original:

    perl -i.bak -p -e "regexp to munge" some_read_only_file

With _no_ temporary file or "mv", "perl -i" allows you to replace a
read-only file in place.

When we replaced the use of "perl" with "sed" in the said commit,
however, because "sed -i" is not portable, we rewrote that in-place
replacement to

    sed "regexp to munge" some_read_only_file >some_tmp &&
    mv some_tmp some_read_only_file

Again, unfortunately that does not work in some environment, without
"mv -f".

We could run "mv -f" here, but we would then need to remove "chmod
+w" and have them use "mv -f" instead at all places that were
touched cdbdc6bf (t: refactor tests depending on Perl substitution
operator, 2025-04-03) to be consistent (and more concise).

For now, let's make it consistent in the other direction by mimick
the other places that made the target read-write before moving.

Speaking of portability, the outcome of using "sed" on non-text
files is unspecified, so the entire exercise of cdbdc6bf may have
needed to be reverted if people still used ancient version of
"standard compliant" sed that barfs on non-text files, but these
days we may be able to get away with "BSDs and GNU seem OK with it"
;-)  But one fix at a time.

Reported-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 t/t6011-rev-list-with-bad-commit.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
index b6f3344dbf..1dd1e50d21 100755
--- a/t/t6011-rev-list-with-bad-commit.sh
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -38,6 +38,7 @@ test_expect_success 'verify number of revisions' \
 test_expect_success 'corrupt second commit object' '
 	for p in .git/objects/pack/*.pack
 	do
+		chmod +w "$p" &&
 		sed "s/second commit/socond commit/" "$p" >"$p.munged" &&
 		mv "$p.munged" "$p" ||
 		return 1
-- 
2.49.0-615-gd1e3f1cce9

Re: [PATCH] t6011: fix misconversion from perl to sed

[Patrick Steinhardt]

On Tue, May 06, 2025 at 03:48:55PM -0700, Junio C Hamano wrote:
> No, this is not about a quiz on regexp compatibility between Perl
> and sed.
> 
> Back when cdbdc6bf (t: refactor tests depending on Perl substitution
> operator, 2025-04-03) rewrite many use of perl with sed, the general

s/rewrite many use/rewrote many uses/

> pattern of the original scripts were
> 
>     chmod +w some_read_only_file &&
>     perl -p -e "regexp to munge" some_read_only_file >some_tmp &&
>     mv some_tmp some_read_only_file
> 
> persumably because the author new replacing some_read_only_file with

s/new/knew?

> diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
> index b6f3344dbf..1dd1e50d21 100755
> --- a/t/t6011-rev-list-with-bad-commit.sh
> +++ b/t/t6011-rev-list-with-bad-commit.sh
> @@ -38,6 +38,7 @@ test_expect_success 'verify number of revisions' \
>  test_expect_success 'corrupt second commit object' '
>  	for p in .git/objects/pack/*.pack
>  	do
> +		chmod +w "$p" &&
>  		sed "s/second commit/socond commit/" "$p" >"$p.munged" &&
>  		mv "$p.munged" "$p" ||
>  		return 1

Ok, the fix makes sense. Thanks!

Patrick

Re: [PATCH] t6011: fix misconversion from perl to sed

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, May 06, 2025 at 03:48:55PM -0700, Junio C Hamano wrote:
>> No, this is not about a quiz on regexp compatibility between Perl
>> and sed.
>> 
>> Back when cdbdc6bf (t: refactor tests depending on Perl substitution
>> operator, 2025-04-03) rewrite many use of perl with sed, the general
>
> s/rewrite many use/rewrote many uses/
>
>> pattern of the original scripts were
>> 
>>     chmod +w some_read_only_file &&
>>     perl -p -e "regexp to munge" some_read_only_file >some_tmp &&
>>     mv some_tmp some_read_only_file
>> 
>> persumably because the author new replacing some_read_only_file with
>
> s/new/knew?
>
>> diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
>> index b6f3344dbf..1dd1e50d21 100755
>> --- a/t/t6011-rev-list-with-bad-commit.sh
>> +++ b/t/t6011-rev-list-with-bad-commit.sh
>> @@ -38,6 +38,7 @@ test_expect_success 'verify number of revisions' \
>>  test_expect_success 'corrupt second commit object' '
>>  	for p in .git/objects/pack/*.pack
>>  	do
>> +		chmod +w "$p" &&
>>  		sed "s/second commit/socond commit/" "$p" >"$p.munged" &&
>>  		mv "$p.munged" "$p" ||
>>  		return 1
>
> Ok, the fix makes sense. Thanks!
>
> Patrick

Thanks for typofixes and sanity checking.