Re: [PATCH] rebase --skip: fix commit message clean up when skipping squash

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

On 03/08/2023 21:20, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> ... The test is also updated to explicitly check
>> the commit messages rather than relying on grep to ensure they do not
>> contain any stay commit headers.
> 
> "stay" -> "stray" presumably.

I see this is in next already, thanks for fixing that and the other typo 
pointed out by Marc

>> To check the commit message the function test_commit_message() is moved
>> from t3437-rebase-fixup-options.sh to test-lib.sh. As the function is
>> now publicly available it is updated to provide better error detection
>> and avoid overwriting the commonly used files "actual" and "expect".
>> Support for reading the expected commit message from stdin is also
>> added.
> 
> It may make it cleaner to do the refactoring as a separate
> preparatory patch, but the end-result is not so large from
> the diffstat below, so it probably is OK.

I was in two minds about splitting out the refactoring, but in the end I 
decided that as I was adding the ability to read the expected message 
from stdin for this commit it was easier include it all here rather than 
splitting it out.

> I am not sure if deviating from expect vs actual is such a good
> idea.  It is not like use of two temporary files are transparent to
> the caller of the new test helper---indeed, expect and actual are
> likely to be used by the caller in tests that comes before or after
> the ones that use test_commit_message, and by using a pair of files
> that are different, the caller will now see two extra untracked
> files left in the working tree.

That's true, looking at test-lib-functions.sh it seems I unwittingly 
followed the example of test_cmp_config() which uses 
{actual,expect}.config when it calls test_cmp(). Anyway as this is in 
next I assume you're happy enough with the implementation as it stands.

Best Wishes

Phillip

> The only case such a renaming could help callers is when they do
> 
> 	cat >expect <<\-EOF &&
> 	here to prepare some outcome
> 	EOF
> 	git do-something-to-make-commit &&
> 	test_commit_message HEAD <<\-EOF &&
> 	here is what we expect to see in HEAD
> 	EOF
> 	git some-other-thing >actual &&
> 	test_cmp expect actual
> 	
> as use of files other than expect/actual in test_commit_message will
> avoid stomping on the "expect" file that was already prepared.
> 
> I suspect that it would be rare, and something we can fix when need
> arises by allowing test_commit_message to accept an option to use
> non-standard filenames for its temporaries.  The current callers,
> both the existing ones in t3437 and the new ones added by this
> patch, would not benefit.  The only externally visible side effect
> is that the existing ones will have two extra untracked files in
> their working tree.
> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>      rebase --skip: fix commit message clean up when skipping squash
>>      
>>      This patch is based on maint.
>> ...
>>   sequencer.c                     | 26 +++++++++++----
>>   t/t3418-rebase-continue.sh      | 58 +++++++++++++++++++++++----------
>>   t/t3437-rebase-fixup-options.sh | 15 ---------
>>   t/test-lib-functions.sh         | 33 +++++++++++++++++++
>>   4 files changed, 93 insertions(+), 39 deletions(-)
> 
> OK.
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index bceb6abcb6c..af271ab6fbd 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -5038,19 +5038,31 @@ static int commit_staged_changes(struct repository *r,
>>   				 * We need to update the squash message to skip
>>   				 * the latest commit message.
>>   				 */
>> +				int res = 0;
>>   				struct commit *commit;
>> +				const char *msg;
>>   				const char *path = rebase_path_squash_msg();
>>   				const char *encoding = get_commit_output_encoding();
>>   
>> -				if (parse_head(r, &commit) ||
>> -				    !(p = repo_logmsg_reencode(r, commit, NULL, encoding)) ||
>> -				    write_message(p, strlen(p), path, 0)) {
>> -					repo_unuse_commit_buffer(r, commit, p);
>> -					return error(_("could not write file: "
>> +				if (parse_head(r, &commit))
>> +					return error(_("could not parse HEAD"));
>> +
>> +				p = repo_logmsg_reencode(r, commit, NULL, encoding);
>> +				if (!p)  {
>> +					res = error(_("could not parse commit %s"),
>> +						    oid_to_hex(&commit->object.oid));
>> +					goto unuse_commit_buffer;
>> +				}
>> +				find_commit_subject(p, &msg);
>> +				if (write_message(msg, strlen(msg), path, 0)) {
>> +					res = error(_("could not write file: "
>>   						       "'%s'"), path);
>> +					goto unuse_commit_buffer;
>>   				}
>> -				repo_unuse_commit_buffer(r,
>> -							 commit, p);
>> +			unuse_commit_buffer:
>> +				repo_unuse_commit_buffer(r, commit, p);
>> +				if (res)
>> +					return res;
>>   			}
>>   		}
> 
> Just as described in the proposed log message.  Looking good.
> 
> Will queue.  Thanks.