From: brian m. carlson <hidden> Date: 2020-05-13 00:55:14
This is part 2 of 3 of the SHA-256 work. This series, which is
unfortunately longer than I'd like, adds all of the protocol logic to
work with SHA-256 repositories.
It was originally planned that we would not upgrade the protocol and
would use SHA-1 for all protocol functionality until some point in the
future. However, doing that requires a huge amount of additional work
(probably incorporating several hundred more patches which are not yet
written) and it's not possible to get the test suite to even come close
to passing without a way to fetch and push repositories. I therefore
decided that implementing an object-format extension was the best way
forward.
This series provides object-format extensions for both the original and
v2 protocols, including full documentation. Helpers, such as
git-remote-https, also learn capabilities to pass the object-format
extension back and forth, and to query its state. The code is designed
to allow multiple object-format extensions to be provided if the server
supports multiple algorithms for one repo and to default to SHA-1 if no
object-format extension is provided.
The other two cases are the dumb HTTP protocol and bundles, both of
which have no object-format extension (because they provide no
capabilities) and are therefore distinguished solely by their hash
length. We will have problems if in the future we need to use another
256-bit algorithm, but I plan to be improvident and hope that we'll move
to longer algorithms in the future to cover ourselves for post-quantum
security.
Clone support is necessarily a little tricky because we are initializing
a repository and then fetching refs, at which point we learn what hash
algorithm the remote side supports. We work around this by calling the
code that updates the hash algorithm and repository version a second
time to rewrite that data once we know what version we're using. This
is the most robust way I could approach this problem, but it is still a
little ugly.
As mentioned, this series is longer than I'd like, but it is complete:
this is all the SHA-256 protocol work. Additional future series include
one last series of test fixes (28 patches) plus six final patches in the
series that enables SHA-256 support.
brian m. carlson (44):
t1050: match object ID paths in a hash-insensitive way
Documentation: document v1 protocol object-format capability
connect: have ref processing code take struct packet_reader
wrapper: add function to compare strings with different NUL
termination
remote: advertise the object-format capability on the server side
connect: add function to parse multiple v1 capability values
connect: add function to fetch value of a v2 server capability
pkt-line: add a member for hash algorithm
transport: add a hash algorithm member
connect: add function to detect supported v1 hash functions
send-pack: detect when the server doesn't support our hash
connect: make parse_feature_value extern
fetch-pack: detect when the server doesn't support our hash
connect: detect algorithm when fetching refs
builtin/receive-pack: detect when the server doesn't support our hash
docs: update remote helper docs for object-format extensions
transport-helper: implement object-format extensions
remote-curl: implement object-format extensions
builtin/clone: initialize hash algorithm properly
t5562: pass object-format in synthesized test data
t5704: send object-format capability with SHA-256
fetch-pack: parse and advertise the object-format capability
setup: set the_repository's hash algo when checking format
t3200: mark assertion with SHA1 prerequisite
packfile: compute and use the index CRC offset
t5302: modernize test formatting
builtin/show-index: provide options to determine hash algo
t1302: expect repo format version 1 for SHA-256
Documentation/technical: document object-format for protocol v2
connect: pass full packet reader when parsing v2 refs
connect: parse v2 refs with correct hash algorithm
serve: advertise object-format capability for protocol v2
t5500: make hash independent
builtin/ls-remote: initialize repository based on fetch
remote-curl: detect algorithm for dumb HTTP by size
builtin/index-pack: add option to specify hash algorithm
t1050: pass algorithm to index-pack when outside repo
remote-curl: avoid truncating refs with ls-remote
t/helper: initialize the repository for test-sha1-array
t5702: offer an object-format capability in the test
t5703: use object-format serve option
t5300: pass --object-format to git index-pack
bundle: detect hash algorithm when reading refs
remote-testgit: adapt for object-format
Documentation/gitremote-helpers.txt | 33 +-
.../technical/protocol-capabilities.txt | 16 +-
Documentation/technical/protocol-v2.txt | 9 +
builtin/clone.c | 9 +
builtin/index-pack.c | 11 +-
builtin/ls-remote.c | 4 +
builtin/receive-pack.c | 10 +
builtin/show-index.c | 29 +-
bundle.c | 22 +-
bundle.h | 1 +
connect.c | 136 +++++--
connect.h | 3 +
fetch-pack.c | 14 +
git-compat-util.h | 2 +
git.c | 2 +-
object-store.h | 1 +
packfile.c | 1 +
pkt-line.c | 1 +
pkt-line.h | 3 +
remote-curl.c | 46 ++-
send-pack.c | 6 +
serve.c | 27 ++
setup.c | 1 +
t/helper/test-oid-array.c | 3 +
t/t1050-large.sh | 6 +-
t/t1302-repo-version.sh | 6 +-
t/t3200-branch.sh | 2 +-
t/t5300-pack-object.sh | 9 +-
t/t5302-pack-index.sh | 360 +++++++++---------
t/t5500-fetch-pack.sh | 5 +-
t/t5562-http-backend-content-length.sh | 14 +-
t/t5701-git-serve.sh | 28 +-
t/t5702-protocol-v2.sh | 2 +
t/t5703-upload-pack-ref-in-want.sh | 19 +-
t/t5704-protocol-violations.sh | 12 +
t/t5801/git-remote-testgit | 6 +
t/test-lib.sh | 1 +
transport-helper.c | 24 +-
transport.c | 18 +-
transport.h | 8 +
upload-pack.c | 3 +-
wrapper.c | 12 +
42 files changed, 670 insertions(+), 255 deletions(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:40
The pattern here looking for failures is specific to SHA-1. Let's
create a variable that matches the regex or glob pattern for a path
within the objects directory.
Signed-off-by: brian m. carlson <redacted>
---
t/t1050-large.sh | 2 +-
t/test-lib.sh | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
@@ -64,7 +64,7 @@ test_expect_success 'add a large file or two' 'test$count=1&&cnt=$(gitshow-index<"$idx"|wc-l)&&test$cnt=2&&-forlin.git/objects/??/??????????????????????????????????????+forlin.git/objects/$OIDPATH_REGEXdotest_path_is_file"$l"||continuebad=t
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:42
Document a capability that indicates which hash algorithms are in use by
both sides of a remote connection. Use the term "object-format", since
this is the term used for the repository extension as well.
Signed-off-by: brian m. carlson <redacted>
---
.../technical/protocol-capabilities.txt | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
@@ -176,6 +176,21 @@ agent strings are purely informative for statistics and debugging purposes, and MUST NOT be used to programmatically assume the presence or absence of particular features.+object-format+-------------++This capability, which takes a hash algorithm as an argument, indicates+that the server supports the given hash algorithms. It may be sent+multiple times; if so, the first one given is the one used in the ref+advertisement.++When provided by the client, this indicates that it intends to use the+given hash algorithm to communicate. The algorithm provided must be one+that the server supports.++If this capability is not provided, it is assumed that the only+supported algorithm is SHA-1.+ symref ------
@@ -189,7 +204,6 @@ refs being sent. Clients MAY use the parameters from this capability to select the proper initial branch when cloning a repository.- shallow -------
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:42
When parsing capabilities for the pack protocol, there are times we'll
want to compare the value of a capability to a NUL-terminated string.
Since the data we're reading will be space-terminated, not
NUL-terminated, we need a function that compares the two strings, but
also checks that they're the same length. Otherwise, if we used strncmp
to compare these strings, we might accidentally accept a parameter that
was a prefix of the expected value.
Add a function, xstrncmpz, that takes a NUL-terminated string and a
non-NUL-terminated string, plus a length, and compares them, ensuring
that they are the same length.
Signed-off-by: brian m. carlson <redacted>
---
git-compat-util.h | 2 ++
wrapper.c | 12 ++++++++++++
2 files changed, 14 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:43
In a capability response, we can have multiple symref entries. In the
future, we will also allow for multiple hash algorithms to be specified.
To avoid duplication, expand the parse_feature_value function to take an
optional offset where the parsing should begin next time. Add a wrapper
function that allows us to query the next server feature value, and use
it in the existing symref parsing code.
Signed-off-by: brian m. carlson <redacted>
---
connect.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:44
So far in protocol v2, all of our server capabilities that have values
have not had values that we've been interested in parsing. For example,
we receive but ignore the agent value.
However, in a future commit, we're going to want to parse out the value
of a server capability. To make this easy, add a function,
server_feature_v2, that can fetch the value provided as part of the
server capability.
Signed-off-by: brian m. carlson <redacted>
---
connect.c | 15 +++++++++++++++
connect.h | 1 +
2 files changed, 16 insertions(+)
@@ -84,6 +84,21 @@ int server_supports_v2(const char *c, int die_on_error)return0;}+intserver_feature_v2(constchar*c,constchar**v)+{+inti;++for(i=0;i<server_capabilities_v2.argc;i++){+constchar*out;+if(skip_prefix(server_capabilities_v2.argv[i],c,&out)&&+(*out=='=')){+*v=out+1;+return1;+}+}+return0;+}+intserver_supports_feature(constchar*c,constchar*feature,intdie_on_error){
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:45
Add a member for the hash algorithm currently in use to the packet
reader so it can parse references correctly.
Signed-off-by: brian m. carlson <redacted>
---
pkt-line.c | 1 +
pkt-line.h | 3 +++
2 files changed, 4 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:47
Add a function, server_supports_hash, to see if the remote server
supports a particular hash algorithm when speaking protocol v1.
Signed-off-by: brian m. carlson <redacted>
---
connect.c | 24 ++++++++++++++++++++++++
connect.h | 1 +
2 files changed, 25 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:51
Detect when the server doesn't support our hash algorithm and abort.
Signed-off-by: brian m. carlson <redacted>
---
builtin/receive-pack.c | 9 +++++++++
1 file changed, 9 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:51
We're going to be using this function in other files, so no longer mark
this function static.
Signed-off-by: brian m. carlson <redacted>
---
connect.c | 3 +--
connect.h | 1 +
2 files changed, 2 insertions(+), 2 deletions(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:52
Detect when the server doesn't support our hash algorithm and abort.
Signed-off-by: brian m. carlson <redacted>
---
fetch-pack.c | 2 ++
1 file changed, 2 insertions(+)
@@ -1039,6 +1039,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,print_verbose(args,_("Server supports %s"),"deepen-relative");elseif(args->deepen_relative)die(_("Server does not support --deepen"));+if(!server_supports_hash(the_hash_algo->name,NULL))+die(_("Server does not support this repository's object format"));if(!args->no_dependents){mark_complete_and_common_ref(negotiator,args,&ref);
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:53
Both v2 pack index files and the v3 format specified as part of the
NewHash work have similar data starting at the CRC table. Much of the
existing code wants to read either this table or the offset entries
following it, and in doing so computes the offset each time.
In order to share as much code between v2 and v3, compute the offset of
the CRC table and store it when the pack is opened. Use this value to
compute offsets to not only the CRC table, but to the offset entries
beyond it.
Signed-off-by: brian m. carlson <redacted>
---
builtin/index-pack.c | 6 +-----
object-store.h | 1 +
packfile.c | 1 +
3 files changed, 3 insertions(+), 5 deletions(-)
@@ -1555,13 +1555,9 @@ static void read_v2_anomalous_offsets(struct packed_git *p,{constuint32_t*idx1,*idx2;uint32_ti;-constuint32_thashwords=the_hash_algo->rawsz/sizeof(uint32_t);/* The address of the 4-byte offset table */-idx1=(((constuint32_t*)p->index_data)-+2/* 8-byte header */-+256/* fan out */-+hashwords*p->num_objects/* object ID table */+idx1=(((constuint32_t*)((constuint8_t*)p->index_data+p->crc_offset))+p->num_objects/* CRC32 table */);
@@ -178,6 +178,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,*/(sizeof(off_t)<=4))returnerror("pack too large for current definition of off_t in %s",path);+p->crc_offset=8+4*256+nr*hashsz;}p->index_version=version;
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:54
One of the test assertions in this test checks that git branch -m works
even without a .git/config file. However, if the repository requires
configuration extensions, such as because it uses a non-SHA-1 algorithm,
this assertion will fail. Mark the assertion as requiring SHA-1.
Signed-off-by: brian m. carlson <redacted>
---
t/t3200-branch.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:56
Parse the server's object-format capability and respond accordingly,
dying if there is a mismatch.
Signed-off-by: brian m. carlson <redacted>
---
fetch-pack.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -1179,6 +1179,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,intsideband_all,intseen_ack){intret=0;+constchar*hash_name;structstrbufreq_buf=STRBUF_INIT;if(server_supports_v2("fetch",1))
@@ -1193,6 +1194,17 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,args->server_options->items[i].string);}+if(server_feature_v2("object-format",&hash_name)){+inthash_algo=hash_algo_by_name(hash_name);+if(hash_algo_by_ptr(the_hash_algo)!=hash_algo)+die(_("mismatched algorithms: client %s; server %s"),+the_hash_algo->name,hash_name);+packet_write_fmt(fd_out,"object-format=%s",the_hash_algo->name);+}+elseif(hash_algo_by_ptr(the_hash_algo)!=GIT_HASH_SHA1)+die(_("the server does not support algorithm '%s'"),+the_hash_algo->name);+packet_buf_delim(&req_buf);if(args->use_thin_pack)packet_buf_write(&req_buf,"thin-pack");
From: brian m. carlson <hidden> Date: 2020-05-13 00:54:58
When we're checking the repository's format, set the hash algorithm at
the same time. This ensures that we perform a suitable initialization
early enough to avoid confusing any parts of the code. If we defer
until later, we can end up with portions of the code which are confused
about the hash algorithm, resulting in segfaults.
Signed-off-by: brian m. carlson <redacted>
---
setup.c | 1 +
1 file changed, 1 insertion(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:00
When we speak protocol v2 in this test, we must pass the object-format
header if the algorithm is not SHA-1. Otherwise, git upload-pack fails
because the hash algorithm doesn't match and not because we've failed to
speak the protocol correctly. Pass the header so that our assertions
test what we're really interested in.
Signed-off-by: brian m. carlson <redacted>
---
t/t5704-protocol-violations.sh | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -6,9 +6,20 @@ communications if the other side says something unexpected. We are mostly makingsurethatwedonotsegfaultorotherwisebehavebadly.' ../test-lib.sh+# If we don't print the object format, we'll fail for a spurious reason: the+# mismatched object format.+print_object_format(){+localalgo=$(test_oidalgo)&&+iftest"$algo"!="sha1"+then+packetize"object-format=$algo"+fi+}+ test_expect_success'extra delim packet in v2 ls-refs args''{packetizecommand=ls-refs&&+print_object_format&&printf0001&&# protocol expects 0000 flush hereprintf0001
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:01
Implement the object-format extensions that let us determine the hash
algorithm in use when pushing, pulling, and fetching.
Signed-off-by: brian m. carlson <redacted>
---
remote-curl.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:02
When we're parsing refs, we need to know not only what the line we're
parsing is, but also the hash algorithm we should use to parse it, which
is stored in the reader object. Pass the packet reader object through
to the protocol v2 ref parsing function.
Signed-off-by: brian m. carlson <redacted>
---
connect.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
@@ -373,7 +373,7 @@ struct ref **get_remote_heads(struct packet_reader *reader,}/* Returns 1 when a valid ref has been added to `list`, 0 otherwise */-staticintprocess_ref_v2(constchar*line,structref***list)+staticintprocess_ref_v2(structpacket_reader*reader,structref***list){intret=1;inti=0;
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:04
git index-pack is usually run in a repository, but need not be. Since
packs don't contains information on the algorithm in use, instead
relying on context, add an option to index-pack to tell it which one
we're using in case someone runs it outside of a repository.
Signed-off-by: brian m. carlson <redacted>
---
builtin/index-pack.c | 5 +++++
1 file changed, 5 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:05
In order to communicate the protocol supported by the server side, add
support for advertising the object-format capability. We check that the
client side sends us an identical algorithm if it sends us its own
object-format capability, and assume it speaks SHA-1 if not.
In the test, when we're using an algorithm other than SHA-1, we need to
specify the algorithm in use so we don't get a failure with an "unknown
format" message. Add a wrapper function that specifies this header if
required. Skip specifying this header for SHA-1 to test that it works
both with and without this header.
Signed-off-by: brian m. carlson <redacted>
---
serve.c | 27 +++++++++++++++++++++++++++
t/t5701-git-serve.sh | 28 ++++++++++++++++++++--------
2 files changed, 47 insertions(+), 8 deletions(-)
@@ -45,6 +56,7 @@ test_expect_success 'request invalid capability' ' test_expect_success'request with no command''test-toolpkt-linepack>in<<-EOF&&agent=git/test+object-format=$(test_oidalgo)0000EOFtest_must_failtest-toolserve-v2--stateless-rpc2>err<in&&
@@ -53,7 +65,7 @@ test_expect_success 'request with no command' ' test_expect_success'request invalid command''test-toolpkt-linepack>in<<-EOF&&-command=foo+$(write_commandfoo)agent=git/test0000EOF
@@ -73,7 +85,7 @@ test_expect_success 'setup some refs and tags' ' test_expect_success'basics of ls-refs''test-toolpkt-linepack>in<<-EOF&&-command=ls-refs+$(write_commandls-refs)0000EOF
@@ -199,7 +211,7 @@ test_expect_success 'unexpected lines are not allowed in fetch request' 'gitinitserver&&test-toolpkt-linepack>in<<-EOF&&-command=fetch+$(write_commandfetch)0001this-is-not-a-command0000
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:06
When using an algorithm other than SHA-1, we need the remote helper to
advertise support for the object-format extension and provide
information back to us so that we can properly parse refs and return
data. Ensure that the test remote helper understands these extensions.
Signed-off-by: brian m. carlson <redacted>
---
t/t5801/git-remote-testgit | 6 ++++++
1 file changed, 6 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:06
Ensure that we pass the object-format capability in the synthesized test
data so that this test works with algorithms other than SHA-1.
In addition, add an additional test using the old data for when we're
using SHA-1 so that we can be sure that we preserve backwards
compatibility with servers not offering the object-format capability.
Signed-off-by: brian m. carlson <redacted>
---
t/t5562-http-backend-content-length.sh | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:08
Much like with the dumb HTTP transport, there isn't a way to explicitly
specify the hash algorithm when dealing with a bundle, so detect the
algorithm based on the length of the object IDs in the prerequisites and
ref advertisements.
Signed-off-by: brian m. carlson <redacted>
---
bundle.c | 22 +++++++++++++++++++++-
bundle.h | 1 +
transport.c | 10 ++++++++--
3 files changed, 30 insertions(+), 3 deletions(-)
@@ -143,6 +143,9 @@ static struct ref *get_refs_from_bundle(struct transport *transport,data->fd=read_bundle_header(transport->url,&data->header);if(data->fd<0)die(_("could not read bundle '%s'"),transport->url);++transport->hash_algo=data->header.hash_algo;+for(i=0;i<data->header.references.nr;i++){structref_list_entry*e=data->header.references.list+i;structref*ref=alloc_ref(e->name);
@@ -157,11 +160,14 @@ static int fetch_refs_from_bundle(struct transport *transport,intnr_heads,structref**to_fetch){structbundle_transport_data*data=transport->data;+intret;if(!data->get_refs_from_bundle_called)get_refs_from_bundle(transport,0,NULL);-returnunbundle(the_repository,&data->header,data->fd,-transport->progress?BUNDLE_VERBOSE:0);+ret=unbundle(the_repository,&data->header,data->fd,+transport->progress?BUNDLE_VERBOSE:0);+transport->hash_algo=data->header.hash_algo;+returnret;}staticintclose_bundle(structtransport*transport)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:10
When we're using an algorithm other than SHA-1, we need to specify the
algorithm in use so we don't get a failure with an "unknown format"
message. Add a wrapper function that specifies this header if required.
Skip specifying this header for SHA-1 to test that it works both with an
without this header.
Signed-off-by: brian m. carlson <redacted>
---
t/t5703-upload-pack-ref-in-want.sh | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
@@ -129,7 +138,7 @@ test_expect_success 'mix want and want-ref' 'gitrev-parseef>expected_commits&&test-toolpkt-linepack>in<<-EOF&&-command=fetch+$(write_commandfetch)0001no-progresswant-refrefs/heads/master
@@ -152,7 +161,7 @@ test_expect_success 'want-ref with ref we already have commit for' 'oid=$(gitrev-parsec)&&test-toolpkt-linepack>in<<-EOF&&-command=fetch+$(write_commandfetch)0001no-progresswant-refrefs/heads/o/foo
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:10
git index-pack by default reads the repository to determine the object
format. However, when outside of a repository, it's necessary to specify
the hash algorithm in use so that the pack can be properly indexed. Add
an --object-format argument when invoking git index-pack outside of a
repository.
Signed-off-by: brian m. carlson <redacted>
---
t/t5300-pack-object.sh | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -12,7 +12,8 @@ TRASH=$(pwd) test_expect_success\'setup'\-'rm-f.git/index*&&+'test_oid_init&&+rm-f.git/index*&&perl-e"print \"a\" x 4096;">a&&perl-e"print \"b\" x 4096;">b&&perl-e"print \"c\" x 4096;">c&&
@@ -412,18 +413,18 @@ test_expect_success 'set up pack for non-repo tests' '' test_expect_success'index-pack --stdin complains of non-repo''-nongittest_must_failgitindex-pack--stdin<foo.pack&&+nongittest_must_failgitindex-pack--object-format=$(test_oidalgo)--stdin<foo.pack&&test_path_is_missingnon-repo/.git' test_expect_success'index-pack <pack> works in non-repo''-nongitgitindex-pack../foo.pack&&+nongitgitindex-pack--object-format=$(test_oidalgo)../foo.pack&&test_path_is_filefoo.idx' test_expect_success'index-pack --strict <pack> works in non-repo''rm-ffoo.idx&&-nongitgitindex-pack--strict../foo.pack&&+nongitgitindex-pack--strict--object-format=$(test_oidalgo)../foo.pack&&test_path_is_filefoo.idx'
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:11
In order to make this test work with SHA-256, offer an object-format
capability so that both sides use the same algorithm.
Signed-off-by: brian m. carlson <redacted>
---
t/t5702-protocol-v2.sh | 2 ++
1 file changed, 2 insertions(+)
@@ -13,6 +13,7 @@ start_git_daemon --export-all --enable=receive-packdaemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent test_expect_success'create repo to be served by git-daemon''+test_oid_init&&gitinit"$daemon_parent"&&test_commit-C"$daemon_parent"one'
@@ -394,6 +395,7 @@ test_expect_success 'even with handcrafted request, filter does not work if not# Custom request that tries to filter even though it is not advertised.test-toolpkt-linepack>in<<-EOF&&command=fetch+object-format=$(test_oidalgo)0001want$(git-Cserverrev-parsemaster)filterblob:none
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:12
test-sha1-array uses the_hash_algo under the hood. Since t0064 wants to
use the value that is correct for the hash algorithm that we're testing,
make sure the test helper initializes the repository to set
the_hash_algo correctly.
Signed-off-by: brian m. carlson <redacted>
---
t/helper/test-oid-array.c | 3 +++
1 file changed, 3 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:16
In a future patch, we'll want to access multiple members from struct
packet_reader when parsing references. Therefore, have the ref parsing
code take pointers to struct reader instead of having to pass multiple
arguments to each function.
Signed-off-by: brian m. carlson <redacted>
---
connect.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:18
When connecting to a remote system, we need to know what hash algorithm
it will be using to talk to us. Add a hash_algo member to struct
transport and add a function to read this data from the transport
object.
Signed-off-by: brian m. carlson <redacted>
---
transport.c | 8 ++++++++
transport.h | 8 ++++++++
2 files changed, 16 insertions(+)
@@ -311,6 +311,7 @@ static struct ref *handshake(struct transport *transport, int for_push,BUG("unknown protocol version");}data->got_remote_heads=1;+transport->hash_algo=reader.hash_algo;if(reader.line_peeked)BUG("buffer must be empty at the end of handshake()");
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:19
Update the remote helper docs to document the object-format extensions
we will implement in remote-curl and the transport helper code shortly.
Signed-off-by: brian m. carlson <redacted>
---
Documentation/gitremote-helpers.txt | 33 +++++++++++++++++++++++++----
1 file changed, 29 insertions(+), 4 deletions(-)
@@ -238,6 +238,9 @@ the remote repository. `--signed-tags=verbatim` to linkgit:git-fast-export[1]. In the absence of this capability, Git will use `--signed-tags=warn-strip`.+'object-format'::+ This indicates that the helper is able to interact with the remote+ side using an explicit hash algorithm extension. COMMANDS
@@ -257,12 +260,14 @@ Support for this command is mandatory. 'list':: Lists the refs, one per line, in the format "<value> <name> [<attr> ...]". The value may be a hex sha1 hash, "@<dest>" for- a symref, or "?" to indicate that the helper could not get the- value of the ref. A space-separated list of attributes follows- the name; unrecognized attributes are ignored. The list ends- with a blank line.+ a symref, ":<keyword> <value>" for a key-value pair, or+ "?" to indicate that the helper could not get the value of the+ ref. A space-separated list of attributes follows the name;+ unrecognized attributes are ignored. The list ends with a+ blank line. + See REF LIST ATTRIBUTES for a list of currently defined attributes.+See REF LIST KEYWORDS for a list of currently defined keywords. + Supported if the helper has the "fetch" or "import" capability.
@@ -430,6 +435,18 @@ attributes are defined. This ref is unchanged since the last import or fetch, although the helper cannot necessarily determine what value that produced.+REF LIST KEYWORDS+-----------------++The 'list' command may produce a list of key-value pairs.+The following keys are defined.++'object-format'::+ The refs are using the given hash algorithm. This keyword is only+ used if the server and client both support the object-format+ extension.++ OPTIONS -------
@@ -514,6 +531,14 @@ set by Git if the remote helper has the 'option' capability. transaction. If successful, all refs will be updated, or none will. If the remote side does not support this capability, the push will fail.+'option object-format' {'true'|algorithm}::+ If 'true', indicate that the caller wants hash algorithm information+ to be passed back from the remote. This mode is used when fetching+ refs.+++If set to an algorithm, indicate that the caller wants to interact with+the remote side using that algorithm.+ SEE ALSO -------- linkgit:git-remote[1]
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:21
When performing a clone, we don't know what hash algorithm the other end
will support. Currently, we don't support fetching data belonging to a
different algorithm, so we must know what algorithm the remote side is
using in order to properly initialize the repository. We can know that
only after fetching the refs, so if the remote side has any references,
use that information to reinitialize the repository with the correct
hash algorithm information.
Signed-off-by: brian m. carlson <redacted>
---
builtin/clone.c | 9 +++++++++
1 file changed, 9 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:21
If we're fetching refs, detect the hash algorithm and parse the refs
using that algorithm.
Signed-off-by: brian m. carlson <redacted>
---
connect.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
@@ -258,7 +270,7 @@ static int process_ref(const struct packet_reader *reader, int len,structobject_idold_oid;constchar*name;-if(parse_oid_hex(line,&old_oid,&name))+if(parse_oid_hex_algop(line,&old_oid,&name,reader->hash_algo))return0;if(*name!=' ')return0;
@@ -270,7 +282,7 @@ static int process_ref(const struct packet_reader *reader, int len,die(_("protocol error: unexpected capabilities^{}"));}elseif(check_ref(name,flags)){structref*ref=alloc_ref(name);-oidcpy(&ref->old_oid,&old_oid);+memcpy(ref->old_oid.hash,old_oid.hash,reader->hash_algo->rawsz);**list=ref;*list=&ref->next;}
@@ -288,7 +300,7 @@ static int process_shallow(const struct packet_reader *reader, int len,if(!skip_prefix(line,"shallow ",&arg))return0;-if(get_oid_hex(arg,&old_oid))+if(get_oid_hex_algop(arg,&old_oid,reader->hash_algo))die(_("protocol error: expected shallow sha-1, got '%s'"),arg);if(!shallow_points)die(_("repository on the other end cannot be shallow"));
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:23
Advertise the current hash algorithm in use by using the object-format
capability as part of the ref advertisement.
Signed-off-by: brian m. carlson <redacted>
---
builtin/receive-pack.c | 1 +
upload-pack.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:26
It's possible to use a variety of index formats with show-index, and we
need a way to indicate the hash algorithm which is in use for a
particular index we'd like to show. Default to using the value for the
repository we're in by calling setup_git_directory_gently, and allow
overriding it by using a --hash argument.
Signed-off-by: brian m. carlson <redacted>
---
builtin/show-index.c | 29 ++++++++++++++++++++++++-----
git.c | 2 +-
2 files changed, 25 insertions(+), 6 deletions(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:26
This test has hard-coded pkt-lines with object IDs. The pkt-line
lengths necessarily differ between hash algorithms, so generate these
lines with the packetize helper so they're always the right size. In
addition, we will require an object-format capability for SHA-256, so
pass that capability on to the upload-pack process.
Signed-off-by: brian m. carlson <redacted>
---
t/t5500-fetch-pack.sh | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:30
Normally, the remote-curl transport helper is aware of the hash
algorithm we're using because we're in a repo with the appropriate hash
algorithm set. However, when using git ls-remote outside of a
repository, we won't have initialized the hash algorithm properly, so
use hash_to_hex_algop to print the ref corresponding to the algorithm
we've detected.
Signed-off-by: brian m. carlson <redacted>
---
remote-curl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:31
When reading the info/refs file for a repository, we have no explicit
way to detect which hash algorithm is in use because the file doesn't
provide one. Detect the hash algorithm in use by the size of the first
object ID.
Signed-off-by: brian m. carlson <redacted>
---
remote-curl.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
@@ -262,6 +275,12 @@ static struct ref *parse_info_refs(struct discovery *heads)structref*ref=NULL;structref*last_ref=NULL;+options.hash_algo=detect_hash_algo(heads);+if(!options.hash_algo)+die("%sinfo/refs not valid: could not determine hash algorithm; "+"is this a git repository?",+url.buf);+data=heads->buf;start=NULL;mid=data;
@@ -272,13 +291,13 @@ static struct ref *parse_info_refs(struct discovery *heads)if(data[i]=='\t')mid=&data[i];if(data[i]=='\n'){-if(mid-start!=the_hash_algo->hexsz)+if(mid-start!=options.hash_algo->hexsz)die(_("%sinfo/refs not valid: is this a git repository?"),transport_anonymize_url(url.buf));data[i]=0;ref_name=mid+1;ref=alloc_ref(ref_name);-get_oid_hex(start,&ref->old_oid);+get_oid_hex_algop(start,&ref->old_oid,options.hash_algo);if(!refs)refs=ref;if(last_ref)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:32
When outside a repository, git index-pack is unable to guess the hash
algorithm in use for a pack, since packs don't contain any information
on the algorithm in use. Pass an option to index-pack to help it out in
this test.
Signed-off-by: brian m. carlson <redacted>
---
t/t1050-large.sh | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -12,6 +12,7 @@ file_size () {} test_expect_successsetup'+test_oid_init&&# clone does not allow us to pass core.bigfilethreshold to# new repos, so set core.bigfilethreshold globallygitconfig--globalcore.bigfilethreshold200k&&
@@ -177,7 +178,8 @@ test_expect_success 'git-show a large file' ' test_expect_success'index-pack''gitclonefile://"$(pwd)"/.gitfoo&&-GIT_DIR=non-existentgitindex-pack--strict--verifyfoo/.git/objects/pack/*.pack+GIT_DIR=non-existentgitindex-pack--object-format=$(test_oidalgo)\+--strict--verifyfoo/.git/objects/pack/*.pack' test_expect_success'repack''
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:36
ls-remote may or may not operate within a repository, and as such will
not have been initialized with the repository's hash algorithm. Even if
it were, the remote side could be using a different algorithm and we
would still want to display those refs properly. Find the hash
algorithm used by the remote side by querying the transport object and
set our hash algorithm accordingly.
Without this change, if the remote side is using SHA-256, we truncate
the refs to 40 hex characters, since that's the length of the default
hash algorithm (SHA-1).
Signed-off-by: brian m. carlson <redacted>
---
builtin/ls-remote.c | 4 ++++
1 file changed, 4 insertions(+)
From: brian m. carlson <hidden> Date: 2020-05-13 00:55:59
When using protocol v2, we need to know what hash algorithm is used by
the remote end. See if the server has sent us an object-format
capability, and if so, use it to determine the hash algorithm in use and
set that value in the packet reader. Parse the refs using this
algorithm.
Note that we use memcpy instead of oidcpy for copying values, since
oidcpy is intentionally limited to the current hash algorithm length,
and the copy will be too short if the server side uses SHA-256 but the
client side has not had a repository set up (and therefore defaults to
SHA-1).
Signed-off-by: brian m. carlson <redacted>
---
connect.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:56:02
Detect when the server doesn't support our hash algorithm and abort.
Signed-off-by: brian m. carlson <redacted>
---
send-pack.c | 6 ++++++
1 file changed, 6 insertions(+)
@@ -362,6 +362,7 @@ int send_pack(struct send_pack_args *args,intatomic_supported=0;intuse_push_options=0;intpush_options_supported=0;+intobject_format_supported=0;unsignedcmds_sent=0;intret;structasyncdemux;
@@ -388,6 +389,9 @@ int send_pack(struct send_pack_args *args,if(server_supports("push-options"))push_options_supported=1;+if(!server_supports_hash(the_hash_algo->name,&object_format_supported))+die(_("the receiving end does not support this repository's hash algorithm"));+if(args->push_cert!=SEND_PACK_PUSH_CERT_NEVER){intlen;push_cert_nonce=server_feature_value("push-cert",&len);
From: brian m. carlson <hidden> Date: 2020-05-13 00:56:03
When using SHA-256, we need to take advantage of the extensions section
in the config file, so we need to use repository format version 1.
Update the test to look for the correct value.
Note that test_oid produces a value without a trailing newline, so use
echo to ensure we print a trailing newline to compare it correctly
against the actual results.
Signed-off-by: brian m. carlson <redacted>
---
t/t1302-repo-version.sh | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
From: brian m. carlson <hidden> Date: 2020-05-13 00:56:04
Our style these days is to place the description and the opening quote
of the body on the same line as test_expect_success (if it fits), to
place the trailing quote on a line by itself after the body, and to use
tabs. Since we're going to be making several significant changes to
this test, modernize the style to aid in readability of the subsequent
patches.
This patch should have no functional change.
Signed-off-by: brian m. carlson <redacted>
---
t/t5302-pack-index.sh | 360 +++++++++++++++++++++---------------------
1 file changed, 184 insertions(+), 176 deletions(-)
@@ -7,65 +7,65 @@ test_description='pack index with 64-bit offsets and object CRC' ../test-lib.sh test_expect_success'setup''-test_oid_init&&-rawsz=$(test_oidrawsz)&&-rm-rf.git&&-gitinit&&-gitconfigpack.threads1&&-i=1&&-whiletest$i-le100-do-iii=$(printf'%03i'$i)-test-toolgenrandom"bar"200>wide_delta_$iii&&-test-toolgenrandom"baz $iii"50>>wide_delta_$iii&&-test-toolgenrandom"foo"$i100>deep_delta_$iii&&-test-toolgenrandom"foo"$(expr$i+1)100>>deep_delta_$iii&&-test-toolgenrandom"foo"$(expr$i+2)100>>deep_delta_$iii&&-echo$iii>file_$iii&&-test-toolgenrandom"$iii"8192>>file_$iii&&-gitupdate-index--addfile_$iiideep_delta_$iiiwide_delta_$iii&&-i=$(expr$i+1)||return1-done&&-{echo101&&test-toolgenrandom1008192;}>file_101&&-gitupdate-index--addfile_101&&-tree=$(gitwrite-tree)&&-commit=$(gitcommit-tree$tree</dev/null)&&{-echo$tree&&-gitls-tree$tree|sed-e"s/.* \\([0-9a-f]*\\) .*/\\1/"-}>obj-list&&-gitupdate-refHEAD$commit+test_oid_init&&+rawsz=$(test_oidrawsz)&&+rm-rf.git&&+gitinit&&+gitconfigpack.threads1&&+i=1&&+whiletest$i-le100+do+iii=$(printf'%03i'$i)+test-toolgenrandom"bar"200>wide_delta_$iii&&+test-toolgenrandom"baz $iii"50>>wide_delta_$iii&&+test-toolgenrandom"foo"$i100>deep_delta_$iii&&+test-toolgenrandom"foo"$(expr$i+1)100>>deep_delta_$iii&&+test-toolgenrandom"foo"$(expr$i+2)100>>deep_delta_$iii&&+echo$iii>file_$iii&&+test-toolgenrandom"$iii"8192>>file_$iii&&+gitupdate-index--addfile_$iiideep_delta_$iiiwide_delta_$iii&&+i=$(expr$i+1)||return1+done&&+{echo101&&test-toolgenrandom1008192;}>file_101&&+gitupdate-index--addfile_101&&+tree=$(gitwrite-tree)&&+commit=$(gitcommit-tree$tree</dev/null)&&{+echo$tree&&+gitls-tree$tree|sed-e"s/.* \\([0-9a-f]*\\) .*/\\1/"+}>obj-list&&+gitupdate-refHEAD$commit'-test_expect_success\-'pack-objects with index version 1'\-'pack1=$(gitpack-objects--index-version=1test-1<obj-list)&&-gitverify-pack-v"test-1-${pack1}.pack"'+test_expect_success'pack-objects with index version 1''+pack1=$(gitpack-objects--index-version=1test-1<obj-list)&&+gitverify-pack-v"test-1-${pack1}.pack"+'-test_expect_success\-'pack-objects with index version 2'\-'pack2=$(gitpack-objects--index-version=2test-2<obj-list)&&-gitverify-pack-v"test-2-${pack2}.pack"'+test_expect_success'pack-objects with index version 2''+pack2=$(gitpack-objects--index-version=2test-2<obj-list)&&+gitverify-pack-v"test-2-${pack2}.pack"+'-test_expect_success\-'both packs should be identical'\-'cmp "test-1-${pack1}.pack" "test-2-${pack2}.pack"'+test_expect_success'both packs should be identical''+cmp"test-1-${pack1}.pack""test-2-${pack2}.pack"+'-test_expect_success\-'index v1 and index v2 should be different'\-'! cmp "test-1-${pack1}.idx" "test-2-${pack2}.idx"'+test_expect_success'index v1 and index v2 should be different''+!cmp"test-1-${pack1}.idx""test-2-${pack2}.idx"+'-test_expect_success\-'index-pack with index version 1'\-'git index-pack --index-version=1 -o 1.idx "test-1-${pack1}.pack"'+test_expect_success'index-pack with index version 1''+gitindex-pack--index-version=1-o1.idx"test-1-${pack1}.pack"+'-test_expect_success\-'index-pack with index version 2'\-'git index-pack --index-version=2 -o 2.idx "test-1-${pack1}.pack"'+test_expect_success'index-pack with index version 2''+gitindex-pack--index-version=2-o2.idx"test-1-${pack1}.pack"+'-test_expect_success\-'index-pack results should match pack-objects ones'\-'cmp"test-1-${pack1}.idx""1.idx"&&-cmp"test-2-${pack2}.idx""2.idx"'+test_expect_success'index-pack results should match pack-objects ones''+cmp"test-1-${pack1}.idx""1.idx"&&+cmp"test-2-${pack2}.idx""2.idx"+' test_expect_success'index-pack --verify on index version 1''gitindex-pack--verify"test-1-${pack1}.pack"
@@ -75,13 +75,13 @@ test_expect_success 'index-pack --verify on index version 2' 'gitindex-pack--verify"test-2-${pack2}.pack"'-test_expect_success\-'pack-objects --index-version=2, is not accepted'\-'test_must_fail git pack-objects --index-version=2, test-3 <obj-list'+test_expect_success'pack-objects --index-version=2, is not accepted''+test_must_failgitpack-objects--index-version=2,test-3<obj-list+'-test_expect_success\-'index v2: force some 64-bit offsets with pack-objects'\-'pack3=$(git pack-objects --index-version=2,0x40000 test-3 <obj-list)'+test_expect_success'index v2: force some 64-bit offsets with pack-objects''+pack3=$(gitpack-objects--index-version=2,0x40000test-3<obj-list)+'ifmsg=$(gitverify-pack-v"test-3-${pack3}.pack"2>&1)||!(echo"$msg"|grep"pack too large .* off_t")
@@ -91,21 +91,21 @@ elsesay"# skipping tests concerning 64-bit offsets"fi-test_expect_successOFF64_T\-'index v2: verify a pack with some 64-bit offsets'\-'git verify-pack -v "test-3-${pack3}.pack"'+test_expect_successOFF64_T'index v2: verify a pack with some 64-bit offsets''+gitverify-pack-v"test-3-${pack3}.pack"+'-test_expect_successOFF64_T\-'64-bit offsets: should be different from previous index v2 results'\-'! cmp "test-2-${pack2}.idx" "test-3-${pack3}.idx"'+test_expect_successOFF64_T'64-bit offsets: should be different from previous index v2 results''+!cmp"test-2-${pack2}.idx""test-3-${pack3}.idx"+'-test_expect_successOFF64_T\-'index v2: force some 64-bit offsets with index-pack'\-'git index-pack --index-version=2,0x40000 -o 3.idx "test-1-${pack1}.pack"'+test_expect_successOFF64_T'index v2: force some 64-bit offsets with index-pack''+gitindex-pack--index-version=2,0x40000-o3.idx"test-1-${pack1}.pack"+'-test_expect_successOFF64_T\-'64-bit offsets: index-pack result should match pack-objects one'\-'cmp "test-3-${pack3}.idx" "3.idx"'+test_expect_successOFF64_T'64-bit offsets: index-pack result should match pack-objects one''+cmp"test-3-${pack3}.idx""3.idx"+' test_expect_successOFF64_T'index-pack --verify on 64-bit offset v2 (cheat)''# This cheats by knowing which lower offset should still be encoded
@@ -120,135 +120,143 @@ test_expect_success OFF64_T 'index-pack --verify on 64-bit offset v2' '# returns the object number for given object in given pack index index_obj_nr(){-idx_file=$1-object_sha1=$2-nr=0-gitshow-index<$idx_file|-whilereadoffssha1extra-do-nr=$(($nr+1))-test"$sha1"="$object_sha1"||continue-echo"$(($nr-1))"-break-done+idx_file=$1+object_sha1=$2+nr=0+gitshow-index<$idx_file|+whilereadoffssha1extra+do+nr=$(($nr+1))+test"$sha1"="$object_sha1"||continue+echo"$(($nr-1))"+break+done}# returns the pack offset for given object as found in given pack index index_obj_offset(){-idx_file=$1-object_sha1=$2-gitshow-index<$idx_file|grep$object_sha1|-(readoffsextra&&echo"$offs")+idx_file=$1+object_sha1=$2+gitshow-index<$idx_file|grep$object_sha1|+(readoffsextra&&echo"$offs")}-test_expect_success\-'[index v1] 1) stream pack to repository'\-'gitindex-pack--index-version=1--stdin<"test-1-${pack1}.pack"&&-gitprune-packed&&-gitcount-objects|(readnrrest&&test"$nr"-eq1)&&-cmp"test-1-${pack1}.pack"".git/objects/pack/pack-${pack1}.pack"&&-cmp"test-1-${pack1}.idx"".git/objects/pack/pack-${pack1}.idx"'+test_expect_success'[index v1] 1) stream pack to repository''+gitindex-pack--index-version=1--stdin<"test-1-${pack1}.pack"&&+gitprune-packed&&+gitcount-objects|(readnrrest&&test"$nr"-eq1)&&+cmp"test-1-${pack1}.pack"".git/objects/pack/pack-${pack1}.pack"&&+cmp"test-1-${pack1}.idx"".git/objects/pack/pack-${pack1}.idx"+' test_expect_success\-'[index v1] 2) create a stealth corruption in a delta base reference'\-'# This test assumes file_101 is a delta smaller than 16 bytes.-# It should be against file_100 but we substitute its base for file_099-sha1_101=$(githash-objectfile_101)&&-sha1_099=$(githash-objectfile_099)&&-offs_101=$(index_obj_offset1.idx$sha1_101)&&-nr_099=$(index_obj_nr1.idx$sha1_099)&&-chmod+w".git/objects/pack/pack-${pack1}.pack"&&-recordsz=$((rawsz+4))&&-ddof=".git/objects/pack/pack-${pack1}.pack"seek=$(($offs_101+1))\-if=".git/objects/pack/pack-${pack1}.idx"\-skip=$((4+256*4+$nr_099*recordsz))\-bs=1count=$rawszconv=notrunc&&-gitcat-fileblob$sha1_101>file_101_foo1'+'[index v1] 2) create a stealth corruption in a delta base reference''+# This test assumes file_101 is a delta smaller than 16 bytes.+# It should be against file_100 but we substitute its base for file_099+sha1_101=$(githash-objectfile_101)&&+sha1_099=$(githash-objectfile_099)&&+offs_101=$(index_obj_offset1.idx$sha1_101)&&+nr_099=$(index_obj_nr1.idx$sha1_099)&&+chmod+w".git/objects/pack/pack-${pack1}.pack"&&+recordsz=$((rawsz+4))&&+ddof=".git/objects/pack/pack-${pack1}.pack"seek=$(($offs_101+1))\+if=".git/objects/pack/pack-${pack1}.idx"\+skip=$((4+256*4+$nr_099*recordsz))\+bs=1count=$rawszconv=notrunc&&+gitcat-fileblob$sha1_101>file_101_foo1+' test_expect_success\-'[index v1] 3) corrupted delta happily returned wrong data'\-'test -f file_101_foo1 && ! cmp file_101 file_101_foo1'+'[index v1] 3) corrupted delta happily returned wrong data''+test-ffile_101_foo1&&!cmpfile_101file_101_foo1+' test_expect_success\-'[index v1] 4) confirm that the pack is actually corrupted'\-'test_must_fail git fsck --full $commit'+'[index v1] 4) confirm that the pack is actually corrupted''+test_must_failgitfsck--full$commit+' test_expect_success\-'[index v1] 5) pack-objects happily reuses corrupted data'\-'pack4=$(gitpack-objectstest-4<obj-list)&&-test-f"test-4-${pack4}.pack"'+'[index v1] 5) pack-objects happily reuses corrupted data''+pack4=$(gitpack-objectstest-4<obj-list)&&+test-f"test-4-${pack4}.pack"+'++test_expect_success'[index v1] 6) newly created pack is BAD !''+test_must_failgitverify-pack-v"test-4-${pack4}.pack"+'++test_expect_success'[index v2] 1) stream pack to repository''+rm-f.git/objects/pack/*&&+gitindex-pack--index-version=2--stdin<"test-1-${pack1}.pack"&&+gitprune-packed&&+gitcount-objects|(readnrrest&&test"$nr"-eq1)&&+cmp"test-1-${pack1}.pack"".git/objects/pack/pack-${pack1}.pack"&&+cmp"test-2-${pack1}.idx"".git/objects/pack/pack-${pack1}.idx"+' test_expect_success\-'[index v1] 6) newly created pack is BAD !'\-'test_must_fail git verify-pack -v "test-4-${pack4}.pack"'+'[index v2] 2) create a stealth corruption in a delta base reference''+# This test assumes file_101 is a delta smaller than 16 bytes.+# It should be against file_100 but we substitute its base for file_099+sha1_101=$(githash-objectfile_101)&&+sha1_099=$(githash-objectfile_099)&&+offs_101=$(index_obj_offset1.idx$sha1_101)&&+nr_099=$(index_obj_nr1.idx$sha1_099)&&+chmod+w".git/objects/pack/pack-${pack1}.pack"&&+ddof=".git/objects/pack/pack-${pack1}.pack"seek=$(($offs_101+1))\+if=".git/objects/pack/pack-${pack1}.idx"\+skip=$((8+256*4+$nr_099*rawsz))\+bs=1count=$rawszconv=notrunc&&+gitcat-fileblob$sha1_101>file_101_foo2+' test_expect_success\-'[index v2] 1) stream pack to repository'\-'rm-f.git/objects/pack/*&&-gitindex-pack--index-version=2--stdin<"test-1-${pack1}.pack"&&-gitprune-packed&&-gitcount-objects|(readnrrest&&test"$nr"-eq1)&&-cmp"test-1-${pack1}.pack"".git/objects/pack/pack-${pack1}.pack"&&-cmp"test-2-${pack1}.idx"".git/objects/pack/pack-${pack1}.idx"'+'[index v2] 3) corrupted delta happily returned wrong data''+test-ffile_101_foo2&&!cmpfile_101file_101_foo2+' test_expect_success\-'[index v2] 2) create a stealth corruption in a delta base reference'\-'# This test assumes file_101 is a delta smaller than 16 bytes.-# It should be against file_100 but we substitute its base for file_099-sha1_101=$(githash-objectfile_101)&&-sha1_099=$(githash-objectfile_099)&&-offs_101=$(index_obj_offset1.idx$sha1_101)&&-nr_099=$(index_obj_nr1.idx$sha1_099)&&-chmod+w".git/objects/pack/pack-${pack1}.pack"&&-ddof=".git/objects/pack/pack-${pack1}.pack"seek=$(($offs_101+1))\-if=".git/objects/pack/pack-${pack1}.idx"\-skip=$((8+256*4+$nr_099*rawsz))\-bs=1count=$rawszconv=notrunc&&-gitcat-fileblob$sha1_101>file_101_foo2'+'[index v2] 4) confirm that the pack is actually corrupted''+test_must_failgitfsck--full$commit+' test_expect_success\-'[index v2] 3) corrupted delta happily returned wrong data'\-'test -f file_101_foo2 && ! cmp file_101 file_101_foo2'+'[index v2] 5) pack-objects refuses to reuse corrupted data''+test_must_failgitpack-objectstest-5<obj-list&&+test_must_failgitpack-objects--no-reuse-objecttest-6<obj-list+' test_expect_success\-'[index v2] 4) confirm that the pack is actually corrupted'\-'test_must_fail git fsck --full $commit'--test_expect_success\-'[index v2] 5) pack-objects refuses to reuse corrupted data'\-'test_must_failgitpack-objectstest-5<obj-list&&-test_must_failgitpack-objects--no-reuse-objecttest-6<obj-list'--test_expect_success\-'[index v2] 6) verify-pack detects CRC mismatch'\-'rm-f.git/objects/pack/*&&-gitindex-pack--index-version=2--stdin<"test-1-${pack1}.pack"&&-gitverify-pack".git/objects/pack/pack-${pack1}.pack"&&-obj=$(githash-objectfile_001)&&-nr=$(index_obj_nr".git/objects/pack/pack-${pack1}.idx"$obj)&&-chmod+w".git/objects/pack/pack-${pack1}.idx"&&-printfxxxx|ddof=".git/objects/pack/pack-${pack1}.idx"conv=notrunc\-bs=1count=4seek=$((8+256*4+$(wc-l<obj-list)*rawsz+$nr*4))&&-(whilereadobj-dogitcat-file-p$obj>/dev/null||exit1-done<obj-list)&&-test_must_failgitverify-pack".git/objects/pack/pack-${pack1}.pack"+'[index v2] 6) verify-pack detects CRC mismatch''+rm-f.git/objects/pack/*&&+gitindex-pack--index-version=2--stdin<"test-1-${pack1}.pack"&&+gitverify-pack".git/objects/pack/pack-${pack1}.pack"&&+obj=$(githash-objectfile_001)&&+nr=$(index_obj_nr".git/objects/pack/pack-${pack1}.idx"$obj)&&+chmod+w".git/objects/pack/pack-${pack1}.idx"&&+printfxxxx|ddof=".git/objects/pack/pack-${pack1}.idx"conv=notrunc\+bs=1count=4seek=$((8+256*4+$(wc-l<obj-list)*rawsz+$nr*4))&&+(whilereadobj+dogitcat-file-p$obj>/dev/null||exit1+done<obj-list)&&+test_must_failgitverify-pack".git/objects/pack/pack-${pack1}.pack"' test_expect_success'running index-pack in the object store''-rm-f.git/objects/pack/*&&-cptest-1-${pack1}.pack.git/objects/pack/pack-${pack1}.pack&&-(-cd.git/objects/pack&&-gitindex-packpack-${pack1}.pack-)&&-test-f.git/objects/pack/pack-${pack1}.idx+rm-f.git/objects/pack/*&&+cptest-1-${pack1}.pack.git/objects/pack/pack-${pack1}.pack&&+(+cd.git/objects/pack&&+gitindex-packpack-${pack1}.pack+)&&+test-f.git/objects/pack/pack-${pack1}.idx' test_expect_success'index-pack --strict warns upon missing tagger in tag''-sha=$(gitrev-parseHEAD)&&-cat>wrong-tag<<EOF&&+sha=$(gitrev-parseHEAD)&&+cat>wrong-tag<<EOF&& object$shatypecommit taggutentag
@@ -256,18 +264,18 @@ tag guten tag Thisisaninvalidtag. EOF-tag=$(githash-object-ttag-w--stdin<wrong-tag)&&-pack1=$(echo$tag$sha|gitpack-objectstag-test)&&-echoremovetagobject&&-thirtyeight=${tag#??}&&-rm-f.git/objects/${tag%$thirtyeight}/$thirtyeight&&-gitindex-pack--stricttag-test-${pack1}.pack2>err&&-grep"^warning:.* expected .tagger. line"err+tag=$(githash-object-ttag-w--stdin<wrong-tag)&&+pack1=$(echo$tag$sha|gitpack-objectstag-test)&&+echoremovetagobject&&+thirtyeight=${tag#??}&&+rm-f.git/objects/${tag%$thirtyeight}/$thirtyeight&&+gitindex-pack--stricttag-test-${pack1}.pack2>err&&+grep"^warning:.* expected .tagger. line"err' test_expect_success'index-pack --fsck-objects also warns upon missing tagger in tag''-gitindex-pack--fsck-objectstag-test-${pack1}.pack2>err&&-grep"^warning:.* expected .tagger. line"err+gitindex-pack--fsck-objectstag-test-${pack1}.pack2>err&&+grep"^warning:.* expected .tagger. line"err' test_done
@@ -453,3 +453,12 @@ included in a request. This is done by sending each option as a a request. The provided options must not contain a NUL or LF character.++ object-format+~~~~~~~~~~~~~~~++The server can advertise the `object-format` capability with a value `X` (in the+form `object-format=X`) to notify the client that the server is able to deal+with objects using hash algorithm X. If not specified, the server is assumed to+only handle SHA-1. If the client would like to use a hash algorithm other than+SHA-1, it should specify its object-format string.
From: brian m. carlson <hidden> Date: 2020-05-13 00:56:09
Implement the object-format extensions that let us determine the hash
algorithm in use when pushing or pulling data.
Signed-off-by: brian m. carlson <redacted>
---
transport-helper.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
From: Martin Ågren <hidden> Date: 2020-05-13 19:29:00
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
quoted hunk
@@ -189,7 +204,6 @@ refs being sent. Clients MAY use the parameters from this capability to select the proper initial branch when cloning a repository.- shallow -------
Looks like a spurious line deletion snuck in.
Martin
From: Martin Ågren <hidden> Date: 2020-05-13 19:30:37
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
In a future patch, we'll want to access multiple members from struct
packet_reader when parsing references. Therefore, have the ref parsing
code take pointers to struct reader instead of having to pass multiple
arguments to each function.
Makes sense.
-static void process_capabilities(const char *line, int *len)
+static void process_capabilities(struct packet_reader *reader, int *len)
{
+ const char *line = reader->line;
int nul_location = strlen(line);
if (nul_location == *len)
return;
"line+len" made it pretty obvious that they belonged together.
"reader+len" not so much. Your patch does minimize the change. Would
s/len/linelen/ be worth the extra churn? Possibly not. Right now, at
least we're pretty consistent about using "len" -- if this ends up as a
mixture of "linelen" and "len" I think it's worse, overall.
Martin
It's not entirely obvious from the context, but this function is
inserted between some "tmp" stuff and some other "tmp" stuff. I don't
think we need to bikeshed its exact home, but maybe "close to other
string stuff", or at least not in the middle of the "tmp" section.
Martin
From: Martin Ågren <hidden> Date: 2020-05-13 19:37:30
On Wed, 13 May 2020 at 02:58, brian m. carlson
[off-list ref] wrote:
+int server_feature_v2(const char *c, const char **v)
+{
+ int i;
+
+ for (i = 0; i < server_capabilities_v2.argc; i++) {
+ const char *out;
+ if (skip_prefix(server_capabilities_v2.argv[i], c, &out) &&
+ (*out == '=')) {
+ *v = out + 1;
+ return 1;
+ }
+ }
+ return 0;
+}
+
This looks like it was based on `server_supports_feature()`, which
explains the "1 means yup got it, 0 means no match". The name of
`server_supports_feature()` does suggest the boolean nature of return
value. For this new function, I would perhaps have expected "0 means
success, negative means error". That said, I'm not familiar with
connect.c. Let's see how this is used...
int server_supports_feature(const char *c, const char *feature,
int die_on_error)
{
Just a thought:
Maybe this existing function could learn to take a pointer (or NULL) and
assign to it if we have a '=' (possibly even requiring a '=' if this new
pointer is non-NULL). I dunno, maybe two similar functions are better
after all than having one with modes like that.
Martin
From: Martin Ågren <hidden> Date: 2020-05-13 19:39:54
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
Add a function, server_supports_hash, to see if the remote server
supports a particular hash algorithm when speaking protocol v1.
+int server_supports_hash(const char *desired, int *feature_supported)
+{
+ int offset = 0;
+ int len, found = 0;
+ const char *hash;
+
+ hash = next_server_feature_value("object-format", &len, &offset);
+ if (feature_supported)
+ *feature_supported = !!hash;
If we got something, anything, the server supports this feature. It just
remains to see if it supports the exact algorithm we're after.
+ if (!hash) {
+ hash = hash_algos[GIT_HASH_SHA1].name;
+ len = strlen(hash);
+ }
OK, if the server doesn't say anything, we fall back to SHA-1. If it's
the desired one, we'll return 1 accordingly below.
+ while (hash) {
+ if (!xstrncmpz(desired, hash, len))
+ found = 1;
+
+ if (found)
+ return 1;
I first thought this structure was because this loop body would learn to
do something else later in the series. But this is it. This looks like
it could just be "if (!xstrncmpz(...)) return 1;" and drop "found".
From: Martin Ågren <hidden> Date: 2020-05-13 19:41:28
On Wed, 13 May 2020 at 02:58, brian m. carlson
[off-list ref] wrote:
Detect when the server doesn't support our hash algorithm and abort.
+ if (!server_supports_hash(the_hash_algo->name, &object_format_supported))
+ die(_("the receiving end does not support this repository's hash algorithm"));
I suppose this isn't the long-term wanted behavior? Would this be where
we would later learn to realize that "oh, crap, we need to
convert/translate on the fly"?
quoted hunk
@@ -428,6 +432,8 @@ int send_pack(struct send_pack_args *args, strbuf_addstr(&cap_buf, " atomic"); if (use_push_options) strbuf_addstr(&cap_buf, " push-options");+ if (object_format_supported)+ strbuf_addf(&cap_buf, " object-format=%s", the_hash_algo->name);
This isn't advertised in the log message: If we do detect support, go on
to reply with our choice of object format / hash algo name.
Martin
From: Martin Ågren <hidden> Date: 2020-05-13 19:48:34
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
We're going to be using this function in other files, so no longer mark
this function static.
static char *server_capabilities_v1;
static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;
-static const char *parse_feature_value(const char *, const char *, int *, int *);
static const char *next_server_feature_value(const char *feature, int *len, int *offset);
-static const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)
+const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)
{
quoted hunk
--- a/connect.h+++ b/connect.h
+const char *parse_feature_value(const char *, const char *, int *, int *);
This "char *, int *" comes from the forward-declaration above, which is
now dropped. Now that this is a header file for everyone to use, I think
these parameters should be named, at least, but even better would be
some documentation. ;-)
I'll stop reading here. I'm not familiar with the technical details here
(i.e., where you'd be most interested in review), so I've just left some
more or less superficial comments.
One thing I've noticed is that there are relatively few tests so far. I
suppose it could be hard to trigger things before everything is properly
plugged through. But maybe at least various error paths could be
exercised already at this point, such as in the previous patch I
commented on.
So far I feel like I'm following along ok and I have a feeling I know
where this is leading up to. Nicely done so far.
Martin
From: brian m. carlson <hidden> Date: 2020-05-13 22:49:49
On 2020-05-13 at 19:39:41, Martin Ågren wrote:
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
quoted
+ while (hash) {
+ if (!xstrncmpz(desired, hash, len))
+ found = 1;
+
+ if (found)
+ return 1;
I first thought this structure was because this loop body would learn to
do something else later in the series. But this is it. This looks like
it could just be "if (!xstrncmpz(...)) return 1;" and drop "found".
Yeah, I think it could. I originally didn't have the helper and the
code was pretty hideous, so I probably forgot to simplify when I used
the helper again.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2020-05-13 22:52:29
On 2020-05-13 at 19:41:15, Martin Ågren wrote:
On Wed, 13 May 2020 at 02:58, brian m. carlson
[off-list ref] wrote:
quoted
Detect when the server doesn't support our hash algorithm and abort.
quoted
+ if (!server_supports_hash(the_hash_algo->name, &object_format_supported))
+ die(_("the receiving end does not support this repository's hash algorithm"));
I suppose this isn't the long-term wanted behavior? Would this be where
we would later learn to realize that "oh, crap, we need to
convert/translate on the fly"?
Yes, this would be the point at which we'd decide whether we could
support the remote side's algorithm and decide to rewrite objects. We
might still fail, such as if we're SHA-256 only without a lookup table
and the remote side is SHA-1, but theoretically we'd do the conversion
here.
quoted
@@ -428,6 +432,8 @@ int send_pack(struct send_pack_args *args, strbuf_addstr(&cap_buf, " atomic"); if (use_push_options) strbuf_addstr(&cap_buf, " push-options");+ if (object_format_supported)+ strbuf_addf(&cap_buf, " object-format=%s", the_hash_algo->name);
This isn't advertised in the log message: If we do detect support, go on
to reply with our choice of object format / hash algo name.
From: Martin Ågren <hidden> Date: 2020-05-16 10:40:30
On Wed, 13 May 2020 at 02:57, brian m. carlson
[off-list ref] wrote:
If we're fetching refs, detect the hash algorithm and parse the refs
using that algorithm.
As the added documentation from patch 2 says, if there are multiple
"object-format" capabilities, "the first one given is the one used in
the ref advertisement". And that's what you implement below.
Explaining that in this commit message and/or referring to "a recent
commit" (patch 2) and/or adding that documentation here, not back then,
would have avoided some confusion on my part, and perhaps also for
future readers.
I don't have a strong opinion on which of those is better, I just think
you could somehow make that a bit clearer here.
static void process_capabilities(struct packet_reader *reader, int *len)
{
+ const char *feat_val;
+ int feat_len;
+ int hash_algo;
xstrndup is needed because we're not guaranteed a terminating NUL. You
remember to call free afterwards. Ok.
If we don't get any "object-format", we do basically nothing here and
`reader->hash_algo` will remain as whatever it already is. The docs from
patch 2 promise that this will be handled as "SHA-1" -- would it be more
robust if we did a similar fallback dance as you do elsewhere?
feat_val = ...;
if (!feat_val) {
feat_val = hash_algos[GIT_HASH_SHA1].name;
feat_len = strlen(feat_val);
}
char *hash_name = ...
...
You do initialize `reader->hash_algo` in patch 8, so I don't think this
changes anything now. Maybe it's just premature future-proofing (if such
a thing exists).
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 10:41:15
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
+ const char *hash;
+ int len = 0;
Micronit: These new variables are used in tandem. I could see these as
"NULL, 0" or both uninitialized, but this is a mixture. That's a really
small nit, of course. (Maybe you needed to fight a compiler warning?)
+ hash = parse_feature_value(feature_list, "object-format", &len, NULL);
+ if (!hash) {
+ hash = hash_algos[GIT_HASH_SHA1].name;
+ len = strlen(hash);
+ }
+ if (xstrncmpz(the_hash_algo->name, hash, len))
+ die("error: unsupported object format '%s'", hash);
Ok, this is a familiar pattern by now: if we get nothing, behave as if
we got SHA-1.
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 10:48:59
On Wed, 13 May 2020 at 02:57, brian m. carlson
[off-list ref] wrote:
quoted hunk
When performing a clone, we don't know what hash algorithm the other end
will support. Currently, we don't support fetching data belonging to a
different algorithm, so we must know what algorithm the remote side is
using in order to properly initialize the repository. We can know that
only after fetching the refs, so if the remote side has any references,
use that information to reinitialize the repository with the correct
hash algorithm information.
Signed-off-by: brian m. carlson <redacted>
---
builtin/clone.c | 9 +++++++++
1 file changed, 9 insertions(+)
This made me go "huh". It's not really new in this series, it's just
that from `initialize_repository_version(int)` I would have expected the
argument to be the, well, repository version, not a hash algo
identifier. But it all makes sense once you realize that the function is
"please initialize the repository version based on this stuff that I
give you" where, currently, the only input is a hash algo. (I see that
Han-Wen's reftable series adds another parameter here.)
+ repo_set_hash_algo(the_repository, hash_algo);
I first wondered whether all calls to `repo_set_hash_algo()` would want
to be preceded by `initialize_repository_version()`, which might call
for the latter being called by the former. But I guess not. Various
users of `repo_set_hash_algo()` -- not that there would be a lot of them
-- might want to do similar updating and/or sanity checks, but the exact
details would differ.
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 10:55:46
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
Ensure that we pass the object-format capability in the synthesized test
data so that this test works with algorithms other than SHA-1.
Right.
In addition, add an additional test using the old data for when we're
using SHA-1 so that we can be sure that we preserve backwards
compatibility with servers not offering the object-format capability.
Hmmm. Isn't this an exact copy of the 'push plain' test immediately
preceding it? The commit message talks about using the "old data"
(i.e., without "object-format=%s"?). Should this test use a variant of
push_body where we're not adding "object-format"? I'm not sure I grok
what exactly we want to test here.. And does it really belong in
t/t*-content-length.sh?
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 11:03:01
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
When we speak protocol v2 in this test, we must pass the object-format
header if the algorithm is not SHA-1. Otherwise, git upload-pack fails
because the hash algorithm doesn't match and not because we've failed to
speak the protocol correctly. Pass the header so that our assertions
test what we're really interested in.
quoted hunk
+# If we don't print the object format, we'll fail for a spurious reason: the
+# mismatched object format.
+print_object_format () {
+ local algo=$(test_oid algo) &&
+ if test "$algo" != "sha1"
+ then
+ packetize "object-format=$algo"
+ fi
+}
+
test_expect_success 'extra delim packet in v2 ls-refs args' '
{
packetize command=ls-refs &&
+ print_object_format &&
printf 0001 &&
# protocol expects 0000 flush here
printf 0001
So we need to pass this capability for the SHA-256 tests to run ok. But
if we start passing "object-format=sha1" unconditionally at this point
in the series, the tests will fail:
error: 'grep expected flush after ls-refs arguments err' didn't find
a match in:
fatal: unknown capability 'object-format=sha1'
That is, we don't yet actually implement "object-format" handling. So
this will still fail with SHA-256 ("unknown capability"), just that once
the implementation is in place, the SHA-256 tests will pass (as will the
normal SHA-1 runs). Do I understand that correctly?
Or put differently, by the end of the series, we can do this:
@@ -6,14 +6,11 @@ communications if the other side says something
unexpected. We are mostly
making sure that we do not segfault or otherwise behave badly.'
. ./test-lib.sh
-# If we don't print the object format, we'll fail for a spurious reason: the
-# mismatched object format.
+# If we don't print the object format, we might fail for a spurious reason:
+# the mismatched object format.
print_object_format () {
local algo=$(test_oid algo) &&
- if test "$algo" != "sha1"
- then
- packetize "object-format=$algo"
- fi
+ packetize "object-format=$algo"
}
test_expect_success 'extra delim packet in v2 ls-refs args' '
Should we? (And if we do, we might as well drop this function and inline
the whole thing, IMHO.)
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 11:04:09
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
When we're checking the repository's format, set the hash algorithm at
the same time. This ensures that we perform a suitable initialization
early enough to avoid confusing any parts of the code. If we defer
until later, we can end up with portions of the code which are confused
about the hash algorithm, resulting in segfaults.
This doesn't make a difference as long as you just use SHA-1, right?
That is, this isn't a bug in the first half of this series nor in
v2.27-rc0 as long as you stick to SHA-1?
From: Martin Ågren <hidden> Date: 2020-05-16 11:05:10
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
One of the test assertions in this test checks that git branch -m works
even without a .git/config file. However, if the repository requires
configuration extensions, such as because it uses a non-SHA-1 algorithm,
this assertion will fail. Mark the assertion as requiring SHA-1.
Makes sense.
-test_expect_success 'git branch -m q q2 without config should succeed' '
+test_expect_success SHA1 'git branch -m q q2 without config should succeed' '
git branch -m q q2 &&
git branch -m q2 q
'
Going forward, we might need config files for other reasons (reftable?),
meaning this would become "SHA1,!REFTABLE". So maybe this should be
"!CONFIG_EXTENSIONS" or "CONFIG_LESS". I think this is ok for now,
though. When/if someone needs to make another fix like this here -- or
at the very least the *third* time around -- that's when we should think
a bit bigger.
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 11:12:39
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
Both v2 pack index files and the v3 format specified as part of the
NewHash work have similar data starting at the CRC table. Much of the
existing code wants to read either this table or the offset entries
following it, and in doing so computes the offset each time.
In order to share as much code between v2 and v3, compute the offset of
the CRC table and store it when the pack is opened. Use this value to
compute offsets to not only the CRC table, but to the offset entries
beyond it.
@@ -1555,13 +1555,9 @@ static void read_v2_anomalous_offsets(struct packed_git *p,{constuint32_t*idx1,*idx2;uint32_ti;-constuint32_thashwords=the_hash_algo->rawsz/sizeof(uint32_t);/* The address of the 4-byte offset table */-idx1=(((constuint32_t*)p->index_data)-+2/* 8-byte header */-+256/* fan out */-+hashwords*p->num_objects/* object ID table */+idx1=(((constuint32_t*)((constuint8_t*)p->index_data+p->crc_offset))+p->num_objects/* CRC32 table */);
This counts in four-byte words (so `+ 2` skips ahead 8B as the comment
notes). And that's why we need to use "rawsz/4".
Not new in this patch, but that outer pair of parenthesis just makes
this harder to read, IMHO. I keep scanning back and forth wondering,
"where is this whole thing going to get multiplied or something?"
idx1 = (const uint32_t *)((const uint8_t *)p->index_data + p->crc_offset)
+ p->num_objects /* CRC32 table */;
The double-casting can be avoided with something like this, but I'm not
sure it's really any better:
idx1 = (const uint32_t *)p->index_data
+ p->crc_offset/sizeof(uint32_t)
+ p->num_objects /* CRC32 table */;
quoted hunk
--- a/packfile.c+++ b/packfile.c
@@ -178,6 +178,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,*/(sizeof(off_t)<=4))returnerror("pack too large for current definition of off_t in %s",path);+p->crc_offset=8+4*256+nr*hashsz;}p->index_version=version;
It doesn't fit in the context, but `nr` will be assigned to
`p->num_objects`. And now we can just use `hashsz` without dividing by
4, so this does the same calculation as the old one above.
Martin
Ah, this is where that line from patch 2 went. ;-)
/* Process response from server */
while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
- if (!process_ref_v2(reader->line, &list))
+ if (!process_ref_v2(reader, &list))
die(_("invalid ls-refs response: %s"), reader->line);
}
From: Martin Ågren <hidden> Date: 2020-05-16 11:14:29
On Wed, 13 May 2020 at 02:58, brian m. carlson
[off-list ref] wrote:
When using protocol v2, we need to know what hash algorithm is used by
the remote end. See if the server has sent us an object-format
capability, and if so, use it to determine the hash algorithm in use and
set that value in the packet reader. Parse the refs using this
algorithm.
Note that we use memcpy instead of oidcpy for copying values, since
oidcpy is intentionally limited to the current hash algorithm length,
and the copy will be too short if the server side uses SHA-256 but the
client side has not had a repository set up (and therefore defaults to
SHA-1).
@@ -450,6 +453,14 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, if (server_supports_v2("agent", 0)) packet_write_fmt(fd_out, "agent=%s", git_user_agent_sanitized());+ if (server_feature_v2("object-format", &hash_name)) {+ int hash_algo = hash_algo_by_name(hash_name);+ if (hash_algo == GIT_HASH_UNKNOWN)+ die(_("unknown object format '%s' specified by server"), hash_name);+ reader->hash_algo = &hash_algos[hash_algo];+ packet_write_fmt(fd_out, "object-format=%s", reader->hash_algo->name);+ }+
(Similar to an earlier comment I made, if we don't see any
"object-format", we rely on `reader->hash_algo` to have been properly
set up (which it has) and to not have been modified since (which we
could probably rely on, hmm?).)
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 11:15:39
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
In order to communicate the protocol supported by the server side, add
support for advertising the object-format capability. We check that the
client side sends us an identical algorithm if it sends us its own
object-format capability, and assume it speaks SHA-1 if not.
In the test, when we're using an algorithm other than SHA-1, we need to
specify the algorithm in use so we don't get a failure with an "unknown
format" message. Add a wrapper function that specifies this header if
required. Skip specifying this header for SHA-1 to test that it works
both with and without this header.
This last sentence sort of answers an earlier question I made: should we
stop special-casing in the test and just always write the capability? I
can see your point here, but it only applies if you actually go to the
trouble of running the tests both with SHA-1 and SHA-256, right?
That is, I wonder if we shouldn't always pass the "object-format"
capability in the tests and, if we have the SHA-1 prereq, execute a
dedicated test where we do not pass it and verify that we default
correctly. Hmm?
quoted hunk
+write_command () {
+ echo "command=$1"
+
+ if test "$(test_oid algo)" != sha1
+ then
+ echo "object-format=$(test_oid algo)"
+ fi
+}
+
test_expect_success 'test capability advertisement' '
+ test_oid_init &&
cat >expect <<-EOF &&
version 2
agent=git/$(git version | cut -d" " -f3)
ls-refs
fetch=shallow
server-option
+ object-format=$(test_oid algo)
0000
EOF
In these two tests, we give "object-format" unconditionally, meaning
that in a SHA-1 run, we don't *always* skip passing in the capability.
So that's good. Should we verify that the implementation acts on the
"object-format=sha1" capability? Can we? The server should behave as
if it wasn't passed in at all, so I'm not sure how we could do that.
But that brings me to another point: Shouldn't we try to test the whole
"mismatched object format" detection by passing in "sha1" in a SHA-256
build and "sha256" with SHA-1. I suppose a `test_oid wrong_algo` could
come in handy in lots of negative tests that we'll want to add
throughout. Or maybe that doesn't quite fit the long-term goal.
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 11:17:00
On Wed, 13 May 2020 at 02:58, brian m. carlson
[off-list ref] wrote:
ls-remote may or may not operate within a repository, and as such will
not have been initialized with the repository's hash algorithm. Even if
it were, the remote side could be using a different algorithm and we
would still want to display those refs properly. Find the hash
algorithm used by the remote side by querying the transport object and
set our hash algorithm accordingly.
Without this change, if the remote side is using SHA-256, we truncate
the refs to 40 hex characters, since that's the length of the default
hash algorithm (SHA-1).
Could we add a test that passes now but would have failed before?
ref = transport_get_remote_refs(transport, &ref_prefixes);
+ if (ref) {
+ int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
+ repo_set_hash_algo(the_repository, hash_algo);
+ }
This will modify `the_hash_algo`. Quoting commit 78a6766802 ("Integrate
hash algorithm support with repo setup", 2017-11-12):
Add a constant, the_hash_algo, which points to the hash_algo structure
pointer in the repository global. Note that this is the hash which is
used to serialize data to disk, not the hash which is used to display
items to the user. The transition plan anticipates that these may be
different. We can add an additional element in the future (say,
ui_hash_algo) to provide for this case.
Don't we violate that here? Is it mostly luck that we can go on to list
what we want to list and that we will never write to disk based on
`the_hash_algo` being "wrong"(?)? Or am I missing something?
Martin
From: Martin Ågren <hidden> Date: 2020-05-16 11:17:19
On Wed, 13 May 2020 at 02:57, brian m. carlson
[off-list ref] wrote:
+ options.hash_algo = detect_hash_algo(heads);
+ if (!options.hash_algo)
+ die("%sinfo/refs not valid: could not determine hash algorithm; "
+ "is this a git repository?",
+ url.buf);
Should this use `transport_anonymize_url()`?
if (data[i] == '\n') {
- if (mid - start != the_hash_algo->hexsz)
+ if (mid - start != options.hash_algo->hexsz)
die(_("%sinfo/refs not valid: is this a git repository?"),
transport_anonymize_url(url.buf));
From: Martin Ågren <hidden> Date: 2020-05-16 11:18:27
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
quoted hunk
git index-pack is usually run in a repository, but need not be. Since
packs don't contains information on the algorithm in use, instead
relying on context, add an option to index-pack to tell it which one
we're using in case someone runs it outside of a repository.
Signed-off-by: brian m. carlson <redacted>
---
builtin/index-pack.c | 5 +++++
1 file changed, 5 insertions(+)
Patch 27 added `--hash` to `git show-index` and I almost commented on
"hash" vs "object-format". In the end I figured the object format was a
more technical (protocol) term. But now I wonder. Should we try to align
such options from the start? Or is there perhaps a reason for those
different approaches?
Similar to an earlier patch where we modify `the_hash_algo` like this, I
feel a bit nervous. What happens if you pass in a "wrong" algo here,
i.e., SHA-1 in a SHA-256 repo? Or, given the motivation in the commit
message, should this only be allowed if we really *are* outside a repo?
Martin
From: brian m. carlson <hidden> Date: 2020-05-16 19:16:51
On 2020-05-16 at 11:02:48, Martin Ågren wrote:
So we need to pass this capability for the SHA-256 tests to run ok. But
if we start passing "object-format=sha1" unconditionally at this point
in the series, the tests will fail:
error: 'grep expected flush after ls-refs arguments err' didn't find
a match in:
fatal: unknown capability 'object-format=sha1'
That is, we don't yet actually implement "object-format" handling. So
this will still fail with SHA-256 ("unknown capability"), just that once
the implementation is in place, the SHA-256 tests will pass (as will the
normal SHA-1 runs). Do I understand that correctly?
Yes, that's correct.
quoted hunk
Or put differently, by the end of the series, we can do this:
@@ -6,14 +6,11 @@ communications if the other side says something
unexpected. We are mostly
making sure that we do not segfault or otherwise behave badly.'
. ./test-lib.sh
-# If we don't print the object format, we'll fail for a spurious reason: the
-# mismatched object format.
+# If we don't print the object format, we might fail for a spurious reason:
+# the mismatched object format.
print_object_format () {
local algo=$(test_oid algo) &&
- if test "$algo" != "sha1"
- then
- packetize "object-format=$algo"
- fi
+ packetize "object-format=$algo"
}
test_expect_success 'extra delim packet in v2 ls-refs args' '
Should we? (And if we do, we might as well drop this function and inline
the whole thing, IMHO.)
We certainly can. I'll move this later on in the series so that we
can simplify the code.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2020-05-16 19:30:35
On 2020-05-16 at 11:03:56, Martin Ågren wrote:
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
quoted
When we're checking the repository's format, set the hash algorithm at
the same time. This ensures that we perform a suitable initialization
early enough to avoid confusing any parts of the code. If we defer
until later, we can end up with portions of the code which are confused
about the hash algorithm, resulting in segfaults.
This doesn't make a difference as long as you just use SHA-1, right?
That is, this isn't a bug in the first half of this series nor in
v2.27-rc0 as long as you stick to SHA-1?
Correct, because the default is SHA-1 if no algorithm is specified.
I'll update the commit message to reflect that this affects only
SHA-256.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2020-05-16 19:50:39
On 2020-05-16 at 10:55:33, Martin Ågren wrote:
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
quoted
Ensure that we pass the object-format capability in the synthesized test
data so that this test works with algorithms other than SHA-1.
Right.
quoted
In addition, add an additional test using the old data for when we're
using SHA-1 so that we can be sure that we preserve backwards
compatibility with servers not offering the object-format capability.
I'll have some questions on this below.
I think this got dropped in the rebase.
Hmmm. Isn't this an exact copy of the 'push plain' test immediately
preceding it? The commit message talks about using the "old data"
(i.e., without "object-format=%s"?). Should this test use a variant of
push_body where we're not adding "object-format"? I'm not sure I grok
what exactly we want to test here.. And does it really belong in
t/t*-content-length.sh?
It is. I'll probably drop this part of the patch.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2020-05-16 20:01:10
On 2020-05-16 at 10:40:11, Martin Ågren wrote:
On Wed, 13 May 2020 at 02:57, brian m. carlson
[off-list ref] wrote:
quoted
If we're fetching refs, detect the hash algorithm and parse the refs
using that algorithm.
As the added documentation from patch 2 says, if there are multiple
"object-format" capabilities, "the first one given is the one used in
the ref advertisement". And that's what you implement below.
Explaining that in this commit message and/or referring to "a recent
commit" (patch 2) and/or adding that documentation here, not back then,
would have avoided some confusion on my part, and perhaps also for
future readers.
I'll try to reword to improve things.
quoted
static void process_capabilities(struct packet_reader *reader, int *len)
{
+ const char *feat_val;
+ int feat_len;
+ int hash_algo;
xstrndup is needed because we're not guaranteed a terminating NUL. You
remember to call free afterwards. Ok.
If we don't get any "object-format", we do basically nothing here and
`reader->hash_algo` will remain as whatever it already is. The docs from
patch 2 promise that this will be handled as "SHA-1" -- would it be more
robust if we did a similar fallback dance as you do elsewhere?
feat_val = ...;
if (!feat_val) {
feat_val = hash_algos[GIT_HASH_SHA1].name;
feat_len = strlen(feat_val);
}
char *hash_name = ...
...
From: brian m. carlson <hidden> Date: 2020-05-16 20:29:03
On 2020-05-16 at 11:16:46, Martin Ågren wrote:
On Wed, 13 May 2020 at 02:58, brian m. carlson
[off-list ref] wrote:
quoted
ls-remote may or may not operate within a repository, and as such will
not have been initialized with the repository's hash algorithm. Even if
it were, the remote side could be using a different algorithm and we
would still want to display those refs properly. Find the hash
algorithm used by the remote side by querying the transport object and
set our hash algorithm accordingly.
Without this change, if the remote side is using SHA-256, we truncate
the refs to 40 hex characters, since that's the length of the default
hash algorithm (SHA-1).
Could we add a test that passes now but would have failed before?
The existing tests that call "git ls-remote" actually fail with SHA-256
if we don't do this, specifically "ls-remote works outside repository"
in t5512. That's the thing with a lot of this series: our existing test
suite is enormously effective at catching these things, but writing a
new test is hard because we can't actually instantiate a SHA-256
repository (because then users could, and it's broken until the end of
the series). Perhaps unsurprisingly, that's how I found this problem.
So while I would love to write a test for this case, I can't without
allowing users to corrupt and destroy their data in the mean time (or
tacking the final six commits to this series).
quoted
ref = transport_get_remote_refs(transport, &ref_prefixes);
+ if (ref) {
+ int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
+ repo_set_hash_algo(the_repository, hash_algo);
+ }
This will modify `the_hash_algo`. Quoting commit 78a6766802 ("Integrate
hash algorithm support with repo setup", 2017-11-12):
Add a constant, the_hash_algo, which points to the hash_algo structure
pointer in the repository global. Note that this is the hash which is
used to serialize data to disk, not the hash which is used to display
items to the user. The transition plan anticipates that these may be
different. We can add an additional element in the future (say,
ui_hash_algo) to provide for this case.
Don't we violate that here? Is it mostly luck that we can go on to list
what we want to list and that we will never write to disk based on
`the_hash_algo` being "wrong"(?)? Or am I missing something?
We do violate that and we also rely on it never having any effect on our
current repository. Unfortunately, as things stand now, we don't
support multiple hash algorithms in the same running binary, and we
can't until we allow a member of struct object_id to vary based on the
hash algorithm. That work is coming in a future series (after we have a
fully functioning SHA-256 stage 4 implementation), but at this point,
I'm still working through all of the crashes we get from random places
where we make assumptions about initializing things, so it's not a
straightforward fix.
For now, I think this is the best we can do without major additional
surgery to the codebase. I'm fine with stating that git ls-remote can
read the repository (to parse remotes) but can't write to it, since
that's the behavior users will expect anyway. I'll update the commit
message to reflect that wart and assumption, since it would be good to
document it.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2020-05-16 20:47:18
On 2020-05-16 at 11:18:12, Martin Ågren wrote:
On Wed, 13 May 2020 at 02:56, brian m. carlson
[off-list ref] wrote:
quoted
git index-pack is usually run in a repository, but need not be. Since
packs don't contains information on the algorithm in use, instead
relying on context, add an option to index-pack to tell it which one
we're using in case someone runs it outside of a repository.
Signed-off-by: brian m. carlson <redacted>
---
builtin/index-pack.c | 5 +++++
1 file changed, 5 insertions(+)
Patch 27 added `--hash` to `git show-index` and I almost commented on
"hash" vs "object-format". In the end I figured the object format was a
more technical (protocol) term. But now I wonder. Should we try to align
such options from the start? Or is there perhaps a reason for those
different approaches?
I'll bring them into sync.
Similar to an earlier patch where we modify `the_hash_algo` like this, I
feel a bit nervous. What happens if you pass in a "wrong" algo here,
i.e., SHA-1 in a SHA-256 repo? Or, given the motivation in the commit
message, should this only be allowed if we really *are* outside a repo?
Unfortunately, we can't prevent the user from being inside repository A,
which is SHA-1, while invoking git index-pack on repository B, which is
SHA-256. That is valid without --stdin, if uncommon, and it needs to be
supported. I can prevent it from being used with --stdin, though.
If you pass in a wrong algorithm, we usually blow up with an inflate
error because we consume more bytes than expected with our ref deltas.
I'm not aware of any cases where we segfault or access invalid memory;
we just blow up in a nonobvious way. That's true, too, if you manually
tamper with the algorithm in extensions.objectformat; usually we blow up
(but not segfault) because the index is "corrupt".
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204