patchsequencer: cleanup for gcc 8.2.1 warning

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(), "");

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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.

Junio C Hamano <gitster@pobox.com> writes:

It turns out that we have dealt with this before.

The trick employed by 7d7d6802 ("silence a bunch of format-zero-length warnings", 2014-05-04) is still a caller side workaround, but to do

	-	status_printf_ln(s, GIT_COLOR_NORMAL, "");
	+	status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");

to the function whose third parameter is printf format.

I do not know if it is a good idea to define a macro

	#define	EMPTY_CONTENTS	"%s", ""

in git-compat-util.h and then replace all the occurrences of "%s", "" in the source code with it. That way, we'd be able to create an empty file with

	write_file(FILENAME, EMPTY_CONTENTS);

and write out an empty line with

	status_printf_ln(s, GIT_COLOR_NORMAL, EMPTY_CONTENTS);

and they would read naturally. But may be it is a bit too cute an idea? I dunno.

On Wed, Oct 24, 2018 at 11:22 PM Junio C Hamano <gitster@pobox.com> wrote:

my first idea was to replace it with a helper called touch_file, since I was expecting it will be a popular operation as flag files are common in shell scripts and that name will be second nature to anyone porting them.

the fact that the quiet flag was created with a single '\n' in the code just immediately above this make me go for the proposed "solution" instead (which I verified wouldn't change behaviour as you described in your post; I apologize for not documenting it in the commit and wasting your time).

would something like this work better? (not to apply, and probably mangled)

Carlo Arenas <carenas@gmail.com> writes:

At least call it "create_empty_file(path)" instead.

"touch" is primarily to update the last-modified-time timestamp of a file. If the file does not exist, it is created while doing so, but when readers of your code sees a "touch", you make them anticipate that you are relying on file timestamp somehow. Using it to create an empty file wastes time of readers who read your code by forcing them wonder why you care about the file timestamp.

For a single-use, not using the macro and just using "%s", "" should suffice. If we want to hide the "%s", "" trickery from distracting the readers (which is what you are trying to address with your touch_file() proposal, I think, and I also suspect that it may be a legitimate one), I tend to think that it may be a better solution to introduce the EMPTY_PRINTF_ARG macro and use it everywhere, in builtin/commit.c, builtin/difftool.c and wt-status.c and not not just here.

On Thu, Oct 25, 2018 at 1:08 AM Junio C Hamano <gitster@pobox.com> wrote:

OK, will send it as v2 then but would think it will be better if applied as a "fixup" on top of the original branch: 34b47315d9 ("rebase -i: move rebase--helper modes to rebase--interactive", 2018-09-27)

would be a good idea to include also enabling this warning in developer mode so it doesn't sneak back?, presume as the last patch of the refactor below?, FWIW this is the only case in current pu, and I suspect the sooner we add it to next the less likely we will find more.

will work on this in a different feature branch, but I had to admit I don't like it for status_printf case where it was IMHO a hack to get the new lines to begin with.

I would think it would make more sense to call a function that says "write_ttycolor_ln" instead for those cases.

Carlo

as shown by:

  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(), "");

where write_file will create an empty file if told to write an empty string as can be inferred by the previous call

the somehow more convoluted syntax works around the issue by providing a non empty format string and is already being used for the abort safety file since 1e41229d96 ("sequencer: make sequencer abort safer", 2016-12-07)

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---

Notes:
    Changes in v2
    
     - Avoid change of behaviour as suggested by Junio

 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)