From: Phillip Wood <phillip.wood@talktalk.net>
Date: Wed, 01 Aug 2018 09:28:54 GMT
Subject: Re: [PATCH v2 1/2] sequencer: handle errors in read_author_ident()
Message-ID: <04f5f533-c971-6adc-9bcd-229e24ab2752@talktalk.net>
In-Reply-To: <CAPig+cT15a-QTTAjC61td4h_YhHz0WSa8iT_3XqUUc6LZHWC=A@mail.gmail.com>

Hi Eric

Thanks for taking a look at this

On 31/07/18 21:47, Eric Sunshine wrote:
> On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
>> The calling code treated NULL as a valid return value, so fix this by
>> returning and integer and passing in a parameter to receive the author.
> 
> It might be difficult for future readers (those who didn't follow the
> discussion) to understand how/why NULL is not sufficient to signal an
> error. Perhaps incorporating the explanation from your email[1] which
> discussed that the author name, email, and/or date might change
> unexpectedly would be sufficient. This excerpt from [1] might be a
> good starting point:
> 
>      ... the caller does not treat NULL as an error, so this will
>      change the date and potentially the author of the commit
>      ... [which] does corrupt the author data compared to its
>      expected value.
> 
> [1]: https://public-inbox.org/git/c80cf729-1bbe-10f5-6837-b074d371b91c@talktalk.net/
> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> diff --git a/sequencer.c b/sequencer.c
>> @@ -701,57 +701,58 @@ static char *get_author(const char *message)
>> -static const char *read_author_ident(struct strbuf *buf)
>> +static int read_author_ident(char **author)
> 
> So, the caller is now responsible for freeing the string placed in
> 'author'. Okay.
> 
>>   {
>> -       if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
>> -               return NULL;
>> +       if (strbuf_read_file(&buf, rebase_path_author_script(), 256) <= 0)
>> +               return -1;
> 
> I think you need to strbuf_release(&buf) in this error path since
> strbuf_read_file() doesn't guarantee that the strbuf hasn't been
> partially populated when it returns an error. (That is, this is
> leaking.)

Good point, I'll fix it

>>          /* dequote values and construct ident line in-place */
> 
> Ugh, this comment should have been adjusted in my series. A minor
> matter, though, which can be tweaked later.
> 
>>          /* validate date since fmt_ident() will die() on bad value */
>>          if (parse_date(val[2], &out)){
>> -               warning(_("invalid date format '%s' in '%s'"),
>> +               error(_("invalid date format '%s' in '%s'"),
>>                          val[2], rebase_path_author_script());
>>                  strbuf_release(&out);
>> -               return NULL;
>> +               strbuf_release(&buf);
>> +               return -1;
> 
> You were careful to print the error, which references a value from
> 'buf', before destroying 'buf'. Good.
> 
> (A simplifying alternative would have been to not print the actual
> value and instead say generally that "the date" was bad. Not a big
> deal.)
> 
>>          }
>> -       strbuf_swap(buf, &out);
>> -       strbuf_release(&out);
>> -       return buf->buf;
>> +       *author = strbuf_detach(&out, NULL);
> 
> And, 'author' is only assigned when 0 is returned, so the caller only
> has to free(author) upon success. Fine.
> 
>> +       strbuf_release(&buf);
>> +       return 0;
>>   }
>>
>>   static const char staged_changes_advice[] =
>> @@ -794,12 +795,14 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>> -               struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
>> -               const char *author = is_rebase_i(opts) ?
>> -                       read_author_ident(&script) : NULL;
>> +               struct strbuf msg = STRBUF_INIT;
>> +               char *author = NULL;
>>                  struct object_id root_commit, *cache_tree_oid;
>>                  int res = 0;
>>
>> +               if (is_rebase_i(opts) && read_author_ident(&author))
>> +                       return -1;
> 
> Logic looks correct, and it's nice to see that you went with 'return
> -1' rather than die(), especially since the caller of run_git_commit()
> is already able to handle -1.

Yes, it reschedules the pick so the user has a chance to fix the 
author-script and then run 'git rebase --continue'

Best Wishes

Phillip