On Wed, Jul 18, 2018 at 1:31 PM Jeff King <peff@xxxxxxxx> wrote: > > 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. I think so, too > 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. :) As someone burned by coming up with renaming (or rather lack thereof), I'd happily watch from the sideline this time. I think this patch is good; ship it. :-) Thanks, Stefan
