{"thread":{"id":"47500","subject":"[PATCH 1/1] diffcore: add a pickaxe option to find a specific blob","startedAt":"2017-12-28T00:00:21Z","lastAt":"2017-12-28T21:43:42Z","messageCount":4,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"335414","messageId":"20171228000003.34539-2-sbeller@google.com","threadId":"47500","inReplyTo":"20171228000003.34539-1-sbeller@google.com","subject":"[PATCH 1/1] diffcore: add a pickaxe option to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-28T00:00:03Z","receivedAt":"2017-12-28T00:00:21Z","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 <blob-id>` gives a description as\n'<commit-ish>:<path>'.  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 <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/diff-options.txt | 10 +++++++\n diff.c                         | 21 ++++++++++++-\n diff.h                         |  3 ++\n diffcore-pickaxe.c             | 14 +++++++--\n revision.c                     |  3 ++\n t/t4064-diff-oidfind.sh        | 68 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 115 insertions(+), 4 deletions(-)\n create mode 100755 t/t4064-diff-oidfind.sh\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex dd0dba5b1d..f9cf85db05 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -492,6 +492,15 @@ occurrences of that string did not change).\n See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more\n information.\n \n+--find-object=<object-id>::\n+\tLook for differences that change the number of occurrences of\n+\tthe specified object. Similar to `-S`, just the argument is different\n+\tin that it doesn't search for a specific string but for a specific\n+\tobject id.\n++\n+The object can be a blob or a submodule commit. It implies the `-t` option in\n+`git-log` to also find trees.\n+\n --pickaxe-all::\n \tWhen `-S` or `-G` finds a change, show all the changes in that\n \tchangeset, not just the files that contain the change\n@@ -500,6 +509,7 @@ information.\n --pickaxe-regex::\n \tTreat the <string> given to `-S` as an extended POSIX regular\n \texpression to match.\n+\n endif::git-format-patch[]\n \n -O<orderfile>::\ndiff --git a/diff.c b/diff.c\nindex 0763e89263..7acddaaee7 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,23 @@ 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+\n+\topt->pickaxe = \"\"; /* trigger pickaxe */\n+\topt->flags.recursive = 1;\n+\topt->flags.tree_in_recursive = 1;\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 +4754,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)\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-pickaxe.c b/diffcore-pickaxe.c\nindex 9476bd2108..0d0c697ae7 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n \tmmfile_t mf1, mf2;\n \tint ret;\n \n-\tif (!o->pickaxe[0])\n-\t\treturn 0;\n-\n \t/* ignore unmerged */\n \tif (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))\n \t\treturn 0;\n \n+\tif (o->objfind) {\n+\t\tif ((DIFF_FILE_VALID(p->one) &&\n+\t\t     oidset_contains(o->objfind, &p->one->oid)) ||\n+\t\t    (DIFF_FILE_VALID(p->two) &&\n+\t\t     oidset_contains(o->objfind, &p->two->oid)))\n+\t\t\treturn 1;\n+\t}\n+\n+\tif (!o->pickaxe[0])\n+\t\treturn 0;\n+\n \tif (o->flags.allow_textconv) {\n \t\ttextconv_one = get_textconv(p->one);\n \t\ttextconv_two = get_textconv(p->two);\ndiff --git a/revision.c b/revision.c\nindex e2e691dd5a..43bb6265cd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2412,6 +2412,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t    revs->diffopt.flags.follow_renames)\n \t\trevs->diff = 1;\n \n+\tif (revs->diffopt.objfind)\n+\t\trevs->simplify_history = 0;\n+\n \tif (revs->topo_order)\n \t\trevs->limited = 1;\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.620.gb9897f4670-goog\n\n"},{"id":"335415","messageId":"20171228000003.34539-1-sbeller@google.com","threadId":"47500","inReplyTo":null,"subject":"[PATCH 0/1] Add blob find pickaxe option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-28T00:00:02Z","receivedAt":"2017-12-28T00:00:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is a resend for sb/diff-blobfind, but now the option --find-object\nis a native of the pickaxe family.\n\nStefan Beller (1):\n  diffcore: add a pickaxe option to find a specific blob\n\n Documentation/diff-options.txt | 10 +++++++\n diff.c                         | 21 ++++++++++++-\n diff.h                         |  3 ++\n diffcore-pickaxe.c             | 14 +++++++--\n revision.c                     |  3 ++\n t/t4064-diff-oidfind.sh        | 68 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 115 insertions(+), 4 deletions(-)\n create mode 100755 t/t4064-diff-oidfind.sh\n\n-- \n2.15.1.620.gb9897f4670-goog\n\ndiff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt\nindex 21d1776996..f9cf85db05 100644\n--- c/Documentation/diff-options.txt\n+++ w/Documentation/diff-options.txt\n@@ -492,6 +492,15 @@ occurrences of that string did not change).\n See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more\n information.\n \n+--find-object=<object-id>::\n+\tLook for differences that change the number of occurrences of\n+\tthe specified object. Similar to `-S`, just the argument is different\n+\tin that it doesn't search for a specific string but for a specific\n+\tobject id.\n++\n+The object can be a blob or a submodule commit. It implies the `-t` option in\n+`git-log` to also find trees.\n+\n --pickaxe-all::\n \tWhen `-S` or `-G` finds a change, show all the changes in that\n \tchangeset, not just the files that contain the change\n@@ -501,11 +510,6 @@ information.\n \tTreat the <string> given to `-S` as an extended POSIX regular\n \texpression to match.\n \n---find-object=<object-id>::\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<orderfile>::\ndiff --git c/Makefile w/Makefile\nindex c26596c30a..ee9d5eb11e 100644\n--- c/Makefile\n+++ w/Makefile\n@@ -775,7 +775,6 @@ 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 c/builtin/log.c w/builtin/log.c\nindex 08ea82d69f..6c1fa896ad 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.objfind)\n+\t    rev->diffopt.flags.follow_renames)\n \t\trev->always_show_header = 0;\n \n \tif (source)\ndiff --git c/diff.c w/diff.c\nindex e13b8229d3..7acddaaee7 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -4497,6 +4497,10 @@ static int parse_objfind_opt(struct diff_options *opt, const char *arg)\n \n \tif (!opt->objfind)\n \t\topt->objfind = xcalloc(1, sizeof(*opt->objfind));\n+\n+\topt->pickaxe = \"\"; /* trigger pickaxe */\n+\topt->flags.recursive = 1;\n+\topt->flags.tree_in_recursive = 1;\n \toidset_insert(opt->objfind, &oid);\n \treturn 1;\n }\n@@ -5785,9 +5789,6 @@ 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 c/diffcore-objfind.c w/diffcore-objfind.c\ndeleted file mode 100644\nindex 676bbfff00..0000000000\n--- c/diffcore-objfind.c\n+++ /dev/null\n@@ -1,42 +0,0 @@\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 c/diffcore-pickaxe.c w/diffcore-pickaxe.c\nindex 9476bd2108..0d0c697ae7 100644\n--- c/diffcore-pickaxe.c\n+++ w/diffcore-pickaxe.c\n@@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n \tmmfile_t mf1, mf2;\n \tint ret;\n \n-\tif (!o->pickaxe[0])\n-\t\treturn 0;\n-\n \t/* ignore unmerged */\n \tif (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))\n \t\treturn 0;\n \n+\tif (o->objfind) {\n+\t\tif ((DIFF_FILE_VALID(p->one) &&\n+\t\t     oidset_contains(o->objfind, &p->one->oid)) ||\n+\t\t    (DIFF_FILE_VALID(p->two) &&\n+\t\t     oidset_contains(o->objfind, &p->two->oid)))\n+\t\t\treturn 1;\n+\t}\n+\n+\tif (!o->pickaxe[0])\n+\t\treturn 0;\n+\n \tif (o->flags.allow_textconv) {\n \t\ttextconv_one = get_textconv(p->one);\n \t\ttextconv_two = get_textconv(p->two);\ndiff --git c/diffcore.h w/diffcore.h\nindex cbde777bdd..a30da161da 100644\n--- c/diffcore.h\n+++ w/diffcore.h\n@@ -107,7 +107,6 @@ 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 c/revision.c w/revision.c\nindex 0a797bdfc7..43bb6265cd 100644\n--- c/revision.c\n+++ w/revision.c\n@@ -2409,10 +2409,12 @@ 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.objfind)\n+\t    revs->diffopt.flags.follow_renames)\n \t\trevs->diff = 1;\n \n+\tif (revs->diffopt.objfind)\n+\t\trevs->simplify_history = 0;\n+\n \tif (revs->topo_order)\n \t\trevs->limited = 1;\n \n@@ -2884,8 +2886,6 @@ 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 c/t/helper/test-lazy-init-name-hash.c w/t/helper/test-lazy-init-name-hash.c\nindex 6368a89345..297fb01d61 100644\n--- c/t/helper/test-lazy-init-name-hash.c\n+++ w/t/helper/test-lazy-init-name-hash.c\n@@ -112,7 +112,7 @@ static void analyze_run(void)\n {\n \tuint64_t t1s, t1m, t2s, t2m;\n \tint cache_nr_limit;\n-\tint nr_threads_used;\n+\tint nr_threads_used = 0;\n \tint i;\n \tint nr;\n \n"},{"id":"335495","messageId":"xmqqy3lmill0.fsf@gitster.mtv.corp.google.com","threadId":"47500","inReplyTo":"20171228000003.34539-2-sbeller@google.com","subject":"Re: [PATCH 1/1] diffcore: add a pickaxe option to find a specific blob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-28T21:33:47Z","receivedAt":"2017-12-28T21:33:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\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> +\n> +\topt->pickaxe = \"\"; /* trigger pickaxe */\n\nHmmm.  This feels like an ugly hack to me.  Do we declare that \"git\nlog -S''\" never matches anything right now (if that is the case the\nI do not think there is any compatibility issues)?\n\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index 9476bd2108..0d0c697ae7 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n>  \tmmfile_t mf1, mf2;\n>  \tint ret;\n>  \n> -\tif (!o->pickaxe[0])\n> -\t\treturn 0;\n> -\n>  \t/* ignore unmerged */\n>  \tif (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))\n>  \t\treturn 0;\n>  \n> +\tif (o->objfind) {\n> +\t\tif ((DIFF_FILE_VALID(p->one) &&\n> +\t\t     oidset_contains(o->objfind, &p->one->oid)) ||\n> +\t\t    (DIFF_FILE_VALID(p->two) &&\n> +\t\t     oidset_contains(o->objfind, &p->two->oid)))\n> +\t\t\treturn 1;\n> +\t}\n> +\n> +\tif (!o->pickaxe[0])\n> +\t\treturn 0;\n> +\n\nVery nice.  With just one place, the code covers both cases with and\nwithout pickaxe-all option in effect.\n\n"},{"id":"335496","messageId":"CAGZ79kaB0G9zetF6QtC45+ZGLM3gOsYWV7e+gkCe2yKOhb0Ssg@mail.gmail.com","threadId":"47500","inReplyTo":"xmqqy3lmill0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1] diffcore: add a pickaxe option to find a specific blob","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-28T21:43:36Z","receivedAt":"2017-12-28T21:43:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 28, 2017 at 1:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +static int parse_objfind_opt(struct diff_options *opt, const char *arg)\n>> +{\n>> +     struct object_id oid;\n>> +\n>> +     if (get_oid(arg, &oid))\n>> +             return error(\"unable to resolve '%s'\", arg);\n>> +\n>> +     if (!opt->objfind)\n>> +             opt->objfind = xcalloc(1, sizeof(*opt->objfind));\n>> +\n>> +     opt->pickaxe = \"\"; /* trigger pickaxe */\n>\n> Hmmm.  This feels like an ugly hack to me.\n\nBecause it is.\n\n$ git grep pickaxe diff.h\ndiff.h:94:      unsigned pickaxe_ignore_case:1;\ndiff.h:130:     const char *pickaxe;\ndiff.h:149:     int pickaxe_opts;\ndiff.h:361:\"  --pickaxe-all\\n\" \\\n\nWe could promote pickaxe_opts to be the important part\nfor pickaxing, currently we're using `pickaxe` in diff.c to\nmake a decision:\n\n    if (options->pickaxe)\n        diffcore_pickaxe(options);\n\nSo I think I'll cleanup the pickaxe_opts to be a real\nunsigned flags field (such that pickaxe_ignore_case\ncould be part of it, too!) and put a new flag in\nthere in addition to DIFF_PICKAXE_KIND_{G, S}\n\n> Do we declare that \"git\n> log -S''\" never matches anything right now (if that is the case the\n> I do not think there is any compatibility issues)?\n\nLet's not go this route.\n\n\n>\n>> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n>> index 9476bd2108..0d0c697ae7 100644\n>> --- a/diffcore-pickaxe.c\n>> +++ b/diffcore-pickaxe.c\n>> @@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair *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 (o->objfind) {\n>> +             if ((DIFF_FILE_VALID(p->one) &&\n>> +                  oidset_contains(o->objfind, &p->one->oid)) ||\n>> +                 (DIFF_FILE_VALID(p->two) &&\n>> +                  oidset_contains(o->objfind, &p->two->oid)))\n>> +                     return 1;\n>> +     }\n>> +\n>> +     if (!o->pickaxe[0])\n>> +             return 0;\n>> +\n>\n> Very nice.  With just one place, the code covers both cases with and\n> without pickaxe-all option in effect.\n\nand this is already fixed regarding the ugly hack, too.\n\nThanks,\nStefan\n"}]}