Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

> On Wed, 18 Jul 2007, Johannes Schindelin wrote:
> 
> > On Tue, 17 Jul 2007, Daniel Barkalow wrote:
> > 
> > > You should use -C on this sort of thing, so that the interesting aspects 
> > > of the patch are easier to see. (It actually comes out longer in this 
> > > case, but it's far easier to tell that the code in the new file is the 
> > > same as the old code.)
> > 
> > Okay, I wanted it to be kept short, since I really get lost easily in 
> > hundreds of "-" lines, with possibly one in the midst being a "+".
> 
> Actually, putting the functions in the original order made the -C diff 
> shorter than without -C.

> > > Aside from presentation, it looks good to me. Shall I stick the 
> > > bundle changes into my series? I'd like to have them come before the 
> > > patch to switch to builtin-fetch, so that there aren't any revisions 
> > > where "git fetch" doesn't have bundle support.
> > 
> > Looks fine to me.  Seems like you should add a SOB line, too.
> 
> Ah, yes. I'll have to see if I'll be the first person in git development 
> to have a SOB line that's neither first nor last. :)

> > > And I think it would be best to take part 3 as a review fix to my 
> > > final patch.
> > 
> > Yes, definitely.  This shows again (to me, at least), that just 
> > looking at the code is not enough, you have to run it, too, to review 
> > patches.
> 
> You caught that by running it? I've been running this code, and I've never 
> done anything with it which caused fetch_refs to fail and then checked the 
> result. I thought you must have found it by looking for missing checks of 
> return values. Or did you find it when you'd implemented half of bundle 
> support and it didn't complain?