On Mon, Jul 14, 2014 at 12:40 PM, Jeff King <p...@peff.net> wrote:
> As Junio and I discussed earlier in [1], this series makes the
> prio_queue struct stable with respect to object insertion (which in turn
> means we can use it to replace commit_list in more places).
> I think everything here is correct, but the second commit fails the
> final test in t5539. I think the test is just flaky (hence the RFH and
> cc to Duy).
> That test creates some unrelated commits in two separate repositories,
> and then fetches from one to the other. Since the commit creation
> happens in a subshell, the first commit in each ends up with the same
> test_tick value. When fetch-pack looks at the two root commits
> "unrelated1" and "new-too", the exact sequence of ACKs is different
> depending on which one it pulls out of the queue first.
> With the current code, it happens to be "unrelated1" (though this is not
> at all guaranteed by the prio_queue data structure, it is deterministic
> for this particular sequence of input). We see the ready-ACK, and the
> test succeeds.
> With the stable queue, we reliably get "new-too" out (since it is our
> local tip, it is added to the queue before we even talk to the remote).
> We never see a ready-ACK, and the test fails due to the grep on the
> TRACE_PACKET output at the end (the fetch itself succeeds as expected).
> I'm really not quite clear on what's supposed to be going on in the
> test. I can make it pass with:
> diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
> index 94553e1..b461188 100755
> --- a/t/t5539-fetch-http-shallow.sh
> +++ b/t/t5539-fetch-http-shallow.sh
> @@ -54,6 +54,7 @@ EOF
>  test_expect_success 'no shallow lines after receiving ACK ready' '
>         (
>                 cd shallow &&
> +               test_tick &&
>                 for i in $(test_seq 15)
>                 do
>                         git checkout --orphan unrelated$i &&
> which just bumps the timestamp for the unrelated* commits (so that they
> are always more recent than "new-too" and get picked first). I'm not
> sure if that is too hacky, or if there's a more robust way to set up the
> test.

The test needs the right amount of "have"s with pkt-flush inserted at
the right place. If test_tick makes it happy, I think we could go with
it for now. I'll try to study this code more and see if I can come up
with a better test. It'll be low priority in my backlog though.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to