[PATCH 0/1] blame: Skip missing ignore-revs file

[PATCH 0/1] blame: Skip missing ignore-revs file

[Noah Pendleton]

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.


Noah Pendleton (1):
  blame: skip missing ignore-revs-file's

 Documentation/blame-options.txt |  2 +-
 Documentation/config/blame.txt  |  3 ++-
 builtin/blame.c                 |  2 +-
 t/t8013-blame-ignore-revs.sh    | 10 ++++++----
 4 files changed, 10 insertions(+), 7 deletions(-)

-- 
2.32.0

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Junio C Hamano]

Noah Pendleton <noah.pendleton@gmail.com> 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.

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Noah Pendleton]

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 <gitster@pobox.com> wrote:
>
> Noah Pendleton <noah.pendleton@gmail.com> 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.

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Junio C Hamano]

Noah Pendleton <noah.pendleton@gmail.com> writes:

> 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:

Unconditionally continuing with just a warning would not be a good
approach for at least two reasons.  (1) the user may truly have
intended that ignoreRevsFile to be optional, in which case the
warning is a nuisance that does not add any value, and (2) it truly
may be a misconfiguration that the named file did not exist, but the
output from "git blame" will wipe the display and the warning would
very well go unnoticed (or more likely the user may notice that
there was a warning, but it will go away before the user has a
chance to really read it, which is a lot worse and frustrating
experience).

I think an easier way out is to introduce a new configuration
variable blame.ignoreRevsFileIsOptional which takes a boolean value,
and when it is set to true, silently ignore when the named file does
not exist without any warning.  When the variable is set to false
(or the variable does not exist), we can keep the current behaviour
of noticing a misconfigured blame.ignoreRevsFile and error out.

That way, the current users who rely on the typo detection feature
can keep relying on it, and those who want to make it optional can
do so without getting annoyed by a warning.

[PATCH v2] blame: add config `blame.ignoreRevsFileIsOptional`

[Noah Pendleton]

Setting the config option `blame.ignoreRevsFile` globally to eg
`.git-blame-ignore-revs` causes `git blame` to error when the file
doesn't exist in the current repository:

```
fatal: could not open object name list: .git-blame-ignore-revs
```

Add a new config option, `blame.ignoreRevsFileIsOptional`, that when set
to true, `git blame` will silently ignore any missing ignoreRevsFile's.

Signed-off-by: Noah Pendleton <noah.pendleton@gmail.com>
---
Reworked this patch to add a new config
`blame.ignoreRevsFileIsOptional`, which controls whether missing
files specified by ignoreRevsFile cause an error or are silently
ignored.

Updated tests and docs to match.

 Documentation/blame-options.txt |  3 ++-
 Documentation/config/blame.txt  |  5 +++++
 builtin/blame.c                 |  7 ++++++-
 t/t8013-blame-ignore-revs.sh    | 14 ++++++++++----
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 117f4cf806..199a28ab79 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -134,7 +134,8 @@ take effect.
 	`fsck.skipList`.  This option may be repeated, and these files will be
 	processed after any files specified with the `blame.ignoreRevsFile` config
 	option.  An empty file name, `""`, will clear the list of revs from
-	previously processed files.
+	previously processed files. If `blame.ignoreRevsFileIsOptional` is true,
+	missing files will be silently ignored.
 
 -h::
 	Show help message.
diff --git a/Documentation/config/blame.txt b/Documentation/config/blame.txt
index 4d047c1790..2aae851e4b 100644
--- a/Documentation/config/blame.txt
+++ b/Documentation/config/blame.txt
@@ -27,6 +27,11 @@ blame.ignoreRevsFile::
 	file names will reset the list of ignored revisions.  This option will
 	be handled before the command line option `--ignore-revs-file`.
 
+blame.ignoreRevsFileIsOptional::
+	Silently skip missing files specified by ignoreRevsFile or the command line
+	option `--ignore-revs-file`. If unset, or set to false, missing files will
+	cause a nonrecoverable error.
+
 blame.markUnblamableLines::
 	Mark lines that were changed by an ignored revision that we could not
 	attribute to another commit with a '*' in the output of
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9a..df132b34ce 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -56,6 +56,7 @@ static int coloring_mode;
 static struct string_list ignore_revs_file_list = STRING_LIST_INIT_NODUP;
 static int mark_unblamable_lines;
 static int mark_ignored_lines;
+static int ignorerevsfileisoptional;
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -715,6 +716,9 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		string_list_insert(&ignore_revs_file_list, str);
 		return 0;
 	}
+	if (!strcmp(var, "blame.ignorerevsfileisoptional")) {
+		ignorerevsfileisoptional = git_config_bool(var, value);
+	}
 	if (!strcmp(var, "blame.markunblamablelines")) {
 		mark_unblamable_lines = git_config_bool(var, value);
 		return 0;
@@ -835,7 +839,8 @@ 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
+		/* skip non-existent files if ignorerevsfileisoptional is set */
+		else if (!ignorerevsfileisoptional || file_exists(i->string))
 			oidset_parse_file_carefully(&sb->ignore_list, i->string,
 						    peel_to_commit_oid, sb);
 	}
diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
index b18633dee1..f789426cbf 100755
--- a/t/t8013-blame-ignore-revs.sh
+++ b/t/t8013-blame-ignore-revs.sh
@@ -127,18 +127,24 @@ test_expect_success override_ignore_revs_file '
 	grep -E "^[0-9a-f]+ [0-9]+ 2" blame_raw | sed -e "s/ .*//" >actual &&
 	test_cmp expect actual
 	'
-test_expect_success bad_files_and_revs '
+test_expect_success bad_revs '
 	test_must_fail git blame file --ignore-rev NOREV 2>err &&
 	test_i18ngrep "cannot find revision NOREV to ignore" err &&
 
-	test_must_fail git blame file --ignore-revs-file NOFILE 2>err &&
-	test_i18ngrep "could not open.*: NOFILE" err &&
-
 	echo NOREV >ignore_norev &&
 	test_must_fail git blame file --ignore-revs-file ignore_norev 2>err &&
 	test_i18ngrep "invalid object name: NOREV" err
 '
 
+# Non-existent ignore-revs-file should fail unless
+# blame.ignoreRevsFileIsOptional is set
+test_expect_success bad_file '
+	test_must_fail git blame file --ignore-revs-file NOFILE &&
+
+	git config --add blame.ignorerevsfileisoptional true &&
+	git blame file --ignore-revs-file NOFILE
+'
+
 # For ignored revs that have added 'unblamable' lines, mark those lines with a
 # '*'
 # 	A--B--X--Y
-- 
2.32.0

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think an easier way out is to introduce a new configuration
> variable blame.ignoreRevsFileIsOptional which takes a boolean value,
> and when it is set to true, silently ignore when the named file does
> not exist without any warning.  When the variable is set to false
> (or the variable does not exist), we can keep the current behaviour
> of noticing a misconfigured blame.ignoreRevsFile and error out.
>
> That way, the current users who rely on the typo detection feature
> can keep relying on it, and those who want to make it optional can
> do so without getting annoyed by a warning.

A bit more ambitious might want to consider another more generally
applicable avenue, which would help the userbase a lot more, before
continuing.

We start from the realization that this is not the only
configuration variable that specifies a filename that could be
missing.  There may be other variables that name files to be used
("git config --help" would hopefully be the most comprehensive, but
"git grep -e git_config_pathname \*.c" would give us quicker
starting point to gauge how big an impact to the system we would be
talking about).

What do the codepaths that use these variables do when they find
that the named files are missing?  Do some of them die, some
others just warn, and yet some others silently ignore?  Would such
an inconsistency hurt our users?

Among the ones that die, are there ones that could reasonably
continue as if the configuration variable weren't there and no file
was specified (i.e. similar to what you want blame.ignoreRevsFile to
do)?  Among the ones that are silently ignored, are there ones that
may benefit by having a typo-detection?  Do all of them benefit if
the behaviour upon missing files can be configurable by the end-user?

Depending on the answers to the above questions, it might be that it
is not a desirable approach to add "blame.ignoreRevsFileIsOptional"
configuration variable, as all the existing configuration variables
that name files would want to add their own.  We might be better off
inventing a syntax for the value of blame.ignoreRevsFile (and other
variables that name files) to mark if the file is optional (i.e.
silently ignore if the named file does not exist) or required (i.e.
diagnose as a configuration error).  For example, we may borrow from
the "magic" syntax for pathspecs that begin with ":(", with comma
separated "magic" keywords and ends with ")" and specify optional
pathname configuration like so:

    [blame] ignoreRevsFile = :(optional).gitignorerevs

and teach the config parser to pretend as if it saw nothing when it
notices that the named file is missing.  That approach would cover
not just this single variable, but other variables that are parsed
using git_config_pathname() may benefit the same way (of course, the
callsites for git_config_pathmame() must be inspected and adjusted
for this to happen).

Thanks.

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Noah Pendleton]

Very good point- I see about 21 call sites for `git_config_pathname`,
plus a few others (`git_config_get_pathname`) that bottom out in the
same function. I could see the utility of optional paths for some of
them: for example, `commit.template`, `core.excludesfile`. Some of the
others seem a little more ambiguous, eg `http.sslcert` probably wants
to always fail in case of missing file.

There seems to be a mix of fail-hard on invalid paths, printing a
warning message and skipping, and silently ignoring.

Hard for me to predict what the least confusing behavior is around
path configuration values, though, so maybe adding support for the
`:(optional)` (and maybe additionally a `:(required)`) tag across the
board to pathname configs is the right move.

That patch might be beyond what I'm capable of, though I'm happy to
put up a draft that applies it to the original `ignoreRevsFile` case
as a starting point.

On Sun, Aug 8, 2021 at 1:50 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I think an easier way out is to introduce a new configuration
> > variable blame.ignoreRevsFileIsOptional which takes a boolean value,
> > and when it is set to true, silently ignore when the named file does
> > not exist without any warning.  When the variable is set to false
> > (or the variable does not exist), we can keep the current behaviour
> > of noticing a misconfigured blame.ignoreRevsFile and error out.
> >
> > That way, the current users who rely on the typo detection feature
> > can keep relying on it, and those who want to make it optional can
> > do so without getting annoyed by a warning.
>
> A bit more ambitious might want to consider another more generally
> applicable avenue, which would help the userbase a lot more, before
> continuing.
>
> We start from the realization that this is not the only
> configuration variable that specifies a filename that could be
> missing.  There may be other variables that name files to be used
> ("git config --help" would hopefully be the most comprehensive, but
> "git grep -e git_config_pathname \*.c" would give us quicker
> starting point to gauge how big an impact to the system we would be
> talking about).
>
> What do the codepaths that use these variables do when they find
> that the named files are missing?  Do some of them die, some
> others just warn, and yet some others silently ignore?  Would such
> an inconsistency hurt our users?
>
> Among the ones that die, are there ones that could reasonably
> continue as if the configuration variable weren't there and no file
> was specified (i.e. similar to what you want blame.ignoreRevsFile to
> do)?  Among the ones that are silently ignored, are there ones that
> may benefit by having a typo-detection?  Do all of them benefit if
> the behaviour upon missing files can be configurable by the end-user?
>
> Depending on the answers to the above questions, it might be that it
> is not a desirable approach to add "blame.ignoreRevsFileIsOptional"
> configuration variable, as all the existing configuration variables
> that name files would want to add their own.  We might be better off
> inventing a syntax for the value of blame.ignoreRevsFile (and other
> variables that name files) to mark if the file is optional (i.e.
> silently ignore if the named file does not exist) or required (i.e.
> diagnose as a configuration error).  For example, we may borrow from
> the "magic" syntax for pathspecs that begin with ":(", with comma
> separated "magic" keywords and ends with ")" and specify optional
> pathname configuration like so:
>
>     [blame] ignoreRevsFile = :(optional).gitignorerevs
>
> and teach the config parser to pretend as if it saw nothing when it
> notices that the named file is missing.  That approach would cover
> not just this single variable, but other variables that are parsed
> using git_config_pathname() may benefit the same way (of course, the
> callsites for git_config_pathmame() must be inspected and adjusted
> for this to happen).
>
> Thanks.
>

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Junio C Hamano]

Noah Pendleton <noah.pendleton@gmail.com> writes:

> Very good point- I see about 21 call sites for `git_config_pathname`,
> plus a few others (`git_config_get_pathname`) that bottom out in the
> same function. I could see the utility of optional paths for some of
> them: for example, `commit.template`, `core.excludesfile`. Some of the
> others seem a little more ambiguous, eg `http.sslcert` probably wants
> to always fail in case of missing file.

Thanks for already doing initial surveillance.  Very useful.

> There seems to be a mix of fail-hard on invalid paths, printing a
> warning message and skipping, and silently ignoring.
>
> Hard for me to predict what the least confusing behavior is around
> path configuration values, though, so maybe adding support for the
> `:(optional)` (and maybe additionally a `:(required)`) tag across the
> board to pathname configs is the right move.

I originally hoped only ":(optional)" would be necessary, but to
keep the continuity in behaviour for those currently that do not die
upon seeing a missing file, we probably should treat an unadorned
value as asking for the "traditional" behaviour, at least in the
shorter term, and allow those users who want to detect typos to
tighten the rule using ":(required)".  I dunno.

> That patch might be beyond what I'm capable of, though I'm happy to
> put up a draft that applies it to the original `ignoreRevsFile` case
> as a starting point.

Thanks for an offer.  We are not in a hurry (especially during the
pre-release feature freeze), and hopefully this discussion would
pique other developers' interest to nudge them to help ;-)

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Thranur Andul]

On 07/08/2021 22:58, Junio C Hamano wrote:
> Noah Pendleton <noah.pendleton@gmail.com> 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.