Re: [PATCH v13 7/8] history: create squashed commits without editing

[Phillip Wood <phillip.wood123@gmail.com>]

On 13/08/2026 18:28, Junio C Hamano wrote:

 > "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
 >
 >> Inspired-by: Sergey Chernov <serega.morph@gmail.com>
 >> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
 >> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>

Thanks for the Helped-by: trailer, but this and several of the other 
patches are missing my sign off from the fixup patches I sent. My sign 
off should come above yours to reflect the chain of custody.

>> +test_expect_success '--no-edit uses last "amend!" message without an editor' '
>> +	git reset --hard three &&
>> +	write_script editor <<-\EOF &&
>> +	exit 1
>> +	EOF
>> +	test_set_editor "$(pwd)/editor" &&
>> +	echo fix >file &&
>> +	git commit --author="Fix Me <fix.me@example.com>" --fixup=HEAD -a &&
>> +	git commit --allow-empty -F - <<-EOF &&
>> +	amend! $(git rev-parse --short HEAD)
>> +
>> +	The first reword
>> +
>> +	More detail
>> +	EOF
>> +
>> +	git commit --allow-empty -F - <<-\EOF &&
>> +	amend! three
>> +
>> +	The second reword
>> +
>> +	Extra detail
>> +	EOF
>> +
>> +	test_commit WIP &&
>> +
>> +	cat >msg <<-EOF &&
>> +	amend! $(git rev-parse HEAD^ | tr a-f A-F)
>> +
>> +	The third reword
>> +
>> +	Excruciating detail
>> +	EOF
> 
> Care to explain why you need to (1) eat the exit status of the 'git
> rev-parse' command and (2) munge the commit object name by piping it
> into "tr"?

I chose to put the command substitution in the here doc, rather than 
move it out and use a variable because it was more convenient and it 
clearly shows what's going to be in the subject line. To me the benefits 
of that outweigh the minute risk that we'll miss a bug, that's not 
triggered elsewhere in the test suite, where "git rev-parse" produces 
the correct output but does not exit cleanly.

It's piped into "tr" to check that

	target = lookup_commit_reference_by_name(s);
	if (target && istarts_with(oid_to_hex(&target->object.oid), s))

uses istarts_with() so that we accept uppercase object ids.

> When another in-flight topic is in effect, this test will break due
> to "amend!" followed by a string that is *not* an object name, as
> the other topic declares that uppercase letters are not valid in a
> hexadecimal string.

I've just had a quick look at that other thread, it would be more 
convincing to me if there was some explanation of the security issues 
that accepting uppercase object ids causes. While I appreciate brian may 
not be able to talk about specific vulnerabilities in particular 
products, it would help to outline the security issues in general terms.

Thanks

Phillip