{"thread":{"id":"28198","subject":"What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","startedAt":"2011-08-23T07:25:17Z","lastAt":"2012-01-18T08:59:52Z","messageCount":56,"participants":["Marat Radchenko","Michael J Gruber","Michael Witten","Junio C Hamano","Nguyen Thai Ngoc Duy","Linus Torvalds","David Michael Barr","Michael Haggerty","Jonathan Nieder","Nguyễn Thái Ngọc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"174078","messageId":"loom.20110823T091132-107@post.gmane.org","threadId":"28198","inReplyTo":null,"subject":"What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2011-08-23T07:25:17Z","receivedAt":"2011-08-23T07:25:17Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"$ time git show branch:file | diff -u - file > /dev/null \n\nreal 0m0.003s\nuser 0m0.000s\nsys 0m0.000s\n\n$ time git diff branch -- file > /dev/null \n\nreal 0m31.442s\nuser 0m31.040s\nsys 0m0.380s\n\nWhat does git diff do so it takes that much time? And is there any flag to git \ndiff so that it will work as fast as show + diff? I thought these two are \nequivalent but from run time it is obvious that they aren't.\n\ngprof output: http://slonopotamus.org/git-diff/git-diff-branch.gprof.txt\n"},{"id":"174083","messageId":"4E537AF0.9070604@drmicha.warpmail.net","threadId":"28198","inReplyTo":"loom.20110823T091132-107@post.gmane.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-08-23T10:03:28Z","receivedAt":"2011-08-23T10:03:28Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Marat Radchenko venit, vidit, dixit 23.08.2011 09:25:\n> $ time git show branch:file | diff -u - file > /dev/null \n> \n> real 0m0.003s\n> user 0m0.000s\n> sys 0m0.000s\n> \n> $ time git diff branch -- file > /dev/null \n> \n> real 0m31.442s\n> user 0m31.040s\n> sys 0m0.380s\n> \n> What does git diff do so it takes that much time? And is there any flag to git \n> diff so that it will work as fast as show + diff? I thought these two are \n> equivalent but from run time it is obvious that they aren't.\n> \n> gprof output: http://slonopotamus.org/git-diff/git-diff-branch.gprof.txt\n> \n\nIs that a very large tree or a very slow file system?\n\n>From the gprof output, it would appear that we compare a lot of cache\nentries (or rather: spent most time on...). Do we enumerate all\ndiffering files and only then limit diff output by path??\n\nMichael\n"},{"id":"174088","messageId":"1314096731.15017.2.camel@n900.home.ru","threadId":"28198","inReplyTo":"4E537AF0.9070604@drmicha.warpmail.net","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2011-08-23T10:52:11Z","receivedAt":"2011-08-23T10:52:11Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"> Is that a very large tree or a very slow file system?\nTree is large (500k files), file system is irrelevant since all time is spend on CPU.\n\n> Do we enumerate all\n> differing files and only then limit diff output by path??\n\nDunno, that's why I am asking why it is so slow.\n"},{"id":"174091","messageId":"CAMOZ1BsLS9kN5M98s7mqELSsPjFOgFcNbDq6G9rhoyb1txL08w@mail.gmail.com","threadId":"28198","inReplyTo":"1314096731.15017.2.camel@n900.home.ru","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Michael Witten","fromEmail":"mfwitten@gmail.com","sentAt":"2011-08-23T15:20:26Z","receivedAt":"2011-08-23T15:20:26Z","isPatch":false,"sender":{"key":"mfwitten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/597101?v=4"},"body":"On Tue, Aug 23, 2011 at 10:52, Marat Radchenko wrote:\n>> Do we enumerate all differing files and only then\n>> limit diff output by path??\n>\n> Dunno, that's why I am asking why it is so slow.\n\nWhy in the world do you think that question was directed at you?\n"},{"id":"174092","messageId":"4E53C89A.9000604@drmicha.warpmail.net","threadId":"28198","inReplyTo":"1314096731.15017.2.camel@n900.home.ru","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-08-23T15:34:50Z","receivedAt":"2011-08-23T15:34:50Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Marat Radchenko venit, vidit, dixit 23.08.2011 12:52:\n>> Is that a very large tree or a very slow file system?\n> Tree is large (500k files), file system is irrelevant since all time is spend on CPU.\n> \n>> Do we enumerate all\n>> differing files and only then limit diff output by path??\n> \n> Dunno, that's why I am asking why it is so slow.\n\nWell, we have to read the full tree before diffing. But I can't\nreproduce the extreme difference which you observed (0.003s vs. 30s),\nonly a factor of ten or so for a repo with 100k files:\n\ngit init\nseq 0 100000|while read n ; do echo a > a$n;done\ngit add .\ngit commit -m m\necho b > a0\n\nOn a ramdisk, I get:\n\ntime git diff > /dev/null\n\nreal 0m0.160s\nuser 0m0.064s\nsys 0m0.190s\n\ntime git diff -- a0 > /dev/null\n\nreal 0m0.070s\nuser 0m0.051s\nsys 0m0.021s\n\ntime git diff HEAD > /dev/null\n\nreal 0m0.266s\nuser 0m0.145s\nsys 0m0.212s\n\ntime git diff HEAD -- a0 > /dev/null\n\nreal 0m0.171s\nuser 0m0.136s\nsys 0m0.033s\n\ntime git show HEAD:a0 > /dev/null\n\nreal 0m0.018s\nuser 0m0.009s\nsys 0m0.007s\n\ntime git show HEAD:a0 | diff -u - a0 > /dev/null\n\nreal 0m0.019s\nuser 0m0.010s\nsys 0m0.008s\n\nStumped.\n\nIn your case, do you have a lot of differing files besides the one you\nare limitting to? Anyway, that does not seem to make a huge difference\nin my timings. (Just tried.) Still Stumped.\n\nMichael\n"},{"id":"174098","messageId":"1314117902.17151.2.camel@n900.home.ru","threadId":"28198","inReplyTo":"4E53C89A.9000604@drmicha.warpmail.net","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2011-08-23T16:45:02Z","receivedAt":"2011-08-23T16:45:02Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"On вт 23 авг 2011 19:34:50 MSD, Michael J Gruber wrote: \n> Well, we have to read the full tree before diffing.\nI don't see why you need that.\n\n> But I can't\n> reproduce the extreme difference which you observed (0.003s vs. 30s)\nWell, I have an extreme repo (sadly, private) that has already shown several scalability issues in various parts of git code. Hope this thread will help to improve it.\n\n> In your case, do you have a lot of differing files besides the one you\n> are limitting to?\n1. I'm diffing a single (and rather small, <50kb) text file\n2. Diff is done between two branches (master and bugfix for a particular release) one of which (master) already has several thousands of commits after fork, so yes, whole tree diffs a lot.\n\nP.S. Fix for [1] might somewhat improve the situation but it still isn't clear to me why whole tree needs to be processed when specific path is given. Btw, it is 30s even with --no-renames.\n\n[1] http://git.661346.n2.nabble.com/git-diff-is-slow-patience-is-fast-td6667216.html\n"},{"id":"174102","messageId":"7vk4a4rqvb.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"4E53C89A.9000604@drmicha.warpmail.net","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-23T17:15:36Z","receivedAt":"2011-08-23T17:15:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Marat Radchenko venit, vidit, dixit 23.08.2011 12:52:\n>>> Is that a very large tree or a very slow file system?\n>> Tree is large (500k files), file system is irrelevant since all time is spend on CPU.\n>> \n>>> Do we enumerate all\n>>> differing files and only then limit diff output by path??\n>> \n>> Dunno, that's why I am asking why it is so slow.\n>\n> Well, we have to read the full tree before diffing.\n\nNot necessarily, especially when pathspec is given like the original post,\ni.e. \"git diff $tree_ish -- $path\". We would need to open tree objects\nthat lead to the leaf of the $path and a blob, but other objects won't be\nneeded.\n\nThe default diff backend tries to come up with minimal changes by spending\nextra cycles, so it is not so surprising if the file compared is large-ish\nand/or has very many similar lines in itself (in which case there are many\npotential matching line pairs between the preimage and the postimage to be\nexamined to produce a minimal diff).\n"},{"id":"174108","messageId":"loom.20110823T201645-980@post.gmane.org","threadId":"28198","inReplyTo":"7vk4a4rqvb.fsf@alter.siamese.dyndns.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2011-08-23T18:21:48Z","receivedAt":"2011-08-23T18:21:48Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Junio C Hamano pobox.com> writes:\n> Not necessarily, especially when pathspec is given like the original post,\n> i.e. \"git diff $tree_ish -- $path\". We would need to open tree objects\n> that lead to the leaf of the $path and a blob, but other objects won't be\n> needed.\nAnd git show branch:path already has that code.\n"},{"id":"174117","messageId":"4E54088F.7050700@drmicha.warpmail.net","threadId":"28198","inReplyTo":"7vk4a4rqvb.fsf@alter.siamese.dyndns.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-08-23T20:07:43Z","receivedAt":"2011-08-23T20:07:43Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 23.08.2011 19:15:\n> Michael J Gruber writes:\n> \n>> Marat Radchenko venit, vidit, dixit 23.08.2011 12:52:\n>>>> Is that a very large tree or a very slow file system?\n>>> Tree is large (500k files), file system is irrelevant since all time is spend on CPU.\n>>>\n>>>> Do we enumerate all\n>>>> differing files and only then limit diff output by path??\n>>>\n>>> Dunno, that's why I am asking why it is so slow.\n>>\n>> Well, we have to read the full tree before diffing.\n> \n> Not necessarily, especially when pathspec is given like the original post,\n> i.e. \"git diff $tree_ish -- $path\". We would need to open tree objects\n> that lead to the leaf of the $path and a blob, but other objects won't be\n> needed.\n\nI meant: The way \"git diff\" is now, it does that.\n\n> \n> The default diff backend tries to come up with minimal changes by spending\n> extra cycles, so it is not so surprising if the file compared is large-ish\n> and/or has very many similar lines in itself (in which case there are many\n> potential matching line pairs between the preimage and the postimage to be\n> examined to produce a minimal diff).\n\nBut the file in this case is not that large, and \"git diff\" spends 30s!\n\nMichael\n"},{"id":"174237","messageId":"1314288543.8665.5.camel@n900.home.ru","threadId":"28198","inReplyTo":"4E54088F.7050700@drmicha.warpmail.net","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2011-08-25T16:09:03Z","receivedAt":"2011-08-25T16:09:03Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"On 08/24/2011 00:07:43 MSD, Michael J Gruber wrote:\n\n> Junio C Hamano venit, vidit, dixit 23.08.2011 19:15:\n> > Michael J Gruber writes:\n> > \n> > > Marat Radchenko venit, vidit, dixit 23.08.2011 12:52:\n> > > > > Is that a very large tree or a very slow file system?\n> > > > Tree is large (500k files), file system is irrelevant since all\n> > > > time is spend on CPU.\n> > > > \n> > > > > Do we enumerate all\n> > > > > differing files and only then limit diff output by path??\n> > > > \n> > > > Dunno, that's why I am asking why it is so slow.\n> > > \n> > > Well, we have to read the full tree before diffing.\n> > \n> > Not necessarily, especially when pathspec is given like the original\n> > post, i.e. \"git diff $tree_ish -- $path\". We would need to open tree\n> > objects that lead to the leaf of the $path and a blob, but other\n> > objects won't be needed.\n> \n> I meant: The way \"git diff\" is now, it does that.\n> \n> > \n> > The default diff backend tries to come up with minimal changes by\n> > spending extra cycles, so it is not so surprising if the file compared\n> > is large-ish and/or has very many similar lines in itself (in which\n> > case there are many potential matching line pairs between the preimage\n> > and the postimage to be examined to produce a minimal diff).\n> \n> But the file in this case is not that large, and \"git diff\" spends 30s!\n\nSo, is some more info required from me or gprof output given in initial report + following discussion are enough to conclude what code needs to be improved?\n"},{"id":"174267","messageId":"7vy5yhi4eq.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"4E54088F.7050700@drmicha.warpmail.net","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-25T21:10:05Z","receivedAt":"2011-08-25T21:10:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>> The default diff backend tries to come up with minimal changes by spending\n>> extra cycles, so it is not so surprising if the file compared is large-ish\n>> and/or has very many similar lines in itself (in which case there are many\n>> potential matching line pairs between the preimage and the postimage to be\n>> examined to produce a minimal diff).\n>\n> But the file in this case is not that large, and \"git diff\" spends 30s!\n\nIf the difference was literally between\n\n $ git diff branch -- file\n $ git show branch:file | diff -u - file\n\nthat is, \"file\" is the name of a file in the top-level directory, I would\nexpect that former would open the top-level tree object for the branch,\nread it thru until it finds \"file\", grabs a single blob and deflate it in\ncore, and compare that with the contents of a single file read from the\nfilesystem.\n\nAn interesting comparison may be to run this once:\n\n $ git show branch:file >fileI\n\nand then compare between these two:\n\n $ diff -u fileI file\n $ git diff --no-index fileI file\n\nIf the latter is slower than the former in the same way as the original\nexperiment, that would mean that the tree traversal time does not have\nanything to do with it (iow, your \"The way 'git diff' is now, it does\nthat\" is not just incorrect---we don't read the full tree to begin\nwith---but irrelevant).\n\nIf the \"we try to come up with minimal changes by spending extra cycles\" I\nmentioned in my messages is indeed the cause, you may see diffferences\nrunning the \"git diff --no-index\" version with backend options, e.g.\n\n $ git diff --no-index --patience fileI file\n $ git diff --no-index --histogram fileI file\n\nAnother thing to try may be to run the version from \"next\" that has\n27af01d (xdiff/xprepare: improve O(n*m) performance in xdl_cleanup_records(),\n2011-08-17), without any backend options.\n"},{"id":"174308","messageId":"loom.20110826T113153-510@post.gmane.org","threadId":"28198","inReplyTo":"7vy5yhi4eq.fsf@alter.siamese.dyndns.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2011-08-26T09:43:31Z","receivedAt":"2011-08-26T09:43:31Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Junio C Hamano pobox.com> writes:\n> An interesting comparison may be to run this once:\n> \n> $ git show branch:file >fileI\n> \n> and then compare between these two:\n> \n> $ diff -u fileI file\n> $ git diff --no-index fileI file\n\nIn all times below, \"file\" is the same file i used in original report.\n\n$ time diff -u fileI file > /dev/null \n\nreal 0m0.002s\nuser 0m0.000s\nsys 0m0.000s\n\n$ time git diff --no-index fileI file > /dev/null \n\nreal 0m0.331s\nuser 0m0.270s\nsys 0m0.060s\n\nSame with \"pu\" branch (has 27af01d):\n$ time ~/git/git-diff --no-index fileI file > /dev/null \n\nreal 0m0.307s\nuser 0m0.220s\nsys 0m0.080s\n\n> \n> If the latter is slower than the former in the same way as the original\n> experiment, that would mean that the tree traversal time does not have\n> anything to do with it (iow, your \"The way 'git diff' is now, it does\n> that\" is not just incorrect---we don't read the full tree to begin\n> with---but irrelevant).\nI still suspect tree traversal. Original report with \"pu\" branch is still 30s.\n"},{"id":"174451","messageId":"CACsJy8DMqjMfb5wVMnsrZhP8yv2rF1wyQ6LM6b-PVcYY1SXkcQ@mail.gmail.com","threadId":"28198","inReplyTo":"loom.20110823T091132-107@post.gmane.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-29T07:41:45Z","receivedAt":"2011-08-29T07:41:45Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 23, 2011 at 2:25 PM, Marat Radchenko wrote:\n> $ time git show branch:file | diff -u - file > /dev/null\n>\n> real    0m0.003s\n> user    0m0.000s\n> sys     0m0.000s\n>\n> $ time git diff branch -- file > /dev/null\n>\n> real    0m31.442s\n> user    0m31.040s\n> sys     0m0.380s\n>\n> What does git diff do so it takes that much time?\n\nYou said elsewhere in this thread this is private repo, so some more questions:\n\n - is \"file\" above at top repo, or is it actually very/deep/path/to/a/file?\n - how many entries in the tree that contain \"file\"?\n - how is \"git ls-files | wc -l\"?\n - how about \"time git diff branch another-branch -- file >/dev/null\"?\nThat'd remove unpack-trees code.\n-- \nDuy\n"},{"id":"174457","messageId":"loom.20110829T155805-331@post.gmane.org","threadId":"28198","inReplyTo":"CACsJy8DMqjMfb5wVMnsrZhP8yv2rF1wyQ6LM6b-PVcYY1SXkcQ@mail.gmail.com","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2011-08-29T14:48:05Z","receivedAt":"2011-08-29T14:48:05Z","isPatch":false,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Nguyen Thai Ngoc Duy gmail.com> writes:\n> - is \"file\" above at top repo, or is it actually very/deep/path/to/a/file?\n3 levels deep. Most parent dir (one after repo root) contains 20k files.\n\n> - how many entries in the tree that contain \"file\"?\nSorry, didn't understand this.\n\n> - how is \"git ls-files | wc -l\"?\n$ time git ls-files | wc -l\n603137\n\nreal 0m0.417s\nuser 0m0.440s\nsys 0m0.060s\n\n> - how about \"time git diff branch another-branch -- file >/dev/null\"?\n> That'd remove unpack-trees code.\nPretty fast:\n$ time git diff branch:file other_branch:file > /dev/null\n\nreal 0m0.278s\nuser 0m0.210s\nsys 0m0.060s\n\nOne more test:\ngit diff HEAD branch -- file > /dev/null \n\nreal 0m0.276s\nuser 0m0.240s\nsys 0m0.030s\n\nSo the only troubled variant is `git diff branch -- file`.\n"},{"id":"174460","messageId":"CACsJy8Dar5i3Fn+rhOq78vdsqRL4D+RNUc5G64BM-6DvKC=L5w@mail.gmail.com","threadId":"28198","inReplyTo":"loom.20110829T155805-331@post.gmane.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-29T16:09:05Z","receivedAt":"2011-08-29T16:09:05Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 29, 2011 at 9:48 PM, Marat Radchenko wrote:\n> Nguyen Thai Ngoc Duy gmail.com> writes:\n>>  - is \"file\" above at top repo, or is it actually very/deep/path/to/a/file?\n> 3 levels deep. Most parent dir (one after repo root) contains 20k files.\n>\n>>  - how many entries in the tree that contain \"file\"?\n> Sorry, didn't understand this.\n\nYou have already answered it. I was asking the size of parent dir, but\nphrased poorly.\n\n>>  - how is \"git ls-files | wc -l\"?\n> $ time git ls-files | wc -l\n> 603137\n>\n> real    0m0.417s\n> user    0m0.440s\n> sys     0m0.060s\n>\n>>  - how about \"time git diff branch another-branch -- file >/dev/null\"?\n>> That'd remove unpack-trees code.\n> Pretty fast:\n>\n> git diff HEAD branch -- file > /dev/null\n>\n> real    0m0.276s\n> user    0m0.240s\n> sys     0m0.030s\n\nThat may explain it. \"git diff \" walks through the index, unpacks\ntree objects along the way, matches up entries with the same path from\nthe branch, the index then feeds matching entries to diff function. If\ntree cutting is not done efficiently, it could very well walk through\nevery entry in the index (~600k entries in your case), unpacking all\ntree objects along the way.\n\nAnd it looks like to me that diff_cache() in diff-lib.c, responsible\nfor this case, does not do any prefix trimming. traverse_trees() also\ndoes not seem to do \"never_interesting\" optimization like in\ntree_interesting(), so if the traversed tree is big (~20k as you told\nme), it will take some time, even though you are only interested in a\nsingle entry.\n\n> So the only troubled variant is `git diff branch -- file`.\n\nNo, I suspect \"git diff --cached\" would be also slow. \"git merge\"\nwould be definitely slow. But we can hardly improve these cases\nbecause the commands are usually called tree-wide, no path limiting.\n\nIf you only work on a small subset of files, there's some (unfinished)\ncode in narrow clone implementation that cuts down index size, which\nmay speed up in big-index repositories like yours. Could be a good\nreason for me (or someone) to extract that part and get it in before\nfull narrow clone is implemented.\n-- \nDuy\n"},{"id":"174469","messageId":"7v1uw46srf.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"CACsJy8Dar5i3Fn+rhOq78vdsqRL4D+RNUc5G64BM-6DvKC=L5w@mail.gmail.com","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T17:18:28Z","receivedAt":"2011-08-29T17:18:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> That may explain it. \"git diff \" walks through the index, unpacks\n> tree objects along the way, matches up entries with the same path from\n> the branch, the index then feeds matching entries to diff function.\n\nYeah, that would explain it. The implementation of diff-index has changed\na few times and the latest incarnation is based on unpack-trees, _merging_\nthe tree and the index, introduced by d1f2d7e (Make run_diff_index() use\nunpack_trees(), not read_tree(), 2008-01-19).\n\nOf course the merge machinery does not know anything about pruning with\npathspec, so it is understandable (not justifiable) it would walk the full\ntree.\n\nWill try to find time this week to cook up something.\n"},{"id":"174493","messageId":"7vty9054qr.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"7v1uw46srf.fsf@alter.siamese.dyndns.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T20:42:36Z","receivedAt":"2011-08-29T20:42:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Of course the merge machinery does not know anything about pruning with\n> pathspec, so it is understandable (not justifiable) it would walk the full\n> tree.\n>\n> Will try to find time this week to cook up something.\n\nThis is still rough, but seems to pass the test suite, and gives me some\nperformance boost when applied to the kernel tree:\n\n (without patch)\n $ time git diff --raw --cached v2.6.30 -- virt/kvm\n real 0m0.114s\n user 0m0.088s\n sys 0m0.028s\n\n (with patch)\n $ time ./git diff --raw --cached v2.6.30 -- virt/kvm\n real 0m0.075s\n user 0m0.068s\n sys 0m0.008s\n\nWhat I do not like about it most is that we have an infrastructure that\nlinks traverse_info across stackframes to store the paths unexpanded and\nwithout extra linear allocation, but tree_entry_interesting() wants the\npath as a single string. Hence unpack_trees() carries an extra baggage\n\"base\" string, even though the general callback in tree-walk machinery\ndoes not need it.\n\nI think this can be trivially optimized to keep a pointer to a single\nstrbuf in the traverse_info (initialize it at the same points as this\npatch sets info.pathspec), extending it as it digs deeper (copy the same\npointer to the strbuf to a child traverse_info and tuck the name of the\ndirectory it descends into it, at the same points as this patch copies\ninfo->pathspec from the parent), and resetting the length back when the\ntraversal into a subdirectory comes back.\n\n\n\n diff-lib.c | 1 +\n tree-walk.c | 39 +++++++++++++++++++++++++++++++++------\n tree-walk.h | 1 +\n unpack-trees.c | 2 ++\n unpack-trees.h | 1 +\n 5 files changed, 38 insertions(+), 6 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f8454dd..ebe751e 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -468,6 +468,7 @@ static int diff_cache(struct rev_info *revs,\n \topts.unpack_data = revs;\n \topts.src_index = &the_index;\n \topts.dst_index = NULL;\n+\topts.pathspec = &revs->diffopt.pathspec;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \treturn unpack_trees(1, &t, &opts);\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 33f749e..808bb55 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -309,6 +309,18 @@ static void free_extended_entry(struct tree_desc_x *t)\n \t}\n }\n \n+static inline int prune_traversal(struct name_entry *e,\n+\t\t\t\t struct traverse_info *info,\n+\t\t\t\t struct strbuf *base,\n+\t\t\t\t int still_interesting)\n+{\n+\tif (!info->pathspec || still_interesting == 2)\n+\t\treturn 2;\n+\tif (still_interesting < 0)\n+\t\treturn still_interesting;\n+\treturn tree_entry_interesting(e, base, 0, info->pathspec);\n+}\n+\n int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n {\n \tint ret = 0;\n@@ -316,10 +328,18 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \tstruct name_entry *entry = xmalloc(n*sizeof(*entry));\n \tint i;\n \tstruct tree_desc_x *tx = xcalloc(n, sizeof(*tx));\n+\tstruct strbuf base = STRBUF_INIT;\n+\tint interesting = 1;\n \n \tfor (i = 0; i < n; i++)\n \t\ttx[i].d = t[i];\n \n+\tif (info->prev) {\n+\t\tstrbuf_grow(&base, info->pathlen);\n+\t\tmake_traverse_path(base.buf, info->prev, &info->name);\n+\t\tbase.buf[info->pathlen-1] = '/';\n+\t\tstrbuf_setlen(&base, info->pathlen);\n+\t}\n \tfor (;;) {\n \t\tunsigned long mask, dirmask;\n \t\tconst char *first = NULL;\n@@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\t\tmask |= 1ul << i;\n \t\t\tif (S_ISDIR(entry[i].mode))\n \t\t\t\tdirmask |= 1ul << i;\n+\t\t\te = &entry[i];\n \t\t}\n \t\tif (!mask)\n \t\t\tbreak;\n-\t\tret = info->fn(n, mask, dirmask, entry, info);\n-\t\tif (ret < 0) {\n-\t\t\terror = ret;\n-\t\t\tif (!info->show_all_errors)\n-\t\t\t\tbreak;\n+\t\tinteresting = prune_traversal(e, info, &base, interesting);\n+\t\tif (interesting < 0)\n+\t\t\tbreak;\n+\t\tif (interesting) {\n+\t\t\tret = info->fn(n, mask, dirmask, entry, info);\n+\t\t\tif (ret < 0) {\n+\t\t\t\terror = ret;\n+\t\t\t\tif (!info->show_all_errors)\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tmask &= ret;\n \t\t}\n-\t\tmask &= ret;\n \t\tret = 0;\n \t\tfor (i = 0; i < n; i++)\n \t\t\tif (mask & (1ul << i))\n@@ -395,6 +421,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \tfor (i = 0; i < n; i++)\n \t\tfree_extended_entry(tx + i);\n \tfree(tx);\n+\tstrbuf_release(&base);\n \treturn error;\n }\n \ndiff --git a/tree-walk.h b/tree-walk.h\nindex 39524b7..0089581 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -44,6 +44,7 @@ struct traverse_info {\n \tstruct traverse_info *prev;\n \tstruct name_entry name;\n \tint pathlen;\n+\tstruct pathspec *pathspec;\n \n \tunsigned long conflicts;\n \ttraverse_callback_t fn;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cc616c3..670b464 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -444,6 +444,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \n \tnewinfo = *info;\n \tnewinfo.prev = info;\n+\tnewinfo.pathspec = info->pathspec;\n \tnewinfo.name = *p;\n \tnewinfo.pathlen += tree_entry_len(p->path, p->sha1) + 1;\n \tnewinfo.conflicts |= df_conflicts;\n@@ -1040,6 +1041,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tinfo.fn = unpack_callback;\n \t\tinfo.data = o;\n \t\tinfo.show_all_errors = o->show_all_errors;\n+\t\tinfo.pathspec = o->pathspec;\n \n \t\tif (o->prefix) {\n \t\t\t/*\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 7998948..5e432f5 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -52,6 +52,7 @@ struct unpack_trees_options {\n \tconst char *prefix;\n \tint cache_bottom;\n \tstruct dir_struct *dir;\n+\tstruct pathspec *pathspec;\n \tmerge_fn_t fn;\n \tconst char *msgs[NB_UNPACK_TREES_ERROR_TYPES];\n \t/*\n"},{"id":"174495","messageId":"7vliuc54ct.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"7vty9054qr.fsf@alter.siamese.dyndns.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T20:50:58Z","receivedAt":"2011-08-29T20:50:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Of course the merge machinery does not know anything about pruning with\n>> pathspec, so it is understandable (not justifiable) it would walk the full\n>> tree.\n>>\n>> Will try to find time this week to cook up something.\n>\n> This is still rough, but seems to pass the test suite, and gives me some\n> performance boost when applied to the kernel tree:\n\nEhh, no, the patch does not apply to the kernel tree, but I meant \"when\nused on the kernel tree\".\n\n> (without patch)\n> $ time git diff --raw --cached v2.6.30 -- virt/kvm\n> real 0m0.114s\n> user 0m0.088s\n> sys 0m0.028s\n>\n> (with patch)\n> $ time ./git diff --raw --cached v2.6.30 -- virt/kvm\n> real 0m0.075s\n> user 0m0.068s\n> sys 0m0.008s\n>\n> What I do not like about it most is that we have an infrastructure that\n> links traverse_info across stackframes to store the paths unexpanded and\n> without extra linear allocation, but tree_entry_interesting() wants the\n> path as a single string. Hence unpack_trees() carries an extra baggage\n> \"base\" string, even though the general callback in tree-walk machinery\n> does not need it.\n>\n> I think this can be trivially optimized to keep a pointer to a single\n> strbuf in the traverse_info (initialize it at the same points as this\n> patch sets info.pathspec), extending it as it digs deeper (copy the same\n> pointer to the strbuf to a child traverse_info and tuck the name of the\n> directory it descends into it, at the same points as this patch copies\n> info->pathspec from the parent), and resetting the length back when the\n> traversal into a subdirectory comes back.\n>\n>\n>\n> diff-lib.c | 1 +\n> tree-walk.c | 39 +++++++++++++++++++++++++++++++++------\n> tree-walk.h | 1 +\n> unpack-trees.c | 2 ++\n> unpack-trees.h | 1 +\n> 5 files changed, 38 insertions(+), 6 deletions(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index f8454dd..ebe751e 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -468,6 +468,7 @@ static int diff_cache(struct rev_info *revs,\n> \topts.unpack_data = revs;\n> \topts.src_index = &the_index;\n> \topts.dst_index = NULL;\n> +\topts.pathspec = &revs->diffopt.pathspec;\n> \n> \tinit_tree_desc(&t, tree->buffer, tree->size);\n> \treturn unpack_trees(1, &t, &opts);\n> diff --git a/tree-walk.c b/tree-walk.c\n> index 33f749e..808bb55 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -309,6 +309,18 @@ static void free_extended_entry(struct tree_desc_x *t)\n> \t}\n> }\n> \n> +static inline int prune_traversal(struct name_entry *e,\n> +\t\t\t\t struct traverse_info *info,\n> +\t\t\t\t struct strbuf *base,\n> +\t\t\t\t int still_interesting)\n> +{\n> +\tif (!info->pathspec || still_interesting == 2)\n> +\t\treturn 2;\n> +\tif (still_interesting < 0)\n> +\t\treturn still_interesting;\n> +\treturn tree_entry_interesting(e, base, 0, info->pathspec);\n> +}\n> +\n> int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n> {\n> \tint ret = 0;\n> @@ -316,10 +328,18 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n> \tstruct name_entry *entry = xmalloc(n*sizeof(*entry));\n> \tint i;\n> \tstruct tree_desc_x *tx = xcalloc(n, sizeof(*tx));\n> +\tstruct strbuf base = STRBUF_INIT;\n> +\tint interesting = 1;\n> \n> \tfor (i = 0; i < n; i++)\n> \t\ttx[i].d = t[i];\n> \n> +\tif (info->prev) {\n> +\t\tstrbuf_grow(&base, info->pathlen);\n> +\t\tmake_traverse_path(base.buf, info->prev, &info->name);\n> +\t\tbase.buf[info->pathlen-1] = '/';\n> +\t\tstrbuf_setlen(&base, info->pathlen);\n> +\t}\n> \tfor (;;) {\n> \t\tunsigned long mask, dirmask;\n> \t\tconst char *first = NULL;\n> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n> \t\t\tmask |= 1ul << i;\n> \t\t\tif (S_ISDIR(entry[i].mode))\n> \t\t\t\tdirmask |= 1ul << i;\n> +\t\t\te = &entry[i];\n> \t\t}\n> \t\tif (!mask)\n> \t\t\tbreak;\n> -\t\tret = info->fn(n, mask, dirmask, entry, info);\n> -\t\tif (ret < 0) {\n> -\t\t\terror = ret;\n> -\t\t\tif (!info->show_all_errors)\n> -\t\t\t\tbreak;\n> +\t\tinteresting = prune_traversal(e, info, &base, interesting);\n> +\t\tif (interesting < 0)\n> +\t\t\tbreak;\n> +\t\tif (interesting) {\n> +\t\t\tret = info->fn(n, mask, dirmask, entry, info);\n> +\t\t\tif (ret < 0) {\n> +\t\t\t\terror = ret;\n> +\t\t\t\tif (!info->show_all_errors)\n> +\t\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t\tmask &= ret;\n> \t\t}\n> -\t\tmask &= ret;\n> \t\tret = 0;\n> \t\tfor (i = 0; i < n; i++)\n> \t\t\tif (mask & (1ul << i))\n> @@ -395,6 +421,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n> \tfor (i = 0; i < n; i++)\n> \t\tfree_extended_entry(tx + i);\n> \tfree(tx);\n> +\tstrbuf_release(&base);\n> \treturn error;\n> }\n> \n> diff --git a/tree-walk.h b/tree-walk.h\n> index 39524b7..0089581 100644\n> --- a/tree-walk.h\n> +++ b/tree-walk.h\n> @@ -44,6 +44,7 @@ struct traverse_info {\n> \tstruct traverse_info *prev;\n> \tstruct name_entry name;\n> \tint pathlen;\n> +\tstruct pathspec *pathspec;\n> \n> \tunsigned long conflicts;\n> \ttraverse_callback_t fn;\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index cc616c3..670b464 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -444,6 +444,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n> \n> \tnewinfo = *info;\n> \tnewinfo.prev = info;\n> +\tnewinfo.pathspec = info->pathspec;\n> \tnewinfo.name = *p;\n> \tnewinfo.pathlen += tree_entry_len(p->path, p->sha1) + 1;\n> \tnewinfo.conflicts |= df_conflicts;\n> @@ -1040,6 +1041,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \t\tinfo.fn = unpack_callback;\n> \t\tinfo.data = o;\n> \t\tinfo.show_all_errors = o->show_all_errors;\n> +\t\tinfo.pathspec = o->pathspec;\n> \n> \t\tif (o->prefix) {\n> \t\t\t/*\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index 7998948..5e432f5 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -52,6 +52,7 @@ struct unpack_trees_options {\n> \tconst char *prefix;\n> \tint cache_bottom;\n> \tstruct dir_struct *dir;\n> +\tstruct pathspec *pathspec;\n> \tmerge_fn_t fn;\n> \tconst char *msgs[NB_UNPACK_TREES_ERROR_TYPES];\n> \t/*\n"},{"id":"174499","messageId":"7v62lf6i2i.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"7vty9054qr.fsf@alter.siamese.dyndns.org","subject":"Re: What's the difference between `git show branch:file | diff -u - file` vs `git diff branch file`?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T21:09:25Z","receivedAt":"2011-08-29T21:09:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Of course the merge machinery does not know anything about pruning with\n>> pathspec, so it is understandable (not justifiable) it would walk the full\n>> tree.\n>>\n>> Will try to find time this week to cook up something.\n>\n> This is still rough, but seems to pass the test suite, and gives me some\n> performance boost when applied to the kernel tree:\n>\n> (without patch)\n> $ time git diff --raw --cached v2.6.30 -- virt/kvm\n> real 0m0.114s\n> user 0m0.088s\n> sys 0m0.028s\n>\n> (with patch)\n> $ time ./git diff --raw --cached v2.6.30 -- virt/kvm\n> real 0m0.075s\n> user 0m0.068s\n> sys 0m0.008s\n\nHeh, this is even better without --cached.\n\n (without patch)\n $ /usr/bin/time git diff --raw v2.6.27 -- net/ipv6\n real 0m0.538s\n user 0m0.492s\n sys 0m0.048s\n\n (with patch)\n $ /usr/bin/time git diff --raw v2.6.27 -- net/ipv6\n real 0m0.038s\n user 0m0.028s\n sys 0m0.012s\n"},{"id":"174504","messageId":"1314653603-7533-1-git-send-email-gitster@pobox.com","threadId":"28198","inReplyTo":"7vty9054qr.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T21:33:20Z","receivedAt":"2011-08-29T21:33:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git diff A B -- $pathspec\" to compare two tree-ishes knew how to apply\npathspec to avoid opening trees that fall outside the area of interest,\nbut \"git diff A -- $pathspec\" used unpack_trees() machinery that was meant\nfor full-tree merges, and ended up reading the whole tree only to discard\npotentially major part of the work it does.\n\nBefore and after applying this series, looking for changes in the kernel\nrepository with a fairly narrow pathspec gets a moderate speeds up.\n\n (without patch)\n $ /usr/bin/time git diff --raw v2.6.27 -- net/ipv6 >/dev/null\n 0.48user 0.05system 0:00.53elapsed 100%CPU (0avgtext+0avgdata 163296maxresident)k\n 0inputs+952outputs (0major+11163minor)pagefaults 0swaps\n\n (with patch)\n $ /usr/bin/time git diff --raw v2.6.27 -- net/ipv6 >/dev/null\n 0.01user 0.00system 0:00.02elapsed 104%CPU (0avgtext+0avgdata 43856maxresident)k\n 0inputs+24outputs (0major+3688minor)pagefaults 0swaps\n\nJunio C Hamano (3):\n tree-walk: allow pruning with pathspec\n unpack-trees: allow pruning with pathspec\n diff-index: pass pathspec down to unpack-trees machinery\n\n diff-lib.c | 1 +\n tree-walk.c | 39 +++++++++++++++++++++++++++++++++------\n tree-walk.h | 1 +\n unpack-trees.c | 2 ++\n unpack-trees.h | 1 +\n 5 files changed, 38 insertions(+), 6 deletions(-)\n\n-- \n1.7.7.rc0.70.g82660\n"},{"id":"174505","messageId":"1314653603-7533-2-git-send-email-gitster@pobox.com","threadId":"28198","inReplyTo":"1314653603-7533-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/3] traverse_trees(): allow pruning with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T21:33:21Z","receivedAt":"2011-08-29T21:33:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The traverse_trees() machinery is primarily meant for merging two (or\nmore) trees, and because a merge is a full tree operation, it doesn't\nsupport any pruning with pathspec.\n\nSince d1f2d7e (Make run_diff_index() use unpack_trees(), not read_tree(),\n2008-01-19), however, we use unpack_trees() to traverse_trees() callchain\nto perform \"diff-index\", which could waste a lot of work traversing trees\noutside the user-supplied pathspec, only to discard at the blob comparison\nlevel in diff-lib.c::oneway_diff() which is way too late.\n\nSigned-off-by: Junio C Hamano \n---\n tree-walk.c | 39 +++++++++++++++++++++++++++++++++------\n tree-walk.h | 1 +\n 2 files changed, 34 insertions(+), 6 deletions(-)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 33f749e..808bb55 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -309,6 +309,18 @@ static void free_extended_entry(struct tree_desc_x *t)\n \t}\n }\n \n+static inline int prune_traversal(struct name_entry *e,\n+\t\t\t\t struct traverse_info *info,\n+\t\t\t\t struct strbuf *base,\n+\t\t\t\t int still_interesting)\n+{\n+\tif (!info->pathspec || still_interesting == 2)\n+\t\treturn 2;\n+\tif (still_interesting < 0)\n+\t\treturn still_interesting;\n+\treturn tree_entry_interesting(e, base, 0, info->pathspec);\n+}\n+\n int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n {\n \tint ret = 0;\n@@ -316,10 +328,18 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \tstruct name_entry *entry = xmalloc(n*sizeof(*entry));\n \tint i;\n \tstruct tree_desc_x *tx = xcalloc(n, sizeof(*tx));\n+\tstruct strbuf base = STRBUF_INIT;\n+\tint interesting = 1;\n \n \tfor (i = 0; i < n; i++)\n \t\ttx[i].d = t[i];\n \n+\tif (info->prev) {\n+\t\tstrbuf_grow(&base, info->pathlen);\n+\t\tmake_traverse_path(base.buf, info->prev, &info->name);\n+\t\tbase.buf[info->pathlen-1] = '/';\n+\t\tstrbuf_setlen(&base, info->pathlen);\n+\t}\n \tfor (;;) {\n \t\tunsigned long mask, dirmask;\n \t\tconst char *first = NULL;\n@@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\t\tmask |= 1ul << i;\n \t\t\tif (S_ISDIR(entry[i].mode))\n \t\t\t\tdirmask |= 1ul << i;\n+\t\t\te = &entry[i];\n \t\t}\n \t\tif (!mask)\n \t\t\tbreak;\n-\t\tret = info->fn(n, mask, dirmask, entry, info);\n-\t\tif (ret < 0) {\n-\t\t\terror = ret;\n-\t\t\tif (!info->show_all_errors)\n-\t\t\t\tbreak;\n+\t\tinteresting = prune_traversal(e, info, &base, interesting);\n+\t\tif (interesting < 0)\n+\t\t\tbreak;\n+\t\tif (interesting) {\n+\t\t\tret = info->fn(n, mask, dirmask, entry, info);\n+\t\t\tif (ret < 0) {\n+\t\t\t\terror = ret;\n+\t\t\t\tif (!info->show_all_errors)\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tmask &= ret;\n \t\t}\n-\t\tmask &= ret;\n \t\tret = 0;\n \t\tfor (i = 0; i < n; i++)\n \t\t\tif (mask & (1ul << i))\n@@ -395,6 +421,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \tfor (i = 0; i < n; i++)\n \t\tfree_extended_entry(tx + i);\n \tfree(tx);\n+\tstrbuf_release(&base);\n \treturn error;\n }\n \ndiff --git a/tree-walk.h b/tree-walk.h\nindex 39524b7..0089581 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -44,6 +44,7 @@ struct traverse_info {\n \tstruct traverse_info *prev;\n \tstruct name_entry name;\n \tint pathlen;\n+\tstruct pathspec *pathspec;\n \n \tunsigned long conflicts;\n \ttraverse_callback_t fn;\n-- \n1.7.7.rc0.70.g82660\n"},{"id":"174506","messageId":"1314653603-7533-3-git-send-email-gitster@pobox.com","threadId":"28198","inReplyTo":"1314653603-7533-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] unpack-trees: allow pruning with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T21:33:22Z","receivedAt":"2011-08-29T21:33:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Use the pathspec pruning of traverse_trees() from unpack_trees(). Again,\nthe unpack_trees() machinery is primarily meant for merging two (or more)\ntrees, and because a merge is a full tree operation, it didn't support any\npruning with pathspec, and this codepath probably should not be enabled\nwhile running a merge, but the caller in diff-lib.c::diff_cache() should\nbe able to take advanrage of it.\n\nSigned-off-by: Junio C Hamano \n---\n unpack-trees.c | 2 ++\n unpack-trees.h | 1 +\n 2 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cc616c3..670b464 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -444,6 +444,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \n \tnewinfo = *info;\n \tnewinfo.prev = info;\n+\tnewinfo.pathspec = info->pathspec;\n \tnewinfo.name = *p;\n \tnewinfo.pathlen += tree_entry_len(p->path, p->sha1) + 1;\n \tnewinfo.conflicts |= df_conflicts;\n@@ -1040,6 +1041,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tinfo.fn = unpack_callback;\n \t\tinfo.data = o;\n \t\tinfo.show_all_errors = o->show_all_errors;\n+\t\tinfo.pathspec = o->pathspec;\n \n \t\tif (o->prefix) {\n \t\t\t/*\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 7998948..5e432f5 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -52,6 +52,7 @@ struct unpack_trees_options {\n \tconst char *prefix;\n \tint cache_bottom;\n \tstruct dir_struct *dir;\n+\tstruct pathspec *pathspec;\n \tmerge_fn_t fn;\n \tconst char *msgs[NB_UNPACK_TREES_ERROR_TYPES];\n \t/*\n-- \n1.7.7.rc0.70.g82660\n"},{"id":"174507","messageId":"1314653603-7533-4-git-send-email-gitster@pobox.com","threadId":"28198","inReplyTo":"1314653603-7533-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] diff-index: pass pathspec down to unpack-trees machinery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T21:33:23Z","receivedAt":"2011-08-29T21:33:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"And finally, pass the pathspec down through unpack_trees() to traverse_trees()\ncallchain.\n\nBefore and after applying this series, looking for changes in the kernel\nrepository with a fairly narrow pathspec gets a moderate speeds up.\n\n (without patch)\n $ /usr/bin/time git diff --raw v2.6.27 -- net/ipv6 >/dev/null\n 0.48user 0.05system 0:00.53elapsed 100%CPU (0avgtext+0avgdata 163296maxresident)k\n 0inputs+952outputs (0major+11163minor)pagefaults 0swaps\n\n (with patch)\n $ /usr/bin/time git diff --raw v2.6.27 -- net/ipv6 >/dev/null\n 0.01user 0.00system 0:00.02elapsed 104%CPU (0avgtext+0avgdata 43856maxresident)k\n 0inputs+24outputs (0major+3688minor)pagefaults 0swaps\n\nSigned-off-by: Junio C Hamano \n---\n diff-lib.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f8454dd..ebe751e 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -468,6 +468,7 @@ static int diff_cache(struct rev_info *revs,\n \topts.unpack_data = revs;\n \topts.src_index = &the_index;\n \topts.dst_index = NULL;\n+\topts.pathspec = &revs->diffopt.pathspec;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \treturn unpack_trees(1, &t, &opts);\n-- \n1.7.7.rc0.70.g82660\n"},{"id":"174510","messageId":"CA+55aFzk+hn9wMAp3H92SHAGO8CQVBpTsmR41FgCy32T7muUzA@mail.gmail.com","threadId":"28198","inReplyTo":"1314653603-7533-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2011-08-29T21:56:02Z","receivedAt":"2011-08-29T21:56:02Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Aug 29, 2011 at 2:33 PM, Junio C Hamano wrote:\n>\n> Before and after applying this series, looking for changes in the kernel\n> repository with a fairly narrow pathspec gets a moderate speeds up.\n\n\"moderate speeds up\"?\n\nLooks like a big win to me. Admittedly it's already a pretty fast\noperation, but script it and repeat it a million times, and that will\nmatter a lot more.\n\nI guess the \"--raw\" diff part means that you are hiding the time to\nmake a real diff, which would otherwise swamp everything else. Even\nso, this looks like a good improvement.\n\n Linus\n"},{"id":"174512","messageId":"7v1uw36fgb.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"CA+55aFzk+hn9wMAp3H92SHAGO8CQVBpTsmR41FgCy32T7muUzA@mail.gmail.com","subject":"Re: [PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T22:05:56Z","receivedAt":"2011-08-29T22:05:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Mon, Aug 29, 2011 at 2:33 PM, Junio C Hamano wrote:\n>>\n>> Before and after applying this series, looking for changes in the kernel\n>> repository with a fairly narrow pathspec gets a moderate speeds up.\n>\n> \"moderate speeds up\"?\n>\n> Looks like a big win to me. Admittedly it's already a pretty fast\n> operation, but script it and repeat it a million times, and that will\n> matter a lot more.\n>\n> I guess the \"--raw\" diff part means that you are hiding the time to\n> make a real diff, which would otherwise swamp everything else. Even\n> so, this looks like a good improvement.\n\nThe topic started by Marat Radchenko in\n\n http://thread.gmane.org/gmane.comp.version-control.git/179926\n\nwho was trying to pick a single path (we do not know how deep it is), and\nwas comparing between these two:\n\n $ time git show branch:file | diff -u - file > /dev/null \n real 0m0.003s\n user 0m0.000s\n sys 0m0.000s\n $ time git diff branch -- file > /dev/null \n real 0m31.442s\n user 0m31.040s\n sys 0m0.380s\n\nReplacing \"diff -u\" with \"git diff --no-index\" in the former didn't make\nmuch of a difference. It turned out that Marat had 603k paths in the index\nand the most of the time was spent in the unpack machinery.\n"},{"id":"174513","messageId":"CA+55aFz9givAWhP5_SfnneY6x_0ekcN2U9HrKr+NTgY=Ot=6TQ@mail.gmail.com","threadId":"28198","inReplyTo":"7v1uw36fgb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2011-08-29T22:11:34Z","receivedAt":"2011-08-29T22:11:34Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Aug 29, 2011 at 3:05 PM, Junio C Hamano wrote:\n>\n> The topic started by Marat Radchenko in\n\nUgh, that's ugly. Is it verified to solve Marat's nasty case too?\n\nBut yeah, ack on the series.\n\n Linus\n"},{"id":"174523","messageId":"7vfwkj4wff.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"CA+55aFz9givAWhP5_SfnneY6x_0ekcN2U9HrKr+NTgY=Ot=6TQ@mail.gmail.com","subject":"Re: [PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T23:42:12Z","receivedAt":"2011-08-29T23:42:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Mon, Aug 29, 2011 at 3:05 PM, Junio C Hamano wrote:\n>>\n>> The topic started by Marat Radchenko in\n>\n> Ugh, that's ugly. Is it verified to solve Marat's nasty case too?\n\nNot yet, though Marat is on the CC list of the original \"this is still\nrough\" draft (the code is identical but it is not split up into\nthree-patch series) message.\n\nMarat, if/when you have a chance could you try a patched git on your\noriginal use case and see if it produces correct output with shorter\namount of time?\n\n> But yeah, ack on the series.\n>\n> Linus\n\nThanks.\n"},{"id":"174536","messageId":"loom.20110830T081442-547@post.gmane.org","threadId":"28198","inReplyTo":"7vfwkj4wff.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Marat Radchenko","fromEmail":"marat@slonopotamus.org","sentAt":"2011-08-30T06:16:04Z","receivedAt":"2011-08-30T06:16:04Z","isPatch":true,"sender":{"key":"marat@slonopotamus.org","avatar":"https://avatars.githubusercontent.com/u/92637?v=4"},"body":"Junio C Hamano pobox.com> writes:\n> Marat, if/when you have a chance could you try a patched git on your\n> original use case and see if it produces correct output with shorter\n> amount of time?\n\n30s without patch and 0.3s with it. You rock :)\n"},{"id":"174543","messageId":"4E5CB5C6.6020403@drmicha.warpmail.net","threadId":"28198","inReplyTo":"1314653603-7533-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-08-30T10:04:54Z","receivedAt":"2011-08-30T10:04:54Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 29.08.2011 23:33:\n> \"git diff A B -- $pathspec\" to compare two tree-ishes knew how to apply\n> pathspec to avoid opening trees that fall outside the area of interest,\n> but \"git diff A -- $pathspec\" used unpack_trees() machinery that was meant\n> for full-tree merges, and ended up reading the whole tree only to discard\n> potentially major part of the work it does.\n\nSeems my analysis wasn't that far off (though, admittedly, unspecific):\n\nMG:\n> Well, we have to read the full tree before diffing. But I can't\n...\n> I meant: The way \"git diff\" is now, it does that.\n\nJC:\n> anything to do with it (iow, your \"The way 'git diff' is now, it does\n> that\" is not just incorrect---we don't read the full tree to begin\n> with---but irrelevant).\n\nAnyways, as MR writes:\n\nMR:\n> 30s without patch and 0.3s with it. You rock ;)\n\nI agree with all of that ;)\n\nMichael\n"},{"id":"174550","messageId":"CACsJy8CmZv7Fuhw+m6X2CVO4vWLYFQNmStu1jJmhVXSi4mPxJQ@mail.gmail.com","threadId":"28198","inReplyTo":"1314653603-7533-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-30T12:53:15Z","receivedAt":"2011-08-30T12:53:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 30, 2011 at 4:33 AM, Junio C Hamano wrote:\n> @@ -316,10 +328,18 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n\nNot related to the patch, but a bit of comment how this function works\nand how it expects info->fn() to return would be appreciated.\n\n\n>        struct name_entry *entry = xmalloc(n*sizeof(*entry));\n>        int i;\n>        struct tree_desc_x *tx = xcalloc(n, sizeof(*tx));\n> +       struct strbuf base = STRBUF_INIT;\n> +       int interesting = 1;\n\nI suspect making \"base\" an argument to traverse_trees() may save us a\nfew more alloc/free (or a lot, depends on how crowded the tree is).\nThere are only two users of traverse_trees(): merge-tree.c and\nunpack-trees.c, changes should be manageable.\n\n\n> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n>                        mask |= 1ul << i;\n>                        if (S_ISDIR(entry[i].mode))\n>                                dirmask |= 1ul << i;\n> +                       e = &entry[i];\n>                }\n\nWhy? \"e\" is not used in that loop or anywhere after that.\n\n\n>                if (!mask)\n>                        break;\n> -               ret = info->fn(n, mask, dirmask, entry, info);\n> -               if (ret < 0) {\n> -                       error = ret;\n> -                       if (!info->show_all_errors)\n> -                               break;\n> +               interesting = prune_traversal(e, info, &base, interesting);\n> +               if (interesting < 0)\n> +                       break;\n\nI don't really understand this function to comment. But I guess when\ninteresting < 0, we only skip info->fn() and assume it returns \"mask\"\n(its user unpack_callback() only returns either \"mask\" or -1). So the\ncode at the end of the main loop\n\n\t\tfor (i = 0; i < n; i++)\n\t\t\tif (mask & (1ul << i))\n\t\t\t\tupdate_extended_entry(tx + i, entry + i);\n\nshould not be skipped (by putting a break here). IOW, there should be\nno \"break;\" here.\n\n\n>                }\n> -               mask &= ret;\n>                ret = 0;\n\nWhile at there, remove \"ret = 0;\" too? I think this statement is useless.\n-- \nDuy\n"},{"id":"174552","messageId":"CACsJy8CBJEimG5am8+aWTGP7kzuZ79WfZ3KQKh7pyjKTx-mR9A@mail.gmail.com","threadId":"28198","inReplyTo":"1314653603-7533-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/3] unpack-trees: allow pruning with pathspec","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-30T13:03:35Z","receivedAt":"2011-08-30T13:03:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 30, 2011 at 4:33 AM, Junio C Hamano wrote:\n> and this codepath probably should not be enabled while running a merge\n\nBut do you think if it works with merge (ie. a partial merge)?\n\nThere's probably no interest in partial merge now, but if narrow clone\ncomes, index will be also narrowed and at least fast-forward merge\nshould work. I think it may work, but I'm not sure. unpack_callback()\nand traverse_trees() look like magic to me. There may be some\ninteractions between index and unpacked trees that make partial merge\nfail in a subtle way.\n-- \nDuy\n"},{"id":"174554","messageId":"CAFfmPPO2xT7bc0EjQRDCxG3hNTBLYYEYe61kipcqtOiNbL8=hA@mail.gmail.com","threadId":"28198","inReplyTo":"1314653603-7533-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/3] unpack-trees: allow pruning with pathspec","fromName":"David Michael Barr","fromEmail":"davidbarr@google.com","sentAt":"2011-08-30T15:24:06Z","receivedAt":"2011-08-30T15:24:06Z","isPatch":true,"sender":{"key":"davidbarr@google.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"On Tue, Aug 30, 2011 at 7:33 AM, Junio C Hamano wrote:\n> while running a merge, but the caller in diff-lib.c::diff_cache() should\n> be able to take advanrage of it.\n\ns/advanrage/advantage/ :)\n\nNice little hot-fix too. I love the understatement, \"moderate speed up.\"\n\n--\nDavid Barr\n"},{"id":"174560","messageId":"7vsjoi3k82.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"4E5CB5C6.6020403@drmicha.warpmail.net","subject":"Re: [PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-30T17:03:25Z","receivedAt":"2011-08-30T17:03:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Seems my analysis wasn't that far off (though, admittedly, unspecific):\n\nYes, the \"diff-index\" implementation was updated over time a few times and\nI was confused. Sorry.\n\nThanks for helping diagnosing the issue.\n"},{"id":"174564","messageId":"7v7h5u3ivy.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"CACsJy8CBJEimG5am8+aWTGP7kzuZ79WfZ3KQKh7pyjKTx-mR9A@mail.gmail.com","subject":"Re: [PATCH 2/3] unpack-trees: allow pruning with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-30T17:32:17Z","receivedAt":"2011-08-30T17:32:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Tue, Aug 30, 2011 at 4:33 AM, Junio C Hamano wrote:\n>> and this codepath probably should not be enabled while running a merge\n>\n> But do you think if it works with merge (ie. a partial merge)?\n\nI highly doubt it.\n\nThe primary reason why I doubt it would work is actually the same as the\nvery reason why a merge would work with unpack_trees() machinery. It walks\nall the entries in the trees involved, picking matching entries that would\ngo in the result, while also picking the corresponding entry from the index,\nand compute their merge and depositing the result in the final index. If\nyou \"optimize\" it by not walking some parts of trees, you don't just omit\nmerging damages made to these parts by other trees, but you also omit\nrecording the contributions by the current HEAD and the index. You would\nneed to compensate for that by doing something that copies the contents\nof the index for paths outside the area covered by pathspec.\n\nAnd whole point of this series is not doing that. We don't want to say\n\"because they are outside pathspec, we pretend that these paths in index\ndo not have corresponding entries in the trees we are merging into\" and\nend up producing creation filepairs for them.\n\nBut I honestly am not interested giving it unnecessary deep thought at\nthis point in the cycle. I haven't found a need for a low-level partial\nmerge machinery implementation to support any higher level workflows in\ngit, and the path-level 3-way merge machinery already exists in the form\nof ll_merge().\n"},{"id":"174567","messageId":"7v39gi3ib6.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"CACsJy8CmZv7Fuhw+m6X2CVO4vWLYFQNmStu1jJmhVXSi4mPxJQ@mail.gmail.com","subject":"Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-30T17:44:45Z","receivedAt":"2011-08-30T17:44:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n>> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n>> mask |= 1ul << i;\n>> if (S_ISDIR(entry[i].mode))\n>> dirmask |= 1ul << i;\n>> + e = &entry[i];\n>> }\n>\n> Why? \"e\" is not used in that loop or anywhere after that.\n\nThis is trying to find _a_ surviving entry to be fed to prune_traversal()\nwhich in turn uses tree_entry_interesting(). At this point in the code, we\nare stuffing the entries of the same name from the input trees (and if one\ntree is missing an entry of the chosen name, it will have NULL there), so\nany non-empty entry would do. It corresponds to \"first\" but that is just a\nsimple string and not a name_entry tree_entry_interesting() wants.\n\n\n>> if (!mask)\n>> break;\n>> - ret = info->fn(n, mask, dirmask, entry, info);\n>> - if (ret < 0) {\n>> - error = ret;\n>> - if (!info->show_all_errors)\n>> - break;\n>> + interesting = prune_traversal(e, info, &base, interesting);\n>> + if (interesting < 0)\n>> + break;\n>\n> I don't really understand this function to comment. But I guess when\n> interesting < 0, we only skip info->fn() and assume it returns \"mask\"\n> (its user unpack_callback() only returns either \"mask\" or -1).\n\nWe consume the entries we have used in merging (which is actually\n\"everything in entry[] array\" as info->fn() returns \"mask\" itself) by\nsaying \"update_extended_entry()\" and the purpose of doing so is to prepare\nto process the next entry of the tree we are traversing.\n\nWhen tree_entry_interesting() returns negative, it tells us \"no, and no\nsubsequent entries will be either\", meaning \"we are done with this tree\".\nAs we are done, there is nothing to prepare for the next round; we are not\nwalking the remaining entries in the trees we are looking at. Is there any\npoint in calling update_extended_entry() I am missing?\n"},{"id":"174589","messageId":"7vr542zb5d.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"loom.20110830T081442-547@post.gmane.org","subject":"Re: [PATCH 0/3] Un-pessimize \"diff-index $commit -- $pathspec\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-31T00:18:22Z","receivedAt":"2011-08-31T00:18:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marat Radchenko writes:\n\n> Junio C Hamano pobox.com> writes:\n>> Marat, if/when you have a chance could you try a patched git on your\n>> original use case and see if it produces correct output with shorter\n>> amount of time?\n>\n> 30s without patch and 0.3s with it.\n\nThanks.\n"},{"id":"174591","messageId":"CACsJy8BsnKQx2Z1bs3Sdx-1ROeFvCZQkwQT8bWnyzL9du-T3Fw@mail.gmail.com","threadId":"28198","inReplyTo":"7v39gi3ib6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-08-31T01:35:02Z","receivedAt":"2011-08-31T01:35:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 31, 2011 at 12:44 AM, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>\n>>> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n>>>                        mask |= 1ul << i;\n>>>                        if (S_ISDIR(entry[i].mode))\n>>>                                dirmask |= 1ul << i;\n>>> +                       e = &entry[i];\n>>>                }\n>>\n>> Why? \"e\" is not used in that loop or anywhere after that.\n>\n> This is trying to find _a_ surviving entry to be fed to prune_traversal()\n> which in turn uses tree_entry_interesting(). At this point in the code, we\n> are stuffing the entries of the same name from the input trees (and if one\n> tree is missing an entry of the chosen name, it will have NULL there), so\n> any non-empty entry would do. It corresponds to \"first\" but that is just a\n> simple string and not a name_entry tree_entry_interesting() wants.\n\nAh yes. I only searched in old code, \"e\" is used in the new\nprune_traversal() call.\n\n>>>                if (!mask)\n>>>                        break;\n>>> -               ret = info->fn(n, mask, dirmask, entry, info);\n>>> -               if (ret < 0) {\n>>> -                       error = ret;\n>>> -                       if (!info->show_all_errors)\n>>> -                               break;\n>>> +               interesting = prune_traversal(e, info, &base, interesting);\n>>> +               if (interesting < 0)\n>>> +                       break;\n>>\n>> I don't really understand this function to comment. But I guess when\n>> interesting < 0, we only skip info->fn() and assume it returns \"mask\"\n>> (its user unpack_callback() only returns either \"mask\" or -1).\n>\n> We consume the entries we have used in merging (which is actually\n> \"everything in entry[] array\" as info->fn() returns \"mask\" itself) by\n> saying \"update_extended_entry()\" and the purpose of doing so is to prepare\n> to process the next entry of the tree we are traversing.\n>\n> When tree_entry_interesting() returns negative, it tells us \"no, and no\n> subsequent entries will be either\", meaning \"we are done with this tree\".\n> As we are done, there is nothing to prepare for the next round; we are not\n> walking the remaining entries in the trees we are looking at. Is there any\n> point in calling update_extended_entry() I am missing?\n\nNo you're right again. Somehow I thought there would be another round\nin the loop (ie. continue, not break). My bad.\n-- \nDuy\n"},{"id":"177276","messageId":"4E91C031.9030205@alum.mit.edu","threadId":"28198","inReplyTo":"1314653603-7533-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-09T15:39:29Z","receivedAt":"2011-10-09T15:39:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/29/2011 11:33 PM, Junio C Hamano wrote:\n> diff --git a/tree-walk.c b/tree-walk.c\n> index 33f749e..808bb55 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> [...]\n> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n> \t\t\tmask |= 1ul << i;\n> \t\t\tif (S_ISDIR(entry[i].mode))\n> \t\t\t\tdirmask |= 1ul << i;\n> +\t\t\te = &entry[i];\n> \t\t}\n> \t\tif (!mask)\n> \t\t\tbreak;\n> -\t\tret = info->fn(n, mask, dirmask, entry, info);\n> -\t\tif (ret < 0) {\n> -\t\t\terror = ret;\n> -\t\t\tif (!info->show_all_errors)\n> -\t\t\t\tbreak;\n> +\t\tinteresting = prune_traversal(e, info, &base, interesting);\n\nAccording to gcc 4.2.4 (though, strangely, not gcc 4.4.3):\n\ntree-walk.c: In function ‘traverse_trees’:\ntree-walk.c:347: warning: ‘e’ may be used uninitialized in this function\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177285","messageId":"CACsJy8D9sJOtXj_jkVSyoAJ9TC4wmKNAD5YwsFXTYkpvM4e13w@mail.gmail.com","threadId":"28198","inReplyTo":"4E91C031.9030205@alum.mit.edu","subject":"Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-09T21:35:34Z","receivedAt":"2011-10-09T21:35:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 10, 2011 at 2:39 AM, Michael Haggerty wrote:\n> On 08/29/2011 11:33 PM, Junio C Hamano wrote:\n>> diff --git a/tree-walk.c b/tree-walk.c\n>> index 33f749e..808bb55 100644\n>> --- a/tree-walk.c\n>> +++ b/tree-walk.c\n>> [...]\n>> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n>>                       mask |= 1ul << i;\n>>                       if (S_ISDIR(entry[i].mode))\n>>                               dirmask |= 1ul << i;\n>> +                     e = &entry[i];\n>>               }\n>>               if (!mask)\n>>                       break;\n>> -             ret = info->fn(n, mask, dirmask, entry, info);\n>> -             if (ret < 0) {\n>> -                     error = ret;\n>> -                     if (!info->show_all_errors)\n>> -                             break;\n>> +             interesting = prune_traversal(e, info, &base, interesting);\n>\n> According to gcc 4.2.4 (though, strangely, not gcc 4.4.3):\n>\n> tree-walk.c: In function ‘traverse_trees’:\n> tree-walk.c:347: warning: ‘e’ may be used uninitialized in this function\n\nFalse alarm. If e is not initialized in the for loop, mask would be\nzero and therefore prune_traversal(e, info, &base, interesting), which\nwould use uninitialized \"e\", would never be called.\n-- \nDuy\n"},{"id":"177293","messageId":"4E927799.6000604@alum.mit.edu","threadId":"28198","inReplyTo":"CACsJy8D9sJOtXj_jkVSyoAJ9TC4wmKNAD5YwsFXTYkpvM4e13w@mail.gmail.com","subject":"Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T04:42:01Z","receivedAt":"2011-10-10T04:42:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/09/2011 11:35 PM, Nguyen Thai Ngoc Duy wrote:\n> On Mon, Oct 10, 2011 at 2:39 AM, Michael Haggerty wrote:\n>> On 08/29/2011 11:33 PM, Junio C Hamano wrote:\n>>> diff --git a/tree-walk.c b/tree-walk.c\n>>> index 33f749e..808bb55 100644\n>>> --- a/tree-walk.c\n>>> +++ b/tree-walk.c\n>>> [...]\n>>> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n>>> mask |= 1ul << i;\n>>> if (S_ISDIR(entry[i].mode))\n>>> dirmask |= 1ul << i;\n>>> + e = &entry[i];\n>>> }\n>>> if (!mask)\n>>> break;\n>>> - ret = info->fn(n, mask, dirmask, entry, info);\n>>> - if (ret < 0) {\n>>> - error = ret;\n>>> - if (!info->show_all_errors)\n>>> - break;\n>>> + interesting = prune_traversal(e, info, &base, interesting);\n>>\n>> According to gcc 4.2.4 (though, strangely, not gcc 4.4.3):\n\nI checked this a bit more carefully. gcc 4.2.4 emits a warning when the\n-O1 or -O2 optimization levels are used, but not with -O0. gcc 4.4.3\ndoes not emit a warning regardless of optimization level.\n\n>> tree-walk.c: In function ‘traverse_trees’:\n>> tree-walk.c:347: warning: ‘e’ may be used uninitialized in this function\n> \n> False alarm. If e is not initialized in the for loop, mask would be\n> zero and therefore prune_traversal(e, info, &base, interesting), which\n> would use uninitialized \"e\", would never be called.\n\nThat's good to know. Still, it might be worthwhile to initialize the\nvariable explicitly to avoid future confusion.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"182321","messageId":"20120111063104.GA3153@burratino","threadId":"28198","inReplyTo":"1314653603-7533-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/3] diff-index: pass pathspec down to unpack-trees machinery","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-11T06:31:04Z","receivedAt":"2012-01-11T06:31:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> And finally, pass the pathspec down through unpack_trees() to traverse_trees()\n> callchain.\n>\n> Before and after applying this series, looking for changes in the kernel\n> repository with a fairly narrow pathspec gets a moderate speeds up.\n\nCheck this out:\n\n\tmkdir -p test-repo/subdir\n\tcd test-repo\n\tgit init\n\techo hi >subdir/hello.h\n\tgit add subdir/hello.h\n\tgit commit -m 'say hi'\n\tgit diff-index --abbrev HEAD -- '*.h'\n\nIn git versions which include the patch described above, the unchanged\nsubdir/hello.h shows up as a newly added file. Reverting that patch\n(v1.7.7.1~22^2, diff-index: pass pathspec down to unpack-trees\nmachinery, 2011-08-29) makes \"git diff HEAD\" with wildcards work\nagain.\n\nIt looks like the pruning of the preimage by pathspec is too\naggressive and is omiting whole directories that do not match the\npathspec without realizing that the path to a file contained within\nmight match.\n\nWe could just add a test for this to the testsuite and do that revert,\nbut I'd rather not yet, in case this is a symptom of some deeper\nunpack_trees() confusion.\n\nHints?\n\nThanks,\nJonathan\n\n> diff-lib.c | 1 +\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index f8454dd..ebe751e 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -468,6 +468,7 @@ static int diff_cache(struct rev_info *revs,\n> \topts.unpack_data = revs;\n> \topts.src_index = &the_index;\n> \topts.dst_index = NULL;\n> +\topts.pathspec = &revs->diffopt.pathspec;\n> \n> \tinit_tree_desc(&t, tree->buffer, tree->size);\n> \treturn unpack_trees(1, &t, &opts);\n> -- \n> 1.7.7.rc0.70.g82660\n>\n>\n"},{"id":"182325","messageId":"7vvcoi7jgn.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"20120111063104.GA3153@burratino","subject":"Re: [PATCH 3/3] diff-index: pass pathspec down to unpack-trees machinery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-11T08:05:12Z","receivedAt":"2012-01-11T08:05:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>> And finally, pass the pathspec down through unpack_trees() to traverse_trees()\n>> callchain.\n>\n> In git versions which include the patch described above, the unchanged\n> subdir/hello.h shows up as a newly added file. Reverting that patch\n> (v1.7.7.1~22^2, diff-index: pass pathspec down to unpack-trees\n> machinery, 2011-08-29) makes \"git diff HEAD\" with wildcards work\n> again.\n\nI suspect that the particular change on the side branch predates Nguyen's\neffort to unify the pathspec semantics to teach the wildcard (i.e. not the\ntraditional \"prefix match\") to the tree traversal code, but it is fairly\nlate here, so I didn't check.\n\nI think the right fix is to update the logic that still assumes that a\npathspec used for tree traversal is always prefix match when leaving the\ntraversal early, and instead use the proper matching logic that knows that\na wildcard pathspec needs to dig deeper into the tree regardless (I think\nthe pathspec implementation used in \"git grep\" got this right, but please\ndouble check), so that we do not dig unnecessary subtrees when pathspecs\nare all prefixes, but still keep digging when there is a wildcard match.\n\nI suspect that it would lead to a fairly complete unification of the three\nimplementations of pathspec matching logic and would allow us to also do\nsomething like \"git log -- '*.h'\" for free.\n"},{"id":"182344","messageId":"CACsJy8D7EnOebAxBYF8ua7htu-81nKY=ghUMgg=JOe4Fc1uigQ@mail.gmail.com","threadId":"28198","inReplyTo":"20120111063104.GA3153@burratino","subject":"Re: [PATCH 3/3] diff-index: pass pathspec down to unpack-trees machinery","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-11T12:33:52Z","receivedAt":"2012-01-11T12:33:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 11, 2012 at 1:31 PM, Jonathan Nieder wrote:\n> Check this out:\n>\n>        mkdir -p test-repo/subdir\n>        cd test-repo\n>        git init\n>        echo hi >subdir/hello.h\n>        git add subdir/hello.h\n>        git commit -m 'say hi'\n>        git diff-index --abbrev HEAD -- '*.h'\n\nThis seems to fix this.\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7c9ecf6..5cf58b6 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1042,6 +1042,7 @@ int unpack_trees(unsigned len, struct tree_desc\n*t, struct unpack_trees_options\n info.data = o;\n info.show_all_errors = o->show_all_errors;\n info.pathspec = o->pathspec;\n+ info.pathspec->recursive = 1;\n\n if (o->prefix) {\n /*\n\nStill scratching my head why this flag is zero by default, which would\naffect all other places.. Or perhaps the right fix would be this\ninstead\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex f82dba6..0345938 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -634,7 +634,7 @@ enum interesting tree_entry_interesting(const\nstruct name_entry *entry,\n * Match all directories. We'll try to\n * match files later on.\n */\n- if (ps->recursive && S_ISDIR(entry->mode))\n+ if (S_ISDIR(entry->mode))\n return entry_interesting;\n }\n\n@@ -662,7 +662,7 @@ match_wildcards:\n * Match all directories. We'll try to match files\n * later on.\n */\n- if (ps->recursive && S_ISDIR(entry->mode))\n+ if (S_ISDIR(entry->mode))\n return entry_interesting;\n }\n return never_interesting; /* No matches */\n-- \nDuy\n"},{"id":"182347","messageId":"CACsJy8DvVoS9Uz94DhkC1+wTOyLRpewp5=0fU4JKqSSQh6VS-A@mail.gmail.com","threadId":"28198","inReplyTo":"CACsJy8D7EnOebAxBYF8ua7htu-81nKY=ghUMgg=JOe4Fc1uigQ@mail.gmail.com","subject":"Re: [PATCH 3/3] diff-index: pass pathspec down to unpack-trees machinery","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-11T12:47:38Z","receivedAt":"2012-01-11T12:47:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 11, 2012 at 7:33 PM, Nguyen Thai Ngoc Duy wrote:\n> Still scratching my head why this flag is zero by default, which would\n> affect all other places.. Or perhaps the right fix would be this\n> instead\n\nYep, it seems the right one. recursive flag is introduced to\nenable/disable max_depth feature, which is only used by grep, so we\nwant this feature to be off by default. max_depth == 0 has special\nmeaning, and we do not want another any other kind of initialization\nbut memset(), so \"recursive\" functions as the feature switch.\n\nThe code portions below deal with the case where we have tried and\nfailed prefix matching. Because this is wildcard, we don't know if\nanything in the given directory may match or not, so we match all\ndirectories, then filter unmatched files out at the end. This has\nnothing to do with \"recursive\" flag as the max_depth switch.\n\nWill think about it and test some more tomorrow when my mind is in better state.\n\n> diff --git a/tree-walk.c b/tree-walk.c\n> index f82dba6..0345938 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -634,7 +634,7 @@ enum interesting tree_entry_interesting(const\n> struct name_entry *entry,\n>                                 * Match all directories. We'll try to\n>                                 * match files later on.\n>                                 */\n> -                               if (ps->recursive && S_ISDIR(entry->mode))\n> +                               if (S_ISDIR(entry->mode))\n>                                        return entry_interesting;\n>                        }\n>\n> @@ -662,7 +662,7 @@ match_wildcards:\n>                 * Match all directories. We'll try to match files\n>                 * later on.\n>                 */\n> -               if (ps->recursive && S_ISDIR(entry->mode))\n> +               if (S_ISDIR(entry->mode))\n>                        return entry_interesting;\n>        }\n>        return never_interesting; /* No matches */\n-- \nDuy\n"},{"id":"182380","messageId":"7vvcoi55xj.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"CACsJy8D7EnOebAxBYF8ua7htu-81nKY=ghUMgg=JOe4Fc1uigQ@mail.gmail.com","subject":"Re: [PATCH 3/3] diff-index: pass pathspec down to unpack-trees machinery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-11T20:40:24Z","receivedAt":"2012-01-11T20:40:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> This seems to fix this.\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 7c9ecf6..5cf58b6 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1042,6 +1042,7 @@ int unpack_trees(unsigned len, struct tree_desc\n> *t, struct unpack_trees_options\n> info.data = o;\n> info.show_all_errors = o->show_all_errors;\n> info.pathspec = o->pathspec;\n> + info.pathspec->recursive = 1;\n>\n> if (o->prefix) {\n> /*\n>\n> Still scratching my head why this flag is zero by default, which would\n> affect all other places.\n\nAhh, thanks for diagnosing.\n\n> ... Or perhaps the right fix would be this\n> instead\n>\n> diff --git a/tree-walk.c b/tree-walk.c\n> index f82dba6..0345938 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -634,7 +634,7 @@ enum interesting tree_entry_interesting(const\n> struct name_entry *entry,\n> * Match all directories. We'll try to\n> * match files later on.\n> */\n> - if (ps->recursive && S_ISDIR(entry->mode))\n> + if (S_ISDIR(entry->mode))\n> return entry_interesting;\n> }\n>\n> @@ -662,7 +662,7 @@ match_wildcards:\n> * Match all directories. We'll try to match files\n> * later on.\n> */\n> - if (ps->recursive && S_ISDIR(entry->mode))\n> + if (S_ISDIR(entry->mode))\n> return entry_interesting;\n> }\n> return never_interesting; /* No matches */\n\nDoesn't that break \"git diff-tree A B\" without the \"-r\" option?\n"},{"id":"182414","messageId":"1326341371-16628-1-git-send-email-pclouds@gmail.com","threadId":"28198","inReplyTo":"20120111063104.GA3153@burratino","subject":"[PATCH] tree_entry_interesting: make recursive mode default","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-12T04:09:31Z","receivedAt":"2012-01-12T04:09:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There is a bit of history behind this. Some time ago, t_e_i() only\nsupported prefix matching. diff-tree supported recursive and\nnon-recursive mode but it did not make any difference to prefix\nmatching.\n\nLater on, t_e_i() gained limited recursion support to unify a similar\nmatching code used by git-grep. It introduced two new fields in struct\npathspec: max_depth and recursive. \"recursive\" field functions as a\nfeature switch so that this feature is off by default.\n\nSome time after that, t_e_i() further gained wildcard support. With\nwildcard matching, recursive and non-recursive diff-tree\nmattered. \"recursive\" field was reused to distinguish recursion in\ndiff-tree.\n\nThis choice has a side effect that by default wildcard matching is in\nnon-recursive mode, which is not true. All current call sites except\n\"diff-tree without -r\" (grep, traverse_commit_list, tree-walk and\ngeneral tree diff) prefer recursive mode.\n\nThis patch decouples the use of recursive field. The max_depth feature\nswitch is now controlled by max_depth_valid field. diff-tree recursion\nis controlled by nonrecursive_diff_tree, which makes it recursive by\ndefault.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Junio, log/diff family takes wildcards just fine (even before this\n patch, just less reliable). If it's not mentioned in release notes\n before, perhaps it's worth a line in 1.7.9 annoucement.\n\n builtin/grep.c | 2 +-\n cache.h | 3 ++-\n dir.c | 4 ++--\n t/t4010-diff-pathspec.sh | 8 ++++++++\n tree-diff.c | 3 +--\n tree-walk.c | 8 ++++----\n 6 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9ce064a..c081bf4 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1051,7 +1051,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tpaths = get_pathspec(prefix, argv + i);\n \tinit_pathspec(&pathspec, paths);\n \tpathspec.max_depth = opt.max_depth;\n-\tpathspec.recursive = 1;\n+\tpathspec.max_depth_valid = 1;\n \n \tif (show_in_pager && (cached || list.nr))\n \t\tdie(_(\"--open-files-in-pager only works on the worktree\"));\ndiff --git a/cache.h b/cache.h\nindex 10afd71..f58e05a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -526,7 +526,8 @@ struct pathspec {\n \tconst char **raw; /* get_pathspec() result, not freed by free_pathspec() */\n \tint nr;\n \tunsigned int has_wildcard:1;\n-\tunsigned int recursive:1;\n+\tunsigned int max_depth_valid:1;\n+\tunsigned int nonrecursive_diff_tree:1;\n \tint max_depth;\n \tstruct pathspec_item {\n \t\tconst char *match;\ndiff --git a/dir.c b/dir.c\nindex 0a78d00..5af3567 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -258,7 +258,7 @@ int match_pathspec_depth(const struct pathspec *ps,\n \tint i, retval = 0;\n \n \tif (!ps->nr) {\n-\t\tif (!ps->recursive || ps->max_depth == -1)\n+\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n \t\t\treturn MATCHED_RECURSIVELY;\n \n \t\tif (within_depth(name, namelen, 0, ps->max_depth))\n@@ -275,7 +275,7 @@ int match_pathspec_depth(const struct pathspec *ps,\n \t\tif (seen && seen[i] == MATCHED_EXACTLY)\n \t\t\tcontinue;\n \t\thow = match_pathspec_item(ps->items+i, prefix, name, namelen);\n-\t\tif (ps->recursive && ps->max_depth != -1 &&\n+\t\tif (ps->max_depth_valid && ps->max_depth != -1 &&\n \t\t how && how != MATCHED_FNMATCH) {\n \t\t\tint len = ps->items[i].len;\n \t\t\tif (name[len] == '/')\ndiff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh\nindex fbc8cd8..af5134b 100755\n--- a/t/t4010-diff-pathspec.sh\n+++ b/t/t4010-diff-pathspec.sh\n@@ -48,6 +48,14 @@ test_expect_success \\\n compare_diff_raw current expected'\n \n cat >expected <<\\EOF\n+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M\tpath1/file1\n+EOF\n+test_expect_success \\\n+ '\"*file1\" should show path1/file1' \\\n+ 'git diff-index --cached $tree -- \"*file1\" >current &&\n+ compare_diff_raw current expected'\n+\n+cat >expected <<\\EOF\n :100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M\tfile0\n EOF\n test_expect_success \\\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 28ad6db..164693f 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -137,8 +137,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \tenum interesting t2_match = entry_not_interesting;\n \n \t/* Enable recursion indefinitely */\n-\topt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);\n-\topt->pathspec.max_depth = -1;\n+\topt->pathspec.nonrecursive_diff_tree = !DIFF_OPT_TST(opt, RECURSIVE);\n \n \tstrbuf_init(&base, PATH_MAX);\n \tstrbuf_add(&base, base_str, baselen);\ndiff --git a/tree-walk.c b/tree-walk.c\nindex f82dba6..04f1b81 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -588,7 +588,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n \t\tentry_not_interesting : all_entries_not_interesting;\n \n \tif (!ps->nr) {\n-\t\tif (!ps->recursive || ps->max_depth == -1)\n+\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n \t\t\treturn all_entries_interesting;\n \t\treturn within_depth(base->buf + base_offset, baselen,\n \t\t\t\t !!S_ISDIR(entry->mode),\n@@ -609,7 +609,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n \t\t\tif (!match_dir_prefix(base_str, match, matchlen))\n \t\t\t\tgoto match_wildcards;\n \n-\t\t\tif (!ps->recursive || ps->max_depth == -1)\n+\t\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n \t\t\t\treturn all_entries_interesting;\n \n \t\t\treturn within_depth(base_str + matchlen + 1,\n@@ -634,7 +634,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n \t\t\t\t * Match all directories. We'll try to\n \t\t\t\t * match files later on.\n \t\t\t\t */\n-\t\t\t\tif (ps->recursive && S_ISDIR(entry->mode))\n+\t\t\t\tif (!ps->nonrecursive_diff_tree && S_ISDIR(entry->mode))\n \t\t\t\t\treturn entry_interesting;\n \t\t\t}\n \n@@ -662,7 +662,7 @@ match_wildcards:\n \t\t * Match all directories. We'll try to match files\n \t\t * later on.\n \t\t */\n-\t\tif (ps->recursive && S_ISDIR(entry->mode))\n+\t\tif (!ps->nonrecursive_diff_tree && S_ISDIR(entry->mode))\n \t\t\treturn entry_interesting;\n \t}\n \treturn never_interesting; /* No matches */\n-- \n1.7.3.1.256.g2539c.dirty\n"},{"id":"182415","messageId":"7v1ur54ikk.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"1326341371-16628-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] tree_entry_interesting: make recursive mode default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-12T05:04:59Z","receivedAt":"2012-01-12T05:04:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> There is a bit of history behind this. Some time ago, t_e_i() only\n> supported prefix matching. diff-tree supported recursive and\n> non-recursive mode but it did not make any difference to prefix\n> matching.\n>\n> Later on, t_e_i() gained limited recursion support to unify a similar\n> matching code used by git-grep. It introduced two new fields in struct\n> pathspec: max_depth and recursive. \"recursive\" field functions as a\n> feature switch so that this feature is off by default.\n>\n> Some time after that, t_e_i() further gained wildcard support. With\n> wildcard matching, recursive and non-recursive diff-tree\n> mattered. \"recursive\" field was reused to distinguish recursion in\n> diff-tree.\n>\n> This choice has a side effect that by default wildcard matching is in\n> non-recursive mode, which is not true. All current call sites except\n> \"diff-tree without -r\" (grep, traverse_commit_list, tree-walk and\n> general tree diff) prefer recursive mode.\n>\n> This patch decouples the use of recursive field. The max_depth feature\n> switch is now controlled by max_depth_valid field. diff-tree recursion\n> is controlled by nonrecursive_diff_tree, which makes it recursive by\n> default.\n\nThanks, but I am curious about two (and a half) things.\n\n - The \"max_depth\" option has perfectly good and natural \"invalid\"\n sentinel values (i.e. 0 or negative). Why do we need a separate\n bitfield?\n\n - Special casing the non-recursive mode of diff-tree is perfectly\n acceptable, but nonrecursive_diff_tree does not sound like a very good\n name for it for two reasons. Perhaps there may be other users that want\n the \"surface only\" behaviour, so having \"diff_tree\" in the name limits\n its future (re)use. Also an option that is named negatively inevitably\n invites \"if (!opt->non_whatever)\" double negative. Can we come up with\n a better name, perhaps \"onelevel_only\" or something?\n\n - Shouldn't \"onelevel_only\" be the same as limiting to a single depth\n with \"max_depth\"?\n"},{"id":"182416","messageId":"CACsJy8BEGKctdeDsZAyDGfG_JQ96RpTpXFZvJs5M1vKH3w=_WA@mail.gmail.com","threadId":"28198","inReplyTo":"7v1ur54ikk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tree_entry_interesting: make recursive mode default","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-12T05:44:30Z","receivedAt":"2012-01-12T05:44:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/1/12 Junio C Hamano :\n> Thanks, but I am curious about two (and a half) things.\n>\n>  - The \"max_depth\" option has perfectly good and natural \"invalid\"\n>   sentinel values (i.e. 0 or negative). Why do we need a separate\n>   bitfield?\n\nWe want infinite recursion by default, by max_depth default value is\n0, which is non-recursive.\n\n>  - Special casing the non-recursive mode of diff-tree is perfectly\n>   acceptable, but nonrecursive_diff_tree does not sound like a very good\n>   name for it for two reasons. Perhaps there may be other users that want\n>   the \"surface only\" behaviour, so having \"diff_tree\" in the name limits\n>   its future (re)use. Also an option that is named negatively inevitably\n>   invites \"if (!opt->non_whatever)\" double negative. Can we come up with\n>   a better name, perhaps \"onelevel_only\" or something?\n\nI'm bad at naming, any other names are welcome. onelevel_only sounds good.\n\n>  - Shouldn't \"onelevel_only\" be the same as limiting to a single depth\n>   with \"max_depth\"?\n\nI thought of that but not sure it's equivalent to max_depth == 1 or\nmax_depth == 0, so I separated it for safety and clarity. max_depth\nfeature is driven by git-grep and there were a few interpretations how\nit should behave last time, so I'm not sure if its behavior may change\nin future.\n-- \nDuy\n"},{"id":"182540","messageId":"1326533003-19686-1-git-send-email-pclouds@gmail.com","threadId":"28198","inReplyTo":"1326341371-16628-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/2] Document limited recursion pathspec matching with wildcards","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-14T09:23:22Z","receivedAt":"2012-01-14T09:23:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"It's actually unlimited recursion if wildcards are active regardless\n--max-depth\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Regarding Junio's question earlier:\n\n > - Shouldn't \"onelevel_only\" be the same as limiting to a single depth\n > with \"max_depth\"?\n\n Doing that would change the behavior of \"git grep --max-depth=0 -- 'a*'\"\n from unlimited recursion currently to limited. We did not come to agree\n how --max-depth should behave with wildcards last time it was discussed,\n so it's best separating two flags (in the next patch) for now.\n\n Documentation/git-grep.txt | 3 +++\n tree-walk.c | 3 +++\n 2 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 15d6711..6a8b1e3 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -79,6 +79,9 @@ OPTIONS\n --max-depth ::\n \tFor each given on command line, descend at most \n \tlevels of directories. A negative value means no limit.\n+\tThis option is ignored if contains active wildcards.\n+\tIn other words if \"a*\" matches a directory named \"a*\",\n+\t\"*\" is matched literally so --max-depth is still effective.\n \n -w::\n --word-regexp::\ndiff --git a/tree-walk.c b/tree-walk.c\nindex f82dba6..492c7cd 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -661,6 +661,9 @@ match_wildcards:\n \t\t/*\n \t\t * Match all directories. We'll try to match files\n \t\t * later on.\n+\t\t * max_depth is ignored but we may consider support it\n+\t\t * in future, see\n+\t\t * http://thread.gmane.org/gmane.comp.version-control.git/163757/focus=163840\n \t\t */\n \t\tif (ps->recursive && S_ISDIR(entry->mode))\n \t\t\treturn entry_interesting;\n-- \n1.7.8.36.g69ee2\n"},{"id":"182541","messageId":"1326533003-19686-2-git-send-email-pclouds@gmail.com","threadId":"28198","inReplyTo":"1326533003-19686-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/2] tree_entry_interesting: make recursive mode default","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-14T09:23:23Z","receivedAt":"2012-01-14T09:23:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There is a bit of history behind this. Some time ago, t_e_i() only\nsupported prefix matching. diff-tree supported recursive and\nnon-recursive mode but it did not make any difference to prefix\nmatching.\n\nLater on, t_e_i() gained limited recursion support to unify a similar\nmatching code used by git-grep. It introduced two new fields in struct\npathspec: max_depth and recursive. \"recursive\" field functions as a\nfeature switch so that this feature is off by default.\n\nSome time after that, t_e_i() further gained wildcard support. With\nwildcard matching, recursive and non-recursive diff-tree\nmattered. \"recursive\" field was reused to distinguish recursion in\ndiff-tree.\n\nThis choice has a side effect that by default wildcard matching is in\nnon-recursive mode, which is not true. All current call sites except\n\"diff-tree without -r\" (grep, traverse_commit_list, tree-walk and\ngeneral tree diff) prefer recursive mode.\n\nThis patch decouples the use of recursive field. The max_depth feature\nswitch is now controlled by max_depth_valid field. diff-tree recursion\nis controlled by onelevel_only, which makes it recursive by default.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n The ugly name \"nonrecursive_diff_tree\" is changed to \"onelevel_only\".\n\n builtin/grep.c | 2 +-\n cache.h | 3 ++-\n dir.c | 4 ++--\n t/t4010-diff-pathspec.sh | 8 ++++++++\n tree-diff.c | 3 +--\n tree-walk.c | 8 ++++----\n 6 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9ce064a..c081bf4 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1051,7 +1051,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tpaths = get_pathspec(prefix, argv + i);\n \tinit_pathspec(&pathspec, paths);\n \tpathspec.max_depth = opt.max_depth;\n-\tpathspec.recursive = 1;\n+\tpathspec.max_depth_valid = 1;\n \n \tif (show_in_pager && (cached || list.nr))\n \t\tdie(_(\"--open-files-in-pager only works on the worktree\"));\ndiff --git a/cache.h b/cache.h\nindex 10afd71..0c4067d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -526,7 +526,8 @@ struct pathspec {\n \tconst char **raw; /* get_pathspec() result, not freed by free_pathspec() */\n \tint nr;\n \tunsigned int has_wildcard:1;\n-\tunsigned int recursive:1;\n+\tunsigned int max_depth_valid:1;\n+\tunsigned int onelevel_only:1;\n \tint max_depth;\n \tstruct pathspec_item {\n \t\tconst char *match;\ndiff --git a/dir.c b/dir.c\nindex 0a78d00..5af3567 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -258,7 +258,7 @@ int match_pathspec_depth(const struct pathspec *ps,\n \tint i, retval = 0;\n \n \tif (!ps->nr) {\n-\t\tif (!ps->recursive || ps->max_depth == -1)\n+\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n \t\t\treturn MATCHED_RECURSIVELY;\n \n \t\tif (within_depth(name, namelen, 0, ps->max_depth))\n@@ -275,7 +275,7 @@ int match_pathspec_depth(const struct pathspec *ps,\n \t\tif (seen && seen[i] == MATCHED_EXACTLY)\n \t\t\tcontinue;\n \t\thow = match_pathspec_item(ps->items+i, prefix, name, namelen);\n-\t\tif (ps->recursive && ps->max_depth != -1 &&\n+\t\tif (ps->max_depth_valid && ps->max_depth != -1 &&\n \t\t how && how != MATCHED_FNMATCH) {\n \t\t\tint len = ps->items[i].len;\n \t\t\tif (name[len] == '/')\ndiff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh\nindex fbc8cd8..af5134b 100755\n--- a/t/t4010-diff-pathspec.sh\n+++ b/t/t4010-diff-pathspec.sh\n@@ -48,6 +48,14 @@ test_expect_success \\\n compare_diff_raw current expected'\n \n cat >expected <<\\EOF\n+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M\tpath1/file1\n+EOF\n+test_expect_success \\\n+ '\"*file1\" should show path1/file1' \\\n+ 'git diff-index --cached $tree -- \"*file1\" >current &&\n+ compare_diff_raw current expected'\n+\n+cat >expected <<\\EOF\n :100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M\tfile0\n EOF\n test_expect_success \\\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 28ad6db..fbc683c 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -137,8 +137,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n \tenum interesting t2_match = entry_not_interesting;\n \n \t/* Enable recursion indefinitely */\n-\topt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);\n-\topt->pathspec.max_depth = -1;\n+\topt->pathspec.onelevel_only = !DIFF_OPT_TST(opt, RECURSIVE);\n \n \tstrbuf_init(&base, PATH_MAX);\n \tstrbuf_add(&base, base_str, baselen);\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 492c7cd..fdecacc 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -588,7 +588,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n \t\tentry_not_interesting : all_entries_not_interesting;\n \n \tif (!ps->nr) {\n-\t\tif (!ps->recursive || ps->max_depth == -1)\n+\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n \t\t\treturn all_entries_interesting;\n \t\treturn within_depth(base->buf + base_offset, baselen,\n \t\t\t\t !!S_ISDIR(entry->mode),\n@@ -609,7 +609,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n \t\t\tif (!match_dir_prefix(base_str, match, matchlen))\n \t\t\t\tgoto match_wildcards;\n \n-\t\t\tif (!ps->recursive || ps->max_depth == -1)\n+\t\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n \t\t\t\treturn all_entries_interesting;\n \n \t\t\treturn within_depth(base_str + matchlen + 1,\n@@ -634,7 +634,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n \t\t\t\t * Match all directories. We'll try to\n \t\t\t\t * match files later on.\n \t\t\t\t */\n-\t\t\t\tif (ps->recursive && S_ISDIR(entry->mode))\n+\t\t\t\tif (!ps->onelevel_only && S_ISDIR(entry->mode))\n \t\t\t\t\treturn entry_interesting;\n \t\t\t}\n \n@@ -665,7 +665,7 @@ match_wildcards:\n \t\t * in future, see\n \t\t * http://thread.gmane.org/gmane.comp.version-control.git/163757/focus=163840\n \t\t */\n-\t\tif (ps->recursive && S_ISDIR(entry->mode))\n+\t\tif (!ps->onelevel_only && S_ISDIR(entry->mode))\n \t\t\treturn entry_interesting;\n \t}\n \treturn never_interesting; /* No matches */\n-- \n1.7.8.36.g69ee2\n"},{"id":"182572","messageId":"7vfwfhwuzd.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"1326533003-19686-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 1/2] Document limited recursion pathspec matching with wildcards","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-15T02:38:30Z","receivedAt":"2012-01-15T02:38:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> It's actually unlimited recursion if wildcards are active regardless\n> --max-depth\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Regarding Junio's question earlier:\n>\n> > - Shouldn't \"onelevel_only\" be the same as limiting to a single depth\n> > with \"max_depth\"?\n>\n> Doing that would change the behavior of \"git grep --max-depth=0 -- 'a*'\"\n> from unlimited recursion currently to limited. We did not come to agree\n> how --max-depth should behave with wildcards last time it was discussed,\n> so it's best separating two flags (in the next patch) for now.\n\nOk, I 100% agree with the \"at least for now\" reasoning. Thanks for digging\ninto the archive.\n\n> Documentation/git-grep.txt | 3 +++\n> tree-walk.c | 3 +++\n> 2 files changed, 6 insertions(+), 0 deletions(-)\n>\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index 15d6711..6a8b1e3 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -79,6 +79,9 @@ OPTIONS\n> --max-depth ::\n> \tFor each given on command line, descend at most \n> \tlevels of directories. A negative value means no limit.\n> +\tThis option is ignored if contains active wildcards.\n> +\tIn other words if \"a*\" matches a directory named \"a*\",\n> +\t\"*\" is matched literally so --max-depth is still effective.\n\nDo we have a definition of \"active wildcard\"?\n\n> diff --git a/tree-walk.c b/tree-walk.c\n> index f82dba6..492c7cd 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -661,6 +661,9 @@ match_wildcards:\n> \t\t/*\n> \t\t * Match all directories. We'll try to match files\n> \t\t * later on.\n> +\t\t * max_depth is ignored but we may consider support it\n> +\t\t * in future, see\n> +\t\t * http://thread.gmane.org/gmane.comp.version-control.git/163757/focus=163840\n> \t\t */\n> \t\tif (ps->recursive && S_ISDIR(entry->mode))\n> \t\t\treturn entry_interesting;\n"},{"id":"182573","messageId":"7v8vl9wtfg.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"1326533003-19686-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/2] tree_entry_interesting: make recursive mode default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-15T03:12:03Z","receivedAt":"2012-01-15T03:12:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> This patch decouples the use of recursive field. The max_depth feature\n> switch is now controlled by max_depth_valid field. diff-tree recursion\n> is controlled by onelevel_only, which makes it recursive by default.\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 9ce064a..c081bf4 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1051,7 +1051,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tpaths = get_pathspec(prefix, argv + i);\n> \tinit_pathspec(&pathspec, paths);\n> \tpathspec.max_depth = opt.max_depth;\n> -\tpathspec.recursive = 1;\n> +\tpathspec.max_depth_valid = 1;\n\nWe initialize opt.max_depth to \"-1\" (unlimited) and then let it be\noverridden by the command line, and we set it to pathspec, so max_depth is\nalways valid, even when it is \"-1\", and is to be honoured.\n\n> diff --git a/dir.c b/dir.c\n> index 0a78d00..5af3567 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -258,7 +258,7 @@ int match_pathspec_depth(const struct pathspec *ps,\n> \tint i, retval = 0;\n> \n> \tif (!ps->nr) {\n> -\t\tif (!ps->recursive || ps->max_depth == -1)\n> +\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n> \t\t\treturn MATCHED_RECURSIVELY;\n> \t\tif (within_depth(name, namelen, 0, ps->max_depth))\n\nWhen there is no pathspec given, if we do not have a valid depth limiter,\nor a valid depth limiter says \"-1\" (unlimited), it is always a match.\nOtherwise we have check the depth. Looks correct.\n\n\n> @@ -275,7 +275,7 @@ int match_pathspec_depth(const struct pathspec *ps,\n> \t\tif (seen && seen[i] == MATCHED_EXACTLY)\n> \t\t\tcontinue;\n> \t\thow = match_pathspec_item(ps->items+i, prefix, name, namelen);\n> -\t\tif (ps->recursive && ps->max_depth != -1 &&\n> +\t\tif (ps->max_depth_valid && ps->max_depth != -1 &&\n> \t\t how && how != MATCHED_FNMATCH) {\n> \t\t\tint len = ps->items[i].len;\n> \t\t\tif (name[len] == '/')\n\nLikewise. When there is a max_depth defined from the caller, and we did\nnot get the desired match, we check if we can go deeper to retry the\nmatch. Looks correct.\n\nThese assume that tree-diff (the sole user of onelevel_only) never calls\ninto this function and ask to limit the recursion, though. Is it a good\nthing for the longer-term code health?\n\nIn any case, both of the above seem to work without max_depth_valid. If\nthe caller does not want to use depth limiting, it can set max_depth to\n\"-1\", and all the code after this patch that check ps->max_depth_valid can\npretend that max_depth_valid is set to 1, no?\n\n> diff --git a/tree-diff.c b/tree-diff.c\n> index 28ad6db..fbc683c 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -137,8 +137,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,\n> \tenum interesting t2_match = entry_not_interesting;\n> \n> \t/* Enable recursion indefinitely */\n> -\topt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);\n> -\topt->pathspec.max_depth = -1;\n> +\topt->pathspec.onelevel_only = !DIFF_OPT_TST(opt, RECURSIVE);\n\nThe comment \"Enable recursion indefinitely\" seems stale (not a problem\nwith this change).\n\n> diff --git a/tree-walk.c b/tree-walk.c\n> index 492c7cd..fdecacc 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -588,7 +588,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n> \t\tentry_not_interesting : all_entries_not_interesting;\n> \n> \tif (!ps->nr) {\n> -\t\tif (!ps->recursive || ps->max_depth == -1)\n> +\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n> \t\t\treturn all_entries_interesting;\n> \t\treturn within_depth(base->buf + base_offset, baselen,\n> \t\t\t\t !!S_ISDIR(entry->mode),\n\nWhen there is no pathspec given, if we do not have a valid depth limiter\n(i.e. caller is diff-tree), or a valid depth limiter says \"-1\" (unlimited), \neverything in this tree is interesting. If we have depth limit, we need to\ncheck it.\n\nBut how would onelevel_only option interact with this codepath? We used to\nhave recursive == false and max_depth == -1 in a non-recursive diff-tree,\nso the old code would have returned all_entries_interesting. Now we rely\non max_depth_valid being invalid. Again, wouldn't this work without\nmax_depth_valid if max_depth is set to \"-1\" in diff-tree?\n\n> @@ -609,7 +609,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n> \t\t\tif (!match_dir_prefix(base_str, match, matchlen))\n> \t\t\t\tgoto match_wildcards;\n> \n> -\t\t\tif (!ps->recursive || ps->max_depth == -1)\n> +\t\t\tif (!ps->max_depth_valid || ps->max_depth == -1)\n> \t\t\t\treturn all_entries_interesting;\n> \t\t\treturn within_depth(base_str + matchlen + 1,\n\nLikewise. Everything is interesting in a matched entry, when not\ndepth-limited. Otherwise we would need to check the depth. Looks correct.\n\nAgain, how would onelevel_only option interact with this part?\n\n> @@ -634,7 +634,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n> \t\t\t\t * Match all directories. We'll try to\n> \t\t\t\t * match files later on.\n> \t\t\t\t */\n> -\t\t\t\tif (ps->recursive && S_ISDIR(entry->mode))\n> +\t\t\t\tif (!ps->onelevel_only && S_ISDIR(entry->mode))\n> \t\t\t\t\treturn entry_interesting;\n> \t\t\t}\n> \n> @@ -665,7 +665,7 @@ match_wildcards:\n> \t\t * in future, see\n> \t\t * http://thread.gmane.org/gmane.comp.version-control.git/163757/focus=163840\n> \t\t */\n> -\t\tif (ps->recursive && S_ISDIR(entry->mode))\n> +\t\tif (!ps->onelevel_only && S_ISDIR(entry->mode))\n> \t\t\treturn entry_interesting;\n> \t}\n> \treturn never_interesting; /* No matches */\n\nBefore we had recursive and max_depth. Now you have three instead of two.\nThe only thing we are trying to say with these three is if we want to\nallow unlimited recursion, no recursion or recursion limited to a certain\ndepth. An integer option ought to work, and various codepaths that used to\nlook at the old two variables are converted to look at only just a few of\nthe new three variables, and never all three of them.\n\nThat makes my head hurt and makes me suspect there is something\nfundamentally wrong in the patch. Sigh...\n"},{"id":"182579","messageId":"CACsJy8BsautpfSARoGgRzCXANAoXP9e_786iBh0wWy=iyNXmew@mail.gmail.com","threadId":"28198","inReplyTo":"7vfwfhwuzd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] Document limited recursion pathspec matching with wildcards","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-15T09:48:51Z","receivedAt":"2012-01-15T09:48:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/1/15 Junio C Hamano :\n>> @@ -79,6 +79,9 @@ OPTIONS\n>>  --max-depth ::\n>>       For each given on command line, descend at most \n>>       levels of directories. A negative value means no limit.\n>> +     This option is ignored if contains active wildcards.\n>> +     In other words if \"a*\" matches a directory named \"a*\",\n>> +     \"*\" is matched literally so --max-depth is still effective.\n>\n> Do we have a definition of \"active wildcard\"?\n\nProbably not. I did not know how to phrase it and ended up with\n\"active wildcard\" and an example for clarification.\n-- \nDuy\n"},{"id":"182580","messageId":"20120115100327.GA10735@do","threadId":"28198","inReplyTo":"7v8vl9wtfg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] tree_entry_interesting: make recursive mode default","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-15T10:03:27Z","receivedAt":"2012-01-15T10:03:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 14, 2012 at 07:12:03PM -0800, Junio C Hamano wrote:\n> That makes my head hurt and makes me suspect there is something\n> fundamentally wrong in the patch. Sigh...\n\nI'll need to think about it. In the meantime perhaps the following\nbandage patch would suffice, rather than revert 2f88c19 (diff-index:\npass pathspec down to unpack-trees machinery)\n\n-- 8< --\nSubject: [PATCH] diff-index: enable recursive pathspec matching in unpack_trees\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n diff-lib.c | 2 ++\n t/t4010-diff-pathspec.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 62f4cd9..fc0dff3 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -469,6 +469,8 @@ static int diff_cache(struct rev_info *revs,\n \topts.src_index = &the_index;\n \topts.dst_index = NULL;\n \topts.pathspec = &revs->diffopt.pathspec;\n+\topts.pathspec->recursive = 1;\n+\topts.pathspec->max_depth = -1;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \treturn unpack_trees(1, &t, &opts);\ndiff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh\nindex fbc8cd8..af5134b 100755\n--- a/t/t4010-diff-pathspec.sh\n+++ b/t/t4010-diff-pathspec.sh\n@@ -48,6 +48,14 @@ test_expect_success \\\n compare_diff_raw current expected'\n \n cat >expected <<\\EOF\n+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M\tpath1/file1\n+EOF\n+test_expect_success \\\n+ '\"*file1\" should show path1/file1' \\\n+ 'git diff-index --cached $tree -- \"*file1\" >current &&\n+ compare_diff_raw current expected'\n+\n+cat >expected <<\\EOF\n :100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M\tfile0\n EOF\n test_expect_success \\\n-- \n1.7.8.36.g69ee2\n\n-- 8< --\n-- \nDuy\n"},{"id":"182639","messageId":"7v1uqzwaxs.fsf@alter.siamese.dyndns.org","threadId":"28198","inReplyTo":"20120115100327.GA10735@do","subject":"Re: [PATCH v2 2/2] tree_entry_interesting: make recursive mode default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-16T22:15:59Z","receivedAt":"2012-01-16T22:15:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Sat, Jan 14, 2012 at 07:12:03PM -0800, Junio C Hamano wrote:\n>> That makes my head hurt and makes me suspect there is something\n>> fundamentally wrong in the patch. Sigh...\n>\n> I'll need to think about it. In the meantime perhaps the following\n> bandage patch would suffice, rather than revert 2f88c19 (diff-index:\n> pass pathspec down to unpack-trees machinery)\n\nYeah, the logic of this correction is very clear. Because diff_cache is\nabout walking a flat index, the \"recursive pathspec\" that allows us to\nlook into deeper levels in directory hierarchy should be set, and also we\nshould not be limiting the depth of the match in any way by setting the\nmax_depth to \"unlimited\".\n\nThanks.\n\n> -- 8< --\n> Subject: [PATCH] diff-index: enable recursive pathspec matching in unpack_trees\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff-lib.c | 2 ++\n> t/t4010-diff-pathspec.sh | 8 ++++++++\n> 2 files changed, 10 insertions(+), 0 deletions(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 62f4cd9..fc0dff3 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -469,6 +469,8 @@ static int diff_cache(struct rev_info *revs,\n> \topts.src_index = &the_index;\n> \topts.dst_index = NULL;\n> \topts.pathspec = &revs->diffopt.pathspec;\n> +\topts.pathspec->recursive = 1;\n> +\topts.pathspec->max_depth = -1;\n> \n> \tinit_tree_desc(&t, tree->buffer, tree->size);\n> \treturn unpack_trees(1, &t, &opts);\n> diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh\n> index fbc8cd8..af5134b 100755\n> --- a/t/t4010-diff-pathspec.sh\n> +++ b/t/t4010-diff-pathspec.sh\n> @@ -48,6 +48,14 @@ test_expect_success \\\n> compare_diff_raw current expected'\n> \n> cat >expected <<\\EOF\n> +:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M\tpath1/file1\n> +EOF\n> +test_expect_success \\\n> + '\"*file1\" should show path1/file1' \\\n> + 'git diff-index --cached $tree -- \"*file1\" >current &&\n> + compare_diff_raw current expected'\n> +\n> +cat >expected <<\\EOF\n> :100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M\tfile0\n> EOF\n> test_expect_success \\\n> -- \n> 1.7.8.36.g69ee2\n>\n> -- 8< --\n"},{"id":"182720","messageId":"20120118085951.GA704@duynguyen-vnpc.dek-tpc.internal","threadId":"28198","inReplyTo":"7v1uqzwaxs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] tree_entry_interesting: make recursive mode default","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-18T08:59:52Z","receivedAt":"2012-01-18T08:59:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 16, 2012 at 02:15:59PM -0800, Junio C Hamano wrote:\n> Yeah, the logic of this correction is very clear. Because diff_cache is\n> about walking a flat index, the \"recursive pathspec\" that allows us to\n> look into deeper levels in directory hierarchy should be set, and also we\n> should not be limiting the depth of the match in any way by setting the\n> max_depth to \"unlimited\".\n\nFor the record, enabling wildcard in git-log is just as simple. But I\nguess you don't want more changes this late in rc cycles.\n\n-- 8< --\ndiff --git a/revision.c b/revision.c\nindex 064e351..c426271 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1816,6 +1816,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \tif (revs->prune_data.nr) {\n \t\tdiff_tree_setup_paths(revs->prune_data.raw, &revs->pruning);\n+\t\trevs->pruning.pathspec.recursive = 1;\n+\t\trevs->pruning.pathspec.max_depth = -1;\n \t\t/* Can't prune commits with rename following: the paths change.. */\n \t\tif (!DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))\n \t\t\trevs->prune = 1;\n-- 8< --\n\n--\nDuy\n"}]}