@matheusbsbs / matheus-bagliano / commits / 9e89dcb66a

builtin/ls-remote: fall back to SHA1 outside of a repo

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 <mh@glandium.org> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Aug 2, 2024 at 06:44 UTC 9e89dcb66a70902fef966083998a75272d47d998

2 files changed +28

builtin/ls-remote.c

+15

@@ -91,6 +91,21 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)

91 PARSE_OPT_STOP_AT_NON_OPTION);

92 dest = argv[0];

94 + /*

95 + * TODO: This is buggy, but required for transport helpers. When a

96 + * transport helper advertises a "refspec", then we'd add that to a

97 + * list of refspecs via `refspec_append()`, which transitively depends

98 + * on `the_hash_algo`. Thus, when the hash algorithm isn't properly set

99 + * up, this would lead to a segfault.

100 + *

101 + * We really should fix this in the transport helper logic such that we

102 + * lazily parse refspec capabilities _after_ we have learned about the

103 + * remote's object format. Otherwise, we may end up misparsing refspecs

104 + * depending on what object hash the remote uses.

105 + */

106 + if (!the_repository->hash_algo)

107 + repo_set_hash_algo(the_repository, GIT_HASH_SHA1);

108 +

109 packet_trace_identity("ls-remote");

110

111 if (argc > 1) {

t/t5512-ls-remote.sh

+13

@@ -402,4 +402,17 @@ test_expect_success 'v0 clients can handle multiple symrefs' '

402 test_cmp expect actual

403 '

404

405 +test_expect_success 'helper with refspec capability fails gracefully' '

406 + mkdir test-bin &&

407 + write_script test-bin/git-remote-foo <<-EOF &&

408 + echo import

409 + echo refspec ${SQ}*:*${SQ}

410 + EOF

411 + (

412 + PATH="$PWD/test-bin:$PATH" &&

413 + export PATH &&

414 + test_must_fail nongit git ls-remote foo::bar

415 + )

416 +'

417 +

418 test_done