Re: [PATCH v4 0/4] scalar: make unregister idempotent
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 27, 2022, 16:31 UTC
- Message-ID
- <xmqqtu4s7p9q.fsf@gitster.g>
- In-Reply-To
- <pull.1358.v4.git.1664287021.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 12 quoted lines
> 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?
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.