Re: [PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status

From: Ronnie Sahlberg <sahlberg@google.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status
Date: Thu, 15 May 2014 09:06:50 -0700	[thread overview]
Message-ID: <CAL=YDW=e1FdVn5cHvRHPQrA8ywPne-uZk_iYNO4EmxJEA=fHSw@mail.gmail.com> (raw)
In-Reply-To: <20140514234015.GE9218@google.com>

On Wed, May 14, 2014 at 4:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> Update ref_transaction_update() do some basic error checking and return
>> true on error. Update all callers to check ref_transaction_update() for error.
>> There are currently no conditions in _update that will return error but there
>> will be in the future.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/update-ref.c | 10 ++++++----
>>  refs.c               |  9 +++++++--
>>  refs.h               | 10 +++++-----
>>  3 files changed, 18 insertions(+), 11 deletions(-)
>
> Revisiting comments from [1]:
>
>  * When I call ref_transaction_update, what does it mean that I get
>    a nonzero return value?  Does it mean the _update failed and had
>    no effect?  What will I want to do next: should I try again or
>    print an error and exit?

It means the transaction will no longer work and must be rolled back.
See below for the updated text I added to refs.h

>
>    Ideally I should be able to answer these questions by reading
>    the signature of ref_transaction_update and the comment documenting
>    it.  The comment doesn't say anything about what errors
>    mean here.

I have updated the description to include :
 * Function returns 0 on success and non-zero on failure. A failure to update
 * means that the transaction as a whole has failed and will need to be
 * rolled back.

>
>  * the error message change for the have_old && !old_sha1 case (to add
>    "BUG:" so users know the impossible has happened and translators
>    know not to bother with it) seems to have snuck ahead into patch 28
>    (refs.c: add transaction.status and track OPEN/CLOSED/ERROR).

Done.

>
>  * It would be easier to make sense of the error path (does the error
>    message have enough information?  Will the user be bewildered?)
>    if there were an example of how ref_transaction_update can fail.
>
>    There still doesn't seem to be one by the end of the series.

This patch series got a lot longer than I initially thought so I did
not get to the point where we it would make sense
to start returning !0. :-(

The next patchseries I sent out for review does add things to _update
that will cause it to return failures.
For example, locking the ref there happens in _update instead of
_commit and then it starts make sense to
return failures back to the caller for things such as "Multiple
updates for ref '%s' not allowed."

Unfortunate but since this patch series reached >40 patches I did not
want to continue expanding on it.
This means that actually starting to use "let _update return error"
did not actually start becomming used until the second
patch series, which now is well over 30 patches in size :-(

I just felt I had to stop growing this series or it would never be finished.

>
> The general idea still seems sensible.
>
> Thanks,
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/246437/focus=247115