Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2018, 05:36 UTC
Message-ID: <xmqqlg6mk2pd.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181025001406.6729-1-carenas@gmail.com>

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> sequencer.c: In function ‘write_basic_state’:
> sequencer.c:2392:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]
>    write_file(rebase_path_verbose(), "");

The change may squelch the above warning, but doesn't it change the behaviour? You need to explain what the changed behaviour is and why it is OK to change that behaviour in this space, in addition to show what warning you are working around.

I _think_ the intent of this code is to create an empty file, because that is what the original version of "git rebase" did. write_file() does use strbuf_complete_line() to avoid creating a file with incomplete last line, but it does allow us to create an empty file. By adding a LF, the created file is no longer an empty one.

Does it matter? IOW, does it cause a regression if we start adding an LF to the file?

By reading master:git-rebase.sh::read_basic_state(), I _think_ it is safe to do so, as the side that consumes this $state_dir/verbose only checks file's existence, and does not care what it contains, even if somehow the scripted version of "git rebase" ends up reading the state file created by this newer version of "git rebase" in C. Also next:sequencer.c::read_populate_opts() only checks for file's existence, so the newer version of "git rebase" is also safe.

So as an immediate workaround for 'next', this patch happens to be OK, but that is only true because the consumer happens to ignore the distinction between an empty file and a file with a single LF in it.

I'd have to say that the ability to create an empty file is more important in the longer term. Can't the workaround be done to write_file() instead? I actually do not mind if the solution were to introduce a newhelper "write_empty_file()", but the way it is written in the code before this patch, i.e.

	write_file(FILENAME, "")

is so obvious a way to create an empty file, so if we do not have to resort to such a hackery to special case an empty file, that would be preferrable.

Thanks.

Show 19 quoted lines

>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 8dd6db5a01..358e83bf6b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2335,7 +2335,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
>  		write_file(rebase_path_quiet(), "\n");
>  
>  	if (opts->verbose)
> -		write_file(rebase_path_verbose(), "");
> +		write_file(rebase_path_verbose(), "\n");
>  	if (opts->strategy)
>  		write_file(rebase_path_strategy(), "%s\n", opts->strategy);
>  	if (opts->xopts_nr > 0)