Re: [GSoC][PATCH v4 15/20] rebase -i: rewrite write_basic_state() in C

On Mon, Jul 30, 2018 at 08:25:16PM +0200, SZEDER Gábor wrote:

quoted

diff --git a/sequencer.c b/sequencer.c
index 1c035ceec7..d257903db0 100644
--- a/sequencer.c
+++ b/sequencer.c

quoted

+int write_basic_state(struct replay_opts *opts, const char *head_name,
+		      const char *onto, const char *orig_head)
+{
+	const char *quiet = getenv("GIT_QUIET");
+
+	if (head_name)
+		write_file(rebase_path_head_name(), "%s\n", head_name);
+	if (onto)
+		write_file(rebase_path_onto(), "%s\n", onto);
+	if (orig_head)
+		write_file(rebase_path_orig_head(), "%s\n", orig_head);
+
+	if (quiet)
+		write_file(rebase_path_quiet(), "%s\n", quiet);
+	else
+		write_file(rebase_path_quiet(), "");

This is not a faithful conversion of the original.  git-rebase.sh writes
this 'quiet' file with:

  echo "$GIT_QUIET" > "$state_dir"/quiet

which means that a single newline character was written even when
$GIT_QUIET was unset/empty.

write_file() will call strbuf_complete_line(), so even passing "" will
result in a file with a newline in it (I didn't dig, but this comes from
e7ffa38c in 2015, so it may well have been a response to the breakage
you were thinking of).

So actually all of the "%s\n" here can be just "%s".

But there _is_ a reason not to use "", which is that it triggers
-Wformat-zero-length (which is part of -Wall unless you explicitly turn
it off, which our DEVELOPER=1 setup does). For a long time you _had_ to
do that, but these days we're actually clean with respect to that
warning.

So using "\n" here is better, and likewise here:

quoted

+	if (opts->verbose)
+		write_file(rebase_path_verbose(), "");

Unless we really do want a zero-length file, in which case:

  write_file_buf(path, "", 0);

is the right option. That matches the shell version, but I'd be
surprised if it mattered, since it's clearly meant to be "if this file
exists, we're verbose".

-Peff