{"thread":{"id":"47260","subject":"[PATCH 0/1] Teaching the diff machinery about blobfind [WAS: git describe <blob>]","startedAt":"2017-11-20T22:25:39Z","lastAt":"2017-12-07T21:40:36Z","messageCount":5,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"333012","messageId":"20171120222529.24995-1-sbeller@google.com","threadId":"47260","inReplyTo":null,"subject":"[PATCH 0/1] Teaching the diff machinery about blobfind [WAS: git describe <blob>]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-20T22:25:28Z","receivedAt":"2017-11-20T22:25:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This seems to be an easier approach; thanks Junio for hinting at it.\nThis certainly solves our immediate needs, we may want to build\n'git describe <blob>' on top of it or defer it until later.\n\nThanks,\nStefan\n\nprevious descussion\nhttps://public-inbox.org/git/20171028004419.10139-1-sbeller@google.com/\n\nStefan Beller (1):\n  diffcore: add a filter to find a specific blob\n\n Documentation/diff-options.txt |  4 ++++\n Makefile                       |  1 +\n builtin/log.c                  |  2 +-\n diff.c                         | 20 ++++++++++++++++-\n diff.h                         |  3 +++\n diffcore-blobfind.c            | 51 ++++++++++++++++++++++++++++++++++++++++++\n diffcore.h                     |  1 +\n revision.c                     |  3 ++-\n t/t4064-diff-blobfind.sh       | 35 +++++++++++++++++++++++++++++\n 9 files changed, 117 insertions(+), 3 deletions(-)\n create mode 100644 diffcore-blobfind.c\n create mode 100755 t/t4064-diff-blobfind.sh\n\n-- \n2.15.0.128.gcadd42da22\n\n"},{"id":"333013","messageId":"20171120222529.24995-2-sbeller@google.com","threadId":"47260","inReplyTo":"20171120222529.24995-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-11-20T22:25:29Z","receivedAt":"2017-11-20T22:25:43Z","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 --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 <sbeller@google.com>\n---\n\nOn playing around with this, trying to find more interesting cases, I observed:\n\n    git log --oneline --blobfind=HEAD:COPYING\n    703601d678 Update COPYING with GPLv2 with new FSF address\n    \n    git log --oneline --blobfind=703601d678^:COPYING\n    459b8d22e5 tests: do not borrow from COPYING and README from the real source\n    703601d678 Update COPYING with GPLv2 with new FSF address\n    075b845a85 Add a COPYING notice, making it explicit that the license is GPLv2.\n\n    t/diff-lib/COPYING may need an update of the adress of the FSF,\n    #leftoverbits I guess.\n    \nAnother interesting case that I found was\n   git log --oneline --blobfind=v2.14.0:Makefile\n   3921a0b3c3 perf: add test for writing the index\n   36f048c5e4 sha1dc: build git plumbing code more explicitly\n   2118805b92 Makefile: add style build rule\n\nall of which were after v2.14, such that the introduction of that blob doesn't\nshow up; I suspect it came in via a merge as unrelated series may have updated\nthe Makefile in parallel, though git-log should have told me?\n\nThanks,\nStefan\n\n Documentation/diff-options.txt |  4 ++++\n Makefile                       |  1 +\n builtin/log.c                  |  2 +-\n diff.c                         | 20 ++++++++++++++++-\n diff.h                         |  3 +++\n diffcore-blobfind.c            | 51 ++++++++++++++++++++++++++++++++++++++++++\n diffcore.h                     |  1 +\n revision.c                     |  3 ++-\n t/t4064-diff-blobfind.sh       | 35 +++++++++++++++++++++++++++++\n 9 files changed, 117 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..252a21cc19 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -500,6 +500,10 @@ information.\n --pickaxe-regex::\n \tTreat the <string> given to `-S` as an extended POSIX regular\n \texpression to match.\n+--blobfind=<blob-id>::\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<orderfile>::\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..5d222fc336\n--- /dev/null\n+++ b/diffcore-blobfind.c\n@@ -0,0 +1,51 @@\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 i, j = 0, c = q->nr;\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+\n+\t\tif (DIFF_PAIR_UNMERGED(p) ||\n+\t\t    (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}\n+\n+\tq->nr = c;\n+\n+\tif (!c) {\n+\t\tfree(q->queue);\n+\t\tDIFF_QUEUE_CLEAR(q);\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..6449619c0a 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)\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.0.128.gcadd42da22\n\n"},{"id":"333422","messageId":"xmqqpo88m896.fsf@gitster.mtv.corp.google.com","threadId":"47260","inReplyTo":"20171120222529.24995-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-11-24T07:43:49Z","receivedAt":"2017-11-24T07:44:01Z","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> 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 <blob-id>` gives a description as\n> '<commit-ish>:<path>'.  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>\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> [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>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> On playing around with this, trying to find more interesting cases, I observed:\n>\n>     git log --oneline --blobfind=HEAD:COPYING\n>     703601d678 Update COPYING with GPLv2 with new FSF address\n>     \n>     git log --oneline --blobfind=703601d678^:COPYING\n>     459b8d22e5 tests: do not borrow from COPYING and README from the real source\n>     703601d678 Update COPYING with GPLv2 with new FSF address\n>     075b845a85 Add a COPYING notice, making it explicit that the license is GPLv2.\n>\n>     t/diff-lib/COPYING may need an update of the adress of the FSF,\n>     # leftoverbits I guess.\n\nI do not think so.  See tz/fsf-address-update topic for details.\n\nPlease do not contaminate the list archive with careless mention of \n\"hash-mark plus left over bits\", as it will make searching the real\ngood bits harder.  Thanks.\n\n> Another interesting case that I found was\n>    git log --oneline --blobfind=v2.14.0:Makefile\n>    3921a0b3c3 perf: add test for writing the index\n>    36f048c5e4 sha1dc: build git plumbing code more explicitly\n>    2118805b92 Makefile: add style build rule\n>\n> all of which were after v2.14, such that the introduction of that blob doesn't\n> show up; I suspect it came in via a merge as unrelated series may have updated\n> the Makefile in parallel, though git-log should have told me?\n\nIf that is the case, shouldn't we make this new mode imply\n--full-history to forbid history simplification?  \"git log\" is a\ntool to find _an_ explanation of the current state, and the usual\nhistory simplification makes tons of sense there, but blobfind is\nrun most likely in order to find _all_ mention of the set of blobs\ngiven.\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index dd0dba5b1d..252a21cc19 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -500,6 +500,10 @@ information.\n>  --pickaxe-regex::\n>  \tTreat the <string> given to `-S` as an extended POSIX regular\n>  \texpression to match.\n> +--blobfind=<blob-id>::\n> +\tRestrict the output such that one side of the diff\n> +\tmatches the given blob-id.\n> +\n>  endif::git-format-patch[]\n\nCan we have a blank line between these enumerations to make the\nsource easier to read?  Thanks.\n\n> diff --git a/diffcore-blobfind.c b/diffcore-blobfind.c\n> new file mode 100644\n> index 0000000000..5d222fc336\n> --- /dev/null\n> +++ b/diffcore-blobfind.c\n> @@ -0,0 +1,51 @@\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 i, j = 0, c = q->nr;\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> +\n> +\t\tif (DIFF_PAIR_UNMERGED(p) ||\n> +\t\t    (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\nSo, we keep an unmerged pair, a pair that mentions a sought-blob on\none side or the other side?  I am not sure if we want to keep the\nunmerged pair for the purpose of this one.\n\n> +\t\tdiff_free_filepair(p);\n> +\t\tq->queue[i] = NULL;\n> +\t\tc--;\n\nAlso, if you are doing the in-place shrinking and have already\nintroduced another counter 'j' that is initialized to 0, I think it\nmakes more sense to do the shrinking in-place.  'i' will stay to be\nthe source-scan pointer that runs 0 thru q->nr, while 'j' can be\nused in this loop (where you have 'continue') to move the current\none that is determined to survive from q->queue[i] to q->queue[j++].\n\nThen you do not need 'c'; when the loop ends, 'j' would be the\nnumber of surviving entries and q->nr can be adjusted to it.  Unlike\nthe usual pattern taken by the other diffcore transformations where\na new queue is populated and the old one discarded, this would leave\nthe q->queue[] over-allocated, but I do not think it is too bad.\n"},{"id":"333471","messageId":"xmqq8tevkl6x.fsf@gitster.mtv.corp.google.com","threadId":"47260","inReplyTo":"xmqqpo88m896.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-11-25T04:59:34Z","receivedAt":"2017-11-25T04:59:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> If that is the case, shouldn't we make this new mode imply\n> --full-history to forbid history simplification?  \"git log\" is a\n> tool to find _an_ explanation of the current state, and the usual\n> history simplification makes tons of sense there, but blobfind is\n> run most likely in order to find _all_ mention of the set of blobs\n> given.\n\nOne scenario that I think we may want to be careful about is this:\n\n ---o---*---*---A*--M*--o---X\n     \\             /\n      o---*---o---B\n\nwhere commits marked with '*' has the same blob M:Makefile you are\nlooking for at the same path Makefile, and we start traversal at X\nwith \"git log --blobfind=M:Makefile X\" (or even with a pathspec, i.e.\n\"git log --blobfind=M:Makefile X -- Makefile).\n\nThe usual merge simplification rules would say \"Ah, M and A are\nTREESAME so we do not have to look at the side branch that ends at\nB\".  If the user is interested in finding all the introduction and\nthe retirement of a specific blob object, we would miss the\ntransition around the '*' on that side branch and ends up finding\nonly the transitions after the fork point where the blob is\nintroduced, and after M where the blob is retired.\n\nAnother interesting case we may want to be careful is this:\n\n    ---A*--M*--o---X\n          /\n      ---B*\n\nfor the same reason.  The usual merge simplification rules are\ndesigned to come up with _an_ explanation for the state in X,\nand because M is TREESAME with both A and B, it would pick just\none (the first parent) while ignoring the other.  Again, that would\nnot be appropriate if the reason why the user is running the command\nis to find all the introduction and the retirement of an object.\n\nIt may be worth covering these in the tests (I didn't try to see\nspecifically if the patch has these cases already, as I didn't think\nof the issue when I responded---sorry about that).\n\nThanks.\n\n\n"},{"id":"334379","messageId":"xmqq8teecj3y.fsf@gitster.mtv.corp.google.com","threadId":"47260","inReplyTo":"xmqqpo88m896.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-07T21:40:17Z","receivedAt":"2017-12-07T21:40:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\nAfter saying \"Will merge to 'next'\" in the recent \"What's cooking\"\nreport, I noticed that a few loose ends were never tied on this\ntopic.\n\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> index dd0dba5b1d..252a21cc19 100644\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -500,6 +500,10 @@ information.\n>>  --pickaxe-regex::\n>>  \tTreat the <string> given to `-S` as an extended POSIX regular\n>>  \texpression to match.\n>> +--blobfind=<blob-id>::\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> Can we have a blank line between these enumerations to make the\n> source easier to read?  Thanks.\n>\n> ...\n> So, we keep an unmerged pair, a pair that mentions a sought-blob on\n> one side or the other side?  I am not sure if we want to keep the\n> unmerged pair for the purpose of this one.\n>\n>> +\t\tdiff_free_filepair(p);\n>> +\t\tq->queue[i] = NULL;\n>> +\t\tc--;\n>\n> Also, if you are doing the in-place shrinking and have already\n> introduced another counter 'j' that is initialized to 0, I think it\n> makes more sense to do the shrinking in-place.  'i' will stay to be\n> the source-scan pointer that runs 0 thru q->nr, while 'j' can be\n> used in this loop (where you have 'continue') to move the current\n> one that is determined to survive from q->queue[i] to q->queue[j++].\n>\n> Then you do not need 'c'; when the loop ends, 'j' would be the\n> number of surviving entries and q->nr can be adjusted to it.  Unlike\n> the usual pattern taken by the other diffcore transformations where\n> a new queue is populated and the old one discarded, this would leave\n> the q->queue[] over-allocated, but I do not think it is too bad.\n\nHere is to illustrate the last point.  I still think we should keep\nthe unmerged entries for the purpose of blobfind but it should be\ntrivial to fix that.\n\n diffcore-blobfind.c | 33 ++++++++++++---------------------\n 1 file changed, 12 insertions(+), 21 deletions(-)\n\ndiff --git a/diffcore-blobfind.c b/diffcore-blobfind.c\nindex 5d222fc336..bf63ba61dc 100644\n--- a/diffcore-blobfind.c\n+++ b/diffcore-blobfind.c\n@@ -8,40 +8,31 @@\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\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 \n"}]}