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

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

Thanks.

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.

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