[PATCH] sequencer: cleanup for gcc 8.2.1 warning

[PATCH] sequencer: cleanup for gcc 8.2.1 warning

[Carlo Marcelo Arenas Belón]

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(-)

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)
-- 
2.19.1

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

[Junio C Hamano]

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.

>
> 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)

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

[Junio C Hamano]

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

> 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.

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.


	

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

[Carlo Arenas]

On Wed, Oct 24, 2018 at 11:22 PM Junio C Hamano <gitster@pobox.com> wrote:
> and they would read naturally.  But may be it is a bit too cute an
> idea?  I dunno.

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)

--- a/sequencer.c
+++ b/sequencer.c
@@ -35,6 +35,8 @@

 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"

+#define touch_file(path) write_file(path, "%s", "")
+
 const char sign_off_header[] = "Signed-off-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";

@@ -2389,7 +2391,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(), "");
+               touch_file(rebase_path_verbose());
        if (opts->strategy)
                write_file(rebase_path_strategy(), "%s\n", opts->strategy);
        if (opts->xopts_nr > 0)

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

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

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

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.

> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -35,6 +35,8 @@
>
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> +#define touch_file(path) write_file(path, "%s", "")
> +
>  const char sign_off_header[] = "Signed-off-by: ";
>  static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>
> @@ -2389,7 +2391,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(), "");
> +               touch_file(rebase_path_verbose());
>         if (opts->strategy)
>                 write_file(rebase_path_strategy(), "%s\n", opts->strategy);
>         if (opts->xopts_nr > 0)

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

[Carlo Arenas]

On Thu, Oct 25, 2018 at 1:08 AM Junio C Hamano <gitster@pobox.com> wrote:
> For a single-use, not using the macro and just using "%s", "" should
> suffice.

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.

> 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.

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

[PATCH v2] sequencer: cleanup for gcc warning in non developer mode

[Carlo Marcelo Arenas Belón]

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(-)

diff --git a/sequencer.c b/sequencer.c
index 8dd6db5a01..10f602c4d4 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(), "%s", "");
 	if (opts->strategy)
 		write_file(rebase_path_strategy(), "%s\n", opts->strategy);
 	if (opts->xopts_nr > 0)
-- 
2.19.1