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@xxxxxxxx>
---
t/t5801/git-remote-testgit | 4 +++-
transport-helper.c | 7 ++-----
2 files changed, 5 insertions(+), 6 deletions(-)
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");
--
2.44.0.650.g4615f65fe0
