{"thread":{"id":"59467","subject":"Feature Request: Option to make \"git rev-list --objects\" output duplicate objects","startedAt":"2023-03-24T15:51:30Z","lastAt":"2023-03-30T10:32:30Z","messageCount":9,"participants":["Baumann, Moritz","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"474097","messageId":"AS1PR02MB8185A45DB63216699AFB2C5494849@AS1PR02MB8185.eurprd02.prod.outlook.com","threadId":"59467","inReplyTo":null,"subject":"Feature Request: Option to make \"git rev-list --objects\" output duplicate objects","fromName":"Baumann, Moritz","fromEmail":"moritz.baumann@sap.com","sentAt":"2023-03-24T15:51:21Z","receivedAt":"2023-03-24T15:51:30Z","isPatch":false,"sender":{"key":"moritz.baumann@sap.com","avatar":"https://avatars.githubusercontent.com/u/67415316?v=4"},"body":"Good afternoon,\n\nWhile rewriting a slow pre-receive hook to (hopefully) speed it up, I ran into\nthe following problem:\n\nWe want to perform a bunch of checks on all newly pushed files, some of which\nare based on the file contents and some of which are based on the file name.\nI started with…\n\n  git rev-list --pretty=format:'' --no-commit-header \\\n  --objects --filter=object:type=blob \"$new_head\" --not --all\n\n…and then used the resulting list for all subsequent checks. After writing some\nunit tests, I noticed that the returned list is not sufficient: If you generate\nthe exact same file twice, once with a \"bad\" name and once with a \"good\" name,\nyou will only see one of those names and therefore the hook will mistakenly\nallow the push.\n\nSo, what I would want/need is an option that forces \"git rev-list --objects\"\nto output the object multiple times if it has multiple names in the commit\nrange. Admittedly, such an option would likely only be useful for hooks that\nvalidate file names.\n\nWould it be feasible to implement such an option? If so, does it sound like a\ngood or bad idea?\n\nIs there any alternative for my use case that doesn't involve walking the\ncommits one-by-one? (That's what we previously did and what turned out to be\nquite slow on our repository.)\n\nBest regards,\nMoritz Baumann\n"},{"id":"474099","messageId":"xmqq4jqa3xgj.fsf@gitster.g","threadId":"59467","inReplyTo":"AS1PR02MB8185A45DB63216699AFB2C5494849@AS1PR02MB8185.eurprd02.prod.outlook.com","subject":"Re: Feature Request: Option to make \"git rev-list --objects\" output duplicate objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-24T16:50:36Z","receivedAt":"2023-03-24T16:50:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Baumann, Moritz\" <moritz.baumann@sap.com> writes:\n\n> Is there any alternative for my use case that doesn't involve walking the\n> commits one-by-one?\n\nI do not think there currently is such an option, and not showing\nthe same object twice is pretty much fundamental in the operation of\nthe command, so it is unclear what the new feature should look like.\n\nShould it also show the same commit it encounters during its history\nwalk (remember, a history can have forks and merges in it) in\nduplicates if it encounters it more than once?  Should it show all\nthe subtrees and blobs in the tree of each commit, even if most of\nthem do not change from commit to commit?  How does the user control\nwhich ones are duplicated and which ones are to be visited only\nonce?  How does the command line option used by the user to tell the\ncommand to give such a choice look like?\n\nOnce there things are designed well, implementation of such a mode\nwould not be too hard.\n\nBy the way, \"rev-list\" internally uses walking the commits\none-by-one, anyway ;-).\n\n"},{"id":"474117","messageId":"20230324192848.GC536967@coredump.intra.peff.net","threadId":"59467","inReplyTo":"AS1PR02MB8185A45DB63216699AFB2C5494849@AS1PR02MB8185.eurprd02.prod.outlook.com","subject":"Re: Feature Request: Option to make \"git rev-list --objects\" output duplicate objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-24T19:28:48Z","receivedAt":"2023-03-24T19:28:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2023 at 03:51:21PM +0000, Baumann, Moritz wrote:\n\n> …and then used the resulting list for all subsequent checks. After writing some\n> unit tests, I noticed that the returned list is not sufficient: If you generate\n> the exact same file twice, once with a \"bad\" name and once with a \"good\" name,\n> you will only see one of those names and therefore the hook will mistakenly\n> allow the push.\n> \n> So, what I would want/need is an option that forces \"git rev-list --objects\"\n> to output the object multiple times if it has multiple names in the commit\n> range. Admittedly, such an option would likely only be useful for hooks that\n> validate file names.\n\nAnother problem you might not have run into yet: the names given by\nrev-list are not quoted in any way, and will just omit newlines. So if\nyour hook is trying to avoid malicious garbage like \"foo\\nbar\", it won't\nwork.\n\nThose names are really just intended as hints for pack-objects. I\nsuspect the documentation could be more clear about these limitations.\n\n> Would it be feasible to implement such an option? If so, does it sound like a\n> good or bad idea?\n> \n> Is there any alternative for my use case that doesn't involve walking the\n> commits one-by-one? (That's what we previously did and what turned out to be\n> quite slow on our repository.)\n\nI'm not sure what you mean by \"one by one\", since that is inherently\nwhat rev-list is doing under the hood. If you mean \"running a separate\nprocess for each commit\", then yes, that will be slow. But if you want\nto know all of the names touched in a set of commits, I have used\nsomething like this before:\n\n  git rev-list $new --not --all |\n  git diff-tree --stdin --format= -r -c --name-only\n\nA few notes:\n\n  - the names may be quoted if they have metacharacters; use \"-z\" if\n    your reading side can handle it to make things simpler\n\n  - merges are always tricky. I think \"-c\" will give you what you want\n    (showing names which differed from any parent), but I didn't think\n    too hard.  Using \"-m\" definitely would work, but may produce extra\n    names (ones where the merge just brought together two lines of\n    history, even though the commit where one of those lines touched the\n    file may have been excluded via \"--not --all\").\n\n  - if you are assuming the existing names are good, then probably\n    --diff-filter=A would be useful, as it would show only\n    newly-introduced names.\n\n-Peff\n"},{"id":"474213","messageId":"AS1PR02MB818545E563E40C5E31A02D1E948B9@AS1PR02MB8185.eurprd02.prod.outlook.com","threadId":"59467","inReplyTo":"xmqq4jqa3xgj.fsf@gitster.g","subject":"RE: Feature Request: Option to make \"git rev-list --objects\" output duplicate objects","fromName":"Baumann, Moritz","fromEmail":"moritz.baumann@sap.com","sentAt":"2023-03-27T07:02:54Z","receivedAt":"2023-03-27T07:03:05Z","isPatch":false,"sender":{"key":"moritz.baumann@sap.com","avatar":"https://avatars.githubusercontent.com/u/67415316?v=4"},"body":"> I do not think there currently is such an option, and not showing\n> the same object twice is pretty much fundamental in the operation of\n> the command, so it is unclear what the new feature should look like.\n>\n> Should it also show the same commit it encounters during its history\n> walk (remember, a history can have forks and merges in it) in\n> duplicates if it encounters it more than once?  Should it show all\n> the subtrees and blobs in the tree of each commit, even if most of\n> them do not change from commit to commit?  How does the user control\n> which ones are duplicated and which ones are to be visited only\n> once?  How does the command line option used by the user to tell the\n> command to give such a choice look like?\n\nI was a bit brief in my description on Friday evening, due to the day & time.\nMy naïve idea would be as follows:\n\nThe option would be called something like \"--include-all-object-names\" and\nbelong to the category of options that only make sense in combination with\n\"--objects\". That name (hopefully) already explains the intended behavior:\n\n * commits are not duplicated\n * as before, only changed blobs / subtrees are shown, however:\n * blobs / subtrees are duplicated in the output if they were previously shown\n   with a different name\n\n> By the way, \"rev-list\" internally uses walking the commits\n> one-by-one, anyway ;-).\n\nI really should not write emails on Friday evening… What I meant was that I'd\nrather use a single call to git than one per commit.\n\n(The previous implementation of my hook then also queried all blobs for each\ncommit, not just the changed ones, which contributed to the bad performance.)\n"},{"id":"474238","messageId":"xmqqzg7ych5i.fsf@gitster.g","threadId":"59467","inReplyTo":"AS1PR02MB818545E563E40C5E31A02D1E948B9@AS1PR02MB8185.eurprd02.prod.outlook.com","subject":"Re: Feature Request: Option to make \"git rev-list --objects\" output duplicate objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-27T16:07:05Z","receivedAt":"2023-03-27T16:07:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Baumann, Moritz\" <moritz.baumann@sap.com> writes:\n\n> The option would be called something like \"--include-all-object-names\" and\n> belong to the category of options that only make sense in combination with\n> \"--objects\". That name (hopefully) already explains the intended behavior:\n>\n>  * commits are not duplicated\n\nDidn't the sentence above just say \"all object names\"?  Why not commits?\n\n>  * as before, only changed blobs / subtrees are shown, however:\n\nWe are not showing \"changed\" things at all.  If you want that, you'd\nneed \"git rev-list | git diff-tree --stdin\" instead.\n\nThe difference matters if you are aiming for automation (i.e. not\ncasual browsing).  If you had a blob A, changed it in a commit to B,\nand then put the original one A back in another commit, you should\nsee (in reverse history traversal) A and then B and then A again if\nwe were showing \"changed\" things.  But rev-list is not about showing\nchanges.  We show A in the latest commit, B in the previous commit,\nand that is the end.  Original needs not to be shown, because rev-list\nis enumerating the objects in the history and it knows that it showed\nA already.\n\nIf you did not have a blob C, and added two instances of the same\nblob C to different paths, you should see C twice if we were showing\n\"changed\" things.  But rev-list is not about showing changes.  We\nshow C once for a path, and it is not shown again for the other\npath.  The other one needs not to be shown, because rev-list is\nenumerating the objects in the history and it knows that it showed C\nalready.\n\n>  * blobs / subtrees are duplicated in the output if they were previously shown\n>    with a different name\n\nI suspect that adding \"if they were ... with a DIFFERENT name\" would\nprobably make it prohibitively more expensive.  The traversal done\nby rev-list is fundamentally driven by \"have we shown this object\n(1-bit)?\" to avoid duplicated work, and not \"at which path did this\nobject appear (unbounded amount of information)?\".\n\nAs I told you in the message you are responding to, I think I am OK\nwith the idea of showing a bit more from \"rev-list --objects\", but\nsuch an enhancement may need to be designed a bit better, I am\nafraid.\n\nI suspect that \"rev-list | diff-tree --stdin\" without \"--objects\"\nadded to the upstream \"rev-list\" might be closer to what you are\nlooking for in this case, but I dunno.\n\nThanks.\n"},{"id":"474269","messageId":"AS1PR02MB8185DF947EBC583318481E1994889@AS1PR02MB8185.eurprd02.prod.outlook.com","threadId":"59467","inReplyTo":"20230324192848.GC536967@coredump.intra.peff.net","subject":"RE: Feature Request: Option to make \"git rev-list --objects\" output duplicate objects","fromName":"Baumann, Moritz","fromEmail":"moritz.baumann@sap.com","sentAt":"2023-03-28T08:08:02Z","receivedAt":"2023-03-28T08:08:16Z","isPatch":false,"sender":{"key":"moritz.baumann@sap.com","avatar":"https://avatars.githubusercontent.com/u/67415316?v=4"},"body":"> Another problem you might not have run into yet: the names given by\n> rev-list are not quoted in any way, and will just omit newlines. So if\n> your hook is trying to avoid malicious garbage like \"foo\\nbar\", it won't\n> work.\n\nThanks for that warning. I was not aware that rev-list didn't quote file names.\n\n> Those names are really just intended as hints for pack-objects. I\n> suspect the documentation could be more clear about these limitations.\n\nThat would indeed be great and would have likely prevented the obvious\nmisconceptions on my side.\n\n> I'm not sure what you mean by \"one by one\", since that is inherently\n> what rev-list is doing under the hood. If you mean \"running a separate\n> process for each commit\", then yes, that will be slow.\n\nYes, that's what I meant to say.\n\n> But if you want\n> to know all of the names touched in a set of commits, I have used\n> something like this before:\n>\n>   git rev-list $new --not --all |\n>   git diff-tree --stdin --format= -r -c --name-only\n\nThanks, that looks promising and solves at least one of my use cases. The only\nminor problem is that there seems to be no way to pipe the diff-tree output to\ncat-file without massaging it with awk first.\n\nI have three uses cases in my pre-receive hooks:\n\n1. Filters solely based on the file name\n   ? your suggestions works perfectly here\n2. Filters based only on file contents\n   ? git rev-list --objects + git cat-file provide everything I need\n3. One filter based on file size and name (forbid large files, with exceptions)\n   ? I'm guessing \"git rev-list | git diff-tree --stdin | awk |\n     git cat-file --batch-check\" is the best solution to extract the necessary\n     information from git in this case?\n\n-- Moritz\n"},{"id":"474350","messageId":"20230328182650.GC18558@coredump.intra.peff.net","threadId":"59467","inReplyTo":"AS1PR02MB8185DF947EBC583318481E1994889@AS1PR02MB8185.eurprd02.prod.outlook.com","subject":"[PATCH] docs: document caveats of rev-list's object-name output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-28T18:26:50Z","receivedAt":"2023-03-28T18:26:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2023 at 08:08:02AM +0000, Baumann, Moritz wrote:\n\n> > Those names are really just intended as hints for pack-objects. I\n> > suspect the documentation could be more clear about these limitations.\n> \n> That would indeed be great and would have likely prevented the obvious\n> misconceptions on my side.\n\nHere's what I came up with.\n\n-- >8 --\nSubject: docs: document caveats of rev-list's object-name output\n\nAt first glance, the names given by \"rev-list --objects\" seem like a\ngood way to see which paths are present in a set of commits. But there\nare some subtle gotchas there. We do not document the format of the\nnames at all, so let's do so, along with warning of these problems.\n\nI intentionally did not document the exact format of the names here, as\nI don't think it's something we want people to rely on (though I doubt\nin practice that we'd change it at this point).\n\nThough all of this is historically tied to \"--objects\", these days we\nhave a separate \"--object-names\" flag which can turn the names off or\non. So I put the detailed documentation there, but added a note from\n--objects (which did not otherwise mention the names at all, even though\nthey are on by default).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI also considered adding a specific \"if you want the names of each file\nin a range of commits, pipe to diff-tree\" example. But it seemed like it\nwould clutter up this section. It might be OK as a stand-alone in the\nEXAMPLES section, but should probably be done as a separate patch if\nanyone is interested.\n\n Documentation/rev-list-options.txt | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 90c73d6708b..3000888a908 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -890,7 +890,7 @@ ifdef::git-rev-list[]\n \tPrint the object IDs of any object referenced by the listed\n \tcommits.  `--objects foo ^bar` thus means ``send me\n \tall object IDs which I need to download if I have the commit\n-\tobject _bar_ but not _foo_''.\n+\tobject _bar_ but not _foo_''. See also `--object-names` below.\n \n --in-commit-order::\n \tPrint tree and blob ids in order of the commits. The tree\n@@ -920,7 +920,12 @@ ifdef::git-rev-list[]\n \n --object-names::\n \tOnly useful with `--objects`; print the names of the object IDs\n-\tthat are found. This is the default behavior.\n+\tthat are found. This is the default behavior. Note that the\n+\t\"name\" of each object is ambiguous, and mostly intended as a\n+\thint for packing objects. In particular: no distinction is made between\n+\tthe names of tags, trees, and blobs; path names may be modified\n+\tto remove newlines; and if an object would appear multiple times\n+\twith different names, only one name is shown.\n \n --no-object-names::\n \tOnly useful with `--objects`; does not print the names of the object\n-- \n2.40.0.616.gf524ec75088\n\n"},{"id":"474352","messageId":"20230328183253.GD18558@coredump.intra.peff.net","threadId":"59467","inReplyTo":"AS1PR02MB8185DF947EBC583318481E1994889@AS1PR02MB8185.eurprd02.prod.outlook.com","subject":"Re: Feature Request: Option to make \"git rev-list --objects\" output duplicate objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-28T18:32:53Z","receivedAt":"2023-03-28T18:32:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2023 at 08:08:02AM +0000, Baumann, Moritz wrote:\n\n> > But if you want\n> > to know all of the names touched in a set of commits, I have used\n> > something like this before:\n> >\n> >   git rev-list $new --not --all |\n> >   git diff-tree --stdin --format= -r -c --name-only\n> \n> Thanks, that looks promising and solves at least one of my use cases. The only\n> minor problem is that there seems to be no way to pipe the diff-tree output to\n> cat-file without massaging it with awk first.\n> \n> I have three uses cases in my pre-receive hooks:\n> \n> 1. Filters solely based on the file name\n>    ? your suggestions works perfectly here\n> 2. Filters based only on file contents\n>    ? git rev-list --objects + git cat-file provide everything I need\n> 3. One filter based on file size and name (forbid large files, with exceptions)\n>    ? I'm guessing \"git rev-list | git diff-tree --stdin | awk |\n>      git cat-file --batch-check\" is the best solution to extract the necessary\n>      information from git in this case?\n\nYes, that's how I would do all of those. Having to massage the output\nbetween diff-tree and cat-file is a little annoying, but at least can\nstill be done in O(1) processes. And you really need some language\ncapable of parsing cat-file output anyway, so it's not too big of a\nlift.\n\nOne thing we did at GitHub is teach index-pack to collect a list of\ntoo-big paths (since naturally it knows the size of every blob as it\nindexes it), and write them out to a special path. Then our pre-receive\nhook could quickly check the list and act on it (warning above a certain\nsize, rejecting above another), without having to traverse again.\n\nI haven't sent those patches upstream, though, for a few reasons:\n\n  - we only hooked index-pack, not unpack-objects (we only use the\n    former, not the latter, but in stock Git you might see either)\n\n  - getting just the object names is kind of awkward. You have to then\n    invoke rev-list to find the names of hits (though at least you only\n    do so when there is a problem object; the happy case remains fast).\n\n  - it's actually not that helpful to avoid the traversal if you have\n    other stuff you want to check anyway (like file contents, or names).\n    It was one of those things we optimized a long time ago, and I kind\n    of doubt is doing much good these days.\n\n-Peff\n"},{"id":"474451","messageId":"AS1PR02MB81854CDA4768A8DB3C51FCCA948E9@AS1PR02MB8185.eurprd02.prod.outlook.com","threadId":"59467","inReplyTo":"20230328182650.GC18558@coredump.intra.peff.net","subject":"RE: [PATCH] docs: document caveats of rev-list's object-name output","fromName":"Baumann, Moritz","fromEmail":"moritz.baumann@sap.com","sentAt":"2023-03-30T10:32:16Z","receivedAt":"2023-03-30T10:32:30Z","isPatch":true,"sender":{"key":"moritz.baumann@sap.com","avatar":"https://avatars.githubusercontent.com/u/67415316?v=4"},"body":"> > > Those names are really just intended as hints for pack-objects. I\n> > > suspect the documentation could be more clear about these limitations.\n> >\n> > That would indeed be great and would have likely prevented the obvious\n> > misconceptions on my side.\n>\n> Here's what I came up with.\n\nThanks, this is one half of what I would have needed to read.\n\n> I also considered adding a specific \"if you want the names of each file\n> in a range of commits, pipe to diff-tree\" example. But it seemed like it\n> would clutter up this section. It might be OK as a stand-alone in the\n> EXAMPLES section, but should probably be done as a separate patch if\n> anyone is interested.\n\nThat would be the other half. Since e.g. GitHub's own \"best practice\" examples\ndo not use this pattern [0], I would assume that I'm not the only one who\ndidn't know about it.\n\n[0] https://github.com/github/platform-samples/blob/master/pre-receive-hooks/block_file_extensions.sh\n"}]}