> On Feb 27, 2016, at 9:34 AM, Geert Janssens <geert.gnuc...@kobaltwit.be> > wrote: > >>>> Some independent commits are on the next branch. > > I didn't realize "next" was actually the name of a branch. I read it to mean > "some other branch > with follow-up commits to be merged afterwards". > > I looked at the next branch. Most of the commits on that branch are fine to > me, except for: > > 1. One commit adds egg-menu-manager files, but is not using them in any way > (yet). Move this > into the gtk+3 branch to the point you actually need them. > 2. I haven't check whether your clang-format-style definition matches the > formatting we have > been doing so far with astyle from time to time. The last time I ran it was > with these settings: > astyle --indent=spaces=4 --brackets=break --suffix=none > If that's what your clang formatter does I'm fine with it. > > Do keep in mind for all subsequent commits we prefer to keep formatting > changes in separate > commits from actual code changes. That helps tremendously in bisecting later > on. > > So if you make these few changes, that part can be merged already as far as > I'm concerned. >
I've put some comments in-line, but in general your commit messages are still too brief. You need to explain why as well as what unless it's blindingly obvious, and the only blindingly obvious changes I saw were the uninitialized variable warnings. You need to make a case somewhere for adding the config files and changing the C standard. Your personal convenience and "why not" (your argument to date for the doap file) are not sufficient to get your changes merged. "Prepare for Gtk-3" is way too generic. I doubt that only 8 files in register-gnome would need the kind of updates you did. ISTM it should be part of a separate branch called something like "register-gnome-gtk3" and this commit could be called "Prepare register-gnome for Gtk3 step 1". If these changes are needed to use GooCanvas instead of GnomeCanvas then "Prepare register-gnome for GooCanvas step 1" would be even better. It might be helpful to you to point out that there are two ways of merging a feature branch: Fast-forward or with a merge commit. Single or a few disjoint commits are normally merged fast-forward, while "projects" that require several commits to accomplish the goal are better done with a merge commit to preserve the integrity of the commit series, even though we're likely to also rebase the PR branch on the target before committing it to minimize conflicts and to keep things linear. (We learned *that* lesson when I merged the private-kvp branch last year without rebasing it first. It made quite a mess which Geert kindly cleaned up for me.) That's one reason why we keep pushing you to keep your work organized into feature branches. The other is that we know from unhappy experience that new code makes bugs and regressions, and that git bisect is a valuable tool for finding their cause. Being able to understand a change's purpose from a well-written commit message and its conte! xt in a series of other changes makes it much easier to tell the difference between a simple mistake and a deeper design issue a year or two (or ten) later when working out how to fix a bug. Regards, John Ralls _______________________________________________ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
