Re: [PATCH v4 04/23] sequencer: reuse strbuf_trim() in read_oneliner()
- From
Denton Liu <liu.denton@gmail.com>
- Date
- Apr 6, 2020, 01:39 UTC
- Message-ID
- <20200406013956.GA1879124@generichostname>
- In-Reply-To
- <xmqqr1x1wqpk.fsf@gitster.c.googlers.com>
Hi Junio,
On Sun, Apr 05, 2020 at 02:46:47PM -0700, Junio C Hamano wrote:
Show 37 quoted lines
> 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.
Show 6 quoted lines
> 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