| Message ID | 20181025001406.6729-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: cleanup for gcc 8.2.1 warning | expand |
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)
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.
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)
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)
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
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)
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(-)