From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20]) by dcvr.yhbt.net (Postfix) with ESMTP id 280671F51E for ; Tue, 27 Sep 2022 16:32:17 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=pass (1024-bit key; unprotected) header.d=pobox.com header.i=@pobox.com header.b="x68T6mOX"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230482AbiI0Qbc (ORCPT ); Tue, 27 Sep 2022 12:31:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50906 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231269AbiI0QbU (ORCPT ); Tue, 27 Sep 2022 12:31:20 -0400 Received: from pb-smtp20.pobox.com (pb-smtp20.pobox.com [173.228.157.52]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 52AEADF98 for ; Tue, 27 Sep 2022 09:31:19 -0700 (PDT) Received: from pb-smtp20.pobox.com (unknown [127.0.0.1]) by pb-smtp20.pobox.com (Postfix) with ESMTP id A18561ACC96; Tue, 27 Sep 2022 12:31:18 -0400 (EDT) (envelope-from junio@pobox.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=EwymQVhx6SeIdGkLZKHwey93A20rzU0UZO8OVS j0zgg=; b=x68T6mOXwTBSbALUgdMErfLg1513Twe6VwinDXsOjPsWo4/a+J5eKv AX1G9+f/ycEHY6zMKehq5/CZ6Ti4BalWHyXo99OlyJ/DbtCppxglvJZgU7Qodp44 nZ84jwuq5R6vJF5HgXEgpJPXDX+knDnFeLvEeb3tNFqnuZpJlWUqs= Received: from pb-smtp20.sea.icgroup.com (unknown [127.0.0.1]) by pb-smtp20.pobox.com (Postfix) with ESMTP id 9917B1ACC95; Tue, 27 Sep 2022 12:31:18 -0400 (EDT) (envelope-from junio@pobox.com) Received: from pobox.com (unknown [34.83.5.33]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp20.pobox.com (Postfix) with ESMTPSA id A708E1ACC90; Tue, 27 Sep 2022 12:31:14 -0400 (EDT) (envelope-from junio@pobox.com) From: Junio C Hamano To: "Derrick Stolee via GitGitGadget" Cc: git@vger.kernel.org, vdye@github.com, SZEDER =?utf-8?Q?G=C3=A1bor?= , =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason , Derrick Stolee Subject: Re: [PATCH v4 0/4] scalar: make unregister idempotent References: Date: Tue, 27 Sep 2022 09:31:13 -0700 In-Reply-To: (Derrick Stolee via GitGitGadget's message of "Tue, 27 Sep 2022 13:56:57 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: CE0CC2E0-3E81-11ED-9851-C2DA088D43B2-77302942!pb-smtp20.pobox.com Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org "Derrick Stolee via GitGitGadget" 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? 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.