Re: [PATCH] add core.usereplacerefs config option

[Jeff King <peff@peff.net>]

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

> 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