Re: [PATCH v11 00/41] Use ref transactions

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Ronnie Sahlberg wrote:

>                                                                        It
> converts all ref updates, inside refs.c as well as external, to use the
> transaction API for updates. This makes most of the ref updates to become
> atomic when there are failures locking or writing to a ref.

I'm still excited about this series.  Most of it looks ready.  The
remaining problems I see fall into a few categories:

 * The rename_ref codepath is strange.  You've convinced me that the
   approach you're taking there is not much of a regression, but it's
   confusing enough that I'd be happier if someone else takes a closer
   look (or I can try to find time to).

 * I think the approach taken in the patch "add transaction.status and
   track OPEN/CLOSED/ERROR" is a mistake and would make callers more
   error-prone.  The basic idea of checking that the caller is using
   the API right is valuable, so there is something in that patch I
   really like --- it's just the details (involving the same kind of
   easy-to-clobber error messages as errno provides with stdio) that
   seem broken.

   I suspect I'm just not communicating very well there.  Maybe
   mhagger or someone else could give it a sanity check.

 * Some commit messages (e.g., the one to "pack all refs before we
   start to rename a ref") are confusing.  That might be a sign that
   what those patches are trying to do is confusing.

 * The error handling in "add an err argument to repack_without_refs"
   is a thorny thicket.  It still has bugs.  I can completely
   understand not wanting to take that on but I think it is important
   to add a NEEDSWORK comment describing the known bugs to help people
   when they work with this code in the future.

I realize the process of addressing review comments in such a long
series has been a bit of a pain, and if there's anything I can do to
make it easier please let me know.  Hopefully adding some other
reviewers can help.

Thanks,
Jonathan