Subject: Re: [PATCH v2 00/25] Prepare to separate out a packed_ref_store

From: Junio C Hamano <[email-masked]>
Date: Wed, 24 May 2017 11:45:34 +0900

Michael Haggerty <[email-masked]> writes:

> * Since v1, branch `bc/object-id` has been merged to `next`, and it
>   has lots of conflicts with these changes. So I rebased this branch
>   onto a merge of `master` and `bc/object-id`. (I hope this makes
>   Junio's job easier.) This unfortunately causes a bit of tbdiff noise
>   between v1 and v2.

Heh, that gives me an excellent excuse to procrastinate, as I am
planning to merge that topic to 'master' by the end of this week ;-)

Jokes aside...

> * Patch [01/25]: in t3600, register the `test_when_finished` command
>   before executing `chmod a-w`.
>
> * Patch [04/25] (new patch): convert a few `die("internal error: ...")`
>   to `die("BUG: ...")`.
>
> * Patch [05/25]: Use `strlen()` rather than `memchr()` to check the
>   trim length, and `die()` rather than skipping if it is longer than
>   the reference name.
>
> * Patch [08/25]: Name the log message arguments `msg` for consistency
>   with existing code.
>
> * Patch [10/25]: Rename the new member from `packlock` to
>   `packed_refs_lock`.
>
> * Patch [13/25] (new patch): Move the check for valid
>   `transaction->state` from `files_transaction_commit()` to
>   `ref_transaction_commit()`.

All of these feel familiar ;-)

> * Patch [14/25]:
>
>   * Add more sanity checks of `transaction->state`.
>
>   * Don't add `ref_transaction_finish()` to the public API. Instead,
>     teach `ref_transaction_commit()` to do the right thing whether or
>     not `ref_transaction_prepare()` has been called.
>
>   * Add and improve docstrings.
>
>   * Allow `ref_transaction_abort()` to be called even before
>     `ref_transaction_prepare()` (in which case it just calls
>     `ref_transaction_free()`).
>
> * Lots of improvements to commit messages and comments, mostly to
>   clarify points that reviewers asked about.

Overall this looked like quite a quality work to me.

> These changes (along with the merge commit that they are based on) are
> also available as branch `packed-ref-store-prep` in my GitHub fork
> [2]. If you'd like to see a preview of the rest of the changes (which
> works but is not yet polished), checkout the `mmap-packed-refs` branch
> from the same place.

Thanks.