[PATCH 02 of 10] scmutil: delegate mustaudit property to the real opener

Mads Kiilerich mads at kiilerich.com
Thu Sep 13 19:25:13 CDT 2012


Bryan O'Sullivan wrote, On 09/14/2012 01:21 AM:
> On Thu, Sep 13, 2012 at 2:56 PM, Mads Kiilerich <mads at kiilerich.com 
> <mailto:mads at kiilerich.com>> wrote:
>
>     This - and the previous change - seems a bit strange without a
>     comment that the mustaudit property so far is unused.
>
>
> Isn't that a bit redundant, given what a small patch this is? 
> Genuinely curious, not being argumentative.

Somewhat redundant, yes. I only found it worth saying in the passing 
before making a less redundant comment.

But not completely redundant, IMO.

The patch - both the description alone and when including the diff - 
answers the 'what' question but not the 'why' question. Not knowing the 
intention makes it harder to review. Why is mustaudit delegated? Why do 
we need this change? Is it a bug fix where it should be reviewed whether 
the bug really has been fixed ... and where there probably should have 
been a test? Or is it a refactoring ... where it is strange that it 
doesn't have any use? Or is it an optimization?

Knowing the answer - that it is a partial refactoring which introduce 
some dead 'commando pattern' code that will be used later on - makes it 
a lot easier to understand and review now and later on when digging history.

/Mads



More information about the Mercurial-devel mailing list