Re: [PATCH 2/3] add read_author_script() to libgit
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Sep 13, 2018, 23:49 UTC
- Message-ID
- <CAPig+cTGYWoW+YiW5tJVPZVie3zsG+pmQiV7k+es-=y9e4=RZw@mail.gmail.com>
- In-Reply-To
- <20180912101029.28052-3-phillip.wood@talktalk.net>
On Wed, Sep 12, 2018 at 6:11 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
Show 11 quoted lines
> Add read_author_script() to sequencer.c based on the implementation in
> builtin/am.c and update read_am_author_script() to use
> read_author_script(). The sequencer code that reads the author script
> will be updated in the next commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> diff --git a/builtin/am.c b/builtin/am.c
> @@ -305,39 +279,16 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)
> static int read_am_author_script(struct am_state *state)
> {This function returns 'int'...
> const char *filename = am_path(state, "author-script"); > + if (read_author_script(filename, &state->author_name, > + &state->author_email, &state->author_date, 1)) > + exit(128);
...but only ever exit()s...
> + return 0;
... or returns 0.
> }
It's a little disturbing that it now invokes exit() directly rather than calling die() since die() may involve extra functionality before actually exiting.
What if, instead of exit()ing directly, you drop the conditional and instead return the value of read_author_script():
return read_author_script(...);
and let the caller deal with the zero or non-zero return value as usual? (True, you'd get two error messages upon failure rather than just one, but that's not necessarily a bad thing.)
A possibly valid response is that such a change is outside the scope of this series since the original code shared this odd architecture of sometimes returning 0, sometimes -1, and sometimes die()ing.