RE: [PATCH] Bigger Chooser 2

[Robert Collins <rbcollins at cygwin dot com>]

On Sun, 2003-03-30 at 16:20, Gary R. Van Sickle wrote:
> > On Sat, 2003-03-29 at 19:30, Gary R. Van Sickle wrote:
> >
> > > > Yes. I've never liked 100K patches, even when I do them.
> > > >
> > >
> > > The source patch plus the new source file is only 50k total, at least 31k of
> > > which is solely bigger-chooser (primarily res.rc, proppage.{cc,h},
> > > propsheet.{cc,h}, Window.{cc,h}) and upon cursory examination simply is not
> > > divisible.  Significant changes were required to implement this
> > functionality,
> > > and it just isn't something that can be checked in piecemeal and maintain a
> > > working HEAD.
> >
> > Even so. There is *always* a path from A to B that doesn't break
> > functionality along the way. Thats what refactoring is all about.
> >
> 
> The 31k of the patch implementing the bigger chooser is not decomposable into
> smaller patches.  There is not always a path from A to B that can or should be
> divisible.

There *always* is. There are well documented techniques to create such a
path. Whether it's worth the effort is debatable, but the existence of
such a path is not.

> > > The other <19k is largely context around the one-liner OnActivate()
> > changes you
> > > mentioned, OnInit() one-liners, and miscellaneous other one-liners.  I don't
> > > know if stripping these from the patch will result in a working or
> > compilable
> > > patch or not.
> >
> > Well, you can bring them in, one at a time, and with just a little care
> > nothing will break.
> >
> 
> How am I going to do that?  The changes are made on my end.  I can't "un-make"
> them and "re-bring" them in, can I?

That is one way.

> > >   When I made these changes (OnActivate() intended to reduce the
> > > "next page specified/controlled in several places" issue), setup
> > patches were
> > > not being reviewed and checked in in a timely manner, so I figured
> > I'd do these
> > > all at the same time since that's what I'd end up having to submit anyway.
> >
> > Well, thats your choice.
> 
> No, it isn't.  I'd rather have made small patches, sent them in, and have them
> reviewed and committed by the time I sent in the next small patch. 

What if I disagreed with the patch and sent you back? Would you have
stopped all coding during the review process? Managing overlapping
patches is quite straightforward - you tag each separate conceptual
change in separate branches and do work that depends on them in
dependent branches. Then when an earlier patch is tweaked, you propogate
the changes down the dependency tree.

>  That hasn't
> been the way things have worked, at least until the last few weeks anyway. 

It's not now. No patch review should ever hold you up, unless it's
seeking top level strategy review - in which case a patch is not the
best (IMO) medium for having such a discussion.

>  So
> any small patch I would have sent would have languished, the next one I sent
> would have to have been diffed against HEAD and hence would contain the previous
> patch anyway, lather, rinse, repeat. 

No. The next patch would either:
* Not contain the previous patch.
* Be conditional on the other, and regenerated once the other was
accepted if changes where needed.

See what we did with Igors overlapping script patches.

>  The only option I see would be to submit a
> patch and wait until it got committed before touching the code again, and I've
> able to devote too little time to setup as it is.

Please see above for a strategy to address this problem - it's really
not needed to wait in such a fashion.

> > Seriously, such commingling raises the bar to
> > having patches accepted. It makes my job harder. My bandwidth is scarce
> > enough as it is.
> 
> As is mine.  As is all of ours.  I would think have a single large patch as
> opposed to many equivalent small patches (assuming it's possible for given large
> patch) would induce less reviewing overhead and have no real effect on actual
> reviewing effort.

Well, in my experience maintaining several OS projects, this isn't true.

>   Particularly in a case like this, where it's one primary
> patch and one "fix/improve a bunch of little things" patch.

The bunch of little things should be decomposed.

> > > And like I said, the new icon is just a bonus ;-).
> >
> > It's also largely orthogonal. You could easily offer a separate patch
> > for that.
> >
> 
> I could, but since it is orthogonal, what's the difference?

Because it's orthogonal, it adds complexity when it's part of a large
patch, it doesn't need to be there, it's simpler to review in isolation.

Rob

-- 
GPG key available at: <http://users.bigpond.net.au/robertc/keys.txt>.

Attachment: signature.asc
Description: This is a digitally signed message part