Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege <skimo@kotnet.org>]

On Wed, Jul 18, 2007 at 12:02:50PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 16 Jul 2007, Sven Verdoolaege wrote:
> 
> > On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> > > On Sun, 15 Jul 2007, Sven Verdoolaege wrote:
> > > > TREECHANGE is only set when path pruning is in effect.
> > > > If I didn't check for path_pruning, then all commits would be
> > > > considered to have been pruned.  (Or am I missing something?
> > > > Honestly, I found all that TREECHANGE stuff difficult to follow.)
> > > 
> > > AFAICT TREECHANGE means that parents were rewritten.
> > 
> > I think you'll find that if all commits touch a path in the
> > path specifiers then all commits will have TREECHANGE set and
> > so no parents will be rewritten.
> 
> The code suggests otherwise.

Check again.

> But I really have to wonder: why do you play games with TREECHANGE?  I had 
> the impression that commit->parents is set appropriately by the revision 
> walker,

Only for unpruned commits and the references (explicitly specified
on the command line if you wish) may have been pruned.

> > > > revision.c itself is also riddled with "prune_fn && ".
> > > > Wouldn't it make sense to invert the meaning of this bit and call
> > > > it, say, PRUNED, so that the default is off and you would only
> > > > have to check if the bit was set ?
> > > 
> > > You meant the TREECHANGE bit?  No.
> > 
> > Yes.  Why?
> 
> Why invert the meaning of a perfectly fine bit?  Because you can?  It is 
> working right now, and it is not even a buglet, so what is there to fix?

Because it is confusing.  As explained above, the bit doesn't have a
meaning of its own.  You can only interpret the bit if some other
conditions are met.
It would be even more confusing if it meant what you claim it means.

> 
> > > BTW what do you plan to do about my objection to UNINTERESTING, given 
> > > the example "git rewrite-commits A..B x/y"?
> > 
> > That was based on an apparent misunderstanding of my code
> > that I tried to address above.  I did not intend to do what
> > you claim I do and a quick test confirms that my code does
> > indeed not to what you claim it does.
> > 
> > More specifically, the history will not be cut off at A
> > because A is marked UNINTERESTING and is therefore not considered
> > to have been pruned.
> 
> Why do you test for TREECHANGE | UNINTERESTING then?

Exactly for the reason mentioned above.
If the commit is marked UNINTERESTING then it has not been pruned,
because it hasn't even been checked for TREECHANGE.

> > A commit is considered pruned if it was either explicitly marked
> > as such or if TREECHANGE is not set, but the later check (in is_pruned)
> > is only done on commits that were checked for tree changes.
> 
> I don't understand.  What do you mean by "a commit is pruned"?  Does it 
> mean that this commit was left out from the revision walk?  What does that 
> have to do with TREECHANGE, which means that the parents set was modified?

You just claim that that is what it means.  The code (see try_to_simplify_commit
where the bit is set) and a simple experiment (explained above) show otherwise.

skimo