{"thread":{"id":"54292","subject":"[PATCH 4/4] sequencer: stop abbreviating stopped-sha file","startedAt":"2020-09-25T06:07:49Z","lastAt":"2020-10-13T20:13:07Z","messageCount":13,"participants":["Junio C Hamano","René Scharfe","Barret Rhoden"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"406299","messageId":"20200925055954.1111389-5-gitster@pobox.com","threadId":"54292","inReplyTo":"20200925055954.1111389-1-gitster@pobox.com","subject":"[PATCH 4/4] sequencer: stop abbreviating stopped-sha file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T05:59:54Z","receivedAt":"2020-09-25T06:07:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The object name written to this file is not exposed to end-users and\nthe only reader of this file immediately expands it back to a full\nobject name.  Stop abbreviating while writing, and expect a full\nobject name while reading, which simplifies the code a bit.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sequencer.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fd7701c88a..7dc9088d09 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -120,7 +120,7 @@ static GIT_PATH_FUNC(rebase_path_author_script, \"rebase-merge/author-script\")\n static GIT_PATH_FUNC(rebase_path_amend, \"rebase-merge/amend\")\n /*\n  * When we stop at a given patch via the \"edit\" command, this file contains\n- * the abbreviated commit name of the corresponding patch.\n+ * the commit object name of the corresponding patch.\n  */\n static GIT_PATH_FUNC(rebase_path_stopped_sha, \"rebase-merge/stopped-sha\")\n /*\n@@ -3012,11 +3012,12 @@ static int make_patch(struct repository *r,\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct rev_info log_tree_opt;\n-\tconst char *subject, *p;\n+\tconst char *subject;\n+\tchar hex[GIT_MAX_HEXSZ + 1];\n \tint res = 0;\n \n-\tp = short_commit_name(commit);\n-\tif (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)\n+\toid_to_hex_r(hex, &commit->object.oid);\n+\tif (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)\n \t\treturn -1;\n \tres |= write_rebase_head(&commit->object.oid);\n \n@@ -4396,7 +4397,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \n \t\tif (read_oneliner(&buf, rebase_path_stopped_sha(),\n \t\t\t\t  READ_ONELINER_SKIP_IF_EMPTY) &&\n-\t\t    !get_oid_committish(buf.buf, &oid))\n+\t\t    !get_oid_hex(buf.buf, &oid))\n \t\t\trecord_in_rewritten(&oid, peek_command(&todo_list, 0));\n \t\tstrbuf_release(&buf);\n \t}\n-- \n2.28.0-718-gd8d5e3da39\n\n"},{"id":"406300","messageId":"20200925055954.1111389-4-gitster@pobox.com","threadId":"54292","inReplyTo":"20200925055954.1111389-1-gitster@pobox.com","subject":"[PATCH 3/4] t1506: rev-parse A..B and A...B","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T05:59:53Z","receivedAt":"2020-09-25T06:08:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Because these constructs can be used to parse user input to be\npassed to rev-list --objects, e.g.\n\n\trange=$(git rev-parse v1.0..v2.0) &&\n\tgit rev-list --objects $range | git pack-objects --stdin\n\nthe endpoints (v1.0 and v2.0 in the example) are shown without\npeeling them to underlying commits, even when they are annotated\ntags.  Make sure it stays that way.\n\nWhile at it, ensure \"rev-parse A...B\" also keeps the endpoints A and\nB unpeeled, even though the negative side (i.e. the merge-base\nbetween A and B) has to become a commit.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t1506-rev-parse-diagnosis.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex dbf690b9c1..3e657e693b 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -190,6 +190,24 @@ test_expect_success 'dotdot is not an empty set' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'dotdot does not peel endpoints' '\n+\tgit tag -a -m \"annote\" annotated HEAD &&\n+\tA=$(git rev-parse annotated) &&\n+\tH=$(git rev-parse annotated^0) &&\n+\t{\n+\t\techo $A && echo ^$A\n+\t} >expect-with-two-dots &&\n+\t{\n+\t\techo $A && echo $A && echo ^$H\n+\t} >expect-with-merge-base &&\n+\n+\tgit rev-parse annotated..annotated >actual-with-two-dots &&\n+\ttest_cmp expect-with-two-dots actual-with-two-dots &&\n+\n+\tgit rev-parse annotated...annotated >actual-with-merge-base &&\n+\ttest_cmp expect-with-merge-base actual-with-merge-base\n+'\n+\n test_expect_success 'arg before dashdash must be a revision (missing)' '\n \ttest_must_fail git rev-parse foobar -- 2>stderr &&\n \ttest_i18ngrep \"bad revision\" stderr\n-- \n2.28.0-718-gd8d5e3da39\n\n"},{"id":"406301","messageId":"20200925055954.1111389-1-gitster@pobox.com","threadId":"54292","inReplyTo":null,"subject":"[PATCH 0/4] Clean-up around get_x_ish()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T05:59:50Z","receivedAt":"2020-09-25T06:09:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is to clean some code I noticed while auditing the use of\nget_committish() and get_treeish() API functions.\n\nThe main topic is to tighten error checking of \"blame --ignore-rev\"\nand \"blame --ignore-revs-list\" arguments, which were not checked all\nthat much.  We make sure the ignore revs are committish objects, and\nalso peel tags pointing at commits down to commits before using them.\nThe breakage in the original is demonstrated by the tests added and/or\ntweaked in the second patch.\n\nThe last two patches are icing on the cake.\n\nJunio C Hamano (4):\n  t8013: minimum preparatory clean-up\n  blame: validate and peel the object names on the ignore list\n  t1506: rev-parse A..B and A...B\n  sequencer: stop abbreviating stopped-sha file\n\n builtin/blame.c                | 27 +++++++++++++--\n oidset.c                       |  9 ++++-\n oidset.h                       |  9 +++++\n sequencer.c                    | 11 +++---\n t/t1506-rev-parse-diagnosis.sh | 18 ++++++++++\n t/t8013-blame-ignore-revs.sh   | 61 ++++++++++++++++++++++------------\n 6 files changed, 105 insertions(+), 30 deletions(-)\n\n-- \n2.28.0-718-gd8d5e3da39\n\n"},{"id":"406302","messageId":"20200925055954.1111389-3-gitster@pobox.com","threadId":"54292","inReplyTo":"20200925055954.1111389-1-gitster@pobox.com","subject":"[PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T05:59:52Z","receivedAt":"2020-09-25T06:09:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The command reads list of object names to place on the ignore list\neither from the command line or from a file, but they are not\nchecked with their object type (those read from the file are not\neven checked for object existence).\n\nExtend the oidset_parse_file() API and allow it to take a callback\nthat can be used to die (e.g. when an inappropriate input is read)\nor modify the object name read (e.g. when a tag pointing at a commit\nis read, and the caller wants a commit object name), and use it in\nthe code that handles ignore list.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/blame.c              | 27 ++++++++++++++++++++++++--\n oidset.c                     |  9 ++++++++-\n oidset.h                     |  9 +++++++++\n t/t8013-blame-ignore-revs.sh | 37 ++++++++++++++++++++++++++----------\n 4 files changed, 69 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 94ef57c1cc..baa5d979cc 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -27,6 +27,7 @@\n #include \"object-store.h\"\n #include \"blame.h\"\n #include \"refs.h\"\n+#include \"tag.h\"\n \n static char blame_usage[] = N_(\"git blame [<options>] [<rev-opts>] [<rev>] [--] <file>\");\n \n@@ -803,6 +804,26 @@ static int is_a_rev(const char *name)\n \treturn OBJ_NONE < oid_object_info(the_repository, &oid, NULL);\n }\n \n+static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)\n+{\n+\tstruct repository *r = ((struct blame_scoreboard *)cbdata)->repo;\n+\tstruct object_id oid;\n+\n+\toidcpy(&oid, oid_ret);\n+\twhile (1) {\n+\t\tstruct object *obj;\n+\t\tint kind = oid_object_info(r, &oid, NULL);\n+\t\tif (kind == OBJ_COMMIT) {\n+\t\t\toidcpy(oid_ret, &oid);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (kind != OBJ_TAG)\n+\t\t\treturn -1;\n+\t\tobj = deref_tag(r, parse_object(r, &oid), NULL, 0);\n+\t\toidcpy(&oid, &obj->oid);\n+\t}\n+}\n+\n static void build_ignorelist(struct blame_scoreboard *sb,\n \t\t\t     struct string_list *ignore_revs_file_list,\n \t\t\t     struct string_list *ignore_rev_list)\n@@ -815,10 +836,12 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n \t\tif (!strcmp(i->string, \"\"))\n \t\t\toidset_clear(&sb->ignore_list);\n \t\telse\n-\t\t\toidset_parse_file(&sb->ignore_list, i->string);\n+\t\t\toidset_parse_file_carefully(&sb->ignore_list, i->string,\n+\t\t\t\t\t\t    peel_to_commit_oid, sb);\n \t}\n \tfor_each_string_list_item(i, ignore_rev_list) {\n-\t\tif (get_oid_committish(i->string, &oid))\n+\t\tif (get_oid_committish(i->string, &oid) ||\n+\t\t    peel_to_commit_oid(&oid, sb))\n \t\t\tdie(_(\"cannot find revision %s to ignore\"), i->string);\n \t\toidset_insert(&sb->ignore_list, &oid);\n \t}\ndiff --git a/oidset.c b/oidset.c\nindex 15d4e18c37..2d0ab76fb5 100644\n--- a/oidset.c\n+++ b/oidset.c\n@@ -42,6 +42,12 @@ int oidset_size(struct oidset *set)\n }\n \n void oidset_parse_file(struct oidset *set, const char *path)\n+{\n+\toidset_parse_file_carefully(set, path, NULL, NULL);\n+}\n+\n+void oidset_parse_file_carefully(struct oidset *set, const char *path,\n+\t\t\t\t oidset_parse_tweak_fn fn, void *cbdata)\n {\n \tFILE *fp;\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -66,7 +72,8 @@ void oidset_parse_file(struct oidset *set, const char *path)\n \t\tif (!sb.len)\n \t\t\tcontinue;\n \n-\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n+\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0' ||\n+\t\t    (fn && fn(&oid, cbdata)))\n \t\t\tdie(\"invalid object name: %s\", sb.buf);\n \t\toidset_insert(set, &oid);\n \t}\ndiff --git a/oidset.h b/oidset.h\nindex 209ae7a173..01f6560283 100644\n--- a/oidset.h\n+++ b/oidset.h\n@@ -73,6 +73,15 @@ void oidset_clear(struct oidset *set);\n  */\n void oidset_parse_file(struct oidset *set, const char *path);\n \n+/*\n+ * Similar to the above, but with a callback which can (1) return non-zero to\n+ * signal displeasure with the object and (2) replace object ID with something\n+ * else (meant to be used to \"peel\").\n+ */\n+typedef int (*oidset_parse_tweak_fn)(struct object_id *, void *);\n+void oidset_parse_file_carefully(struct oidset *set, const char *path,\n+\t\t\t\t oidset_parse_tweak_fn fn, void *cbdata);\n+\n struct oidset_iter {\n \tkh_oid_set_t *set;\n \tkhiter_t iter;\ndiff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh\nindex 67de83ae2b..24ae5018e8 100755\n--- a/t/t8013-blame-ignore-revs.sh\n+++ b/t/t8013-blame-ignore-revs.sh\n@@ -21,6 +21,7 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m X &&\n \tgit tag X &&\n+\tgit tag -a -m \"X (annotated)\" XT &&\n \n \tgit blame --line-porcelain file >blame_raw &&\n \n@@ -33,19 +34,35 @@ test_expect_success setup '\n \ttest_cmp expect actual\n '\n \n-# Ignore X, make sure A is blamed for line 1 and B for line 2.\n-test_expect_success ignore_rev_changing_lines '\n-\tgit blame --line-porcelain --ignore-rev X file >blame_raw &&\n-\n-\tgrep -E \"^[0-9a-f]+ [0-9]+ 1\" blame_raw | sed -e \"s/ .*//\" >actual &&\n-\tgit rev-parse A >expect &&\n-\ttest_cmp expect actual &&\n+# Ensure bogus --ignore-rev requests are caught\n+test_expect_success 'validate --ignore-rev' '\n+\ttest_must_fail git blame --ignore-rev X^{tree} file\n+'\n \n-\tgrep -E \"^[0-9a-f]+ [0-9]+ 2\" blame_raw | sed -e \"s/ .*//\" >actual &&\n-\tgit rev-parse B >expect &&\n-\ttest_cmp expect actual\n+# Ensure bogus --ignore-revs-file requests are caught\n+test_expect_success 'validate --ignore-revs-file' '\n+\tgit rev-parse X^{tree} >ignore_x &&\n+\ttest_must_fail git blame --ignore-revs-file ignore_x file\n '\n \n+for I in X XT\n+do\n+\t# Ignore X (or XT), make sure A is blamed for line 1 and B for line 2.\n+\t# Giving X (i.e. commit) and XT (i.e. annotated tag to commit) should\n+\t# produce the same result.\n+\ttest_expect_success \"ignore_rev_changing_lines ($I)\" '\n+\t\tgit blame --line-porcelain --ignore-rev $I file >blame_raw &&\n+\n+\t\tgrep -E \"^[0-9a-f]+ [0-9]+ 1\" blame_raw | sed -e \"s/ .*//\" >actual &&\n+\t\tgit rev-parse A >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgrep -E \"^[0-9a-f]+ [0-9]+ 2\" blame_raw | sed -e \"s/ .*//\" >actual &&\n+\t\tgit rev-parse B >expect &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n # For ignored revs that have added 'unblamable' lines, attribute those to the\n # ignored commit.\n # \tA--B--X--Y\n-- \n2.28.0-718-gd8d5e3da39\n\n"},{"id":"406303","messageId":"20200925055954.1111389-2-gitster@pobox.com","threadId":"54292","inReplyTo":"20200925055954.1111389-1-gitster@pobox.com","subject":"[PATCH 1/4] t8013: minimum preparatory clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T05:59:51Z","receivedAt":"2020-09-25T06:09:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The closing sq for each test piece should be placed at the beginning\nof line.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t8013-blame-ignore-revs.sh | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh\nindex 36dc31eb39..67de83ae2b 100755\n--- a/t/t8013-blame-ignore-revs.sh\n+++ b/t/t8013-blame-ignore-revs.sh\n@@ -31,7 +31,7 @@ test_expect_success setup '\n \tgrep -E \"^[0-9a-f]+ [0-9]+ 2\" blame_raw | sed -e \"s/ .*//\" >actual &&\n \tgit rev-parse X >expect &&\n \ttest_cmp expect actual\n-\t'\n+'\n \n # Ignore X, make sure A is blamed for line 1 and B for line 2.\n test_expect_success ignore_rev_changing_lines '\n@@ -44,7 +44,7 @@ test_expect_success ignore_rev_changing_lines '\n \tgrep -E \"^[0-9a-f]+ [0-9]+ 2\" blame_raw | sed -e \"s/ .*//\" >actual &&\n \tgit rev-parse B >expect &&\n \ttest_cmp expect actual\n-\t'\n+'\n \n # For ignored revs that have added 'unblamable' lines, attribute those to the\n # ignored commit.\n@@ -67,7 +67,7 @@ test_expect_success ignore_rev_adding_unblamable_lines '\n \n \tgrep -E \"^[0-9a-f]+ [0-9]+ 4\" blame_raw | sed -e \"s/ .*//\" >actual &&\n \ttest_cmp expect actual\n-\t'\n+'\n \n # Ignore X and Y, both in separate files.  Lines 1 == A, 2 == B.\n test_expect_success ignore_revs_from_files '\n@@ -82,7 +82,7 @@ test_expect_success ignore_revs_from_files '\n \tgrep -E \"^[0-9a-f]+ [0-9]+ 2\" blame_raw | sed -e \"s/ .*//\" >actual &&\n \tgit rev-parse B >expect &&\n \ttest_cmp expect actual\n-\t'\n+'\n \n # Ignore X from the config option, Y from a file.\n test_expect_success ignore_revs_from_configs_and_files '\n@@ -96,7 +96,7 @@ test_expect_success ignore_revs_from_configs_and_files '\n \tgrep -E \"^[0-9a-f]+ [0-9]+ 2\" blame_raw | sed -e \"s/ .*//\" >actual &&\n \tgit rev-parse B >expect &&\n \ttest_cmp expect actual\n-\t'\n+'\n \n # Override blame.ignoreRevsFile (ignore_x) with an empty string.  X should be\n # blamed now for lines 1 and 2, since we are no longer ignoring X.\n@@ -120,7 +120,7 @@ test_expect_success bad_files_and_revs '\n \techo NOREV >ignore_norev &&\n \ttest_must_fail git blame file --ignore-revs-file ignore_norev 2>err &&\n \ttest_i18ngrep \"invalid object name: NOREV\" err\n-\t'\n+'\n \n # For ignored revs that have added 'unblamable' lines, mark those lines with a\n # '*'\n@@ -138,7 +138,7 @@ test_expect_success mark_unblamable_lines '\n \n \tsed -n \"4p\" blame_raw | cut -c1 >actual &&\n \ttest_cmp expect actual\n-\t'\n+'\n \n # Commit Z will touch the first two lines.  Y touched all four.\n # \tA--B--X--Y--Z\n@@ -171,7 +171,7 @@ test_expect_success mark_ignored_lines '\n \n \tsed -n \"4p\" blame_raw | cut -c1 >actual &&\n \t! test_cmp expect actual\n-\t'\n+'\n \n # For ignored revs that added 'unblamable' lines and more recent commits changed\n # the blamable lines, mark the unblamable lines with a\n@@ -190,7 +190,7 @@ test_expect_success mark_unblamable_lines_intermediate '\n \n \tsed -n \"4p\" blame_raw | cut -c1 >actual &&\n \ttest_cmp expect actual\n-\t'\n+'\n \n # The heuristic called by guess_line_blames() tries to find the size of a\n # blame_entry 'e' in the parent's address space.  Those calculations need to\n@@ -227,7 +227,7 @@ test_expect_success ignored_chunk_negative_parent_size '\n \tgit tag C &&\n \n \tgit blame file --ignore-rev B >blame_raw\n-\t'\n+'\n \n # Resetting the repo and creating:\n #\n@@ -269,6 +269,6 @@ test_expect_success ignore_merge '\n \tgrep -E \"^[0-9a-f]+ [0-9]+ 9\" blame_raw | sed -e \"s/ .*//\" >actual &&\n \tgit rev-parse C >expect &&\n \ttest_cmp expect actual\n-\t'\n+'\n \n test_done\n-- \n2.28.0-718-gd8d5e3da39\n\n"},{"id":"406429","messageId":"40488753-c179-4ce2-42d0-e57b5b1ec6cd@web.de","threadId":"54292","inReplyTo":"20200925055954.1111389-3-gitster@pobox.com","subject":"Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-09-26T16:23:42Z","receivedAt":"2020-09-26T16:23:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.09.20 um 07:59 schrieb Junio C Hamano:\n> The command reads list of object names to place on the ignore list\n> either from the command line or from a file, but they are not\n> checked with their object type (those read from the file are not\n> even checked for object existence).\n>\n> Extend the oidset_parse_file() API and allow it to take a callback\n> that can be used to die (e.g. when an inappropriate input is read)\n> or modify the object name read (e.g. when a tag pointing at a commit\n> is read, and the caller wants a commit object name), and use it in\n> the code that handles ignore list.\n\nWhat's the benefit of such a check?  Ignoring a non-existing or\ntype-mismatched object is really easy -- no actual effort is required to\nfulfill that request.\n\nWhen I request \"Don't eat any glue!\", perfectly human responses could be\n\"But I don't have any glue!\" or \"It doesn't even taste that good.\", but\nI'd expect a computer program to act I bit more logical and just don't\ndo it, without talking back.  Maybe that's just me.\n\n(I had been bitten by a totally different software adding such a check,\nwhich made it complain about my long catch-all ignore list, and I had to\ncraft and maintain a specific \"clean\" list for each deployment --\nperhaps I'm still bitter about that.)\n\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/blame.c              | 27 ++++++++++++++++++++++++--\n>  oidset.c                     |  9 ++++++++-\n>  oidset.h                     |  9 +++++++++\n>  t/t8013-blame-ignore-revs.sh | 37 ++++++++++++++++++++++++++----------\n>  4 files changed, 69 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 94ef57c1cc..baa5d979cc 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -27,6 +27,7 @@\n>  #include \"object-store.h\"\n>  #include \"blame.h\"\n>  #include \"refs.h\"\n> +#include \"tag.h\"\n>\n>  static char blame_usage[] = N_(\"git blame [<options>] [<rev-opts>] [<rev>] [--] <file>\");\n>\n> @@ -803,6 +804,26 @@ static int is_a_rev(const char *name)\n>  \treturn OBJ_NONE < oid_object_info(the_repository, &oid, NULL);\n>  }\n>\n> +static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)\n> +{\n> +\tstruct repository *r = ((struct blame_scoreboard *)cbdata)->repo;\n> +\tstruct object_id oid;\n> +\n> +\toidcpy(&oid, oid_ret);\n> +\twhile (1) {\n> +\t\tstruct object *obj;\n> +\t\tint kind = oid_object_info(r, &oid, NULL);\n> +\t\tif (kind == OBJ_COMMIT) {\n> +\t\t\toidcpy(oid_ret, &oid);\n\nAt that point we know it's an object, but cast it up to the most generic\nclass we have -- an object ID.  We could have set an object flag to mark\nit ignored instead, which would be trivial to check later.  On the other\nhand it probably wouldn't make much of a difference -- hashmaps are\npretty fast, and blame has lots of things to do beyond ignoring commits.\n\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\tif (kind != OBJ_TAG)\n> +\t\t\treturn -1;\n> +\t\tobj = deref_tag(r, parse_object(r, &oid), NULL, 0);\n> +\t\toidcpy(&oid, &obj->oid);\n> +\t}\n> +}\n> +\n>  static void build_ignorelist(struct blame_scoreboard *sb,\n>  \t\t\t     struct string_list *ignore_revs_file_list,\n>  \t\t\t     struct string_list *ignore_rev_list)\n> @@ -815,10 +836,12 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n>  \t\tif (!strcmp(i->string, \"\"))\n>  \t\t\toidset_clear(&sb->ignore_list);\n\nThis preexisting feature is curious.  It's even documented ('An empty\nfile name, \"\", will clear the list of revs from previously processed\nfiles.') and covered by t8013.6.  Why would we need such magic in\naddition to the standard negation (--no-ignore-revs-file) for clearing\nthe list?  The latter counters blame.ignoreRevsFile as well. *puzzled*\n\n>  \t\telse\n> -\t\t\toidset_parse_file(&sb->ignore_list, i->string);\n> +\t\t\toidset_parse_file_carefully(&sb->ignore_list, i->string,\n> +\t\t\t\t\t\t    peel_to_commit_oid, sb);\n>  \t}\n>  \tfor_each_string_list_item(i, ignore_rev_list) {\n> -\t\tif (get_oid_committish(i->string, &oid))\n> +\t\tif (get_oid_committish(i->string, &oid) ||\n> +\t\t    peel_to_commit_oid(&oid, sb))\n>  \t\t\tdie(_(\"cannot find revision %s to ignore\"), i->string);\n>  \t\toidset_insert(&sb->ignore_list, &oid);\n>  \t}\n> diff --git a/oidset.c b/oidset.c\n> index 15d4e18c37..2d0ab76fb5 100644\n> --- a/oidset.c\n> +++ b/oidset.c\n> @@ -42,6 +42,12 @@ int oidset_size(struct oidset *set)\n>  }\n>\n>  void oidset_parse_file(struct oidset *set, const char *path)\n> +{\n> +\toidset_parse_file_carefully(set, path, NULL, NULL);\n> +}\n> +\n> +void oidset_parse_file_carefully(struct oidset *set, const char *path,\n> +\t\t\t\t oidset_parse_tweak_fn fn, void *cbdata)\n>  {\n>  \tFILE *fp;\n>  \tstruct strbuf sb = STRBUF_INIT;\n> @@ -66,7 +72,8 @@ void oidset_parse_file(struct oidset *set, const char *path)\n>  \t\tif (!sb.len)\n>  \t\t\tcontinue;\n>\n> -\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n> +\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0' ||\n> +\t\t    (fn && fn(&oid, cbdata)))\n\nOK, so this turns the basic all-I-know-is-hashes oidset loader into a\nflexible higher-order map function.  Fun, but wise?  Can't make up my\nmind.\n\nRené\n"},{"id":"406431","messageId":"xmqqtuvkii1j.fsf@gitster.c.googlers.com","threadId":"54292","inReplyTo":"40488753-c179-4ce2-42d0-e57b5b1ec6cd@web.de","subject":"Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-26T17:06:48Z","receivedAt":"2020-09-26T17:06:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> When I request \"Don't eat any glue!\", perfectly human responses could be\n> \"But I don't have any glue!\" or \"It doesn't even taste that good.\", but\n> I'd expect a computer program to act I bit more logical and just don't\n> do it, without talking back.  Maybe that's just me.\n>\n> (I had been bitten by a totally different software adding such a check,\n> which made it complain about my long catch-all ignore list, and I had to\n> craft and maintain a specific \"clean\" list for each deployment --\n> perhaps I'm still bitter about that.)\n\nA user who says \"ignore v2.3\", sees that the commit pointed at by\nthat release tag is not ignored, comes here to complain, and is told\nto write v2.3^0 instead, would not be happy.  It is a mistake easy\nto catch to help users, so I am more for than against that part of\nthe change.  I am completely neutral about \"you told me to ignore\nthis, but as far as I can tell it does not even exist---did you \nscrew up when you prepared the list of stuff to ignore?\" part.  I do\nnot mind seeing it removed.\n\n>> +\t\tif (kind == OBJ_COMMIT) {\n>> +\t\t\toidcpy(oid_ret, &oid);\n>\n> At that point we know it's an object, but cast it up to the most generic\n> class we have -- an object ID.  We could have set an object flag to mark\n> it ignored instead, which would be trivial to check later.  On the other\n> hand it probably wouldn't make much of a difference -- hashmaps are\n> pretty fast, and blame has lots of things to do beyond ignoring commits.\n\nQuite honestly, I am not interested in the \"blame --ignore\" feature\nitself.  It is good that you CC'ed Barret so that such an\nimprovement suggestion would be heard by the right party ;-).\n\n>> @@ -815,10 +836,12 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n>>  \t\tif (!strcmp(i->string, \"\"))\n>>  \t\t\toidset_clear(&sb->ignore_list);\n>\n> This preexisting feature is curious.  It's even documented ('An empty\n> file name, \"\", will clear the list of revs from previously processed\n> files.') and covered by t8013.6.  Why would we need such magic in\n> addition to the standard negation (--no-ignore-revs-file) for clearing\n> the list?  The latter counters blame.ignoreRevsFile as well. *puzzled*\n\nI shared the puzzlement when I saw it, but ditto.\n\n>> +void oidset_parse_file_carefully(struct oidset *set, const char *path,\n>> +\t\t\t\t oidset_parse_tweak_fn fn, void *cbdata)\n>>  {\n>>  \tFILE *fp;\n>>  \tstruct strbuf sb = STRBUF_INIT;\n>> @@ -66,7 +72,8 @@ void oidset_parse_file(struct oidset *set, const char *path)\n>>  \t\tif (!sb.len)\n>>  \t\t\tcontinue;\n>>\n>> -\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n>> +\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0' ||\n>> +\t\t    (fn && fn(&oid, cbdata)))\n>\n> OK, so this turns the basic all-I-know-is-hashes oidset loader into a\n> flexible higher-order map function.  Fun, but wise?  Can't make up my\n> mind.\n\nFun and probably useful.  It is a different matter if it is wise to\nuse it to (1) peel tags to commits and (2) fail on an nonexistent\nobject.  My take on them is (1) is probably true, and (2) is Meh ;-)\n\nThanks.\n"},{"id":"406463","messageId":"xmqqsgb4gkf5.fsf@gitster.c.googlers.com","threadId":"54292","inReplyTo":"xmqqtuvkii1j.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-26T23:58:22Z","receivedAt":"2020-09-26T23:58:32Z","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>>> @@ -66,7 +72,8 @@ void oidset_parse_file(struct oidset *set, const char *path)\n>>>  \t\tif (!sb.len)\n>>>  \t\t\tcontinue;\n>>>\n>>> -\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n>>> +\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0' ||\n>>> +\t\t    (fn && fn(&oid, cbdata)))\n>>\n>> OK, so this turns the basic all-I-know-is-hashes oidset loader into a\n>> flexible higher-order map function.  Fun, but wise?  Can't make up my\n>> mind.\n>\n> Fun and probably useful.  It is a different matter if it is wise to\n> use it to (1) peel tags to commits and (2) fail on an nonexistent\n> object.  My take on them is (1) is probably true, and (2) is Meh ;-)\n\nIf we choose to do (2) differently, we only need the following\none-liner patch.  I might suggest tweaking the semantics of the\ncallback function a bit to allow it to tell the caller (i.e.\noidset_parse_file_carefully()) that it wants to go on as usual, it\nwants to omit the object from the hashtable, it replaced the given\nobject to something else, or it detected an error and wants to\nabort, and if we were doing that, we'd be returning \"do not add this\nobject to the table\" signal, instead of 0 that signals \"we are good\ndoing business as usual\", from here.\n\n\n builtin/blame.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex baa5d979cc..8d7b66e970 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -817,8 +817,19 @@ static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)\n \t\t\toidcpy(oid_ret, &oid);\n \t\t\treturn 0;\n \t\t}\n+\n+\t\t/*\n+\t\t * We can ignore a request to ignore any nonexistent\n+\t\t * objects, trees and blobs by not doing anything\n+\t\t * special, as the blame machinery works with commits,\n+\t\t * so entries in the hashtable from these objects will\n+\t\t * never be looked up.  But we do allow dereferencing\n+\t\t * an annotated tag, as silently ignoring a request to\n+\t\t * ignore v1.0.0 because it is an annotated tag is a\n+\t\t * bit too unfriendly to end-users.\n+\t\t */\n \t\tif (kind != OBJ_TAG)\n-\t\t\treturn -1;\n+\t\t\treturn 0;\n \t\tobj = deref_tag(r, parse_object(r, &oid), NULL, 0);\n \t\toidcpy(&oid, &obj->oid);\n \t}\n"},{"id":"406534","messageId":"32370477-c6e4-5378-fedc-c86b9ddf96bd@google.com","threadId":"54292","inReplyTo":"xmqqtuvkii1j.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2020-09-28T13:26:33Z","receivedAt":"2020-09-28T13:26:42Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"Hi -\n\nOn 9/26/20 1:06 PM, Junio C Hamano wrote:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> When I request \"Don't eat any glue!\", perfectly human responses could be\n>> \"But I don't have any glue!\" or \"It doesn't even taste that good.\", but\n>> I'd expect a computer program to act I bit more logical and just don't\n>> do it, without talking back.  Maybe that's just me.\n>>\n>> (I had been bitten by a totally different software adding such a check,\n>> which made it complain about my long catch-all ignore list, and I had to\n>> craft and maintain a specific \"clean\" list for each deployment --\n>> perhaps I'm still bitter about that.)\n> \n> A user who says \"ignore v2.3\", sees that the commit pointed at by\n> that release tag is not ignored, comes here to complain, and is told\n> to write v2.3^0 instead, would not be happy.  It is a mistake easy\n> to catch to help users, so I am more for than against that part of\n> the change.\n\nThat sounds like a nice change.\n\n> I am completely neutral about \"you told me to ignore\n> this, but as far as I can tell it does not even exist---did you\n> screw up when you prepared the list of stuff to ignore?\" part.  I do\n> not mind seeing it removed.\n\nPart of my reasoning for \"fail if you can't find it\" was that it was \nhighly likely to be a user error.  Especially because it will fail for a \nshort hash from a file.  If you do have a list of commits to ignore \n(.git-blame-ignore-revs), that list is probably under version control in \nthe same git repo, so it should change as you change branches.\n\nBut all in all, I'm fine with skipping unknown objects.  Or for warning \nor having a git-config option, like we do for a couple other aspects of \nblame-ignore, since one size doesn't fit all.\n\n>>> +\t\tif (kind == OBJ_COMMIT) {\n>>> +\t\t\toidcpy(oid_ret, &oid);\n>>\n>> At that point we know it's an object, but cast it up to the most generic\n>> class we have -- an object ID.  We could have set an object flag to mark\n>> it ignored instead, which would be trivial to check later.  On the other\n>> hand it probably wouldn't make much of a difference -- hashmaps are\n>> pretty fast, and blame has lots of things to do beyond ignoring commits.\n> \n> Quite honestly, I am not interested in the \"blame --ignore\" feature\n> itself.  It is good that you CC'ed Barret so that such an\n> improvement suggestion would be heard by the right party ;-).\n\nAny performance improvement would be welcome.  I haven't looked at the \ncode in a while, but I don't recall any reasons why this wouldn't work.\n\n> \n>>> @@ -815,10 +836,12 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n>>>   \t\tif (!strcmp(i->string, \"\"))\n>>>   \t\t\toidset_clear(&sb->ignore_list);\n>>\n>> This preexisting feature is curious.  It's even documented ('An empty\n>> file name, \"\", will clear the list of revs from previously processed\n>> files.') and covered by t8013.6.  Why would we need such magic in\n>> addition to the standard negation (--no-ignore-revs-file) for clearing\n>> the list?  The latter counters blame.ignoreRevsFile as well. *puzzled*\n> \n> I shared the puzzlement when I saw it, but ditto.\n\nI don't recall exactly.  Someone on the list might have wanted to both \ncounter the blame.ignoreRevsFile and specify another file.  Or maybe \nthey just wanted to counter the ignoreRevsFile, and I didn't know that \n--no- would already do that.  I'm certainly not wed to it.\n\nThanks,\n\nBarret\n\n\n>>> +void oidset_parse_file_carefully(struct oidset *set, const char *path,\n>>> +\t\t\t\t oidset_parse_tweak_fn fn, void *cbdata)\n>>>   {\n>>>   \tFILE *fp;\n>>>   \tstruct strbuf sb = STRBUF_INIT;\n>>> @@ -66,7 +72,8 @@ void oidset_parse_file(struct oidset *set, const char *path)\n>>>   \t\tif (!sb.len)\n>>>   \t\t\tcontinue;\n>>>\n>>> -\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n>>> +\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0' ||\n>>> +\t\t    (fn && fn(&oid, cbdata)))\n>>\n>> OK, so this turns the basic all-I-know-is-hashes oidset loader into a\n>> flexible higher-order map function.  Fun, but wise?  Can't make up my\n>> mind.\n> \n> Fun and probably useful.  It is a different matter if it is wise to\n> use it to (1) peel tags to commits and (2) fail on an nonexistent\n> object.  My take on them is (1) is probably true, and (2) is Meh ;-)\n> \n> Thanks.\n> \n\n"},{"id":"407303","messageId":"1fa730c4-eaef-2f32-e1b4-716a27ed4646@web.de","threadId":"54292","inReplyTo":"32370477-c6e4-5378-fedc-c86b9ddf96bd@google.com","subject":"Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-10-11T16:03:20Z","receivedAt":"2020-10-11T16:03:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.09.20 um 15:26 schrieb Barret Rhoden:\n> On 9/26/20 1:06 PM, Junio C Hamano wrote:\n>> A user who says \"ignore v2.3\", sees that the commit pointed at by\n>> that release tag is not ignored, comes here to complain, and is\n>> told to write v2.3^0 instead, would not be happy.  It is a mistake\n>> easy to catch to help users, so I am more for than against that\n>> part of the change.\n>\n> That sounds like a nice change.\n\nI agree -- peeling down to the commit is good.\n\n>> I am completely neutral about \"you told me to ignore this, but as\n>> far as I can tell it does not even exist---did you screw up when\n>> you prepared the list of stuff to ignore?\" part.  I do not mind\n>> seeing it removed.\n>\n> Part of my reasoning for \"fail if you can't find it\" was that it was\n> highly likely to be a user error.  Especially because it will fail\n> for a short hash from a file.  If you do have a list of commits to\n> ignore (.git-blame-ignore-revs), that list is probably under version\n> control in the same git repo, so it should change as you change\n> branches.\n>\n> But all in all, I'm fine with skipping unknown objects.  Or for\n> warning or having a git-config option, like we do for a couple other\n> aspects of blame-ignore, since one size doesn't fit all.\n\nI would expect user errors to be more likely with --ignore-rev and\ninteractive use (command line typos).  I see how aborting if the\nreferenced commit doesn't exists can help, and it's consistent with\nother options that require a revision name.\n\nI don't know how most people use --ignore-revs-file, but can imagine\na big bin of boring commits that is just appended to.  Having to\nkeep that file clean by removing commits from abandoned and garbage-\ncollected branches sounds like unnecessary busy-work to me.  (What\ncan I say -- I'm lazy.)\n\n> Any performance improvement would be welcome.  I haven't looked at\n> the code in a while, but I don't recall any reasons why this wouldn't\n> work.\n\nUsing a commit flag instead of an oidset would only improve\nperformance noticeably if the product of the number of suspects and\nignored commits was huge, I guess.\n\nI get weird timings for an ignore file containing basically all commits\n(created with \"git log --format=%H\").  With Git's own repo and rc1:\n\nBenchmark #1: ./git-blame --ignore-revs-file hashes Makefile\n  Time (mean ± σ):      8.470 s ±  0.049 s    [User: 7.923 s, System: 0.547 s]\n  Range (min … max):    8.434 s …  8.605 s    10 runs\n\nAnd with the patch at the bottom:\n\nBenchmark #1: ./git-blame --ignore-revs-file hashes Makefile\n  Time (mean ± σ):      8.048 s ±  0.061 s    [User: 7.899 s, System: 0.146 s]\n  Range (min … max):    7.987 s …  8.175 s    10 runs\n\nThat looks like a nice speedup, but why for system time alone?  Malloc\noverhead perhaps?  And when I get rid of the intermediate oidset by\npartially duplicating oidset_parse_file_carefully() it takes longer than\nnine seconds.  Weird.  Perhaps a silly bug.\n\n>>> This preexisting feature is curious.  It's even documented ('An\n>>> empty file name, \"\", will clear the list of revs from previously\n>>> processed files.') and covered by t8013.6.  Why would we need\n>>> such magic in addition to the standard negation\n>>> (--no-ignore-revs-file) for clearing the list?  The latter\n>>> counters blame.ignoreRevsFile as well. *puzzled*\n>>\n>> I shared the puzzlement when I saw it, but ditto.\n>\n> I don't recall exactly.  Someone on the list might have wanted to\n> both counter the blame.ignoreRevsFile and specify another file.  Or\n> maybe they just wanted to counter the ignoreRevsFile, and I didn't\n> know that --no- would already do that.  I'm certainly not wed to it.\n\nThe first step would be to show a deprecation warning, wait a few\nreleases and then remove that feature.  Not sure the effort and\npotential user irritation is worth the saved conditional, doc lines\nand test.  (We already established that I'm lazy.)\n\nAnyway, here's the patch:\n---\n blame.c         |  2 +-\n blame.h         |  5 +++--\n builtin/blame.c | 16 ++++++++++++----\n object.h        |  3 ++-\n 4 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/blame.c b/blame.c\nindex 686845b2b4..6e8c8fec9b 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -2487,7 +2487,7 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin,\n \t/*\n \t * Pass remaining suspects for ignored commits to their parents.\n \t */\n-\tif (oidset_contains(&sb->ignore_list, &commit->object.oid)) {\n+\tif (commit->object.flags & BLAME_IGNORE) {\n \t\tfor (i = 0, sg = first_scapegoat(revs, commit, sb->reverse);\n \t\t     i < num_sg && sg;\n \t\t     sg = sg->next, i++) {\ndiff --git a/blame.h b/blame.h\nindex b6bbee4147..d35167e8bd 100644\n--- a/blame.h\n+++ b/blame.h\n@@ -16,6 +16,9 @@\n #define BLAME_DEFAULT_MOVE_SCORE\t20\n #define BLAME_DEFAULT_COPY_SCORE\t40\n\n+/* Remember to update object flag allocation in object.h */\n+#define BLAME_IGNORE\t(1u<<14)\n+\n struct fingerprint;\n\n /*\n@@ -125,8 +128,6 @@ struct blame_scoreboard {\n \t/* linked list of blames */\n \tstruct blame_entry *ent;\n\n-\tstruct oidset ignore_list;\n-\n \t/* look-up a line in the final buffer */\n \tint num_lines;\n \tint *lineno;\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bb0f29300e..1c6721b5d5 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -830,21 +830,29 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n {\n \tstruct string_list_item *i;\n \tstruct object_id oid;\n+\tconst struct object_id *o;\n+\tstruct oidset_iter iter;\n+\tstruct oidset ignore_list = OIDSET_INIT;\n\n-\toidset_init(&sb->ignore_list, 0);\n \tfor_each_string_list_item(i, ignore_revs_file_list) {\n \t\tif (!strcmp(i->string, \"\"))\n-\t\t\toidset_clear(&sb->ignore_list);\n+\t\t\toidset_clear(&ignore_list);\n \t\telse\n-\t\t\toidset_parse_file_carefully(&sb->ignore_list, i->string,\n+\t\t\toidset_parse_file_carefully(&ignore_list, i->string,\n \t\t\t\t\t\t    peel_to_commit_oid, sb);\n \t}\n \tfor_each_string_list_item(i, ignore_rev_list) {\n \t\tif (get_oid_committish(i->string, &oid) ||\n \t\t    peel_to_commit_oid(&oid, sb))\n \t\t\tdie(_(\"cannot find revision %s to ignore\"), i->string);\n-\t\toidset_insert(&sb->ignore_list, &oid);\n+\t\toidset_insert(&ignore_list, &oid);\n \t}\n+\toidset_iter_init(&ignore_list, &iter);\n+\twhile ((o = oidset_iter_next(&iter))) {\n+\t\tstruct commit *commit = lookup_commit(sb->repo, o);\n+\t\tcommit->object.flags |= BLAME_IGNORE;\n+\t}\n+\toidset_clear(&ignore_list);\n }\n\n int cmd_blame(int argc, const char **argv, const char *prefix)\ndiff --git a/object.h b/object.h\nindex 20b18805f0..6818c9296b 100644\n--- a/object.h\n+++ b/object.h\n@@ -64,7 +64,8 @@ struct object_array {\n  * negotiator/default.c:       2--5\n  * walker.c:                 0-2\n  * upload-pack.c:                4       11-----14  16-----19\n- * builtin/blame.c:                        12-13\n+ * blame.c:                                     14\n+ * builtin/blame.c:                        12---14\n  * bisect.c:                                        16\n  * bundle.c:                                        16\n  * http-push.c:                          11-----14\n--\n2.28.0\n\n"},{"id":"407347","messageId":"xmqqa6wrqt98.fsf@gitster.c.googlers.com","threadId":"54292","inReplyTo":"1fa730c4-eaef-2f32-e1b4-716a27ed4646@web.de","subject":"Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T16:54:59Z","receivedAt":"2020-10-12T16:55:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>>>> This preexisting feature is curious.  It's even documented ('An\n>>>> empty file name, \"\", will clear the list of revs from previously\n>>>> processed files.') and covered by t8013.6.  Why would we need\n>>>> such magic in addition to the standard negation\n>>>> (--no-ignore-revs-file) for clearing the list?  The latter\n>>>> counters blame.ignoreRevsFile as well. *puzzled*\n>>>\n>>> I shared the puzzlement when I saw it, but ditto.\n>>\n>> I don't recall exactly.  Someone on the list might have wanted to\n>> both counter the blame.ignoreRevsFile and specify another file.  Or\n>> maybe they just wanted to counter the ignoreRevsFile, and I didn't\n>> know that --no- would already do that.  I'm certainly not wed to it.\n>\n> The first step would be to show a deprecation warning, wait a few\n> releases and then remove that feature.  Not sure the effort and\n> potential user irritation is worth the saved conditional, doc lines\n> and test.  (We already established that I'm lazy.)\n\nI do not particularly see the need to.  Perhaps when somebody\ncomplains the next time?\n\n> Anyway, here's the patch:\n> ---\n>  blame.c         |  2 +-\n>  blame.h         |  5 +++--\n>  builtin/blame.c | 16 ++++++++++++----\n>  object.h        |  3 ++-\n>  4 files changed, 18 insertions(+), 8 deletions(-)\n\nLooks OK to me from a quick scan.\n"},{"id":"407388","messageId":"cd2c51da-55c6-cc5e-2da1-69db90aaf438@google.com","threadId":"54292","inReplyTo":"1fa730c4-eaef-2f32-e1b4-716a27ed4646@web.de","subject":"Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2020-10-12T20:39:33Z","receivedAt":"2020-10-12T20:39:41Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"Hi -\n\nOn 10/11/20 12:03 PM, René Scharfe wrote:\n[snip]\n>> Any performance improvement would be welcome.  I haven't looked at\n>> the code in a while, but I don't recall any reasons why this wouldn't\n>> work.\n> \n> Using a commit flag instead of an oidset would only improve\n> performance noticeably if the product of the number of suspects and\n> ignored commits was huge, I guess.\n> \n> I get weird timings for an ignore file containing basically all commits\n> (created with \"git log --format=%H\").  With Git's own repo and rc1:\n> \n> Benchmark #1: ./git-blame --ignore-revs-file hashes Makefile\n>    Time (mean ± σ):      8.470 s ±  0.049 s    [User: 7.923 s, System: 0.547 s]\n>    Range (min … max):    8.434 s …  8.605 s    10 runs\n> \n> And with the patch at the bottom:\n> \n> Benchmark #1: ./git-blame --ignore-revs-file hashes Makefile\n>    Time (mean ± σ):      8.048 s ±  0.061 s    [User: 7.899 s, System: 0.146 s]\n>    Range (min … max):    7.987 s …  8.175 s    10 runs\n> \n> That looks like a nice speedup, but why for system time alone?  Malloc\n> overhead perhaps?\n\nHard to say.  Maybe page faults when walking the old ignore_list?\n\n> Anyway, here's the patch:\n\nLooks good to me.\n\nBarret\n\n\n> ---\n>   blame.c         |  2 +-\n>   blame.h         |  5 +++--\n>   builtin/blame.c | 16 ++++++++++++----\n>   object.h        |  3 ++-\n>   4 files changed, 18 insertions(+), 8 deletions(-)\n> \n> diff --git a/blame.c b/blame.c\n> index 686845b2b4..6e8c8fec9b 100644\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -2487,7 +2487,7 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin,\n>   \t/*\n>   \t * Pass remaining suspects for ignored commits to their parents.\n>   \t */\n> -\tif (oidset_contains(&sb->ignore_list, &commit->object.oid)) {\n> +\tif (commit->object.flags & BLAME_IGNORE) {\n>   \t\tfor (i = 0, sg = first_scapegoat(revs, commit, sb->reverse);\n>   \t\t     i < num_sg && sg;\n>   \t\t     sg = sg->next, i++) {\n> diff --git a/blame.h b/blame.h\n> index b6bbee4147..d35167e8bd 100644\n> --- a/blame.h\n> +++ b/blame.h\n> @@ -16,6 +16,9 @@\n>   #define BLAME_DEFAULT_MOVE_SCORE\t20\n>   #define BLAME_DEFAULT_COPY_SCORE\t40\n> \n> +/* Remember to update object flag allocation in object.h */\n> +#define BLAME_IGNORE\t(1u<<14)\n> +\n>   struct fingerprint;\n> \n>   /*\n> @@ -125,8 +128,6 @@ struct blame_scoreboard {\n>   \t/* linked list of blames */\n>   \tstruct blame_entry *ent;\n> \n> -\tstruct oidset ignore_list;\n> -\n>   \t/* look-up a line in the final buffer */\n>   \tint num_lines;\n>   \tint *lineno;\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index bb0f29300e..1c6721b5d5 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -830,21 +830,29 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n>   {\n>   \tstruct string_list_item *i;\n>   \tstruct object_id oid;\n> +\tconst struct object_id *o;\n> +\tstruct oidset_iter iter;\n> +\tstruct oidset ignore_list = OIDSET_INIT;\n> \n> -\toidset_init(&sb->ignore_list, 0);\n>   \tfor_each_string_list_item(i, ignore_revs_file_list) {\n>   \t\tif (!strcmp(i->string, \"\"))\n> -\t\t\toidset_clear(&sb->ignore_list);\n> +\t\t\toidset_clear(&ignore_list);\n>   \t\telse\n> -\t\t\toidset_parse_file_carefully(&sb->ignore_list, i->string,\n> +\t\t\toidset_parse_file_carefully(&ignore_list, i->string,\n>   \t\t\t\t\t\t    peel_to_commit_oid, sb);\n>   \t}\n>   \tfor_each_string_list_item(i, ignore_rev_list) {\n>   \t\tif (get_oid_committish(i->string, &oid) ||\n>   \t\t    peel_to_commit_oid(&oid, sb))\n>   \t\t\tdie(_(\"cannot find revision %s to ignore\"), i->string);\n> -\t\toidset_insert(&sb->ignore_list, &oid);\n> +\t\toidset_insert(&ignore_list, &oid);\n>   \t}\n> +\toidset_iter_init(&ignore_list, &iter);\n> +\twhile ((o = oidset_iter_next(&iter))) {\n> +\t\tstruct commit *commit = lookup_commit(sb->repo, o);\n> +\t\tcommit->object.flags |= BLAME_IGNORE;\n> +\t}\n> +\toidset_clear(&ignore_list);\n>   }\n> \n>   int cmd_blame(int argc, const char **argv, const char *prefix)\n> diff --git a/object.h b/object.h\n> index 20b18805f0..6818c9296b 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -64,7 +64,8 @@ struct object_array {\n>    * negotiator/default.c:       2--5\n>    * walker.c:                 0-2\n>    * upload-pack.c:                4       11-----14  16-----19\n> - * builtin/blame.c:                        12-13\n> + * blame.c:                                     14\n> + * builtin/blame.c:                        12---14\n>    * bisect.c:                                        16\n>    * bundle.c:                                        16\n>    * http-push.c:                          11-----14\n> --\n> 2.28.0\n> \n\n"},{"id":"407484","messageId":"ea1f2a1e-c525-c735-bdf7-65d44771cb3f@web.de","threadId":"54292","inReplyTo":"cd2c51da-55c6-cc5e-2da1-69db90aaf438@google.com","subject":"Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-10-13T20:12:37Z","receivedAt":"2020-10-13T20:13:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.10.20 um 22:39 schrieb Barret Rhoden:\n> Hi -\n>\n> On 10/11/20 12:03 PM, René Scharfe wrote:\n> [snip]\n>>> Any performance improvement would be welcome.  I haven't looked at\n>>> the code in a while, but I don't recall any reasons why this wouldn't\n>>> work.\n>>\n>> Using a commit flag instead of an oidset would only improve\n>> performance noticeably if the product of the number of suspects and\n>> ignored commits was huge, I guess.\n>>\n>> I get weird timings for an ignore file containing basically all commits\n>> (created with \"git log --format=%H\").  With Git's own repo and rc1:\n>>\n>> Benchmark #1: ./git-blame --ignore-revs-file hashes Makefile\n>>    Time (mean ± σ):      8.470 s ±  0.049 s    [User: 7.923 s, System: 0.547 s]\n>>    Range (min … max):    8.434 s …  8.605 s    10 runs\n>>\n>> And with the patch at the bottom:\n>>\n>> Benchmark #1: ./git-blame --ignore-revs-file hashes Makefile\n>>    Time (mean ± σ):      8.048 s ±  0.061 s    [User: 7.899 s, System: 0.146 s]\n>>    Range (min … max):    7.987 s …  8.175 s    10 runs\n>>\n>> That looks like a nice speedup, but why for system time alone?  Malloc\n>> overhead perhaps?\n>\n> Hard to say.  Maybe page faults when walking the old ignore_list?\n\nbrk(2) calls.  strace -c says that rc1 has 21657 of them and the patch\ngets that down to 8132.  They dominate system time in both cases.\n\n>\n>> Anyway, here's the patch:\n>\n> Looks good to me.\n>\n> Barret\n>\n>\n>> ---\n>>   blame.c         |  2 +-\n>>   blame.h         |  5 +++--\n>>   builtin/blame.c | 16 ++++++++++++----\n>>   object.h        |  3 ++-\n>>   4 files changed, 18 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/blame.c b/blame.c\n>> index 686845b2b4..6e8c8fec9b 100644\n>> --- a/blame.c\n>> +++ b/blame.c\n>> @@ -2487,7 +2487,7 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin,\n>>       /*\n>>        * Pass remaining suspects for ignored commits to their parents.\n>>        */\n>> -    if (oidset_contains(&sb->ignore_list, &commit->object.oid)) {\n>> +    if (commit->object.flags & BLAME_IGNORE) {\n>>           for (i = 0, sg = first_scapegoat(revs, commit, sb->reverse);\n>>                i < num_sg && sg;\n>>                sg = sg->next, i++) {\n>> diff --git a/blame.h b/blame.h\n>> index b6bbee4147..d35167e8bd 100644\n>> --- a/blame.h\n>> +++ b/blame.h\n>> @@ -16,6 +16,9 @@\n>>   #define BLAME_DEFAULT_MOVE_SCORE    20\n>>   #define BLAME_DEFAULT_COPY_SCORE    40\n>>\n>> +/* Remember to update object flag allocation in object.h */\n>> +#define BLAME_IGNORE    (1u<<14)\n>> +\n>>   struct fingerprint;\n>>\n>>   /*\n>> @@ -125,8 +128,6 @@ struct blame_scoreboard {\n>>       /* linked list of blames */\n>>       struct blame_entry *ent;\n>>\n>> -    struct oidset ignore_list;\n>> -\n>>       /* look-up a line in the final buffer */\n>>       int num_lines;\n>>       int *lineno;\n>> diff --git a/builtin/blame.c b/builtin/blame.c\n>> index bb0f29300e..1c6721b5d5 100644\n>> --- a/builtin/blame.c\n>> +++ b/builtin/blame.c\n>> @@ -830,21 +830,29 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n>>   {\n>>       struct string_list_item *i;\n>>       struct object_id oid;\n>> +    const struct object_id *o;\n>> +    struct oidset_iter iter;\n>> +    struct oidset ignore_list = OIDSET_INIT;\n>>\n>> -    oidset_init(&sb->ignore_list, 0);\n>>       for_each_string_list_item(i, ignore_revs_file_list) {\n>>           if (!strcmp(i->string, \"\"))\n>> -            oidset_clear(&sb->ignore_list);\n>> +            oidset_clear(&ignore_list);\n>>           else\n>> -            oidset_parse_file_carefully(&sb->ignore_list, i->string,\n>> +            oidset_parse_file_carefully(&ignore_list, i->string,\n>>                               peel_to_commit_oid, sb);\n>>       }\n>>       for_each_string_list_item(i, ignore_rev_list) {\n>>           if (get_oid_committish(i->string, &oid) ||\n>>               peel_to_commit_oid(&oid, sb))\n>>               die(_(\"cannot find revision %s to ignore\"), i->string);\n>> -        oidset_insert(&sb->ignore_list, &oid);\n>> +        oidset_insert(&ignore_list, &oid);\n>>       }\n>> +    oidset_iter_init(&ignore_list, &iter);\n>> +    while ((o = oidset_iter_next(&iter))) {\n>> +        struct commit *commit = lookup_commit(sb->repo, o);\n>> +        commit->object.flags |= BLAME_IGNORE;\n>> +    }\n>> +    oidset_clear(&ignore_list);\n\nWithout this cleanup the number of brk(2) calls goes up to 24071 for\nme, increasing system time beyond the one for rc1.\n\nSo it seems the improvement comes from allocating a few MB (the oidset)\nand releasing it again to pre-size the heap and avoid thousands of\nsystem calls that would otherwise extend it lazily.\n\nThe patch below on top of rc1 simulates this.  New day, new numbers,\nthis time with less background programs; here are the times for rc1:\n\nBenchmark #1: ./git-blame --ignore-revs-file=hashes Makefile\n  Time (mean ± σ):      8.210 s ±  0.020 s    [User: 7.647 s, System: 0.558 s]\n  Range (min … max):    8.182 s …  8.258 s    10 runs\n\nAnd here with the patch at the bottom:\n\nBenchmark #1: ./git-blame --ignore-revs-file=hashes Makefile\n  Time (mean ± σ):      7.879 s ±  0.023 s    [User: 7.827 s, System: 0.052 s]\n  Range (min … max):    7.859 s …  7.936 s    10 runs\n\nMy conclusion: object flags won last time by cheating -- lookup speed\nisn't really all that different, what matters is allocation overhead.\nExtending the heap by just a few MB helps a lot.  Which is very likely\nto be a platform-specific (system-specific even?) win.\n\nSo let's drop this.  But it shows that a better direction for\nimproving performance might be to reduce the number of allocations,\ne.g. by using a mem_pool.\n\n>>   }\n>>\n>>   int cmd_blame(int argc, const char **argv, const char *prefix)\n>> diff --git a/object.h b/object.h\n>> index 20b18805f0..6818c9296b 100644\n>> --- a/object.h\n>> +++ b/object.h\n>> @@ -64,7 +64,8 @@ struct object_array {\n>>    * negotiator/default.c:       2--5\n>>    * walker.c:                 0-2\n>>    * upload-pack.c:                4       11-----14  16-----19\n>> - * builtin/blame.c:                        12-13\n>> + * blame.c:                                     14\n>> + * builtin/blame.c:                        12---14\n>>    * bisect.c:                                        16\n>>    * bundle.c:                                        16\n>>    * http-push.c:                          11-----14\n>> --\n>> 2.28.0\n>>\n>\n\n---\n builtin/blame.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex bb0f29300e..aa6970f452 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -845,6 +845,7 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n \t\t\tdie(_(\"cannot find revision %s to ignore\"), i->string);\n \t\toidset_insert(&sb->ignore_list, &oid);\n \t}\n+\tfree(xmalloc(10*1000*1000));\n }\n\n int cmd_blame(int argc, const char **argv, const char *prefix)\n--\n2.28.0\n"}]}