builtin/blame: ignore nonexistent ignore files

From: Jade Lovelace <[email protected]>

It's currently a problem to put blame.ignoreRevsFile in a global gitconfig, for example, to use the GitHub (and other) supported filename of .git-blame-ignore-revs by default if present in a repo, since the current implementation exits the process if it fails to open the file.

If we change this to ignore nonexistent files when specified in the config option (but NOT the command line, since the command line surely means to actually assert the files exist), this remains API compatible with old usages, while behaving properly when you want to ignore missing ones.

Because of this idea of putting .git-blame-ignore-revs in there, it doesn't seem to necessarily merit a warning, but maybe there should be one to be slightly less confusing if you make a typo?

---
blame: ignore nonexistent ignore files

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1947%2Flf-%2Fjade%2Fignore-revs-ignore-missing-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1947/lf-/jade/ignore-revs-ignore-missing-v1
Pull-Request: https://github.com/git/git/pull/1947

 Documentation/config/blame.adoc |  3 +++
 builtin/blame.c                 | 27 +++++++++++++++++++++++++--
 oidset.c                        | 17 ++++++++++++++---
 oidset.h                        | 10 ++++++++++
 t/t8013-blame-ignore-revs.sh    | 16 +++++++++++++++-
 5 files changed, 67 insertions(+), 6 deletions(-)

index 4d047c17908..6dedc48069f 100644
@@ -26,6 +26,9 @@ blame.ignoreRevsFile::
 	`#` are ignored.  This option may be repeated multiple times.  Empty
 	file names will reset the list of ignored revisions.  This option will
 	be handled before the command line option `--ignore-revs-file`.
++
+If a file in this list does not exist, it is disregarded and the next files in
+the list are processed as normal.
 
 blame.markUnblamableLines::
 	Mark lines that were changed by an ignored revision that we could not

index c470654c7ec..90fa8d4107a 100644
@@ -68,6 +68,9 @@ static int no_whole_file_rename;
 static int show_progress;
 static char repeated_meta_color[COLOR_MAXLEN];
 static int coloring_mode;
+/* Ignore-revs files specified in the config file. Processed separately to be
+ * able to ignore failing to open them. */
+static struct string_list ignore_revs_config_file_list = STRING_LIST_INIT_DUP;
 static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP;
 static int mark_unblamable_lines;
 static int mark_ignored_lines;

@@ -731,7 +734,7 @@ static int git_blame_config(const char *var, const char *value,
 		ret = git_config_pathname(&str, var, value);
 		if (ret)
 			return ret;
-		string_list_insert(&ignore_revs_file_list, str);
+		string_list_insert(&ignore_revs_config_file_list, str);
 		free(str);
 		return 0;
 	}

@@ -848,6 +851,9 @@ static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)
 }
 
 static void build_ignorelist(struct blame_scoreboard *sb,
+			     /* Ignore-revs files specified via the config,
+			      * which are allowed to fail to open. */
+			     struct string_list *ignore_revs_config_file_list,
 			     struct string_list *ignore_revs_file_list,
 			     struct string_list *ignore_rev_list)
 {

@@ -855,6 +861,23 @@ static void build_ignorelist(struct blame_scoreboard *sb,
 	struct object_id oid;
 
 	oidset_init(&sb->ignore_list, 0);
+	for_each_string_list_item(i, ignore_revs_config_file_list) {
+		if (!strcmp(i->string, ""))
+			oidset_clear(&sb->ignore_list);
+		else {
+			FILE *fp = fopen(i->string, "r");
+			if (!fp) {
+				if (errno == ENOENT) continue;
+				die("could not open blame.ignoreRevsFile file at %s", i->string);
+			}
+
+			oidset_parse_filep_carefully(&sb->ignore_list, fp, i->string,
+						    the_repository->hash_algo,
+						    peel_to_commit_oid, sb);
+
+			fclose(fp);
+		}
+	}
 	for_each_string_list_item(i, ignore_revs_file_list) {
 		if (!strcmp(i->string, ""))
 			oidset_clear(&sb->ignore_list);

@@ -1119,7 +1142,7 @@ parse_done:
 	sb.reverse = reverse;
 	sb.repo = the_repository;
 	sb.path = path;
-	build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);
+	build_ignorelist(&sb, &ignore_revs_config_file_list, &ignore_revs_file_list, &ignore_rev_list);
 	string_list_clear(&ignore_revs_file_list, 0);
 	string_list_clear(&ignore_rev_list, 0);
 	setup_scoreboard(&sb, &o);

index 8d36aef8dca..35958cbc3d6 100644
@@ -59,12 +59,24 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,
 				 oidset_parse_tweak_fn fn, void *cbdata)
 {
 	FILE *fp;
-	struct strbuf sb = STRBUF_INIT;
-	struct object_id oid;
 
 	fp = fopen(path, "r");
 	if (!fp)
 		die("could not open object name list: %s", path);
+
+	oidset_parse_filep_carefully(set, fp, path, algop, fn, cbdata);
+
+	fclose(fp);
+}
+
+void oidset_parse_filep_carefully(struct oidset *set, FILE *fp,
+				 const char *path,
+				 const struct git_hash_algo *algop,
+				 oidset_parse_tweak_fn fn, void *cbdata)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct object_id oid;
+
 	while (!strbuf_getline(&sb, fp)) {
 		const char *p;
 		const char *name;

@@ -89,6 +101,5 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,
 	}
 	if (ferror(fp))
 		die_errno("Could not read '%s'", path);
-	fclose(fp);
 	strbuf_release(&sb);
 }

index 0106b6f2787..f68aa8a3b0d 100644
@@ -2,6 +2,7 @@
 #define OIDSET_H
 
 #include "khash.h"
+#include <stdio.h>
 
 /**
  * This API is similar to oid-array, in that it maintains a set of object ids

@@ -93,6 +94,15 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,
 				 const struct git_hash_algo *algop,
 				 oidset_parse_tweak_fn fn, void *cbdata);
 
+/*
+ * Same as oidset_parse_file_carefully, but is given a pre-opened file handle.
+ * The given path is only used for diagnostics.
+ */
+void oidset_parse_filep_carefully(struct oidset *set, FILE *fp,
+				 const char *path,
+				 const struct git_hash_algo *algop,
+				 oidset_parse_tweak_fn fn, void *cbdata);
+
 struct oidset_iter {
 	kh_oid_set_t *set;
 	khiter_t iter;

index 370b7681492..0456aaba9dd 100755
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/nix/store/cvlbhhrvzfkjl2hrrzhq3vr5gzan1r60-bash-interactive-5.2p37/bin/sh
 
 test_description='ignore revisions when blaming'
 

@@ -116,6 +116,20 @@ test_expect_success ignore_revs_from_configs_and_files '
 	test_cmp expect actual
 '
 
+# Ignore-revs from the config tolerates the file not existing
+test_expect_success ignore_revs_config_file_nonexistent '
+	git config --add blame.ignoreRevsFile nonexistent_ignore_file &&
+	git blame --line-porcelain file --ignore-revs-file ignore_y >blame_raw &&
+
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse A >expect &&
+	test_cmp expect actual &&
+
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+	git rev-parse B >expect &&
+	test_cmp expect actual
+'
+
 # Override blame.ignoreRevsFile (ignore_x) with an empty string.  X should be
 # blamed now for lines 1 and 2, since we are no longer ignoring X.
 test_expect_success override_ignore_revs_file '

base-commit: c152ae3ef50dc7bbbf5089571df5bba404a96e0d

gitgitgadget

Well, that is how it is designed to be used. If the file you specify does not exist, it is likely you made a typo, which you would want to be told about.

I however am somewhat sympathetic to the cause. If the user had a way to tell Git that they know what they are doing, it would make sense in certain situations to allow the paths they specify, either from the command line or in the configuration files, to be optional, without triggering "no, there is no such file, perhaps you have a typo there?" error.

Having said all that, I am not interested in the design and implementaion presented in this patch at all, for many reasons.

- The code essentially duplicates the loop that goes over the specified files.

- Unconditionally robbing the typo protection from existing users who expect the command would refuse to start when they misconfigure is not absolute no-no. We would have some way for the user to say "I am giving this path to be used, but this is optional. Instead of failing, just pretend I didn't specify it if it does not exist".

- Singling out the ignorefile configuration and treating it differently from the command line option makes things inconsistent. Perhaps people may want to do

    [alias] fooblame = blame --ignore-revs-file="foo"
    [alias] barblame = blame --ignore-revs-file="bar"

instead of using a single configuration variable, and somehow want to mark them to be "optional".

- The situation where users want to specify a pathname for a file that is optional is not limited to blame.ignoreRevsFile. Any codepath that takes user-specified/specifiable pathname (or blob object name) should benefit if we had a single consistent way to tell Git that the path is optional.

An alternative design that goes along the following lines may be more palatable:

- The way to spell for the users to specify a path that is optional, either as the value of a command line option or a configuration variable, is to prefix it with ":(optional)". E.g.

    [blame]
	ignoreRevsFile = ":(optional).git-blame-ignore"

    $ git blame --ignore-revs-file=":(optional).git-blame-ignore"

- For command line options, all commands that use parse-options API would automatically benefit by updating parse-options.c and tweak its handling of OPTION_FILENAME; when the specified string begins with ":(optional)", you strip the prefix and see if the remainder or the string names an existing file. If it does, you use the filename as the value of that command line option; otherwise you pretend that the option didn't even exist on the command line.

- For configuration variables, you update git_config_pathname() to do the same.

Thanks.

For what it's worth, an initial implementation of ":(optional)" exists[*]. It was eventually dropped from Junio's "seen" branch merely because it never received any reviews, not due to any particular problem with it.

[*]: https://lore.kernel.org/git/[email protected]/