patchrebase --skip: fix commit message clean up when skipping squash

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

> 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(-)

> 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;
>  			}
>  		}

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

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

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

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> During a series of "fixup" and/or "squash" commands, the interactive
> rebase accumulates a commit message from all the commits that are being
> squashed together. If one of the commits has conflicts when it is picked
> and the user chooses to skip that commit then we need to remove that
> commit's message from accumulated messages.  To do this 15ef69314d5
> (rebase --skip: clean up commit message after a failed fixup/squash,
> 2018-04-27) updated commit_staged_changes() to reset the accumulated
> message to the commit message of HEAD (which does not contain the
> message from the skipped commit) when the last command was "fixup" or
> "squash" and there are no staged changes. Unfortunately the code to do
> this contains two bugs.
> 
> (1) If parse_head() fails we pass an invalid pointer to
>      unuse_commit_buffer().
> 
> (2) The reconstructed message uses the entire commit buffer from HEAD
>      including the headers, rather than just the commit message.
> 
> The fist issue is fixed by splitting up the "if" condition into several

>> The fist issue is fixed by splitting up the "if" condition into several
> 
> s/fist/first/
> 
> (I'm not qualified to review anything else about this patch, but I like 
> its intention...  :D )