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

[Junio C Hamano <gitster@pobox.com>]

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:

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

Thanks.

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

The rules under which I operate are (1) 'next', 'master', or
'maint' will not rewind, hence (2) anything that is merged into
these three branches won't be amended, either.

Running "git -p show-branch next master maint 154a955" and
scrolling to the end would show that up to "peel_onion: handle
NULL" are in 'next' and 'master' (I wanted to make sure these
safety-tightening commits are fine and then wanted to merge them
eventually to 'maint', so this topic forked from 'maint' branch
and is not meant to contain any new stuff in 'master').

 ! [next] Merge branch 'db/cover-letter' into next
  ! [master] git-apply --whitespace=fix: fix off by one thinko
   ! [maint] Documentation/git-am.txt: Pass -r in the example invoca...
    ! [154a955] receive-pack: use strict mode for unpacking objects
 ----
    ...
    + [154a955] receive-pack: use strict mode for unpacking objects
    + [154a955^] index-pack: introduce checking mode
    + [154a955~2] unpack-objects: prevent writing of inconsistent objects
    + [154a955~3] unpack-object: cache for non written objects
    + [154a955~4] add common fsck error printing function
    + [154a955~5] builtin-fsck: move common object checking code to fsck.c
    + [154a955~6] builtin-fsck: reports missing parent commits
    + [154a955~7] Remove unused object-ref code
    + [154a955~8] builtin-fsck: move away from object-refs to fsck_walk
    + [154a955~9] add generic, type aware object chain walker
 ++ + [154a955~10] peel_onion: handle NULL
 ++ + [154a955~11] check return value from parse_commit() in various functions
 ++ + [154a955~12] parse_commit: don't fail, if object is NULL
 ++ + [154a955~13] revision.c: handle tag->tagged == NULL
 ++ + [154a955~14] reachable.c::process_tree/blob: check for NULL
 ++ + [154a955~15] process_tag: handle tag->tagged == NULL
 ++ + [154a955~16] check results of parse_commit in merge_bases
 ++ + [154a955~17] list-objects.c::process_tree/blob: check for NULL
 ++ + [154a955~18] reachable.c::add_one_tree: handle NULL from lookup_tree
 ++ + [154a955~19] mark_blob/tree_uninteresting: check for NULL
 ++ + [154a955~20] get_sha1_oneline: check return value of parse_object
 ++ + [154a955~21] read_object_with_reference: don't read beyond the buffer
 ++++ [maint~20] GIT 1.5.4.2

So "peel_onion" fix and all commits below it are cast in stone.  But
"add generic walker" and later ones are not, and we can do whatever we
want to them.

If you have additional checks for other commands, that are not included
in 154a955, they naturally would make separate independent commits to be
applied on top of 154a955.  But if you find embarrassing typo or a grave
bug in them, you may want to send a replacement patch instead of
incremental fix-up, because there is no point to record an earlier
mistake in the public history only to later amend it, if mistakes are
already known.

I am tempted to merge the ones up to "peel_onion" to 'maint' soon, by
the way.

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

Yeah, some words in CodingGuidelines might be a good idea.