Re: [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 13, 2021, 18:43 UTC
Message-ID: <X/8/WassxF7ujqjX@nand.local>
In-Reply-To: <20210108092345.2178-2-charvi077@gmail.com>

On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:

Show 32 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup
> commands, there's no point in writing it for squash commands as it is
> immediately deleted.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>  sequencer.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 8909a46770..f888a7ed3b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r,
>  			return error(_("could not read HEAD's commit message"));
>
>  		find_commit_subject(head_message, &body);
> -		if (write_message(body, strlen(body),
> -				  rebase_path_fixup_msg(), 0)) {
> -			unuse_commit_buffer(head_commit, head_message);
> -			return error(_("cannot write '%s'"),
> -				     rebase_path_fixup_msg());
> +		if (command == TODO_FIXUP) {
> +			if (write_message(body, strlen(body),
> +					  rebase_path_fixup_msg(), 0)) {
> +				unuse_commit_buffer(head_commit, head_message);
> +				return error(_("cannot write '%s'"),
> +					     rebase_path_fixup_msg());
> +			}

I'm nit-picking here, but would this be clearer instead as:

    if (command == TODO_FIXUP && write_message(...) < 0) {
      unuse_commit_buffer(...);
      // ...
    }

There are two changes there. One is two squash the two if-statements together, and the latter is to add a check that 'write_message()' returns an error. This explicit '< 0' checking was discussed recently in another thread[1], and I think makes the conditional here read more clearly.

Thanks, Taylor

[1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/