From patchwork Fri Aug 2 04:44:11 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13751092 Received: from fhigh5-smtp.messagingengine.com (fhigh5-smtp.messagingengine.com [103.168.172.156]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EFEED256D for ; Fri, 2 Aug 2024 04:44:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.156 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722573860; cv=none; b=rbGQMFwQbIA4T9kQr7zF34ocDH1bnqOmERRyBmk7SQ+D+slE03/BAY/0DAJ+lPxhyUOg9Vgvr/jG34boPrY1/6p4KKG7DYmNwk6v+wiYKdQ2kPAMvrs6sCh7nCMoVhoDfxAOUFfNFtvTcOVwWxQRZ/yFOEr0lj8EhPlJkfVUS2I= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722573860; c=relaxed/simple; bh=jJck9f8G3xhCQyK6YKVVlY5twrpd/X7sMznLemIF0vA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=WHsS963l8bBjULvbED1PoeuK2z94Cgcp4iqs4yyeVgmfHEmSrnf5AGu7te7idFf8YjK0isQLDVCiSlkuyqRJ6j4lG3udh9+yovxGZYNCZWIcELSlxTEd/wV0ym+KeWBY8WljlXfP1kF4dCOPFfHSF3B4VKEO9PNmb2VOZIU1Zmc= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=DzVmj/KN; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=Oklssq4v; arc=none smtp.client-ip=103.168.172.156 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="DzVmj/KN"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="Oklssq4v" Received: from compute7.internal (compute7.nyi.internal [10.202.2.48]) by mailfhigh.nyi.internal (Postfix) with ESMTP id E295E1151B55; Fri, 2 Aug 2024 00:44:16 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute7.internal (MEProxy); Fri, 02 Aug 2024 00:44:16 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm3; t=1722573856; x=1722660256; bh=DF/yOd6G7P zot1DatKXNgR/jvZBLd6Iq7K4AxMJBvKU=; b=DzVmj/KNLi9eCKdtY/8lfDz/6s I9MSx01cH6ZnpafSR2kAHMEzZj+e9z7P238uYqiN5nyiq6McmMLobgoxaaMSWHn9 DNCJFiUb9Qzlkq+iqYw85UB/lZXSh1KD6RBuCGPMX/YWSZPCV09+o8YfQoIPcTJ0 HBQpuA/3nc65d9MpJH5Ip/9O3XHWsh5JUA5a1mXm8ziLy2lj2aN2IxN6xmdPsqot XFxATVU6oFLzXUQFKtl3x4ND0ufEiVA0mJFJDWfs1QLCoCv/FFmLG6y0096jB35I EU1mW0bPtxXvJvXZXj4sQZEEGjrmQbPZ+3xUJOuERKAPK91nKqV4ofMSM7xg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1722573856; x=1722660256; bh=DF/yOd6G7Pzot1DatKXNgR/jvZBL d6Iq7K4AxMJBvKU=; b=Oklssq4vR3p5cITZjW+dWtRjYKxx54Gm+qeCPgGyqNaZ o3olyz2BrkPGJerJwWh90edBLpDsnLICQJHmlC8VXzw2AulpqmbvIqdnbcNjudl0 nCl/VUz8lQLEt5qpYI0MhGcFXfBpu6QigHgsWG3ZG0S45o2qL1UdhVCzEhGOFrNc Z/0uJDkikC8+EEOGFWDxecHaAMA915RBEzBWUqYIsq4QU0ph4NtfJv3MWFlYSJJb 1XJ3lpWrS2MUiGR6X+cwXhamzRpgTWCIVU1mOyVhIhjRauqYFr9cXJSOAeBnR8Ih 2C9iS4Kcs0RHro99srysiIR/VpeAbuiG1JrTJJbQlw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrjeelgdeklecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeeukedtvedtffevleejtefgheehieegkeeluddvfeefgeehgfeltddtheejleffteen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpshesph hkshdrihhmpdhnsggprhgtphhtthhopedt X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 2 Aug 2024 00:44:15 -0400 (EDT) Received: by localhost (OpenSMTPD) with ESMTPSA id 109966f0 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Fri, 2 Aug 2024 04:42:40 +0000 (UTC) Date: Fri, 2 Aug 2024 06:44:11 +0200 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Mike Hommey , Junio C Hamano Subject: [PATCH] builtin/ls-remote: fall back to SHA1 outside of a repo Message-ID: References: <20240727191917.p64ul4jybpm2a7hm@glandium.org> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240727191917.p64ul4jybpm2a7hm@glandium.org> In c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), we have stopped setting the default hash algorithm for `the_repository`. Consequently, code that relies on `the_hash_algo` will now crash when it hasn't explicitly been initialized, which may be the case when running outside of a Git repository. It was reported that git-ls-remote(1) may crash in such a way when using a remote helper that advertises refspecs. This is because the refspec announced by the helper will get parsed during capability negotiation. At that point we haven't yet figured out what object format the remote uses though, so when run outside of a repository then we will fail. The course of action is somewhat dubious in the first place. Ideally, we should only parse object IDs once we have asked the remote helper for the object format. And if the helper didn't announce the "object-format" capability, then we should always assume SHA256. But instead, we used to take either SHA1 if there was no repository, or we used the hash of the local repository, which is wrong. Arguably though, crashing hard may not be in the best interest of our users, either. So while the old behaviour was buggy, let's restore it for now as a short-term fix. We should eventually revisit, potentially by deferring the point in time when we parse the refspec until after we have figured out the remote's object hash. Reported-by: Mike Hommey Signed-off-by: Patrick Steinhardt --- I didn't spot this in the "What's cooking" report. I guess that's my own fault for not sending it as a proper patch, so let me fix that now :) Patrick builtin/ls-remote.c | 15 +++++++++++++++ t/t5512-ls-remote.sh | 13 +++++++++++++ 2 files changed, 28 insertions(+) diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index debf2d4f88..6da63a67f5 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -91,6 +91,21 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) PARSE_OPT_STOP_AT_NON_OPTION); dest = argv[0]; + /* + * TODO: This is buggy, but required for transport helpers. When a + * transport helper advertises a "refspec", then we'd add that to a + * list of refspecs via `refspec_append()`, which transitively depends + * on `the_hash_algo`. Thus, when the hash algorithm isn't properly set + * up, this would lead to a segfault. + * + * We really should fix this in the transport helper logic such that we + * lazily parse refspec capabilities _after_ we have learned about the + * remote's object format. Otherwise, we may end up misparsing refspecs + * depending on what object hash the remote uses. + */ + if (!the_repository->hash_algo) + repo_set_hash_algo(the_repository, GIT_HASH_SHA1); + packet_trace_identity("ls-remote"); if (argc > 1) { diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index 42e77eb5a9..bc442ec221 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -402,4 +402,17 @@ test_expect_success 'v0 clients can handle multiple symrefs' ' test_cmp expect actual ' +test_expect_success 'helper with refspec capability fails gracefully' ' + mkdir test-bin && + write_script test-bin/git-remote-foo <<-EOF && + echo import + echo refspec ${SQ}*:*${SQ} + EOF + ( + PATH="$PWD/test-bin:$PATH" && + export PATH && + test_must_fail nongit git ls-remote foo::bar + ) +' + test_done