Carlo Marcelo Arenas Belón <care...@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.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <care...@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)
