On 9/27/2022 12:31 PM, Junio C Hamano wrote:

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted

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