On 9/27/2022 12:31 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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"). > 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
