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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> wrote:
>
> Noah Pendleton <noah.pendleton@xxxxxxxxx> 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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux