{"thread":{"id":"24853","subject":"[PATCH 1/7] Add testcase showing how pathspecs are ignored with rev-list --objects","startedAt":"2010-08-25T02:53:08Z","lastAt":"2010-08-26T23:41:20Z","messageCount":16,"participants":["Elijah Newren","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"148920","messageId":"1282704795-29661-1-git-send-email-newren@gmail.com","threadId":"24853","inReplyTo":null,"subject":"[PATCH 0/7] Minor bug fix and optimizations for revision/tree walking","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T02:53:08Z","receivedAt":"2010-08-25T02:53:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"While working on sparse clones[1], I discovered a minor bug in the\ntree-walking machinery for rev-list when both --objects and paths are\nspecified.  In particular, such a combination of options would\n(correctly) result in the paths being used to select relevant commits,\nbut (incorrectly) being ignored when walking the subtrees of those\ncommits.  While I was at it, I also cleaned a few things up and\nprovided some small optimizations.  Passes all the tests for me.\n\nNOTE: The last two patches are here mostly to see if anyone knows of\nany real uses of git with ginormous trees that are really deep and\nhave few entries per tree.  These two patches might help with such a\ncase, but otherwise they seem to make the code a bit uglier and don't\nreally help performance-wise (and I think may even hurt a little bit).\n\nPerformance-wise, ignoring the last two patches, I get the following\napproximate speedups:\n  (A -  0%) git rev-list --quiet HEAD\n  (B -  4%) git rev-list --quiet HEAD -- Documentation/\n  (C -  3%) git rev-list --quiet HEAD -- t/\n  (D -  1%) git rev-list --objects HEAD > /dev/null\n  (E - 66%) git rev-list --objects HEAD -- Documentation/ > /dev/null\n\nComplete timings (in seconds) on my laptop (core 2 duo?):\n               A      B      C      D      E\n  maint       0.35   0.69   1.35   1.92   1.40\n  Patch-1     0.35   0.70   1.35   1.92   1.40\n  Patch-2     0.34   0.69   1.35   1.90   0.85\n  Patch-3     0.35   0.69   1.35   1.90   0.84\n  Patch-4     0.34   0.70   1.35   1.90   0.85\n  Patch-5     0.35   0.66   1.31   1.90   0.84\n  Patch-6     0.35   0.66   1.31   1.92   0.82\n  Patch-7     0.35   0.66   1.31   1.91   0.81\n\nNote that for each case, I ran 6 times and averaged the last 5 runs.\nI've rerun the cases a few times to regenerate the above table; the\nnumbers seem to vary about +/- 0.01 seconds between runs so the\nspeedups are slightly noisy given the small values, but they are\nconsistently positive for me.  I sometimes see the last two patches\nhave a little more negative impact, though it's pretty close to noise.\n\n\nElijah Newren (7):\n  Add testcase showing how pathspecs are ignored with rev-list\n    --objects\n  Fix ignoring of pathspecs with rev-list --objects\n  tree-walk: Correct bitrotted comment about tree_entry()\n  tree_entry_interesting(): Make return value more specific\n  diff_tree(): Skip skip_uninteresting() when all remaining paths\n    interesting\n  list-objects.c: Avoid recomputing interesting-ness for subtrees when\n    possible\n  tree-diff.c: Avoid recomputing interesting-ness for subtrees when\n    possible\n\n diff.h                   |    1 +\n list-objects.c           |   27 ++++++++++++++++++---\n t/t6000-rev-list-misc.sh |   23 ++++++++++++++++++\n tree-diff.c              |   58 +++++++++++++++++++++++-----------------------\n tree-walk.h              |    4 ++-\n 5 files changed, 79 insertions(+), 34 deletions(-)\n create mode 100755 t/t6000-rev-list-misc.sh\n\n[1] It looks like Nguyễn Thái Ngọc Duy is much further along than I am\non such work.  Perhaps the only point in the work I was doing was to\nenable me to help review a few of his patches (which I will try to\nfind some time to do), though perhaps the different route I have taken\nwill end up helping some.  It's fun and educational either way.\n\n-- \n1.7.2.2.39.gf7e23\n"},{"id":"148918","messageId":"1282704795-29661-2-git-send-email-newren@gmail.com","threadId":"24853","inReplyTo":"1282704795-29661-1-git-send-email-newren@gmail.com","subject":"[PATCH 1/7] Add testcase showing how pathspecs are ignored with rev-list --objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T02:53:09Z","receivedAt":"2010-08-25T02:53:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\nIt looks like the 19 rev-list testcases (t6000*-t6019*) are all fairly\nspecific, and I didn't see one where this testcase seemed to make sense;\nor perhaps I just missed it.  I just created this 20th file to hold all\nother miscellaneous rev-list testcases; if it does make more sense in\none of the other existing files, just let me know.\n\n t/t6000-rev-list-misc.sh |   23 +++++++++++++++++++++++\n 1 files changed, 23 insertions(+), 0 deletions(-)\n create mode 100755 t/t6000-rev-list-misc.sh\n\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nnew file mode 100755\nindex 0000000..2c403ac\n--- /dev/null\n+++ b/t/t6000-rev-list-misc.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='miscellaneous rev-list tests'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\techo content1 > wanted_file &&\n+\techo content2 > unwanted_file &&\n+\tgit add wanted_file unwanted_file &&\n+\tgit commit -mone\n+'\n+\n+test_expect_failure 'rev-list --objects heeds pathspecs' '\n+\n+\tgit rev-list --objects HEAD -- wanted_file >output &&\n+\tgrep wanted_file output &&\n+\t! grep unwanted_file output\n+\n+'\n+\n+test_done\n-- \n1.7.2.2.39.gf7e23\n"},{"id":"148919","messageId":"1282704795-29661-3-git-send-email-newren@gmail.com","threadId":"24853","inReplyTo":"1282704795-29661-1-git-send-email-newren@gmail.com","subject":"[PATCH 2/7] Fix ignoring of pathspecs with rev-list --objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T02:53:10Z","receivedAt":"2010-08-25T02:53:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When traversing commits, the selection of commits would heed the list of\npathspecs passed, but subsequent walking of the trees of those commits\nwould not.  This resulted in 'rev-list --objects HEAD -- <paths>'\ndisplaying objects at unwanted paths.\n\nHave process_tree() call tree_entry_interesting() to determine which paths\nare interesting and should be walked.\n\nNaturally, this change can provide a large speedup when paths are specified\ntogether with --objects, since many tree entries are now correctly ignored.\nInterestingly, though, this change also gives me a small (~1%) but\nrepeatable speedup even when no paths are specified with --objects.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n diff.h                   |    1 +\n list-objects.c           |   20 +++++++++++++++++++-\n t/t6000-rev-list-misc.sh |    2 +-\n tree-diff.c              |    2 +-\n 4 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.h b/diff.h\nindex 6fff024..35130b8 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -169,6 +169,7 @@ extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,\n \t\t\t  const char *base, struct diff_options *opt);\n extern int diff_root_tree_sha1(const unsigned char *new, const char *base,\n                                struct diff_options *opt);\n+extern int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen, struct diff_options *opt);\n \n struct combine_diff_path {\n \tstruct combine_diff_path *next;\ndiff --git a/list-objects.c b/list-objects.c\nindex 8953548..daa50bf 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -67,6 +67,7 @@ static void process_tree(struct rev_info *revs,\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n \tstruct name_path me;\n+\tint all_interesting = (revs->diffopt.nr_paths == 0);\n \n \tif (!revs->tree_objects)\n \t\treturn;\n@@ -84,7 +85,24 @@ static void process_tree(struct rev_info *revs,\n \n \tinit_tree_desc(&desc, tree->buffer, tree->size);\n \n-\twhile (tree_entry(&desc, &entry)) {\n+\tfor (; desc.size; update_tree_entry(&desc)) {\n+\t\tentry = desc.entry;\n+\n+\t\tif (!all_interesting) {\n+\t\t\tchar *full_path = path_name(path, name);\n+\t\t\tint full_path_len = strlen(full_path);\n+\t\t\tint showit = tree_entry_interesting(&desc, full_path, full_path_len,\n+\t\t\t\t\t\t\t    &revs->diffopt);\n+\t\t\tfree(full_path);\n+\n+\t\t\tif (showit < 0)\n+\t\t\t\tbreak;\n+\t\t\telse if (!showit)\n+\t\t\t\tcontinue;\n+\t\t\telse if (showit == 2)\n+\t\t\t\tall_interesting = 1;\n+\t\t}\n+\n \t\tif (S_ISDIR(entry.mode))\n \t\t\tprocess_tree(revs,\n \t\t\t\t     lookup_tree(entry.sha1),\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 2c403ac..1bc395c 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -12,7 +12,7 @@ test_expect_success setup '\n \tgit commit -mone\n '\n \n-test_expect_failure 'rev-list --objects heeds pathspecs' '\n+test_expect_success 'rev-list --objects heeds pathspecs' '\n \n \tgit rev-list --objects HEAD -- wanted_file >output &&\n \tgrep wanted_file output &&\ndiff --git a/tree-diff.c b/tree-diff.c\nindex cd659c6..2fb670b 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -91,7 +91,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n  *  - zero for no\n  *  - negative for \"no, and no subsequent entries will be either\"\n  */\n-static int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen, struct diff_options *opt)\n+int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen, struct diff_options *opt)\n {\n \tconst char *path;\n \tconst unsigned char *sha1;\n-- \n1.7.2.2.39.gf7e23\n"},{"id":"148921","messageId":"1282704795-29661-4-git-send-email-newren@gmail.com","threadId":"24853","inReplyTo":"1282704795-29661-1-git-send-email-newren@gmail.com","subject":"[PATCH 3/7] tree-walk: Correct bitrotted comment about tree_entry()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T02:53:11Z","receivedAt":"2010-08-25T02:53:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"There was a code comment that referred to the \"above two functions\" but\nover time the functions immediately preceding the comment have changed.\nJust mention the relevant functions by name.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n tree-walk.h |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/tree-walk.h b/tree-walk.h\nindex 42110a4..a5175fa 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -28,7 +28,9 @@ static inline int tree_entry_len(const char *name, const unsigned char *sha1)\n void update_tree_entry(struct tree_desc *);\n void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);\n \n-/* Helper function that does both of the above and returns true for success */\n+/* Helper function that does both tree_entry_extract() and update_tree_entry()\n+ * and returns true for success\n+ */\n int tree_entry(struct tree_desc *, struct name_entry *);\n \n void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);\n-- \n1.7.2.2.39.gf7e23\n"},{"id":"148925","messageId":"1282704795-29661-5-git-send-email-newren@gmail.com","threadId":"24853","inReplyTo":"1282704795-29661-1-git-send-email-newren@gmail.com","subject":"[PATCH 4/7] tree_entry_interesting(): Make return value more specific","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T02:53:12Z","receivedAt":"2010-08-25T02:53:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"tree_entry_interesting() can signal to its callers not only if the given\nentry matches one of the specified paths, but whether all remaining paths\nwill (or will not) match.  When no paths are specified, all paths are\nconsidered interesting, so intead of returning 1 (this path is interesting)\nreturn 2 (all paths are interesting).\n\nThis will allow the caller to avoid calling tree_entry_interesting() again,\nwhich theoretically should speed up tree walking.  I am not able to measure\nany actual gains in practice, but it certainly can not hurt and makes more\nsense to me.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n tree-diff.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 2fb670b..a740a9c 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -101,7 +101,7 @@ int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen\n \tint never_interesting = -1;\n \n \tif (!opt->nr_paths)\n-\t\treturn 1;\n+\t\treturn 2;\n \n \tsha1 = tree_entry_extract(desc, &path, &mode);\n \n-- \n1.7.2.2.39.gf7e23\n"},{"id":"148924","messageId":"1282704795-29661-6-git-send-email-newren@gmail.com","threadId":"24853","inReplyTo":"1282704795-29661-1-git-send-email-newren@gmail.com","subject":"[PATCH 5/7] diff_tree(): Skip skip_uninteresting() when all remaining paths interesting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T02:53:13Z","receivedAt":"2010-08-25T02:53:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In 1d848f6 (tree_entry_interesting(): allow it to say \"everything is\ninteresting\" 2007-03-21), both show_tree() and skip_uninteresting() were\nmodified to determine if all remaining tree entries were interesting.\nHowever, the latter returns as soon as it finds the first interesting path,\nwithout any way to signal to its caller (namely, diff_tree()) that all\nremaining paths are interesting, making these extra checks useless.\n\nPass whether all remaining entries are interesting back to diff_tree(), and\nwhenever they are, have diff_tree() skip subsequent calls to\nskip_uninteresting().\n\nWith this change, I measure speedups of 3-4% for the commands\n  git rev-list --quiet HEAD -- Documentation/\n  git rev-list --quiet HEAD -- t/\nin git.git.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n tree-diff.c |   25 ++++++++++++-------------\n 1 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex a740a9c..292cb6b 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -257,19 +257,12 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n \t}\n }\n \n-static void skip_uninteresting(struct tree_desc *t, const char *base, int baselen, struct diff_options *opt)\n+static void skip_uninteresting(struct tree_desc *t, const char *base, int baselen, struct diff_options *opt, int *all_interesting)\n {\n-\tint all_interesting = 0;\n \twhile (t->size) {\n-\t\tint show;\n-\n-\t\tif (all_interesting)\n-\t\t\tshow = 1;\n-\t\telse {\n-\t\t\tshow = tree_entry_interesting(t, base, baselen, opt);\n-\t\t\tif (show == 2)\n-\t\t\t\tall_interesting = 1;\n-\t\t}\n+\t\tint show = tree_entry_interesting(t, base, baselen, opt);\n+\t\tif (show == 2)\n+\t\t\t*all_interesting = 1;\n \t\tif (!show) {\n \t\t\tupdate_tree_entry(t);\n \t\t\tcontinue;\n@@ -284,14 +277,20 @@ static void skip_uninteresting(struct tree_desc *t, const char *base, int basele\n int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, struct diff_options *opt)\n {\n \tint baselen = strlen(base);\n+\tint all_t1_interesting = 0;\n+\tint all_t2_interesting = 0;\n \n \tfor (;;) {\n \t\tif (DIFF_OPT_TST(opt, QUICK) &&\n \t\t    DIFF_OPT_TST(opt, HAS_CHANGES))\n \t\t\tbreak;\n \t\tif (opt->nr_paths) {\n-\t\t\tskip_uninteresting(t1, base, baselen, opt);\n-\t\t\tskip_uninteresting(t2, base, baselen, opt);\n+\t\t\tif (!all_t1_interesting)\n+\t\t\t\tskip_uninteresting(t1, base, baselen, opt,\n+\t\t\t\t\t\t   &all_t1_interesting);\n+\t\t\tif (!all_t2_interesting)\n+\t\t\t\tskip_uninteresting(t2, base, baselen, opt,\n+\t\t\t\t\t\t   &all_t2_interesting);\n \t\t}\n \t\tif (!t1->size) {\n \t\t\tif (!t2->size)\n-- \n1.7.2.2.39.gf7e23\n"},{"id":"148922","messageId":"1282704795-29661-7-git-send-email-newren@gmail.com","threadId":"24853","inReplyTo":"1282704795-29661-1-git-send-email-newren@gmail.com","subject":"[PATCH 6/7] list-objects.c: Avoid recomputing interesting-ness for subtrees when possible","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T02:53:14Z","receivedAt":"2010-08-25T02:53:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Weather balloon patch.  Doesn't seem to help much in benchmarks; in fact\nI think it sometimes hurts a bit.  Is it worthwhile just in case someone\ncomes up with a ginormous tree that is really deep with few entries per\ntree?  I'm leaning against it, but am sending the patch to at least show\nothers that it has been considered.\n\n---\nNo signed-off-by, since I'm not sold on this patch and am somewhat leaning\nagainst it.  It would need a better commit message anyway.  :-)\n\n list-objects.c |    9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex daa50bf..66e4ccc 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -61,13 +61,13 @@ static void process_tree(struct rev_info *revs,\n \t\t\t struct tree *tree,\n \t\t\t show_object_fn show,\n \t\t\t struct name_path *path,\n-\t\t\t const char *name)\n+\t\t\t const char *name,\n+\t\t\t int all_interesting)\n {\n \tstruct object *obj = &tree->object;\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n \tstruct name_path me;\n-\tint all_interesting = (revs->diffopt.nr_paths == 0);\n \n \tif (!revs->tree_objects)\n \t\treturn;\n@@ -106,7 +106,8 @@ static void process_tree(struct rev_info *revs,\n \t\tif (S_ISDIR(entry.mode))\n \t\t\tprocess_tree(revs,\n \t\t\t\t     lookup_tree(entry.sha1),\n-\t\t\t\t     show, &me, entry.path);\n+\t\t\t\t     show, &me, entry.path,\n+\t\t\t\t     all_interesting);\n \t\telse if (S_ISGITLINK(entry.mode))\n \t\t\tprocess_gitlink(revs, entry.sha1,\n \t\t\t\t\tshow, &me, entry.path);\n@@ -182,7 +183,7 @@ void traverse_commit_list(struct rev_info *revs,\n \t\t}\n \t\tif (obj->type == OBJ_TREE) {\n \t\t\tprocess_tree(revs, (struct tree *)obj, show_object,\n-\t\t\t\t     NULL, name);\n+\t\t\t\t     NULL, name, revs->diffopt.nr_paths == 0);\n \t\t\tcontinue;\n \t\t}\n \t\tif (obj->type == OBJ_BLOB) {\n-- \n1.7.2.2.39.gf7e23\n"},{"id":"148923","messageId":"1282704795-29661-8-git-send-email-newren@gmail.com","threadId":"24853","inReplyTo":"1282704795-29661-1-git-send-email-newren@gmail.com","subject":"[PATCH 7/7] tree-diff.c: Avoid recomputing interesting-ness for subtrees when possible","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T02:53:15Z","receivedAt":"2010-08-25T02:53:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Weather balloon patch.  Doesn't seem to help much in benchmarks, and after\nmany runs I think it might occasionally hurt slightly.  It ought to help\nin the case of a ginormous tree that is really deep with few entries per\ntree, but that case seems kind of contrived.\n\nKind of makes the code look icky too.\n\n---\nNo signed-off-by, since I'm not sold on this patch and am somewhat leaning\nagainst it.  It would need a better commit message anyway.  :-)\n\n tree-diff.c |   29 +++++++++++++++--------------\n 1 files changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 292cb6b..3c34c77 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -25,9 +25,9 @@ static char *malloc_fullname(const char *base, int baselen, const char *path, in\n }\n \n static void show_entry(struct diff_options *opt, const char *prefix, struct tree_desc *desc,\n-\t\t       const char *base, int baselen);\n+\t\t       const char *base, int baselen, int all_interesting);\n \n-static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const char *base, int baselen, struct diff_options *opt)\n+static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const char *base, int baselen, struct diff_options *opt, int all_t1_interesting, int all_t2_interesting)\n {\n \tunsigned mode1, mode2;\n \tconst char *path1, *path2;\n@@ -42,11 +42,11 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n \tpathlen2 = tree_entry_len(path2, sha2);\n \tcmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);\n \tif (cmp < 0) {\n-\t\tshow_entry(opt, \"-\", t1, base, baselen);\n+\t\tshow_entry(opt, \"-\", t1, base, baselen, all_t1_interesting);\n \t\treturn -1;\n \t}\n \tif (cmp > 0) {\n-\t\tshow_entry(opt, \"+\", t2, base, baselen);\n+\t\tshow_entry(opt, \"+\", t2, base, baselen, all_t2_interesting);\n \t\treturn 1;\n \t}\n \tif (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(sha1, sha2) && mode1 == mode2)\n@@ -57,8 +57,8 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n \t * file, we need to consider it a remove and an add.\n \t */\n \tif (S_ISDIR(mode1) != S_ISDIR(mode2)) {\n-\t\tshow_entry(opt, \"-\", t1, base, baselen);\n-\t\tshow_entry(opt, \"+\", t2, base, baselen);\n+\t\tshow_entry(opt, \"-\", t1, base, baselen, all_t1_interesting);\n+\t\tshow_entry(opt, \"+\", t2, base, baselen, all_t2_interesting);\n \t\treturn 0;\n \t}\n \n@@ -197,9 +197,8 @@ int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen\n }\n \n /* A whole sub-tree went away or appeared */\n-static void show_tree(struct diff_options *opt, const char *prefix, struct tree_desc *desc, const char *base, int baselen)\n+static void show_tree(struct diff_options *opt, const char *prefix, struct tree_desc *desc, const char *base, int baselen, int all_interesting)\n {\n-\tint all_interesting = 0;\n \twhile (desc->size) {\n \t\tint show;\n \n@@ -214,14 +213,14 @@ static void show_tree(struct diff_options *opt, const char *prefix, struct tree_\n \t\tif (show < 0)\n \t\t\tbreak;\n \t\tif (show)\n-\t\t\tshow_entry(opt, prefix, desc, base, baselen);\n+\t\t\tshow_entry(opt, prefix, desc, base, baselen, all_interesting);\n \t\tupdate_tree_entry(desc);\n \t}\n }\n \n /* A file entry went away or appeared */\n static void show_entry(struct diff_options *opt, const char *prefix, struct tree_desc *desc,\n-\t\t       const char *base, int baselen)\n+\t\t       const char *base, int baselen, int all_interesting)\n {\n \tunsigned mode;\n \tconst char *path;\n@@ -246,7 +245,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n \t\t}\n \n \t\tinit_tree_desc(&inner, tree, size);\n-\t\tshow_tree(opt, prefix, &inner, newbase, baselen + 1 + pathlen);\n+\t\tshow_tree(opt, prefix, &inner, newbase, baselen + 1 + pathlen, all_interesting);\n \n \t\tfree(tree);\n \t\tfree(newbase);\n@@ -295,16 +294,18 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, stru\n \t\tif (!t1->size) {\n \t\t\tif (!t2->size)\n \t\t\t\tbreak;\n-\t\t\tshow_entry(opt, \"+\", t2, base, baselen);\n+\t\t\tshow_entry(opt, \"+\", t2, base, baselen, all_t2_interesting);\n \t\t\tupdate_tree_entry(t2);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!t2->size) {\n-\t\t\tshow_entry(opt, \"-\", t1, base, baselen);\n+\t\t\tshow_entry(opt, \"-\", t1, base, baselen, all_t1_interesting);\n \t\t\tupdate_tree_entry(t1);\n \t\t\tcontinue;\n \t\t}\n-\t\tswitch (compare_tree_entry(t1, t2, base, baselen, opt)) {\n+\t\tswitch (compare_tree_entry(t1, t2, base, baselen, opt,\n+\t\t\t\t\t   all_t1_interesting,\n+\t\t\t\t\t   all_t2_interesting)) {\n \t\tcase -1:\n \t\t\tupdate_tree_entry(t1);\n \t\t\tcontinue;\n-- \n1.7.2.2.39.gf7e23\n"},{"id":"149010","messageId":"AANLkTimguNHXSFyO1EvS+dHbHiXU-dXTNwkWfEsVUbVC@mail.gmail.com","threadId":"24853","inReplyTo":"1282704795-29661-3-git-send-email-newren@gmail.com","subject":"Re: [PATCH 2/7] Fix ignoring of pathspecs with rev-list --objects","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-25T22:11:07Z","receivedAt":"2010-08-25T22:11:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 25, 2010 at 12:53 PM, Elijah Newren <newren@gmail.com> wrote:\n> @@ -84,7 +85,24 @@ static void process_tree(struct rev_info *revs,\n>\n>        init_tree_desc(&desc, tree->buffer, tree->size);\n>\n> -       while (tree_entry(&desc, &entry)) {\n> +       for (; desc.size; update_tree_entry(&desc)) {\n> +               entry = desc.entry;\n> +\n> +               if (!all_interesting) {\n> +                       char *full_path = path_name(path, name);\n> +                       int full_path_len = strlen(full_path);\n> +                       int showit = tree_entry_interesting(&desc,\n> full_path, full_path_len,\n> +                                                           &revs->diffopt);\n> +                       free(full_path);\n\nI wonder if we can avoid xmalloc/free so many times here. If full_path\nis unchanged, how about moving it outside the loop?\n\n> diff --git a/tree-diff.c b/tree-diff.c\n> index cd659c6..2fb670b 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -91,7 +91,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct\n> tree_desc *t2, const\n>  *  - zero for no\n>  *  - negative for \"no, and no subsequent entries will be either\"\n>  */\n> -static int tree_entry_interesting(struct tree_desc *desc, const char *base,\n> int baselen, struct diff_options *opt)\n> +int tree_entry_interesting(struct tree_desc *desc, const char *base, int\n> baselen, struct diff_options *opt)\n>  {\n>        const char *path;\n>        const unsigned char *sha1;\n\nWhile at it, can you please also fix its comments? The comments say\npathspec while what it uses is actually path prefix. Maybe something\nlike this\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 3c34c77..514dbca 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -83,7 +83,7 @@ static int compare_tree_entry(struct tree_desc *t1,\nstruct tree_desc *t2, const\n }\n\n /*\n- * Is a tree entry interesting given the pathspec we have?\n+ * Is a tree entry interesting given the path prefix we have?\n  *\n  * Return:\n  *  - 2 for \"yes, and all subsequent entries will be\"\n\n\nThis patch is good stuff. Please be informed I will steal this patch\nfor my narrow use.\n-- \nDuy\n"},{"id":"149018","messageId":"7v1v9mp95o.fsf@alter.siamese.dyndns.org","threadId":"24853","inReplyTo":"1282704795-29661-6-git-send-email-newren@gmail.com","subject":"Re: [PATCH 5/7] diff_tree(): Skip skip_uninteresting() when all remaining paths interesting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-25T23:25:23Z","receivedAt":"2010-08-25T23:25:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This and the previous patch seem to make sense to me, independent of the\nrest.\n\nAs to mixing pathspecs with --objects, I would say it is unfair to Linus\nto call it a bug that that particular use case has not been supported.  I'd\nsay it _is_ a bug that we didn't error out when --objects was given with\npathspec at the command line level, though, as that is a combination we\nknowingly ignored to support.\n\nIt simply hasn't been deemed as a sane operation to produce a pack using\nan output from rev-list with pathspec and --objects, as it will leave some\ntrees and blobs that the pack knows about in the result, without actually\nhaving them in it.  In the context of \"narrow clone\", these wounds to\ntrees are deliberate, and the existence of these wounds alone is not the\nreason why I called it insane to produce such a pack.\n\nTo make use of such a pack, however, you need to somehow cauterize these\ndeliberate wounds in trees so that fsck, a later run of pack-objects,\nfetch-pack and friends will not choke on them.  We didn't plan to have\nsuch an infrastructure so far, and that is what made such a pack with\nthousands of cuts \"insane\".  As soon as \"narrow clone\" addresses that\nissue, mixture of pathspecs with --objects stops being an insane use\ncase.\n"},{"id":"149019","messageId":"AANLkTimCv-fn-h21pM8L9NiGg3w7OXBJ-kfiC+HJLsTP@mail.gmail.com","threadId":"24853","inReplyTo":"AANLkTimguNHXSFyO1EvS+dHbHiXU-dXTNwkWfEsVUbVC@mail.gmail.com","subject":"Re: [PATCH 2/7] Fix ignoring of pathspecs with rev-list --objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T23:50:39Z","receivedAt":"2010-08-25T23:50:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 25, 2010 at 4:11 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:\n> On Wed, Aug 25, 2010 at 12:53 PM, Elijah Newren <newren@gmail.com> wrote:\n>> @@ -84,7 +85,24 @@ static void process_tree(struct rev_info *revs,\n>>\n>>        init_tree_desc(&desc, tree->buffer, tree->size);\n>>\n>> -       while (tree_entry(&desc, &entry)) {\n>> +       for (; desc.size; update_tree_entry(&desc)) {\n>> +               entry = desc.entry;\n>> +\n>> +               if (!all_interesting) {\n>> +                       char *full_path = path_name(path, name);\n>> +                       int full_path_len = strlen(full_path);\n>> +                       int showit = tree_entry_interesting(&desc,\n>> full_path, full_path_len,\n>> +                                                           &revs->diffopt);\n>> +                       free(full_path);\n>\n> I wonder if we can avoid xmalloc/free so many times here. If full_path\n> is unchanged, how about moving it outside the loop?\n\nGood point.\n\n>> diff --git a/tree-diff.c b/tree-diff.c\n>> index cd659c6..2fb670b 100644\n>> --- a/tree-diff.c\n>> +++ b/tree-diff.c\n>> @@ -91,7 +91,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct\n>> tree_desc *t2, const\n>>  *  - zero for no\n>>  *  - negative for \"no, and no subsequent entries will be either\"\n>>  */\n>> -static int tree_entry_interesting(struct tree_desc *desc, const char *base,\n>> int baselen, struct diff_options *opt)\n>> +int tree_entry_interesting(struct tree_desc *desc, const char *base, int\n>> baselen, struct diff_options *opt)\n>>  {\n>>        const char *path;\n>>        const unsigned char *sha1;\n>\n> While at it, can you please also fix its comments? The comments say\n> pathspec while what it uses is actually path prefix. Maybe something\n> like this\n>\n> diff --git a/tree-diff.c b/tree-diff.c\n> index 3c34c77..514dbca 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -83,7 +83,7 @@ static int compare_tree_entry(struct tree_desc *t1,\n> struct tree_desc *t2, const\n>  }\n>\n>  /*\n> - * Is a tree entry interesting given the pathspec we have?\n> + * Is a tree entry interesting given the path prefix we have?\n\nI believe the comment is parsed thus: \"tree entry\" == combination of\ndesc, base, and baselen.  \"pathspec\" == paths and pathlens fields of\nopt (which do provide a pathspec).  So I believe the original was\ncorrect, though I can see how it's confusing at first.\n\n>  *\n>  * Return:\n>  *  - 2 for \"yes, and all subsequent entries will be\"\n>\n>\n> This patch is good stuff. Please be informed I will steal this patch\n> for my narrow use.\n\nThanks, but I just found a bug with it; I'm investigating.\n"},{"id":"149021","messageId":"AANLkTi=Z0nZ7dPqZisPkbpGUBvoFbu9UVxkMq0hucGGg@mail.gmail.com","threadId":"24853","inReplyTo":"7v1v9mp95o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/7] diff_tree(): Skip skip_uninteresting() when all remaining paths interesting","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-25T23:57:06Z","receivedAt":"2010-08-25T23:57:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 25, 2010 at 5:25 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> This and the previous patch seem to make sense to me, independent of the\n> rest.\n\nThanks.\n\n> As to mixing pathspecs with --objects, I would say it is unfair to Linus\n> to call it a bug that that particular use case has not been supported.  I'd\n> say it _is_ a bug that we didn't error out when --objects was given with\n> pathspec at the command line level, though, as that is a combination we\n> knowingly ignored to support.\n>\n> It simply hasn't been deemed as a sane operation to produce a pack using\n> an output from rev-list with pathspec and --objects, as it will leave some\n> trees and blobs that the pack knows about in the result, without actually\n> having them in it.  In the context of \"narrow clone\", these wounds to\n> trees are deliberate, and the existence of these wounds alone is not the\n> reason why I called it insane to produce such a pack.\n>\n> To make use of such a pack, however, you need to somehow cauterize these\n> deliberate wounds in trees so that fsck, a later run of pack-objects,\n> fetch-pack and friends will not choke on them.  We didn't plan to have\n> such an infrastructure so far, and that is what made such a pack with\n> thousands of cuts \"insane\".  As soon as \"narrow clone\" addresses that\n> issue, mixture of pathspecs with --objects stops being an insane use\n> case.\n\nWell, I did call it a minor bug, because I figured no one was using it\nand so it wasn't hurting anyone.  But saying you knowingly ignored\nsupporting it is understandable.\n\nI understand that for this combination of options to be useful, all\nthe other stuff you mention needs to be done.  Duy and I are both\nworking on separate ways of doing that; I just figured it made sense\nto submit any independent pieces early and separately.  I don't think\nsupporting this would hurt anyone, as I don't think anyone could\naccidentally use it at this point.  However, if you'd prefer that I\nsubmitted a patch to just error out with this combination of options\nfor now, and resubmit this patch later with a full sparse clone\npatchset, I can do that.  (Note though that my fix for supporting both\n--objects and pathspec isn't correct; there's a bug I'm looking in\nto.)\n"},{"id":"149026","messageId":"AANLkTimRLUwB7ycnaUGjpztLYtN7SoLyA8rXvYDqfi4P@mail.gmail.com","threadId":"24853","inReplyTo":"AANLkTimCv-fn-h21pM8L9NiGg3w7OXBJ-kfiC+HJLsTP@mail.gmail.com","subject":"Re: [PATCH 2/7] Fix ignoring of pathspecs with rev-list --objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-26T02:49:32Z","receivedAt":"2010-08-26T02:49:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 25, 2010 at 5:50 PM, Elijah Newren <newren@gmail.com> wrote:\n> On Wed, Aug 25, 2010 at 4:11 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:\n>>\n>> I wonder if we can avoid xmalloc/free so many times here. If full_path\n>> is unchanged, how about moving it outside the loop?\n>\n> Good point.\n>\n\n<snip>\n\n> Thanks, but I just found a bug with it; I'm investigating.\n\ntree_entry_interesting assumes base[baselen-1] == '/' || baselen == 0;\nit will fail to notice matches if this precondition is not met, and I\nfailed to meet it except for files at the toplevel tree.\n\nI'll send out an updated series with a fix for this and the\nimprovement you suggested.\n"},{"id":"149028","messageId":"7voccqnidk.fsf@alter.siamese.dyndns.org","threadId":"24853","inReplyTo":"AANLkTi=Z0nZ7dPqZisPkbpGUBvoFbu9UVxkMq0hucGGg@mail.gmail.com","subject":"Re: [PATCH 5/7] diff_tree(): Skip skip_uninteresting() when all remaining paths interesting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-26T03:49:11Z","receivedAt":"2010-08-26T03:49:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> ...  I don't think\n> supporting this would hurt anyone, as I don't think anyone could\n> accidentally use it at this point.\n\nI agree 100%.  There is no harm in doing what the user asks us to do, so\nproducing list of objects that is path limited is fine [*1*].\n\nAfter all, you can feed any random set of object names to pack-objects and\nuse the resulting pack as a way to sneakernet just a random set of objects\nwithout any connectivity guarantee even today.\n\nAlso, since it is unlikely anybody would accidentally use the combination,\nit wouldn't make much sense to retroactively add a sanity check that\nerrors out either, as that needs to be lifted later when you two are done.\n\n\n[Footnote]\n\n*1* As long as it is done correctly, of course.  Watch out for a case like\nthe same blob that is outside the pathspec (marked as \"uninteresting\")\nappears inside (needs to be sent), i.e.\n\n    mkdir two\n    echo frotz >one\n    cp one two/three\n    git add one two/three\n    git commit -m that\n    git rev-list --objects two\n\nwhich _should_ show the blob that records \"frotz\\n\" because it appears at\n\"two/three\" which is in the area, even though you happen to have the same\none at a path outside (i.e. \"one\").\n"},{"id":"149093","messageId":"AANLkTi=Y5tbdxc4f2j_1D1gib2gwEzNw6_Az3Ag9T1nn@mail.gmail.com","threadId":"24853","inReplyTo":"AANLkTimCv-fn-h21pM8L9NiGg3w7OXBJ-kfiC+HJLsTP@mail.gmail.com","subject":"Re: [PATCH 2/7] Fix ignoring of pathspecs with rev-list --objects","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-08-26T23:15:29Z","receivedAt":"2010-08-26T23:15:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 26, 2010 at 9:50 AM, Elijah Newren <newren@gmail.com> wrote:\n>> While at it, can you please also fix its comments? The comments say\n>> pathspec while what it uses is actually path prefix. Maybe something\n>> like this\n>>\n>> diff --git a/tree-diff.c b/tree-diff.c\n>> index 3c34c77..514dbca 100644\n>> --- a/tree-diff.c\n>> +++ b/tree-diff.c\n>> @@ -83,7 +83,7 @@ static int compare_tree_entry(struct tree_desc *t1,\n>> struct tree_desc *t2, const\n>>  }\n>>\n>>  /*\n>> - * Is a tree entry interesting given the pathspec we have?\n>> + * Is a tree entry interesting given the path prefix we have?\n>\n> I believe the comment is parsed thus: \"tree entry\" == combination of\n> desc, base, and baselen.  \"pathspec\" == paths and pathlens fields of\n> opt (which do provide a pathspec).  So I believe the original was\n> correct, though I can see how it's confusing at first.\n\nPathspec as in match_pathspec() in dir.c allows wildcards. Though it\nseems only used with worktree or index-related operations. To me the\none with wildcard support is called pathspec. The one without is path\nprefix. But it's probably just me.\n\nThere is also a GSoC suggestion about this [1] and I posted a related\npatch a while back [2] but forgot it until now.\n\n[1] https://git.wiki.kernel.org/index.php/SoC2010Ideas#Unify_Pathspec_Semantics\n\n[2] http://mid.gmane.org/1243240924-5981-1-git-send-email-pclouds@gmail.com\n-- \nDuy\n"},{"id":"149094","messageId":"7v7hjdkkm7.fsf@alter.siamese.dyndns.org","threadId":"24853","inReplyTo":"AANLkTi=Y5tbdxc4f2j_1D1gib2gwEzNw6_Az3Ag9T1nn@mail.gmail.com","subject":"Re: [PATCH 2/7] Fix ignoring of pathspecs with rev-list --objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-26T23:41:20Z","receivedAt":"2010-08-26T23:41:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Thu, Aug 26, 2010 at 9:50 AM, Elijah Newren <newren@gmail.com> wrote:\n>>> While at it, can you please also fix its comments? The comments say\n>>> pathspec while what it uses is actually path prefix...\n>>> ...\n>>> - * Is a tree entry interesting given the pathspec we have?\n>>> + * Is a tree entry interesting given the path prefix we have?\n>>\n>> I believe the comment is parsed thus: \"tree entry\" == combination of\n>> desc, base, and baselen... So I believe the original was\n>> correct, though I can see how it's confusing at first.\n>\n> Pathspec as in match_pathspec() in dir.c allows wildcards...\n>\n> There is also a GSoC suggestion about this [1] and I posted a related\n> patch a while back [2] but forgot it until now.\n>\n> [1] https://git.wiki.kernel.org/index.php/SoC2010Ideas#Unify_Pathspec_Semantics\n>\n> [2] http://mid.gmane.org/1243240924-5981-1-git-send-email-pclouds@gmail.com\n\nAs [1] you quoted says, both are called pathspecs and because our longer\nterm goal is to unify them, I think the original comment is fine.\n\nI wonder why I was Cc'ed on this shed-coloring committee, though...\n"}]}