{"thread":{"id":"63313","subject":"[PATCH] builtin/blame: ignore nonexistent ignore files","startedAt":"2025-04-19T18:43:18Z","lastAt":"2025-04-20T05:36:11Z","messageCount":3,"participants":["jade via GitGitGadget","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"516392","messageId":"pull.1947.git.git.1745088194384.gitgitgadget@gmail.com","threadId":"63313","inReplyTo":null,"subject":"[PATCH] builtin/blame: ignore nonexistent ignore files","fromName":"jade via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-04-19T18:43:14Z","receivedAt":"2025-04-19T18:43:18Z","isPatch":true,"sender":{"key":"name:jade","avatar":null},"body":"From: Jade Lovelace <software@lfcode.ca>\n\nIt's currently a problem to put blame.ignoreRevsFile in a global\ngitconfig, for example, to use the GitHub (and other) supported filename\nof .git-blame-ignore-revs by default if present in a repo, since the\ncurrent implementation exits the process if it fails to open the file.\n\nIf we change this to ignore nonexistent files when specified in the\nconfig option (but NOT the command line, since the command line surely\nmeans to actually assert the files exist), this remains API compatible\nwith old usages, while behaving properly when you want to ignore missing\nones.\n\nBecause of this idea of putting .git-blame-ignore-revs in there, it\ndoesn't seem to necessarily merit a warning, but maybe there should be\none to be slightly less confusing if you make a typo?\n\nSigned-off-by: Jade Lovelace <software@lfcode.ca>\n---\n    blame: ignore nonexistent ignore files\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1947%2Flf-%2Fjade%2Fignore-revs-ignore-missing-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1947/lf-/jade/ignore-revs-ignore-missing-v1\nPull-Request: https://github.com/git/git/pull/1947\n\n Documentation/config/blame.adoc |  3 +++\n builtin/blame.c                 | 27 +++++++++++++++++++++++++--\n oidset.c                        | 17 ++++++++++++++---\n oidset.h                        | 10 ++++++++++\n t/t8013-blame-ignore-revs.sh    | 16 +++++++++++++++-\n 5 files changed, 67 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/blame.adoc b/Documentation/config/blame.adoc\nindex 4d047c17908..6dedc48069f 100644\n--- a/Documentation/config/blame.adoc\n+++ b/Documentation/config/blame.adoc\n@@ -26,6 +26,9 @@ blame.ignoreRevsFile::\n \t`#` are ignored.  This option may be repeated multiple times.  Empty\n \tfile names will reset the list of ignored revisions.  This option will\n \tbe handled before the command line option `--ignore-revs-file`.\n++\n+If a file in this list does not exist, it is disregarded and the next files in\n+the list are processed as normal.\n \n blame.markUnblamableLines::\n \tMark lines that were changed by an ignored revision that we could not\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex c470654c7ec..90fa8d4107a 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -68,6 +68,9 @@ 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+/* Ignore-revs files specified in the config file. Processed separately to be\n+ * able to ignore failing to open them. */\n+static struct string_list ignore_revs_config_file_list = STRING_LIST_INIT_DUP;\n static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP;\n static int mark_unblamable_lines;\n static int mark_ignored_lines;\n@@ -731,7 +734,7 @@ static int git_blame_config(const char *var, const char *value,\n \t\tret = git_config_pathname(&str, var, value);\n \t\tif (ret)\n \t\t\treturn ret;\n-\t\tstring_list_insert(&ignore_revs_file_list, str);\n+\t\tstring_list_insert(&ignore_revs_config_file_list, str);\n \t\tfree(str);\n \t\treturn 0;\n \t}\n@@ -848,6 +851,9 @@ static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)\n }\n \n static void build_ignorelist(struct blame_scoreboard *sb,\n+\t\t\t     /* Ignore-revs files specified via the config,\n+\t\t\t      * which are allowed to fail to open. */\n+\t\t\t     struct string_list *ignore_revs_config_file_list,\n \t\t\t     struct string_list *ignore_revs_file_list,\n \t\t\t     struct string_list *ignore_rev_list)\n {\n@@ -855,6 +861,23 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n \tstruct object_id oid;\n \n \toidset_init(&sb->ignore_list, 0);\n+\tfor_each_string_list_item(i, ignore_revs_config_file_list) {\n+\t\tif (!strcmp(i->string, \"\"))\n+\t\t\toidset_clear(&sb->ignore_list);\n+\t\telse {\n+\t\t\tFILE *fp = fopen(i->string, \"r\");\n+\t\t\tif (!fp) {\n+\t\t\t\tif (errno == ENOENT) continue;\n+\t\t\t\tdie(\"could not open blame.ignoreRevsFile file at %s\", i->string);\n+\t\t\t}\n+\n+\t\t\toidset_parse_filep_carefully(&sb->ignore_list, fp, i->string,\n+\t\t\t\t\t\t    the_repository->hash_algo,\n+\t\t\t\t\t\t    peel_to_commit_oid, sb);\n+\n+\t\t\tfclose(fp);\n+\t\t}\n+\t}\n \tfor_each_string_list_item(i, ignore_revs_file_list) {\n \t\tif (!strcmp(i->string, \"\"))\n \t\t\toidset_clear(&sb->ignore_list);\n@@ -1119,7 +1142,7 @@ parse_done:\n \tsb.reverse = reverse;\n \tsb.repo = the_repository;\n \tsb.path = path;\n-\tbuild_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);\n+\tbuild_ignorelist(&sb, &ignore_revs_config_file_list, &ignore_revs_file_list, &ignore_rev_list);\n \tstring_list_clear(&ignore_revs_file_list, 0);\n \tstring_list_clear(&ignore_rev_list, 0);\n \tsetup_scoreboard(&sb, &o);\ndiff --git a/oidset.c b/oidset.c\nindex 8d36aef8dca..35958cbc3d6 100644\n--- a/oidset.c\n+++ b/oidset.c\n@@ -59,12 +59,24 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,\n \t\t\t\t oidset_parse_tweak_fn fn, void *cbdata)\n {\n \tFILE *fp;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct object_id oid;\n \n \tfp = fopen(path, \"r\");\n \tif (!fp)\n \t\tdie(\"could not open object name list: %s\", path);\n+\n+\toidset_parse_filep_carefully(set, fp, path, algop, fn, cbdata);\n+\n+\tfclose(fp);\n+}\n+\n+void oidset_parse_filep_carefully(struct oidset *set, FILE *fp,\n+\t\t\t\t const char *path,\n+\t\t\t\t const struct git_hash_algo *algop,\n+\t\t\t\t oidset_parse_tweak_fn fn, void *cbdata)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct object_id oid;\n+\n \twhile (!strbuf_getline(&sb, fp)) {\n \t\tconst char *p;\n \t\tconst char *name;\n@@ -89,6 +101,5 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,\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 0106b6f2787..f68aa8a3b0d 100644\n--- a/oidset.h\n+++ b/oidset.h\n@@ -2,6 +2,7 @@\n #define OIDSET_H\n \n #include \"khash.h\"\n+#include <stdio.h>\n \n /**\n  * This API is similar to oid-array, in that it maintains a set of object ids\n@@ -93,6 +94,15 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,\n \t\t\t\t const struct git_hash_algo *algop,\n \t\t\t\t oidset_parse_tweak_fn fn, void *cbdata);\n \n+/*\n+ * Same as oidset_parse_file_carefully, but is given a pre-opened file handle.\n+ * The given path is only used for diagnostics.\n+ */\n+void oidset_parse_filep_carefully(struct oidset *set, FILE *fp,\n+\t\t\t\t const char *path,\n+\t\t\t\t const struct git_hash_algo *algop,\n+\t\t\t\t oidset_parse_tweak_fn fn, void *cbdata);\n+\n struct oidset_iter {\n \tkh_oid_set_t *set;\n \tkhiter_t iter;\ndiff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh\nindex 370b7681492..0456aaba9dd 100755\n--- a/t/t8013-blame-ignore-revs.sh\n+++ b/t/t8013-blame-ignore-revs.sh\n@@ -1,4 +1,4 @@\n-#!/bin/sh\n+#!/nix/store/cvlbhhrvzfkjl2hrrzhq3vr5gzan1r60-bash-interactive-5.2p37/bin/sh\n \n test_description='ignore revisions when blaming'\n \n@@ -116,6 +116,20 @@ test_expect_success ignore_revs_from_configs_and_files '\n \ttest_cmp expect actual\n '\n \n+# Ignore-revs from the config tolerates the file not existing\n+test_expect_success ignore_revs_config_file_nonexistent '\n+\tgit config --add blame.ignoreRevsFile nonexistent_ignore_file &&\n+\tgit blame --line-porcelain file --ignore-revs-file ignore_y >blame_raw &&\n+\n+\tsed -ne \"/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p\" blame_raw >actual &&\n+\tgit rev-parse A >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tsed -ne \"/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p\" blame_raw >actual &&\n+\tgit rev-parse B >expect &&\n+\ttest_cmp expect actual\n+'\n+\n # Override blame.ignoreRevsFile (ignore_x) with an empty string.  X should be\n # blamed now for lines 1 and 2, since we are no longer ignoring X.\n test_expect_success override_ignore_revs_file '\n\nbase-commit: c152ae3ef50dc7bbbf5089571df5bba404a96e0d\n-- \ngitgitgadget\n"},{"id":"516393","messageId":"xmqqo6wr95r6.fsf@gitster.g","threadId":"63313","inReplyTo":"pull.1947.git.git.1745088194384.gitgitgadget@gmail.com","subject":"Re: [PATCH] builtin/blame: ignore nonexistent ignore files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-19T21:54:37Z","receivedAt":"2025-04-19T21:54:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"jade via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> It's currently a problem to put blame.ignoreRevsFile in a global\n> gitconfig, for example, to use the GitHub (and other) supported filename\n> of .git-blame-ignore-revs by default if present in a repo, since the\n> current implementation exits the process if it fails to open the file.\n\nWell, that is how it is designed to be used.  If the file you\nspecify does not exist, it is likely you made a typo, which you\nwould want to be told about.\n\nI however am somewhat sympathetic to the cause.  If the user had a\nway to tell Git that they know what they are doing, it would make\nsense in certain situations to allow the paths they specify, either\nfrom the command line or in the configuration files, to be optional,\nwithout triggering \"no, there is no such file, perhaps you have a\ntypo there?\" error.\n\nHaving said all that, I am not interested in the design and\nimplementaion presented in this patch at all, for many reasons.\n\n - The code essentially duplicates the loop that goes over the\n   specified files.\n\n - Unconditionally robbing the typo protection from existing users\n   who expect the command would refuse to start when they\n   misconfigure is not absolute no-no.  We would have some way for\n   the user to say \"I am giving this path to be used, but this is\n   optional. Instead of failing, just pretend I didn't specify it if\n   it does not exist\".\n\n - Singling out the ignorefile configuration and treating it\n   differently from the command line option makes things\n   inconsistent.  Perhaps people may want to do\n\n    [alias] fooblame = blame --ignore-revs-file=\"foo\"\n    [alias] barblame = blame --ignore-revs-file=\"bar\"\n\n   instead of using a single configuration variable, and somehow\n   want to mark them to be \"optional\".\n\n - The situation where users want to specify a pathname for a file\n   that is optional is not limited to blame.ignoreRevsFile.  Any\n   codepath that takes user-specified/specifiable pathname (or blob\n   object name) should benefit if we had a single consistent way to\n   tell Git that the path is optional.\n\nAn alternative design that goes along the following lines may be\nmore palatable:\n\n - The way to spell for the users to specify a path that is\n   optional, either as the value of a command line option or a\n   configuration variable, is to prefix it with \":(optional)\".  E.g.\n\n    [blame]\n\tignoreRevsFile = \":(optional).git-blame-ignore\"\n\n    $ git blame --ignore-revs-file=\":(optional).git-blame-ignore\"\n\n - For command line options, all commands that use parse-options API\n   would automatically benefit by updating parse-options.c and tweak\n   its handling of OPTION_FILENAME; when the specified string begins\n   with \":(optional)\", you strip the prefix and see if the remainder\n   or the string names an existing file.  If it does, you use the\n   filename as the value of that command line option; otherwise you\n   pretend that the option didn't even exist on the command line.\n\n - For configuration variables, you update git_config_pathname() to\n   do the same.\n\nThanks.\n"},{"id":"516397","messageId":"CAPig+cT1BNXRotrz=rnVgvhQjZZwYgsAOQMonHFFTPfK-C0LOQ@mail.gmail.com","threadId":"63313","inReplyTo":"xmqqo6wr95r6.fsf@gitster.g","subject":"Re: [PATCH] builtin/blame: ignore nonexistent ignore files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-04-20T05:35:58Z","receivedAt":"2025-04-20T05:36:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 19, 2025 at 5:54 PM Junio C Hamano <gitster@pobox.com> wrote:\n> \"jade via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> > It's currently a problem to put blame.ignoreRevsFile in a global\n> > gitconfig, for example, to use the GitHub (and other) supported filename\n> > of .git-blame-ignore-revs by default if present in a repo, since the\n> > current implementation exits the process if it fails to open the file.\n>\n> An alternative design that goes along the following lines may be\n> more palatable:\n>\n>  - The way to spell for the users to specify a path that is\n>    optional, either as the value of a command line option or a\n>    configuration variable, is to prefix it with \":(optional)\".  E.g.\n>\n>     [blame]\n>         ignoreRevsFile = \":(optional).git-blame-ignore\"\n>\n>     $ git blame --ignore-revs-file=\":(optional).git-blame-ignore\"\n>\n>  - For command line options, all commands that use parse-options API\n>    would automatically benefit by updating parse-options.c and tweak\n>    its handling of OPTION_FILENAME; when the specified string begins\n>    with \":(optional)\", you strip the prefix and see if the remainder\n>    or the string names an existing file.  If it does, you use the\n>    filename as the value of that command line option; otherwise you\n>    pretend that the option didn't even exist on the command line.\n\nFor what it's worth, an initial implementation of \":(optional)\"\nexists[*]. It was eventually dropped from Junio's \"seen\" branch merely\nbecause it never received any reviews, not due to any particular\nproblem with it.\n\n[*]: https://lore.kernel.org/git/20241014204427.1712182-1-gitster@pobox.com/\n"}]}