From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 06 Apr 2020 14:42:59 GMT
Subject: Re: [PATCH v4 04/23] sequencer: reuse strbuf_trim() in read_oneliner()
Message-ID: <1c382358-08ca-8bf8-cbd6-d00406472bcd@gmail.com>
In-Reply-To: <4e04fd90-bb71-0d7e-e611-5c408161d3e5@gmail.com>

Hi Denton

On 06/04/2020 15:03, Phillip Wood wrote:
> Hi Denton
> 
> On 04/04/2020 02:11, Denton Liu wrote:
>> In the original read_oneliner() logic, we duplicated the logic for
>> strbuf_trim_trailing_newline() with one exception: instead of preventing
>> buffer accesses below index 0, it would prevent buffer accesses below
>> index `orig_len`. Although this is correct, it isn't worth having the
>> duplicated logic around.
>>
>> Reset `buf` before using it then replace the trimming logic with
>> strbuf_trim().
> 
> I should have picked up on this before but this changes the semantics of 
> the function as it strips all whitespace from the start and end of the 
> strbuf. Above you talked about using strbuf_trim_trailing_newline() 
> instead which would not change the semantics of this function. You could 
> test to see if we've read anything and only call 
> strbuf_trim_trailing_newline() in that case without messing with 
> strbuf_reset(). (there is a corner case where if the buffer ends with 
> '\r' when the function is called and it reads a single '\n' then the 
> '\r' would be stripped as well but I think that is unlikely to happen in 
> the wild)

I'd also be fine with dropping this patch and leaving the trimming code 
as is

Best Wishes

Phillip

> Best Wishes
> 
> Phillip
> 
>> As a cleanup, remove all reset_strbuf()s that happen before
>> read_oneliner() is called.
>>
>> Signed-off-by: Denton Liu <liu.denton@gmail.com>
>> ---
>>   sequencer.c | 18 +++++-------------
>>   1 file changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index faab0b13e8..09ca68f540 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -420,8 +420,8 @@ static int write_message(const void *buf, size_t 
>> len, const char *filename,
>>   }
>>   /*
>> - * Reads a file that was presumably written by a shell script, i.e. 
>> with an
>> - * end-of-line marker that needs to be stripped.
>> + * Resets a strbuf then reads a file that was presumably written by a 
>> shell
>> + * script, i.e. with an end-of-line marker that needs to be stripped.
>>    *
>>    * Note that only the last end-of-line marker is stripped, 
>> consistent with the
>>    * behavior of "$(cat path)" in a shell script.
>> @@ -431,23 +431,19 @@ static int write_message(const void *buf, size_t 
>> len, const char *filename,
>>   static int read_oneliner(struct strbuf *buf,
>>       const char *path, int skip_if_empty)
>>   {
>> -    int orig_len = buf->len;
>>       if (!file_exists(path))
>>           return 0;
>> +    strbuf_reset(buf);
>>       if (strbuf_read_file(buf, path, 0) < 0) {
>>           warning_errno(_("could not read '%s'"), path);
>>           return 0;
>>       }
>> -    if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
>> -        if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r')
>> -            --buf->len;
>> -        buf->buf[buf->len] = '\0';
>> -    }
>> +    strbuf_trim(buf);
>> -    if (skip_if_empty && buf->len == orig_len)
>> +    if (skip_if_empty && !buf->len)
>>           return 0;
>>       return 1;
>> @@ -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)) {
>> @@ -4006,7 +3999,6 @@ static int pick_commits(struct repository *r,
>>                   res = error(_("could not read orig-head"));
>>                   goto cleanup_head_ref;
>>               }
>> -            strbuf_reset(&buf);
>>               if (!read_oneliner(&buf, rebase_path_onto(), 0)) {
>>                   res = error(_("could not read 'onto'"));
>>                   goto cleanup_head_ref;
>>