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

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Jul 18, 2007, 16:25 UTC
Message-ID: <Pine.LNX.4.64.0707181140450.14596@iabervon.org>
In-Reply-To: <Pine.LNX.4.64.0707181053130.14781@racer.site>

On Wed, 18 Jul 2007, Johannes Schindelin wrote:

Show 11 quoted lines

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

Show 6 quoted lines

> > 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. :)

Show 5 quoted lines

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