Hi, On Wed, 18 Jul 2007, Daniel Barkalow wrote: > 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. True enough! > > > 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. > > > 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 - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
