Re: [PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 19, 2016, 09:30 UTC
Message-ID: <20160219093023.GA12470@lanh>
In-Reply-To: <xmqqvb5y7vuj.fsf@gitster.mtv.corp.google.com>

On Mon, Feb 08, 2016 at 01:09:24PM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
> > Instead of a custom commit walker like get_shallow_commits(), this new
> > function uses rev-list to mark NOT_SHALLOW to all reachable commits,
> > except borders. The definition of reachable is to be defined by the
> > protocol later. This makes it more flexible to define shallow boundary.
> >
> > Note: if a commit has one NOT_SHALLOW parent and one SHALLOW parent,
> > then it's considered the boundary. Which means in the client side, this
> > commit has _no_ parents. This could lead to surprising cuts if we're not
> > careful.
> >
> > Another option is to include more commits and only mark commits whose
> > all parents are SHALLOW as boundary.
> 
> The second and third are greek to me at this point ;-) but hopefully
> they will become clear as we read on.

Yeah. Everything looks clearer with illustration. This should be a better. The question is should we do something about it now, or leave it as is.

I'm tempted to go with "the first way" in future (so add some comments about this in is_repository_shallow, instead of leaving it as commit message in this patch).

-- 8< -- The way we find find border is paint all reachable commits NOT_SHALLOW. Any of them that "touches" commits without NOT_SHALLOW flag are considered shallow (e.g. zero parents via grafting mechanism). Shallow commits and their true parents are all marked SHALLOW. Then NOT_SHALLOW is removed from shallow commits at the end.

There is interesting observation, though somewhat off topic for this patch. In the following graph, "x" is unreachable commits. "b" is the parent of "a".

           x -- a -- o
	       /    /
         x -- b -- o

And as a result, "a" and "b" are both considered shallow commits. After grafting occurs at the client side, what we see is

                a -- o
	            /
              b -- o

Notice that because of grafting, "a" has zero parents, so "b" is no longer a parent of "a".

This is unfortunate and may be solved in two ways. The first is change the way shallow grafting works and keeps "b -- a" connection if "b" exists and is a shallow commit.

The second way is produce this graph (at client side) instead

           x -- a -- o
	       /    /
              b -- o

Which means we mark "x" as a shallow commit instead of "a". -- 8< -- -- Duy