Soft wrote: > On Fri, May 1, 2009 at 11:40 AM, Boroondas Gupte > <[email protected]> wrote: > >> Soft schrieb: >> >>> On Fri, May 1, 2009 at 8:26 AM, Boroondas Gupte >>> <[email protected]> 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 _______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/SLDev Please read the policies before posting to keep unmoderated posting privileges
