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

From: Sven Verdoolaege <skimo@kotnet.org>
Date: Jul 18, 2007, 12:05 UTC
Message-ID: <20070718120502.GZ999MdfPADPa@greensroom.kotnet.org>
In-Reply-To: <Pine.LNX.4.64.0707181153200.14781@racer.site>

On Wed, Jul 18, 2007 at 12:02:50PM +0100, Johannes Schindelin wrote:

Show 18 quoted lines

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

Show 11 quoted lines

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

Show 14 quoted lines

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

Show 7 quoted lines

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