Re: [PATCH 2/3] add read_author_script() to libgit

From: Eric Sunshine <sunshine@sunshineco.com>
To: Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Git List <git@vger.kernel.org>
Subject: Re: [PATCH 2/3] add read_author_script() to libgit
Date: Wed, 10 Oct 2018 06:22:23 -0400	[thread overview]
Message-ID: <CAPig+cQg2OGkdjug-hqYWmxVs4d701ytHoe3aKf9BU7bKU=HOg@mail.gmail.com> (raw)
In-Reply-To: <28505444-0798-6fda-6d1b-b9dbc0007123@talktalk.net>

On Wed, Oct 10, 2018 at 6:14 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> On 14/09/2018 00:49, Eric Sunshine wrote:
> > 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.
>
> My aim was to try and to keep the changes to a minimum as this patch
> isn't about changing the odd architecture of the original. I could add a
> follow up patch that cleans things up as you suggest.

The code already had that weird mix of return(s) and die(), hence the
state is no worse after this patch than before. So, a cleanup patch
later in the series, a follow up series, or doing nothing at all are
all reasonable approaches. I don't insist on it for this patch series.
Thanks.