[Bf-committers] Cycles-Baking - 1st round of code review

[Julien RIVAUD (_FrnchFrgg_)]

Le 22/03/2014 23:43, Dalai Felinto a écrit :
> Dear all,
>
> I just submitted the bake-cycles branch for review:
> https://developer.blender.org/D421

I know I'm repeating myself, and I'm aware I'm not a blender dev and you 
chose your code policies as you saw fit, but:

That's not good practice at all to develop in a side tree then commit as 
a huge "all changes squashed into one" patch. And even more it is not a 
good format for reviewing.

Should a bug have crept in that huge commit you'll be very sorry because 
bisect will only give you "the bug was introduced among these 10000 line 
changes" instead of "the bug was introduced by this 50-line commit, 
which does exactly that single modification for that reason explained in 
the commit message".

The branch should be reviewed as a series of commits, 
rebased/tweaked/rewritten until it is satisfactory, then merged into 
master with a non-ff merge (that is you still have a merge commit even 
if git doesn't need one) where you put the message you would have put in 
the huge single commit.

It would be acceptable if the branch was rebased/rewritten until 
satisfactory, then just pushed with linear history on top of master. Not 
as good, because you loose the information that all these patches belong 
together, and you loose the possibility to explain why the feature is 
good, how it works from a wide point of view, etc...
But it is far less ugly than a single big commit, maintenance costs-wise 
at the very least.

I thought the idea behind git migration was to enable devs that already 
used git to interact more sanely with the master repo, instead of hiding 
their work with git and submitting patches with SVN as everybody else did.

Instead, what happened is that you changed your backend to git, but devs 
still only submit big patches, completely ignoring what tool they used 
to develop, and ignoring all good practice that tool would have brought. 
Apart from the easier cloning, most work still occurs exactly as it did 
before, and that's bad. (I purposedly ignore the happy few that know/can 
push multiple commits at once).

The main problem I see for now is Phabricator itself, because it doesn't 
correctly support git workflow (you cannot even export a real git patch 
out of it, it outputs SVN-style patches, with no information about 
commit parents, messages, etc.), so unless you have push access and 
decide to ignore review process, you cannot really send a new feature as 
a list of self contained, small commits with an explanatory message for 
each.

I don't know if Phabricator Differential is able to cope with patch sets 
and to propose an easy to use interface for them but the current 
situation is less than ideal to say the least. And it's not the first 
time I saw this pattern, or the last I expect.

>
> For simple changes/fixes feel free to push (or send a pull request)
> directly to my github repository:
> https://github.com/dfelinto/blender-git/tree/bake-cycles
>
> For general review and discussions is probably better if we centralize
> it in developer.blender.org.
>
> For bugs/issues I would like to use my github too, at least until the
> code is 'master'-ready:
> https://github.com/dfelinto/blender-git/issues?labels=bake-cycles&state=open
>
> Thanks,
> Dalai