{"thread":{"id":"52907","subject":"[PATCH 0/2] Documentation of pack and repack","startedAt":"2020-02-28T15:44:29Z","lastAt":"2020-03-27T22:21:32Z","messageCount":15,"participants":["Damien Robert","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"392665","messageId":"20200228154357.1710521-1-damien.olivier.robert+git@gmail.com","threadId":"52907","inReplyTo":null,"subject":"[PATCH 0/2] Documentation of pack and repack","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-02-28T15:43:55Z","receivedAt":"2020-02-28T15:44:29Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"The first patch update the documentation, the second patch is a small clean\nup I found when reading the source to try to understand the behaviour of\npack-objects when writing the documentation.\n\nDamien Robert (2):\n doc: update the documentation of pack-objects and repack\n pack-objects: change the name of add_objects_in_unpacked_packs\n\n Documentation/git-pack-objects.txt | 27 +++++++++----\n Documentation/git-repack.txt | 61 +++++++++++++++++++-----------\n builtin/pack-objects.c | 4 +-\n 3 files changed, 60 insertions(+), 32 deletions(-)\n\n-- \nPatched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)\n\n"},{"id":"392666","messageId":"20200228154357.1710521-2-damien.olivier.robert+git@gmail.com","threadId":"52907","inReplyTo":"20200228154357.1710521-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH 1/2] doc: update the documentation of pack-objects and repack","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-02-28T15:43:56Z","receivedAt":"2020-02-28T15:44:31Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"For pack-objects:\n\n - add a documentation for --reflog, --indexed-objects and\n --write-bitmap-index.\n\n - clarify --keep-unreachable. Indeed the current description is out of\n date:\n\n Objects unreachable from the refs in packs named with\n --unpacked= option are added to the resulting pack, in\n addition to the reachable objects that are not in packs marked\n with *.keep files. This implies `--revs`.\n\n For example --unpacked= option is now a boolean, and objects in .keep\n packs are packed except if --honor-pack-keep is given.\n What --keep-unreachable currently does is simply to add all packed\n objects to the object list. In particular this includes unreachable\n objects (both unreachable from the passed revs or even from all revs).\n\n - rework the grammar for --pack-loose-unreachable\n\n - --unpack-unreachable can also accept a time\n\nFor repack:\n\n- Unify the notation to `git pack-objects` and not `git-pack-objects`.\n\n- Specify all options that are passed to `git pack-objects`\n\nSigned-off-by: Damien Robert \n---\n Documentation/git-pack-objects.txt | 27 +++++++++----\n Documentation/git-repack.txt | 61 +++++++++++++++++++-----------\n 2 files changed, 58 insertions(+), 30 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex fecdf2600c..7f4923ddea 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -80,6 +80,14 @@ base-name::\n \tas if all refs under `refs/` are specified to be\n \tincluded.\n \n+--reflog::\n+\tThis implies `--revs`.\n+\tInclude objects referred by reflog entries.\n+\n+--indexed-objects::\n+\tThis implies `--revs`.\n+\tInclude objects referred to by the index\n+\n --include-tag::\n \tInclude unasked-for annotated tags if the object they\n \treference was included in the resulting packfile. This\n@@ -123,6 +131,11 @@ depth is 4095.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\n \n+--write-bitmap-index::\n+\tWrite a reachability bitmap index as part of the pack. This\n+\tonly makes sense when used with `--all` and the pack is not\n+\toutputted to stdout.\n+\n --honor-pack-keep::\n \tThis flag causes an object already in a local pack that\n \thas a .keep file to be ignored, even if it would have\n@@ -286,17 +299,17 @@ Unexpected missing object will raise an error.\n \tpromisor remote [with .promisor].) This is used with partial clone.\n \n --keep-unreachable::\n-\tObjects unreachable from the refs in packs named with\n-\t--unpacked= option are added to the resulting pack, in\n-\taddition to the reachable objects that are not in packs marked\n-\twith *.keep files. This implies `--revs`.\n+\tUnreachable packed objects are added to the resulting pack.\n+\tThis implies `--revs`.\n \n --pack-loose-unreachable::\n-\tPack unreachable loose objects (and their loose counterparts\n-\tremoved). This implies `--revs`.\n+\tPack unreachable loose objects (and remove their loose counterparts).\n+\tThis implies `--revs`.\n \n---unpack-unreachable::\n+--unpack-unreachable=::\n \tKeep unreachable objects in loose form. This implies `--revs`.\n+\tIf `` is specified, do not bother loosening any objects older\n+\tthan ``.\n \n --delta-islands::\n \tRestrict delta matches based on \"islands\". See DELTA ISLANDS\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 92f146d27d..267edce2d6 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -68,11 +68,11 @@ to the new separate pack will be written.\n \tlinkgit:git-pack-objects[1].\n \n -f::\n-\tPass the `--no-reuse-delta` option to `git-pack-objects`, see\n+\tPass the `--no-reuse-delta` option to `git pack-objects`, see\n \tlinkgit:git-pack-objects[1].\n \n -F::\n-\tPass the `--no-reuse-object` option to `git-pack-objects`, see\n+\tPass the `--no-reuse-object` option to `git pack-objects`, see\n \tlinkgit:git-pack-objects[1].\n \n -q::\n@@ -88,13 +88,14 @@ to the new separate pack will be written.\n \n --window=::\n --depth=::\n-\tThese two options affect how the objects contained in the pack are\n-\tstored using delta compression. The objects are first internally\n-\tsorted by type, size and optionally names and compared against the\n-\tother objects within `--window` to see if using delta compression saves\n-\tspace. `--depth` limits the maximum delta depth; making it too deep\n-\taffects the performance on the unpacker side, because delta data needs\n-\tto be applied that many times to get to the necessary object.\n+\tThese two options are passed to `git pack-objects` and affect how\n+\tthe objects contained in the pack are stored using delta\n+\tcompression. The objects are first internally sorted by type, size\n+\tand optionally names and compared against the other objects within\n+\t`--window` to see if using delta compression saves space. `--depth`\n+\tlimits the maximum delta depth; making it too deep affects the\n+\tperformance on the unpacker side, because delta data needs to be\n+\tapplied that many times to get to the necessary object.\n +\n The default value for --window is 10 and --depth is 50. The maximum\n depth is 4095.\n@@ -103,13 +104,13 @@ depth is 4095.\n \tThis option is passed through to `git pack-objects`.\n \n --window-memory=::\n-\tThis option provides an additional limit on top of `--window`;\n-\tthe window size will dynamically scale down so as to not take\n-\tup more than '' bytes in memory. This is useful in\n-\trepositories with a mix of large and small objects to not run\n-\tout of memory with a large window, but still be able to take\n-\tadvantage of the large window for the smaller objects. The\n-\tsize can be suffixed with \"k\", \"m\", or \"g\".\n+\tThis option is passed to `git pack-objects` and provides an\n+\tadditional limit on top of `--window`; the window size will\n+\tdynamically scale down so as to not take up more than '' bytes\n+\tin memory. This is useful in repositories with a mix of large and\n+\tsmall objects to not run out of memory with a large window, but\n+\tstill be able to take advantage of the large window for the smaller\n+\tobjects. The size can be suffixed with \"k\", \"m\", or \"g\".\n \t`--window-memory=0` makes memory usage unlimited. The default\n \tis taken from the `pack.windowMemory` configuration variable.\n \tNote that the actual memory usage will be the limit multiplied\n@@ -122,6 +123,7 @@ depth is 4095.\n \tprevents the creation of a bitmap index.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\n+\tThis option is passed to `git pack-objects`.\n \n -b::\n --write-bitmap-index::\n@@ -129,7 +131,8 @@ depth is 4095.\n \tonly makes sense when used with `-a` or `-A`, as the bitmaps\n \tmust be able to refer to all reachable objects. This option\n \toverrides the setting of `repack.writeBitmaps`. This option\n-\thas no effect if multiple packfiles are created.\n+\thas no effect if multiple packfiles are created, and is passed to\n+\t`git pack-objects`.\n \n --pack-kept-objects::\n \tInclude objects in `.keep` files when repacking. Note that we\n@@ -145,26 +148,38 @@ depth is 4095.\n \tof having `.keep` file on the pack. `` is the\n \tpack file name without leading directory (e.g. `pack-123.pack`).\n \tThe option could be specified multiple times to keep multiple\n-\tpacks.\n+\tpacks, and is passed to `git pack-objects`.\n \n --unpack-unreachable=::\n+\tWhen used with `-ad`, this option is passed to `git pack-objects`.\n \tWhen loosening unreachable objects, do not bother loosening any\n \tobjects older than ``. This can be used to optimize out\n \tthe write of any objects that would be immediately pruned by\n \ta follow-up `git prune`.\n+\tThe `-A` option is synonymous with `-a --unpack-unreachable`.\n \n -k::\n --keep-unreachable::\n-\tWhen used with `-ad`, any unreachable objects from existing\n-\tpacks will be appended to the end of the packfile instead of\n-\tbeing removed. In addition, any unreachable loose objects will\n-\tbe packed (and their loose counterparts removed).\n+\tWhen used with `-ad`, '--keep-unreachable' and\n+\t'--pack-loose-unreachable' are passed to `git pack-objects`.\n+\tAny unreachable objects from existing packs will be appended to the\n+\tend of the packfile instead of being removed. In addition, any\n+\tunreachable loose objects will be packed (and their loose\n+\tcounterparts removed).\n \n -i::\n --delta-islands::\n-\tPass the `--delta-islands` option to `git-pack-objects`, see\n+\tPass the `--delta-islands` option to `git pack-objects`, see\n \tlinkgit:git-pack-objects[1].\n \n+Default options\n+---------------\n+\n+The command passes the following options to `git pack-objects`:\n+`--keep-true-parents`, `--no-empty`, `--all`, `--reflog`, `--indexed-objects`.\n+It also add `--exclude-promisor-objects` if there exists a promisor remote,\n+and `--honor-pack-keep` except if `--pack-kept-objects` is passed.\n+\n Configuration\n -------------\n \n-- \nPatched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)\n\n"},{"id":"392667","messageId":"20200228154357.1710521-3-damien.olivier.robert+git@gmail.com","threadId":"52907","inReplyTo":"20200228154357.1710521-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH 2/2] pack-objects: change the name of add_objects_in_unpacked_packs","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-02-28T15:43:57Z","receivedAt":"2020-02-28T15:44:32Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"`add_objects_in_unpacked_packs` was added in commit\n08cdfb13374f31b0c1c47444f55042e7b72c3190 (Sep 2007) to handle the\n`--keep-unreachable` option.\n\nBack then this function would iterate through packs associated to a list\nof revs, and add all objects that was not already in the object list,\nhence the name.\n\nNow the function simply iterate through all packs (more precisely all\nlocal packs not marked as .keep), and add all objects not already in the\nobject list.\n\nSo rename the function to add_unreachable_packed_objects, to mimic the\nnaming of add_unreachable_loose_objects.\n\nSigned-off-by: Damien Robert \n---\n builtin/pack-objects.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 940fbcb7b3..16c2efdbec 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3031,7 +3031,7 @@ static int ofscmp(const void *a_, const void *b_)\n \t\treturn oidcmp(&a->object->oid, &b->object->oid);\n }\n \n-static void add_objects_in_unpacked_packs(void)\n+static void add_unreachable_packed_objects(void)\n {\n \tstruct packed_git *p;\n \tstruct in_pack in_pack;\n@@ -3290,7 +3290,7 @@ static void get_object_list(int ac, const char **av)\n \t}\n \n \tif (keep_unreachable)\n-\t\tadd_objects_in_unpacked_packs();\n+\t\tadd_unreachable_packed_objects();\n \tif (pack_loose_unreachable)\n \t\tadd_unreachable_loose_objects();\n \tif (unpack_unreachable)\n-- \nPatched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)\n\n"},{"id":"392668","messageId":"20200228160137.iqd3pxtsdameijs3@feanor","threadId":"52907","inReplyTo":"20200228154357.1710521-3-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH 2/2] pack-objects: change the name of add_objects_in_unpacked_packs","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-02-28T16:01:37Z","receivedAt":"2020-02-28T16:01:43Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Damien Robert, Fri 28 Feb 2020 at 16:43:57 (+0100) :\n> Now the function simply iterate through all packs (more precisely all\n> local packs not marked as .keep), and add all objects not already in the\n> object list.\n\nBy the way is there a corner case here?\n\nWhen using `git repack --keep-unreachable --pack-kept-objects`, since\n`add_objects_in_unpacked_packs` does not add objects in *.keep packs, then\nunreachable objects there won't be added. Or am I missing something?\n"},{"id":"392749","messageId":"xmqqk142bn5f.fsf@gitster-ct.c.googlers.com","threadId":"52907","inReplyTo":"20200228154357.1710521-2-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH 1/2] doc: update the documentation of pack-objects and repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-02T18:57:16Z","receivedAt":"2020-03-02T18:57:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\n> index fecdf2600c..7f4923ddea 100644\n> --- a/Documentation/git-pack-objects.txt\n> +++ b/Documentation/git-pack-objects.txt\n> @@ -80,6 +80,14 @@ base-name::\n> \tas if all refs under `refs/` are specified to be\n> \tincluded.\n> \n> +--reflog::\n> +\tThis implies `--revs`.\n> +\tInclude objects referred by reflog entries.\n> +\n> +--indexed-objects::\n> +\tThis implies `--revs`.\n> +\tInclude objects referred to by the index\n> +\n\nMissing full-stop (.) at the end.\n\n> +--write-bitmap-index::\n> +\tWrite a reachability bitmap index as part of the pack. This\n> +\tonly makes sense when used with `--all` and the pack is not\n> +\toutputted to stdout.\n\nUsing \"output\" as a verb and conjugating it like this makes my head\nhurt. Let's instead borrow the phrase used in the description for\nthe \"--stdout\" option, i.e.\n\n\t... and the pack is not written to the standard output.\n\n> --keep-unreachable::\n> -\tObjects unreachable from the refs in packs named with\n> -\t--unpacked= option are added to the resulting pack, in\n> -\taddition to the reachable objects that are not in packs marked\n> -\twith *.keep files. This implies `--revs`.\n> +\tUnreachable packed objects are added to the resulting pack.\n> +\tThis implies `--revs`.\n\nWell spotted, and this update is very much appreciated.\n`--unpacked` does not take a name of a packfile at all, at least\nsince 03a9683d (\"Simplify is_kept_pack()\", 2009-02-28).\n\n> @@ -88,13 +88,14 @@ to the new separate pack will be written.\n> \n> --window=::\n> --depth=::\n> -\tThese two options affect how the objects contained in the pack are\n> -\tstored using delta compression. The objects are first internally\n> -\tsorted by type, size and optionally names and compared against the\n> -\tother objects within `--window` to see if using delta compression saves\n> -\tspace. `--depth` limits the maximum delta depth; making it too deep\n> -\taffects the performance on the unpacker side, because delta data needs\n> -\tto be applied that many times to get to the necessary object.\n> +\tThese two options are passed to `git pack-objects` and affect how\n> +\tthe objects contained in the pack are stored using delta\n> +\tcompression. The objects are first internally sorted by type, size\n> +\tand optionally names and compared against the other objects within\n> +\t`--window` to see if using delta compression saves space. `--depth`\n> +\tlimits the maximum delta depth; making it too deep affects the\n> +\tperformance on the unpacker side, because delta data needs to be\n> +\tapplied that many times to get to the necessary object.\n\nIt took me a while to realize that this only inserts \"are passed to\n`git pack-objects` and\" and does nothing else. It would have saved\nreviewers' time if the whole paragraph did not get rewrapped.\n\nI wonder if it helps the readers to tell the implementation detail\n(i.e. are passed to X) upfront like the updated text. It is true\nthat it would help the interested readers who want to know _more_\nto tell them that these corresponds to the options the underlying\ncommand has so they can go to the documentation of that other\ncommand and read more about them, though. \n\n> The default value for --window is 10 and --depth is 50. The maximum\n> depth is 4095.\n> @@ -103,13 +104,13 @@ depth is 4095.\n> \tThis option is passed through to `git pack-objects`.\n> \n> --window-memory=::\n> -\tThis option provides an additional limit on top of `--window`;\n> -\tthe window size will dynamically scale down so as to not take\n> -\tup more than '' bytes in memory. This is useful in\n> -\trepositories with a mix of large and small objects to not run\n> -\tout of memory with a large window, but still be able to take\n> -\tadvantage of the large window for the smaller objects. The\n> -\tsize can be suffixed with \"k\", \"m\", or \"g\".\n> +\tThis option is passed to `git pack-objects` and provides an\n> +\tadditional limit on top of `--window`; the window size will\n> +\tdynamically scale down so as to not take up more than '' bytes\n> +\tin memory. This is useful in repositories with a mix of large and\n> +\tsmall objects to not run out of memory with a large window, but\n> +\tstill be able to take advantage of the large window for the smaller\n> +\tobjects. The size can be suffixed with \"k\", \"m\", or \"g\".\n\nLikewise.\n\n> \t`--window-memory=0` makes memory usage unlimited. The default\n> \tis taken from the `pack.windowMemory` configuration variable.\n> \tNote that the actual memory usage will be the limit multiplied\n> @@ -122,6 +123,7 @@ depth is 4095.\n> \tprevents the creation of a bitmap index.\n> \tThe default is unlimited, unless the config variable\n> \t`pack.packSizeLimit` is set.\n> +\tThis option is passed to `git pack-objects`.\n\nHere, you use a different way to add the information to help readers\nwho would want to learn _more_. And I think this approach makes\nmore sense than the previous two. All readers would appreciate if\nthey can learn what they need to know to drive _this_ subcommand on\nthe documentation page for _this_ subcommand without having to\nconsulte another page, but those interested _can_ use reference like\nthis.\n\n> @@ -129,7 +131,8 @@ depth is 4095.\n> \tonly makes sense when used with `-a` or `-A`, as the bitmaps\n> \tmust be able to refer to all reachable objects. This option\n> \toverrides the setting of `repack.writeBitmaps`. This option\n> -\thas no effect if multiple packfiles are created.\n> +\thas no effect if multiple packfiles are created, and is passed to\n> +\t`git pack-objects`.\n\nDitto.\n\n> +Default options\n> +---------------\n> +\n> +The command passes the following options to `git pack-objects`:\n> +`--keep-true-parents`, `--no-empty`, `--all`, `--reflog`, `--indexed-objects`.\n> +It also add `--exclude-promisor-objects` if there exists a promisor remote,\n> +and `--honor-pack-keep` except if `--pack-kept-objects` is passed.\n\nThis is somewhat unconventional. I think we usually say, when\ndescribing each option --