Code review comment for lp:~fahad-aizaz/hipl/hipd-relay-config

On Mon, Dec 12, 2011 at 08:52:24PM +0000, Fahad Aizaz wrote:
> On 12/12/2011 08:34 PM, Diego Biurrun wrote:
> > On Fri, Dec 09, 2011 at 03:58:46PM +0100, Diego Biurrun wrote:
> >> [... lengthy explanation that was ignored ...]
> >
> > So I take the time to elaborate and explain how to merge this properly
> > and outline two alternative ways to go about this correctly that would
> > result in either a single merge or a single commit.
> >
> > Instead of picking one of those two, you instead decide to split your
> > work arbitrarily and push seven commits, six of which are broken in
> > different ways and all of which have bad log messages.
>
> 1) Only merge the second branch.
> 2) Drop the history of the second branch.
>
> - merging hipd-relay-config into your trunk (or whatever) branch
> - running "bzr revert --forget-merges" in that branch so that the
> changes remain, but the merged history is dropped
> - committing all the changes and
> - pushing the resulting commit to the central repository.
>
> I followed what you mentioned in your email.
>
> 1. I discarded the first branch.
> 2. Merged 2nd on into a sandbox (branch) and I didn't find any
> conflicts.
> 3. Merged this branch into trunk with and used
> bzr revert --forget-merges switch to remove all the history.

Good so far - note the operative term here: remove all the history.

> 4. I committed the changes into the trunk any typed all log messages
> again.

So now you are recreating the history which I told you to drop!

I told you to commit *all* the changes, not split the changes into
multiple commits again. And I told you to push the resulting commit
to the central repository. Note the singular, it should have been a
single one.

You have some very deep-seated misunderstandings about what a commit is
and what it should do. A commit must be atomic and it must work.

None of the commits you pushed fit that bill, most don't even compile, at
least on some platform, but trunk must *always* compile[1]. And related
changes *must* be grouped into one commit, not split arbitrarily by files
as you did.

Also your log messages are bad. You should describe what the commit does
and why, not just list the files created or touched by the commit. This
information is already in the Bazaar metadata.

I notice that HIPL's HACKING document does not say much about the subject,
so go read PISA's HACKING document again instead. Some of that and more
will need to get ported over...

Diego

[1] Hey, of course people may sometimes screw up, but not knowingly.
That's why we have private branches. To screw up in private and then
push a cleaned-up history without the messups.