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

Reviews for the first patches of pclouds/narrow-checkout

From
Stefan Beller <sbeller@google.com>
Date
May 23, 2016, 18:26 UTC
Message-ID
<CAGZ79kb-y6gUHEBSdVB6Y+A=EjykZgcNsLynXcwSMyVNHiDU2Q@mail.gmail.com>
Hi,

so I started looking into your narrow checkout branch and started reviewing the patches. Thanks for working on the narrow checkout!

Reviewed-by: Stefan Beller <sbeller@google.com> (just asking for signoff)
    tree.c: break long lines
    read-cache: realign some statements
    read-cache: add sort_index()
    read-cache: description for base_name_compare()
    unpack-trees: break down long lines
Regarding:
    base_name_compare: do not assume name[len] == '\0'
Could you clarify if this is an existing bug or a preparation for a future
different use?
    read-cache: new sort compare function that pays attention to ce_mode

Would a new mode (0100 (directory)) require bumping the index version? Or the other way round: What If I use these patches and then try to use another version of Git without this feature?

    read-cache: refactor check_ce_order()

Would it make sense to avoid the yoda condition? (i.e. "cmp > 0" instead of "0 < cmp" -> die("unordered stage entries")) Or rather: I found it confusing that cmp is on both sides of the < in two different conditions, i.e. it looks like you prefer to keep the "<" sign constant, whereas I would have written

    if (cmp < 0)
        continue;
    if (cmp > 0)
        die(...);

It's a style thing, so I guess either is fine. I would however put the case for (cmp < 0) first as that is the expected case?

    read-cache: check ce_mode in check_ce_order()
    Can we replace
        cmp = (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0;
    by:
        cmp = c1 - c2;
    as it is only used in comparisons lesser/greater than 0 afterwards.
Again, I would put the "continue" case first (cmp >0)
    read-cache: index_name_stage_pos() => index_name_mode_stage_pos()

"After this read-cache.c code is pretty much ready for accepting dir entries in the index."

What is missing?
So that's a review for the first third of the patches. :)

I wonder how much is left for actually finishing the narrow checkout, as I could not find documentation or code the user interacts with. (i.e. I would like to use a narrow checkout. How do I start? Where do I put the pathspec of things I would like to use? Or are you envisioning a git wrapper for that? "git narrow [make-go-away, revive] <pathspec>" ?)

Thanks, Stefan

Next: Duy Nguyen
Message 1 of 2 in “Reviews for the first patches of pclouds/narrow-checkout”
  1. Stefan BellerMay 23, 2016
  2. Duy NguyenMay 24, 2016

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.