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@xxxxxxxxxxxxx> Thanks! -Peff
