{"thread":{"id":"53905","subject":"[PATCH v2] doc: remove misleading documentation on pack names","startedAt":"2020-07-22T20:26:41Z","lastAt":"2020-07-22T21:16:41Z","messageCount":4,"participants":["Johannes Berg","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"401857","messageId":"20200722202629.109277-1-johannes@sipsolutions.net","threadId":"53905","inReplyTo":null,"subject":"[PATCH v2] doc: remove misleading documentation on pack names","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2020-07-22T20:26:29Z","receivedAt":"2020-07-22T20:26:41Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"The index-pack documentation explicitly states that the pack\nname is derived from the sorted list of object names, but\nthat clearly isn't true. I can't seem to be able to figure\nout if this was ever changed though.\n\nBe less explicit in the docs as to what the exact output is,\nand just say that it's whatever goes into the pack name.\n\nReally it seems to be the sha1 of the entire file, without\nthe checksum footer.\n\nSigned-off-by: Johannes Berg \n---\nIt was reported that bup writes pack files that have a name\ndifferent from what git does, and I think it's quite possibly\nbecause of this documentation ... it doesn't actually really\n*matter* though, as long as the file is internally consistent\nnothing checks that the name also matches the footer.\n\nYou can also take this as a bug report and fix the language in\nsome other, perhaps more precise way, if you prefer :-)\n\nv2: correct bup list address, oops\n---\n Documentation/git-index-pack.txt | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 9316d9a80b0d..ace40fa9f363 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -104,12 +104,11 @@ This option cannot be used with --stdin.\n NOTES\n -----\n \n-Once the index has been created, the list of object names is sorted\n-and the SHA-1 hash of that list is printed to stdout. If --stdin was\n-also used then this is prefixed by either \"pack\\t\", or \"keep\\t\" if a\n-new .keep file was successfully created. This is useful to remove a\n-.keep file used as a lock to prevent the race with 'git repack'\n-mentioned above.\n+Once the index has been created, the hash that goes into the name of\n+the pack/idx file is printed to stdout. If --stdin was also used then\n+this is prefixed by either \"pack\\t\", or \"keep\\t\" if a new .keep file\n+was successfully created. This is useful to remove a .keep file used\n+as a lock to prevent the race with 'git repack' mentioned above.\n \n GIT\n ---\n-- \n2.26.2\n\n"},{"id":"401860","messageId":"xmqq5zaf5ka6.fsf@gitster.c.googlers.com","threadId":"53905","inReplyTo":"20200722202629.109277-1-johannes@sipsolutions.net","subject":"Re: [PATCH v2] doc: remove misleading documentation on pack names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-22T21:09:37Z","receivedAt":"2020-07-22T21:09:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Berg writes:\n\n> The index-pack documentation explicitly states that the pack\n> name is derived from the sorted list of object names, but\n> that clearly isn't true. I can't seem to be able to figure\n> out if this was ever changed though.\n>\n> Be less explicit in the docs as to what the exact output is,\n> and just say that it's whatever goes into the pack name.\n>\n> Really it seems to be the sha1 of the entire file, without\n> the checksum footer.\n\nPlease avoid \"seems to be\" and spend a bit of effort digging the\nhistory especially when we are not in a hurry to get to the definite\nanswer. We can go \"less explicit\", or be a bit more informative by\nsaying that it is the trailer hash that is standard practice shared\nacross our binary files like the index and the packfile.\n\nI think this is 1190a1ac (pack-objects: name pack files after\ntrailer hash, 2013-12-05). It forgot to update the comment before\nwrite_idx_file() function when it did this change:\n\n /*\n * On entry *sha1 contains the pack content SHA1 hash, on exit it is\n * the SHA1 hash of sorted object names. The objects array passed in\n * will be sorted by SHA1 on exit.\n */\n const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,\n \t\t\t int nr_objects, const struct pack_idx_option *opts,\n-\t\t\t unsigned char *sha1)\n+\t\t\t const unsigned char *sha1)\n {\n\nObviously, after it turned *sha1 into 'const', it no longer is\npossible for it to have anything different from what was passed in\nupon exit.\n\n> +Once the index has been created, the hash that goes into the name of\n> +the pack/idx file is printed to stdout. If --stdin was also used then\n> +this is prefixed by either \"pack\\t\", or \"keep\\t\" if a new .keep file\n> +was successfully created. This is useful to remove a .keep file used\n> +as a lock to prevent the race with 'git repack' mentioned above.\n\nThe change is good---I made sure that among these filve lines, what\nchanged was only the first one and half lines. I however would have\npreferred not to see the line rewrapping.\n\nThanks.\n"},{"id":"401863","messageId":"ab8993fc6840765895ad39c59af1e536582e76d7.camel@sipsolutions.net","threadId":"53905","inReplyTo":"xmqq5zaf5ka6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] doc: remove misleading documentation on pack names","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2020-07-22T21:14:03Z","receivedAt":"2020-07-22T21:14:09Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"On Wed, 2020-07-22 at 14:09 -0700, Junio C Hamano wrote:\n> \n> Please avoid \"seems to be\" and spend a bit of effort digging the\n> history especially when we are not in a hurry to get to the definite\n> answer.\n\nI did ... but between all the file renames and moves, and not\nunderstanding the code very well, I didn't really understand what was\ngoing on.\n\nOk, so maybe \"seems to be\" was a bit of a cop-out because I do\nunderstand that's what git does *now* (having just replicated it in\nbup), but I have no idea how it got there.\n\n> We can go \"less explicit\", or be a bit more informative by\n> saying that it is the trailer hash that is standard practice shared\n> across our binary files like the index and the packfile.\n> \n> I think this is 1190a1ac (pack-objects: name pack files after\n> trailer hash, 2013-12-05).\n\nIndeed, that makes sense. Somehow I didn't come across this commit, but\nperhaps that's because I was looking too much at index-pack.c (and its\nvarious renames).\n\n> It forgot to update the comment before\n> write_idx_file() function when it did this change:\n> \n> /*\n> * On entry *sha1 contains the pack content SHA1 hash, on exit it is\n> * the SHA1 hash of sorted object names. The objects array passed in\n> * will be sorted by SHA1 on exit.\n> */\n> const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,\n> \t\t\t int nr_objects, const struct pack_idx_option *opts,\n> -\t\t\t unsigned char *sha1)\n> +\t\t\t const unsigned char *sha1)\n> {\n> \n> Obviously, after it turned *sha1 into 'const', it no longer is\n> possible for it to have anything different from what was passed in\n> upon exit.\n\nIndeed :-)\n\n> > +Once the index has been created, the hash that goes into the name of\n> > +the pack/idx file is printed to stdout. If --stdin was also used then\n> > +this is prefixed by either \"pack\\t\", or \"keep\\t\" if a new .keep file\n> > +was successfully created. This is useful to remove a .keep file used\n> > +as a lock to prevent the race with 'git repack' mentioned above.\n> \n> The change is good---I made sure that among these filve lines, what\n> changed was only the first one and half lines. I however would have\n> preferred not to see the line rewrapping.\n\nOk, fair - the text all seemed wrapped \"nicely\" so I preserved that\nrather than have one line significantly shorter than the others, but if\nyou prefer that it's fine by me.\n\nReally all I was trying to do is be a *little* more helpful than just\npoint out \"the documentation is wrong\"...\n\njohannes\n\n"},{"id":"401864","messageId":"xmqqsgdj45e2.fsf@gitster.c.googlers.com","threadId":"53905","inReplyTo":"ab8993fc6840765895ad39c59af1e536582e76d7.camel@sipsolutions.net","subject":"Re: [PATCH v2] doc: remove misleading documentation on pack names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-22T21:16:37Z","receivedAt":"2020-07-22T21:16:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Berg writes:\n\n> Really all I was trying to do is be a *little* more helpful than just\n> point out \"the documentation is wrong\"...\n\nThanks.\n"}]}