{"thread":{"id":"45218","subject":"[PATCH 2/3] revision: exclude trees/blobs given commit","startedAt":"2017-02-25T01:19:21Z","lastAt":"2017-03-03T22:31:34Z","messageCount":17,"participants":["Jonathan Tan","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"312599","messageId":"7082d91f30663b2e6d7fb1795c5ea37d3fe3446c.1487984670.git.jonathantanmy@google.com","threadId":"45218","inReplyTo":"cover.1487984670.git.jonathantanmy@google.com","subject":"[PATCH 2/3] revision: exclude trees/blobs given commit","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-25T01:18:37Z","receivedAt":"2017-02-25T01:19:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When the --objects argument is given to rev-list, an argument of the\nform \"^$tree\" can be given to exclude all blobs and trees reachable from\nthat tree, but an argument of the form \"^$commit\" only excludes that\ncommit, not any blob or tree reachable from it. Make \"^$commit\" behave\nconsistent to \"^$tree\".\n\nAlso, formalize this behavior in unit tests. (Some of the added tests\nwould already pass even before this commit, but are included\nnevertheless for completeness.)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n revision.c               |  2 ++\n t/t6000-rev-list-misc.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 90 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 5e49d9e0e..e6a62da98 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -254,6 +254,8 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\tdie(\"unable to parse commit %s\", name);\n \t\tif (flags & UNINTERESTING) {\n \t\t\tmark_parents_uninteresting(commit);\n+\t\t\tif (revs->tree_and_blob_objects)\n+\t\t\t\tmark_tree_uninteresting(commit->tree);\n \t\t\trevs->limited = 1;\n \t\t}\n \t\tif (revs->show_source && !commit->util)\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 969e4e9e5..c04a9582b 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -114,4 +114,92 @@ test_expect_success '--header shows a NUL after each commit' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup tree-granularity and blob-granularity tests' '\n+\techo 0 >file &&\n+\n+\tmkdir subdir &&\n+\techo 1 >subdir/file &&\n+\tgit add file subdir/file &&\n+\tgit commit -m one &&\n+\n+\techo 2 >subdir/file &&\n+\tgit add subdir/file &&\n+\tgit commit -m two &&\n+\n+\tcommit1=$(git rev-parse HEAD^1) &&\n+\tcommit2=$(git rev-parse HEAD) &&\n+\ttree1=$(git rev-parse $commit1^{tree}) &&\n+\ttree2=$(git rev-parse $commit2^{tree}) &&\n+\tsubtree1=$(git cat-file -p $tree1 | grep \"subdir\" | cut -c13-52) &&\n+\tsubtree2=$(git cat-file -p $tree2 | grep \"subdir\" | cut -c13-52) &&\n+\tblob0=$(echo 0 | git hash-object --stdin) &&\n+\tblob1=$(echo 1 | git hash-object --stdin) &&\n+\tblob2=$(echo 2 | git hash-object --stdin)\n+'\n+\n+test_expect_success 'include commit, exclude blob' '\n+\tgit rev-list --objects $commit2 >out &&\n+\tgrep \"$blob1\" out &&\n+\tgrep \"$blob2\" out &&\n+\n+\tgit rev-list --objects $commit2 \"^$blob2\" >out &&\n+\tgrep \"$blob1\" out &&\n+\t! grep \"$blob2\" out\n+'\n+\n+test_expect_success 'include commit, exclude tree (also excludes nested trees/blobs)' '\n+\tgit rev-list --objects $commit2 \"^$tree2\" >out &&\n+\tgrep \"$tree1\" out &&\n+\tgrep \"$subtree1\" out &&\n+\tgrep \"$blob1\" out &&\n+\t! grep \"$tree2\" out &&\n+\t! grep \"$subtree2\" out &&\n+\t! grep \"$blob2\" out &&\n+\n+\tgit rev-list --objects $commit2 \"^$subtree2\" >out &&\n+\tgrep \"$tree1\" out &&\n+\tgrep \"$subtree1\" out &&\n+\tgrep \"$blob1\" out &&\n+\tgrep \"$tree2\" out &&\n+\t! grep \"$subtree2\" out &&\n+\t! grep \"$blob2\" out\n+'\n+\n+test_expect_success 'include tree, exclude commit' '\n+\tgit rev-list --objects \"$tree1\" \"^$commit2\" >out &&\n+\t! grep \"$blob0\" out &&  # common to both\n+\tgrep \"$blob1\" out &&    # only in tree\n+\t! grep \"$blob2\" out     # only in commit\n+'\n+\n+test_expect_success 'include tree, exclude tree' '\n+\tgit rev-list --objects \"$tree1\" \"^$tree2\" >out &&\n+\t! grep \"$blob0\" out &&  # common to both\n+\tgrep \"$blob1\" out &&    # only in tree1\n+\t! grep \"$blob2\" out     # only in tree2\n+'\n+\n+test_expect_success 'include tree, exclude blob' '\n+\tgit rev-list --objects \"$tree1\" \"^$blob2\" >out &&\n+\tgrep \"$blob0\" out &&\n+\tgrep \"$blob1\" out &&\n+\t! grep \"$blob2\" out\n+'\n+\n+test_expect_success 'include blob, exclude commit' '\n+\tgit rev-list --objects \"$blob2\" \"^$commit1\" >out &&\n+\tgrep \"$blob2\" out &&\n+\n+\tgit rev-list --objects \"$blob2\" \"^$commit2\" >out &&\n+\t! grep \"$blob2\" out\n+'\n+\n+test_expect_success 'include blob, exclude tree' '\n+\tgit rev-list --objects \"$blob2\" \"^$tree1\" >out &&\n+\tgrep \"$blob2\" out &&\n+\n+\tgit rev-list --objects \"$blob2\" \"^$tree2\" >out &&\n+\t! grep \"$blob2\" out\n+'\n+\n test_done\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312600","messageId":"cover.1487984670.git.jonathantanmy@google.com","threadId":"45218","inReplyTo":null,"subject":"[PATCH 0/3] Test fetch-pack's ability to fetch arbitrary blobs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-25T01:18:35Z","receivedAt":"2017-02-25T01:19:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"As stated in a previous e-mail [1], I was trying to think a way to allow\nGit to fetch arbitrary blobs from another Git server, and it turned out\nthat fetch-pack already can. However, there were some bugs with blob\nreachability. This patch set fixes those bugs, and verifies (with tests)\nthat fetch-pack can fetch reachable blobs and cannot fetch unreachable\nblobs.\n\nThese patches are (I think) worthwhile on their own, but may be of\nspecial interest to people who need Git to tolerate missing objects in\nthe local repo (for example, the e-mail discussion \"[RFC] Add support\nfor downloading blobs on demand\" [2]) because a way for Git to download\nmissing objects natively is (I think) a prerequisite to that.\n\n[1] <20170223230358.30050-1-jonathantanmy@google.com>\n[2] <20170113155253.1644-1-benpeart@microsoft.com>\n\nJonathan Tan (3):\n  revision: unify {tree,blob}_objects in rev_info\n  revision: exclude trees/blobs given commit\n  upload-pack: compute blob reachability correctly\n\n bisect.c                 |  2 +-\n builtin/rev-list.c       |  6 ++--\n list-objects.c           |  4 +--\n pack-bitmap-write.c      |  3 +-\n pack-bitmap.c            |  3 +-\n reachable.c              |  3 +-\n revision.c               | 18 +++++-----\n revision.h               |  3 +-\n t/t5500-fetch-pack.sh    | 30 +++++++++++++++++\n t/t6000-rev-list-misc.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++\n upload-pack.c            | 15 +++++++++\n 11 files changed, 151 insertions(+), 24 deletions(-)\n\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312601","messageId":"06a84f8c77924b275606384ead8bb2fd7d75f7b6.1487984670.git.jonathantanmy@google.com","threadId":"45218","inReplyTo":"cover.1487984670.git.jonathantanmy@google.com","subject":"[PATCH 1/3] revision: unify {tree,blob}_objects in rev_info","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-25T01:18:36Z","receivedAt":"2017-02-25T01:19:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Whenever tree_objects is set to 1 in revision.h's struct rev_info,\nblob_objects is likewise set, and vice versa. Combine those two fields\ninto one.\n\nSome of the existing code does not handle tree_objects being different\nfrom blob_objects properly. For example, \"handle_commit\" in revision.c\nrecurses from an UNINTERESTING tree into its subtree if tree_objects ==\n1, completely ignoring blob_objects; it probably should still recurse if\ntree_objects == 0 and blob_objects == 1 (to mark the blobs), and should\nbehave differently depending on blob_objects (controlling the\ninstantiation and marking of blob objects). This commit resolves the\nissue by forbidding tree_objects from being different to blob_objects.\n\nIt could be argued that in the future, Git might need to distinguish\ntree_objects from blob_objects - in particular, a user might want\nrev-list to print the trees but not the blobs. However, this results in\na minor performance savings at best in that objects no longer need to be\ninstantiated (causing memory allocations and hashtable insertions) - no\ndisk reads are being done for objects whether blob_objects is set or\nnot.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n bisect.c            |  2 +-\n builtin/rev-list.c  |  6 +++---\n list-objects.c      |  4 ++--\n pack-bitmap-write.c |  3 +--\n pack-bitmap.c       |  3 +--\n reachable.c         |  3 +--\n revision.c          | 16 ++++++----------\n revision.h          |  3 +--\n 8 files changed, 16 insertions(+), 24 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 8e63c40d2..265b32905 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -634,7 +634,7 @@ static void bisect_common(struct rev_info *revs)\n {\n \tif (prepare_revision_walk(revs))\n \t\tdie(\"revision walk setup failed\");\n-\tif (revs->tree_objects)\n+\tif (revs->tree_and_blob_objects)\n \t\tmark_edges_uninteresting(revs, NULL);\n }\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 0aa93d589..6c2651b31 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -345,7 +345,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\trevs.commit_format = CMIT_FMT_RAW;\n \n \tif ((!revs.commits &&\n-\t     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n+\t     (!(revs.tag_objects || revs.tree_and_blob_objects) &&\n \t      !revs.pending.nr)) ||\n \t    revs.diff)\n \t\tusage(rev_list_usage);\n@@ -374,7 +374,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t} else if (revs.max_count < 0 &&\n-\t\t\t   revs.tag_objects && revs.tree_objects && revs.blob_objects) {\n+\t\t\t   revs.tag_objects && revs.tree_and_blob_objects) {\n \t\t\tif (!prepare_bitmap_walk(&revs)) {\n \t\t\t\ttraverse_bitmap_commit_list(&show_object_fast);\n \t\t\t\treturn 0;\n@@ -384,7 +384,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n-\tif (revs.tree_objects)\n+\tif (revs.tree_and_blob_objects)\n \t\tmark_edges_uninteresting(&revs, show_edge);\n \n \tif (bisect_list) {\ndiff --git a/list-objects.c b/list-objects.c\nindex f3ca6aafb..796957105 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -18,7 +18,7 @@ static void process_blob(struct rev_info *revs,\n \tstruct object *obj = &blob->object;\n \tsize_t pathlen;\n \n-\tif (!revs->blob_objects)\n+\tif (!revs->tree_and_blob_objects)\n \t\treturn;\n \tif (!obj)\n \t\tdie(\"bad blob object\");\n@@ -78,7 +78,7 @@ static void process_tree(struct rev_info *revs,\n \t\tall_entries_interesting: entry_not_interesting;\n \tint baselen = base->len;\n \n-\tif (!revs->tree_objects)\n+\tif (!revs->tree_and_blob_objects)\n \t\treturn;\n \tif (!obj)\n \t\tdie(\"bad tree object\");\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 970559601..5080e276b 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -258,8 +258,7 @@ void bitmap_writer_build(struct packing_data *to_pack)\n \n \tinit_revisions(&revs, NULL);\n \trevs.tag_objects = 1;\n-\trevs.tree_objects = 1;\n-\trevs.blob_objects = 1;\n+\trevs.tree_and_blob_objects = 1;\n \trevs.no_walk = 0;\n \n \trevs.include_check = should_include;\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 39bcc1684..445a24e0d 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -951,8 +951,7 @@ void test_bitmap_walk(struct rev_info *revs)\n \t\tdie(\"Commit %s doesn't have an indexed bitmap\", oid_to_hex(&root->oid));\n \n \trevs->tag_objects = 1;\n-\trevs->tree_objects = 1;\n-\trevs->blob_objects = 1;\n+\trevs->tree_and_blob_objects = 1;\n \n \tresult_popcnt = bitmap_popcount(result);\n \ndiff --git a/reachable.c b/reachable.c\nindex d0199cace..9a3beb75f 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -166,8 +166,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t * in all object types, not just commits.\n \t */\n \trevs->tag_objects = 1;\n-\trevs->blob_objects = 1;\n-\trevs->tree_objects = 1;\n+\trevs->tree_and_blob_objects = 1;\n \n \t/* Add all refs from the index file */\n \tadd_index_objects_to_pending(revs, 0);\ndiff --git a/revision.c b/revision.c\nindex b37dbec37..5e49d9e0e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -267,7 +267,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t */\n \tif (object->type == OBJ_TREE) {\n \t\tstruct tree *tree = (struct tree *)object;\n-\t\tif (!revs->tree_objects)\n+\t\tif (!revs->tree_and_blob_objects)\n \t\t\treturn NULL;\n \t\tif (flags & UNINTERESTING) {\n \t\t\tmark_tree_contents_uninteresting(tree);\n@@ -281,7 +281,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t * Blob object? You know the drill by now..\n \t */\n \tif (object->type == OBJ_BLOB) {\n-\t\tif (!revs->blob_objects)\n+\t\tif (!revs->tree_and_blob_objects)\n \t\t\treturn NULL;\n \t\tif (flags & UNINTERESTING)\n \t\t\treturn NULL;\n@@ -1817,23 +1817,19 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--objects\")) {\n \t\trevs->tag_objects = 1;\n-\t\trevs->tree_objects = 1;\n-\t\trevs->blob_objects = 1;\n+\t\trevs->tree_and_blob_objects = 1;\n \t} else if (!strcmp(arg, \"--objects-edge\")) {\n \t\trevs->tag_objects = 1;\n-\t\trevs->tree_objects = 1;\n-\t\trevs->blob_objects = 1;\n+\t\trevs->tree_and_blob_objects = 1;\n \t\trevs->edge_hint = 1;\n \t} else if (!strcmp(arg, \"--objects-edge-aggressive\")) {\n \t\trevs->tag_objects = 1;\n-\t\trevs->tree_objects = 1;\n-\t\trevs->blob_objects = 1;\n+\t\trevs->tree_and_blob_objects = 1;\n \t\trevs->edge_hint = 1;\n \t\trevs->edge_hint_aggressive = 1;\n \t} else if (!strcmp(arg, \"--verify-objects\")) {\n \t\trevs->tag_objects = 1;\n-\t\trevs->tree_objects = 1;\n-\t\trevs->blob_objects = 1;\n+\t\trevs->tree_and_blob_objects = 1;\n \t\trevs->verify_objects = 1;\n \t} else if (!strcmp(arg, \"--unpacked\")) {\n \t\trevs->unpacked = 1;\ndiff --git a/revision.h b/revision.h\nindex 9fac1a607..43cce137f 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -89,8 +89,7 @@ struct rev_info {\n \t\t\tsimplify_merges:1,\n \t\t\tsimplify_by_decoration:1,\n \t\t\ttag_objects:1,\n-\t\t\ttree_objects:1,\n-\t\t\tblob_objects:1,\n+\t\t\ttree_and_blob_objects:1,\n \t\t\tverify_objects:1,\n \t\t\tedge_hint:1,\n \t\t\tedge_hint_aggressive:1,\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312602","messageId":"0d36e20e3c35c5c60fcf451c8f810fcbfbdd004c.1487984670.git.jonathantanmy@google.com","threadId":"45218","inReplyTo":"cover.1487984670.git.jonathantanmy@google.com","subject":"[PATCH 3/3] upload-pack: compute blob reachability correctly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-25T01:18:38Z","receivedAt":"2017-02-25T01:19:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"If allowreachablesha1inwant is set, upload-pack will provide a blob to a\nuser, provided its hash, regardless of whether the blob is reachable or\nnot. Teach upload-pack to compute reachability correctly by passing the\n\"--objects\" argument when it invokes \"rev-list\" if necessary.\n\nThis commit only affects the case where blob/tree hashes are provided to\nupload-pack; the more typical case of only commit/tag hashes being\nprovided is not affected. In the case where blob/tree hashes are\nprovided, the reachability check is now slower (since trees need to be\nread) but correct. (The user may still set allowanysha1inwant instead of\nallowreachablesha1inwant to opt-out of the reachability check.)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++\n upload-pack.c         | 15 +++++++++++++++\n 2 files changed, 45 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 505e1b4a7..a4ae888ff 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,36 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'setup for tests that fetch blobs by hash' '\n+\tgit init blobserver &&\n+\ttest_commit -C blobserver 1 &&\n+\ttest_commit -C blobserver 2 &&\n+\ttest_commit -C blobserver 3 &&\n+\tblob1=$(echo 1 | git hash-object --stdin) &&\n+\tblob2=$(echo 2 | git hash-object --stdin) &&\n+\tblob3=$(echo 3 | git hash-object --stdin) &&\n+\n+\tunreachable=$(echo 4 | git -C blobserver hash-object -w --stdin) &&\n+\tgit -C blobserver cat-file -e \"$unreachable\"\n+'\n+\n+test_expect_success 'fetch-pack can fetch reachable blobs by hash' '\n+\ttest_config -C blobserver uploadpack.allowreachablesha1inwant 1 &&\n+\n+\tgit init reachabletest &&\n+\tgit -C reachabletest fetch-pack ../blobserver \"$blob1\" \"$blob2\" &&\n+\tgit -C reachabletest cat-file -e \"$blob1\" &&\n+\tgit -C reachabletest cat-file -e \"$blob2\" &&\n+\ttest_must_fail git -C reachabletest cat-file -e \"$blob3\"\n+'\n+\n+test_expect_success 'fetch-pack cannot fetch unreachable blobs' '\n+\ttest_config -C blobserver uploadpack.allowreachablesha1inwant 1 &&\n+\n+\tgit init unreachabletest &&\n+\ttest_must_fail git -C unreachabletest fetch-pack ../blobserver \"$blob1\" \"$unreachable\"\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7597ba340..f05cc2b5e 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -471,6 +471,9 @@ static int do_reachable_revlist(struct child_process *cmd,\n \tstatic const char *argv[] = {\n \t\t\"rev-list\", \"--stdin\", NULL,\n \t};\n+\tstatic const char *argv_with_objects[] = {\n+\t\t\"rev-list\", \"--objects\", \"--stdin\", NULL,\n+\t};\n \tstruct object *o;\n \tchar namebuf[42]; /* ^ + SHA-1 + LF */\n \tint i;\n@@ -488,6 +491,18 @@ static int do_reachable_revlist(struct child_process *cmd,\n \t */\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n+\t/*\n+\t * If we are testing reachability of a tree or blob, rev-list needs to\n+\t * operate more granularly.\n+\t */\n+\tfor (i = 0; i < src->nr; i++) {\n+\t\to = src->objects[i].item;\n+\t\tif (o->type == OBJ_TREE || o->type == OBJ_BLOB) {\n+\t\t\tcmd->argv = argv_with_objects;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tif (start_command(cmd))\n \t\tgoto error;\n \n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312907","messageId":"xmqq1suij8kr.fsf@gitster.mtv.corp.google.com","threadId":"45218","inReplyTo":"06a84f8c77924b275606384ead8bb2fd7d75f7b6.1487984670.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T21:42:44Z","receivedAt":"2017-02-28T21:42:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> It could be argued that in the future, Git might need to distinguish\n> tree_objects from blob_objects - in particular, a user might want\n> rev-list to print the trees but not the blobs. \n\nThat was exactly why these bits were originally made to \"appear\nindependent but in practice nobody sets only one and leaves others\noff\".  \n\nAnd it didn't happen in the past 10 years, which tells us that we\nshould take this patch.\n\nThanks.\n\n"},{"id":"312908","messageId":"xmqqwpcahtxt.fsf@gitster.mtv.corp.google.com","threadId":"45218","inReplyTo":"7082d91f30663b2e6d7fb1795c5ea37d3fe3446c.1487984670.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/3] revision: exclude trees/blobs given commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T21:44:14Z","receivedAt":"2017-02-28T21:46:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When the --objects argument is given to rev-list, an argument of the\n> form \"^$tree\" can be given to exclude all blobs and trees reachable from\n> that tree, but an argument of the form \"^$commit\" only excludes that\n> commit, not any blob or tree reachable from it. Make \"^$commit\" behave\n> consistent to \"^$tree\".\n>\n> Also, formalize this behavior in unit tests. (Some of the added tests\n> would already pass even before this commit, but are included\n> nevertheless for completeness.)\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  revision.c               |  2 ++\n>  t/t6000-rev-list-misc.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 90 insertions(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 5e49d9e0e..e6a62da98 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -254,6 +254,8 @@ static struct commit *handle_commit(struct rev_info *revs,\n>  \t\t\tdie(\"unable to parse commit %s\", name);\n>  \t\tif (flags & UNINTERESTING) {\n>  \t\t\tmark_parents_uninteresting(commit);\n> +\t\t\tif (revs->tree_and_blob_objects)\n> +\t\t\t\tmark_tree_uninteresting(commit->tree);\n\nI fear that this may end up to be quite expensive.  Can we have a\nperf test?\n"},{"id":"312913","messageId":"20170228215937.yd4juycjf7y3vish@sigill.intra.peff.net","threadId":"45218","inReplyTo":"xmqq1suij8kr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T21:59:37Z","receivedAt":"2017-02-28T22:00:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2017 at 01:42:44PM -0800, Junio C Hamano wrote:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > It could be argued that in the future, Git might need to distinguish\n> > tree_objects from blob_objects - in particular, a user might want\n> > rev-list to print the trees but not the blobs. \n> \n> That was exactly why these bits were originally made to \"appear\n> independent but in practice nobody sets only one and leaves others\n> off\".  \n> \n> And it didn't happen in the past 10 years, which tells us that we\n> should take this patch.\n\nI actually have a patch which uses the distinction. It's for\nupload-archive doing reachability checks (which seems rather familiar to\nwhat's going on here).\n\nThe whole series (from 2013!) is at:\n\n  git://github.com/peff/git jk/archive-reachability\n\nbut the relevant commits are below.\n\nI don't think the same logic holds for this case, though, because\nsomebody actually can ask for a single blob.\n\n-- >8 --\nFrom: Jeff King <peff@peff.net>\nDate: Wed, 5 Jun 2013 17:57:02 -0400\nSubject: [PATCH] list-objects: optimize \"revs->blob_objects = 0\" case\n\nIf we are traversing trees during a \"--objects\"\ntraversal, we may skip blobs if the \"blob_objects\" field of\nrev_info is not set. But we do so as the first thing in\nprocess_blob(), only after we have actually created the\n\"struct blob\" object, incurring a hash lookup. We can\noptimize out this no-op call completely.\n\nThis does not actually affect any current code, as all of\nthe current traversals always set blob_objects when looking\nat objects, anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex f3ca6aafb..58ad69557 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -117,7 +117,7 @@ static void process_tree(struct rev_info *revs,\n \t\t\tprocess_gitlink(revs, entry.oid->hash,\n \t\t\t\t\tshow, base, entry.path,\n \t\t\t\t\tcb_data);\n-\t\telse\n+\t\telse if (revs->blob_objects)\n \t\t\tprocess_blob(revs,\n \t\t\t\t     lookup_blob(entry.oid->hash),\n \t\t\t\t     show, base, entry.path,\n-- \n2.12.0.359.gd4c8c42e9\n\n-- >8 --\nFrom: Jeff King <peff@peff.net>\nDate: Wed, 5 Jun 2013 18:02:42 -0400\nSubject: [PATCH] archive: ignore blob objects when checking reachability\n\nWe cannot create an archive from a blob object, so we would\nnot expect anyone to provide one to us. And if they do, we\nwill fail anyway just after the reachability check.  We can\ntherefore optimize our reachability check to ignore blobs\ncompletely, and not even create a \"struct blob\" for them.\n\nDepending on the repository size and the exact place we find\nthe reachable object in the traversal, this can save 20-25%,\na we can avoid many lookups in the object hash.\n\nThe downside of this is that a blob provided to a remote\narchive process will fail with \"no such object\" rather than\n\"object is not a tree\" (we could organize the code to retain\nthe old message, but since we no longer know whether the\nblob is reachable or not, we would potentially be leaking\ninformation about the existence of unreachable objects).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n archive.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/archive.c b/archive.c\nindex ef89b2556..489115f9f 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -383,6 +383,7 @@ static int object_is_reachable(const unsigned char *sha1)\n \tsave_commit_buffer = 0;\n \tinit_revisions(&data.revs, NULL);\n \tsetup_revisions(ARRAY_SIZE(argv) - 1, argv, &data.revs, NULL);\n+\tdata.revs.blob_objects = 0;\n \tif (prepare_revision_walk(&data.revs))\n \t\treturn 0;\n \n-- \n2.12.0.359.gd4c8c42e9\n\n"},{"id":"312917","messageId":"20170228220626.at4cihedmvkqiq5c@sigill.intra.peff.net","threadId":"45218","inReplyTo":"06a84f8c77924b275606384ead8bb2fd7d75f7b6.1487984670.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T22:06:26Z","receivedAt":"2017-02-28T22:33:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2017 at 05:18:36PM -0800, Jonathan Tan wrote:\n\n> Whenever tree_objects is set to 1 in revision.h's struct rev_info,\n> blob_objects is likewise set, and vice versa. Combine those two fields\n> into one.\n> \n> Some of the existing code does not handle tree_objects being different\n> from blob_objects properly. For example, \"handle_commit\" in revision.c\n> recurses from an UNINTERESTING tree into its subtree if tree_objects ==\n> 1, completely ignoring blob_objects; it probably should still recurse if\n> tree_objects == 0 and blob_objects == 1 (to mark the blobs), and should\n> behave differently depending on blob_objects (controlling the\n> instantiation and marking of blob objects). This commit resolves the\n> issue by forbidding tree_objects from being different to blob_objects.\n\nYeah, I agree that is awkward. I'm OK with the rule \"if blob_objects is\nset, then tree_objects must also be set\". It's the other way around I\ncare more about.\n\n> It could be argued that in the future, Git might need to distinguish\n> tree_objects from blob_objects - in particular, a user might want\n> rev-list to print the trees but not the blobs. However, this results in\n> a minor performance savings at best in that objects no longer need to be\n> instantiated (causing memory allocations and hashtable insertions) - no\n> disk reads are being done for objects whether blob_objects is set or\n> not.\n\nIn a full object-graph traversal, we actually spend a big chunk of our\ntime in hash lookups. My measurements (admittedly from 2013, which I\nhaven't repeated lately) show something like a 20-25% speedup for this\ncase.\n\nMy only use for it (and the source of those timings) was to compute\narchive reachability, which nobody seems to care too much about. But I\nsuspect we could speed up your case, too, when we are just computing the\nreachability of a non-blob. I.e., you should be able to turn on the\nsmallest subset of \"commits only\", \"commits and trees\", and \"commits,\ntrees, and blobs\", based on what the other side has asked for.\n\n-Peff\n"},{"id":"312921","messageId":"20170228221236.selqkf5wme3fvued@sigill.intra.peff.net","threadId":"45218","inReplyTo":"7082d91f30663b2e6d7fb1795c5ea37d3fe3446c.1487984670.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/3] revision: exclude trees/blobs given commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T22:12:37Z","receivedAt":"2017-02-28T22:36:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2017 at 05:18:37PM -0800, Jonathan Tan wrote:\n\n> When the --objects argument is given to rev-list, an argument of the\n> form \"^$tree\" can be given to exclude all blobs and trees reachable from\n> that tree, but an argument of the form \"^$commit\" only excludes that\n> commit, not any blob or tree reachable from it. Make \"^$commit\" behave\n> consistent to \"^$tree\".\n\nLike Junio, I suspect this is going to be quite expensive. This is\nsimilar to the \"--objects-edge\" and \"--objects-edge-aggressive\" options,\nwhich we had to pull back on the use of because of their expensiveness.\n\n(And as an aside, wouldn't those options be the right place for what\nyou're doing?).\n\nI also think that the mechanism here is not 100% accurate. The commit\ntraversal will stop once it has painted down, so you're effectively\nexploring the trees of the merge bases. But older history could mention\nan object that has resurfaced again (e.g., due to a cherry-pick).\n\nGetting the 100% accurate answer is _really_ expensive, though with\nreachability bitmaps it's not too bad. I just wonder if that's a better\napproach to be taking.\n\n-Peff\n"},{"id":"312924","messageId":"xmqqa895j4er.fsf@gitster.mtv.corp.google.com","threadId":"45218","inReplyTo":"7082d91f30663b2e6d7fb1795c5ea37d3fe3446c.1487984670.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/3] revision: exclude trees/blobs given commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T23:12:44Z","receivedAt":"2017-02-28T23:14:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When the --objects argument is given to rev-list, an argument of the\n> form \"^$tree\" can be given to exclude all blobs and trees reachable from\n> that tree, but an argument of the form \"^$commit\" only excludes that\n> commit, not any blob or tree reachable from it. Make \"^$commit\" behave\n> consistent to \"^$tree\".\n\nSo with this:\n\n    $ git rev-list --objects ^HEAD^@ HEAD ^HEAD^{tree}\n\nshould be a round-about way to say\n\n    $ git rev-parse HEAD\n\n;-)\n\nThe expression wants to list everything reachable from HEAD, but it\ndoes not want to show its parents (i.e. ^HEAD^@) and it does not\nwant to show its tree (i.e. ^HEAD^{tree}), so the only thing that\nremains is the commit object HEAD and nothing else?\n\nI agree with Peff's comment about objects that may appear beyond the\nboundary (i.e. merge base between interesting ones and uninteresting\nones); whether that inaccuracy matters depends on what you want to\nuse this for---if you want to hide sensitive objects it does, if you\nwant to reduce the network cost without incurring too much cpu cost,\nit probably does not.\n\n\n\n"},{"id":"313090","messageId":"xmqq7f47cyqa.fsf@gitster.mtv.corp.google.com","threadId":"45218","inReplyTo":"20170228215937.yd4juycjf7y3vish@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-02T18:36:29Z","receivedAt":"2017-03-02T19:19:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Feb 28, 2017 at 01:42:44PM -0800, Junio C Hamano wrote:\n>\n>> Jonathan Tan <jonathantanmy@google.com> writes:\n>> \n>> > It could be argued that in the future, Git might need to distinguish\n>> > tree_objects from blob_objects - in particular, a user might want\n>> > rev-list to print the trees but not the blobs. \n>> \n>> That was exactly why these bits were originally made to \"appear\n>> independent but in practice nobody sets only one and leaves others\n>> off\".  \n>> \n>> And it didn't happen in the past 10 years, which tells us that we\n>> should take this patch.\n>\n> I actually have a patch which uses the distinction. It's for\n> upload-archive doing reachability checks (which seems rather familiar to\n> what's going on here).\n\nOK.  Thanks for stopping me ;-)\n"},{"id":"313136","messageId":"20170303064512.khs2seru5onl54mh@sigill.intra.peff.net","threadId":"45218","inReplyTo":"20170302195041.1699-1-jonathantanmy@google.com","subject":"Re: [PATCH] t/perf: export variable used in other blocks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-03T06:45:12Z","receivedAt":"2017-03-03T06:53:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 02, 2017 at 11:50:41AM -0800, Jonathan Tan wrote:\n\n> In p0001, a variable was created in a test_expect_success block to be\n> used in later test_perf blocks, but was not exported. This caused the\n> variable to not appear in those blocks (this can be verified by writing\n> 'test -n \"$commit\"' in those blocks), resulting in a slightly different\n> invocation than what was intended. Export that variable.\n\nThanks, this is obviously the right thing to do, and the mistake is mine\nfrom ea97002fc (t/perf: time rev-list with UNINTERESTING commits,\n2014-01-20). This is not the first time I've been confused by missing\nvariables in t/perf scripts, since it behaves differently than the\nnormal test suite. I wonder if we should turn on \"set -a\" during t/perf\nsetup snippets. That's a bit of a blunt tool, but I suspect it would\njust be easier to work with.\n\nI was curious that the tests added by ea97002fc showed something useful\neven with the bug you're fixing here. But it's because the actual slice\nof history we meant to traverse isn't important. It's intentionally\ntiny to show off the time spent dealing with the UNINTERESTING commits.\nSo in effect we were traversing no commits instead of a tiny set, but\nthe timing results were the same.\n\nI repeated the tests over fbd4a703 given in the commit message of\nee9a7002fc and confirmed that it behaves the same with your fixed\nversion of the test. I did have to tweak a few other things to get the\ntest to run against such an old version of git, though. I'll follow-up\nwith a patch.\n\n-Peff\n"},{"id":"313138","messageId":"20170302195041.1699-1-jonathantanmy@google.com","threadId":"45218","inReplyTo":"20170228221236.selqkf5wme3fvued@sigill.intra.peff.net","subject":"[PATCH] t/perf: export variable used in other blocks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-03-02T19:50:41Z","receivedAt":"2017-03-03T06:58:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In p0001, a variable was created in a test_expect_success block to be\nused in later test_perf blocks, but was not exported. This caused the\nvariable to not appear in those blocks (this can be verified by writing\n'test -n \"$commit\"' in those blocks), resulting in a slightly different\ninvocation than what was intended. Export that variable.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n\nPerformance indeed does drop significantly with this patch. And I cannot\nsay \"at least this is more correct\", because it isn't - as Peff thinks,\nit is indeed still not 100% accurate because of the resurfacing-object\nissue he mentions (I've verified this by constructing a test that fails\neven after this patch set).\n\nAlso, since we don't want to unify tree_ and blob_ (for the reason Peff\nmentioned in another e-mail), feel free to drop this patch set.\n\nHaving said that, while looking at the perf test, I noticed an issue\nwith a non-exported variable, which is corrected in this patch.\n\n t/perf/p0001-rev-list.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/p0001-rev-list.sh b/t/perf/p0001-rev-list.sh\nindex 16359d51a..ebf172401 100755\n--- a/t/perf/p0001-rev-list.sh\n+++ b/t/perf/p0001-rev-list.sh\n@@ -15,7 +15,8 @@ test_perf 'rev-list --all --objects' '\n '\n \n test_expect_success 'create new unreferenced commit' '\n-\tcommit=$(git commit-tree HEAD^{tree} -p HEAD)\n+\tcommit=$(git commit-tree HEAD^{tree} -p HEAD) &&\n+\ttest_export commit\n '\n \n test_perf 'rev-list $commit --not --all' '\n-- \n2.12.0.rc1.440.g5b76565f74-goog\n\n"},{"id":"313140","messageId":"20170303071403.2k3vbcdstllq2j3y@sigill.intra.peff.net","threadId":"45218","inReplyTo":"20170303064512.khs2seru5onl54mh@sigill.intra.peff.net","subject":"[PATCH] t/perf: use $MODERN_GIT for all repo-copying steps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-03T07:14:03Z","receivedAt":"2017-03-03T07:29:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2017 at 01:45:12AM -0500, Jeff King wrote:\n\n> I repeated the tests over fbd4a703 given in the commit message of\n> ee9a7002fc and confirmed that it behaves the same with your fixed\n> version of the test. I did have to tweak a few other things to get the\n> test to run against such an old version of git, though. I'll follow-up\n> with a patch.\n\nHere's that patch.\n\n-- >8 --\nSubject: [PATCH] t/perf: use $MODERN_GIT for all repo-copying steps\n\nSince 1a0962dee (t/perf: fix regression in testing older\nversions of git, 2016-06-22), we point \"$MODERN_GIT\" to a\ncopy of git that matches the t/perf script itself, and which\ncan be used for tasks outside of the actual timings. This is\nneeded because the setup done by perf scripts keeps moving\nforward in time, and may use features that the older\nversions of git we are testing do not have.\n\nThat commit used $MODERN_GIT to fix a case where we relied\non the relatively recent --git-path option. But if you go\nback further still, there are more problems.\n\nSince 7501b5921 (perf: make the tests work in worktrees,\n2016-05-13), we use \"git -C\", but versions of git older than\n44e1e4d67 (git: run in a directory given with -C option,\n2013-09-09) don't know about \"-C\". So testing an old version\nof git with a new version of t/perf will fail the setup\nstep.\n\nWe can fix this by using $MODERN_GIT during the setup;\nthere's no need to use the antique version, since it doesn't\naffect the timings. Likewise, we'll adjust the \"init\"\ninvocation; antique versions of git called this \"init-db\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWith this patch I was able to run p0001 against v1.7.0. I don't think we\ncan go further back than that because the perf library depends on the\npresence of bin-wrappers. That's probably enough. Unlike the t/interop\nlibrary I proposed recently it's not that interesting to go really far\nback in time (and I did hack around the bin-wrappers thing in t/interop;\nyou really can test against v1.0.0 there).\n\n t/perf/perf-lib.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 46f08ee08..ab4b8b06a 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -83,7 +83,7 @@ test_perf_create_repo_from () {\n \terror \"bug in the test script: not 2 parameters to test-create-repo\"\n \trepo=\"$1\"\n \tsource=\"$2\"\n-\tsource_git=\"$(git -C \"$source\" rev-parse --git-dir)\"\n+\tsource_git=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-dir)\"\n \tobjects_dir=\"$(\"$MODERN_GIT\" -C \"$source\" rev-parse --git-path objects)\"\n \tmkdir -p \"$repo/.git\"\n \t(\n@@ -102,7 +102,7 @@ test_perf_create_repo_from () {\n \t) &&\n \t(\n \t\tcd \"$repo\" &&\n-\t\tgit init -q && {\n+\t\t\"$MODERN_GIT\" init -q && {\n \t\t\ttest_have_prereq SYMLINKS ||\n \t\t\tgit config core.symlinks false\n \t\t} &&\n-- \n2.12.0.385.gdf4947bc7\n\n"},{"id":"313144","messageId":"20170303073633.maoaovchkz32h467@sigill.intra.peff.net","threadId":"45218","inReplyTo":"20170303071403.2k3vbcdstllq2j3y@sigill.intra.peff.net","subject":"[PATCH] t/perf: add fallback for pre-bin-wrappers versions of git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-03T07:36:33Z","receivedAt":"2017-03-03T09:17:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2017 at 02:14:03AM -0500, Jeff King wrote:\n\n> With this patch I was able to run p0001 against v1.7.0. I don't think we\n> can go further back than that because the perf library depends on the\n> presence of bin-wrappers. That's probably enough. Unlike the t/interop\n> library I proposed recently it's not that interesting to go really far\n> back in time (and I did hack around the bin-wrappers thing in t/interop;\n> you really can test against v1.0.0 there).\n\nThis is easy to fix (see below). I doubt anybody cares, but it's\nprobably worth fixing just because the failure mode (quietly running\nwhatever git is in your PATH) is so confusing. It would also be an\nimprovement to just detect the situation and die(), but this is\nliterally not any more effort.\n\n-- >8 --\nSubject: [PATCH] t/perf: add fallback for pre-bin-wrappers versions of git\n\nIt's tempting to say:\n\n  ./run v1.0.0 HEAD\n\nto see how we've sped up Git over the years. Unfortunately,\nthis doesn't quite work because versions of Git prior to\nv1.7.0 lack bin-wrappers, so our \"run\" script doesn't\ncorrectly put them in the PATH.\n\nWorse, it means we silently find whatever other \"git\" is in\nthe PATH, and produce test results that have no bearing on\nwhat we asked for.\n\nLet's fallback to the main git directory when bin-wrappers\nisn't present. Many modern perf scripts won't run with such\nan antique version of Git, of course, but at least those\nfailures are detected and reported (and you're free to write\na limited perf script that works across many versions).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/perf/run | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/perf/run b/t/perf/run\nindex e8adedadf..c788d713a 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -63,6 +63,9 @@ run_dirs_helper () {\n \t\tunset GIT_TEST_INSTALLED\n \telse\n \t\tGIT_TEST_INSTALLED=\"$mydir/bin-wrappers\"\n+\t\t# Older versions of git lacked bin-wrappers; fallback to the\n+\t\t# files in the root.\n+\t\ttest -d \"$GIT_TEST_INSTALLED\" || GIT_TEST_INSTALLED=$mydir\n \t\texport GIT_TEST_INSTALLED\n \tfi\n \trun_one_dir \"$@\"\n-- \n2.12.0.385.gdf4947bc7\n\n"},{"id":"313217","messageId":"xmqqbmti9os2.fsf@gitster.mtv.corp.google.com","threadId":"45218","inReplyTo":"20170303064512.khs2seru5onl54mh@sigill.intra.peff.net","subject":"Re: [PATCH] t/perf: export variable used in other blocks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-03T18:51:57Z","receivedAt":"2017-03-03T18:52:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Thanks, this is obviously the right thing to do, and the mistake is mine\n> from ea97002fc (t/perf: time rev-list with UNINTERESTING commits,\n> 2014-01-20). This is not the first time I've been confused by missing\n> variables in t/perf scripts, since it behaves differently than the\n> normal test suite. I wonder if we should turn on \"set -a\" during t/perf\n> setup snippets. That's a bit of a blunt tool, but I suspect it would\n> just be easier to work with.\n\nI wonder if we can make t/perf to behave more similar to the normal\ntest suite to eliminate the need for this exporting, by the way.\nt/perf/README does not say anything more than \"for lack of better\noptions\" throughout its history, which does not help very much.\n"},{"id":"313244","messageId":"20170303223127.vqqkmxmcclkmw6wm@sigill.intra.peff.net","threadId":"45218","inReplyTo":"xmqqbmti9os2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t/perf: export variable used in other blocks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-03T22:31:27Z","receivedAt":"2017-03-03T22:31:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2017 at 10:51:57AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Thanks, this is obviously the right thing to do, and the mistake is mine\n> > from ea97002fc (t/perf: time rev-list with UNINTERESTING commits,\n> > 2014-01-20). This is not the first time I've been confused by missing\n> > variables in t/perf scripts, since it behaves differently than the\n> > normal test suite. I wonder if we should turn on \"set -a\" during t/perf\n> > setup snippets. That's a bit of a blunt tool, but I suspect it would\n> > just be easier to work with.\n> \n> I wonder if we can make t/perf to behave more similar to the normal\n> test suite to eliminate the need for this exporting, by the way.\n> t/perf/README does not say anything more than \"for lack of better\n> options\" throughout its history, which does not help very much.\n\nThe problem is that it has to run each block multiple times under an\nexternal \"time\" program. If you assume that the shell running the perf\nsuite is bash, you can do:\n\n  i=0\n  foo() { echo $i; i=$(($i+1)); }\n  for run in 1 2 3; do time foo; done\n\nwhich shows that you can get repeated timings from a shell function.\nThat shell function would run the actual test snippet, and would have to\ndo some redirect trickery to split the \"time\" results from the stderr of\nthe underlying test. And parse bash's time output.\n\nBut those are all do-able things. The main thing is throwing out a\ndependency on an external \"time\" for \"bash\". That may not be a big loss,\nthough.\n\n-Peff\n"}]}