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: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-4.1 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_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 15B811F670 for ; Tue, 26 Oct 2021 00:46:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233736AbhJZAsW (ORCPT ); Mon, 25 Oct 2021 20:48:22 -0400 Received: from pb-smtp2.pobox.com ([64.147.108.71]:51720 "EHLO pb-smtp2.pobox.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232476AbhJZAsV (ORCPT ); Mon, 25 Oct 2021 20:48:21 -0400 Received: from pb-smtp2.pobox.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 1516CFA6A6; Mon, 25 Oct 2021 20:45:58 -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=4fpmRGBWDeZGMfSf+tTwpBqfwFHIljbCX79mCO AiJZA=; b=ZPvbGrsJss2WNnq67nC9l7XSwQQbFWa4P2LIJzgvTUPcRA93BQm/ZZ T6U6ZFfdjcgXI4Ur5JcA0kTfdCMs4KOMZu18PJO/B1YGejkDmcM8J9Sov24zQeRd sv36+fzLGzb/87ALthI2qWsia9ZjX+bZPrWwLvXQPVMd/rrx7PfDs= Received: from pb-smtp2.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 0C9C5FA6A5; Mon, 25 Oct 2021 20:45:58 -0400 (EDT) (envelope-from junio@pobox.com) Received: from pobox.com (unknown [104.133.2.91]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp2.pobox.com (Postfix) with ESMTPSA id 6D865FA6A4; Mon, 25 Oct 2021 20:45:57 -0400 (EDT) (envelope-from junio@pobox.com) From: Junio C Hamano To: Glen Choo Cc: git@vger.kernel.org Subject: Re: [PATCH v3 2/4] remote: use remote_state parameter internally References: Date: Mon, 25 Oct 2021 17:45:56 -0700 In-Reply-To: (Glen Choo's message of "Mon, 25 Oct 2021 16:00:05 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 1525EA84-35F6-11EC-8620-CD991BBA3BAF-77302942!pb-smtp2.pobox.com Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Glen Choo writes: >> If the answer is yes, then the function need to take both branch and >> remote_state as two separate parameters. If the answer is no (i.e. >> we never ask hypothetical questions, we just ask what we should do >> in the current, real, state), then the function can just take the >> remote_state and remote_state->branch being NULL would be used as a >> signal that the HEAD is detached. I suspect it is the former, as >> this information is used in string-name-to-object translation for >> "topic@{push}" in object-name.c::interpret_branch_mark() function. > > I agree that the need for hypothetical "what happens if HEAD were > detached?" questions may arise, though I'm not sure if there are any > right now. When we call branch_get(NULL), the expected return value is > the "current_branch". If there is no "current_branch" i.e. the return > value of branch_get() is the NULL branch. A NULL branch is not usable - > branch_get_push() and branch_get_upstream() return error messages > indicating "HEAD does not point to a branch". (these are the functions > used by object-name.c::interpret_branch_mark()). > > Given the convention of "NULL branch == detached HEAD", how do we > proceed? Some options: > > a) Represent detached HEAD with a non-NULL "struct branch". This will > let us continue using the remote_state backpointer, which makes many > interfaces clean, but is somewhat invasive, error-prone and it uses > "struct branch" for something that is not a branch, which is itself > an error-prone interface. I'd rather not to go there. > b) Keep the backpointer, but add remote_state as a parameter to > pushremote_for_branch(). The least possible effort to fix the problem > and might be 'easy' but is inconsistent with the other functions and > is prone to misuse because the backpointer and parameter can be > different. Make the function take a remote_state as a parameter (instead of, not in addition to, the branch parameter), and use the remote_state structure to find which branch's branch specific configuration we want to use by checking its current_branch member. I think that would be the best approach for "no, we won't ask hypothetical question" case. On the other hand,... > c) Replace the backpointer with a remote_state parameter. Expressive and > fits the paradigm of "defaulting to the repo when needed", but > interfaces are repetitive and shifts the responsibility of > correctness to the caller (see v2). ... if we want to support the what-if callers, I think the best approach would be a slight variant of c) above. That is, pass branch and remote_state as two parameters, and when branch is not NULL, barf if it is not among remote_state.branches[], to protect against nonsense combinations. Thanks.