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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2018, 08:08 UTC
Message-ID: <xmqqwoq6ih34.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPUEsphHsWVzdGXBB0EOSNuUb5gNgjyJ+i7PnDJx7LBqejQrNQ@mail.gmail.com>

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.

Show 21 quoted lines

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