Re: [PATCH 2/3] commit: add function to unparse a commit and its parents

[Christian Couder <chriscool@tuxfamily.org>]

Le lundi 18 mai 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Its parents are recursively unparsed too, because they might have
> > been changed. But its tree is not unparsed as it should not have
> > been modifed.
>
> It is a bug in any codepath if it used commit->tree without first
> checking if commit->parsed is true to begin with, so you could NULLify
> commit->tree but that shouldn't make any difference.  I agree leaving the
> tree object as-is would make sense.
>
> I am not convinced that unparsing all the _remaining_ parents recursively
> like your patch does is enough.  Isn't there a codepath that 
>
>  - parses a commit A to find list of true parents X, Y, Z;
>
>  - iterates over that list and descend into one of these parents X, doing
>    nasty things such as pruning its parents list after parsing it; and
>
>  - decides to prune that parent X from the parent list, making the parent
>    list of A into Y and Z?
>
> After such a sequence, calling your unparse_commit(A) will unparse A and
> remaining parents of Y and Z, but will still leave X in the dirty state.

I agree that unparsing all the remaining parents recursively may not be 
enough in some cases, but I don't know much about these cases. Is it only 
when revs->prune is set?

I think it should be ok when checking that all good revs are ancestor of the 
bad rev, but I may be missing something. 

It should be ok too when using "git replace" and/or grafts as the parent 
list of a commit should be changed before its parents are changed.

I also found the clear_commit_marks function that could be used when 
checking ancestors so I will resend a patch series using it and without 
patch 2/3. But I can continue to work on unparsing commits to improve 
the "git replace" series. I need to find some test cases though, and I 
cannot think of any interesting one right now.

By the way, when you say that you suspect an attempt to replace an object 
that is directly listed on the command line would not work very well with 
the current replace series, is it related to the unparsing commit problem?

Thanks,
Christian.