Denton Liu <liu.denton@xxxxxxxxx> 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?
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.
