archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <>
To: "René Scharfe" <>
Cc:, Barret Rhoden <>
Subject: Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list
Date: Sat, 26 Sep 2020 10:06:48 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (=?utf-8?Q?=22R?= =?utf-8?Q?en=C3=A9?= Scharfe"'s message of "Sat, 26 Sep 2020 18:23:42 +0200")

René Scharfe <> writes:

> When I request "Don't eat any glue!", perfectly human responses could be
> "But I don't have any glue!" or "It doesn't even taste that good.", but
> I'd expect a computer program to act I bit more logical and just don't
> do it, without talking back.  Maybe that's just me.
> (I had been bitten by a totally different software adding such a check,
> which made it complain about my long catch-all ignore list, and I had to
> craft and maintain a specific "clean" list for each deployment --
> perhaps I'm still bitter about that.)

A user who says "ignore v2.3", sees that the commit pointed at by
that release tag is not ignored, comes here to complain, and is told
to write v2.3^0 instead, would not be happy.  It is a mistake easy
to catch to help users, so I am more for than against that part of
the change.  I am completely neutral about "you told me to ignore
this, but as far as I can tell it does not even exist---did you 
screw up when you prepared the list of stuff to ignore?" part.  I do
not mind seeing it removed.

>> +		if (kind == OBJ_COMMIT) {
>> +			oidcpy(oid_ret, &oid);
> At that point we know it's an object, but cast it up to the most generic
> class we have -- an object ID.  We could have set an object flag to mark
> it ignored instead, which would be trivial to check later.  On the other
> hand it probably wouldn't make much of a difference -- hashmaps are
> pretty fast, and blame has lots of things to do beyond ignoring commits.

Quite honestly, I am not interested in the "blame --ignore" feature
itself.  It is good that you CC'ed Barret so that such an
improvement suggestion would be heard by the right party ;-).

>> @@ -815,10 +836,12 @@ static void build_ignorelist(struct blame_scoreboard *sb,
>>  		if (!strcmp(i->string, ""))
>>  			oidset_clear(&sb->ignore_list);
> This preexisting feature is curious.  It's even documented ('An empty
> file name, "", will clear the list of revs from previously processed
> files.') and covered by t8013.6.  Why would we need such magic in
> addition to the standard negation (--no-ignore-revs-file) for clearing
> the list?  The latter counters blame.ignoreRevsFile as well. *puzzled*

I shared the puzzlement when I saw it, but ditto.

>> +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;
>> @@ -66,7 +72,8 @@ void oidset_parse_file(struct oidset *set, const char *path)
>>  		if (!sb.len)
>>  			continue;
>> -		if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0')
>> +		if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0' ||
>> +		    (fn && fn(&oid, cbdata)))
> OK, so this turns the basic all-I-know-is-hashes oidset loader into a
> flexible higher-order map function.  Fun, but wise?  Can't make up my
> mind.

Fun and probably useful.  It is a different matter if it is wise to
use it to (1) peel tags to commits and (2) fail on an nonexistent
object.  My take on them is (1) is probably true, and (2) is Meh ;-)


  reply	other threads:[~2020-09-26 17:06 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-25  5:59 [PATCH 0/4] Clean-up around get_x_ish() Junio C Hamano
2020-09-25  5:59 ` [PATCH 1/4] t8013: minimum preparatory clean-up Junio C Hamano
2020-09-25  5:59 ` [PATCH 2/4] blame: validate and peel the object names on the ignore list Junio C Hamano
2020-09-26 16:23   ` René Scharfe
2020-09-26 17:06     ` Junio C Hamano [this message]
2020-09-26 23:58       ` Junio C Hamano
2020-09-28 13:26       ` Barret Rhoden
2020-10-11 16:03         ` René Scharfe
2020-10-12 16:54           ` Junio C Hamano
2020-10-12 20:39           ` Barret Rhoden
2020-10-13 20:12             ` René Scharfe
2020-09-25  5:59 ` [PATCH 3/4] t1506: rev-parse A..B and A...B Junio C Hamano
2020-09-25  5:59 ` [PATCH 4/4] sequencer: stop abbreviating stopped-sha file Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \
    --subject='Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).