Re: [PATCH v4 0/4] scalar: make unregister idempotent
On 9/27/2022 12:31 PM, Junio C Hamano wrote:
Show 21 quoted lines
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> I noticed this while we were updating the microsoft/git fork to include
>> v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
>> before, but instead some change in 'scalar unregister' led to it relying on
>> the return code of 'git maintenance unregister'. Our functional tests expect
>> 'scalar unregister' to be idempotent, and I think that's a good pattern for
>> 'git maintenance unregister', so I'm fixing it at that layer.
>>
>> Despite finding this during the 2.38.0-rc0 integration, this isn't critical
>> to the release.
>>
>> Perhaps an argument could be made that "failure means it wasn't registered
>> before", but I think that isn't terribly helpful.
>
> I happen _not_ to share the idea that "unregister is expected to be
> idempotent" is a good pattern at all, and it is a better pattern to
> make failure mean that the object specified to be acted upon did not
> even exist (cf. "rm no-such-file"). But that aside, does what the
> above paragraphs mention still true for this round, namely, you are
> "fixing" it at that (i.e. "maintenance unregister") layer?
Sorry, I forgot to update the cover letter when updating the title.
"git maintenance unregister" will fail if the repository is not already
registered (unless --force is given). Now, "scalar unregister" _is_
idempotent (it uses "git maintenance unregister --force").
Show 6 quoted lines
> The cover letter does not become part of the permanent history, so
> it is not a huge deal as long as the reviewers know what they are
> looking at ;-). Just leaving a note in case somebody who missed the
> iterations up to v3 is now interested in the topic.
>
> Thanks for a quick iteration.
Thank you for your very careful review!
-Stolee
Derrick Stolee <derrickstolee@github.com>