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

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