| Message ID | 20240320094103.GC2445682@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b5b7b17b2efafeaf421d284ddd4eca55272bc47d |
| Headers | show |
| Series | some transport-helper "option object-format" confusion | expand |
Jeff King <peff@peff.net> writes: > - remote-curl, which quietly converts missing values into "true". This > goes all the way back to ef08ef9ea0 (remote-helpers: Support custom > transport options, 2009-10-30), despite the fact that I don't think > any other option has ever made use of it. Interesting. > When the documentation and code do not match, it is often good to fix > the documentation rather than break compatibility. And in this case, we > have had the mis-match since 8b85ee4f47 (transport-helper: implement > object-format extensions, 2020-05-25). However, the sha256 feature was > listed as experimental until 8e42eb0e9a (doc: sha256 is no longer > experimental, 2023-07-31). > ... > And doing so has some value: it brings "object-format" in line with the > syntax of other options, making the protocol more consistent. It also > lets us use set_helper_option(), which has better error reporting. I suspect that this may have been an attempt to mimick the value-less true in the configuration syntax, but I agree with the conclusion of this patch. Boolean "true" in the context of the transport options may be fairly common, but unlike configuration files, it is not something we have users write manually, and there is not much point giving a special short form. Thanks for a pleasant read. Will queue.
diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit index bcfb358c51..c5b10f5775 100755 --- a/t/t5801/git-remote-testgit +++ b/t/t5801/git-remote-testgit @@ -30,6 +30,7 @@ GIT_DIR="$url/.git" export GIT_DIR force= +object_format= mkdir -p "$dir" @@ -61,7 +62,8 @@ do echo ;; list) - echo ":object-format $(git rev-parse --show-object-format=storage)" + test -n "$object_format" && + echo ":object-format $(git rev-parse --show-object-format=storage)" git for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/' head=$(git symbolic-ref HEAD) echo "@$head HEAD" diff --git a/transport-helper.c b/transport-helper.c index 7f6bbd06bb..8d284b24d5 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -1210,11 +1210,8 @@ static struct ref *get_refs_list_using_list(struct transport *transport, data->get_refs_list_called = 1; helper = get_helper(transport); - if (data->object_format) { - write_constant(helper->in, "option object-format\n"); - if (recvline(data, &buf) || strcmp(buf.buf, "ok")) - exit(128); - } + if (data->object_format) + set_helper_option(transport, "object-format", "true"); if (data->push && for_push) write_constant(helper->in, "list for-push\n");
The documentation in gitremote-helpers.txt claims that after a helper has advertised the "object-format" capability, Git may then send "option object-format true" to indicate that it would like to hear which object format the helper is using when it returns refs. However, the code implementing this has always written just "option object-format", without the extra "true" value. Nobody noticed in practice or in the tests because the only two helpers we ship are: - remote-curl, which quietly converts missing values into "true". This goes all the way back to ef08ef9ea0 (remote-helpers: Support custom transport options, 2009-10-30), despite the fact that I don't think any other option has ever made use of it. - remote-testgit in t5801 does insist on having a "true" value. But since it sends the ":object-format" response regardless of whether it thinks the caller asked for it (technically breaking protocol), everything just works, albeit with an extra shell error: .../git/t/t5801/git-remote-testgit: 150: test: =: unexpected operator printed to stderr, which you can see running t5801 with --verbose. (The problem is that $val is the empty string, and since we don't double-quote it in "test $val = true", we invoke "test = true" instead). When the documentation and code do not match, it is often good to fix the documentation rather than break compatibility. And in this case, we have had the mis-match since 8b85ee4f47 (transport-helper: implement object-format extensions, 2020-05-25). However, the sha256 feature was listed as experimental until 8e42eb0e9a (doc: sha256 is no longer experimental, 2023-07-31). It's possible there are some third party helpers that tried to follow the documentation, and are broken. Changing the code will fix them. It's also possible that there are ones that follow the code and will be broken if we change it. I suspect neither is the case given that no helper authors have brought this up as an issue (I only noticed it because I was running t5801 in verbose mode for other reasons and wondered about the weird shell error). That, coupled with the relative new-ness of sha256, makes me think nobody has really worked on helpers for it yet, which gives us an opportunity to correct the code before too much time passes. And doing so has some value: it brings "object-format" in line with the syntax of other options, making the protocol more consistent. It also lets us use set_helper_option(), which has better error reporting. Note that we don't really need to allow any other values like "false" here. The point is for Git to tell the helper that it understands ":object-format" lines coming back as part of the ref listing. There's no point in future versions saying "no, I don't understand that". To make sure everything works as expected, we can improve the remote-testgit helper from t5801 to send the ":object-format" line only if the other side correctly asked for it (which modern Git will always do). With that test change and without the matching code fix here, t5801 will fail when run with GIT_TEST_DEFAULT_HASH=sha256. Signed-off-by: Jeff King <peff@peff.net> --- t/t5801/git-remote-testgit | 4 +++- transport-helper.c | 7 ++----- 2 files changed, 5 insertions(+), 6 deletions(-)