Subject: Re: [PATCH v12 7/8] fetch: set remote/HEAD if it does not exist

From: "Bence Ferdinandy" <[email-masked]>
Date: Wed, 23 Oct 2024 19:07:06 +0200

On Wed Oct 23, 2024 at 18:50, Kristoffer Haugsbakk <[email-masked]> wrote:
> On Wed, Oct 23, 2024, at 17:36, Bence Ferdinandy wrote:
>> If the user has remote/HEAD set already and it looks like it has changed
>> on the server, then print a message, otherwise set it if we can.
>> Silently pass if the user already has the same remote/HEAD set as
>> reported by the server or if we encounter any errors along the way.
>>
>> Signed-off-by: Bence Ferdinandy <[email-masked]>
>> ---
>>
>> Notes:
>>     v3: - does not rely on remote set-head anymore so it only authenticates
>>         once
>>         - uses the new REF_CREATE_ONLY to atomically check if the ref exists
>>           and only write it if it doesn't
>>         - in all other cases the maximum it does is print a warning
>>
>>     v4: - instead of the discarded REF_CREATE_ONLY, it uses the existing,
>>           but updated transaction api to request a silent create only
>>         - it now uses the atomic before_target to determine reporting
>>         - refactored for legibility
>>
>>     v5: - instead of printing a not too useful message, it now fails
>>           silently, this in line with the objective to only set up
>>           remote/HEAD automatically if the right thing is trivial, for
>>           everything else there is remote set-head
>>         - fixed all failing tests
>>         - added two new tests, one for checking if remote/HEAD is set to the
>>           correct one, and one to test that we do not override remote/HEAD
>>           if it has changed on the server from what we have locally
>>
>>     v6: - fixed style issues and unintended extra empty line
>>         - updated function call with bool to int from previous patch's
>>           change
>>         - removed calls to error(...) inherited from builtin/remote.c so we
>>           actually fail silently
>>         - set the test for remote set-head --auto to the correct value here,
>>           which was previously erronously set in the remote set-head patch
>>
>>     v7: - no change
>>
>>     v8: - changed logmsg in call to refs_update_symref from "remote
>>           set-head" to "fetch"
>>
>>     v9: - follow through with refs_update_symref_extended
>>         - fix test errors uncovered by the new patch
>>
>>     v10: no change
>>
>>     v11: fixed some memory leaks
>>
>>     v12: no change
>
> I think it would be better to reverse-order these patch changelog
> comments so that the newest is on top/first.  (for next time)
>
> Thanks for the careful versioning here.

Yeah, this works fine when you only go up to v3 and it all fits on a screen :D
This is definitely the longest patch series I've made thus far, both in number
of commits and versions. I also noticed this today when I realized that I did the
cover letter in reverse and how much better readable that was than some of the
patches ...

One thing that would not be easy is that with so many patches, how I start out
the notes is

	git rev-list HEAD~8..HEAD | xargs -i git notes append {} -m "v12: no change"

and there's no "git notes prepend". That could be another patch for another
time :) It also bothers me, that the branch description that can be used to
save cover letters is not a note, just a local configuration, but that's
a third issue.

Anyhow, thanks for calling it out, if we do come to a v13 I might just spend
some time on reversing (or be lazy and just continue at the top ...).

Best,
Bence

-- 
bence.ferdinandy.com