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

Hi,

On Wed, 18 Jul 2007, Daniel Barkalow wrote:

On Wed, 18 Jul 2007, Johannes Schindelin wrote:

quoted

On Tue, 17 Jul 2007, Daniel Barkalow wrote:

quoted

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.

True enough!

quoted

quoted

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

We have 28 commits ATM in next's history, having 3 SOB lines: 
1e76b702c1e754c7e6df1ced9ce6f1863cb7e092 is the most recent one.

quoted

quoted

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?

Exactly.  It is the "bundle 1" test that failed.

Ciao,
Dscho