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

> 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.
>