[v3,0/7] v0 multiple-symref infinite loop fix and test cleanup

Message

Jeff King April 14, 2023, 9:24 p.m. UTC

On Wed, Apr 12, 2023 at 05:04:23AM -0400, Jeff King wrote:

> I'll squash that in and update the commit message before I do the next
> re-roll (but will still hold off a bit to get further comments).

Nobody said anything, so I assume the rest of the series is perfect. ;)

Junio, I see that you picked up this fix as a "squash", along with my
other "v2" update. Here's a v3 that does the actual squash along with a
commit message update. That ties up all loose ends from my perspective,
but of course if anybody has review comments, please send them.

The range-diff against what you have in jk/protocol-cap-parse-fix
(after squashing) is below.

  [1/7]: v0 protocol: fix infinite loop when parsing multi-valued capabilities
  [2/7]: t5512: stop referring to "v1" protocol
  [3/7]: v0 protocol: fix sha1/sha256 confusion for capabilities^{}
  [4/7]: t5512: add v2 support for "ls-remote --symref" test
  [5/7]: t5512: allow any protocol version for filtered symref test
  [6/7]: t5512: test "ls-remote --heads --symref" filtering with v0 and v2
  [7/7]: v0 protocol: use size_t for capability length/offset

 builtin/receive-pack.c |   2 +-
 connect.c              |  30 ++++----
 connect.h              |   4 +-
 fetch-pack.c           |   4 +-
 send-pack.c            |   2 +-
 t/t5512-ls-remote.sh   | 156 +++++++++++++++++++++++------------------
 transport.c            |   2 +-
 upload-pack.c          |   2 +-
 8 files changed, 112 insertions(+), 90 deletions(-)

1:  5471cf388b = 1:  117f371be1 v0 protocol: fix infinite loop when parsing multi-valued capabilities
2:  eb3e6e6d1c = 2:  de695291b0 t5512: stop referring to "v1" protocol
3:  c77b8ae4a0 ! 3:  515149d67a t5512: stop using jgit for capabilities^{} test
    @@ Metadata
     Author: Jeff King <[e-mail]>
     
      ## Commit message ##
    -    t5512: stop using jgit for capabilities^{} test
    +    v0 protocol: fix sha1/sha256 confusion for capabilities^{}
     
         Commit eb398797cd (connect: advertized capability is not a ref,
         2016-09-09) added support for an upload-pack server responding with:
     
           0000000000000000000000000000000000000000        capabilities^{}
     
    -    followed by a NUL and capabilities. This is not something Git itself has
    -    ever produced for upload-pack, but JGit does. And hence the test used
    -    JGit to reproduce the real-world situation. That was good for verifying
    -    that the incompatibility was fixed, but it's a lousy regression test for
    -    a few reasons:
    +    followed by a NUL and the actual capabilities. We correctly parse the
    +    oid using the packet_reader's hash_algo field, but then we compare it to
    +    null_oid(), which will instead use our current repo's default algorithm.
    +    If we're defaulting to sha256 locally but the other side is sha1, they
    +    won't match and we'll fail to parse the line (and thus die()).
     
    -      - hardly anybody runs it, because you have to have jgit installed
    +    This can cause a test failure when the suite is run with
    +    GIT_TEST_DEFAULT_HASH=sha256, and we even do so regularly via the
    +    linux-sha256 CI job. But since the test requires JGit to run, it's
    +    usually just skipped, and nobody noticed the problem.
    +
    +    The reason the original patch used JGit is that Git itself does not ever
    +    produce such a line via upload-pack; the feature was added to fix a
    +    real-world problem when interacting with JGit. That was good for
    +    verifying that the incompatibility was fixed, but it's not a good
    +    regression test:
    +
    +      - hardly anybody runs it, because you have to have jgit installed;
    +        hence this bug going unnoticed
     
           - we're depending on jgit's behavior for the test to do anything
             useful. In particular, this behavior is only relevant to the v0
    @@ Commit message
             script.
     
         Instead, let's just hard-code the response that's of interest to us.
    -    That will test exactly what we want for every run.
    +    That will test exactly what we want for every run, and reveals the bug
    +    when run in sha256 mode. And of course we'll fix the actual bug by using
    +    the correct hash_algo struct.
     
         Signed-off-by: Jeff King <[e-mail]>
     
4:  8db5b3c3bf = 4:  152d904a4a t5512: add v2 support for "ls-remote --symref" test
5:  f1cd63e16e = 5:  87053ab90b t5512: allow any protocol version for filtered symref test
6:  b6b9d1ad44 = 6:  37d300d244 t5512: test "ls-remote --heads --symref" filtering with v0 and v2
7:  870d6e0a3b = 7:  4db6853ea2 v0 protocol: use size_t for capability length/offset

Comments

Junio C Hamano April 17, 2023, 4:06 p.m. UTC | #1

Jeff King <[e-mail]> writes:

> On Wed, Apr 12, 2023 at 05:04:23AM -0400, Jeff King wrote:
>
>> I'll squash that in and update the commit message before I do the next
>> re-roll (but will still hold off a bit to get further comments).
>
> Nobody said anything, so I assume the rest of the series is perfect. ;)
>
> Junio, I see that you picked up this fix as a "squash", along with my
> other "v2" update. Here's a v3 that does the actual squash along with a
> commit message update. That ties up all loose ends from my perspective,
> but of course if anybody has review comments, please send them.

These look good.  Queued.

Thanks.