{"thread":{"id":"47404","subject":"[PATCH 0/1] diffcore-blobfind","startedAt":"2017-12-08T00:25:22Z","lastAt":"2017-12-27T19:57:36Z","messageCount":25,"participants":["Stefan Beller","Jeff King","Junio C Hamano","Ramsay Jones","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"334394","messageId":"20171208002447.20261-1-sbeller@google.com","threadId":"47404","inReplyTo":null,"subject":"[PATCH 0/1] diffcore-blobfind","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-08T00:24:46Z","receivedAt":"2017-12-08T00:25:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This includes the suggestions by Junio,\n\nThanks,\nStefan\n\ninterdiff to currently queued below.\n\nStefan Beller (1):\n diffcore: add a filter to find a specific blob\n\n Documentation/diff-options.txt | 5 +++++\n Makefile | 1 +\n builtin/log.c | 2 +-\n diff.c | 20 +++++++++++++++++++-\n diff.h | 3 +++\n diffcore-blobfind.c | 41 +++++++++++++++++++++++++++++++++++++++++\n diffcore.h | 1 +\n revision.c | 5 ++++-\n t/t4064-diff-blobfind.sh | 35 +++++++++++++++++++++++++++++++++++\n 9 files changed, 110 insertions(+), 3 deletions(-)\n create mode 100644 diffcore-blobfind.c\n create mode 100755 t/t4064-diff-blobfind.sh\n\n-- \n2.15.1.424.g9478a66081-goog\n\ndiff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt\nindex 252a21cc19..34d53b95f1 100644\n--- c/Documentation/diff-options.txt\n+++ w/Documentation/diff-options.txt\n@@ -500,6 +500,7 @@ information.\n --pickaxe-regex::\n \tTreat the given to `-S` as an extended POSIX regular\n \texpression to match.\n+\n --blobfind=::\n \tRestrict the output such that one side of the diff\n \tmatches the given blob-id.\ndiff --git c/diffcore-blobfind.c w/diffcore-blobfind.c\nindex 5d222fc336..e65c7cad6e 100644\n--- c/diffcore-blobfind.c\n+++ w/diffcore-blobfind.c\n@@ -8,40 +8,30 @@\n static void diffcore_filter_blobs(struct diff_queue_struct *q,\n \t\t\t\t struct diff_options *options)\n {\n-\tint i, j = 0, c = q->nr;\n+\tint src, dst;\n \n \tif (!options->blobfind)\n \t\tBUG(\"blobfind oidset not initialized???\");\n \n-\tfor (i = 0; i < q->nr; i++) {\n-\t\tstruct diff_filepair *p = q->queue[i];\n+\tfor (src = dst = 0; src < q->nr; src++) {\n+\t\tstruct diff_filepair *p = q->queue[src];\n \n-\t\tif (DIFF_PAIR_UNMERGED(p) ||\n-\t\t (DIFF_FILE_VALID(p->one) &&\n+\t\tif ((DIFF_FILE_VALID(p->one) &&\n \t\t oidset_contains(options->blobfind, &p->one->oid)) ||\n \t\t (DIFF_FILE_VALID(p->two) &&\n-\t\t oidset_contains(options->blobfind, &p->two->oid)))\n-\t\t\tcontinue;\n-\n-\t\tdiff_free_filepair(p);\n-\t\tq->queue[i] = NULL;\n-\t\tc--;\n-\t}\n-\n-\t/* Keep it sorted. */\n-\ti = 0; j = 0;\n-\twhile (i < c) {\n-\t\twhile (!q->queue[j])\n-\t\t\tj++;\n-\t\tq->queue[i] = q->queue[j];\n-\t\ti++; j++;\n+\t\t oidset_contains(options->blobfind, &p->two->oid))) {\n+\t\t\tq->queue[dst] = p;\n+\t\t\tdst++;\n+\t\t} else {\n+\t\t\tdiff_free_filepair(p);\n+\t\t}\n \t}\n \n-\tq->nr = c;\n-\n-\tif (!c) {\n+\tif (!dst) {\n \t\tfree(q->queue);\n \t\tDIFF_QUEUE_CLEAR(q);\n+\t} else {\n+\t\tq->nr = dst;\n \t}\n }\n \ndiff --git c/revision.c w/revision.c\nindex 6449619c0a..8e1a89f832 100644\n--- c/revision.c\n+++ w/revision.c\n@@ -2884,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tsimplify_merges(revs);\n \tif (revs->children.name)\n \t\tset_children(revs);\n+\tif (revs->diffopt.blobfind)\n+\t\trevs->simplify_history = 0;\n \treturn 0;\n }\n \n"},{"id":"334395","messageId":"20171208002447.20261-2-sbeller@google.com","threadId":"47404","inReplyTo":"20171208002447.20261-1-sbeller@google.com","subject":"[PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-08T00:24:47Z","receivedAt":"2017-12-08T00:25:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Sometimes users are given a hash of an object and they want to\nidentify it further (ex.: Use verify-pack to find the largest blobs,\nbut what are these? or [1])\n\nOne might be tempted to extend git-describe to also work with blobs,\nsuch that `git describe ` gives a description as\n':'. This was implemented at [2]; as seen by the sheer\nnumber of responses (>110), it turns out this is tricky to get right.\nThe hard part to get right is picking the correct 'commit-ish' as that\ncould be the commit that (re-)introduced the blob or the blob that\nremoved the blob; the blob could exist in different branches.\n\nJunio hinted at a different approach of solving this problem, which this\npatch implements. Teach the diff machinery another flag for restricting\nthe information to what is shown. For example:\n\n $ ./git log --oneline --blobfind=v2.0.0:Makefile\n b2feb64309 Revert the whole \"ask curl-config\" topic for now\n 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n\nwe observe that the Makefile as shipped with 2.0 was introduced in\nv1.9.2-471-g47fbfded53 and replaced in v2.0.0-rc1-5-gb2feb64309 by\na different blob.\n\n[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob\n[2] https://public-inbox.org/git/20171028004419.10139-1-sbeller@google.com/\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 5 +++++\n Makefile | 1 +\n builtin/log.c | 2 +-\n diff.c | 20 +++++++++++++++++++-\n diff.h | 3 +++\n diffcore-blobfind.c | 41 +++++++++++++++++++++++++++++++++++++++++\n diffcore.h | 1 +\n revision.c | 5 ++++-\n t/t4064-diff-blobfind.sh | 35 +++++++++++++++++++++++++++++++++++\n 9 files changed, 110 insertions(+), 3 deletions(-)\n create mode 100644 diffcore-blobfind.c\n create mode 100755 t/t4064-diff-blobfind.sh\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex dd0dba5b1d..34d53b95f1 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -500,6 +500,11 @@ information.\n --pickaxe-regex::\n \tTreat the given to `-S` as an extended POSIX regular\n \texpression to match.\n+\n+--blobfind=::\n+\tRestrict the output such that one side of the diff\n+\tmatches the given blob-id.\n+\n endif::git-format-patch[]\n \n -O::\ndiff --git a/Makefile b/Makefile\nindex ee9d5eb11e..fdfa8f38f6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -775,6 +775,7 @@ LIB_OBJS += date.o\n LIB_OBJS += decorate.o\n LIB_OBJS += diffcore-break.o\n LIB_OBJS += diffcore-delta.o\n+LIB_OBJS += diffcore-blobfind.o\n LIB_OBJS += diffcore-order.o\n LIB_OBJS += diffcore-pickaxe.o\n LIB_OBJS += diffcore-rename.o\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6c1fa896ad..7b91f61423 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tinit_display_notes(&rev->notes_opt);\n \n \tif (rev->diffopt.pickaxe || rev->diffopt.filter ||\n-\t rev->diffopt.flags.follow_renames)\n+\t rev->diffopt.flags.follow_renames || rev->diffopt.blobfind)\n \t\trev->always_show_header = 0;\n \n \tif (source)\ndiff --git a/diff.c b/diff.c\nindex 0763e89263..8861f89ab1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)\n \toptions->interhunkcontext = diff_interhunk_context_default;\n \toptions->ws_error_highlight = ws_error_highlight_default;\n \toptions->flags.rename_empty = 1;\n+\toptions->blobfind = NULL;\n \n \t/* pathchange left =NULL by default */\n \toptions->change = diff_change;\n@@ -4487,6 +4488,19 @@ static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *ar\n \treturn 1;\n }\n \n+static int parse_blobfind_opt(struct diff_options *opt, const char *arg)\n+{\n+\tstruct object_id oid;\n+\n+\tif (get_oid_blob(arg, &oid) || sha1_object_info(oid.hash, NULL) != OBJ_BLOB)\n+\t\treturn error(\"object '%s' is not a blob\", arg);\n+\n+\tif (!opt->blobfind)\n+\t\topt->blobfind = xcalloc(1, sizeof(*opt->blobfind));\n+\toidset_insert(opt->blobfind, &oid);\n+\treturn 1;\n+}\n+\n int diff_opt_parse(struct diff_options *options,\n \t\t const char **av, int ac, const char *prefix)\n {\n@@ -4736,7 +4750,8 @@ int diff_opt_parse(struct diff_options *options,\n \telse if ((argcount = short_opt('O', av, &optarg))) {\n \t\toptions->orderfile = prefix_filename(prefix, optarg);\n \t\treturn argcount;\n-\t}\n+\t} else if (skip_prefix(arg, \"--blobfind=\", &arg))\n+\t\treturn parse_blobfind_opt(options, arg);\n \telse if ((argcount = parse_long_opt(\"diff-filter\", av, &optarg))) {\n \t\tint offending = parse_diff_filter_opt(optarg, options);\n \t\tif (offending)\n@@ -5770,6 +5785,9 @@ void diffcore_std(struct diff_options *options)\n \t\tdiffcore_skip_stat_unmatch(options);\n \tif (!options->found_follow) {\n \t\t/* See try_to_follow_renames() in tree-diff.c */\n+\n+\t\tif (options->blobfind)\n+\t\t\tdiffcore_blobfind(options);\n \t\tif (options->break_opt != -1)\n \t\t\tdiffcore_break(options->break_opt);\n \t\tif (options->detect_rename)\ndiff --git a/diff.h b/diff.h\nindex 0fb18dd735..9178e498fa 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -7,6 +7,7 @@\n #include \"tree-walk.h\"\n #include \"pathspec.h\"\n #include \"object.h\"\n+#include \"oidset.h\"\n \n struct rev_info;\n struct diff_options;\n@@ -174,6 +175,8 @@ struct diff_options {\n \tenum diff_words_type word_diff;\n \tenum diff_submodule_format submodule_format;\n \n+\tstruct oidset *blobfind;\n+\n \t/* this is set by diffcore for DIFF_FORMAT_PATCH */\n \tint found_changes;\n \ndiff --git a/diffcore-blobfind.c b/diffcore-blobfind.c\nnew file mode 100644\nindex 0000000000..e65c7cad6e\n--- /dev/null\n+++ b/diffcore-blobfind.c\n@@ -0,0 +1,41 @@\n+/*\n+ * Copyright (c) 2017 Google Inc.\n+ */\n+#include \"cache.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n+\n+static void diffcore_filter_blobs(struct diff_queue_struct *q,\n+\t\t\t\t struct diff_options *options)\n+{\n+\tint src, dst;\n+\n+\tif (!options->blobfind)\n+\t\tBUG(\"blobfind oidset not initialized???\");\n+\n+\tfor (src = dst = 0; src < q->nr; src++) {\n+\t\tstruct diff_filepair *p = q->queue[src];\n+\n+\t\tif ((DIFF_FILE_VALID(p->one) &&\n+\t\t oidset_contains(options->blobfind, &p->one->oid)) ||\n+\t\t (DIFF_FILE_VALID(p->two) &&\n+\t\t oidset_contains(options->blobfind, &p->two->oid))) {\n+\t\t\tq->queue[dst] = p;\n+\t\t\tdst++;\n+\t\t} else {\n+\t\t\tdiff_free_filepair(p);\n+\t\t}\n+\t}\n+\n+\tif (!dst) {\n+\t\tfree(q->queue);\n+\t\tDIFF_QUEUE_CLEAR(q);\n+\t} else {\n+\t\tq->nr = dst;\n+\t}\n+}\n+\n+void diffcore_blobfind(struct diff_options *options)\n+{\n+\tdiffcore_filter_blobs(&diff_queued_diff, options);\n+}\ndiff --git a/diffcore.h b/diffcore.h\nindex a30da161da..431917672f 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -107,6 +107,7 @@ extern struct diff_filepair *diff_queue(struct diff_queue_struct *,\n \t\t\t\t\tstruct diff_filespec *);\n extern void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n \n+extern void diffcore_blobfind(struct diff_options *);\n extern void diffcore_break(int);\n extern void diffcore_rename(struct diff_options *);\n extern void diffcore_merge_broken(void);\ndiff --git a/revision.c b/revision.c\nindex e2e691dd5a..8e1a89f832 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2409,7 +2409,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t/* Pickaxe, diff-filter and rename following need diffs */\n \tif (revs->diffopt.pickaxe ||\n \t revs->diffopt.filter ||\n-\t revs->diffopt.flags.follow_renames)\n+\t revs->diffopt.flags.follow_renames ||\n+\t revs->diffopt.blobfind)\n \t\trevs->diff = 1;\n \n \tif (revs->topo_order)\n@@ -2883,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tsimplify_merges(revs);\n \tif (revs->children.name)\n \t\tset_children(revs);\n+\tif (revs->diffopt.blobfind)\n+\t\trevs->simplify_history = 0;\n \treturn 0;\n }\n \ndiff --git a/t/t4064-diff-blobfind.sh b/t/t4064-diff-blobfind.sh\nnew file mode 100755\nindex 0000000000..b2c2964d77\n--- /dev/null\n+++ b/t/t4064-diff-blobfind.sh\n@@ -0,0 +1,35 @@\n+#!/bin/sh\n+\n+test_description='test finding specific blobs in the revision walking'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup ' '\n+\tgit commit --allow-empty -m \"empty initial commit\" &&\n+\n+\techo \"Hello, world!\" >greeting &&\n+\tgit add greeting &&\n+\tgit commit -m \"add the greeting blob\" && # borrowed from Git from the Bottom Up\n+\tgit tag -m \"the blob\" greeting $(git rev-parse HEAD:greeting) &&\n+\n+\techo asdf >unrelated &&\n+\tgit add unrelated &&\n+\tgit commit -m \"unrelated history\" &&\n+\n+\tgit revert HEAD^ &&\n+\n+\tgit commit --allow-empty -m \"another unrelated commit\"\n+'\n+\n+test_expect_success 'find the greeting blob' '\n+\tcat >expect <<-EOF &&\n+\tRevert \"add the greeting blob\"\n+\tadd the greeting blob\n+\tEOF\n+\n+\tgit log --abbrev=12 --oneline --blobfind=greeting^{blob} >actual.raw &&\n+\tcut -c 14- actual.raw >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334402","messageId":"20171208093434.GD26199@sigill.intra.peff.net","threadId":"47404","inReplyTo":"20171208002447.20261-2-sbeller@google.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T09:34:34Z","receivedAt":"2017-12-08T09:36:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 04:24:47PM -0800, Stefan Beller wrote:\n\n> Sometimes users are given a hash of an object and they want to\n> identify it further (ex.: Use verify-pack to find the largest blobs,\n> but what are these? or [1])\n> \n> One might be tempted to extend git-describe to also work with blobs,\n> such that `git describe ` gives a description as\n> ':'. This was implemented at [2]; as seen by the sheer\n> number of responses (>110), it turns out this is tricky to get right.\n> The hard part to get right is picking the correct 'commit-ish' as that\n> could be the commit that (re-)introduced the blob or the blob that\n> removed the blob; the blob could exist in different branches.\n\nNeat. I didn't follow the original thread very closely, but I think this\nis a good idea (and is definitely something I've simulated manually with\n\"git log --raw | grep\" before).\n\nBear with me while I pontificate for a moment.\n\nWe do something similar at GitHub to report on too-large objects\nduring a push (we identify the object during index-pack, but then want\nto hand back a human-readable pathname).\n\nWe do it with a patch to \"rev-list\", so that you can use the normal\ntraversal options to limit the set of visited commits. Which effectively\nmakes it \"traverse and find a place where this object is referenced, and\nthen tell me the path at which you found it (or tell me if it was not\nreferenced at all)\".\n\nThat sidesteps the \"describe\" problem, because now it is the user's\nresponsibility to tell you which commits to look in. :)\n\nBut the rev-list solution only finds _a_ commit that contains the\nobject, and which likely has nothing to do with the object at all. Your\nsolution here finds the introduction and removal of the object, which\nare interesting for a wider variety of searches.\n\nSo I wondered if this could replace my rev-list patch (which I've been\nmeaning to upstream for a while). But I think the answer is \"no\", there\nis room for both features. What you're finding here is much more\nspecific and useful data, but it's also much more expensive to generate.\nFor my uses cases, it was enough to ask \"show me a plausible path\nquickly\" or even \"is this object reachable\" (which you can answer out of\nbitmaps!).\n\n> Junio hinted at a different approach of solving this problem, which this\n> patch implements. Teach the diff machinery another flag for restricting\n> the information to what is shown. For example:\n> \n> $ ./git log --oneline --blobfind=v2.0.0:Makefile\n> b2feb64309 Revert the whole \"ask curl-config\" topic for now\n> 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n> \n> we observe that the Makefile as shipped with 2.0 was introduced in\n> v1.9.2-471-g47fbfded53 and replaced in v2.0.0-rc1-5-gb2feb64309 by\n> a different blob.\n\nSo I like this very much as an addition to Git's toolbox. But does it\nreally need to be limited to blobs? We should be able to ask for trees\n(both root trees and sub-trees), too. That's going to be less common, I\nthink, but I have definitely done\n\n git log --raw -t --pretty=raw --no-abbrev |\n less +/$tree_sha1\n\nto debug things (corrupted repos, funny merges, etc).\n\nYou could even do it for submodule commits. Which demonstrates that this\nfeature does not even need to actually have a resolvable object; you're\njust looking for the textual sha1.\n\nYou do your filtering on the diff queue, which I think will have the\nentries you need if \"-t\" is specified (and should always have submodule\ncommits, I think). So you'd just need to relax the incoming object\ncheck, like:\n\ndiff --git a/diff.c b/diff.c\nindex e4571df3bf..0007bb0a09 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4490,8 +4490,12 @@ static int parse_blobfind_opt(struct diff_options *opt, const char *arg)\n {\n \tstruct object_id oid;\n \n-\tif (get_oid_blob(arg, &oid) || sha1_object_info(oid.hash, NULL) != OBJ_BLOB)\n-\t\treturn error(\"object '%s' is not a blob\", arg);\n+\t/*\n+\t * We don't even need to have the object, and 40-hex sha1s will\n+\t * just resolve here.\n+\t */\n+\tif (get_oid_blob(arg, &oid))\n+\t\treturn error(\"unable to resolve '%s'\", arg);\n \n \tif (!opt->blobfind)\n \t\topt->blobfind = xcalloc(1, sizeof(*opt->blobfind));\n\nAt which point:\n\n ./git log --oneline -t --blobfind=v2.0.0:Documentation\n\njust works (the results are kind of interesting, too; you see that tree\n\"go away\" in a lot of different commits because many independent\nbranches touched the directory).\n\nAlso interesting:\n\n ./git log --oneline --blobfind=HEAD:sha1collisiondetection\n\nWell, the output for this particular case isn't that interesting. But it\nshows that we can find submodules, too.\n\n-Peff\n"},{"id":"334417","messageId":"xmqq1sk5b6rx.fsf@gitster.mtv.corp.google.com","threadId":"47404","inReplyTo":"20171208002447.20261-2-sbeller@google.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T15:04:18Z","receivedAt":"2017-12-08T15:04:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> diff --git a/diffcore-blobfind.c b/diffcore-blobfind.c\n> new file mode 100644\n> index 0000000000..e65c7cad6e\n> --- /dev/null\n> +++ b/diffcore-blobfind.c\n> @@ -0,0 +1,41 @@\n> +/*\n> + * Copyright (c) 2017 Google Inc.\n> + */\n> +#include \"cache.h\"\n> +#include \"diff.h\"\n> +#include \"diffcore.h\"\n> +\n> +static void diffcore_filter_blobs(struct diff_queue_struct *q,\n> +\t\t\t\t struct diff_options *options)\n> +{\n> +\tint src, dst;\n> +\n> +\tif (!options->blobfind)\n> +\t\tBUG(\"blobfind oidset not initialized???\");\n> +\n> +\tfor (src = dst = 0; src < q->nr; src++) {\n> +\t\tstruct diff_filepair *p = q->queue[src];\n> +\n> +\t\tif ((DIFF_FILE_VALID(p->one) &&\n> +\t\t oidset_contains(options->blobfind, &p->one->oid)) ||\n> +\t\t (DIFF_FILE_VALID(p->two) &&\n> +\t\t oidset_contains(options->blobfind, &p->two->oid))) {\n\nShouldn't this make sure that !DIFF_PAIR_UNMERGED(p) before looking\nat the sides of the pair? I think an unmerged pair is queued with\nfilespecs whose modes are cleared, but we should not be relying on\nthat---I think we even had bugs we fixed by introducing an explicit\nis_unmerged field to the filepair struct.\n\n> @@ -2883,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)\n> \t\tsimplify_merges(revs);\n> \tif (revs->children.name)\n> \t\tset_children(revs);\n> +\tif (revs->diffopt.blobfind)\n> +\t\trevs->simplify_history = 0;\n> \treturn 0;\n> }\n\nIt makes sense to clear this bit by default, but is this an\nunconditonal clearing? In other words, is there a way for the user\nto countermand this default and ask for merge simplification from\nthe command line, e.g. \"diff --simplify-history --blobfind=\"?\n\n> +test_description='test finding specific blobs in the revision walking'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup ' '\n> +\tgit commit --allow-empty -m \"empty initial commit\" &&\n> +\n> +\techo \"Hello, world!\" >greeting &&\n> +\tgit add greeting &&\n> +\tgit commit -m \"add the greeting blob\" && # borrowed from Git from the Bottom Up\n> +\tgit tag -m \"the blob\" greeting $(git rev-parse HEAD:greeting) &&\n> +\n> +\techo asdf >unrelated &&\n> +\tgit add unrelated &&\n> +\tgit commit -m \"unrelated history\" &&\n> +\n> +\tgit revert HEAD^ &&\n> +\n> +\tgit commit --allow-empty -m \"another unrelated commit\"\n> +'\n> +\n> +test_expect_success 'find the greeting blob' '\n> +\tcat >expect <<-EOF &&\n> +\tRevert \"add the greeting blob\"\n> +\tadd the greeting blob\n> +\tEOF\n> +\n> +\tgit log --abbrev=12 --oneline --blobfind=greeting^{blob} >actual.raw &&\n> +\tcut -c 14- actual.raw >actual &&\n> +\ttest_cmp expect actual\n\nThe hardcoded numbers look strange and smell like a workaround of\nnot asking for full object names.\n\nAlso, now it has the simplify-history bit in the change, don't we\nwant to check a mergy history, and also running \"diff-files\" while\nthe index has unmerged entries?\n\nOther than that, the changes look quite sane and pleasnt read.\n\nThanks.\n\n\n> +'\n> +\n> +test_done\n"},{"id":"334452","messageId":"643e1afd-43e2-7fcc-452b-8b9ceac6868c@ramsayjones.plus.com","threadId":"47404","inReplyTo":"20171208093434.GD26199@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-12-08T16:28:23Z","receivedAt":"2017-12-08T16:28:31Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 08/12/17 09:34, Jeff King wrote:\n> On Thu, Dec 07, 2017 at 04:24:47PM -0800, Stefan Beller wrote:\n[snip]\n>> Junio hinted at a different approach of solving this problem, which this\n>> patch implements. Teach the diff machinery another flag for restricting\n>> the information to what is shown. For example:\n>>\n>> $ ./git log --oneline --blobfind=v2.0.0:Makefile\n>> b2feb64309 Revert the whole \"ask curl-config\" topic for now\n>> 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n>>\n>> we observe that the Makefile as shipped with 2.0 was introduced in\n>> v1.9.2-471-g47fbfded53 and replaced in v2.0.0-rc1-5-gb2feb64309 by\n>> a different blob.\n\nSorry, this has nothing to do with this specific thread. :(\n\nHowever, the above caught my eye. Commit b2feb64309 does not 'replace\nthe Makefile with a different blob'. That commit was a 'last minute'\nrevert of a topic _prior_ to v2.0.0, which resulted in the _same_\nblob as commit 47fbfded53. (i.e. a53f3a8326c2e62dc79bae7169d64137ac3dab20).\n\n[I haven't been following this topic, so just ignore me if I have\nmisunderstood what the above was describing! :-D ]\n\nATB,\nRamsay Jones\n\n"},{"id":"334458","messageId":"xmqqbmj99lul.fsf@gitster.mtv.corp.google.com","threadId":"47404","inReplyTo":"xmqq1sk5b6rx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T17:21:38Z","receivedAt":"2017-12-08T17:21:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n> ...\n>> @@ -2883,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)\n>> \t\tsimplify_merges(revs);\n>> \tif (revs->children.name)\n>> \t\tset_children(revs);\n>> +\tif (revs->diffopt.blobfind)\n>> +\t\trevs->simplify_history = 0;\n>> \treturn 0;\n>> }\n>\n> It makes sense to clear this bit by default, but is this an\n> unconditonal clearing? In other words, is there a way for the user\n> to countermand this default and ask for merge simplification from\n> the command line, e.g. \"diff --simplify-history --blobfind=\"?\n\nLooking at the places that assign to revs->simplify_history in\nrevision.c it seems that \"this option turns the merge simplification\noff unconditionally\" is pretty much the norm, and this change just\nfollows suit. So perhaps it is OK to let this pass, at least for\nnow.\n\n"},{"id":"334490","messageId":"20171208201957.GA456@sigill.intra.peff.net","threadId":"47404","inReplyTo":"643e1afd-43e2-7fcc-452b-8b9ceac6868c@ramsayjones.plus.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T20:19:57Z","receivedAt":"2017-12-08T20:20:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2017 at 04:28:23PM +0000, Ramsay Jones wrote:\n\n> On 08/12/17 09:34, Jeff King wrote:\n> > On Thu, Dec 07, 2017 at 04:24:47PM -0800, Stefan Beller wrote:\n> [snip]\n> >> Junio hinted at a different approach of solving this problem, which this\n> >> patch implements. Teach the diff machinery another flag for restricting\n> >> the information to what is shown. For example:\n> >>\n> >> $ ./git log --oneline --blobfind=v2.0.0:Makefile\n> >> b2feb64309 Revert the whole \"ask curl-config\" topic for now\n> >> 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n> >>\n> >> we observe that the Makefile as shipped with 2.0 was introduced in\n> >> v1.9.2-471-g47fbfded53 and replaced in v2.0.0-rc1-5-gb2feb64309 by\n> >> a different blob.\n> \n> Sorry, this has nothing to do with this specific thread. :(\n> \n> However, the above caught my eye. Commit b2feb64309 does not 'replace\n> the Makefile with a different blob'. That commit was a 'last minute'\n> revert of a topic _prior_ to v2.0.0, which resulted in the _same_\n> blob as commit 47fbfded53. (i.e. a53f3a8326c2e62dc79bae7169d64137ac3dab20).\n\nRight, I think the patch is working as advertised but the commit message\nmisinterprets the results. :)\n\nIf you add --raw, you can see that both commits introduce that blob, and\nit never \"goes away\". That's because that happened in a merge, which we\ndon't diff in a default log invocation.\n\nAnd in fact finding where this \"goes away\" is hard, because the merge\ndoesn't trigger \"-c\" or \"--cc\". It's 8eaf517835, which presumably was\nforked before the revert in b2feb64309, and then the merge was able to\nreplace that blob completely with the other side of the merge.\n\nViewing with \"-m\" turns up a bunch of uninteresting merges. Looking at\n\"--first-parent\" is how I got 8eaf517835.\n\nSo I think this one is tricky because of the revert. In the same way\nthat pathspec-limiting is often tricky in the face of a revert, because\nthe merges \"hide\" interesting things happening.\n\n-Peff\n"},{"id":"334495","messageId":"CAGZ79kaos7cmQe3bmR5gCVXbUjBBQdSYYOE11egnDEMaX-7xSA@mail.gmail.com","threadId":"47404","inReplyTo":"20171208201957.GA456@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-08T20:39:55Z","receivedAt":"2017-12-08T20:40:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 8, 2017 at 12:19 PM, Jeff King wrote:\n> On Fri, Dec 08, 2017 at 04:28:23PM +0000, Ramsay Jones wrote:\n>\n>> On 08/12/17 09:34, Jeff King wrote:\n>> > On Thu, Dec 07, 2017 at 04:24:47PM -0800, Stefan Beller wrote:\n>> [snip]\n>> >> Junio hinted at a different approach of solving this problem, which this\n>> >> patch implements. Teach the diff machinery another flag for restricting\n>> >> the information to what is shown. For example:\n>> >>\n>> >> $ ./git log --oneline --blobfind=v2.0.0:Makefile\n>> >> b2feb64309 Revert the whole \"ask curl-config\" topic for now\n>> >> 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n>> >>\n>> >> we observe that the Makefile as shipped with 2.0 was introduced in\n>> >> v1.9.2-471-g47fbfded53 and replaced in v2.0.0-rc1-5-gb2feb64309 by\n>> >> a different blob.\n>>\n>> Sorry, this has nothing to do with this specific thread. :(\n>>\n>> However, the above caught my eye. Commit b2feb64309 does not 'replace\n>> the Makefile with a different blob'. That commit was a 'last minute'\n>> revert of a topic _prior_ to v2.0.0, which resulted in the _same_\n>> blob as commit 47fbfded53. (i.e. a53f3a8326c2e62dc79bae7169d64137ac3dab20).\n>\n> Right, I think the patch is working as advertised but the commit message\n> misinterprets the results. :)\n\nThanks for digging. I came to a similar realization.\n\n> If you add --raw, you can see that both commits introduce that blob, and\n> it never \"goes away\". That's because that happened in a merge, which we\n> don't diff in a default log invocation.\n\nWe should when --raw is given.\n--raw is documented as \"For each commit, show a summary of changes\nusing the raw diff format.\" and I would argue that 'each commit' includes\nmerges. Though I guess this may have implications for long time users.\n\n> And in fact finding where this \"goes away\" is hard, because the merge\n> doesn't trigger \"-c\" or \"--cc\". It's 8eaf517835, which presumably was\n> forked before the revert in b2feb64309, and then the merge was able to\n> replace that blob completely with the other side of the merge.\n>\n> Viewing with \"-m\" turns up a bunch of uninteresting merges. Looking at\n> \"--first-parent\" is how I got 8eaf517835.\n>\n> So I think this one is tricky because of the revert. In the same way\n> that pathspec-limiting is often tricky in the face of a revert, because\n> the merges \"hide\" interesting things happening.\n\nyup, hidden merges are unfortunate. Is there an easy way to find out\nabout merges? (Junio hints at having tests around merges, which I'll do\nnext)\n"},{"id":"334499","messageId":"CAGZ79kbE9ePfxdD+rxp=P_jZwv4RPiVSbhdpjEf48dAMsKWYrQ@mail.gmail.com","threadId":"47404","inReplyTo":"xmqq1sk5b6rx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-08T21:11:32Z","receivedAt":"2017-12-08T21:11:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">> +\n>> + if ((DIFF_FILE_VALID(p->one) &&\n>> + oidset_contains(options->blobfind, &p->one->oid)) ||\n>> + (DIFF_FILE_VALID(p->two) &&\n>> + oidset_contains(options->blobfind, &p->two->oid))) {\n>\n> Shouldn't this make sure that !DIFF_PAIR_UNMERGED(p) before looking\n> at the sides of the pair? I think an unmerged pair is queued with\n> filespecs whose modes are cleared, but we should not be relying on\n> that---I think we even had bugs we fixed by introducing an explicit\n> is_unmerged field to the filepair struct.\n\nI am not sure I follow. IIUC 'unmerged' only ever happens in the\nindex when there are multiple stages for one path (represented in the\nworking tree with diff markers). Aren't we supposed to find such\nan unmerged blob as well (despite wrong mode, but the oid ought to fit)?\n\n>> + if (revs->diffopt.blobfind)\n>> + revs->simplify_history = 0;\n>> return 0;\n>> }\n>\n> It makes sense to clear this bit by default, but is this an\n> unconditonal clearing? In other words, is there a way for the user\n> to countermand this default and ask for merge simplification from\n> the command line, e.g. \"diff --simplify-history --blobfind=\"?\n\nAs noted in your reply, this is consistent with other occurrences of\nsimplify_history, so let's keep it this way.\n\n>> +\n>> + git log --abbrev=12 --oneline --blobfind=greeting^{blob} >actual.raw &&\n>> + cut -c 14- actual.raw >actual &&\n>> + test_cmp expect actual\n>\n> The hardcoded numbers look strange and smell like a workaround of\n> not asking for full object names.\n\nYou mean the 12 and 14 ? Yeah I can fix that to 40 and 42 if you want.\nI wrote it this way to be agnostic of the actual object id, but thought 12\nwould be enough for this test no matter the future hash.\n\n> Also, now it has the simplify-history bit in the change, don't we\n> want to check a mergy history, and also running \"diff-files\" while\n> the index has unmerged entries?\n\nyup, I am working on that.\n"},{"id":"334501","messageId":"xmqqefo47wgo.fsf@gitster.mtv.corp.google.com","threadId":"47404","inReplyTo":"CAGZ79kbE9ePfxdD+rxp=P_jZwv4RPiVSbhdpjEf48dAMsKWYrQ@mail.gmail.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T21:15:19Z","receivedAt":"2017-12-08T21:15:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>>> +\n>>> + if ((DIFF_FILE_VALID(p->one) &&\n>>> + oidset_contains(options->blobfind, &p->one->oid)) ||\n>>> + (DIFF_FILE_VALID(p->two) &&\n>>> + oidset_contains(options->blobfind, &p->two->oid))) {\n>>\n>> Shouldn't this make sure that !DIFF_PAIR_UNMERGED(p) before looking\n>> at the sides of the pair? I think an unmerged pair is queued with\n>> filespecs whose modes are cleared, but we should not be relying on\n>> that---I think we even had bugs we fixed by introducing an explicit\n>> is_unmerged field to the filepair struct.\n>\n> I am not sure I follow. IIUC 'unmerged' only ever happens in the\n> index when there are multiple stages for one path (represented in the\n> working tree with diff markers). Aren't we supposed to find such\n> an unmerged blob as well (despite wrong mode, but the oid ought to fit)?\n\nI think diff_unmerged() records \"this path is unmerged\" without\ngiving any object name to either side, so you do not get to compare\n(or even look at) p->{one,two}->oid with anything. That is why I\nthink checking p->one and p->two like the above code, without making\nsure that you are looking at a pair that is not unmerged, is wrong.\n\n"},{"id":"334507","messageId":"20171208213822.GD7355@sigill.intra.peff.net","threadId":"47404","inReplyTo":"CAGZ79kaos7cmQe3bmR5gCVXbUjBBQdSYYOE11egnDEMaX-7xSA@mail.gmail.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T21:38:22Z","receivedAt":"2017-12-08T21:38:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2017 at 12:39:55PM -0800, Stefan Beller wrote:\n\n> > If you add --raw, you can see that both commits introduce that blob, and\n> > it never \"goes away\". That's because that happened in a merge, which we\n> > don't diff in a default log invocation.\n> \n> We should when --raw is given.\n> --raw is documented as \"For each commit, show a summary of changes\n> using the raw diff format.\" and I would argue that 'each commit' includes\n> merges. Though I guess this may have implications for long time users.\n\nAnd \"--patch\" is documented as \"generate patch\", but it also does\nnothing for merges by default. I think it has little to do with \"--raw\".\nIt is simply that the default for \"log\" is none of \"-c\", \"--cc\", or\n\"-m\".\n\nWe _could_ change that default (\"--cc\" is already the default for\ngit-show), but I would not be surprised if that has fallouts (certainly\nit makes git-log much slower).\n\n> > So I think this one is tricky because of the revert. In the same way\n> > that pathspec-limiting is often tricky in the face of a revert, because\n> > the merges \"hide\" interesting things happening.\n> \n> yup, hidden merges are unfortunate. Is there an easy way to find out\n> about merges? (Junio hints at having tests around merges, which I'll do\n> next)\n\nIf you find such an easy way, let me know. :)\n\nOne of the few really manual types of query I remember having done in\nrecent years is trying to pinpoint a bad merge. I.e., somebody during\nmerge resolution accidentally does \"git checkout --ours foo.c\", blowing\naway changes which they didn't mean to. And then later you want to\nfigure out which merge did it.\n\nIf you use \"-c\" or \"--cc\", that isn't an \"interesting\" change, because\nit resolves to one side of the merge. If you use \"-m\", you get way too\nmany changes and have to comb through them manually. I've resorted to\n\"-m --first-parent\", but then you frequently have to dig down several\nlayers (e.g., the bad merge is a merge from \"master\" onto a topic\nbranch, and your first \"--first-parent\" attempt will just find the bad\ntopic being merged back into master).\n\nI think the most promising tool I've seen there is to redo the merge and\nshow the diff between the auto-merge (including conflicts) and the\ncommitted tree. It's just another definition of \"is this hunk\ninteresting\" that's different from \"--cc\".\n\nI'm not sure how that would interact with something like \"--blobfind\",\nthough. For that matter, I'm not quite sure how your patch would\ninteract with \"--cc\". I think you may need to special-case it.\n\n-Peff\n"},{"id":"334623","messageId":"20171211195835.57057-1-sbeller@google.com","threadId":"47404","inReplyTo":"20171208002447.20261-1-sbeller@google.com","subject":"[PATCH 0/1] diff-core blobfind","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-11T19:58:34Z","receivedAt":"2017-12-11T19:59:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"* added check for unmerged entries (!DIFF_PAIR_UNMERGED)\n* added support to find submodules\n* renamed the UI to `--find-object` instead of --blobfind.\n\ndiff to currently queued below.\n\nThanks,\nStefan\n\nStefan Beller (1):\n diffcore: add a filter to find a specific blob\n\n Documentation/diff-options.txt | 6 +++++\n Makefile | 1 +\n builtin/log.c | 2 +-\n diff.c | 21 ++++++++++++++++-\n diff.h | 3 +++\n diffcore-oidfind.c | 42 ++++++++++++++++++++++++++++++++++\n diffcore.h | 1 +\n revision.c | 5 ++++-\n t/t4064-diff-oidfind.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 129 insertions(+), 3 deletions(-)\n create mode 100644 diffcore-oidfind.c\n create mode 100755 t/t4064-diff-oidfind.sh\n\n-- \n2.15.1.424.g9478a66081-goog\n\ndiff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt\nindex 34d53b95f1..67a99e522b 100644\n--- c/Documentation/diff-options.txt\n+++ w/Documentation/diff-options.txt\n@@ -501,9 +501,10 @@ information.\n \tTreat the given to `-S` as an extended POSIX regular\n \texpression to match.\n \n---blobfind=::\n+--find-object=::\n \tRestrict the output such that one side of the diff\n-\tmatches the given blob-id.\n+\tmatches the given object id. The object can be a blob,\n+\tor gitlink entry.\n \n endif::git-format-patch[]\n \ndiff --git c/Makefile w/Makefile\nindex fdfa8f38f6..fc2b136694 100644\n--- c/Makefile\n+++ w/Makefile\n@@ -775,7 +775,7 @@ LIB_OBJS += date.o\n LIB_OBJS += decorate.o\n LIB_OBJS += diffcore-break.o\n LIB_OBJS += diffcore-delta.o\n-LIB_OBJS += diffcore-blobfind.o\n+LIB_OBJS += diffcore-oidfind.o\n LIB_OBJS += diffcore-order.o\n LIB_OBJS += diffcore-pickaxe.o\n LIB_OBJS += diffcore-rename.o\ndiff --git c/builtin/log.c w/builtin/log.c\nindex 7b91f61423..2ab7f338e6 100644\n--- c/builtin/log.c\n+++ w/builtin/log.c\n@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tinit_display_notes(&rev->notes_opt);\n \n \tif (rev->diffopt.pickaxe || rev->diffopt.filter ||\n-\t rev->diffopt.flags.follow_renames || rev->diffopt.blobfind)\n+\t rev->diffopt.flags.follow_renames || rev->diffopt.oidfind)\n \t\trev->always_show_header = 0;\n \n \tif (source)\ndiff --git c/diff.c w/diff.c\nindex 8861f89ab1..cb35934634 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -4082,7 +4082,7 @@ void diff_setup(struct diff_options *options)\n \toptions->interhunkcontext = diff_interhunk_context_default;\n \toptions->ws_error_highlight = ws_error_highlight_default;\n \toptions->flags.rename_empty = 1;\n-\toptions->blobfind = NULL;\n+\toptions->oidfind = NULL;\n \n \t/* pathchange left =NULL by default */\n \toptions->change = diff_change;\n@@ -4488,16 +4488,17 @@ static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *ar\n \treturn 1;\n }\n \n-static int parse_blobfind_opt(struct diff_options *opt, const char *arg)\n+static int parse_oidfind_opt(struct diff_options *opt, const char *arg)\n {\n \tstruct object_id oid;\n \n-\tif (get_oid_blob(arg, &oid) || sha1_object_info(oid.hash, NULL) != OBJ_BLOB)\n-\t\treturn error(\"object '%s' is not a blob\", arg);\n+\t/* We don't even need to have the object, any oid works. */\n+\tif (get_oid_blob(arg, &oid))\n+\t\treturn error(\"unable to resolve '%s'\", arg);\n \n-\tif (!opt->blobfind)\n-\t\topt->blobfind = xcalloc(1, sizeof(*opt->blobfind));\n-\toidset_insert(opt->blobfind, &oid);\n+\tif (!opt->oidfind)\n+\t\topt->oidfind = xcalloc(1, sizeof(*opt->oidfind));\n+\toidset_insert(opt->oidfind, &oid);\n \treturn 1;\n }\n \n@@ -4750,8 +4751,8 @@ int diff_opt_parse(struct diff_options *options,\n \telse if ((argcount = short_opt('O', av, &optarg))) {\n \t\toptions->orderfile = prefix_filename(prefix, optarg);\n \t\treturn argcount;\n-\t} else if (skip_prefix(arg, \"--blobfind=\", &arg))\n-\t\treturn parse_blobfind_opt(options, arg);\n+\t} else if (skip_prefix(arg, \"--find-object=\", &arg))\n+\t\treturn parse_oidfind_opt(options, arg);\n \telse if ((argcount = parse_long_opt(\"diff-filter\", av, &optarg))) {\n \t\tint offending = parse_diff_filter_opt(optarg, options);\n \t\tif (offending)\n@@ -5786,8 +5787,8 @@ void diffcore_std(struct diff_options *options)\n \tif (!options->found_follow) {\n \t\t/* See try_to_follow_renames() in tree-diff.c */\n \n-\t\tif (options->blobfind)\n-\t\t\tdiffcore_blobfind(options);\n+\t\tif (options->oidfind)\n+\t\t\tdiffcore_oidfind(options);\n \t\tif (options->break_opt != -1)\n \t\t\tdiffcore_break(options->break_opt);\n \t\tif (options->detect_rename)\ndiff --git c/diff.h w/diff.h\nindex 9178e498fa..d2badb29a1 100644\n--- c/diff.h\n+++ w/diff.h\n@@ -175,7 +175,7 @@ struct diff_options {\n \tenum diff_words_type word_diff;\n \tenum diff_submodule_format submodule_format;\n \n-\tstruct oidset *blobfind;\n+\tstruct oidset *oidfind;\n \n \t/* this is set by diffcore for DIFF_FORMAT_PATCH */\n \tint found_changes;\ndiff --git c/diffcore-blobfind.c w/diffcore-oidfind.c\nsimilarity index 64%\nrename from diffcore-blobfind.c\nrename to diffcore-oidfind.c\nindex e65c7cad6e..39a0b5390f 100644\n--- c/diffcore-blobfind.c\n+++ w/diffcore-oidfind.c\n@@ -10,16 +10,17 @@ static void diffcore_filter_blobs(struct diff_queue_struct *q,\n {\n \tint src, dst;\n \n-\tif (!options->blobfind)\n-\t\tBUG(\"blobfind oidset not initialized???\");\n+\tif (!options->oidfind)\n+\t\tBUG(\"oidfind oidset not initialized???\");\n \n \tfor (src = dst = 0; src < q->nr; src++) {\n \t\tstruct diff_filepair *p = q->queue[src];\n \n-\t\tif ((DIFF_FILE_VALID(p->one) &&\n-\t\t oidset_contains(options->blobfind, &p->one->oid)) ||\n+\t\tif (!DIFF_PAIR_UNMERGED(p) &&\n+\t\t ((DIFF_FILE_VALID(p->one) &&\n+\t\t oidset_contains(options->oidfind, &p->one->oid)) ||\n \t\t (DIFF_FILE_VALID(p->two) &&\n-\t\t oidset_contains(options->blobfind, &p->two->oid))) {\n+\t\t oidset_contains(options->oidfind, &p->two->oid)))) {\n \t\t\tq->queue[dst] = p;\n \t\t\tdst++;\n \t\t} else {\n@@ -35,7 +36,7 @@ static void diffcore_filter_blobs(struct diff_queue_struct *q,\n \t}\n }\n \n-void diffcore_blobfind(struct diff_options *options)\n+void diffcore_oidfind(struct diff_options *options)\n {\n \tdiffcore_filter_blobs(&diff_queued_diff, options);\n }\ndiff --git c/diffcore.h w/diffcore.h\nindex 431917672f..6bb1d76b1a 100644\n--- c/diffcore.h\n+++ w/diffcore.h\n@@ -107,7 +107,7 @@ extern struct diff_filepair *diff_queue(struct diff_queue_struct *,\n \t\t\t\t\tstruct diff_filespec *);\n extern void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n \n-extern void diffcore_blobfind(struct diff_options *);\n+extern void diffcore_oidfind(struct diff_options *);\n extern void diffcore_break(int);\n extern void diffcore_rename(struct diff_options *);\n extern void diffcore_merge_broken(void);\ndiff --git c/revision.c w/revision.c\nindex 8e1a89f832..e4611054b9 100644\n--- c/revision.c\n+++ w/revision.c\n@@ -2410,7 +2410,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tif (revs->diffopt.pickaxe ||\n \t revs->diffopt.filter ||\n \t revs->diffopt.flags.follow_renames ||\n-\t revs->diffopt.blobfind)\n+\t revs->diffopt.oidfind)\n \t\trevs->diff = 1;\n \n \tif (revs->topo_order)\n@@ -2884,7 +2884,7 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tsimplify_merges(revs);\n \tif (revs->children.name)\n \t\tset_children(revs);\n-\tif (revs->diffopt.blobfind)\n+\tif (revs->diffopt.oidfind)\n \t\trevs->simplify_history = 0;\n \treturn 0;\n }\ndiff --git c/t/t4064-diff-blobfind.sh w/t/t4064-diff-oidfind.sh\nsimilarity index 65%\nrename from t/t4064-diff-blobfind.sh\nrename to t/t4064-diff-oidfind.sh\nindex b2c2964d77..7afe62df87 100755\n--- c/t/t4064-diff-blobfind.sh\n+++ w/t/t4064-diff-oidfind.sh\n@@ -26,8 +26,24 @@ test_expect_success 'find the greeting blob' '\n \tadd the greeting blob\n \tEOF\n \n-\tgit log --abbrev=12 --oneline --blobfind=greeting^{blob} >actual.raw &&\n-\tcut -c 14- actual.raw >actual &&\n+\tgit log --format=%s --find-object=greeting^{blob} >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'setup a submodule' '\n+\ttest_create_repo sub &&\n+\ttest_commit -C sub sub &&\n+\tgit submodule add ./sub sub &&\n+\tgit commit -a -m \"add sub\"\n+'\n+\n+test_expect_success 'find a submodule' '\n+\tcat >expect <<-EOF &&\n+\tadd sub\n+\tEOF\n+\n+\tgit log --format=%s --find-object=HEAD:sub >actual &&\n \n \ttest_cmp expect actual\n '\n"},{"id":"334624","messageId":"20171211195835.57057-2-sbeller@google.com","threadId":"47404","inReplyTo":"20171211195835.57057-1-sbeller@google.com","subject":"[PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-11T19:58:35Z","receivedAt":"2017-12-11T19:59:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Sometimes users are given a hash of an object and they want to\nidentify it further (ex.: Use verify-pack to find the largest blobs,\nbut what are these? or [1])\n\nOne might be tempted to extend git-describe to also work with blobs,\nsuch that `git describe ` gives a description as\n':'. This was implemented at [2]; as seen by the sheer\nnumber of responses (>110), it turns out this is tricky to get right.\nThe hard part to get right is picking the correct 'commit-ish' as that\ncould be the commit that (re-)introduced the blob or the blob that\nremoved the blob; the blob could exist in different branches.\n\nJunio hinted at a different approach of solving this problem, which this\npatch implements. Teach the diff machinery another flag for restricting\nthe information to what is shown. For example:\n\n $ ./git log --oneline --blobfind=v2.0.0:Makefile\n b2feb64309 Revert the whole \"ask curl-config\" topic for now\n 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n\nwe observe that the Makefile as shipped with 2.0 was introduced in\nv1.9.2-471-g47fbfded53 and replaced in v2.0.0-rc1-5-gb2feb64309 by\na different blob.\n\n[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob\n[2] https://public-inbox.org/git/20171028004419.10139-1-sbeller@google.com/\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 6 +++++\n Makefile | 1 +\n builtin/log.c | 2 +-\n diff.c | 21 ++++++++++++++++-\n diff.h | 3 +++\n diffcore-oidfind.c | 42 ++++++++++++++++++++++++++++++++++\n diffcore.h | 1 +\n revision.c | 5 ++++-\n t/t4064-diff-oidfind.sh | 51 ++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 129 insertions(+), 3 deletions(-)\n create mode 100644 diffcore-oidfind.c\n create mode 100755 t/t4064-diff-oidfind.sh\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex dd0dba5b1d..67a99e522b 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -500,6 +500,12 @@ information.\n --pickaxe-regex::\n \tTreat the given to `-S` as an extended POSIX regular\n \texpression to match.\n+\n+--find-object=::\n+\tRestrict the output such that one side of the diff\n+\tmatches the given object id. The object can be a blob,\n+\tor gitlink entry.\n+\n endif::git-format-patch[]\n \n -O::\ndiff --git a/Makefile b/Makefile\nindex ee9d5eb11e..fc2b136694 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -775,6 +775,7 @@ LIB_OBJS += date.o\n LIB_OBJS += decorate.o\n LIB_OBJS += diffcore-break.o\n LIB_OBJS += diffcore-delta.o\n+LIB_OBJS += diffcore-oidfind.o\n LIB_OBJS += diffcore-order.o\n LIB_OBJS += diffcore-pickaxe.o\n LIB_OBJS += diffcore-rename.o\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6c1fa896ad..2ab7f338e6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tinit_display_notes(&rev->notes_opt);\n \n \tif (rev->diffopt.pickaxe || rev->diffopt.filter ||\n-\t rev->diffopt.flags.follow_renames)\n+\t rev->diffopt.flags.follow_renames || rev->diffopt.oidfind)\n \t\trev->always_show_header = 0;\n \n \tif (source)\ndiff --git a/diff.c b/diff.c\nindex 0763e89263..cb35934634 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)\n \toptions->interhunkcontext = diff_interhunk_context_default;\n \toptions->ws_error_highlight = ws_error_highlight_default;\n \toptions->flags.rename_empty = 1;\n+\toptions->oidfind = NULL;\n \n \t/* pathchange left =NULL by default */\n \toptions->change = diff_change;\n@@ -4487,6 +4488,20 @@ static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *ar\n \treturn 1;\n }\n \n+static int parse_oidfind_opt(struct diff_options *opt, const char *arg)\n+{\n+\tstruct object_id oid;\n+\n+\t/* We don't even need to have the object, any oid works. */\n+\tif (get_oid_blob(arg, &oid))\n+\t\treturn error(\"unable to resolve '%s'\", arg);\n+\n+\tif (!opt->oidfind)\n+\t\topt->oidfind = xcalloc(1, sizeof(*opt->oidfind));\n+\toidset_insert(opt->oidfind, &oid);\n+\treturn 1;\n+}\n+\n int diff_opt_parse(struct diff_options *options,\n \t\t const char **av, int ac, const char *prefix)\n {\n@@ -4736,7 +4751,8 @@ int diff_opt_parse(struct diff_options *options,\n \telse if ((argcount = short_opt('O', av, &optarg))) {\n \t\toptions->orderfile = prefix_filename(prefix, optarg);\n \t\treturn argcount;\n-\t}\n+\t} else if (skip_prefix(arg, \"--find-object=\", &arg))\n+\t\treturn parse_oidfind_opt(options, arg);\n \telse if ((argcount = parse_long_opt(\"diff-filter\", av, &optarg))) {\n \t\tint offending = parse_diff_filter_opt(optarg, options);\n \t\tif (offending)\n@@ -5770,6 +5786,9 @@ void diffcore_std(struct diff_options *options)\n \t\tdiffcore_skip_stat_unmatch(options);\n \tif (!options->found_follow) {\n \t\t/* See try_to_follow_renames() in tree-diff.c */\n+\n+\t\tif (options->oidfind)\n+\t\t\tdiffcore_oidfind(options);\n \t\tif (options->break_opt != -1)\n \t\t\tdiffcore_break(options->break_opt);\n \t\tif (options->detect_rename)\ndiff --git a/diff.h b/diff.h\nindex 0fb18dd735..d2badb29a1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -7,6 +7,7 @@\n #include \"tree-walk.h\"\n #include \"pathspec.h\"\n #include \"object.h\"\n+#include \"oidset.h\"\n \n struct rev_info;\n struct diff_options;\n@@ -174,6 +175,8 @@ struct diff_options {\n \tenum diff_words_type word_diff;\n \tenum diff_submodule_format submodule_format;\n \n+\tstruct oidset *oidfind;\n+\n \t/* this is set by diffcore for DIFF_FORMAT_PATCH */\n \tint found_changes;\n \ndiff --git a/diffcore-oidfind.c b/diffcore-oidfind.c\nnew file mode 100644\nindex 0000000000..39a0b5390f\n--- /dev/null\n+++ b/diffcore-oidfind.c\n@@ -0,0 +1,42 @@\n+/*\n+ * Copyright (c) 2017 Google Inc.\n+ */\n+#include \"cache.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n+\n+static void diffcore_filter_blobs(struct diff_queue_struct *q,\n+\t\t\t\t struct diff_options *options)\n+{\n+\tint src, dst;\n+\n+\tif (!options->oidfind)\n+\t\tBUG(\"oidfind oidset not initialized???\");\n+\n+\tfor (src = dst = 0; src < q->nr; src++) {\n+\t\tstruct diff_filepair *p = q->queue[src];\n+\n+\t\tif (!DIFF_PAIR_UNMERGED(p) &&\n+\t\t ((DIFF_FILE_VALID(p->one) &&\n+\t\t oidset_contains(options->oidfind, &p->one->oid)) ||\n+\t\t (DIFF_FILE_VALID(p->two) &&\n+\t\t oidset_contains(options->oidfind, &p->two->oid)))) {\n+\t\t\tq->queue[dst] = p;\n+\t\t\tdst++;\n+\t\t} else {\n+\t\t\tdiff_free_filepair(p);\n+\t\t}\n+\t}\n+\n+\tif (!dst) {\n+\t\tfree(q->queue);\n+\t\tDIFF_QUEUE_CLEAR(q);\n+\t} else {\n+\t\tq->nr = dst;\n+\t}\n+}\n+\n+void diffcore_oidfind(struct diff_options *options)\n+{\n+\tdiffcore_filter_blobs(&diff_queued_diff, options);\n+}\ndiff --git a/diffcore.h b/diffcore.h\nindex a30da161da..6bb1d76b1a 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -107,6 +107,7 @@ extern struct diff_filepair *diff_queue(struct diff_queue_struct *,\n \t\t\t\t\tstruct diff_filespec *);\n extern void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n \n+extern void diffcore_oidfind(struct diff_options *);\n extern void diffcore_break(int);\n extern void diffcore_rename(struct diff_options *);\n extern void diffcore_merge_broken(void);\ndiff --git a/revision.c b/revision.c\nindex e2e691dd5a..e4611054b9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2409,7 +2409,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t/* Pickaxe, diff-filter and rename following need diffs */\n \tif (revs->diffopt.pickaxe ||\n \t revs->diffopt.filter ||\n-\t revs->diffopt.flags.follow_renames)\n+\t revs->diffopt.flags.follow_renames ||\n+\t revs->diffopt.oidfind)\n \t\trevs->diff = 1;\n \n \tif (revs->topo_order)\n@@ -2883,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tsimplify_merges(revs);\n \tif (revs->children.name)\n \t\tset_children(revs);\n+\tif (revs->diffopt.oidfind)\n+\t\trevs->simplify_history = 0;\n \treturn 0;\n }\n \ndiff --git a/t/t4064-diff-oidfind.sh b/t/t4064-diff-oidfind.sh\nnew file mode 100755\nindex 0000000000..7afe62df87\n--- /dev/null\n+++ b/t/t4064-diff-oidfind.sh\n@@ -0,0 +1,51 @@\n+#!/bin/sh\n+\n+test_description='test finding specific blobs in the revision walking'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup ' '\n+\tgit commit --allow-empty -m \"empty initial commit\" &&\n+\n+\techo \"Hello, world!\" >greeting &&\n+\tgit add greeting &&\n+\tgit commit -m \"add the greeting blob\" && # borrowed from Git from the Bottom Up\n+\tgit tag -m \"the blob\" greeting $(git rev-parse HEAD:greeting) &&\n+\n+\techo asdf >unrelated &&\n+\tgit add unrelated &&\n+\tgit commit -m \"unrelated history\" &&\n+\n+\tgit revert HEAD^ &&\n+\n+\tgit commit --allow-empty -m \"another unrelated commit\"\n+'\n+\n+test_expect_success 'find the greeting blob' '\n+\tcat >expect <<-EOF &&\n+\tRevert \"add the greeting blob\"\n+\tadd the greeting blob\n+\tEOF\n+\n+\tgit log --format=%s --find-object=greeting^{blob} >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'setup a submodule' '\n+\ttest_create_repo sub &&\n+\ttest_commit -C sub sub &&\n+\tgit submodule add ./sub sub &&\n+\tgit commit -a -m \"add sub\"\n+'\n+\n+test_expect_success 'find a submodule' '\n+\tcat >expect <<-EOF &&\n+\tadd sub\n+\tEOF\n+\n+\tgit log --format=%s --find-object=HEAD:sub >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334639","messageId":"xmqqr2s04zxw.fsf@gitster.mtv.corp.google.com","threadId":"47404","inReplyTo":"20171211195835.57057-2-sbeller@google.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-11T23:17:31Z","receivedAt":"2017-12-11T23:17:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> the information to what is shown. For example:\n>\n> $ ./git log --oneline --blobfind=v2.0.0:Makefile\n> b2feb64309 Revert the whole \"ask curl-config\" topic for now\n> 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n\nThis part is a bit stale???\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index dd0dba5b1d..67a99e522b 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -500,6 +500,12 @@ information.\n> --pickaxe-regex::\n> \tTreat the given to `-S` as an extended POSIX regular\n> \texpression to match.\n> +\n> +--find-object=::\n> +\tRestrict the output such that one side of the diff\n> +\tmatches the given object id. The object can be a blob,\n> +\tor gitlink entry.\n\nOK. In principle you should also be able to find a tree, but I do\nnot now how useful it would be. Extending it to gitlink, which is\nanother kind of leaf node in the reachability DAG, does make tons of\nsense---it's a no brainer that I feel ashamed not to have thought of\nmyself ;-)\n\n> +LIB_OBJS += diffcore-oidfind.o\n\nJust to nitpick, but \"blobfind\" was to find \"blob\", and if you are\nextending it to find any \"object\", then that should be \"objfind\".\n\"oid\" is _A_ way to refer to an object (i.e. the _name_ of it), and\nname is *not* the same as the thing the name refers to, so...\n\n> +static int parse_oidfind_opt(struct diff_options *opt, const char *arg)\n> +{\n> +\tstruct object_id oid;\n> +\n> +\t/* We don't even need to have the object, any oid works. */\n> +\tif (get_oid_blob(arg, &oid))\n> +\t\treturn error(\"unable to resolve '%s'\", arg);\n\nShould this still be get_oid_blob(), or should it be less specific\nto blobs?\n\n> +test_expect_success 'find the greeting blob' '\n> +\tcat >expect <<-EOF &&\n> +\tRevert \"add the greeting blob\"\n> +\tadd the greeting blob\n> +\tEOF\n> +\n> +\tgit log --format=%s --find-object=greeting^{blob} >actual &&\n> +\n> +\ttest_cmp expect actual\n> +'\n\nMakes sense.\n\n> +\n> +test_expect_success 'setup a submodule' '\n> +\ttest_create_repo sub &&\n> +\ttest_commit -C sub sub &&\n> +\tgit submodule add ./sub sub &&\n> +\tgit commit -a -m \"add sub\"\n> +'\n> +\n> +test_expect_success 'find a submodule' '\n> +\tcat >expect <<-EOF &&\n> +\tadd sub\n> +\tEOF\n> +\n> +\tgit log --format=%s --find-object=HEAD:sub >actual &&\n> +\n> +\ttest_cmp expect actual\n> +'\n\nNice (and cute).\n\n> +test_done\n\nLooking good. Thanks.\n"},{"id":"334654","messageId":"CAGZ79kaJgCchQKcRMkW-cjRmx_7eYQDqLs5GgDOgLEb1H=w-0g@mail.gmail.com","threadId":"47404","inReplyTo":"xmqqr2s04zxw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1] diffcore: add a filter to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-12T00:21:23Z","receivedAt":"2017-12-12T00:21:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 11, 2017 at 3:17 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> the information to what is shown. For example:\n>>\n>> $ ./git log --oneline --blobfind=v2.0.0:Makefile\n>> b2feb64309 Revert the whole \"ask curl-config\" topic for now\n>> 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n>\n> This part is a bit stale???\n\nfixed in the next reroll :(\n\n>> +--find-object=::\n>> + Restrict the output such that one side of the diff\n>> + matches the given object id. The object can be a blob,\n>> + or gitlink entry.\n>\n> OK. In principle you should also be able to find a tree, but I do\n> not now how useful it would be. Extending it to gitlink, which is\n> another kind of leaf node in the reachability DAG, does make tons of\n> sense---it's a no brainer that I feel ashamed not to have thought of\n> myself ;-)\n\nThe current patch under discussion doesn't find trees, though.\nHence the documentation is accurate saying that only blobs and\ngitlinks work.\n\n>\n>> +LIB_OBJS += diffcore-oidfind.o\n>\n> Just to nitpick, but \"blobfind\" was to find \"blob\", and if you are\n> extending it to find any \"object\", then that should be \"objfind\".\n> \"oid\" is _A_ way to refer to an object (i.e. the _name_ of it), and\n> name is *not* the same as the thing the name refers to, so...\n\nobj-find sounds good.\n\n>\n>> +static int parse_oidfind_opt(struct diff_options *opt, const char *arg)\n>> +{\n>> + struct object_id oid;\n>> +\n>> + /* We don't even need to have the object, any oid works. */\n>> + if (get_oid_blob(arg, &oid))\n>> + return error(\"unable to resolve '%s'\", arg);\n>\n> Should this still be get_oid_blob(), or should it be less specific\n> to blobs?\n\nWe could check if it is a tree/commit and die as they are not\nbeing handled correctly.\n"},{"id":"334661","messageId":"20171212012422.123332-1-sbeller@google.com","threadId":"47404","inReplyTo":"CAGZ79kaJgCchQKcRMkW-cjRmx_7eYQDqLs5GgDOgLEb1H=w-0g@mail.gmail.com","subject":"[PATCH] diffcore: add a filter to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-12T01:24:22Z","receivedAt":"2017-12-12T01:24:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Sometimes users are given a hash of an object and they want to\nidentify it further (ex.: Use verify-pack to find the largest blobs,\nbut what are these? or [1])\n\nOne might be tempted to extend git-describe to also work with blobs,\nsuch that `git describe ` gives a description as\n':'. This was implemented at [2]; as seen by the sheer\nnumber of responses (>110), it turns out this is tricky to get right.\nThe hard part to get right is picking the correct 'commit-ish' as that\ncould be the commit that (re-)introduced the blob or the blob that\nremoved the blob; the blob could exist in different branches.\n\nJunio hinted at a different approach of solving this problem, which this\npatch implements. Teach the diff machinery another flag for restricting\nthe information to what is shown. For example:\n\n $ ./git log --oneline --find-object=v2.0.0:Makefile\n b2feb64309 Revert the whole \"ask curl-config\" topic for now\n 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n\nwe observe that the Makefile as shipped with 2.0 was appeared in\nv1.9.2-471-g47fbfded53 and in v2.0.0-rc1-5-gb2feb6430b. The\nreason why these commits both occur prior to v2.0.0 are evil\nmerges that are not found using this new mechanism.\n\n[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob\n[2] https://public-inbox.org/git/20171028004419.10139-1-sbeller@google.com/\n\nSigned-off-by: Stefan Beller \n\nasdf\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 6 ++++\n Makefile | 1 +\n builtin/log.c | 2 +-\n diff.c | 20 ++++++++++++-\n diff.h | 3 ++\n diffcore-objfind.c | 42 ++++++++++++++++++++++++++\n diffcore.h | 1 +\n revision.c | 5 +++-\n t/t4064-diff-oidfind.sh | 68 ++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 145 insertions(+), 3 deletions(-)\n create mode 100644 diffcore-objfind.c\n create mode 100755 t/t4064-diff-oidfind.sh\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex dd0dba5b1d..21d1776996 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -500,6 +500,12 @@ information.\n --pickaxe-regex::\n \tTreat the given to `-S` as an extended POSIX regular\n \texpression to match.\n+\n+--find-object=::\n+\tRestrict the output such that one side of the diff\n+\tmatches the given object id. The object can be a blob,\n+\tgitlink entry or tree (when `-t` is given).\n+\n endif::git-format-patch[]\n \n -O::\ndiff --git a/Makefile b/Makefile\nindex ee9d5eb11e..c26596c30a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -775,6 +775,7 @@ LIB_OBJS += date.o\n LIB_OBJS += decorate.o\n LIB_OBJS += diffcore-break.o\n LIB_OBJS += diffcore-delta.o\n+LIB_OBJS += diffcore-objfind.o\n LIB_OBJS += diffcore-order.o\n LIB_OBJS += diffcore-pickaxe.o\n LIB_OBJS += diffcore-rename.o\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6c1fa896ad..08ea82d69f 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tinit_display_notes(&rev->notes_opt);\n \n \tif (rev->diffopt.pickaxe || rev->diffopt.filter ||\n-\t rev->diffopt.flags.follow_renames)\n+\t rev->diffopt.flags.follow_renames || rev->diffopt.objfind)\n \t\trev->always_show_header = 0;\n \n \tif (source)\ndiff --git a/diff.c b/diff.c\nindex 0763e89263..e13b8229d3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)\n \toptions->interhunkcontext = diff_interhunk_context_default;\n \toptions->ws_error_highlight = ws_error_highlight_default;\n \toptions->flags.rename_empty = 1;\n+\toptions->objfind = NULL;\n \n \t/* pathchange left =NULL by default */\n \toptions->change = diff_change;\n@@ -4487,6 +4488,19 @@ static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *ar\n \treturn 1;\n }\n \n+static int parse_objfind_opt(struct diff_options *opt, const char *arg)\n+{\n+\tstruct object_id oid;\n+\n+\tif (get_oid(arg, &oid))\n+\t\treturn error(\"unable to resolve '%s'\", arg);\n+\n+\tif (!opt->objfind)\n+\t\topt->objfind = xcalloc(1, sizeof(*opt->objfind));\n+\toidset_insert(opt->objfind, &oid);\n+\treturn 1;\n+}\n+\n int diff_opt_parse(struct diff_options *options,\n \t\t const char **av, int ac, const char *prefix)\n {\n@@ -4736,7 +4750,8 @@ int diff_opt_parse(struct diff_options *options,\n \telse if ((argcount = short_opt('O', av, &optarg))) {\n \t\toptions->orderfile = prefix_filename(prefix, optarg);\n \t\treturn argcount;\n-\t}\n+\t} else if (skip_prefix(arg, \"--find-object=\", &arg))\n+\t\treturn parse_objfind_opt(options, arg);\n \telse if ((argcount = parse_long_opt(\"diff-filter\", av, &optarg))) {\n \t\tint offending = parse_diff_filter_opt(optarg, options);\n \t\tif (offending)\n@@ -5770,6 +5785,9 @@ void diffcore_std(struct diff_options *options)\n \t\tdiffcore_skip_stat_unmatch(options);\n \tif (!options->found_follow) {\n \t\t/* See try_to_follow_renames() in tree-diff.c */\n+\n+\t\tif (options->objfind)\n+\t\t\tdiffcore_objfind(options);\n \t\tif (options->break_opt != -1)\n \t\t\tdiffcore_break(options->break_opt);\n \t\tif (options->detect_rename)\ndiff --git a/diff.h b/diff.h\nindex 0fb18dd735..8fa2ad8e2d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -7,6 +7,7 @@\n #include \"tree-walk.h\"\n #include \"pathspec.h\"\n #include \"object.h\"\n+#include \"oidset.h\"\n \n struct rev_info;\n struct diff_options;\n@@ -174,6 +175,8 @@ struct diff_options {\n \tenum diff_words_type word_diff;\n \tenum diff_submodule_format submodule_format;\n \n+\tstruct oidset *objfind;\n+\n \t/* this is set by diffcore for DIFF_FORMAT_PATCH */\n \tint found_changes;\n \ndiff --git a/diffcore-objfind.c b/diffcore-objfind.c\nnew file mode 100644\nindex 0000000000..676bbfff00\n--- /dev/null\n+++ b/diffcore-objfind.c\n@@ -0,0 +1,42 @@\n+/*\n+ * Copyright (c) 2017 Google Inc.\n+ */\n+#include \"cache.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n+\n+static void diffcore_filter_blobs(struct diff_queue_struct *q,\n+\t\t\t\t struct diff_options *options)\n+{\n+\tint src, dst;\n+\n+\tif (!options->objfind)\n+\t\tBUG(\"objfind oidset not initialized???\");\n+\n+\tfor (src = dst = 0; src < q->nr; src++) {\n+\t\tstruct diff_filepair *p = q->queue[src];\n+\n+\t\tif (!DIFF_PAIR_UNMERGED(p) &&\n+\t\t ((DIFF_FILE_VALID(p->one) &&\n+\t\t oidset_contains(options->objfind, &p->one->oid)) ||\n+\t\t (DIFF_FILE_VALID(p->two) &&\n+\t\t oidset_contains(options->objfind, &p->two->oid)))) {\n+\t\t\tq->queue[dst] = p;\n+\t\t\tdst++;\n+\t\t} else {\n+\t\t\tdiff_free_filepair(p);\n+\t\t}\n+\t}\n+\n+\tif (!dst) {\n+\t\tfree(q->queue);\n+\t\tDIFF_QUEUE_CLEAR(q);\n+\t} else {\n+\t\tq->nr = dst;\n+\t}\n+}\n+\n+void diffcore_objfind(struct diff_options *options)\n+{\n+\tdiffcore_filter_blobs(&diff_queued_diff, options);\n+}\ndiff --git a/diffcore.h b/diffcore.h\nindex a30da161da..cbde777bdd 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -107,6 +107,7 @@ extern struct diff_filepair *diff_queue(struct diff_queue_struct *,\n \t\t\t\t\tstruct diff_filespec *);\n extern void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n \n+extern void diffcore_objfind(struct diff_options *);\n extern void diffcore_break(int);\n extern void diffcore_rename(struct diff_options *);\n extern void diffcore_merge_broken(void);\ndiff --git a/revision.c b/revision.c\nindex e2e691dd5a..0a797bdfc7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2409,7 +2409,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t/* Pickaxe, diff-filter and rename following need diffs */\n \tif (revs->diffopt.pickaxe ||\n \t revs->diffopt.filter ||\n-\t revs->diffopt.flags.follow_renames)\n+\t revs->diffopt.flags.follow_renames ||\n+\t revs->diffopt.objfind)\n \t\trevs->diff = 1;\n \n \tif (revs->topo_order)\n@@ -2883,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tsimplify_merges(revs);\n \tif (revs->children.name)\n \t\tset_children(revs);\n+\tif (revs->diffopt.objfind)\n+\t\trevs->simplify_history = 0;\n \treturn 0;\n }\n \ndiff --git a/t/t4064-diff-oidfind.sh b/t/t4064-diff-oidfind.sh\nnew file mode 100755\nindex 0000000000..3bdf317af8\n--- /dev/null\n+++ b/t/t4064-diff-oidfind.sh\n@@ -0,0 +1,68 @@\n+#!/bin/sh\n+\n+test_description='test finding specific blobs in the revision walking'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup ' '\n+\tgit commit --allow-empty -m \"empty initial commit\" &&\n+\n+\techo \"Hello, world!\" >greeting &&\n+\tgit add greeting &&\n+\tgit commit -m \"add the greeting blob\" && # borrowed from Git from the Bottom Up\n+\tgit tag -m \"the blob\" greeting $(git rev-parse HEAD:greeting) &&\n+\n+\techo asdf >unrelated &&\n+\tgit add unrelated &&\n+\tgit commit -m \"unrelated history\" &&\n+\n+\tgit revert HEAD^ &&\n+\n+\tgit commit --allow-empty -m \"another unrelated commit\"\n+'\n+\n+test_expect_success 'find the greeting blob' '\n+\tcat >expect <<-EOF &&\n+\tRevert \"add the greeting blob\"\n+\tadd the greeting blob\n+\tEOF\n+\n+\tgit log --format=%s --find-object=greeting^{blob} >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'setup a tree' '\n+\tmkdir a &&\n+\techo asdf >a/file &&\n+\tgit add a/file &&\n+\tgit commit -m \"add a file in a subdirectory\"\n+'\n+\n+test_expect_success 'find a tree' '\n+\tcat >expect <<-EOF &&\n+\tadd a file in a subdirectory\n+\tEOF\n+\n+\tgit log --format=%s -t --find-object=HEAD:a >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'setup a submodule' '\n+\ttest_create_repo sub &&\n+\ttest_commit -C sub sub &&\n+\tgit submodule add ./sub sub &&\n+\tgit commit -a -m \"add sub\"\n+'\n+\n+test_expect_success 'find a submodule' '\n+\tcat >expect <<-EOF &&\n+\tadd sub\n+\tEOF\n+\n+\tgit log --format=%s --find-object=HEAD:sub >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.15.1.504.g5279b80103-goog\n\n"},{"id":"334688","messageId":"xmqqpo7j3i9x.fsf@gitster.mtv.corp.google.com","threadId":"47404","inReplyTo":"20171212012422.123332-1-sbeller@google.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-12T18:36:42Z","receivedAt":"2017-12-12T18:36:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Sometimes users are given a hash of an object and they want to\n> identify it further (ex.: Use verify-pack to find the largest blobs,\n> but what are these? or [1])\n> ...\n> Documentation/diff-options.txt | 6 ++++\n> Makefile | 1 +\n> builtin/log.c | 2 +-\n> diff.c | 20 ++++++++++++-\n> diff.h | 3 ++\n> diffcore-objfind.c | 42 ++++++++++++++++++++++++++\n> diffcore.h | 1 +\n> revision.c | 5 +++-\n> t/t4064-diff-oidfind.sh | 68 ++++++++++++++++++++++++++++++++++++++++++\n> 9 files changed, 145 insertions(+), 3 deletions(-)\n> create mode 100644 diffcore-objfind.c\n> create mode 100755 t/t4064-diff-oidfind.sh\n\nThanks. Will drop an asdf and queue. \n\nI think this is ready for 'next'; I'll wait for a day or two just to\ngive us a final chance to spot and save us from minor embarrassments,\nbut I expect anything we'd spot from here on would be so minor that\nwe can go incremental.\n\n"},{"id":"334843","messageId":"20171214212234.GC32842@aiede.mtv.corp.google.com","threadId":"47404","inReplyTo":"20171212012422.123332-1-sbeller@google.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-12-14T21:22:34Z","receivedAt":"2017-12-14T21:22:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Junio hinted at a different approach of solving this problem, which this\n> patch implements. Teach the diff machinery another flag for restricting\n> the information to what is shown. For example:\n>\n> $ ./git log --oneline --find-object=v2.0.0:Makefile\n> b2feb64309 Revert the whole \"ask curl-config\" topic for now\n> 47fbfded53 i18n: only extract comments marked with \"TRANSLATORS:\"\n>\n> we observe that the Makefile as shipped with 2.0 was appeared in\n> v1.9.2-471-g47fbfded53 and in v2.0.0-rc1-5-gb2feb6430b.\n\nNeat!\n\nNit: s/was appeared/appeared/ (not a big deal though, since this is\nalready in 'next').\n\nFrom the above it's not clear to me whether this is about commits\nwhere the object was added or where the object was removed.\nFortunately, the docs are a bit clearer:\n\n ... one side of the diff\n matches the given object id. The object can be a blob,\n gitlink entry or tree (when `-t` is given).\n\nSo this appears to be about both additions and removals. Followup\nquestions:\n\n- are copies and renames shown (if I am passing -M -C)?\n- what about mode changes? If the file became executable but the\n blob content didn't change, does that commit match?\n\nNit, not related to this change: it would be nice to have a long\noption to go along with the short name '-t' --- e.g. --include-trees.\n\nAnother nit: s/gitlink entry/submodule commit/, perhaps. The commit\nobject is not a gitlink entry: it is pointed to by a gitlink entry.\n\nAnother documentation idea: it may be nice to point out that this\nis only about the preimage and postimage submodule commit and that\nit doesn't look at the history in between.\n\n> The\n> reason why these commits both occur prior to v2.0.0 are evil\n> merges that are not found using this new mechanism.\n\nWould it be worth the doc mentioning that this doesn't look at merges\nunless you use one of the -m/-c/--cc options, or does that go without\nsaying?\n\n[...]\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -500,6 +500,12 @@ information.\n> --pickaxe-regex::\n> \tTreat the given to `-S` as an extended POSIX regular\n> \texpression to match.\n> +\n> +--find-object=::\n> +\tRestrict the output such that one side of the diff\n> +\tmatches the given object id. The object can be a blob,\n> +\tgitlink entry or tree (when `-t` is given).\n\nI like this name --find-object more than --blobfind! I am not sure it\nquite matches what the user is looking for, though. We are not\nlooking for all occurences of the object; we only care about when the\nobject appears (was added or removed) in the diff.\n\nPutting it in context, we have:\n\n\tpickaxe options:\n\t-S: detect changes in occurence count of a string\n\t-G: grep lines in diff for a string\n\n\t--pickaxe-all:\n\t\tdo not filter the diff when the patch matches pickaxe\n\t\tconditions.\n\n\t\tkind of like log --full-diff, but restricted to pickaxe\n\t\toptions.\n\t--pickaxe-regex: treat -S argument as a regex, not a string\n\nIs this another kind of pickaxe option? It feels similar to -S, but\nat an object level instead of a substring level, so in a way it would\nbe appealing to call it --pickaxe-object. Does --pickaxe-all affect\nit like it affects -S and -G?\n\nAnother context to put it in is:\n\n\t--diff-filter:\n\t\tlimit paths (but not commits?) to those with a change\n\t\tmatching optarg\n\nIf I understand correctly, then --diff-filter does not interact with\n--pickaxe-all, or in other words it is a different filtering\ncondition. Is this another kind of diff filter? In that context, it\nmay be appealing to call it something like --object-filter.\n\n--diff-filter is an example where it seems appealing to have a\n--full-diff option to diff-tree that could apply to all filters and\nnot just pickaxe.\n\n[... implementation snipped ...]\n\nThe implementation looks lovely and I'm especially happy about the\ntests. Thanks for writing it.\n\nThoughts?\nJonathan\n"},{"id":"334854","messageId":"CAGZ79kZdUuoM79n09ziG0F7WCWNLpZ2AiFA6fb_qgND1b3_F9A@mail.gmail.com","threadId":"47404","inReplyTo":"20171214212234.GC32842@aiede.mtv.corp.google.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-14T22:30:22Z","receivedAt":"2017-12-14T22:30:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 14, 2017 at 1:22 PM, Jonathan Nieder wrote:\n\n> - what about mode changes? If the file became executable but the\n> blob content didn't change, does that commit match?\n\n./git log --find-object=$(git rev-parse ba67504f:t/perf/p3400-rebase.sh)\n\nclaims it does find the mode change (commit ba67504f is just a mode\nchange)\n\n> - are copies and renames shown (if I am passing -M -C)?\n\nIt restricts the commits shown, not the renamed files. But I assume\nyou mean it the same way as with mode changes.\nI did not find a good commit in gits history to demonstrate, but as\nit is orthogonal to the object id restrictions, I would think it works\n\n> Nit, not related to this change: it would be nice to have a long\n> option to go along with the short name '-t' --- e.g. --include-trees.\n\nfollow up patches welcome. :)\n\n>\n> Another nit: s/gitlink entry/submodule commit/, perhaps. The commit\n> object is not a gitlink entry: it is pointed to by a gitlink entry.\n\nWell, what if the user doesn't have a submodule, but uses gitlinks\nfor other purposes? We do inspect the gitlink, so it is correct IMHO.\n\n> Another documentation idea: it may be nice to point out that this\n> is only about the preimage and postimage submodule commit and that\n> it doesn't look at the history in between.\n\nThat is sensible. One might be tempted to ask: \"Which superproject\ncommit contains a submodule pointer, that has commit $X in the\nsubmodule history?\", but this new option is totally not answering this.\n\n>> The\n>> reason why these commits both occur prior to v2.0.0 are evil\n>> merges that are not found using this new mechanism.\n>\n> Would it be worth the doc mentioning that this doesn't look at merges\n> unless you use one of the -m/-c/--cc options, or does that go without\n> saying?\n\nI assumed it goes without saying, just like the lacking -t could mean\nto ignore trees. ;)\n\n\n>\n> [...]\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -500,6 +500,12 @@ information.\n>> --pickaxe-regex::\n>> Treat the given to `-S` as an extended POSIX regular\n>> expression to match.\n>> +\n>> +--find-object=::\n>> + Restrict the output such that one side of the diff\n>> + matches the given object id. The object can be a blob,\n>> + gitlink entry or tree (when `-t` is given).\n>\n> I like this name --find-object more than --blobfind! I am not sure it\n> quite matches what the user is looking for, though. We are not\n> looking for all occurences of the object; we only care about when the\n> object appears (was added or removed) in the diff.\n\nThanks! Yes, but the 'edges' are so few commits that a further walk\nwill reveal all you need to know?\n\n\n>\n> Putting it in context, we have:\n>\n> pickaxe options:\n> -S: detect changes in occurence count of a string\n> -G: grep lines in diff for a string\n>\n> --pickaxe-all:\n> do not filter the diff when the patch matches pickaxe\n> conditions.\n>\n> kind of like log --full-diff, but restricted to pickaxe\n> options.\n> --pickaxe-regex: treat -S argument as a regex, not a string\n>\n> Is this another kind of pickaxe option? It feels similar to -S, but\n> at an object level instead of a substring level, so in a way it would\n> be appealing to call it --pickaxe-object. Does --pickaxe-all affect\n> it like it affects -S and -G?\n>\n> Another context to put it in is:\n>\n> --diff-filter:\n> limit paths (but not commits?) to those with a change\n> matching optarg\n>\n> If I understand correctly, then --diff-filter does not interact with\n> --pickaxe-all, or in other words it is a different filtering\n> condition. Is this another kind of diff filter? In that context, it\n> may be appealing to call it something like --object-filter.\n>\n> --diff-filter is an example where it seems appealing to have a\n> --full-diff option to diff-tree that could apply to all filters and\n> not just pickaxe.\n>\n> [... implementation snipped ...]\n>\n> The implementation looks lovely and I'm especially happy about the\n> tests. Thanks for writing it.\n>\n> Thoughts?\n> Jonathan\n\nRegarding finding a better name, I would want to hear from others,\nI am happy with --find-object, though I can see --pickaxe-object\nor --object--filter to be a good narrative as well.\n\nStefan\n"},{"id":"334855","messageId":"xmqqefnxrktm.fsf@gitster.mtv.corp.google.com","threadId":"47404","inReplyTo":"20171214212234.GC32842@aiede.mtv.corp.google.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-14T22:44:37Z","receivedAt":"2017-12-14T22:44:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Putting it in context, we have:\n>\n> \tpickaxe options:\n> \t-S: detect changes in occurence count of a string\n> \t-G: grep lines in diff for a string\n>\n> \t--pickaxe-all:\n> \t\tdo not filter the diff when the patch matches pickaxe\n> \t\tconditions.\n>\n> \t\tkind of like log --full-diff, but restricted to pickaxe\n> \t\toptions.\n> \t--pickaxe-regex: treat -S argument as a regex, not a string\n>\n> Is this another kind of pickaxe option? It feels similar to -S, but\n> at an object level instead of a substring level, so in a way it would\n> be appealing to call it --pickaxe-object. Does --pickaxe-all affect\n> it like it affects -S and -G?\n\nIt does feel quite close to -S, but is slightly different.\n\nInstead of spelling out the exact contents to pass as the argument\nto the -S option, you are giving a blob object name instead.\n\nHowever, if you rename path F to G without any modification, or if\nyou flip only the mode bits, -S would reject such a filepair, as\nboth sides have the same number of the sought-after string, I think.\nUnlike -S, this new thing should show such a filepair because it is\ndesigned to show any filepair with either (or both) side has the\nsought-after object.\n\nWe could make this identical to -S, i.e. only when a filepair has\nsought-after object on either (or both) sides *and* its pre and post\nimage sides do not talk about the same object.\n\n Note. I am phrasing the above awkwardly because we can ask for\n more than one object, and a filepair that changes a path from\n object A to object B, both of which are what we are looking for,\n want to be caught. If I phrased the above as \"only when only\n one side of a filepair has an object we are looking for and the\n other side does not\", such a filepair would not be shown.\n\nI have a suspicion that such a change may be worthwhile.\n\n> Another context to put it in is:\n>\n> \t--diff-filter:\n> \t\tlimit paths (but not commits?) to those with a change\n> \t\tmatching optarg\n\nIIUC, this is applied at the output phase after all of the rename,\npickaxe etc. are done (e.g. \"Now you have the diff to give me, show\nme only the additions in it\"). It is a poor match for this new\nthing, as \"Now you have the diff to give me, show me only the ones\nthat involve these blobs\" does not sound all that useful.\n\n\n\n"},{"id":"334856","messageId":"20171214225200.GA44616@aiede.mtv.corp.google.com","threadId":"47404","inReplyTo":"CAGZ79kZdUuoM79n09ziG0F7WCWNLpZ2AiFA6fb_qgND1b3_F9A@mail.gmail.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-12-14T22:52:00Z","receivedAt":"2017-12-14T22:52:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n> On Thu, Dec 14, 2017 at 1:22 PM, Jonathan Nieder wrote:\n\n>> - what about mode changes? If the file became executable but the\n>> blob content didn't change, does that commit match?\n>\n> ./git log --find-object=$(git rev-parse ba67504f:t/perf/p3400-rebase.sh)\n>\n> claims it does find the mode change (commit ba67504f is just a mode\n> change)\n\nThanks. Reminder to self to add a test + docs about that (as a followup\nchange; this isn't a complaint about the patch).\n\n>> - are copies and renames shown (if I am passing -M -C)?\n>\n> It restricts the commits shown, not the renamed files. But I assume\n> you mean it the same way as with mode changes.\n> I did not find a good commit in gits history to demonstrate, but as\n> it is orthogonal to the object id restrictions, I would think it works\n\nOk, will add test + doc.\n\n>> Nit, not related to this change: it would be nice to have a long\n>> option to go along with the short name '-t' --- e.g. --include-trees.\n>\n> follow up patches welcome. :)\n\nWill think more and try to send a patch if it still seems like a good\nidea in a day or so. ;)\n\n>> Another nit: s/gitlink entry/submodule commit/, perhaps. The commit\n>> object is not a gitlink entry: it is pointed to by a gitlink entry.\n>\n> Well, what if the user doesn't have a submodule, but uses gitlinks\n> for other purposes? We do inspect the gitlink, so it is correct IMHO.\n\nIt's a language nit. The argument to --find-object is a commit object\nname, not a gitlink entry. A gitlink entry looks like\n\n 160000 \n\n>> Another documentation idea: it may be nice to point out that this\n>> is only about the preimage and postimage submodule commit and that\n>> it doesn't look at the history in between.\n>\n> That is sensible. One might be tempted to ask: \"Which superproject\n> commit contains a submodule pointer, that has commit $X in the\n> submodule history?\", but this new option is totally not answering this.\n\nOk, will try to come up with wording.\n\n>>> The\n>>> reason why these commits both occur prior to v2.0.0 are evil\n>>> merges that are not found using this new mechanism.\n>>\n>> Would it be worth the doc mentioning that this doesn't look at merges\n>> unless you use one of the -m/-c/--cc options, or does that go without\n>> saying?\n>\n> I assumed it goes without saying, just like the lacking -t could mean\n> to ignore trees. ;)\n\nI suspect it's worth a mention, based on the discussion in this thread\n(i.e. without such docs it was non-obvious and took some time to\ndiagnose).\n\n[...]\n>>> +--find-object=::\n>>> + Restrict the output such that one side of the diff\n>>> + matches the given object id. The object can be a blob,\n>>> + gitlink entry or tree (when `-t` is given).\n>>\n>> I like this name --find-object more than --blobfind! I am not sure it\n>> quite matches what the user is looking for, though. We are not\n>> looking for all occurences of the object; we only care about when the\n>> object appears (was added or removed) in the diff.\n>\n> Thanks! Yes, but the 'edges' are so few commits that a further walk\n> will reveal all you need to know?\n\nSorry for the lack of clarity: I actually like this behavior *more*\nthan a \"find trees pointing to object\" behavior. I'm just saying the\nname sets an unclear expectation.\n\n[...]\n> Regarding finding a better name, I would want to hear from others,\n> I am happy with --find-object, though I can see --pickaxe-object\n> or --object--filter to be a good narrative as well.\n\nDrat, I was hoping for an opinion.\n\nBased on the answers above about mode changes and renames, at the\nmoment --object-filter seems clearest to me.\n\nThanks,\nJonathan\n"},{"id":"334867","messageId":"xmqq374cspgw.fsf@gitster.mtv.corp.google.com","threadId":"47404","inReplyTo":"20171214225200.GA44616@aiede.mtv.corp.google.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-15T02:18:55Z","receivedAt":"2017-12-15T02:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> Regarding finding a better name, I would want to hear from others,\n>> I am happy with --find-object, though I can see --pickaxe-object\n>> or --object--filter to be a good narrative as well.\n>\n> Drat, I was hoping for an opinion.\n\nI think it would make it a better companion to --pickaxe but we need\nto align its behaviour a little bit so that it plays better with the\n\"--pickaxe-all\" option, and also needs to hide mode and name only\nchanges just like pickaxe. \n\nAfter all, the diffcore-blobfind code was written while looking at\nthe diffcore-pickaxe's code in another window shown in the pager,\nand I tend to agree with your earlier message that this is an\nextreme case of -S where the contents happens to be the\nwhole file.\n"},{"id":"335378","messageId":"CAGZ79kYiL0St-600sjxN5gjLSfyRodS1=AqxyALsidntXkg_xA@mail.gmail.com","threadId":"47404","inReplyTo":"xmqq374cspgw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-27T18:49:00Z","receivedAt":"2017-12-27T18:49:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 14, 2017 at 6:18 PM, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n>>> Regarding finding a better name, I would want to hear from others,\n>>> I am happy with --find-object, though I can see --pickaxe-object\n>>> or --object--filter to be a good narrative as well.\n>>\n>> Drat, I was hoping for an opinion.\n>\n> I think it would make it a better companion to --pickaxe but we need\n> to align its behaviour a little bit so that it plays better with the\n> \"--pickaxe-all\" option, and also needs to hide mode and name only\n> changes just like pickaxe.\n\nI looked into this, and the small changes needed led me to thinking\nit could be integrated into the diffcore-pickaxe code completely,\nroughly like (spaces mangled):\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 9476bd2108..46f875a7b4 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair\n*p, struct diff_options *o,\n mmfile_t mf1, mf2;\n int ret;\n\n- if (!o->pickaxe[0])\n- return 0;\n-\n /* ignore unmerged */\n if (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))\n return 0;\n\n+ if (options->objfind) {\n+ if ((DIFF_FILE_VALID(p->one) &&\n+ oidset_contains(options->objfind, &p->one->oid)) ||\n+ (DIFF_FILE_VALID(p->two) &&\n+ oidset_contains(options->objfind, &p->two->oid)))\n+ return 1;\n+ }\n+\n+ if (!o->pickaxe[0])\n+ return 0;\n+\n if (o->flags.allow_textconv) {\n textconv_one = get_textconv(p->one);\n textconv_two = get_textconv(p->two);\n---8<---\n\nBut then, it seems as if any pickaxe option is incompatible with\nany other, i.e. from reading the code, you cannot combine -S\nand -G, or even give one of them twice.\n\nI guess that would be not a big deal for the --pickaxe-object,\nbut just want to point it out.\n\n> After all, the diffcore-blobfind code was written while looking at\n> the diffcore-pickaxe's code in another window shown in the pager,\n> and I tend to agree with your earlier message that this is an\n> extreme case of -S where the contents happens to be the\n> whole file.\n\nI disagree, as the user doesn't have the content, but the hash\nover the content only and wants to know more about it. The new\noption cannot be used to find a file whose partial content hashes to\nthe given sha1, either.\n\nSo with these considerations, I would keep the patch as currently\\\nqueued at sb/diff-blobfind.\n\nThanks,\nStefan\n"},{"id":"335383","messageId":"20171227185907.GE149622@aiede.mtv.corp.google.com","threadId":"47404","inReplyTo":"CAGZ79kYiL0St-600sjxN5gjLSfyRodS1=AqxyALsidntXkg_xA@mail.gmail.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-12-27T18:59:07Z","receivedAt":"2017-12-27T18:59:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Thu, Dec 14, 2017 at 6:18 PM, Junio C Hamano wrote:\n\n>> I think it would make it a better companion to --pickaxe but we need\n>> to align its behaviour a little bit so that it plays better with the\n>> \"--pickaxe-all\" option, and also needs to hide mode and name only\n>> changes just like pickaxe.\n>\n> I looked into this, and the small changes needed led me to thinking\n> it could be integrated into the diffcore-pickaxe code completely,\n> roughly like (spaces mangled):\n\nNice, this looks promising.\n\n[...]\n> But then, it seems as if any pickaxe option is incompatible with\n> any other, i.e. from reading the code, you cannot combine -S\n> and -G, or even give one of them twice.\n>\n> I guess that would be not a big deal for the --pickaxe-object,\n> but just want to point it out.\n\nAgreed that that's not a big deal for --pickaxe-object.\n\n>> After all, the diffcore-blobfind code was written while looking at\n>> the diffcore-pickaxe's code in another window shown in the pager,\n>> and I tend to agree with your earlier message that this is an\n>> extreme case of -S where the contents happens to be the\n>> whole file.\n>\n> I disagree, as the user doesn't have the content, but the hash\n> over the content only and wants to know more about it. The new\n> option cannot be used to find a file whose partial content hashes to\n> the given sha1, either.\n>\n> So with these considerations, I would keep the patch as currently\\\n> queued at sb/diff-blobfind.\n\nInteresting --- I come to the opposite conclusion.\n\nThe pickaxe-style behavior seems more consistent and simpler to\nexplain and better matches the use cases I can think of.\n\nThanks,\nJonathan\n"},{"id":"335393","messageId":"xmqqefnglz9y.fsf@gitster.mtv.corp.google.com","threadId":"47404","inReplyTo":"20171227185907.GE149622@aiede.mtv.corp.google.com","subject":"Re: [PATCH] diffcore: add a filter to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-27T19:57:29Z","receivedAt":"2017-12-27T19:57:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Stefan Beller wrote:\n>> On Thu, Dec 14, 2017 at 6:18 PM, Junio C Hamano wrote:\n>\n>>> I think it would make it a better companion to --pickaxe but we need\n>>> to align its behaviour a little bit so that it plays better with the\n>>> \"--pickaxe-all\" option, and also needs to hide mode and name only\n>>> changes just like pickaxe.\n>>\n>> I looked into this, and the small changes needed led me to thinking\n>> it could be integrated into the diffcore-pickaxe code completely,\n>> roughly like (spaces mangled):\n>\n> Nice, this looks promising.\n\nIndeed it is very simple, straight-forward and nice ;-)\n\n>> I disagree, as the user doesn't have the content, but the hash\n>> over the content only and wants to know more about it....\n\n(correcting Stefan) The user can do \"git cat-file blob $oid\" to\nlearn about the contents, so from end user's point of view, the\n\"pickaxe with object ID\" can be seen as merely a short-hand for\n\n git log -S\"$(git cat-file blob $oid)\"\n\nalmost.\n\nBut that is a tangent in this response.\n\n> Interesting --- I come to the opposite conclusion.\n>\n> The pickaxe-style behavior seems more consistent and simpler to\n> explain and better matches the use cases I can think of.\n\nYeah, I am more leaning toward agreeing with you.\n"}]}