Re: [PATCH v2 00/13] Use ref transactions from most callers

[Junio C Hamano <gitster@pobox.com>]

Ronnie Sahlberg <sahlberg@google.com> writes:

> This patch series changes most of the places where the ref functions for
> locking and writing refs to instead use the new ref transaction API. There
> are still three more places where write_ref_sha1() is called from outside
> of refs.c but those all will require more complex work and review so those
> changes belong in a different patch series.
>
> Version 2:
>  - Add a patch to ref_transaction_commit to make it honor onerr even if the
>    error triggered in ref_Transaction_commit itself rather than in a call
>    to other functions (that already honor onerr).
>  - Add a patch to make the update_ref() helper function use transactions
>    internally.
>  - Change ref_transaction_update to die() instead of error() if we pass
>    if a NULL old_sha1 but have have_old == true.
>  - Change ref_transaction_create to die() instead of error() if new_sha1
>    is false but we pass it a null_sha1.
>  - Change ref_transaction_delete die() instead of error() if we pass
>    if a NULL old_sha1 but have have_old == true.
>  - Change several places to do  if(!transaction || ref_transaction_update()
>    || ref_Transaction_commit()) die(generic-message) instead of checking each
>    step separately and having a different message for each failure.
>    Most users are likely not interested in what step of the transaction
>    failed and only whether it failed or not.
>  - Change commit.c to only pass a pointer to ref_transaction_update
>    iff current_head is non-NULL.
>    The previous patch used to compute a garbage pointer for
>    current_head->object.sha1 and relied on the fact that ref_transaction_update
>    would not try to dereference this pointer if !!current_head was 0.
>  - Updated commit message for the walker_fetch change to try to justify why
>    the change in locking semantics should not be harmful.

Will queue, but when applied on top of mh/ref-transaction and tested
standalone, it appears to fail test #14 of t5550-http-fetch-dumb.sh
for me.

It seems that the culprit is that this step:

    git http-fetch -a -w heads/master-new $HEAD $(git config remote.origin.url) &&

creates ".git/heads/master-new" when what it was asked to create was
".git/refs/heads/master-new".  Perhaps there is something fishy in
the conversion in walker.c?  We used to do lock_ref_sha1() on
"heads/master-new", which prepended "refs/" prefix before calling
lock_ref_sha1_basic() that expects the full path from $GIT_DIR/, but
ref_transaction_update(), which wants to see the full path, is still
fed "heads/master-new" after this series.