[sldev] [META] indentation fixes/changes and merging

[Melinda Green]

Soft wrote:
> On Fri, May 1, 2009 at 11:40 AM, Boroondas Gupte
> <sllists at boroon.dasgupta.ch> wrote:
>   
>> Soft schrieb:
>>     
>>> On Fri, May 1, 2009 at 8:26 AM, Boroondas Gupte
>>> <sllists at boroon.dasgupta.ch> wrote:
>>>
>>>       
>>>> If I remember correctly, Linden Lab once discouraged us from sending them
>>>> indentation-only patches, because the overhead to review and import them was
>>>> too much in relation to the benefit of such changes. Instead we should clean
>>>> up code while working on the specific parts for bigger bugs of features.
>>>> This is quite some while ago, so I don't know if that's still the current
>>>> recommendation.
>>>>
>>>>         
>>> The problem with indentation patches isn't review, it's merging.
>>>
>>> If 200 lines in one file are indented, they all show up as changed.
>>> What happens when that's merged with a branch without the indentation
>>> change, but where one line in that soup of 200 was changed? It's very
>>> likely that the change with a code effect will get lost without a
>>> very, very careful eye.
>>>       
>> Hmm ... indeed. Forgot about that one. :-(
>>     
>>> It's also going to slow down the merge, which
>>> extends our critical path.
>>>
>>> LL's own future move to mercurial or better merge tools may change
>>> that. The remaining question would then be review time. Right now, a
>>> bunch of formatting changes wouldn't make it back to the tree, though.
>>>
>>>       
>> I fear hg (or any other modern SCM) won't help very much here, as their
>> automatic merging is still line-based, so this will almost always
>> trigger a merge conflict to be resolved manually. I wonder how other big
>> projects handle this issue, as keeping false indentation in the code
>> until the specific part of code is touched for another reason can't be
>> the proper solution.
>>     
>
> Honestly, I think they handle it by not having branches spend so long
> diverged from trunk/. The chances of merge conflicts go up
> exponentially with the number of changes made independently. Better
> compartmentalization of the code would also help. It would become more
> likely that cleanup changes could be made in the branch where other
> development on that code is happening.
>
> The take-away I get from this is that cleanup is hard if the code is
> already poorly structured. So that underscores the importance of
> cleaning up code when touching the code for other purposes. Maybe this
> means the best way for you to go forward with your project would be to
> look for ways to link pools of janitorial work to other patch
> submissions touching the same code.

I love Boroondas' intentions and I agree with Soft's analysis but I 
don't agree completely with his conclusion. If you are only going to 
clean up the actual lines you need to change, then that will not cause 
diff obfuscation, but the temptation to then also clean up neighboring 
lines becomes huge and leads to the problem you're trying to avoid. The 
solution I prefer is to submit changes in two commits. In the first one 
I do all the clean-up of the whole general area in which I intend to 
change and commit that with a standard comment such as "Whitespace only. 
No functional change." I then do my substantive work and check it in as 
usual. That way the diffs will always make sense. When looking at the 
whitespace diffs, you can confirm at a glance that nothing substantive 
changed, and when looking at the substantive diff, you won't see any 
whitespace changes highlighted. Note that this makes code reviews much 
easier too. I don't feel that the whitespace commits should require code 
reviews, and I certainly don't want my reviewers to be distracted by 
whitespace changes while trying to understand my substantive changes.

So my proposal for our situation is:
1) Don't make whitespace fixes if you don't also have substantive 
changes to make to that area of code.
2) Commit all your whitespace fixes separately from your substantive 
changes.

-Melinda