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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 18, 2007, 11:02 UTC
Message-ID: <Pine.LNX.4.64.0707181153200.14781@racer.site>
In-Reply-To: <20070716102407.GL999MdfPADPa@greensroom.kotnet.org>

Hi,

On Mon, 16 Jul 2007, Sven Verdoolaege wrote:

Show 19 quoted lines

> On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> > On Sun, 15 Jul 2007, Sven Verdoolaege wrote:
> > > > > +	if (path_pruning &&
> > > > > +	    !(commit->object.flags & (TREECHANGE | UNINTERESTING)))
> > > > > +		return 1;
> > > > 
> > > > Why only with "path_pruning"?  Ah yes.  Because otherwise, you would 
> > > > assume "A" in "A..B" to be pruned.
> > > 
> > > 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.

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, and that you do not have to do _anything_ for that to work.

Maybe the "--grep" thing does not yet. But then you should fix it in revision.c. Not in builtin-rewrite-commits.c

Show 8 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?

Show 11 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?

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

Ciao, Dscho