On Wed, Aug 09, 2006 at 02:42:13PM -0700, Junio C Hamano wrote:
> Fredrik Kuivinen <freku045@xxxxxxxxxxxxxx> 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.
> >> 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)".
> 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
-
: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html