Re: [PATCH v4 04/23] sequencer: reuse strbuf_trim() in read_oneliner()

[Denton Liu <liu.denton@gmail.com>]

Hi Junio,

On Sun, Apr 05, 2020 at 02:46:47PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > @@ -2471,7 +2467,6 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
> >  
> >  static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
> >  {
> > -	strbuf_reset(buf);
> >  	if (!read_oneliner(buf, rebase_path_strategy(), 0))
> >  		return;
> >  	opts->strategy = strbuf_detach(buf, NULL);
> > @@ -2494,7 +2489,6 @@ static int read_populate_opts(struct replay_opts *opts)
> >  				free(opts->gpg_sign);
> >  				opts->gpg_sign = xstrdup(buf.buf + 2);
> >  			}
> > -			strbuf_reset(&buf);
> >  		}
> >  
> >  		if (read_oneliner(&buf, rebase_path_allow_rerere_autoupdate(), 1)) {
> > @@ -2526,7 +2520,6 @@ static int read_populate_opts(struct replay_opts *opts)
> >  			opts->keep_redundant_commits = 1;
> >  
> >  		read_strategy_opts(opts, &buf);
> > -		strbuf_reset(&buf);
> >  
> 
> >  		if (read_oneliner(&opts->current_fixups,
> >  				  rebase_path_current_fixups(), 1)) {
> 
> Is this conversion correct around here?  read_oneliner() used to
> "append" what was read from the file to what is already in the
> strbuf, and many strbuf_reset() in this function was because these
> callers of read_oneliner() in this function that has strbuf_reset()
> immediately before did *not* want the "append" semantics.  But this
> one looks different.  Where in the original does the current_fixups
> strbuf get emptied for this read_oneliner() to ignore the previous
> contents?  Or are we relying on the caller not to have done anything
> to current_fixups before it calls this function?

As far as I can tell, opts->current_fixups is always empty when
read_oneliner() is called here.

> In other words, the original behaviour of read_oneliner() having the
> "append" semantics suggests me that there were callers that wanted
> to keep the current contents and append---this current_fixups may
> not be one of them, but nevertheless, changing the semantics of the
> function from "append" to "discard and read from scratch" without
> vetting all the existing callers smells iffy to me.

Before making this change, I manually checked all invocations of
read_oneliner() and ensured that they all passed in an empty strbuf.
Same thing with "rebase: use read_oneliner()", I manually checked all of
those invocations as well. It's quite possible that I made a mistake,
though.

To be doubly sure that I caught everything, I ran the test suite on
'master' with this patch:

	diff --git a/builtin/rebase.c b/builtin/rebase.c
	index 27a07d4e78..a0c03dd1d6 100644
	--- a/builtin/rebase.c
	+++ b/builtin/rebase.c
	@@ -589,6 +589,9 @@ static const char *state_dir_path(const char *filename, struct rebase_options *o
	 /* Read one file, then strip line endings */
	 static int read_one(const char *path, struct strbuf *buf)
	 {
	+	if (buf->len)
	+		BUG("rebase change bad: %s", buf->buf);
	+
		if (strbuf_read_file(buf, path, 0) < 0)
			return error_errno(_("could not read '%s'"), path);
		strbuf_trim_trailing_newline(buf);
	diff --git a/sequencer.c b/sequencer.c
	index 6fd2674632..d7bc5c9c95 100644
	--- a/sequencer.c
	+++ b/sequencer.c
	@@ -433,6 +433,9 @@ static int read_oneliner(struct strbuf *buf,
	 {
		int orig_len = buf->len;
	 
	+	if (buf->len)
	+		BUG("sequencer change bad: %s", buf->buf);
	+
		if (!file_exists(path))
			return 0;

Thanks,

Denton