Re: [PATCH] add core.usereplacerefs config option

From: Jeff King <peff@peff.net>
Date: Jul 18, 2018, 20:31 UTC
Message-ID: <20180718203152.GA26110@sigill.intra.peff.net>
In-Reply-To: <c36c1b42-3f37-b32d-08a8-2ced5d254149@gmail.com>

On Wed, Jul 18, 2018 at 04:23:20PM -0400, Derrick Stolee wrote:

Show 9 quoted lines

> This patch looks good to me. The only thing I saw was when I ran 'git grep
> check_replace_refs' and saw the following in environment.c:
> 
>     int check_replace_refs = 1; /* NEEDSWORK: rename to read_replace_refs */
> 
> This does help me feel confident that the case where the config value is
> missing will default to 'yes, please check replace refs', but also the
> NEEDSWORK could be something to either (1) do, or (2) remove the comment.
> Neither needs to happen as part of this patch.

Yeah, it was actually that comment that led me to Stefan's recent c3c36d7de2 (replace-object: check_replace_refs is safe in multi repo environment, 2018-04-11).

And ironically, back when I originally wrote this patch, it _was_ called read_replace_refs. That changed in afc711b8e1 (rename read_replace_refs to check_replace_refs, 2014-02-18), which was in turn picking up a leftover from e1111cef23 (inline lookup_replace_object() calls, 2011-05-15).

Since Stefan's patch logically undoes e1111cef23, I think that's why he put in the comment to move back to the old name.

Personally, I do not find one name any more informative than the other, and would be happy to leave it as-is (dropping the comment).

But I'm also fine with following through on the "do". According to c3c36d7de2, that was waiting for a calmer time in the code base. I guess the best way to find out is to write the patch and see how terribly it conflicts with pu. :)

> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>

Thanks!

-Peff