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

[Daniel Barkalow <barkalow@iabervon.org>]

On Wed, 18 Jul 2007, Johannes Schindelin wrote:

> Hi,
> 
> 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. In general, a hundred lines of '-' with maybe a 
'+' is hard to read, but I think whole functions of '-' without anything 
else are easy; scan the left column, and find that the whole thing goes 
away. If a patch is discarding blocks which are exactly whole top-level 
definitions, those changes are probably either correct or totally broken 
(depending on whether those blocks were actually used); you just have to 
get suspicious around preprocessor stuff. Your change ended up being 
trivially what the message described: bunch of blocks not in one or the 
other of the resulting files, some functions make not static, renamed, 
and/or had arguments changed, and the function you have to call after 
unbundle if you want it.

> > 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?