Noah Pendleton [off-list ref] writes:
Setting a global `blame.ignoreRevsFile` can be convenient, since I
usually use `.git-blame-ignore-revs` in repos. If the file is missing,
though, `git blame` exits with failure. This patch changes it to skip
over non-existent ignore-rev files instead of erroring.
That cuts both ways, though. Failing upon missing configuration
file is a way to catch misconfiguration that is hard to diagnose.
I wonder if we can easily learn where the configuration variable
came from in the codepath that diagnoses it as a misconfiguration.
If it came from a per-repo configuration and names a non-existent
file, it clearly is a misconfiguration that we want to flag as an
error. Even if it came from a per-user configuration, if it was
specified in a conditionally included file, it is likely to be a
misconfiguration. If it came from a per-user configuration that
applies without any condition, it can be a good convenience feature
to silently (or with a warning) ignore missing file.
Thanks for the quick response!
Very good point about no longer catching misconfiguration. For
detecting provenance of a setting, I think we'd need to tag the config
options with it when they're loaded, possibly in 'struct
config_set_element' or similar. What do you think about instead
emitting a warning message on stderr in the case of misconfiguration,
but still continuing? Eg:
diff --git a/builtin/blame.c b/builtin/blame.c
index e5b45eddf4..6ee8f29313 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -835,7 +835,9 @@ static void build_ignorelist(struct blame_scoreboard *sb,
for_each_string_list_item(i, ignore_revs_file_list) {
if (!strcmp(i->string, ""))
oidset_clear(&sb->ignore_list);
- else if (file_exists(i->string))
+ else if (!file_exists(i->string))
+ warning(_("skipping ignore-revs-file %s"), i->string);
+ else
oidset_parse_file_carefully(&sb->ignore_list, i->string,
peel_to_commit_oid, sb);
}
On Sat, Aug 7, 2021, 16:58 Junio C Hamano [off-list ref] wrote:
Noah Pendleton [off-list ref] writes:
quoted
Setting a global `blame.ignoreRevsFile` can be convenient, since I
usually use `.git-blame-ignore-revs` in repos. If the file is missing,
though, `git blame` exits with failure. This patch changes it to skip
over non-existent ignore-rev files instead of erroring.
That cuts both ways, though. Failing upon missing configuration
file is a way to catch misconfiguration that is hard to diagnose.
I wonder if we can easily learn where the configuration variable
came from in the codepath that diagnoses it as a misconfiguration.
If it came from a per-repo configuration and names a non-existent
file, it clearly is a misconfiguration that we want to flag as an
error. Even if it came from a per-user configuration, if it was
specified in a conditionally included file, it is likely to be a
misconfiguration. If it came from a per-user configuration that
applies without any condition, it can be a good convenience feature
to silently (or with a warning) ignore missing file.
On 07/08/2021 22:58, Junio C Hamano wrote:
Noah Pendleton [off-list ref] writes:
That cuts both ways, though. Failing upon missing configuration
file is a way to catch misconfiguration that is hard to diagnose.
I wonder if we can easily learn where the configuration variable
came from in the codepath that diagnoses it as a misconfiguration.
If it came from a per-repo configuration and names a non-existent
file, it clearly is a misconfiguration that we want to flag as an
error. Even if it came from a per-user configuration, if it was
specified in a conditionally included file, it is likely to be a
misconfiguration. If it came from a per-user configuration that
applies without any condition, it can be a good convenience feature
to silently (or with a warning) ignore missing file.
I am very interested in this feature, but I'd like to add another point
to the discussion: in the case of ignoreRevsFile in particular, no one
creates a repository with such a file; it is always added later.
However, when bisecting (a typical usage scenario for git-blame), we may
end up returning back to a point _before_ the file had been added, and
then, git-blame fails. This often happens to me, and I am then forced to
`touch` the file to create it again, only to ensure git-blame keeps
working. And then, when I want to return to the HEAD commit, the file
must be erased again otherwise there is a conflict. So, for me, the
"ignore if absent" behavior seems to me like it should be the default.