{"thread":{"id":"60358","subject":"[PATCH 0/2] Documentation/gitformat-pack.txt: correct a few issues/typos","startedAt":"2023-10-12T21:09:26Z","lastAt":"2023-10-31T20:00:46Z","messageCount":10,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"483177","messageId":"cover.1697144959.git.me@ttaylorr.com","threadId":"60358","inReplyTo":null,"subject":"[PATCH 0/2] Documentation/gitformat-pack.txt: correct a few issues/typos","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-12T21:09:21Z","receivedAt":"2023-10-12T21:09:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"These short couple of patches are some fallout from a larger series[^1]\nthat I'm working on. I noticed a couple of minor issues in the MIDX\nformat's documentation, and figured that sending these patches sooner\nrather than later would be worthwhile.\n\nThe first patch swaps \"null\" for \"NUL\" when describing the NUL\ncharacter. The second patch corrects a piece of the documentation which\nclaims the PNAM chunk is not aligned (it is externally padded and thus\nguaranteed to be well-aligned).\n\nThanks in advance for your review.\n\n[^1]: The broader goal of that series is to allow verbatim pack reuse\n from multiple packs (instead of just the bitmapped one, or the\n preferred pack of a MIDX). This is only possible in certain\n circumstances, which require some tweaks to the MIDX format for\n bookkeeping reasons, which is how I noticed these issues in the first\n place.\n\nTaylor Blau (2):\n Documentation/gitformat-pack.txt: fix typo\n Documentation/gitformat-pack.txt: fix incorrect MIDX documentation\n\n Documentation/gitformat-pack.txt | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\n-- \n2.42.0.349.gf0c1128f8b.dirty\n"},{"id":"483178","messageId":"8c5fa1ff4f1fc069094b5acbc70cd0808742fd24.1697144959.git.me@ttaylorr.com","threadId":"60358","inReplyTo":"cover.1697144959.git.me@ttaylorr.com","subject":"[PATCH 1/2] Documentation/gitformat-pack.txt: fix typo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-12T21:09:25Z","receivedAt":"2023-10-12T21:09:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"e0d1bcf825 (multi-pack-index: add format details, 2018-07-12) describes\nthe MIDX's \"PNAM\" chunk as having entries which are \"null-terminated\nstrings\".\n\nThis is a typo, as strings are terminated with a NUL character, which is\na distinct concept from \"NULL\" or \"null\", which we typically reserve for\nthe void pointer to address 0.\n\nCorrect the documentation accordingly.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/gitformat-pack.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex 870e00f298..d7153962d4 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -390,7 +390,7 @@ CHUNK LOOKUP:\n CHUNK DATA:\n \n \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n-\t Stores the packfile names as concatenated, null-terminated strings.\n+\t Stores the packfile names as concatenated, NUL-terminated strings.\n \t Packfiles must be listed in lexicographic order for fast lookups by\n \t name. This is the only chunk not guaranteed to be a multiple of four\n \t bytes in length, so should be the last chunk for alignment reasons.\n-- \n2.42.0.349.gf0c1128f8b.dirty\n\n"},{"id":"483179","messageId":"af2742e05dff48c4ba0a9f36d58bcbfc052dca40.1697144959.git.me@ttaylorr.com","threadId":"60358","inReplyTo":"cover.1697144959.git.me@ttaylorr.com","subject":"[PATCH 2/2] Documentation/gitformat-pack.txt: fix incorrect MIDX documentation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-12T21:09:31Z","receivedAt":"2023-10-12T21:09:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Back in 32f3c541e3 (multi-pack-index: write pack names in chunk, 2018-07-12)\nthe MIDX's \"Packfile Names\" (or \"PNAM\", for short) chunk was described\nas containing an array of string entries. e0d1bcf825 notes that this is\nthe only chunk in the MIDX format's specification that is not guaranteed\nto be 4-byte aligned, and so should be placed last.\n\nThis isn't quite accurate: the entries within the PNAM chunk are not\nguaranteed to be aligned since they are arbitrary strings, but the\nchunk itself is aligned since the ending is padded with NUL bytes.\n\nThat external padding has always been there since 32f3c541e3 via\nmidx.c::write_midx_pack_names(), which ended with:\n\n i = MIDX_CHUNK_ALIGNMENT - (written % MIDX_CHUNK_ALIGNMENT)\n if (i < MIDX_CHUNK_ALIGNMENT) {\n unsigned char padding[MIDX_CHUNK_ALIGNMENT];\n memset(padding, 0, sizeof(padding))\n hashwrite(f, padding, i);\n written += i;\n }\n\nIn fact, 32f3c541e3's log message itself describes the chunk in its\nfirst paragraph with:\n\n Since filenames are not well structured, add padding to keep good\n alignment in later chunks.\n\nSo these have always been externally aligned. Correct the corresponding\npart of our documentation to reflect that.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/gitformat-pack.txt | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex d7153962d4..54000c9412 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -392,8 +392,9 @@ CHUNK DATA:\n \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n \t Stores the packfile names as concatenated, NUL-terminated strings.\n \t Packfiles must be listed in lexicographic order for fast lookups by\n-\t name. This is the only chunk not guaranteed to be a multiple of four\n-\t bytes in length, so should be the last chunk for alignment reasons.\n+\t name. Individual entries in this chunk are not guarenteed to be\n+\t aligned. The chunk is externally padded with zeros to align\n+\t remaining chunks.\n \n \tOID Fanout (ID: {'O', 'I', 'D', 'F'})\n \t The ith entry, F[i], stores the number of OIDs with first\n-- \n2.42.0.349.gf0c1128f8b.dirty\n"},{"id":"483182","messageId":"xmqq5y3b4id2.fsf@gitster.g","threadId":"60358","inReplyTo":"af2742e05dff48c4ba0a9f36d58bcbfc052dca40.1697144959.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] Documentation/gitformat-pack.txt: fix incorrect MIDX documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-12T21:54:17Z","receivedAt":"2023-10-12T21:54:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> diff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\n> index d7153962d4..54000c9412 100644\n> --- a/Documentation/gitformat-pack.txt\n> +++ b/Documentation/gitformat-pack.txt\n> @@ -392,8 +392,9 @@ CHUNK DATA:\n> \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n> \t Stores the packfile names as concatenated, NUL-terminated strings.\n\nNot a problem this series (neither this or the previous step)\nintroduces, but I had to read the implementation of\nwrite_midx_pack_names() to find out what \"concatenated\nNUL-terminated string\" really means. The code has a list of\nstrings, writes each of them as a NUL-terminated string in sequence,\nand to align the beginning of the next chunk, NULs are added to make\nthe whole thing multiple of MIDX_CHUNK_ALIGNMENT bytes.\n\nA naive reader code might implement a loop like so:\n\n\twhile (ptr[0] != '\\0') {\n\t\tendp = strlen(ptr);\n\t\t... ptr[0..endp] is one pathname ...\n\t\tptr = endp + 1;\n\t}\n\t\t\nexpecting that the terminating NUL of the last entry would be\nfollowed by a NUL, but that is buggy. The sum of the pathname\nstrings (with one NUL after each) may happen to be multiple of\nMIDX_CHUNK_ALIGNMENT bytes, in which case no extra padding NUL bytes\nwill be there. So the reader also needs to pay attention to the\nchunk size to notice when to stop reading. It feels somewhat\nsuboptimal.\n\n> \t Packfiles must be listed in lexicographic order for fast lookups by\n> -\t name. This is the only chunk not guaranteed to be a multiple of four\n> -\t bytes in length, so should be the last chunk for alignment reasons.\n> +\t name. Individual entries in this chunk are not guarenteed to be\n> +\t aligned. The chunk is externally padded with zeros to align\n> +\t remaining chunks.\n\nI am not sure what \"externally padded\" means.\n\nBefore write_midx_pack_names() returns, there is a code to pad the\nspace after it writes those names, which does not look any external\nthan the bytes used to record the pathnames or NUL bytes used to\nterminate these pathnames.\n\nTo me, \"externally padded\" is an appropriate phrase to use if this\nfunction does not bother with the alignment after what it needs to\nrecord, but the caller, i.e., write_chunkfile(), has code to notice\nthat the number of bytes written by cf->chunks[i].write_fn() it just\ncalled is not a multiple of some required alignment and adds padding\nbytes after .write_fn() returned. I do not think that is what is\ngoing on here.\n\nMy observations.\n\n * The packfile names chunk is used to record N pathnames; N is not\n recorded anywhere explicitly. To record N pathnames, a single\n chunk with N names in it is used. It is not like N chunks, each\n with one name is used.\n\n * Each of these pathname is recorded literally, followed by a NUL,\n in some order, without any extra padding.\n\n * To keep the size of the chunk multiple of alignment requirement,\n there may be extra NUL bytes after the names.\n\nThis data structure does not allow you to binary search or hash\nlookup without an extra table of pointers into this table of strings\nat runtime, and once you build such a table, the source being sorted\ndoes not help all that much. So I am not sure how strict the\n\"lexicographic\" requirement needs to be, but of course, starting\nstrict and loosening later is much easier than starting loose, so\ndocumenting \"must be listed\" and following that rule is fine.\n\nThanks.\n"},{"id":"484158","messageId":"ZUAmWWh1WHujiN05@nand.local","threadId":"60358","inReplyTo":"xmqq5y3b4id2.fsf@gitster.g","subject":"Re: [PATCH 2/2] Documentation/gitformat-pack.txt: fix incorrect MIDX documentation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-30T21:55:37Z","receivedAt":"2023-10-30T21:55:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 12, 2023 at 02:54:17PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > diff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\n> > index d7153962d4..54000c9412 100644\n> > --- a/Documentation/gitformat-pack.txt\n> > +++ b/Documentation/gitformat-pack.txt\n> > @@ -392,8 +392,9 @@ CHUNK DATA:\n> > \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n> > \t Stores the packfile names as concatenated, NUL-terminated strings.\n>\n> Not a problem this series (neither this or the previous step)\n> introduces, but I had to read the implementation of\n> write_midx_pack_names() to find out what \"concatenated\n> NUL-terminated string\" really means. The code has a list of\n> strings, writes each of them as a NUL-terminated string in sequence,\n> and to align the beginning of the next chunk, NULs are added to make\n> the whole thing multiple of MIDX_CHUNK_ALIGNMENT bytes.\n>\n> A naive reader code might implement a loop like so:\n>\n> \twhile (ptr[0] != '\\0') {\n> \t\tendp = strlen(ptr);\n> \t\t... ptr[0..endp] is one pathname ...\n> \t\tptr = endp + 1;\n> \t}\n>\n> expecting that the terminating NUL of the last entry would be\n> followed by a NUL, but that is buggy. The sum of the pathname\n> strings (with one NUL after each) may happen to be multiple of\n> MIDX_CHUNK_ALIGNMENT bytes, in which case no extra padding NUL bytes\n> will be there. So the reader also needs to pay attention to the\n> chunk size to notice when to stop reading. It feels somewhat\n> suboptimal.\n\nI agree.\n\n> > \t Packfiles must be listed in lexicographic order for fast lookups by\n> > -\t name. This is the only chunk not guaranteed to be a multiple of four\n> > -\t bytes in length, so should be the last chunk for alignment reasons.\n> > +\t name. Individual entries in this chunk are not guarenteed to be\n> > +\t aligned. The chunk is externally padded with zeros to align\n> > +\t remaining chunks.\n>\n> I am not sure what \"externally padded\" means.\n\nHow about something like this, instead?\n\n--- 8< ---\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex 0bc80f0d46..229490f82f 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -392,9 +392,10 @@ CHUNK DATA:\n \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n \t Stores the packfile names as concatenated, NUL-terminated strings.\n \t Packfiles must be listed in lexicographic order for fast lookups by\n-\t name. Individual entries in this chunk are not guarenteed to be\n-\t aligned. The chunk is externally padded with zeros to align\n-\t remaining chunks.\n+\t name. Individual entries in this chunk are not guaranteed to be\n+\t aligned, since the packfile names can be of arbitrary length. The\n+\t chunk itself is padded at the end with NUL bytes in order to align\n+\t the remaining chunks.\n\n \tOID Fanout (ID: {'O', 'I', 'D', 'F'})\n \t The ith entry, F[i], stores the number of OIDs with first\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"484171","messageId":"xmqq8r7j7hby.fsf@gitster.g","threadId":"60358","inReplyTo":"ZUAmWWh1WHujiN05@nand.local","subject":"Re: [PATCH 2/2] Documentation/gitformat-pack.txt: fix incorrect MIDX documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-31T00:42:41Z","receivedAt":"2023-10-31T00:42:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> > \t Packfiles must be listed in lexicographic order for fast lookups by\n>> > -\t name. This is the only chunk not guaranteed to be a multiple of four\n>> > -\t bytes in length, so should be the last chunk for alignment reasons.\n>> > +\t name. Individual entries in this chunk are not guarenteed to be\n>> > +\t aligned. The chunk is externally padded with zeros to align\n>> > +\t remaining chunks.\n>>\n>> I am not sure what \"externally padded\" means.\n>\n> How about something like this, instead?\n>\n> --- 8< ---\n> diff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\n> index 0bc80f0d46..229490f82f 100644\n> --- a/Documentation/gitformat-pack.txt\n> +++ b/Documentation/gitformat-pack.txt\n> @@ -392,9 +392,10 @@ CHUNK DATA:\n> \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n> \t Stores the packfile names as concatenated, NUL-terminated strings.\n> \t Packfiles must be listed in lexicographic order for fast lookups by\n> -\t name. Individual entries in this chunk are not guarenteed to be\n> -\t aligned. The chunk is externally padded with zeros to align\n> -\t remaining chunks.\n> +\t name. Individual entries in this chunk are not guaranteed to be\n> +\t aligned, since the packfile names can be of arbitrary length. The\n> +\t chunk itself is padded at the end with NUL bytes in order to align\n> +\t the remaining chunks.\n\nThere is no alignment requirement described, so \"not guaranteed\" and\n\"in order to align\" sound hollow. These are always byte-aligned ;-)\n\nHow about something along this line to simplify it a bit?\n\n\tStore the names of packfiles as a sequence of NUL-terminated\n\tstrings. There is no extra padding between the filenames,\n\tand they are listed in lexicographic order. The chunk\n\titself is padded at the end with NUL bytes to make the chunk\n\tsize a multiple of 4 bytes.\n\nI did not ccheck if the chunks need to be 4-byte aligned, so if the\nnumber is wrong, please adjust accordingly.\n\n"},{"id":"484250","messageId":"cover.1698780244.git.me@ttaylorr.com","threadId":"60358","inReplyTo":"cover.1697144959.git.me@ttaylorr.com","subject":"[PATCH v2 0/2] Documentation/gitformat-pack.txt: correct a few issues/typos","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-31T19:24:05Z","receivedAt":"2023-10-31T19:24:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"A minor reroll to adjust the text of the second patch to read more\nclearly, thanks to input from Junio.\n\nThis has been rebased onto 692be87cbb (Merge branch\n'jm/bisect-run-synopsis-fix', 2023-10-31). Thanks in advance for your\nreview!\n\nTaylor Blau (2):\n Documentation/gitformat-pack.txt: fix typo\n Documentation/gitformat-pack.txt: fix incorrect MIDX documentation\n\n Documentation/gitformat-pack.txt | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\nRange-diff against v1:\n1: 8c5fa1ff4f = 1: 92e9bee4ad Documentation/gitformat-pack.txt: fix typo\n2: af2742e05d ! 2: c149be35a1 Documentation/gitformat-pack.txt: fix incorrect MIDX documentation\n @@ Metadata\n ## Commit message ##\n Documentation/gitformat-pack.txt: fix incorrect MIDX documentation\n \n - Back in 32f3c541e3 (multi-pack-index: write pack names in chunk, 2018-07-12)\n - the MIDX's \"Packfile Names\" (or \"PNAM\", for short) chunk was described\n - as containing an array of string entries. e0d1bcf825 notes that this is\n - the only chunk in the MIDX format's specification that is not guaranteed\n - to be 4-byte aligned, and so should be placed last.\n + Back in 32f3c541e3 (multi-pack-index: write pack names in chunk,\n + 2018-07-12) the MIDX's \"Packfile Names\" (or \"PNAM\", for short) chunk was\n + described as containing an array of string entries. e0d1bcf825 notes\n + that this is the only chunk in the MIDX format's specification that is\n + not guaranteed to be 4-byte aligned, and so should be placed last.\n \n This isn't quite accurate: the entries within the PNAM chunk are not\n - guaranteed to be aligned since they are arbitrary strings, but the\n - chunk itself is aligned since the ending is padded with NUL bytes.\n + guaranteed to be 4-byte aligned since they are arbitrary strings, but\n + the chunk itself is 4-byte aligned since the ending is padded with NUL\n + bytes.\n \n - That external padding has always been there since 32f3c541e3 via\n + That padding has always been there since 32f3c541e3 via\n midx.c::write_midx_pack_names(), which ended with:\n \n i = MIDX_CHUNK_ALIGNMENT - (written % MIDX_CHUNK_ALIGNMENT)\n @@ Commit message\n So these have always been externally aligned. Correct the corresponding\n part of our documentation to reflect that.\n \n + Helped-by: Junio C Hamano \n Signed-off-by: Taylor Blau \n \n ## Documentation/gitformat-pack.txt ##\n -@@ Documentation/gitformat-pack.txt: CHUNK DATA:\n +@@ Documentation/gitformat-pack.txt: CHUNK LOOKUP:\n + CHUNK DATA:\n + \n \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n - \t Stores the packfile names as concatenated, NUL-terminated strings.\n - \t Packfiles must be listed in lexicographic order for fast lookups by\n +-\t Stores the packfile names as concatenated, NUL-terminated strings.\n +-\t Packfiles must be listed in lexicographic order for fast lookups by\n -\t name. This is the only chunk not guaranteed to be a multiple of four\n -\t bytes in length, so should be the last chunk for alignment reasons.\n -+\t name. Individual entries in this chunk are not guarenteed to be\n -+\t aligned. The chunk is externally padded with zeros to align\n -+\t remaining chunks.\n ++\t Store the names of packfiles as a sequence of NUL-terminated\n ++\t strings. There is no extra padding between the filenames,\n ++\t and they are listed in lexicographic order. The chunk itself\n ++\t is padded at the end with between 0 and 3 NUL bytes to make the\n ++\t chunk size a multiple of 4 bytes.\n \n \tOID Fanout (ID: {'O', 'I', 'D', 'F'})\n \t The ith entry, F[i], stores the number of OIDs with first\n\nbase-commit: 692be87cbba55e8488f805d236f2ad50483bd7d5\n-- \n2.42.0.527.ge89c67d052\n"},{"id":"484251","messageId":"92e9bee4ad39906c773e4a6eba84ec1ebf8c1d3b.1698780244.git.me@ttaylorr.com","threadId":"60358","inReplyTo":"cover.1698780244.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] Documentation/gitformat-pack.txt: fix typo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-31T19:24:08Z","receivedAt":"2023-10-31T19:24:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"e0d1bcf825 (multi-pack-index: add format details, 2018-07-12) describes\nthe MIDX's \"PNAM\" chunk as having entries which are \"null-terminated\nstrings\".\n\nThis is a typo, as strings are terminated with a NUL character, which is\na distinct concept from \"NULL\" or \"null\", which we typically reserve for\nthe void pointer to address 0.\n\nCorrect the documentation accordingly.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/gitformat-pack.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex 4a4d87e7db..c4eb09d52a 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -390,7 +390,7 @@ CHUNK LOOKUP:\n CHUNK DATA:\n \n \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n-\t Stores the packfile names as concatenated, null-terminated strings.\n+\t Stores the packfile names as concatenated, NUL-terminated strings.\n \t Packfiles must be listed in lexicographic order for fast lookups by\n \t name. This is the only chunk not guaranteed to be a multiple of four\n \t bytes in length, so should be the last chunk for alignment reasons.\n-- \n2.42.0.527.ge89c67d052\n\n"},{"id":"484252","messageId":"c149be35a1da66c5e1bbc1dd82839e32a52ace36.1698780244.git.me@ttaylorr.com","threadId":"60358","inReplyTo":"cover.1698780244.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] Documentation/gitformat-pack.txt: fix incorrect MIDX documentation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-31T19:24:11Z","receivedAt":"2023-10-31T19:24:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Back in 32f3c541e3 (multi-pack-index: write pack names in chunk,\n2018-07-12) the MIDX's \"Packfile Names\" (or \"PNAM\", for short) chunk was\ndescribed as containing an array of string entries. e0d1bcf825 notes\nthat this is the only chunk in the MIDX format's specification that is\nnot guaranteed to be 4-byte aligned, and so should be placed last.\n\nThis isn't quite accurate: the entries within the PNAM chunk are not\nguaranteed to be 4-byte aligned since they are arbitrary strings, but\nthe chunk itself is 4-byte aligned since the ending is padded with NUL\nbytes.\n\nThat padding has always been there since 32f3c541e3 via\nmidx.c::write_midx_pack_names(), which ended with:\n\n i = MIDX_CHUNK_ALIGNMENT - (written % MIDX_CHUNK_ALIGNMENT)\n if (i < MIDX_CHUNK_ALIGNMENT) {\n unsigned char padding[MIDX_CHUNK_ALIGNMENT];\n memset(padding, 0, sizeof(padding))\n hashwrite(f, padding, i);\n written += i;\n }\n\nIn fact, 32f3c541e3's log message itself describes the chunk in its\nfirst paragraph with:\n\n Since filenames are not well structured, add padding to keep good\n alignment in later chunks.\n\nSo these have always been externally aligned. Correct the corresponding\npart of our documentation to reflect that.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n Documentation/gitformat-pack.txt | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex c4eb09d52a..9fcb29a9c8 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -390,10 +390,11 @@ CHUNK LOOKUP:\n CHUNK DATA:\n \n \tPackfile Names (ID: {'P', 'N', 'A', 'M'})\n-\t Stores the packfile names as concatenated, NUL-terminated strings.\n-\t Packfiles must be listed in lexicographic order for fast lookups by\n-\t name. This is the only chunk not guaranteed to be a multiple of four\n-\t bytes in length, so should be the last chunk for alignment reasons.\n+\t Store the names of packfiles as a sequence of NUL-terminated\n+\t strings. There is no extra padding between the filenames,\n+\t and they are listed in lexicographic order. The chunk itself\n+\t is padded at the end with between 0 and 3 NUL bytes to make the\n+\t chunk size a multiple of 4 bytes.\n \n \tOID Fanout (ID: {'O', 'I', 'D', 'F'})\n \t The ith entry, F[i], stores the number of OIDs with first\n-- \n2.42.0.527.ge89c67d052\n"},{"id":"484254","messageId":"20231031200041.GA877408@coredump.intra.peff.net","threadId":"60358","inReplyTo":"c149be35a1da66c5e1bbc1dd82839e32a52ace36.1698780244.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/2] Documentation/gitformat-pack.txt: fix incorrect MIDX documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-31T20:00:41Z","receivedAt":"2023-10-31T20:00:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 31, 2023 at 03:24:11PM -0400, Taylor Blau wrote:\n\n> Back in 32f3c541e3 (multi-pack-index: write pack names in chunk,\n> 2018-07-12) the MIDX's \"Packfile Names\" (or \"PNAM\", for short) chunk was\n> described as containing an array of string entries. e0d1bcf825 notes\n> that this is the only chunk in the MIDX format's specification that is\n> not guaranteed to be 4-byte aligned, and so should be placed last.\n> \n> This isn't quite accurate: the entries within the PNAM chunk are not\n> guaranteed to be 4-byte aligned since they are arbitrary strings, but\n> the chunk itself is 4-byte aligned since the ending is padded with NUL\n> bytes.\n\nWe also don't place it last! :) So the alignment is very important, as I\nfound out in the recent chunk-corruption series.\n\n> So these have always been externally aligned. Correct the corresponding\n> part of our documentation to reflect that.\n\nBoth this and the previous patch look good to me.\n\n-Peff\n"}]}