git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH/RFH 0/3] stable priority-queue

From
Duy Nguyen <pclouds@gmail.com>
Date
Jul 14, 2014, 10:12 UTC
Message-ID
<CACsJy8DCbY7hOknGVM7zd6uJBq70+MG9N+Mj-+VXvrSan5njcA@mail.gmail.com>
In-Reply-To
<20140714054021.GA4422@sigill.intra.peff.net>
On Mon, Jul 14, 2014 at 12:40 PM, Jeff King <peff@peff.net> wrote:
Show 45 quoted lines
> 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.

-- 
Duy
Previous: Jeff KingNext: David Kastrup
Message 5 of 7 in “stable priority-queue”
  1. 0/3 stable priority-queueJeff King, Jul 14, 2014
  2. 1/3 prio-queue: factor out compare and swap operationsJeff King, Jul 14, 2014
  3. 2/3 prio-queue: make output stable with respect to insertionJeff King, Jul 14, 2014
  4. 3/3 paint_down_to_common: use prio_queueJeff King, Jul 14, 2014
  5. Duy NguyenJul 14, 2014
  6. David KastrupJul 14, 2014
  7. Jeff KingJul 21, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.