{"thread":{"id":"42423","subject":"Reviews for the first patches of pclouds/narrow-checkout","startedAt":"2016-05-23T18:26:16Z","lastAt":"2016-05-24T12:02:48Z","messageCount":2,"participants":["Stefan Beller","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"287268","messageId":"CAGZ79kb-y6gUHEBSdVB6Y+A=EjykZgcNsLynXcwSMyVNHiDU2Q@mail.gmail.com","threadId":"42423","inReplyTo":null,"subject":"Reviews for the first patches of pclouds/narrow-checkout","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-23T18:26:16Z","receivedAt":"2016-05-23T18:26:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi,\n\nso I started looking into your narrow checkout branch and started reviewing\nthe patches. Thanks for working on the narrow checkout!\n\nReviewed-by: Stefan Beller <sbeller@google.com> (just asking for signoff)\n    tree.c: break long lines\n    read-cache: realign some statements\n    read-cache: add sort_index()\n    read-cache: description for base_name_compare()\n    unpack-trees: break down long lines\n\nRegarding:\n    base_name_compare: do not assume name[len] == '\\0'\nCould you clarify if this is an existing bug or a preparation for a future\ndifferent use?\n\n\n\n    read-cache: new sort compare function that pays attention to ce_mode\n\nWould a new mode (0100 (directory)) require bumping the index version?\nOr the other way round: What If I use these patches and then try to use\nanother version of Git without this feature?\n\n\n\n    read-cache: refactor check_ce_order()\n\nWould it make sense to avoid the yoda condition?\n(i.e. \"cmp > 0\" instead of \"0 < cmp\" -> die(\"unordered stage entries\"))\nOr rather: I found it confusing that cmp is on both sides of the < in two\ndifferent conditions, i.e. it looks like you prefer to keep the \"<\" sign\nconstant, whereas I would have written\n\n    if (cmp < 0)\n        continue;\n    if (cmp > 0)\n        die(...);\n\nIt's a style thing, so I guess either is fine.\nI would however put the case for (cmp < 0) first as that is the expected case?\n\n\n\n    read-cache: check ce_mode in check_ce_order()\n\n    Can we replace\n        cmp = (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0;\n    by:\n        cmp = c1 - c2;\n    as it is only used in comparisons lesser/greater than 0 afterwards.\n\nAgain, I would put the \"continue\" case first (cmp >0)\n\n\n\n    read-cache: index_name_stage_pos() => index_name_mode_stage_pos()\n\n\"After this read-cache.c code is pretty much ready for accepting dir\nentries in the index.\"\n\nWhat is missing?\n\n\nSo that's a review for the first third of the patches. :)\n\nI wonder how much is left for actually finishing the narrow checkout,\nas I could not find documentation or code the user interacts with.\n(i.e. I would like to use a narrow checkout. How do I start? Where do I put\nthe pathspec of things I would like to use? Or are you envisioning a git wrapper\nfor that? \"git narrow [make-go-away, revive] <pathspec>\" ?)\n\nThanks,\nStefan\n"},{"id":"287410","messageId":"CACsJy8DVr7iL-bGWbQs52ZgnVeC0NhFM2NzZYmNA-FZ=Yu7+oA@mail.gmail.com","threadId":"42423","inReplyTo":"CAGZ79kb-y6gUHEBSdVB6Y+A=EjykZgcNsLynXcwSMyVNHiDU2Q@mail.gmail.com","subject":"Re: Reviews for the first patches of pclouds/narrow-checkout","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-24T12:02:48Z","receivedAt":"2016-05-24T12:02:48Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 24, 2016 at 1:26 AM, Stefan Beller <sbeller@google.com> wrote:\n> Hi,\n>\n> so I started looking into your narrow checkout branch and started reviewing\n> the patches. Thanks for working on the narrow checkout!\n>\n> Reviewed-by: Stefan Beller <sbeller@google.com> (just asking for signoff)\n>     tree.c: break long lines\n>     read-cache: realign some statements\n>     read-cache: add sort_index()\n>     read-cache: description for base_name_compare()\n>     unpack-trees: break down long lines\n>\n> Regarding:\n>     base_name_compare: do not assume name[len] == '\\0'\n> Could you clarify if this is an existing bug or a preparation for a future\n> different use?\n\nIt's a hidden bug if I remember correctly. New code activates it.\n\n\n>     read-cache: new sort compare function that pays attention to ce_mode\n>\n> Would a new mode (0100 (directory)) require bumping the index version?\n> Or the other way round: What If I use these patches and then try to use\n> another version of Git without this feature?\n\nGood question. Current git happily takes index with directories (not\ngood). However, high level like git-diff will detect and reject\nentries that are neither regular files or git-links. I might bump\nindex version up in the end to be on the safe side.\n\n>     read-cache: refactor check_ce_order()\n>\n> Would it make sense to avoid the yoda condition?\n> (i.e. \"cmp > 0\" instead of \"0 < cmp\" -> die(\"unordered stage entries\"))\n> Or rather: I found it confusing that cmp is on both sides of the < in two\n> different conditions, i.e. it looks like you prefer to keep the \"<\" sign\n> constant, whereas I would have written\n>\n>     if (cmp < 0)\n>         continue;\n>     if (cmp > 0)\n>         die(...);\n>\n> It's a style thing, so I guess either is fine.\n\nI didn't notice the new \"cmp < 0\" makes the old \"cmp <0\" look weird. Will fix.\n\n> I would however put the case for (cmp < 0) first as that is the expected case?\n\nYep.\n\n>     read-cache: check ce_mode in check_ce_order()\n>\n>     Can we replace\n>         cmp = (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0;\n>     by:\n>         cmp = c1 - c2;\n>     as it is only used in comparisons lesser/greater than 0 afterwards.\n\n\nHmm.. c1 and c2 are unsigned. I guess it's ok to do the subtraction,\ngoing to need to run some tests...\n>\n> Again, I would put the \"continue\" case first (cmp >0)\n>\n>\n>\n>     read-cache: index_name_stage_pos() => index_name_mode_stage_pos()\n>\n> \"After this read-cache.c code is pretty much ready for accepting dir\n> entries in the index.\"\n>\n> What is missing?\n\nSomething like the next patch, replacing file with a dir of same name\nor vice versa.\n\n> So that's a review for the first third of the patches. :)\n\nThanks :)\n\n> I wonder how much is left for actually finishing the narrow checkout,\n> as I could not find documentation or code the user interacts with.\n> (i.e. I would like to use a narrow checkout. How do I start? Where do I put\n> the pathspec of things I would like to use? Or are you envisioning a git wrapper\n> for that? \"git narrow [make-go-away, revive] <pathspec>\" ?)\n\nUI is bare bone at this stage. You can do \"git read-tree -mu HEAD --\n<pathspec>\". Directories that are _completely_ excluded will be folded\nand you should have some nice dir entries in the index (can be\nexamined with ls-files --stage, notice the mode column). Then you can\ntry out various commands and see whether they still work (they\nshould). The shape of the index is not defined in a file like sparse\ncheckout. You can fold and unfold directories at will and they should\nremain so (I think git reset --hard just drops the old index and write\na new one, we may need to fix that, because we would lose fold/unfold\nstate)\n\nThe next step is unfold operation, then re-fold. I think I aimed too\nhigh trying to combine those operations as one in unpack-trees.c.\nUnfolding a directory should be simple (UI in update-index), you read\nthe corresponding tree, you check out all files (and need to make sure\nthe directory is missing, so no overwrite/data loss can happen).\nRe-folding is simply running \"read-tree\" command again, I think.\nread-tree (or unpack_trees() in fact) already knows how to fold and\ndelete files in folded directories (or refuse to do so if files to be\ndeleted are dirty). I was trying to do both operations in one step to\navoid unnecessary I/O (e.g. you have folded directory \"a\", now you\nwant to unfold \"a/b\", but you want to keep \"a/b/c\" folded). That sort\nof work can be done at a later phase.\n\nOnce you support  both fold and unfold, you can teach \"git checkout\"\nabout this so we have a friendlier UI. \"git checkout --reshape --\n<pathspec>\" (or whatever option name) will try to unfold first (only\nrelevant directories, not unfold the entire worktree) then refold.\nNarrow checkout is pretty much done at this point. More work may be\nrequired, e.g. audit the code and prevent it from reading dir's SHA-1,\nfor narrow _clone_ because such an operation will fail in narrow clone\n(but not narrow checkout with full repository). If I remember\ncorrectly the major work to make narrow clone work is dealing with\nworktree. Once narrow checkout works, it's probably ~10 patches to get\na naive version of narrow clone.\n\nPS. I forgot about merge. If a merge conflict happens at a directory,\nI guess we can simply reject the merge and ask the user to unfold\nthose directories first... It's probably the same behavior we have\nwith submodules today, and we are probably fine as submodules have\nmostly addressed the \"directory in index\" issue for us. We just need\nto follow the path and accept directories where it already accepts\ngit-links.\n-- \nDuy\n"}]}