Re: [PATCH 05/10] builtin-fsck: move common object checking code to fsck.c

From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Date: Feb 26, 2008, 21:35 UTC
Message-ID: <20080226213523.GA26618@auto.tuwien.ac.at>
In-Reply-To: <7vskzg6pmw.fsf@gitster.siamese.dyndns.org>

On Tue, Feb 26, 2008 at 01:19:51AM -0800, Junio C Hamano wrote:

> Is this series an unadjusted resend or something?  This particular one
> had funny interaction with your own d4fe07f (git-fsck: report missing
> author/commit line in a commit as an error) that is already in
> 'master', so I had to munge it by hand.

I sent the series directly based on master, as Shawn suggested (21c34 is only changing the prefix in Makefile): 3c5fb6a798a0b686e7818bf1da63791fb94a7b21 receive-pack: use strict mode for unpacking objects 786bf704ce4067c80055a1fa69be242a59880eb0 index-pack: introduce checking mode 1f7ae754550fb6e0509c1498ba9de6b5f4bba438 unpack-objects: prevent writing of inconsistent objects 143aa20e11c70595e4119a3adac0887446524c7f unpack-object: cache for non written objects 997a515fccb3ef200cb96fbb757366eff8a2ee66 add common fsck error printing function b0785b6b99c641b2fec99eb48da340af627e3b0d builtin-fsck: move common object checking code to fsck.c fa9c45a16cc194c87c113c9740eb5a6e17b66cc1 builtin-fsck: reports missing parent commits a93e35027c53f06d2db2adbb14fa916871e23e46 Remove unused object-ref code 19eae91b8e3d2e72616397edf77a13d4ac79d7ab builtin-fsck: move away from object-refs to fsck_walk 0ca75709265281548be81cad4f396f4cf936dbfb add generic, type aware object chain walker 21c34821c02458f45422e747853bde913d43c625 Lokale Anpassung Makefile 99d8ea2c5ce6fc0b06fe8a43e7c0c108ddad853b git-bundle.txt: Add different strategies to create the bundle 8e0fbe671f6a63b885702917bf4e7d7a85c59ab4 builtin-for-each-ref.c: fix typo in error message 8a8bf4690e20a545561249a9b393c1ef3239c03d send-email: test compose functionality

The patch was sent as usual, so I don't know, why it should not apply.

> It was not so pleasant
> (a large chunk of code was moved from builtin-fsck.c to fsck.c),
> but that is what the maintainer does, so it's Ok.  But I'd like
> you to eyeball the result to see if it looks sane.

I have compared it to 3c5fb6a798a0b686e7818bf1da63791fb94a7b21 and everything seems to look OK. I'll do better verification in the next days.

Show 7 quoted lines

> I'll push it out as part of 'pu'.  The tip of your topic is
> 154a955 (receive-pack: use strict mode for unpacking objects)
> tonight:
> 
> 	Side note: To find a tip of a topic yourself, look for "Merge
> 	mk/maint-parse-careful" in "git log --first-parent
> 	origin/next..origin/pu" output and find the latest one.

Show 5 quoted lines

> One thing I noticed was that parse_$type_buffer() family all take
> non-const void *buf pointers but one new caller you introduced takes
> "const void *data" and passes that pointer to them.  I hated to, but
> ended up loose-casting it.  You may want to make the function take
> non-const pointer, but I did not look very carefully.

The easiest thing would be to remove the const from the data parameter in sha1_object (index-pack.c).

How should I handle changes? Send a patch ontop of 154a955 or should I send a amended version of the patches?

> By the way, while I was at it, many stylistic issues bugged me too
> much, so I ended up fixing them as well:
>

[...]

Show 12 quoted lines

> 
>  * If you cast, cast to the right type ;-)
> 
> 	struct tree *item = (struct tree *) obj;
> 
>    not
> 
> 	struct tree *item = (struct item*) obj;
>
> Please do not make me do this again, as I do not have infinite amount
> of time.  This plea is not only about your patch but applies to
> everybody.

Sorry for struct item. I really should have looked more carefully at the make output.

I did not know all of these styling guidelines. SubmittingPatches only talks about broken mailer. Maybe it would be a good thing to include them somewhere.

As I'm not very good at catching all these issues, I tried checkpatch.pl (from the linux.git:/scripts) on my patches. After turning the 80 characters/line check off, it show me formating errors, about which you complained. So I'll try to run it over my patches in the future.

mfg Martin Kögler