Re: [PATCH] refs.c: clean up write_ref_sha1 returns

[Stefan Beller <sbeller@google.com>]

On Mon, Jan 26, 2015 at 7:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>     I can redo the atomic-push-fix series with this cleanup merged
>>     into the appropriate patches or you could just queue it on top
>>     of said series.
>
> Yeah, I do not think we are expecting to fast track these two series
> through 'next' to 'master' before 2.3 final, so I think it would be
> better to use this patch _only_ to see if the final shape of the
> code this patch represents makes sense, so that we can expedite the
> final submission in the next development cycle, at which time we
> will have a chance to refresh 'next', hence a chance to clean-up
> atomic-push series in place.

I tried to rip this patch and its 3 previous patches apart to see if it could be
done another way. The outcome was to actually sqquash this patch
completely into b1c6da0a13 (refs.c: remove unlock_ref and commit_ref
from write_ref_sha1).

Looking at the end result the write_ref_sha1 function it has a good
design contract. Either you get 0 returned and all is good, or -1 is returned
and errno is set to a meaningful value which seems to adress your concerns
on that patch:

> I am not sure if it is sensible to call that "correct but hard to
> understand".  I'd rather see us admit that its behaviour is screwey
> and needs fixing for better code health longer term.


>> @@ -2880,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>               error("unable to lock %s for update", newrefname);
>>               goto rollback;
>>       }
>> -     lock->force_write = 1;
>>       hashcpy(lock->old_sha1, orig_sha1);
>
> Is this hashcpy() still necessary?

Thanks for catching that! It is not necessary any more and will be
removed in a reroll.
I think I'll wait for rerolling the atomic-push-fix series until 2.3
is out then?