{"thread":{"id":"50175","subject":"[PATCH] blame: add the ability to ignore commits","startedAt":"2019-01-07T21:30:28Z","lastAt":"2019-02-01T20:37:11Z","messageCount":39,"participants":["Barret Rhoden","Junio C Hamano","Ævar Arnfjörð Bjarmason","Stefan Beller","Randall S. Becker","Johannes Schindelin","Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"366291","messageId":"20190107213013.231514-1-brho@google.com","threadId":"50175","inReplyTo":null,"subject":"[PATCH] blame: add the ability to ignore commits","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-07T21:30:12Z","receivedAt":"2019-01-07T21:30:28Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"Commits that make formatting changes or renames are often not\ninteresting when blaming a file. This commit, similar to\ngit-hyper-blame, allows one to specify certain revisions to ignore during\nthe blame process.\n\nTo ignore a revision, put its committish in a file specified by\n--ignore-file= or use -i , which can be repeated. The file\n.git-blame-ignore-revs is checked by default.\n\nIt's useful to be alerted to the presence of an ignored commit in the\nhistory of a line. Those lines will be marked with '*' in the\nnon-porcelain output. The '*' is attached to the line number to keep\nfrom breaking tools that rely on the whitespace between columns.\n\nA blame_entry attributed to an ignored commit will get passed to its\nparent. If an ignored commit changed a line, an ancestor that changed\nthe line will get blamed. However, if an ignored commit added lines, a\ncommit changing a nearby line may get blamed. If no commit is found,\nthe original commit for the file will get blamed.\n\nSigned-off-by: Barret Rhoden \n---\n Documentation/git-blame.txt | 15 +++++++++\n blame.c | 38 +++++++++++++++++----\n blame.h | 3 ++\n builtin/blame.c | 66 +++++++++++++++++++++++++++++++++++--\n 4 files changed, 112 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 16323eb80e31..e41375374892 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n [verse]\n 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]\n \t [-L ] [-S ] [-M] [-C] [-C] [-C] [--since=]\n+\t [-i ] [--no-default-ignores] [--ignore-file=]\n \t [--progress] [--abbrev=] [ | --contents | --reverse ..]\n \t [--] \n \n@@ -84,6 +85,20 @@ include::blame-options.txt[]\n \tIgnore whitespace when comparing the parent's version and\n \tthe child's to find where the lines came from.\n \n+-i ::\n+\tIgnore revision when assigning blame. Lines that were changed by an\n+\tignored commit will be marked with a `*` in the blame output. Lines\n+\tthat were added by an ignored commit may be attributed commits making\n+\tnearby changes or to the first commit touching the file.\n+\n+--no-default-ignores::\n+\tDo not automatically ignore revisions in the file\n+\t`.git-blame-ignore-revs`.\n+\n+--ignore-file=::\n+\tIgnore revisions listed in `file`, one revision per line. Whitespace\n+\tand comments beginning with `#` are ignored.\n+\n --abbrev=::\n \tInstead of using the default 7+1 hexadecimal digits as the\n \tabbreviated object name, use +1 digits. Note that 1 column\ndiff --git a/blame.c b/blame.c\nindex d84c93778080..9e338cfa83e3 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -474,7 +474,8 @@ void blame_coalesce(struct blame_scoreboard *sb)\n \n \tfor (ent = sb->ent; ent && (next = ent->next); ent = next) {\n \t\tif (ent->suspect == next->suspect &&\n-\t\t ent->s_lno + ent->num_lines == next->s_lno) {\n+\t\t ent->s_lno + ent->num_lines == next->s_lno &&\n+\t\t ent->ignored == next->ignored) {\n \t\t\tent->num_lines += next->num_lines;\n \t\t\tent->next = next->next;\n \t\t\tblame_origin_decref(next->suspect);\n@@ -726,6 +727,8 @@ static void split_overlap(struct blame_entry *split,\n \tint chunk_end_lno;\n \tmemset(split, 0, sizeof(struct blame_entry [3]));\n \n+\tsplit[0].ignored = split[1].ignored = split[2].ignored = e->ignored;\n+\n \tif (e->s_lno < tlno) {\n \t\t/* there is a pre-chunk part not blamed on parent */\n \t\tsplit[0].suspect = blame_origin_incref(e->suspect);\n@@ -845,10 +848,10 @@ static struct blame_entry *reverse_blame(struct blame_entry *head,\n */\n static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tint tlno, int offset, int same,\n-\t\t\tstruct blame_origin *parent)\n+\t\t\tstruct blame_origin *parent, int ignore_diffs)\n {\n \tstruct blame_entry *e = **srcq;\n-\tstruct blame_entry *samep = NULL, *diffp = NULL;\n+\tstruct blame_entry *samep = NULL, *diffp = NULL, *ignoredp = NULL;\n \n \twhile (e && e->s_lno < tlno) {\n \t\tstruct blame_entry *next = e->next;\n@@ -862,6 +865,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tint len = tlno - e->s_lno;\n \t\t\tstruct blame_entry *n = xcalloc(1, sizeof (struct blame_entry));\n \t\t\tn->suspect = e->suspect;\n+\t\t\tn->ignored = e->ignored;\n \t\t\tn->lno = e->lno + len;\n \t\t\tn->s_lno = e->s_lno + len;\n \t\t\tn->num_lines = e->num_lines - len;\n@@ -916,6 +920,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tint len = same - e->s_lno;\n \t\t\tstruct blame_entry *n = xcalloc(1, sizeof (struct blame_entry));\n \t\t\tn->suspect = blame_origin_incref(e->suspect);\n+\t\t\tn->ignored = e->ignored;\n \t\t\tn->lno = e->lno + len;\n \t\t\tn->s_lno = e->s_lno + len;\n \t\t\tn->num_lines = e->num_lines - len;\n@@ -925,10 +930,24 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tn->next = samep;\n \t\t\tsamep = n;\n \t\t}\n-\t\te->next = diffp;\n-\t\tdiffp = e;\n+\t\tif (ignore_diffs) {\n+\t\t\t/* These go to the parent, like the ones before tlno. */\n+\t\t\tblame_origin_decref(e->suspect);\n+\t\t\te->suspect = blame_origin_incref(parent);\n+\t\t\te->s_lno += offset;\n+\t\t\te->ignored = 1;\n+\t\t\te->next = ignoredp;\n+\t\t\tignoredp = e;\n+\t\t} else {\n+\t\t\te->next = diffp;\n+\t\t\tdiffp = e;\n+\t\t}\n \t\te = next;\n \t}\n+\tif (ignoredp) {\n+\t\t**dstq = reverse_blame(ignoredp, **dstq);\n+\t\t*dstq = &ignoredp->next;\n+\t}\n \t**srcq = reverse_blame(diffp, reverse_blame(samep, e));\n \t/* Move across elements that are in the unblamable portion */\n \tif (diffp)\n@@ -938,6 +957,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n struct blame_chunk_cb_data {\n \tstruct blame_origin *parent;\n \tlong offset;\n+\tint ignore_diffs;\n \tstruct blame_entry **dstq;\n \tstruct blame_entry **srcq;\n };\n@@ -950,7 +970,7 @@ static int blame_chunk_cb(long start_a, long count_a,\n \tif (start_a - start_b != d->offset)\n \t\tdie(\"internal error in blame::blame_chunk_cb\");\n \tblame_chunk(&d->dstq, &d->srcq, start_b, start_a - start_b,\n-\t\t start_b + count_b, d->parent);\n+\t\t start_b + count_b, d->parent, d->ignore_diffs);\n \td->offset = start_a + count_a - (start_b + count_b);\n \treturn 0;\n }\n@@ -973,18 +993,22 @@ static void pass_blame_to_parent(struct blame_scoreboard *sb,\n \n \td.parent = parent;\n \td.offset = 0;\n+\td.ignore_diffs = 0;\n \td.dstq = &newdest; d.srcq = &target->suspects;\n \n \tfill_origin_blob(&sb->revs->diffopt, parent, &file_p, &sb->num_read_blob);\n \tfill_origin_blob(&sb->revs->diffopt, target, &file_o, &sb->num_read_blob);\n \tsb->num_get_patch++;\n \n+\tif (oidset_contains(&sb->ignores, &target->commit->object.oid))\n+\t\td.ignore_diffs = 1;\n+\n \tif (diff_hunks(&file_p, &file_o, blame_chunk_cb, &d, sb->xdl_opts))\n \t\tdie(\"unable to generate diff (%s -> %s)\",\n \t\t oid_to_hex(&parent->commit->object.oid),\n \t\t oid_to_hex(&target->commit->object.oid));\n \t/* The rest are the same as the parent */\n-\tblame_chunk(&d.dstq, &d.srcq, INT_MAX, d.offset, INT_MAX, parent);\n+\tblame_chunk(&d.dstq, &d.srcq, INT_MAX, d.offset, INT_MAX, parent, 0);\n \t*d.dstq = NULL;\n \tqueue_blames(sb, parent, newdest);\n \ndiff --git a/blame.h b/blame.h\nindex be3a895043e0..3fe71a59d372 100644\n--- a/blame.h\n+++ b/blame.h\n@@ -92,6 +92,7 @@ struct blame_entry {\n \t * scanning the lines over and over.\n \t */\n \tunsigned score;\n+\tint ignored;\n };\n \n /*\n@@ -117,6 +118,8 @@ struct blame_scoreboard {\n \t/* linked list of blames */\n \tstruct blame_entry *ent;\n \n+\tstruct oidset ignores;\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 6d798f99392e..698834426771 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -516,8 +516,13 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\t\t\t\t\t ci.author_tz.buf,\n \t\t\t\t\t\t show_raw_time));\n \t\t\t}\n-\t\t\tprintf(\" %*d) \",\n-\t\t\t max_digits, ent->lno + 1 + cnt);\n+\t\t\tif (ent->ignored) {\n+\t\t\t\tprintf(\" %*d%c) \", max_digits - 1,\n+\t\t\t\t ent->lno + 1 + cnt, '*');\n+\t\t\t} else {\n+\t\t\t\tprintf(\" %*d) \", max_digits,\n+\t\t\t\t ent->lno + 1 + cnt);\n+\t\t\t}\n \t\t}\n \t\tif (reset)\n \t\t\tfputs(reset, stdout);\n@@ -603,6 +608,7 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)\n {\n \tint longest_src_lines = 0;\n \tint longest_dst_lines = 0;\n+\tint has_ignore = 0;\n \tunsigned largest_score = 0;\n \tstruct blame_entry *e;\n \tint compute_auto_abbrev = (abbrev < 0);\n@@ -639,9 +645,11 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)\n \t\t\tlongest_dst_lines = num;\n \t\tif (largest_score < blame_entry_score(sb, e))\n \t\t\tlargest_score = blame_entry_score(sb, e);\n+\t\tif (e->ignored)\n+\t\t\thas_ignore = 1;\n \t}\n \tmax_orig_digits = decimal_width(longest_src_lines);\n-\tmax_digits = decimal_width(longest_dst_lines);\n+\tmax_digits = decimal_width(longest_dst_lines) + has_ignore;\n \tmax_score_digits = decimal_width(largest_score);\n \n \tif (compute_auto_abbrev)\n@@ -774,6 +782,43 @@ static int is_a_rev(const char *name)\n \treturn OBJ_NONE < oid_object_info(the_repository, &oid, NULL);\n }\n \n+static void handle_ignore_list(struct blame_scoreboard *sb,\n+\t\t\t struct string_list *ignores)\n+{\n+\tstruct string_list_item *i;\n+\tstruct object_id oid;\n+\n+\toidset_init(&sb->ignores, 0);\n+\tfor_each_string_list_item(i, ignores) {\n+\t\tif (get_oid_committish(i->string, &oid))\n+\t\t\tdie(_(\"Can't find revision '%s' to ignore\"), i->string);\n+\t\toidset_insert(&sb->ignores, &oid);\n+\t}\n+}\n+\n+static int handle_ignore_file(const char *path, struct string_list *ignores)\n+{\n+\tFILE *fp = fopen(path, \"r\");\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (!fp)\n+\t\treturn -1;\n+\twhile (!strbuf_getline(&sb, fp)) {\n+\t\tconst char *hash;\n+\n+\t\thash = strchr(sb.buf, '#');\n+\t\tif (hash)\n+\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n+\t\tstrbuf_trim(&sb);\n+\t\tif (!sb.len)\n+\t\t\tcontinue;\n+\t\tstring_list_append(ignores, sb.buf);\n+\t}\n+\tfclose(fp);\n+\tstrbuf_release(&sb);\n+\treturn 0;\n+}\n+\n int cmd_blame(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n@@ -785,8 +830,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct progress_info pi = { NULL, 0 };\n \n \tstruct string_list range_list = STRING_LIST_INIT_NODUP;\n+\tstruct string_list ignore_list = STRING_LIST_INIT_DUP;\n \tint output_option = 0, opt = 0;\n \tint show_stats = 0;\n+\tint no_default_ignores = 0;\n+\tconst char *ignore_file = NULL;\n \tconst char *revs_file = NULL;\n \tconst char *contents_from = NULL;\n \tconst struct option options[] = {\n@@ -806,6 +854,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('s', NULL, &output_option, N_(\"Suppress author name and timestamp (Default: off)\"), OUTPUT_NO_AUTHOR),\n \t\tOPT_BIT('e', \"show-email\", &output_option, N_(\"Show author email instead of name (Default: off)\"), OUTPUT_SHOW_EMAIL),\n \t\tOPT_BIT('w', NULL, &xdl_opts, N_(\"Ignore whitespace differences\"), XDF_IGNORE_WHITESPACE),\n+\t\tOPT_STRING_LIST('i', NULL, &ignore_list, N_(\"rev\"), N_(\"Ignore when blaming\")),\n+\t\tOPT_BOOL(0, \"no-default-ignores\", &no_default_ignores, N_(\"Do not ignore revisions from the .git-blame-ignore-revs file\")),\n+\t\tOPT_STRING(0, \"ignore-file\", &ignore_file, N_(\"file\"), N_(\"Ignore revisions from \")),\n \t\tOPT_BIT(0, \"color-lines\", &output_option, N_(\"color redundant metadata from previous line differently\"), OUTPUT_COLOR_LINE),\n \t\tOPT_BIT(0, \"color-by-age\", &output_option, N_(\"color lines by age\"), OUTPUT_SHOW_AGE_WITH_COLOR),\n \n@@ -987,6 +1038,13 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\targv[argc - 1] = \"--\";\n \t}\n \n+\tif (!no_default_ignores)\n+\t\thandle_ignore_file(\".git-blame-ignore-revs\", &ignore_list);\n+\tif (ignore_file) {\n+\t\tif (handle_ignore_file(ignore_file, &ignore_list))\n+\t\t\tdie(_(\"Unable to open ignore-file '%s'\"), ignore_file);\n+\t}\n+\n \trevs.disable_stdin = 1;\n \tsetup_revisions(argc, argv, &revs, NULL);\n \n@@ -995,6 +1053,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tsb.contents_from = contents_from;\n \tsb.reverse = reverse;\n \tsb.repo = the_repository;\n+\thandle_ignore_list(&sb, &ignore_list);\n+\tstring_list_clear(&ignore_list, 0);\n \tsetup_scoreboard(&sb, path, &o);\n \tlno = sb.num_lines;\n \n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366299","messageId":"xmqqbm4s6ozl.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"20190107213013.231514-1-brho@google.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-07T23:13:02Z","receivedAt":"2019-01-07T23:13:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Barret Rhoden writes:\n\n> Commits that make formatting changes or renames are often not\n> interesting when blaming a file. This commit, similar to\n> git-hyper-blame, allows one to specify certain revisions to ignore during\n> the blame process.\n>\n> To ignore a revision, put its committish in a file specified by\n> --ignore-file= or use -i , which can be repeated.\n\nIf I read it correctly, this gives a very limited form of -S, in the\nsense that anything this can do can be expressed by using -S but the\nreverse is not true, but is designed to be easier to use, in the\nsense that unlike -S, this does not have to describe the part of the\nhistory you do not have to lie about. The documentation should say\nsomething about these pros-and-cons to help readers decide which\nfeature to use.\n\nI somehow feel that this is rare enough that it should not squat on\nshort-and-sweet '-i'. We would want to reserve it to something like\n\"--ignore-case\", for example.\n\n> The file .git-blame-ignore-revs is checked by default.\n\nGiving the projects a way to easily help participants to share the\nsame distorted view of the history is a good idea, but I do not\nthink we should allow projects to do so to those who merely clone it\nwithout their consent. IOW, \"by default\" is a terrible idea.\n\nPerhaps paying attention to blame.skipRevsFile configuration\nvariable that is set by the end user would be sufficient----the\nproject can ship .blame-skip-revs (or any random filename of their\nchoice) in-tree as tracked contents and tell its users that they can\noptionally use it.\n\n> It's useful to be alerted to the presence of an ignored commit in the\n> history of a line. Those lines will be marked with '*' in the\n> non-porcelain output. The '*' is attached to the line number to keep\n> from breaking tools that rely on the whitespace between columns.\n\nA policy decision like the above two shouldn't be hardcoded in the\nfeature like this, but should be done as a separate option. By\ndefault, these shouldn't be marked with '*', as the same tools you\nsaid you are afraid of breaking would be expecting a word with only\ndigits and no asterisk in the column where line numbers appear and\nwill get broken by this change if done unconditionally.\n\nIn general, I find this patch trying to change too many things at\nthe same time, without sufficient justification. Perhaps do these\ndifferent things as separate steps in a single series?\n\n> A blame_entry attributed to an ignored commit will get passed to its\n> parent.\n\nObviously, an interesting consideration is what happens when a merge\ncommit is skipped. Is it sufficient to change this description to\n\"...will get passed to its parentS\", or would the code do completely\nnonsensical things without further tweaks (a possible simple tweak\ncould be to refuse skipping merges)?\n\n> If an ignored commit changed a line, an ancestor that changed\n> the line will get blamed. However, if an ignored commit added lines, a\n> commit changing a nearby line may get blamed. If no commit is found,\n> the original commit for the file will get blamed.\n\nThe above somehow does not read as describing a carefully designed\nbehaviour; rather, it sounds as if it is saying \"the code does\nwhatever random things it happens to do\". For example, when there\nis a newly added line how is \"A\" commit changing a nearby line\nchosen (a line has lines before it and after it---they may be\ntouched by different commits, and before and after that skipped\ncommit, so there are multiple commits to choose from)?\n\n> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\n> index 16323eb80e31..e41375374892 100644\n> --- a/Documentation/git-blame.txt\n> +++ b/Documentation/git-blame.txt\n> @@ -10,6 +10,7 @@ SYNOPSIS\n> [verse]\n> 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]\n> \t [-L ] [-S ] [-M] [-C] [-C] [-C] [--since=]\n> +\t [-i ] [--no-default-ignores] [--ignore-file=]\n> \t [--progress] [--abbrev=] [ | --contents | --reverse ..]\n> \t [--] \n> \n> @@ -84,6 +85,20 @@ include::blame-options.txt[]\n> \tIgnore whitespace when comparing the parent's version and\n> \tthe child's to find where the lines came from.\n> \n> +-i ::\n> +\tIgnore revision when assigning blame. Lines that were changed by an\n> +\tignored commit will be marked with a `*` in the blame output. Lines\n> +\tthat were added by an ignored commit may be attributed commits making\n> +\tnearby changes or to the first commit touching the file.\n\nIt probably deserves to be told that this option can be given\nmultiple times and used cumulatively (unlike usual \"last one wins\"\nrule).\n\n> +--no-default-ignores::\n> +\tDo not automatically ignore revisions in the file\n> +\t`.git-blame-ignore-revs`.\n\nThis should not be \"opt-out\" like this.\n\n> +--ignore-file=::\n> +\tIgnore revisions listed in `file`, one revision per line. Whitespace\n> +\tand comments beginning with `#` are ignored.\n\nShould it be capable to take two or more ignore-files? Or should we\nuse the usual \"the last one wins\" rule?\n\nI think we should support blame.skipRevFile configuration variable\nso that the users do not have to constantly give the option from the\ncommand line. And with that, there is no need to have a hardcoded\nfilename .git-blame-ignore-revs or anything like that.\n\nIf we are to use configuration variable, however, we'd need a way to\noverride its use from the command line, too. Perhaps a sane\narrangement would be\n\n - if one or more --skip-revs-file= are given from the\n command line, use all of them and ignore blame.skipRevsFile\n configuration variable.\n\n - if no --skip-revs-file= is given from the command line,\n use blame.skipRevsFile configuration variable.\n\n - regardless of the above two, pay attention to --skip-rev=\n command line option(s).\n\n\nSomehow the damage to blame.c codebase looks way too noisy for what\nthe code wants to do. If all we want is to pretend in a history,\ne.g.\n\n ---O---A---B---C---D\n\nthat commit B does not exist, i.e. use a distorted view of the\nhistory\n\n ---O---A-------C---D\n\nwouldn't it be sufficient to modify pass_blame(), i.e. the only the\ncaller of the pass_blame_to_parent(), where we find the parent\ncommits of \"C\" and dig the history to pass blame to \"B\", and have it\npretend as if \"B\" does not exist and pass blame directly to \"A\"?\n\nThanks. I am personally not all that interested in this yet.\n"},{"id":"366325","messageId":"8736q3qon0.fsf@evledraar.gmail.com","threadId":"50175","inReplyTo":"20190107213013.231514-1-brho@google.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-08T13:12:35Z","receivedAt":"2019-01-08T13:12:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 07 2019, Barret Rhoden wrote:\n\n> +static int handle_ignore_file(const char *path, struct string_list *ignores)\n> +{\n> +\tFILE *fp = fopen(path, \"r\");\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tif (!fp)\n> +\t\treturn -1;\n> +\twhile (!strbuf_getline(&sb, fp)) {\n> +\t\tconst char *hash;\n> +\n> +\t\thash = strchr(sb.buf, '#');\n> +\t\tif (hash)\n> +\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n> +\t\tstrbuf_trim(&sb);\n> +\t\tif (!sb.len)\n> +\t\t\tcontinue;\n> +\t\tstring_list_append(ignores, sb.buf);\n> +\t}\n> +\tfclose(fp);\n> +\tstrbuf_release(&sb);\n> +\treturn 0;\n> +}\n\nAside from other comments on this patch that Junio had either you mostly\ncopy-pasted this from init_skiplist() or you've come up with almost the\nsame code on your own.\n\nIn any case, if we're going to integrate something like this patch let's\nsplit this \"parse file with SHA-1s or comments/whitespace\" into a\nutility function that both this and init_skiplist() can call.\n\nThen we could split up the description for the fsck.skipList config\nvariable to reference that format, and say that both it and this new\nthing should consult those docs for how it's parsed.\n"},{"id":"366335","messageId":"20190108112742.7878d4cb@gnomeregan.cam.corp.google.com","threadId":"50175","inReplyTo":"xmqqbm4s6ozl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-08T16:27:42Z","receivedAt":"2019-01-08T16:27:48Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-07 at 15:13 Junio C Hamano wrote:\n> If I read it correctly, this gives a very limited form of -S, in the\n> sense that anything this can do can be expressed by using -S but the\n> reverse is not true, but is designed to be easier to use, in the\n> sense that unlike -S, this does not have to describe the part of the\n> history you do not have to lie about. The documentation should say\n> something about these pros-and-cons to help readers decide which\n> feature to use.\n\nYeah, -S lists the revs to use, this lists the revs to *not* use. I'll\nadd a note.\n\n> I somehow feel that this is rare enough that it should not squat on\n> short-and-sweet '-i'. We would want to reserve it to something like\n> \"--ignore-case\", for example.\n\nCan do. I'll change the interface to your suggestion from down below.\n\n> > The file .git-blame-ignore-revs is checked by default. \n> \n> Giving the projects a way to easily help participants to share the\n> same distorted view of the history is a good idea, but I do not\n> think we should allow projects to do so to those who merely clone it\n> without their consent. IOW, \"by default\" is a terrible idea.\n> \n> Perhaps paying attention to blame.skipRevsFile configuration\n> variable that is set by the end user would be sufficient----the\n> project can ship .blame-skip-revs (or any random filename of their\n> choice) in-tree as tracked contents and tell its users that they can\n> optionally use it.\n\nA blame config option works for me. I'd like the users/cloners of a\nrepo to not need to do anything extravagant, but a one-time config\nwould be fine.\n\n> > It's useful to be alerted to the presence of an ignored commit in the\n> > history of a line. Those lines will be marked with '*' in the\n> > non-porcelain output. The '*' is attached to the line number to keep\n> > from breaking tools that rely on the whitespace between columns. \n> \n> A policy decision like the above two shouldn't be hardcoded in the\n> feature like this, but should be done as a separate option. By\n> default, these shouldn't be marked with '*', as the same tools you\n> said you are afraid of breaking would be expecting a word with only\n> digits and no asterisk in the column where line numbers appear and\n> will get broken by this change if done unconditionally.\n\nSince users are already opting-in to the blame-ignore, do you also want\nthem to opt-in to the annotation? I can make a separate config option\nto turn on the annotation. Any preference for how it is marked?\n\n> In general, I find this patch trying to change too many things at\n> the same time, without sufficient justification. Perhaps do these\n> different things as separate steps in a single series?\n> \n> > A blame_entry attributed to an ignored commit will get passed to its\n> > parent. \n> \n> Obviously, an interesting consideration is what happens when a merge\n> commit is skipped. Is it sufficient to change this description to\n> \"...will get passed to its parentS\", or would the code do completely\n> nonsensical things without further tweaks (a possible simple tweak\n> could be to refuse skipping merges)?\n\nIf we skip a merge commit, it might pick the wrong parent. For\nexample, this line was brought in via a merge:\n\n$ ~/src/git/git-blame include/linux/mm.h | grep VM_SYNC\nb6fb293f2497a (Jan Kara 2017-11-01 16:36:41 +0100 204) #define VM_SYNC\n\nIt's from merge: a3841f94c7ec (\"Merge tag 'libnvdimm-for-4.15', and if\nwe ignore it:\n\n$ ~/src/git/git-blame -i a3841f94c7ecb include/linux/mm.h | grep VM_SYNC\ncc2383ec06be0 (Konstantin Khlebnikov 2012-10-08 16:28:37 -0700 204*) #define VM_SYNC\n\nThe wrong commit is blamed.\n\nI can put a note in the doc about it and die if we're given a merge\ncommit. Is there a convenient helper for detecting a merge, or can I\njust check for multiple parents? (something like commit->parents &&\ncommit->parents->next)\n \n> > If an ignored commit changed a line, an ancestor that changed\n> > the line will get blamed. However, if an ignored commit added lines, a\n> > commit changing a nearby line may get blamed. If no commit is found,\n> > the original commit for the file will get blamed. \n> \n> The above somehow does not read as describing a carefully designed\n> behaviour; rather, it sounds as if it is saying \"the code does\n> whatever random things it happens to do\". For example, when there\n> is a newly added line how is \"A\" commit changing a nearby line\n> chosen (a line has lines before it and after it---they may be\n> touched by different commits, and before and after that skipped\n> commit, so there are multiple commits to choose from)?\n\nThis was more of a commentary about its behavior. If you ignore a\ncommit that added lines, it'd be nice to get a hint of what might have\ncaused it, and picking a commit that affected an adjacent line seemed\nfine. But yeah, it's not doing anything crazy.\n\n> > diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\n> > index 16323eb80e31..e41375374892 100644\n> > --- a/Documentation/git-blame.txt\n> > +++ b/Documentation/git-blame.txt\n> > @@ -10,6 +10,7 @@ SYNOPSIS\n> > [verse]\n> > 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]\n> > \t [-L ] [-S ] [-M] [-C] [-C] [-C] [--since=]\n> > +\t [-i ] [--no-default-ignores] [--ignore-file=]\n> > \t [--progress] [--abbrev=] [ | --contents | --reverse ..]\n> > \t [--] \n> > \n> > @@ -84,6 +85,20 @@ include::blame-options.txt[]\n> > \tIgnore whitespace when comparing the parent's version and\n> > \tthe child's to find where the lines came from.\n> > \n> > +-i ::\n> > +\tIgnore revision when assigning blame. Lines that were changed by an\n> > +\tignored commit will be marked with a `*` in the blame output. Lines\n> > +\tthat were added by an ignored commit may be attributed commits making\n> > +\tnearby changes or to the first commit touching the file. \n> \n> It probably deserves to be told that this option can be given\n> multiple times and used cumulatively (unlike usual \"last one wins\"\n> rule).\n> \n> > +--no-default-ignores::\n> > +\tDo not automatically ignore revisions in the file\n> > +\t`.git-blame-ignore-revs`. \n> \n> This should not be \"opt-out\" like this.\n> \n> > +--ignore-file=::\n> > +\tIgnore revisions listed in `file`, one revision per line. Whitespace\n> > +\tand comments beginning with `#` are ignored. \n> \n> Should it be capable to take two or more ignore-files? Or should we\n> use the usual \"the last one wins\" rule?\n> \n> I think we should support blame.skipRevFile configuration variable\n> so that the users do not have to constantly give the option from the\n> command line. And with that, there is no need to have a hardcoded\n> filename .git-blame-ignore-revs or anything like that.\n> \n> If we are to use configuration variable, however, we'd need a way to\n> override its use from the command line, too. Perhaps a sane\n> arrangement would be\n> \n> - if one or more --skip-revs-file= are given from the\n> command line, use all of them and ignore blame.skipRevsFile\n> configuration variable.\n> \n> - if no --skip-revs-file= is given from the command line,\n> use blame.skipRevsFile configuration variable.\n> \n> - regardless of the above two, pay attention to --skip-rev=\n> command line option(s).\n\nSounds fine to me.\n\n> Somehow the damage to blame.c codebase looks way too noisy for what\n> the code wants to do. If all we want is to pretend in a history,\n> e.g.\n> \n> ---O---A---B---C---D\n> \n> that commit B does not exist, i.e. use a distorted view of the\n> history\n> \n> ---O---A-------C---D\n> \n> wouldn't it be sufficient to modify pass_blame(), i.e. the only the\n> caller of the pass_blame_to_parent(), where we find the parent\n> commits of \"C\" and dig the history to pass blame to \"B\", and have it\n> pretend as if \"B\" does not exist and pass blame directly to \"A\"?\n\nI originally tried to skip 'B' in pass_blame() when B popped up as a\nscapegoat. That broke the offsets of the blame_entries in the\nparent. By running diff_hunks(), we get the opportunity to adjust the\noffsets. Also, when it comes to marking the blame_entries for marking\nthe output, we want to know the specific diffs and to break them up at\nthe boundaries of [tlno,same) in blame_chunk().\n\n> Thanks. I am personally not all that interested in this yet.\n\nThanks for taking a look.\n\nBarret\n\n"},{"id":"366337","messageId":"20190108114106.5cf57e67@gnomeregan.cam.corp.google.com","threadId":"50175","inReplyTo":"8736q3qon0.fsf@evledraar.gmail.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-08T16:41:06Z","receivedAt":"2019-01-08T16:41:12Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-08 at 14:12 Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Jan 07 2019, Barret Rhoden wrote:\n> \n> > +static int handle_ignore_file(const char *path, struct string_list *ignores)\n> > +{\n> > +\tFILE *fp = fopen(path, \"r\");\n> > +\tstruct strbuf sb = STRBUF_INIT;\n> > +\n> > +\tif (!fp)\n> > +\t\treturn -1;\n> > +\twhile (!strbuf_getline(&sb, fp)) {\n> > +\t\tconst char *hash;\n> > +\n> > +\t\thash = strchr(sb.buf, '#');\n> > +\t\tif (hash)\n> > +\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n> > +\t\tstrbuf_trim(&sb);\n> > +\t\tif (!sb.len)\n> > +\t\t\tcontinue;\n> > +\t\tstring_list_append(ignores, sb.buf);\n> > +\t}\n> > +\tfclose(fp);\n> > +\tstrbuf_release(&sb);\n> > +\treturn 0;\n> > +} \n> \n> Aside from other comments on this patch that Junio had either you mostly\n> copy-pasted this from init_skiplist() or you've come up with almost the\n> same code on your own.\n> \n> In any case, if we're going to integrate something like this patch let's\n> split this \"parse file with SHA-1s or comments/whitespace\" into a\n> utility function that both this and init_skiplist() can call.\n\nOne minor difference is that fsck wants an unabbreviated SHA-1, using\nparse_oid_hex() instead of get_oid_committish(). Would you be OK with\nalso changing fsck to take a committish instead of a full SHA-1?\n\nIs there a good place for the common helper? Since it's an oidset, I\ncould put it in oidset.c. oidset_parse_file() or something.\n\n> Then we could split up the description for the fsck.skipList config\n> variable to reference that format, and say that both it and this new\n> thing should consult those docs for how it's parsed.\n\nIs there a good spot for the generic skipList documentation? The only\ncommon text would be: \n\n\t... comments ('#'), empty lines, and any leading and trailing\n\twhitespace is ignored\n\nThanks,\n\nBarret\n\n"},{"id":"366348","messageId":"20190108130418.052c52d5@gnomeregan.cam.corp.google.com","threadId":"50175","inReplyTo":"20190108114106.5cf57e67@gnomeregan.cam.corp.google.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-08T18:04:18Z","receivedAt":"2019-01-08T18:04:25Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-08 at 11:41 Barret Rhoden wrote:\n> Would you be OK with\n> also changing fsck to take a committish instead of a full SHA-1?\n\nActually, in retrospect, I can keep the unabbreviated SHA-1 for the\nfile inputs and use get_oid_committish() for the one-off --skip-rev=\ncases.\n\nThanks,\n\nBarret\n\n"},{"id":"366352","messageId":"xmqqimyz57l9.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"20190108112742.7878d4cb@gnomeregan.cam.corp.google.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-08T18:26:26Z","receivedAt":"2019-01-08T18:26:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Barret Rhoden writes:\n\n>> A policy decision like the above two shouldn't be hardcoded in the\n>> feature like this, but should be done as a separate option. By\n>> default, these shouldn't be marked with '*', as the same tools you\n>> said you are afraid of breaking would be expecting a word with only\n>> digits and no asterisk in the column where line numbers appear and\n>> will get broken by this change if done unconditionally.\n>\n> Since users are already opting-in to the blame-ignore, do you also want\n> them to opt-in to the annotation?\n\nAbsolutely.\n\nAfter all, the users of a moral equivalent that is -S\nnever needed such an extra annotation, which tells us two things.\n(1) the claim \"It's useful to be alerted to the presence of an\nignored commit\" in the proposed log message is merely a personal\npreference and universal users' requirement; (2) if it is useful to\nmark a blame-source whose parents (used while blaming) do not match\nthe actual parents, such an annotation would also be useful while\nrunning -S. So probably it should be a separate option that can be\ngiven when any of the --skip-commit=, --skip-commits-file=,\nr -S option is given.\n\n>> Obviously, an interesting consideration is what happens when a merge\n>> commit is skipped. Is it sufficient to change this description to\n>> \"...will get passed to its parentS\", or would the code do completely\n>> nonsensical things without further tweaks (a possible simple tweak\n>> could be to refuse skipping merges)?\n>\n> If we skip a merge commit, it might pick the wrong parent.\n\nActually after thinking about it a bit more, I no longer think a\nmerge is so special. In this topology (as usual, time flows from\nleft to right), if you are skipping M,\n\n ---A---M---C---D\n /\n ----B\n\nyou'd simply pretend that the ancestry is like this and you'd be\nfine, no?\n\n\n ---A-------C---D\n /\n --------B\n\nThat is, when inspecting C, pass_blame() would enumerate its true\nparents, notice that M to be skipped, which would cause it to\npretend as if C has M's parents instead of M itself as its parents.\nIf M in the true history is the first parent of C, then M's first\nparent in the true history becomes C's first parent, M's second\nparent in the true history becomes C's second parent, etc. (if C\nwere a merge in the true history, such a rewriting would make C an\noctopus in the distorted history)\n\n> The wrong commit is blamed.\n\nSo... I still suspect that it is merely a bug in your implementation\nand there is nothing inherent that forces us to avoid skipping a\nmerge.\n\n>> Somehow the damage to blame.c codebase looks way too noisy for what\n>> the code wants to do. If all we want is to pretend in a history,\n>> e.g.\n>> \n>> ---O---A---B---C---D\n>> \n>> that commit B does not exist, i.e. use a distorted view of the\n>> history\n>> \n>> ---O---A-------C---D\n>> \n>> wouldn't it be sufficient to modify pass_blame(), i.e. the only the\n>> caller of the pass_blame_to_parent(), where we find the parent\n>> commits of \"C\" and dig the history to pass blame to \"B\", and have it\n>> pretend as if \"B\" does not exist and pass blame directly to \"A\"?\n>\n> I originally tried to skip 'B' in pass_blame() when B popped up as a\n> scapegoat. That broke the offsets of the blame_entries in the\n> parent.\n\nWhy? When inspecting C in order to exonerate it from as many lines\nas possible, we'd run \"git diff $P C\" for each parent of C, but in\nthe distorted history (i.e. instead of using $P==B, we use $P==A).\nAs long as the code reads from \"git diff A C\", I do not see why \"the\noffsets in the parent\" can be broken. Care to elaborate a bit more?\n\n"},{"id":"366448","messageId":"20190109154828.1b19dbb4@gnomeregan.cam.corp.google.com","threadId":"50175","inReplyTo":"xmqqimyz57l9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-09T20:48:28Z","receivedAt":"2019-01-09T20:48:36Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"Hi -\n\nOn 2019-01-08 at 10:26 Junio C Hamano wrote:\n> >> Obviously, an interesting consideration is what happens when a merge\n> >> commit is skipped. Is it sufficient to change this description to\n> >> \"...will get passed to its parentS\", or would the code do completely\n> >> nonsensical things without further tweaks (a possible simple tweak\n> >> could be to refuse skipping merges)? \n> >\n> > If we skip a merge commit, it might pick the wrong parent. \n> \n> Actually after thinking about it a bit more, I no longer think a\n> merge is so special. In this topology (as usual, time flows from\n> left to right), if you are skipping M,\n> \n> ---A---M---C---D\n> /\n> ----B\n> \n> you'd simply pretend that the ancestry is like this and you'd be\n> fine, no?\n> \n> \n> ---A-------C---D\n> /\n> --------B\n> \n> That is, when inspecting C, pass_blame() would enumerate its true\n> parents, notice that M to be skipped, which would cause it to\n> pretend as if C has M's parents instead of M itself as its parents.\n> If M in the true history is the first parent of C, then M's first\n> parent in the true history becomes C's first parent, M's second\n> parent in the true history becomes C's second parent, etc. (if C\n> were a merge in the true history, such a rewriting would make C an\n> octopus in the distorted history)\n> \n> > The wrong commit is blamed. \n> \n> So... I still suspect that it is merely a bug in your implementation\n> and there is nothing inherent that forces us to avoid skipping a\n> merge.\n\nProbably! =) I tried a bunch of things initially, and might have had\nthings broken for other reasons. One reason was not calling\nparse_commit() at the right time.\n\nI took another look and had first_scapegoat() scan the parent list, and\nfor any parent in the list, replace it with its own parent list. e.g.\nwhen removing 'A':\n\n ---X---A---B---C\n /\n Y \n \n(B's parent was A, A is the skipped commit, and A has parents X and Y)\n\nbecomes:\n\n ---X---B---C\n /\n Y\n\nBut just yanking skipped commits from the scapegoat list doesn't help,\nsince B gets blamed for the changes made by A, since the file B and\n{X,Y} differ, specifically due to the changes A made. \n\nYou'd think that would have worked, since it sounds similar to what -S\nrevs-file does, but it turns out I want different behavior than -S for\nthe ignore/skip. With -S, you blame a nearby commit (at least based on\nmy testing - what is the desired behavior there?). With ignore/skip, I\nwant to blame the last commit that modified the line.\n\nFor instance, commit X does this:\n\n-foo(x,y);\n+foo(x,y,z);\n\nThen commit Y comes along to reformat it:\n\n-foo(x,y,z);\n+foo(x, y, z);\n\nAnd the history / rev-list for the file looks like:\n\n---O---A---X---B---C---D---Y---E---F\n\nI want to ignore/skip Y and see X in the blame output. \n\nSorry that I wasn't clear about that from the beginning; I can see how\none could have different expectations for what happens when a commit is\nskipped.\n\n> >> Somehow the damage to blame.c codebase looks way too noisy for what\n> >> the code wants to do. If all we want is to pretend in a history,\n> >> e.g.\n> >> \n> >> ---O---A---B---C---D\n> >> \n> >> that commit B does not exist, i.e. use a distorted view of the\n> >> history\n> >> \n> >> ---O---A-------C---D\n> >> \n> >> wouldn't it be sufficient to modify pass_blame(), i.e. the only the\n> >> caller of the pass_blame_to_parent(), where we find the parent\n> >> commits of \"C\" and dig the history to pass blame to \"B\", and have it\n> >> pretend as if \"B\" does not exist and pass blame directly to \"A\"? \n> >\n> > I originally tried to skip 'B' in pass_blame() when B popped up as a\n> > scapegoat. That broke the offsets of the blame_entries in the\n> > parent. \n> \n> Why? When inspecting C in order to exonerate it from as many lines\n> as possible, we'd run \"git diff $P C\" for each parent of C, but in\n> the distorted history (i.e. instead of using $P==B, we use $P==A).\n> As long as the code reads from \"git diff A C\", I do not see why \"the\n> offsets in the parent\" can be broken. Care to elaborate a bit more?\n\nWhen we run \"git diff $P C\" with $P == A, the diffs from B (the one we\nwant to remove) will pop up and be attributed to C. This is what\nhappened when I tried the \"yank B from the history\" approach from\nabove. If we ever diff over a skipped commit like that, we'll blame\nthe wrong one. (C, here).\n\nThe first thing I tried was to simply move the blame_entry list from C\nto B to A without calling diff_hunks(). That's where the \"file offsets\nwere broken,\" since e->s_lno should be adjusted when passing an entry\nfrom the target blame_origin to its parent blame_origin. An example of\nthis is in blame_chunk() when we hand every blame_entry before tlno to\nthe parent in the \"e->s_lno < tlno\" block.\n\nWithout diffing commit B, we can't propagate the blame_entries. And\nif we just diff A and C, we'll get the \"wrong\" commit.\n\nThe other reason to run the diffs was to identify and mark the\nblame_entry that would have been blamed for the skipped commit. That's\nfor the annotations during the output phase. \n\nIf we don't track and taint the blame entries, you could just call\nblame_chunk() from blame_chunk_cb() with tlno = same, which in essence\ntells the blame code that \"this diff belongs to the parent.\" But that\nloses the blame_entry boundaries, and is almost nearly as much of a\nchange to blame.c.\n\nWhen it comes to skipping merges, my existing code couldn't do it, but\nI found a way to make it work:\n\n ---A---M---C---D\n /\n ---X---B\n\nSay the commit changing the line/blame_entry was X, and we try to skip\nM. 'A' made no changes to the line.\n\nIf B is the first scapegoat, pass_blame_to_parent() will see no diff\nbetween B and M for that line, and the blame will get passed to B, and\nwe're done. This is the same both with my change and without.\n\nHowever, if A is the first scapegoat, the line will have a diff, and\nfrom the perspective of the ---A---M---C graph, it looks like M\nintroduced the change. (In fact it did - it was via B though). The\nexisting code would keep the blame on M and then check the next\nscapegoat: B. \n\nWith my change, it couldn't tell the difference between M introducing\na change that we wanted to skip and M having another parent that\nintroduced the change. Both look like a diff from A..M.\n\nI changed my version to handle that case. In pass_blame(), if it fails\nto find a scapegoat in the loop that calls pass_blame_to_parent(),\nit'll make another loop through the scapegoats, where it will pass the\nblame to the first scapegoat that has a diff.\n\nThis way, we get a first attempt to pass the blame for real (e.g. to\nB), and then when we failed, we skip the commit. In other words, we\nonly skip the commit when we need to.\n\nWith that change, from my earlier example:\n\nThis was wrong:\n$ git-blame --skip-rev a3841f94c7ecb include/linux/mm.h | grep VM_SYNC\ncc2383ec06be0 (Konstantin Khlebnikov 2012-10-08 16:28:37 -0700 204*) #define VM_SYNC\n\nThis is right:\n$ git-blame --skip-rev a3841f94c7ecb include/linux/mm.h | grep VM_SYNC\nb6fb293f2497a (Jan Kara 2017-11-01 16:36:41 +0100 204) #define VM_SYNC\n\nAlso note that in the 'right' case it didn't annotate it with the *.\nThat's because we didn't need to ignore the commit. b6fb29 was the\ncorrect one, and we didn't need to ignore M.\n\nAnyway, thanks for reading and for the feedback. I'll clean things up\nand explain it better in another patchset.\n\nThanks,\n\nBarret\n\n"},{"id":"366450","messageId":"CAGZ79kYFC+ws00isp0F1kxmxVx0geg+R0VjjRf0=u2ocbkFUUQ@mail.gmail.com","threadId":"50175","inReplyTo":"xmqqimyz57l9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-09T21:21:11Z","receivedAt":"2019-01-09T21:21:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 8, 2019 at 10:26 AM Junio C Hamano wrote:\n>\n> Barret Rhoden writes:\n>\n> >> A policy decision like the above two shouldn't be hardcoded in the\n> >> feature like this, but should be done as a separate option. By\n> >> default, these shouldn't be marked with '*', as the same tools you\n> >> said you are afraid of breaking would be expecting a word with only\n> >> digits and no asterisk in the column where line numbers appear and\n> >> will get broken by this change if done unconditionally.\n> >\n> > Since users are already opting-in to the blame-ignore, do you also want\n> > them to opt-in to the annotation?\n>\n> Absolutely.\n>\n> After all, the users of a moral equivalent that is -S\n> never needed such an extra annotation, which tells us two things.\n> (1) the claim \"It's useful to be alerted to the presence of an\n> ignored commit\" in the proposed log message is merely a personal\n> preference and universal users' requirement; (2) if it is useful to\n> mark a blame-source whose parents (used while blaming) do not match\n> the actual parents, such an annotation would also be useful while\n> running -S. So probably it should be a separate option that can be\n> given when any of the --skip-commit=, --skip-commits-file=,\n> r -S option is given.\n\nFrom a users point of view it may be desirable to express all this\nin the grafts file, i.e. -S where the syntax of that file is extended.\nFor example we could introduce\n !\nto make it exclude that commit.\nOf course this could lead to confusion, as this puts 2 very different\nconcepts into the same option/file.\n\n\nSpeaking of the implementation:\nThis patch proposes an oid-set that is handled specially\nin blame itself. I wonder if this could be generalized.\n\nJonathan Tan (cc'd) refactored and extended revision walking\nfor git-fetch and its negotiation leading to\n7c85ee6c58 (Merge branch 'jt/fetch-negotiator-skipping',\n2018-08-02), and 3390e42adb (fetch-pack:\nsupport negotiation tip whitelist, 2018-07-02)\nwhich implements another revision walking\nalgorithm that can be used to fine-tune revisions\nwalked when fetching.\n\nI wonder if that work could be generalized more\nto have \"generic\" revision walking algorithms\nand then making use of them in either fetch or\nblame.\n\nFor git-fetch there is a new algorithm that increases\nstep size between commits, which would be funny to\ntry for blame here. It would give the wrong blamed\ncommit, but would speed up blaming a lot.\n\nOmitting some revisions seems to be applicable to\nmore than just blame/fetch, too.\n\nStefan\n"},{"id":"366542","messageId":"xmqq1s5k16zj.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"20190109154828.1b19dbb4@gnomeregan.cam.corp.google.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-10T22:29:52Z","receivedAt":"2019-01-10T22:29:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Barret Rhoden writes:\n\n> For instance, commit X does this:\n>\n> -foo(x,y);\n> +foo(x,y,z);\n>\n> Then commit Y comes along to reformat it:\n>\n> -foo(x,y,z);\n> +foo(x, y, z);\n>\n> And the history / rev-list for the file looks like:\n>\n> ---O---A---X---B---C---D---Y---E---F\n>\n> I want to ignore/skip Y and see X in the blame output. \n\nIf you skip Y, the altered history would have \"foo(x, y, z)\" in E,\n\"foo(x,y,z)\" in X, and \"foo(x,y)\" in A. If you start blaming from\nF, you'd get E as the commit that explains the latest state. If you\ndo not skip Y, you'd get Y. I am not sure how you'd get X in either\ncase. \n"},{"id":"366656","messageId":"20190114101956.2b899d0e@brho.roam.corp.google.com","threadId":"50175","inReplyTo":"xmqq1s5k16zj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-14T15:19:56Z","receivedAt":"2019-01-14T15:20:01Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-10 at 14:29 Junio C Hamano wrote:\n> > For instance, commit X does this:\n> >\n> > -foo(x,y);\n> > +foo(x,y,z);\n> >\n> > Then commit Y comes along to reformat it:\n> >\n> > -foo(x,y,z);\n> > +foo(x, y, z);\n> >\n> > And the history / rev-list for the file looks like:\n> >\n> > ---O---A---X---B---C---D---Y---E---F\n> >\n> > I want to ignore/skip Y and see X in the blame output. \n> \n> If you skip Y, the altered history would have \"foo(x, y, z)\" in E,\n> \"foo(x,y,z)\" in X, and \"foo(x,y)\" in A. If you start blaming from\n> F, you'd get E as the commit that explains the latest state. If you\n> do not skip Y, you'd get Y. I am not sure how you'd get X in either\n> case. \n\nThe way to do it is to let the blames get passed to Y - don't yank it\nfrom the graph. Then when trying to pass the blames from Y to its\nparent, when we get a diff chunk that Y is responsible for, instead of\nkeeping it on Y's suspect list, we hand the blame_entry to D. That\nblame_entry will get passed all the way back to X, which also has a\ndiff that touches that line.\n\nBasically we do the same blame processing as usual, but just don't let\nany blames stick to Y.\n\nBarret\n\n"},{"id":"366660","messageId":"xmqqfttvw2sv.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"20190114101956.2b899d0e@brho.roam.corp.google.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-14T17:45:52Z","receivedAt":"2019-01-14T17:45:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Barret Rhoden writes:\n\n> On 2019-01-10 at 14:29 Junio C Hamano wrote:\n>> > For instance, commit X does this:\n>> >\n>> > -foo(x,y);\n>> > +foo(x,y,z);\n>> >\n>> > Then commit Y comes along to reformat it:\n>> >\n>> > -foo(x,y,z);\n>> > +foo(x, y, z);\n>> >\n>> > And the history / rev-list for the file looks like:\n>> >\n>> > ---O---A---X---B---C---D---Y---E---F\n>> >\n>> > I want to ignore/skip Y and see X in the blame output. \n>> \n>> If you skip Y, the altered history would have \"foo(x, y, z)\" in E,\n>> \"foo(x,y,z)\" in X, and \"foo(x,y)\" in A. If you start blaming from\n>> F, you'd get E as the commit that explains the latest state. If you\n>> do not skip Y, you'd get Y. I am not sure how you'd get X in either\n>> case. \n>\n> The way to do it is ...\n\nSorry, I made a too-fuzzy statement. What I meant was, that unless\nyou are ignoring E, I do not know why you \"would want to\" attribute\na line \"foo(x, y, z)\" that appears in F to X. Starting from X up to\nD (and to Y in real history, but you are ignoring Y), the line was\n\"foo(x,y,z)\", after E, it is \"foo(x, y, z)\". I didn't mean to ask\nhow you \"would show\" such a result---as I do not yet understand why\nyou would want such a result to begin with.\n\n"},{"id":"366663","messageId":"003c01d4ac36$bdf29470$39d7bd50$@nexbridge.com","threadId":"50175","inReplyTo":"xmqqfttvw2sv.fsf@gitster-ct.c.googlers.com","subject":"RE: [PATCH] blame: add the ability to ignore commits","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-01-14T18:26:56Z","receivedAt":"2019-01-14T18:27:09Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 14, 2019 12:46, Junio C Hamano wrote:\n> Barret Rhoden writes:\n> \n> > On 2019-01-10 at 14:29 Junio C Hamano wrote:\n> >> > For instance, commit X does this:\n> >> >\n> >> > -foo(x,y);\n> >> > +foo(x,y,z);\n> >> >\n> >> > Then commit Y comes along to reformat it:\n> >> >\n> >> > -foo(x,y,z);\n> >> > +foo(x, y, z);\n> >> >\n> >> > And the history / rev-list for the file looks like:\n> >> >\n> >> > ---O---A---X---B---C---D---Y---E---F\n> >> >\n> >> > I want to ignore/skip Y and see X in the blame output.\n> >>\n> >> If you skip Y, the altered history would have \"foo(x, y, z)\" in E,\n> >> \"foo(x,y,z)\" in X, and \"foo(x,y)\" in A. If you start blaming from F,\n> >> you'd get E as the commit that explains the latest state. If you do\n> >> not skip Y, you'd get Y. I am not sure how you'd get X in either\n> >> case.\n> >\n> > The way to do it is ...\n> \n> Sorry, I made a too-fuzzy statement. What I meant was, that unless you\nare\n> ignoring E, I do not know why you \"would want to\" attribute a line \"foo(x,\ny,\n> z)\" that appears in F to X. Starting from X up to D (and to Y in real\nhistory, but\n> you are ignoring Y), the line was \"foo(x,y,z)\", after E, it is \"foo(x, y,\nz)\". I\n> didn't mean to ask how you \"would show\" such a result---as I do not yet\n> understand why you would want such a result to begin with.\n\nFrom my own community, this came up also. The intent was to show everyone\nwho touched a particular line, throughout history, not just the current one.\nPerhaps that is what Barret is going for.\n\nRegards,\nRandall\n\n"},{"id":"366673","messageId":"20190114140330.0fe3e382@gnomeregan.cam.corp.google.com","threadId":"50175","inReplyTo":"003c01d4ac36$bdf29470$39d7bd50$@nexbridge.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-14T19:03:30Z","receivedAt":"2019-01-14T19:03:36Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-14 at 13:26 \"Randall S. Becker\" \nwrote:\n> > \n> > Sorry, I made a too-fuzzy statement. What I meant was, that unless you \n> are\n> > ignoring E, I do not know why you \"would want to\" attribute a line \"foo(x, \n> y,\n> > z)\" that appears in F to X. Starting from X up to D (and to Y in real \n> history, but\n> > you are ignoring Y), the line was \"foo(x,y,z)\", after E, it is \"foo(x, y, \n> z)\". I\n> > didn't mean to ask how you \"would show\" such a result---as I do not yet\n> > understand why you would want such a result to begin with. \n> \n> From my own community, this came up also. The intent was to show everyone\n> who touched a particular line, throughout history, not just the current one.\n> Perhaps that is what Barret is going for.\n\nYeah. I want to find the most recent commit that changed a line, minus\na set of commits that are deemed 'not interesting' by the user. \n\nThe primary reason for this is to not see a blame attributed to a\ncommit that does nothing but reformatting the code base.\n\nOne could also use this feature for one-off investigation with the\ncommand-line switch. Imagine a manually driven git-blame + git-log,\nwhere you do a git-blame, check the commit, decide you want to see the\nnext one back, and rerun git-blame with --skip-rev=SHA1.\n\nThe Chromium depot_tools has a python tool that can do this, called\nhyper-blame: \n(https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html).\n\nI thought it was a useful feature for all of git's users. And I also\ndidn't want people to have to install depot_tools.\n\nBarret\n\n"},{"id":"366702","messageId":"xmqqd0oytpv2.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"003c01d4ac36$bdf29470$39d7bd50$@nexbridge.com","subject":"Re: [PATCH] blame: add the ability to ignore commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T06:08:17Z","receivedAt":"2019-01-15T06:08:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" writes:\n\n> On January 14, 2019 12:46, Junio C Hamano wrote:\n>> Barret Rhoden writes:\n>> \n>> > On 2019-01-10 at 14:29 Junio C Hamano wrote:\n>> >> > For instance, commit X does this:\n>> >> >\n>> >> > -foo(x,y);\n>> >> > +foo(x,y,z);\n>> >> >\n>> >> > Then commit Y comes along to reformat it:\n>> >> >\n>> >> > -foo(x,y,z);\n>> >> > +foo(x, y, z);\n>> >> >\n>> >> > And the history / rev-list for the file looks like:\n>> >> >\n>> >> > ---O---A---X---B---C---D---Y---E---F\n>> >> >\n>> >> > I want to ignore/skip Y and see X in the blame output.\n>> >>\n>> >> If you skip Y, the altered history would have \"foo(x, y, z)\" in E,\n>> >> \"foo(x,y,z)\" in X, and \"foo(x,y)\" in A. If you start blaming from F,\n>> >> you'd get E as the commit that explains the latest state. If you do\n>> >> not skip Y, you'd get Y. I am not sure how you'd get X in either\n>> >> case.\n>> >\n>> ...\n>\n> From my own community, this came up also. The intent was to show everyone\n> who touched a particular line, throughout history, not just the current one.\n> Perhaps that is what Barret is going for.\n\nI think I now understand what is going on. In short, what Barret\nwanted is way more ambitious than \"blame in a hypothetical history\nthat lacks certain commits\". There is no similiarity with existing\n\"-S \" option that I asked about in my initial review. This is\nquite a different beast (and I kind-a like it ;-).\n\nInstead of \"skipping\" Y, Barret wants to pretend that the effect of\ncommit Y, in addition to the commit Y itself, never existed in the\nhistory. Even when blaming back from F (where it has \"foo(x, y,\nz)\", which is the same as E and down to Y), the algorithm wants to\npretend as if E and F had that line in \"foo(x,y,z)\" form, i.e.\nbefore Y touched it, and also pretend that the user started blaming\na line in that shape.\n\nAs there is no general solution [*1*] to \"how did this line looked\nlike before this commit\", such a blame may not have a meaningful\nanswer. When Y added a block of text without removing anything\n(i.e. \"git show Y\" has no line that begins with '-'), and when the\nuser asked to blame that block of text in F, there is no sensible\nanswer to the question: \"what got replaced by Y in this text?\", so a\nblame that \"ignores\" Y would not be able to answer \"where did these\nlines come from?\" question. But in some cases (e.g. when \"git show\nY\" has only one '-' line, immediately followed by '+' line, both\nwith quite a similar contents and no other change in the patch---it\nis highly plausible that in such a change, the former became the\nlatter, like the example in Barret's message I was responding to),\nwe can say \"this was how the line that was modified in Y looked like\nbefore Y\" and blaming while ignoring Y can have a reasonable answer.\n\n\n[Footnote]\n\n*1* Here, I use the word \"solution\" in the same sense as \"solution\nto a math equation\", not in the sense to describe the steps to\nderive that \"solution\".\n"},{"id":"367023","messageId":"20190117202919.157326-1-brho@google.com","threadId":"50175","inReplyTo":"20190107213013.231514-1-brho@google.com","subject":"[PATCH v2 0/3] blame: add the ability to ignore commits","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-17T20:29:16Z","receivedAt":"2019-01-17T20:29:35Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"Adds the ability to ignore a set of commits and their changes when\nblaming. This can be used to ignore a commit deemed 'not interesting,'\nsuch as reformatting.\n\nv1: https://public-inbox.org/git/20190107213013.231514-1-brho@google.com/\nv1 -> v2\n- extracted the skiplist from fsck to avoid duplicating code\n- overhauled the interface and options\n- split out markIgnoredFiles\n- handled merges\n\nBarret Rhoden (3):\n Move init_skiplist() outside of fsck\n blame: add the ability to ignore commits and their changes\n blame: add a config option to mark ignored lines\n\n Documentation/blame-options.txt | 15 +++++++++\n Documentation/git-blame.txt | 1 +\n blame.c | 56 +++++++++++++++++++++++++++------\n blame.h | 3 ++\n builtin/blame.c | 33 +++++++++++++++++++\n fsck.c | 37 +---------------------\n oidset.c | 35 +++++++++++++++++++++\n oidset.h | 7 +++++\n 8 files changed, 142 insertions(+), 45 deletions(-)\n\n-- \n2.20.1.321.g9e740568ce-goog\n\n"},{"id":"367024","messageId":"20190117202919.157326-3-brho@google.com","threadId":"50175","inReplyTo":"20190117202919.157326-1-brho@google.com","subject":"[PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-17T20:29:18Z","receivedAt":"2019-01-17T20:29:38Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"Commits that make formatting changes or function renames are often not\ninteresting when blaming a file. A user may deem such a commit as 'not\ninteresting' and want to ignore and its changes it when assigning blame.\n\nFor example, say a file has the following git history / rev-list:\n\n---O---A---X---B---C---D---Y---E---F\n\nCommits X and Y both touch a particular line, and the other commits do\nnot:\n\nX: \"Take a third parameter\"\n-MyFunc(1, 2);\n+MyFunc(1, 2, 3);\n\nY: \"Remove camelcase\"\n-MyFunc(1, 2, 3);\n+my_func(1, 2, 3);\n\ngit-blame will blame Y for the change. I'd like to be able to ignore Y:\nboth the existence of the commit as well as any changes it made. This\ndiffers from -S rev-list, which specifies the list of commits to\nprocess for the blame. We still process Y, we just don't let the blame\n'stick.'\n\nUsers can ignore a revision with --ignore-rev=rev, which may be\nrepeated. They can specify a file of full SHA-1 hashes of revs (one per\nline) with the blame.ignoreRevFile config option. They can also specify\na file with --ignore-rev-file=file, which overrides the config option.\n\nFor a typical use case, projects will maintain the file containing\nrevisions for commits that perform mass reformatting, and their users\nhave the optional to ignore all of the commits in that file.\n\nAdditionally, a user can use the --ignore-rev option for one-off\ninvestigation. To go back to the example above, X was a substantive\nchange to the function, but not the change the user is interested in.\nThe user inspected X, but wanted to find the previous change to that\nline - perhaps a commit that introduced that function call.\n\nTo make this work, we can't simply remove all ignored commits from the\nrev-list. We need to diff the changes introduced by Y so that we can\nignore them. We let the blames get passed to Y, just like when\nprocessing normally. When Y is the target, we make sure that Y does not\n*keep* any blames. Any changes that Y is responsible for get passed to\nits parent. Note we make one pass through all of the scapegoats\n(parents) to attempt to pass blame normally; we don't know if we *need*\nto ignore the commit until we've checked all of the parents.\n\nThe blame_entry will get passed up the tree until we find a commit that\nhas a diff chunk that affects those lines. If an ignored commit added\nmore lines than it removed, the blame will fall on a commit that made a\nchange nearby. There is no general solution here, just a best-effort\napproach. For a trivial example, consider ignoring this commit:\n\nZ: \"Adding Lines\"\n foo\n+No commit\n+ever touched\n+these lines\n bar\n\nSigned-off-by: Barret Rhoden \n---\n Documentation/blame-options.txt | 13 +++++++++\n Documentation/git-blame.txt | 1 +\n blame.c | 48 +++++++++++++++++++++++++++------\n blame.h | 2 ++\n builtin/blame.c | 24 +++++++++++++++++\n 5 files changed, 80 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex dc41957afab2..424a63f0b45c 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -110,5 +110,18 @@ commit. And the default value is 40. If there are more than one\n `-C` options given, the argument of the last `-C` will\n take effect.\n \n+--ignore-rev ::\n+\tIgnore changes made by the revision when assigning blame, as if the\n+\tchange never happened. Lines that were changed or added by an ignored\n+\tcommit will be blamed on the previous commit that changed that line or\n+\tnearby lines. This option may be specified multiple times to ignore\n+\tmore than one revision.\n+\n+--ignore-revs-file ::\n+\tIgnore revisions listed in `file`, one full SHA-1 hash per line.\n+\tWhitespace and comments beginning with `#` are ignored. This overrides\n+\tthe `blame.ignoreRevsFile` config option, which specifies a default\n+\tfile.\n+\n -h::\n \tShow help message.\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 16323eb80e31..7e8154199635 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n [verse]\n 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]\n \t [-L ] [-S ] [-M] [-C] [-C] [-C] [--since=]\n+\t [--ignore-rev ] [--ignore-revs-file ]\n \t [--progress] [--abbrev=] [ | --contents | --reverse ..]\n \t [--] \n \ndiff --git a/blame.c b/blame.c\nindex d84c93778080..0b91fba2d04c 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -845,10 +845,10 @@ static struct blame_entry *reverse_blame(struct blame_entry *head,\n */\n static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tint tlno, int offset, int same,\n-\t\t\tstruct blame_origin *parent)\n+\t\t\tstruct blame_origin *parent, int ignore_diffs)\n {\n \tstruct blame_entry *e = **srcq;\n-\tstruct blame_entry *samep = NULL, *diffp = NULL;\n+\tstruct blame_entry *samep = NULL, *diffp = NULL, *ignoredp = NULL;\n \n \twhile (e && e->s_lno < tlno) {\n \t\tstruct blame_entry *next = e->next;\n@@ -925,10 +925,23 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tn->next = samep;\n \t\t\tsamep = n;\n \t\t}\n-\t\te->next = diffp;\n-\t\tdiffp = e;\n+\t\tif (ignore_diffs) {\n+\t\t\t/* These go to the parent, like the ones before tlno. */\n+\t\t\tblame_origin_decref(e->suspect);\n+\t\t\te->suspect = blame_origin_incref(parent);\n+\t\t\te->s_lno += offset;\n+\t\t\te->next = ignoredp;\n+\t\t\tignoredp = e;\n+\t\t} else {\n+\t\t\te->next = diffp;\n+\t\t\tdiffp = e;\n+\t\t}\n \t\te = next;\n \t}\n+\tif (ignoredp) {\n+\t\t**dstq = reverse_blame(ignoredp, **dstq);\n+\t\t*dstq = &ignoredp->next;\n+\t}\n \t**srcq = reverse_blame(diffp, reverse_blame(samep, e));\n \t/* Move across elements that are in the unblamable portion */\n \tif (diffp)\n@@ -938,6 +951,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n struct blame_chunk_cb_data {\n \tstruct blame_origin *parent;\n \tlong offset;\n+\tint ignore_diffs;\n \tstruct blame_entry **dstq;\n \tstruct blame_entry **srcq;\n };\n@@ -950,7 +964,7 @@ static int blame_chunk_cb(long start_a, long count_a,\n \tif (start_a - start_b != d->offset)\n \t\tdie(\"internal error in blame::blame_chunk_cb\");\n \tblame_chunk(&d->dstq, &d->srcq, start_b, start_a - start_b,\n-\t\t start_b + count_b, d->parent);\n+\t\t start_b + count_b, d->parent, d->ignore_diffs);\n \td->offset = start_a + count_a - (start_b + count_b);\n \treturn 0;\n }\n@@ -962,7 +976,7 @@ static int blame_chunk_cb(long start_a, long count_a,\n */\n static void pass_blame_to_parent(struct blame_scoreboard *sb,\n \t\t\t\t struct blame_origin *target,\n-\t\t\t\t struct blame_origin *parent)\n+\t\t\t\t struct blame_origin *parent, int ignore_diffs)\n {\n \tmmfile_t file_p, file_o;\n \tstruct blame_chunk_cb_data d;\n@@ -973,6 +987,7 @@ static void pass_blame_to_parent(struct blame_scoreboard *sb,\n \n \td.parent = parent;\n \td.offset = 0;\n+\td.ignore_diffs = ignore_diffs;\n \td.dstq = &newdest; d.srcq = &target->suspects;\n \n \tfill_origin_blob(&sb->revs->diffopt, parent, &file_p, &sb->num_read_blob);\n@@ -984,7 +999,7 @@ static void pass_blame_to_parent(struct blame_scoreboard *sb,\n \t\t oid_to_hex(&parent->commit->object.oid),\n \t\t oid_to_hex(&target->commit->object.oid));\n \t/* The rest are the same as the parent */\n-\tblame_chunk(&d.dstq, &d.srcq, INT_MAX, d.offset, INT_MAX, parent);\n+\tblame_chunk(&d.dstq, &d.srcq, INT_MAX, d.offset, INT_MAX, parent, 0);\n \t*d.dstq = NULL;\n \tqueue_blames(sb, parent, newdest);\n \n@@ -1489,11 +1504,28 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin,\n \t\t\tblame_origin_incref(porigin);\n \t\t\torigin->previous = porigin;\n \t\t}\n-\t\tpass_blame_to_parent(sb, origin, porigin);\n+\t\tpass_blame_to_parent(sb, origin, porigin, 0);\n \t\tif (!origin->suspects)\n \t\t\tgoto finish;\n \t}\n \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+\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+\t\t\tstruct blame_origin *porigin = sg_origin[i];\n+\n+\t\t\tif (!porigin)\n+\t\t\t\tcontinue;\n+\t\t\tpass_blame_to_parent(sb, origin, porigin, 1);\n+\t\t\tif (!origin->suspects)\n+\t\t\t\tgoto finish;\n+\t\t}\n+\t}\n+\n \t/*\n \t * Optionally find moves in parents' files.\n \t */\ndiff --git a/blame.h b/blame.h\nindex be3a895043e0..086b92915e4b 100644\n--- a/blame.h\n+++ b/blame.h\n@@ -117,6 +117,8 @@ 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 6d798f99392e..2f9183fb5fbd 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -52,6 +52,7 @@ static int no_whole_file_rename;\n static int show_progress;\n static char repeated_meta_color[COLOR_MAXLEN];\n static int coloring_mode;\n+static const char *ignore_revs_file;\n \n static struct date_mode blame_date_mode = { DATE_ISO8601 };\n static size_t blame_date_width;\n@@ -695,6 +696,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\tparse_date_format(value, &blame_date_mode);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"blame.ignorerevsfile\"))\n+\t\treturn git_config_pathname(&ignore_revs_file, var, value);\n \tif (!strcmp(var, \"color.blame.repeatedlines\")) {\n \t\tif (color_parse_mem(value, strlen(value), repeated_meta_color))\n \t\t\twarning(_(\"invalid color '%s' in color.blame.repeatedLines\"),\n@@ -774,6 +777,22 @@ static int is_a_rev(const char *name)\n \treturn OBJ_NONE < oid_object_info(the_repository, &oid, NULL);\n }\n \n+static void build_ignorelist(struct blame_scoreboard *sb,\n+\t\t\t struct string_list *ignore_rev_list)\n+{\n+\tstruct string_list_item *i;\n+\tstruct object_id oid;\n+\n+\toidset_init(&sb->ignore_list, 0);\n+\tif (ignore_revs_file)\n+\t\toidset_parse_file(&sb->ignore_list, ignore_revs_file);\n+\tfor_each_string_list_item(i, ignore_rev_list) {\n+\t\tif (get_oid_committish(i->string, &oid))\n+\t\t\tdie(_(\"Can't find revision '%s' to ignore\"), i->string);\n+\t\toidset_insert(&sb->ignore_list, &oid);\n+\t}\n+}\n+\n int cmd_blame(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n@@ -785,6 +804,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstruct progress_info pi = { NULL, 0 };\n \n \tstruct string_list range_list = STRING_LIST_INIT_NODUP;\n+\tstruct string_list ignore_rev_list = STRING_LIST_INIT_NODUP;\n \tint output_option = 0, opt = 0;\n \tint show_stats = 0;\n \tconst char *revs_file = NULL;\n@@ -806,6 +826,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('s', NULL, &output_option, N_(\"Suppress author name and timestamp (Default: off)\"), OUTPUT_NO_AUTHOR),\n \t\tOPT_BIT('e', \"show-email\", &output_option, N_(\"Show author email instead of name (Default: off)\"), OUTPUT_SHOW_EMAIL),\n \t\tOPT_BIT('w', NULL, &xdl_opts, N_(\"Ignore whitespace differences\"), XDF_IGNORE_WHITESPACE),\n+\t\tOPT_STRING_LIST(0, \"ignore-rev\", &ignore_rev_list, N_(\"rev\"), N_(\"Ignore when blaming\")),\n+\t\tOPT_STRING(0, \"ignore-revs-file\", &ignore_revs_file, N_(\"file\"), N_(\"Ignore revisions from \")),\n \t\tOPT_BIT(0, \"color-lines\", &output_option, N_(\"color redundant metadata from previous line differently\"), OUTPUT_COLOR_LINE),\n \t\tOPT_BIT(0, \"color-by-age\", &output_option, N_(\"color lines by age\"), OUTPUT_SHOW_AGE_WITH_COLOR),\n \n@@ -995,6 +1017,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tsb.contents_from = contents_from;\n \tsb.reverse = reverse;\n \tsb.repo = the_repository;\n+\tbuild_ignorelist(&sb, &ignore_rev_list);\n+\tstring_list_clear(&ignore_rev_list, 0);\n \tsetup_scoreboard(&sb, path, &o);\n \tlno = sb.num_lines;\n \n-- \n2.20.1.321.g9e740568ce-goog\n\n"},{"id":"367025","messageId":"20190117202919.157326-4-brho@google.com","threadId":"50175","inReplyTo":"20190117202919.157326-1-brho@google.com","subject":"[PATCH v2 3/3] blame: add a config option to mark ignored lines","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-17T20:29:19Z","receivedAt":"2019-01-17T20:29:39Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"When ignoring commits, the commit that is blamed might not be\nresponsible for the change. Users might want to know when a particular\nline has a potentially inaccurate blame.\n\nBy specifying blame.markIgnoredFiles, each blame line is marked with an\n'*'. For example:\n\n278b6158d6fdb (Barret Rhoden 2016-04-11 13:57:54 -0400 26)\n\nappears as:\n\n*278b6158d6fd (Barret Rhoden 2016-04-11 13:57:54 -0400 26)\n\nwhere the '*' is placed before the commit, and the hash has one fewer\ncharacters.\n\nSigned-off-by: Barret Rhoden \n---\n Documentation/blame-options.txt | 4 +++-\n blame.c | 8 +++++++-\n blame.h | 1 +\n builtin/blame.c | 9 +++++++++\n 4 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 424a63f0b45c..92787ae951ac 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -115,7 +115,9 @@ take effect.\n \tchange never happened. Lines that were changed or added by an ignored\n \tcommit will be blamed on the previous commit that changed that line or\n \tnearby lines. This option may be specified multiple times to ignore\n-\tmore than one revision.\n+\tmore than one revision. If the `blame.markIgnoredLines` config option\n+\tis set, then lines that were changed by an ignored commit will be\n+\tmarked with a `*` in the blame output.\n \n --ignore-revs-file ::\n \tIgnore revisions listed in `file`, one full SHA-1 hash per line.\ndiff --git a/blame.c b/blame.c\nindex 0b91fba2d04c..b1805633fb23 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -474,7 +474,8 @@ void blame_coalesce(struct blame_scoreboard *sb)\n \n \tfor (ent = sb->ent; ent && (next = ent->next); ent = next) {\n \t\tif (ent->suspect == next->suspect &&\n-\t\t ent->s_lno + ent->num_lines == next->s_lno) {\n+\t\t ent->s_lno + ent->num_lines == next->s_lno &&\n+\t\t ent->ignored == next->ignored) {\n \t\t\tent->num_lines += next->num_lines;\n \t\t\tent->next = next->next;\n \t\t\tblame_origin_decref(next->suspect);\n@@ -726,6 +727,8 @@ static void split_overlap(struct blame_entry *split,\n \tint chunk_end_lno;\n \tmemset(split, 0, sizeof(struct blame_entry [3]));\n \n+\tsplit[0].ignored = split[1].ignored = split[2].ignored = e->ignored;\n+\n \tif (e->s_lno < tlno) {\n \t\t/* there is a pre-chunk part not blamed on parent */\n \t\tsplit[0].suspect = blame_origin_incref(e->suspect);\n@@ -862,6 +865,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tint len = tlno - e->s_lno;\n \t\t\tstruct blame_entry *n = xcalloc(1, sizeof (struct blame_entry));\n \t\t\tn->suspect = e->suspect;\n+\t\t\tn->ignored = e->ignored;\n \t\t\tn->lno = e->lno + len;\n \t\t\tn->s_lno = e->s_lno + len;\n \t\t\tn->num_lines = e->num_lines - len;\n@@ -916,6 +920,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tint len = same - e->s_lno;\n \t\t\tstruct blame_entry *n = xcalloc(1, sizeof (struct blame_entry));\n \t\t\tn->suspect = blame_origin_incref(e->suspect);\n+\t\t\tn->ignored = e->ignored;\n \t\t\tn->lno = e->lno + len;\n \t\t\tn->s_lno = e->s_lno + len;\n \t\t\tn->num_lines = e->num_lines - len;\n@@ -930,6 +935,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n \t\t\tblame_origin_decref(e->suspect);\n \t\t\te->suspect = blame_origin_incref(parent);\n \t\t\te->s_lno += offset;\n+\t\t\te->ignored = 1;\n \t\t\te->next = ignoredp;\n \t\t\tignoredp = e;\n \t\t} else {\ndiff --git a/blame.h b/blame.h\nindex 086b92915e4b..56aeff582b01 100644\n--- a/blame.h\n+++ b/blame.h\n@@ -92,6 +92,7 @@ struct blame_entry {\n \t * scanning the lines over and over.\n \t */\n \tunsigned score;\n+\tint ignored;\n };\n \n /*\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 2f9183fb5fbd..8c3c5e435c9c 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -53,6 +53,7 @@ static int show_progress;\n static char repeated_meta_color[COLOR_MAXLEN];\n static int coloring_mode;\n static const char *ignore_revs_file;\n+static int mark_ignored_lines;\n \n static struct date_mode blame_date_mode = { DATE_ISO8601 };\n static size_t blame_date_width;\n@@ -480,6 +481,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\t\t}\n \t\t}\n \n+\t\tif (mark_ignored_lines && ent->ignored) {\n+\t\t\tlength--;\n+\t\t\tputchar('*');\n+\t\t}\n \t\tprintf(\"%.*s\", length, hex);\n \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n \t\t\tconst char *name;\n@@ -698,6 +703,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(var, \"blame.ignorerevsfile\"))\n \t\treturn git_config_pathname(&ignore_revs_file, var, value);\n+\tif (!strcmp(var, \"blame.markignoredlines\")) {\n+\t\tmark_ignored_lines = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"color.blame.repeatedlines\")) {\n \t\tif (color_parse_mem(value, strlen(value), repeated_meta_color))\n \t\t\twarning(_(\"invalid color '%s' in color.blame.repeatedLines\"),\n-- \n2.20.1.321.g9e740568ce-goog\n\n"},{"id":"367026","messageId":"20190117202919.157326-2-brho@google.com","threadId":"50175","inReplyTo":"20190117202919.157326-1-brho@google.com","subject":"[PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-17T20:29:17Z","receivedAt":"2019-01-17T20:29:40Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"init_skiplist() took a file consisting of SHA-1s and comments and added\nthe objects to an oidset. This functionality is useful for other\ncommands.\n\nSigned-off-by: Barret Rhoden \n---\n fsck.c | 37 +------------------------------------\n oidset.c | 35 +++++++++++++++++++++++++++++++++++\n oidset.h | 7 +++++++\n 3 files changed, 43 insertions(+), 36 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 68502ce85b11..80b53e6f4968 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -181,41 +181,6 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n \treturn msg_type;\n }\n \n-static void init_skiplist(struct fsck_options *options, const char *path)\n-{\n-\tFILE *fp;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct object_id oid;\n-\n-\tfp = fopen(path, \"r\");\n-\tif (!fp)\n-\t\tdie(\"Could not open skip list: %s\", path);\n-\twhile (!strbuf_getline(&sb, fp)) {\n-\t\tconst char *p;\n-\t\tconst char *hash;\n-\n-\t\t/*\n-\t\t * Allow trailing comments, leading whitespace\n-\t\t * (including before commits), and empty or whitespace\n-\t\t * only lines.\n-\t\t */\n-\t\thash = strchr(sb.buf, '#');\n-\t\tif (hash)\n-\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n-\t\tstrbuf_trim(&sb);\n-\t\tif (!sb.len)\n-\t\t\tcontinue;\n-\n-\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n-\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n-\t\toidset_insert(&options->skiplist, &oid);\n-\t}\n-\tif (ferror(fp))\n-\t\tdie_errno(\"Could not read '%s'\", path);\n-\tfclose(fp);\n-\tstrbuf_release(&sb);\n-}\n-\n static int parse_msg_type(const char *str)\n {\n \tif (!strcmp(str, \"error\"))\n@@ -284,7 +249,7 @@ void fsck_set_msg_types(struct fsck_options *options, const char *values)\n \t\tif (!strcmp(buf, \"skiplist\")) {\n \t\t\tif (equal == len)\n \t\t\t\tdie(\"skiplist requires a path\");\n-\t\t\tinit_skiplist(options, buf + equal + 1);\n+\t\t\toidset_parse_file(&options->skiplist, buf + equal + 1);\n \t\t\tbuf += len + 1;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/oidset.c b/oidset.c\nindex fe4eb921df81..a4f38a040320 100644\n--- a/oidset.c\n+++ b/oidset.c\n@@ -35,3 +35,38 @@ void oidset_clear(struct oidset *set)\n \tkh_release_oid(&set->set);\n \toidset_init(set, 0);\n }\n+\n+void oidset_parse_file(struct oidset *set, const char *path)\n+{\n+\tFILE *fp;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct object_id oid;\n+\n+\tfp = fopen(path, \"r\");\n+\tif (!fp)\n+\t\tdie(\"Could not open skip list: %s\", path);\n+\twhile (!strbuf_getline(&sb, fp)) {\n+\t\tconst char *p;\n+\t\tconst char *hash;\n+\n+\t\t/*\n+\t\t * Allow trailing comments, leading whitespace\n+\t\t * (including before commits), and empty or whitespace\n+\t\t * only lines.\n+\t\t */\n+\t\thash = strchr(sb.buf, '#');\n+\t\tif (hash)\n+\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n+\t\tstrbuf_trim(&sb);\n+\t\tif (!sb.len)\n+\t\t\tcontinue;\n+\n+\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n+\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n+\t\toidset_insert(set, &oid);\n+\t}\n+\tif (ferror(fp))\n+\t\tdie_errno(\"Could not read '%s'\", path);\n+\tfclose(fp);\n+\tstrbuf_release(&sb);\n+}\ndiff --git a/oidset.h b/oidset.h\nindex c9d0f6d3cc8b..a3452eb7de84 100644\n--- a/oidset.h\n+++ b/oidset.h\n@@ -73,6 +73,13 @@ int oidset_remove(struct oidset *set, const struct object_id *oid);\n */\n void oidset_clear(struct oidset *set);\n \n+/**\n+ * Add the contents of the file 'path' to an initialized oidset. Each line is\n+ * an unabbreviated SHA-1. Comments begin with '#', and trailing comments are\n+ * allowed. Leading whitespace and empty or white-space only lines are ignored.\n+ */\n+void oidset_parse_file(struct oidset *set, const char *path);\n+\n struct oidset_iter {\n \tkh_oid_t *set;\n \tkhiter_t iter;\n-- \n2.20.1.321.g9e740568ce-goog\n\n"},{"id":"367065","messageId":"nycvar.QRO.7.76.6.1901181024090.41@tvgsbejvaqbjf.bet","threadId":"50175","inReplyTo":"20190117202919.157326-2-brho@google.com","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T09:25:24Z","receivedAt":"2019-01-18T09:26:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Barret,\n\nOn Thu, 17 Jan 2019, Barret Rhoden wrote:\n\n> init_skiplist() took a file consisting of SHA-1s and comments and added\n> the objects to an oidset. This functionality is useful for other\n> commands.\n> \n> Signed-off-by: Barret Rhoden \n\nThis patch looks good, I have just one small suggestion: SHA-1's days are\ncounted. We already know the roadmap, that we want to use SHA-256 instead\nat some stage. Why not talk about \"object hashes\" instead of \"SHA-1s\"?\n\nThanks,\nJohannes\n\n> ---\n> fsck.c | 37 +------------------------------------\n> oidset.c | 35 +++++++++++++++++++++++++++++++++++\n> oidset.h | 7 +++++++\n> 3 files changed, 43 insertions(+), 36 deletions(-)\n> \n> diff --git a/fsck.c b/fsck.c\n> index 68502ce85b11..80b53e6f4968 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -181,41 +181,6 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,\n> \treturn msg_type;\n> }\n> \n> -static void init_skiplist(struct fsck_options *options, const char *path)\n> -{\n> -\tFILE *fp;\n> -\tstruct strbuf sb = STRBUF_INIT;\n> -\tstruct object_id oid;\n> -\n> -\tfp = fopen(path, \"r\");\n> -\tif (!fp)\n> -\t\tdie(\"Could not open skip list: %s\", path);\n> -\twhile (!strbuf_getline(&sb, fp)) {\n> -\t\tconst char *p;\n> -\t\tconst char *hash;\n> -\n> -\t\t/*\n> -\t\t * Allow trailing comments, leading whitespace\n> -\t\t * (including before commits), and empty or whitespace\n> -\t\t * only lines.\n> -\t\t */\n> -\t\thash = strchr(sb.buf, '#');\n> -\t\tif (hash)\n> -\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n> -\t\tstrbuf_trim(&sb);\n> -\t\tif (!sb.len)\n> -\t\t\tcontinue;\n> -\n> -\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n> -\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n> -\t\toidset_insert(&options->skiplist, &oid);\n> -\t}\n> -\tif (ferror(fp))\n> -\t\tdie_errno(\"Could not read '%s'\", path);\n> -\tfclose(fp);\n> -\tstrbuf_release(&sb);\n> -}\n> -\n> static int parse_msg_type(const char *str)\n> {\n> \tif (!strcmp(str, \"error\"))\n> @@ -284,7 +249,7 @@ void fsck_set_msg_types(struct fsck_options *options, const char *values)\n> \t\tif (!strcmp(buf, \"skiplist\")) {\n> \t\t\tif (equal == len)\n> \t\t\t\tdie(\"skiplist requires a path\");\n> -\t\t\tinit_skiplist(options, buf + equal + 1);\n> +\t\t\toidset_parse_file(&options->skiplist, buf + equal + 1);\n> \t\t\tbuf += len + 1;\n> \t\t\tcontinue;\n> \t\t}\n> diff --git a/oidset.c b/oidset.c\n> index fe4eb921df81..a4f38a040320 100644\n> --- a/oidset.c\n> +++ b/oidset.c\n> @@ -35,3 +35,38 @@ void oidset_clear(struct oidset *set)\n> \tkh_release_oid(&set->set);\n> \toidset_init(set, 0);\n> }\n> +\n> +void oidset_parse_file(struct oidset *set, const char *path)\n> +{\n> +\tFILE *fp;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct object_id oid;\n> +\n> +\tfp = fopen(path, \"r\");\n> +\tif (!fp)\n> +\t\tdie(\"Could not open skip list: %s\", path);\n> +\twhile (!strbuf_getline(&sb, fp)) {\n> +\t\tconst char *p;\n> +\t\tconst char *hash;\n> +\n> +\t\t/*\n> +\t\t * Allow trailing comments, leading whitespace\n> +\t\t * (including before commits), and empty or whitespace\n> +\t\t * only lines.\n> +\t\t */\n> +\t\thash = strchr(sb.buf, '#');\n> +\t\tif (hash)\n> +\t\t\tstrbuf_setlen(&sb, hash - sb.buf);\n> +\t\tstrbuf_trim(&sb);\n> +\t\tif (!sb.len)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (parse_oid_hex(sb.buf, &oid, &p) || *p != '\\0')\n> +\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n> +\t\toidset_insert(set, &oid);\n> +\t}\n> +\tif (ferror(fp))\n> +\t\tdie_errno(\"Could not read '%s'\", path);\n> +\tfclose(fp);\n> +\tstrbuf_release(&sb);\n> +}\n> diff --git a/oidset.h b/oidset.h\n> index c9d0f6d3cc8b..a3452eb7de84 100644\n> --- a/oidset.h\n> +++ b/oidset.h\n> @@ -73,6 +73,13 @@ int oidset_remove(struct oidset *set, const struct object_id *oid);\n> */\n> void oidset_clear(struct oidset *set);\n> \n> +/**\n> + * Add the contents of the file 'path' to an initialized oidset. Each line is\n> + * an unabbreviated SHA-1. Comments begin with '#', and trailing comments are\n> + * allowed. Leading whitespace and empty or white-space only lines are ignored.\n> + */\n> +void oidset_parse_file(struct oidset *set, const char *path);\n> +\n> struct oidset_iter {\n> \tkh_oid_t *set;\n> \tkhiter_t iter;\n> -- \n> 2.20.1.321.g9e740568ce-goog\n> \n> \n"},{"id":"367071","messageId":"87k1j247ui.fsf@evledraar.gmail.com","threadId":"50175","inReplyTo":"20190117202919.157326-2-brho@google.com","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-18T09:45:41Z","receivedAt":"2019-01-18T09:45:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 17 2019, Barret Rhoden wrote:\n\n> -\t\tdie(\"Could not open skip list: %s\", path);\n> [...]\n> +\t\tdie(\"Could not open skip list: %s\", path);\n\nYou're just moving this around, but now that this has two uses let's say\n\"Could not open SHA-1 list; %s\" or something like that.\n\n> +\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n\nUnlike Johannes I think it's fine to leave this. This file-format is\nSHA-1 only now. We can cross the bridge of making it (and others)\nSHA-256 somehow when we come to that, whether that'll be allowing\nvariable width or a different file.\n"},{"id":"367072","messageId":"nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet","threadId":"50175","inReplyTo":"20190117202919.157326-3-brho@google.com","subject":"Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T09:47:36Z","receivedAt":"2019-01-18T09:48:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Barret,\n\nOn Thu, 17 Jan 2019, Barret Rhoden wrote:\n\n> [...]\n> \n> Users can ignore a revision with --ignore-rev=rev, which may be\n> repeated. They can specify a file of full SHA-1 hashes of revs (one per\n> line) with the blame.ignoreRevFile config option. They can also specify\n> a file with --ignore-rev-file=file, which overrides the config option.\n\nThis sounds like that is already the case in Git, but the truth is: this\npatch *introduces* that feature. Maybe start the paragraph with \"With this\npatch, ...\"?\n\nI cannot speak for the correctness of the changes to blame.c, others on\nthe Cc: list are already much more familiar with that code, so I'll leave\nit to them to comment on that.\n\nHowever, I am missing a regression test for this behavior, the best idea\nwould be likely to add t/t8013-blame-ignore-revs.sh (copy-edit it from\nt/t8009-blame-vs-topicbranches.sh, maybe).\n\nAnd there is another thing:\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 6d798f99392e..2f9183fb5fbd 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -52,6 +52,7 @@ static int no_whole_file_rename;\n> static int show_progress;\n> static char repeated_meta_color[COLOR_MAXLEN];\n> static int coloring_mode;\n> +static const char *ignore_revs_file;\n\n... this...\n\n> \n> static struct date_mode blame_date_mode = { DATE_ISO8601 };\n> static size_t blame_date_width;\n> @@ -695,6 +696,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n> \t\tparse_date_format(value, &blame_date_mode);\n> \t\treturn 0;\n> \t}\n> +\tif (!strcmp(var, \"blame.ignorerevsfile\"))\n> +\t\treturn git_config_pathname(&ignore_revs_file, var, value);\n\n... this...\n\n> \tif (!strcmp(var, \"color.blame.repeatedlines\")) {\n> \t\tif (color_parse_mem(value, strlen(value), repeated_meta_color))\n> \t\t\twarning(_(\"invalid color '%s' in color.blame.repeatedLines\"),\n> @@ -806,6 +826,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BIT('s', NULL, &output_option, N_(\"Suppress author name and timestamp (Default: off)\"), OUTPUT_NO_AUTHOR),\n> \t\tOPT_BIT('e', \"show-email\", &output_option, N_(\"Show author email instead of name (Default: off)\"), OUTPUT_SHOW_EMAIL),\n> \t\tOPT_BIT('w', NULL, &xdl_opts, N_(\"Ignore whitespace differences\"), XDF_IGNORE_WHITESPACE),\n> +\t\tOPT_STRING_LIST(0, \"ignore-rev\", &ignore_rev_list, N_(\"rev\"), N_(\"Ignore when blaming\")),\n> +\t\tOPT_STRING(0, \"ignore-revs-file\", &ignore_revs_file, N_(\"file\"), N_(\"Ignore revisions from \")),\n\n... and this change limit the user to specifying a single file, for no\ngood reason. Worse: specifying two different files via two\n`--ignore-revs-file` parameters will only heed the latter and skip the\nformer without any warning.\n\nA better idea IMHO would be to use an OPT_STRING_LIST() for\n`--ignore-revs-file`, too, and to allow for multiple\n`blame.ignoreRevsFile` config entries (with our usual trick of handling an\nempty setting by resetting the list of paths that were accumulated so\nfar, see e.g. how `credential.helper` is handled).\n\nCiao,\nJohannes\n\n> \t\tOPT_BIT(0, \"color-lines\", &output_option, N_(\"color redundant metadata from previous line differently\"), OUTPUT_COLOR_LINE),\n> \t\tOPT_BIT(0, \"color-by-age\", &output_option, N_(\"color lines by age\"), OUTPUT_SHOW_AGE_WITH_COLOR),\n> \n> @@ -995,6 +1017,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> \tsb.contents_from = contents_from;\n> \tsb.reverse = reverse;\n> \tsb.repo = the_repository;\n> +\tbuild_ignorelist(&sb, &ignore_rev_list);\n> +\tstring_list_clear(&ignore_rev_list, 0);\n> \tsetup_scoreboard(&sb, path, &o);\n> \tlno = sb.num_lines;\n> \n> -- \n> 2.20.1.321.g9e740568ce-goog\n> \n> \n"},{"id":"367075","messageId":"87imym47iu.fsf@evledraar.gmail.com","threadId":"50175","inReplyTo":"20190117202919.157326-1-brho@google.com","subject":"Re: [PATCH v2 0/3] blame: add the ability to ignore commits","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-18T09:52:41Z","receivedAt":"2019-01-18T09:52:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 17 2019, Barret Rhoden wrote:\n\n> Adds the ability to ignore a set of commits and their changes when\n> blaming. This can be used to ignore a commit deemed 'not interesting,'\n> such as reformatting.\n>\n> v1: https://public-inbox.org/git/20190107213013.231514-1-brho@google.com/\n> v1 -> v2\n> - extracted the skiplist from fsck to avoid duplicating code\n> - overhauled the interface and options\n> - split out markIgnoredFiles\n> - handled merges\n>\n> Barret Rhoden (3):\n> Move init_skiplist() outside of fsck\n> blame: add the ability to ignore commits and their changes\n> blame: add a config option to mark ignored lines\n\nLooks much better this time around, I skimmed it, but quickly discovered\nthat there's no tests for any of it, which is a prerequisite for getting\nsomething like this in.\n\n"},{"id":"367077","messageId":"nycvar.QRO.7.76.6.1901181100390.41@tvgsbejvaqbjf.bet","threadId":"50175","inReplyTo":"20190117202919.157326-4-brho@google.com","subject":"Re: [PATCH v2 3/3] blame: add a config option to mark ignored lines","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T10:03:02Z","receivedAt":"2019-01-18T10:03:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Barret,\n\nOn Thu, 17 Jan 2019, Barret Rhoden wrote:\n\n> When ignoring commits, the commit that is blamed might not be\n> responsible for the change. Users might want to know when a particular\n> line has a potentially inaccurate blame.\n> \n> By specifying blame.markIgnoredFiles, each blame line is marked with an\n> '*'. For example:\n> \n> 278b6158d6fdb (Barret Rhoden 2016-04-11 13:57:54 -0400 26)\n> \n> appears as:\n> \n> *278b6158d6fd (Barret Rhoden 2016-04-11 13:57:54 -0400 26)\n> \n> where the '*' is placed before the commit, and the hash has one fewer\n> characters.\n> \n> Signed-off-by: Barret Rhoden \n\nAgain, I cannot comment on blame.c, there are more competent people Cc:ed.\nBut I do have to point out that Git prefers commit messages in the\nimperative form rather than the present tense (reading the commit message\ncould leave the inclined reader wondering what the patch changes, if Git\nalready does all that).\n\nCiao,\nJohannes\n\n> ---\n> Documentation/blame-options.txt | 4 +++-\n> blame.c | 8 +++++++-\n> blame.h | 1 +\n> builtin/blame.c | 9 +++++++++\n> 4 files changed, 20 insertions(+), 2 deletions(-)\n> \n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 424a63f0b45c..92787ae951ac 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -115,7 +115,9 @@ take effect.\n> \tchange never happened. Lines that were changed or added by an ignored\n> \tcommit will be blamed on the previous commit that changed that line or\n> \tnearby lines. This option may be specified multiple times to ignore\n> -\tmore than one revision.\n> +\tmore than one revision. If the `blame.markIgnoredLines` config option\n> +\tis set, then lines that were changed by an ignored commit will be\n> +\tmarked with a `*` in the blame output.\n> \n> --ignore-revs-file ::\n> \tIgnore revisions listed in `file`, one full SHA-1 hash per line.\n> diff --git a/blame.c b/blame.c\n> index 0b91fba2d04c..b1805633fb23 100644\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -474,7 +474,8 @@ void blame_coalesce(struct blame_scoreboard *sb)\n> \n> \tfor (ent = sb->ent; ent && (next = ent->next); ent = next) {\n> \t\tif (ent->suspect == next->suspect &&\n> -\t\t ent->s_lno + ent->num_lines == next->s_lno) {\n> +\t\t ent->s_lno + ent->num_lines == next->s_lno &&\n> +\t\t ent->ignored == next->ignored) {\n> \t\t\tent->num_lines += next->num_lines;\n> \t\t\tent->next = next->next;\n> \t\t\tblame_origin_decref(next->suspect);\n> @@ -726,6 +727,8 @@ static void split_overlap(struct blame_entry *split,\n> \tint chunk_end_lno;\n> \tmemset(split, 0, sizeof(struct blame_entry [3]));\n> \n> +\tsplit[0].ignored = split[1].ignored = split[2].ignored = e->ignored;\n> +\n> \tif (e->s_lno < tlno) {\n> \t\t/* there is a pre-chunk part not blamed on parent */\n> \t\tsplit[0].suspect = blame_origin_incref(e->suspect);\n> @@ -862,6 +865,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n> \t\t\tint len = tlno - e->s_lno;\n> \t\t\tstruct blame_entry *n = xcalloc(1, sizeof (struct blame_entry));\n> \t\t\tn->suspect = e->suspect;\n> +\t\t\tn->ignored = e->ignored;\n> \t\t\tn->lno = e->lno + len;\n> \t\t\tn->s_lno = e->s_lno + len;\n> \t\t\tn->num_lines = e->num_lines - len;\n> @@ -916,6 +920,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n> \t\t\tint len = same - e->s_lno;\n> \t\t\tstruct blame_entry *n = xcalloc(1, sizeof (struct blame_entry));\n> \t\t\tn->suspect = blame_origin_incref(e->suspect);\n> +\t\t\tn->ignored = e->ignored;\n> \t\t\tn->lno = e->lno + len;\n> \t\t\tn->s_lno = e->s_lno + len;\n> \t\t\tn->num_lines = e->num_lines - len;\n> @@ -930,6 +935,7 @@ static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,\n> \t\t\tblame_origin_decref(e->suspect);\n> \t\t\te->suspect = blame_origin_incref(parent);\n> \t\t\te->s_lno += offset;\n> +\t\t\te->ignored = 1;\n> \t\t\te->next = ignoredp;\n> \t\t\tignoredp = e;\n> \t\t} else {\n> diff --git a/blame.h b/blame.h\n> index 086b92915e4b..56aeff582b01 100644\n> --- a/blame.h\n> +++ b/blame.h\n> @@ -92,6 +92,7 @@ struct blame_entry {\n> \t * scanning the lines over and over.\n> \t */\n> \tunsigned score;\n> +\tint ignored;\n> };\n> \n> /*\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 2f9183fb5fbd..8c3c5e435c9c 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -53,6 +53,7 @@ static int show_progress;\n> static char repeated_meta_color[COLOR_MAXLEN];\n> static int coloring_mode;\n> static const char *ignore_revs_file;\n> +static int mark_ignored_lines;\n> \n> static struct date_mode blame_date_mode = { DATE_ISO8601 };\n> static size_t blame_date_width;\n> @@ -480,6 +481,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n> \t\t\t}\n> \t\t}\n> \n> +\t\tif (mark_ignored_lines && ent->ignored) {\n> +\t\t\tlength--;\n> +\t\t\tputchar('*');\n> +\t\t}\n> \t\tprintf(\"%.*s\", length, hex);\n> \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n> \t\t\tconst char *name;\n> @@ -698,6 +703,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n> \t}\n> \tif (!strcmp(var, \"blame.ignorerevsfile\"))\n> \t\treturn git_config_pathname(&ignore_revs_file, var, value);\n> +\tif (!strcmp(var, \"blame.markignoredlines\")) {\n> +\t\tmark_ignored_lines = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> \tif (!strcmp(var, \"color.blame.repeatedlines\")) {\n> \t\tif (color_parse_mem(value, strlen(value), repeated_meta_color))\n> \t\t\twarning(_(\"invalid color '%s' in color.blame.repeatedLines\"),\n> -- \n> 2.20.1.321.g9e740568ce-goog\n> \n> \n"},{"id":"367115","messageId":"20190118123349.346ca2d1@gnomeregan.cam.corp.google.com","threadId":"50175","inReplyTo":"nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-18T17:33:49Z","receivedAt":"2019-01-18T17:33:57Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-18 at 10:47 Johannes Schindelin \nwrote:\n> ... and this change limit the user to specifying a single file, for no\n> good reason. Worse: specifying two different files via two\n> `--ignore-revs-file` parameters will only heed the latter and skip the\n> former without any warning.\n> \n> A better idea IMHO would be to use an OPT_STRING_LIST() for\n> `--ignore-revs-file`, too, and to allow for multiple\n> `blame.ignoreRevsFile` config entries (with our usual trick of handling an\n> empty setting by resetting the list of paths that were accumulated so\n> far, see e.g. how `credential.helper` is handled).\n\nI can do this if you all want, though I was modeling it off the behavior\nof -S, which also only takes a single file. \n\nThanks, everyone, for all of the feedback.\n\nBarret\n\n"},{"id":"367116","messageId":"xmqqzhrxlvfj.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"87k1j247ui.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-18T17:36:32Z","receivedAt":"2019-01-18T17:36:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Jan 17 2019, Barret Rhoden wrote:\n>\n>> -\t\tdie(\"Could not open skip list: %s\", path);\n>> [...]\n>> +\t\tdie(\"Could not open skip list: %s\", path);\n>\n> You're just moving this around, but now that this has two uses let's say\n> \"Could not open SHA-1 list; %s\" or something like that.\n>\n>> +\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n>\n> Unlike Johannes I think it's fine to leave this. This file-format is\n> SHA-1 only now. We can cross the bridge of making it (and others)\n> SHA-256 somehow when we come to that, whether that'll be allowing\n> variable width or a different file.\n\nI tend to agree. The Documentation/glossary-contents.txt makes it\nclear that \"object name\" is the most formal term to use here, with\nsynonyms like \"object identifier\" and much less formal \"hash\". For\nnow, \"SHA-1\" is good enough, even though \"object name\" is acceptable\nif we really want to future-proof. But I would suspect that people\nwould colloquially keep saying Shaah-one even when we start using\ndifferent hash function(s), so such a future-proofing may not be\nworth it ;-)\n\n"},{"id":"367141","messageId":"nycvar.QRO.7.76.6.1901182155020.41@tvgsbejvaqbjf.bet","threadId":"50175","inReplyTo":"xmqqzhrxlvfj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-18T20:59:21Z","receivedAt":"2019-01-18T21:00:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 18 Jan 2019, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > On Thu, Jan 17 2019, Barret Rhoden wrote:\n> >\n> >> -\t\tdie(\"Could not open skip list: %s\", path);\n> >> [...]\n> >> +\t\tdie(\"Could not open skip list: %s\", path);\n> >\n> > You're just moving this around, but now that this has two uses let's say\n> > \"Could not open SHA-1 list; %s\" or something like that.\n> >\n> >> +\t\t\tdie(\"Invalid SHA-1: %s\", sb.buf);\n> >\n> > Unlike Johannes I think it's fine to leave this. This file-format is\n> > SHA-1 only now. We can cross the bridge of making it (and others)\n> > SHA-256 somehow when we come to that, whether that'll be allowing\n> > variable width or a different file.\n> \n> I tend to agree. The Documentation/glossary-contents.txt makes it\n> clear that \"object name\" is the most formal term to use here, with\n> synonyms like \"object identifier\" and much less formal \"hash\". For\n> now, \"SHA-1\" is good enough, even though \"object name\" is acceptable\n> if we really want to future-proof. But I would suspect that people\n> would colloquially keep saying Shaah-one even when we start using\n> different hash function(s), so such a future-proofing may not be\n> worth it ;-)\n\nBy that reasoning all the preparatory work for switching to SHA-256 and\nmaking the references in the Git code base less tied to SHA-1 would be\nirrelevant now, \"because we can cross that bridge when we reach it\".\n\nYou are suggesting to incur technical debt here. Let's be smarter about\nthis. We do not *have* to incur said technical debt. Nothing (except\nmental laziness) makes use do that.\n\nInstead, we can make our load \"when we reach that bridge\" a lot lighter\nby already doing the right thing.\n\nBTW I totally disagree that the skip list is bound to be SHA-1. It is\nbound to be a list of object names, that's what its purpose is, and just\nbecause we happen to not yet support other hash algorithms but SHA-1 does\nnot mean that the skip list is fixed to SHA-1. It'll always be whatever\nhash algorithm is used in the current repository.\n\nSo no, introducing mentions of \"SHA-1\" *now* is not a smart thing to do.\n\nCiao,\nJohannes"},{"id":"367145","messageId":"20190118213018.GA28808@sigill.intra.peff.net","threadId":"50175","inReplyTo":"nycvar.QRO.7.76.6.1901182155020.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-18T21:30:19Z","receivedAt":"2019-01-18T21:30:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 18, 2019 at 09:59:21PM +0100, Johannes Schindelin wrote:\n\n> By that reasoning all the preparatory work for switching to SHA-256 and\n> making the references in the Git code base less tied to SHA-1 would be\n> irrelevant now, \"because we can cross that bridge when we reach it\".\n> \n> You are suggesting to incur technical debt here. Let's be smarter about\n> this. We do not *have* to incur said technical debt. Nothing (except\n> mental laziness) makes use do that.\n> \n> Instead, we can make our load \"when we reach that bridge\" a lot lighter\n> by already doing the right thing.\n> \n> BTW I totally disagree that the skip list is bound to be SHA-1. It is\n> bound to be a list of object names, that's what its purpose is, and just\n> because we happen to not yet support other hash algorithms but SHA-1 does\n> not mean that the skip list is fixed to SHA-1. It'll always be whatever\n> hash algorithm is used in the current repository.\n\nYeah, I agree with this. In particular, the code has already been\nmodified to use \"struct object_id\" and parse_oid_hex(). So it is not\neven like somebody will have to come through later and fix the\nimplementation here, and while they're at it change the \"SHA-1\" in the\nmessage. It has literally already been fixed, and is just waiting on\nparse_oid_hex() to learn about the new hashes behind the scenes.\n\nIMHO the conversion to object_id probably would have been the time to\nfix that message so we would not even have to be revisiting the\ndiscussion now. But that conversion was such a monumental pain it is\nhard to fault the authors for not picking up every scrap at that moment. ;)\n\nThat is no excuse not to do it now, though.\n\n-Peff\n"},{"id":"367154","messageId":"87h8e54n6y.fsf@evledraar.gmail.com","threadId":"50175","inReplyTo":"20190118213018.GA28808@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-18T22:26:29Z","receivedAt":"2019-01-18T22:26:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 18 2019, Jeff King wrote:\n\n> On Fri, Jan 18, 2019 at 09:59:21PM +0100, Johannes Schindelin wrote:\n>\n>> By that reasoning all the preparatory work for switching to SHA-256 and\n>> making the references in the Git code base less tied to SHA-1 would be\n>> irrelevant now, \"because we can cross that bridge when we reach it\".\n>>\n>> You are suggesting to incur technical debt here. Let's be smarter about\n>> this. We do not *have* to incur said technical debt. Nothing (except\n>> mental laziness) makes use do that.\n>>\n>> Instead, we can make our load \"when we reach that bridge\" a lot lighter\n>> by already doing the right thing.\n>>\n>> BTW I totally disagree that the skip list is bound to be SHA-1. It is\n>> bound to be a list of object names, that's what its purpose is, and just\n>> because we happen to not yet support other hash algorithms but SHA-1 does\n>> not mean that the skip list is fixed to SHA-1. It'll always be whatever\n>> hash algorithm is used in the current repository.\n>\n> Yeah, I agree with this. In particular, the code has already been\n> modified to use \"struct object_id\" and parse_oid_hex(). So it is not\n> even like somebody will have to come through later and fix the\n> implementation here, and while they're at it change the \"SHA-1\" in the\n> message. It has literally already been fixed, and is just waiting on\n> parse_oid_hex() to learn about the new hashes behind the scenes.\n>\n> IMHO the conversion to object_id probably would have been the time to\n> fix that message so we would not even have to be revisiting the\n> discussion now. But that conversion was such a monumental pain it is\n> hard to fault the authors for not picking up every scrap at that moment. ;)\n>\n> That is no excuse not to do it now, though.\n\nI stand corrected, I thought these still needed to be updated to parse\nanything that wasn't 40 chars, since I hadn't seen anything about these\nformats in the hash transition document.\n\nSo fair enough, let's change that while we're at it, but this seems like\nsomething that needs to be planned for in more detail / documented in\nthe hash transition doc.\n\nI.e. many (e.g. me) maintain some system-wide skiplist for strict fsck\ncloning of legacy repos. So I can see there being some need for a\nSHA1<->SHA256 map in this case, but since these files might stretch\nacross repo boundaries and not be checked into the repo itself this is a\nnew use-case that needs thinking about.\n\nBut now that I think about it this sort of thing would be a good\nuse-case for just fixing these various historical fsck issues while\nwe're at it when possible, e.g. \"missing space before email\" (probably\nnot all could be unambiguously fixed). So instead of sha256<->sha1\nfn(sha256)<->fn(sha1)[1]?\n\n1. https://public-inbox.org/git/87ftyyedqd.fsf@evledraar.gmail.com/\n"},{"id":"367201","messageId":"f5170cb1-4109-4ae3-7722-8e3b62fb0b92@web.de","threadId":"50175","inReplyTo":"20190117202919.157326-3-brho@google.com","subject":"Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-01-20T18:19:32Z","receivedAt":"2019-01-20T18:19:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.01.2019 um 21:29 schrieb Barret Rhoden:\n> The blame_entry will get passed up the tree until we find a commit that\n> has a diff chunk that affects those lines. If an ignored commit added\n> more lines than it removed, the blame will fall on a commit that made a\n> change nearby. There is no general solution here, just a best-effort\n> approach. For a trivial example, consider ignoring this commit:\n>\n> Z: \"Adding Lines\"\n> foo\n> +No commit\n> +ever touched\n> +these lines\n> bar\n\nWouldn't it make more sense to assign such lines to unknown, perhaps\nrepresented by an all-zero commit ID, instead of blaming a semi-random\nbystander?\n\nRené\n"},{"id":"367283","messageId":"20190122071251.GB28555@sigill.intra.peff.net","threadId":"50175","inReplyTo":"87h8e54n6y.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-22T07:12:51Z","receivedAt":"2019-01-22T07:12:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 18, 2019 at 11:26:29PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I stand corrected, I thought these still needed to be updated to parse\n> anything that wasn't 40 chars, since I hadn't seen anything about these\n> formats in the hash transition document.\n> \n> So fair enough, let's change that while we're at it, but this seems like\n> something that needs to be planned for in more detail / documented in\n> the hash transition doc.\n> \n> I.e. many (e.g. me) maintain some system-wide skiplist for strict fsck\n> cloning of legacy repos. So I can see there being some need for a\n> SHA1<->SHA256 map in this case, but since these files might stretch\n> across repo boundaries and not be checked into the repo itself this is a\n> new use-case that needs thinking about.\n\nMy assumption had been that changing your local repository would be a\n(local) flag day, and you'd update any ancillary files like skiplists,\nmailmap.blob, etc at the same time. I'm not opposed to making those\nfeatures more clever, though.\n\n> But now that I think about it this sort of thing would be a good\n> use-case for just fixing these various historical fsck issues while\n> we're at it when possible, e.g. \"missing space before email\" (probably\n> not all could be unambiguously fixed). So instead of sha256<->sha1\n> fn(sha256)<->fn(sha1)[1]?\n\nThat is a very tempting thing to do, but I think it comes with its own\ncomplications. We do not want to do fn(sha1), I don't think; the reason\nwe care about sha1 at all is that those hashes are already set in stone.\n\nThere could be a \"clean up the data as we convert to sha256\" operation,\nbut:\n\n - it needs to be set in stone from day 1, I'd think. The last thing we\n want is to modify it after conversions are in the wild\n\n - I think we need to be bi-directional. So it must be a mapping that\n can be undone to retrieve the original bytes, so we can compute\n their \"real\" sha1.\n\nAt which point, I think it might be simpler to just make git more\npermissive with respect to those minor data errors (and in fact, we are\nalready pretty permissive for the most part in non-fsck operations).\n\n-Peff\n"},{"id":"367294","messageId":"87ef953tyn.fsf@evledraar.gmail.com","threadId":"50175","inReplyTo":"20190122071251.GB28555@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-22T09:46:56Z","receivedAt":"2019-01-22T09:47:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 22 2019, Jeff King wrote:\n\n> On Fri, Jan 18, 2019 at 11:26:29PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I stand corrected, I thought these still needed to be updated to parse\n>> anything that wasn't 40 chars, since I hadn't seen anything about these\n>> formats in the hash transition document.\n>>\n>> So fair enough, let's change that while we're at it, but this seems like\n>> something that needs to be planned for in more detail / documented in\n>> the hash transition doc.\n>>\n>> I.e. many (e.g. me) maintain some system-wide skiplist for strict fsck\n>> cloning of legacy repos. So I can see there being some need for a\n>> SHA1<->SHA256 map in this case, but since these files might stretch\n>> across repo boundaries and not be checked into the repo itself this is a\n>> new use-case that needs thinking about.\n>\n> My assumption had been that changing your local repository would be a\n> (local) flag day, and you'd update any ancillary files like skiplists,\n> mailmap.blob, etc at the same time. I'm not opposed to making those\n> features more clever, though.\n>\n>> But now that I think about it this sort of thing would be a good\n>> use-case for just fixing these various historical fsck issues while\n>> we're at it when possible, e.g. \"missing space before email\" (probably\n>> not all could be unambiguously fixed). So instead of sha256<->sha1\n>> fn(sha256)<->fn(sha1)[1]?\n>\n> That is a very tempting thing to do, but I think it comes with its own\n> complications. We do not want to do fn(sha1), I don't think; the reason\n> we care about sha1 at all is that those hashes are already set in stone.\n>\n> There could be a \"clean up the data as we convert to sha256\" operation,\n> but:\n>\n> - it needs to be set in stone from day 1, I'd think. The last thing we\n> want is to modify it after conversions are in the wild\n>\n> - I think we need to be bi-directional. So it must be a mapping that\n> can be undone to retrieve the original bytes, so we can compute\n> their \"real\" sha1.\n\nIt needing to be bidirectional is a very good point, and I think that\nmakes my suggestion a non-starter. Thanks.\n\n> At which point, I think it might be simpler to just make git more\n> permissive with respect to those minor data errors (and in fact, we are\n> already pretty permissive for the most part in non-fsck operations).\n\nYeah it's probably better to make some of these \"errors\" softer\nwarnings.\n\nThe X-Y issue I have is that I turned on transfer.fsckObjects, so then I\ncan't clone repos with various minor historical issues in commit headers\netc., so I maintain a big skip list. But what I was actually after was\nfsck checks like the .gitmodules security check.\n\nOf course I could chase them all down and turn them into\nwarn/error/ignore individually, but it would be better if we e.g. had\nsome way to say \"serious things error, minor things warn\", maybe with\nthe option of only having the looser version on fetch but not recieve\nwith the principle that we should be loose in what we accept from\nexisting data but strict with new data #leftoverbits\n"},{"id":"367319","messageId":"20190122102601.24426c8e@brho.roam.corp.google.com","threadId":"50175","inReplyTo":"f5170cb1-4109-4ae3-7722-8e3b62fb0b92@web.de","subject":"Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-22T15:26:01Z","receivedAt":"2019-01-22T15:26:08Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-20 at 19:19 René Scharfe wrote:\n> Am 17.01.2019 um 21:29 schrieb Barret Rhoden:\n> > The blame_entry will get passed up the tree until we find a commit that\n> > has a diff chunk that affects those lines. If an ignored commit added\n> > more lines than it removed, the blame will fall on a commit that made a\n> > change nearby. There is no general solution here, just a best-effort\n> > approach. For a trivial example, consider ignoring this commit:\n> >\n> > Z: \"Adding Lines\"\n> > foo\n> > +No commit\n> > +ever touched\n> > +these lines\n> > bar \n> \n> Wouldn't it make more sense to assign such lines to unknown, perhaps\n> represented by an all-zero commit ID, instead of blaming a semi-random\n> bystander?\n\nI don't know if we can algorithmicly determine whether or not an\nassignment was the commit the user wanted. Maybe we could add a set\nof heuristics, like \"any diff hunk that only added\", as in my\nexample. Maybe if count_a == 0 in blame_chunk_cb, that would work for\nthat case; I'm not too familiar with that code.\n\nEither way, in my next commit I provide the option to mark the commit\nwith a '*', which could serve the same purpose as the all-zero commit\nID: flag the commit so the user knows it might not be correct.\n\nBarret\n\n"},{"id":"367329","messageId":"xmqqpnsoh92x.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"87ef953tyn.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T17:54:14Z","receivedAt":"2019-01-22T17:54:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> It needing to be bidirectional is a very good point, and I think that\n> makes my suggestion a non-starter. Thanks.\n\nYes, it is a bit sad that we need to carry the mistakes forward\nwhile moving to the new hash, but bidi convertibility is a must\nfor the transition to work smoothly, I think.\n\nThanks for a good discussion. FWIW, on the original issue that\nbrought it up, I think using \"object name\" from the glossary to\nmove away from saying \"SHA-1\" would be good.\n\n"},{"id":"367330","messageId":"xmqqlg3ch85x.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"f5170cb1-4109-4ae3-7722-8e3b62fb0b92@web.de","subject":"Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T18:14:02Z","receivedAt":"2019-01-22T18:14:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 17.01.2019 um 21:29 schrieb Barret Rhoden:\n>> The blame_entry will get passed up the tree until we find a commit that\n>> has a diff chunk that affects those lines. If an ignored commit added\n>> more lines than it removed, the blame will fall on a commit that made a\n>> change nearby. There is no general solution here, just a best-effort\n>> approach. For a trivial example, consider ignoring this commit:\n>>\n>> Z: \"Adding Lines\"\n>> foo\n>> +No commit\n>> +ever touched\n>> +these lines\n>> bar\n>\n> Wouldn't it make more sense to assign such lines to unknown, perhaps\n> represented by an all-zero commit ID, instead of blaming a semi-random\n> bystander?\n\nI share the sentiment.\n\nIsn't it, however, showing a bigger problem in the \"feature\"?\n\nYour \"a change that adds lines without removing any\" is an obvious\ncase where these added lines have no corresponding lines in the\npreimage, but most of the time it is unclear what line corresponds\nto what previous line. If a commit being \"ignored\" brought a change\nlike this:\n\n 1\n -four\n -three\n +3\n +4\n 5\n\ndid \"+3\" come from \"-three\"?\n\nOr did \"+4\" (read: \"added '4'\") come from \"-three\" (read: \"removed\n'three'\")? Did it come from \"-four\"? Or was it genuinely added by\nthat ignored commit? Your suggestion deals with the case where we\ndecide that \"+4\" had no corresponding lines in the preimage (and\npaint it as \"no blame can be assigned\"). But when we decide that\n\"+4\" came from \"-four\" or \"-three\", we continue drilling down from\nthat ignored commit and assign the blame to either the commit that\nintroduced \"four\" or the commit that introduced \"three\", which would\nobviously give us different result. Worse yet, if a reader expected\nus to consider \"+4\" came from \"-four\" at that ignored commit, but\nthe algorithm decided that \"+4\" corresponded to \"-three\", when we\nshow the commit that eventually gets blamed for that line that has\n\"4\" has no \"four\" (it has \"three\"), which I suspect would confuse\nthe reader of the output.\n\nSo... I dunno.\n\n\n"},{"id":"367335","messageId":"20190122182849.GD4399@sigill.intra.peff.net","threadId":"50175","inReplyTo":"87ef953tyn.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] Move init_skiplist() outside of fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-22T18:28:49Z","receivedAt":"2019-01-22T18:28:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 22, 2019 at 10:46:56AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > At which point, I think it might be simpler to just make git more\n> > permissive with respect to those minor data errors (and in fact, we are\n> > already pretty permissive for the most part in non-fsck operations).\n> \n> Yeah it's probably better to make some of these \"errors\" softer\n> warnings.\n> \n> The X-Y issue I have is that I turned on transfer.fsckObjects, so then I\n> can't clone repos with various minor historical issues in commit headers\n> etc., so I maintain a big skip list. But what I was actually after was\n> fsck checks like the .gitmodules security check.\n>\n> Of course I could chase them all down and turn them into\n> warn/error/ignore individually, but it would be better if we e.g. had\n> some way to say \"serious things error, minor things warn\", maybe with\n> the option of only having the looser version on fetch but not recieve\n> with the principle that we should be loose in what we accept from\n> existing data but strict with new data #leftoverbits\n\nYeah, I think the current state here is rather unfortunate. The worst\npart is that many of the things _are_ marked as warnings, but we reject\ntransfers even for warnings. So now we have \"info\" as well, which is\nreally just silly.\n\nI think the big blocker to simply loosening \"warning\" is that the\ncurrent severities are pretty arbitrary. MISSING_NAME_BEFORE_EMAIL\nprobably ought to be warning, but it's an warning. Whereas HAS_DOTGIT is\na warning, but has pretty serious security implications.\n\nSo that does not save you from chasing them all down, but if you do, at\nleast the work could benefit everybody. ;)\n\n-Peff\n"},{"id":"367344","messageId":"20190122143500.397abc8e@gnomeregan.cam.corp.google.com","threadId":"50175","inReplyTo":"xmqqlg3ch85x.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-01-22T19:35:00Z","receivedAt":"2019-01-22T19:35:08Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-22 at 10:14 Junio C Hamano wrote:\n> René Scharfe writes:\n> \n> > Am 17.01.2019 um 21:29 schrieb Barret Rhoden: \n> >> The blame_entry will get passed up the tree until we find a commit that\n> >> has a diff chunk that affects those lines. If an ignored commit added\n> >> more lines than it removed, the blame will fall on a commit that made a\n> >> change nearby. There is no general solution here, just a best-effort\n> >> approach. For a trivial example, consider ignoring this commit:\n> >>\n> >> Z: \"Adding Lines\"\n> >> foo\n> >> +No commit\n> >> +ever touched\n> >> +these lines\n> >> bar \n> >\n> > Wouldn't it make more sense to assign such lines to unknown, perhaps\n> > represented by an all-zero commit ID, instead of blaming a semi-random\n> > bystander? \n> \n> I share the sentiment.\n> \n> Isn't it, however, showing a bigger problem in the \"feature\"?\n> \n> Your \"a change that adds lines without removing any\" is an obvious\n> case where these added lines have no corresponding lines in the\n> preimage, but most of the time it is unclear what line corresponds\n> to what previous line. If a commit being \"ignored\" brought a change\n> like this:\n> \n> 1\n> -four\n> -three\n> +3\n> +4\n> 5\n> \n> did \"+3\" come from \"-three\"?\n> \n> Or did \"+4\" (read: \"added '4'\") come from \"-three\" (read: \"removed\n> 'three'\")? Did it come from \"-four\"? Or was it genuinely added by\n> that ignored commit? Your suggestion deals with the case where we\n> decide that \"+4\" had no corresponding lines in the preimage (and\n> paint it as \"no blame can be assigned\"). But when we decide that\n> \"+4\" came from \"-four\" or \"-three\", we continue drilling down from\n> that ignored commit and assign the blame to either the commit that\n> introduced \"four\" or the commit that introduced \"three\", which would\n> obviously give us different result. Worse yet, if a reader expected\n> us to consider \"+4\" came from \"-four\" at that ignored commit, but\n> the algorithm decided that \"+4\" corresponded to \"-three\", when we\n> show the commit that eventually gets blamed for that line that has\n> \"4\" has no \"four\" (it has \"three\"), which I suspect would confuse\n> the reader of the output.\n> \n> So... I dunno.\n\nI guess if you swap the lines as well as change them, then we're not\ngoing to be able to detect that. Just to be clear, if you did this:\n\nCommit A:\n---------\n other_stuff\n+one\n other_stuff\n\nCommit B:\n---------\n other_stuff\n one\n+two\n other_stuff\n\nCommit C:\n---------\n other_stuff\n-one\n-two\n+1\n+2\n other_stuff\n\nAnd ignore commit C, my change will properly identify commit A and B,\ne.g.:\n\nOTHER_HASH Author Date 11) other_stuff\n*A_HASH Author Date 12) 1\n*B_HASH Author Date 13) 2\nOTHER_HASH Author Date 14) other_stuff\n\nBut if you swapped the lines in addition to change names to numbers:\n\nCommit C-swap:\n--------------\n other_stuff\n-one\n-two\n+2\n+1\n other_stuff\n\nThen it won't have the semantic knowledge that \"one\" == \"1\". If a user\nis ignoring a commit, we don't have an oracle that knows exactly what\nthat commit did to determine what commit the user wants blamed. The\ncurrent change attempts to find the last commit that touched a line.\n\nBarret\n\n"},{"id":"367453","messageId":"xmqqo987b2g5.fsf@gitster-ct.c.googlers.com","threadId":"50175","inReplyTo":"20190122143500.397abc8e@gnomeregan.cam.corp.google.com","subject":"Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-23T19:26:18Z","receivedAt":"2019-01-23T19:26:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Barret Rhoden writes:\n\n>> So... I dunno.\n>\n> I guess if you swap the lines as well as change them,...\n> ... Then it won't have the semantic knowledge that \"one\" == \"1\". If a user\n> is ignoring a commit, we don't have an oracle that knows exactly what\n> that commit did to determine what commit the user wants blamed.\n\nYeah, and if the original had two adjacent lines, and replacement\nhas three adjacent lines, the algorithm would not even know if \n\n - the first line in the original was split into first two in the\n update and the second line was modified in place; or\n\n - the first line in the original was modified in place and the\n second line was split into the latter two lines in the update\n\nIn short, there is no answer to \"what is the corresponding line of\nthis line before this commit changed it?\" in general, and any\nalgorithm, as long as it tries to see what was the \"corresponding\nline\" of the line that is blamed to a commit, would not produce\nresults human readers would expect all the time.\n\nAs you said, heuristics may get us far enough to be useful, though\n;-).\n\n\n\n\n"},{"id":"368369","messageId":"20190201153702.2d029143@gnomeregan.cam.corp.google.com","threadId":"50175","inReplyTo":"xmqqo987b2g5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-02-01T20:37:02Z","receivedAt":"2019-02-01T20:37:11Z","isPatch":true,"sender":{"key":"brho@google.com","avatar":null},"body":"On 2019-01-23 at 11:26 Junio C Hamano wrote:\n> Yeah, and if the original had two adjacent lines, and replacement\n> has three adjacent lines, the algorithm would not even know if \n> \n> - the first line in the original was split into first two in the\n> update and the second line was modified in place; or\n> \n> - the first line in the original was modified in place and the\n> second line was split into the latter two lines in the update\n> \n> In short, there is no answer to \"what is the corresponding line of\n> this line before this commit changed it?\" in general, and any\n> algorithm, as long as it tries to see what was the \"corresponding\n> line\" of the line that is blamed to a commit, would not produce\n> results human readers would expect all the time.\n> \n> As you said, heuristics may get us far enough to be useful, though\n> ;-).\n\nYeah. We can do one more thing: when we ignore a change that added\nmore lines than it removed, we can at least not report totally\nunrelated commits. \n\nFor example, the parent of an ignored commit has this, say at line 11:\n\ncommit-a 11) void new_func_1(void *x, void *y);\ncommit-b 12) void new_func_2(void *x, void *y);\ncommit-c 13) some_line_c\ncommit-d 14) some_line_d\n\nAfter a commit 'X', we have:\n\ncommit-X 11) void new_func_1(void *x,\ncommit-X 12) void *y);\ncommit-X 13) void new_func_2(void *x,\ncommit-X 14) void *y);\ncommit-c 15) some_line_c\ncommit-d 16) some_line_d\n\nIn my existing code, if you ignore commit X, the blames look like this:\n\ncommit-a 11) void new_func_1(void *x,\ncommit-b 12) void *y);\ncommit-c 13) void new_func_2(void *x,\ncommit-d 14) void *y);\ncommit-c 15) some_line_c\ncommit-d 16) some_line_d\n\nLines 13 and 14 are blamed on the nearby commits C and D. The reason\nis the blame entry for X is four lines long, rooted at line 11, but when\nwe look back through the history, we'll look at the parent's image of\nthe file where that diff hunk is only two lines long. The extra two\nlines just blame whatever follows the diff hunk in the parent's image.\nIn this case, it is just the lines C and D repeated again.\n\nI can detect this situation when we ignore the diffs from commit X. If\nX added more lines than it removed, then I only pass the number of\nlines to the parent that the parent had. The rest get their own\nblame_entry, marked 'unblamable', which I'll catch when we create the\noutput. The parent can't find blame for lines that don't exist in its\nimage of the file. \n\nWith that change, the above example blames like this:\n\ncommit-a 11) void new_func_1(void *x,\ncommit-b 12) void *y);\n00000000 13) void new_func_2(void *x,\n00000000 14) void *y);\ncommit-c 15) some_line_c\ncommit-d 16) some_line_d\n\nAs we discussed, we still can never be certain about which commits\n*should* be blamed for which lines. (Note that line 12 was blamed on\nB, though B was the commit for new_func_2(), not new_func_1()). But I\ncan avoid blaming commits that just happen to be in the area and would\nonly be 'correct' due to dumb luck.\n\nThis also handles cases where an ignored commit only adds lines,\nwhich is a specific case of \"added more than removed.\"\n\nHandling this case is a surprisingly small change. I'll post it once I\nsort out the tests and other comments on this patch series. \n\nFor now, I went with unconditionally marking the commit as all 0s for\nthe case where we know it is 'wrong.' I can do that conditionally\nbased on blame.markIgnoredLines if you want, though I think any commit\nattributed to those lines would be misleading.\n\nThanks,\n\nBarret\n\n"}]}