Christian Couder [off-list ref] 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.
Le lundi 18 mai 2009, Junio C Hamano a écrit :
Christian Couder [off-list ref] writes:
quoted
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.