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 _______________________________________________ Bf-committers mailing list [email protected] http://lists.blender.org/mailman/listinfo/bf-committers
