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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> > diff --git a/sequencer.c b/sequencer.c
> > index 1c035ceec7..d257903db0 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> 
> > +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:

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux