Re: [PATCH and RFC] gitweb: Remove --full-history from git_history

From: Fredrik Kuivinen <freku045@student.liu.se>
Date: Aug 10, 2006, 21:39 UTC
Message-ID: <20060810213920.GC13446@c165.ib.student.liu.se>
In-Reply-To: <7vk65h1t8q.fsf@assigned-by-dhcp.cox.net>

On Wed, Aug 09, 2006 at 02:42:13PM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> Fredrik Kuivinen <freku045@student.liu.se> writes:
>
> The thing is, your patch, while I very much liked the direction
> it was taking us, was so intrusive that it scared the h*ck out
> of me ;-).
>

I kind of suspected that :) It certainly is intrusive and it changes revision.c which is at the very core of git.

Show 23 quoted lines

> >> What is needed by gitweb is for git-rev-list to not only follow revisions
> >> of file contents across renames, and return more revisions, 
> >
> > Note that it is not enough to only return more revisions.
> >
> > For example, consider the commits (newest commit first)
> > A: Rename "bar" to "foo"
> > B: No changes to "bar"
> > C: No changes to "bar", delete "foo"
> > <more commits here>
> >
> > Then you want "git-rev-list --renames A -- foo" to return A,... not A,C,...
> 
> Yes.  For this "following renames for a single file" example, we
> should not extend the pathspec which originally starts out as
> "foo" to _include_ "bar"; instead it should _replace_, and after
> that point we should not care about "foo".
> 
> This was another reason I shied away from your patch back then.
> We would need to think about interactions between this pathspec
> for a single file (which should be switched) and pathspecs for
> directories ("more useful form of usage than single file", as
> Linus would put it).

I mentioned a couple of different strategies for handling the directory case in the original mail.

> I have this vague feeling, without revisiting the code to make
> an informed argument, that it might be cleaner and with less
> impact (read: smaller chance to break the "directories" usage)
> if we special case "follow a single file" case.  I dunno.

Yes, it most certainly is. We actually already have the "follow a single file from a single commit" case implemented already, in blame.c. But it is much cooler to be able to track multiple files from several different starting points ("git-rev-list --renames A B C -- foo bar") :) It also makes the git-rev-list interface more consistent. No special cases such as "if you use --renames then you can only specify one pathspec (which has to be a filename)".

Show 21 quoted lines

> Also I was not sure how well your pathspec switching worked
> across forks and merges.
> 
>                             M bar
>            .----------------4------5
>           /                         \
>  A bar   /    R bar->foo  M foo      \     M foo
>  0------1-----2-----------3-----------6----7-------8
> 
> Suppose we are at 8 and start digging for the history of "foo".
> Our pathspec starts out as "foo" at 8 and stays so until we hit
> 6.  If the lower branch renamed bar->foo while the upper didn't,
> and merge-recursive merged them correctly at 6, we would use
> "foo" as pathspec while on the lower branch while traversing 6,
> 3 and 2 (at that point we switch to "bar").  On the upper
> branch, we switch pathspec to "bar" while traversing 6, 5, 4.
> When we hit 1, pathspec of both of its children (2 and 4) happen
> to match in this example.  But what would we do if for some
> reason they didn't?  Do we care?  Is that die("an internal
> error")?  I did not have a good anser to that question, and I
> still don't.

I _think_ that the patch as it currently is would track both files from 1 and onwards. That is, if the upper branch tracks "foobar" when we hit 1 and the lower branch tracks "bar" then we would track both "foobar" and "bar" at 1 and 0.

- Fredrik