[PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Yann Dirson]

This test fails with 1.3.x and HEAD.  This is a very serious bug, since it
causes data loss.

I am not sure whether it is normal that git-fsck-objects does not retun an
error code, while we can see it reports the inconsistency in --verbose
mode.  At least trying to directly access the dropped commit triggers an
error anyway.

Signed-off-by: Yann Dirson <ydirson@altern.org>
---

 t/t6200-prune-grafts.sh |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/t/t6200-prune-grafts.sh b/t/t6200-prune-grafts.sh
new file mode 100755
index 0000000..80d0d59
--- /dev/null
+++ b/t/t6200-prune-grafts.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Yann Dirson
+#
+set -e
+
+test_description='Test that git-prune does not nuke revs hidden by a graft'
+
+. ./test-lib.sh
+
+echo First > A && git-add A && git-commit -m "Add A."
+echo First > B && git-add B && git-commit -m "Add B."
+echo Second >> A && git-update-index A && git-commit -m "Append to A."
+
+test_expect_success 'initial state is valid' 'git-fsck-objects'
+
+echo $(git-rev-parse HEAD) $(git-rev-parse HEAD^^) > .git/info/grafts
+
+test_expect_success 'grafted state is valid' 'git-fsck-objects'
+test_expect_success 'prune with the graft in effect' 'git-prune'
+test_expect_success 'grafted state is valid' 'git-fsck-objects'
+
+rm .git/info/grafts
+
+test_expect_success 'grafted state is still valid' 'git-fsck-objects'
+test_expect_success 'previously-hidden rev is still there' 'git-cat-file commit HEAD^'
+
+test_done

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Linus Torvalds]

On Thu, 18 May 2006, Yann Dirson wrote:
> 
> This test fails with 1.3.x and HEAD.  This is a very serious bug, since it
> causes data loss.

Is it/does it?

I'd assume that if you have a graft, you _want_ the history to be hidden 
and pruned. 

That's how you'd drop history, if you wanted to do it on purpose.

		Linus

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Is it/does it?
>
> I'd assume that if you have a graft, you _want_ the history to be hidden 
> and pruned. 
>
> That's how you'd drop history, if you wanted to do it on purpose.

I haven't looked at what the test does, but I think he is
talking about the opposite.  fsck by design does not honor
grafts, and if you grafted a history back to your true root
commit, that "older" history will be lost.

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Linus Torvalds]

On Thu, 18 May 2006, Junio C Hamano wrote:
> 
> I haven't looked at what the test does, but I think he is
> talking about the opposite.  fsck by design does not honor
> grafts, and if you grafted a history back to your true root
> commit, that "older" history will be lost.

Ahh. Ok. Gotcha.

		Linus

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Thu, 18 May 2006, Junio C Hamano wrote:
>> 
>> I haven't looked at what the test does, but I think he is
>> talking about the opposite.  fsck by design does not honor
>> grafts, and if you grafted a history back to your true root
>> commit, that "older" history will be lost.
>
> Ahh. Ok. Gotcha.
>
> 		Linus

Is it really OK?

I said "fsck by design does not honor" as a flamebait.

And what I said was completely untrue.  Sorry.

If you have a commit chain A->B->C and graft B away by saying
C's parent is A, fsck does read graft and discards B.  But that
is what the user asked to do, so I agree with your initial
response to Yann.

And the opposite case of grafting older history back to the real
root commit was a false alarm.  You would not lose such a
history, because the ancestry traversal will go right through
the real root and traverses the older history.

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Yann Dirson]

On Thu, May 18, 2006 at 02:46:16PM -0700, Junio C Hamano wrote:
> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > Is it/does it?
> >
> > I'd assume that if you have a graft, you _want_ the history to be hidden 
> > and pruned. 
> >
> > That's how you'd drop history, if you wanted to do it on purpose.
> 
> I haven't looked at what the test does, but I think he is
> talking about the opposite.  fsck by design does not honor
> grafts, and if you grafted a history back to your true root
> commit, that "older" history will be lost.

I'm not sure I understand what you're saying.  AFACT fsck does not
ignore grafts: if a rev is not accessible from heads because of a
graft, prune drops it, and fsck does not see a problem.

Linus, I understand your point, but the current situation is
problematic: a graft does not get propagated by cg-clone (and I
suppose not by git-clone or git-fetch either), so cloning a tree which
has undergone such a pruning operation results in an incomplete clone
(indeed it is how I met the problem).  Since grafts are supposed to
have local effect only (as far as I understand), I see it as a bad
indication to have such a remote effect.

Maybe to make things safe, prune should by default consider both
physical and grafted parents as accessible, and a flag could be added
to get the current behaviour for people really knowing what they're
doing ?

Best regards,
-- 
Yann Dirson    <ydirson@altern.org> |
Debian-related: <dirson@debian.org> |   Support Debian GNU/Linux:
                                    |  Freedom, Power, Stability, Gratis
     http://ydirson.free.fr/        | Check <http://www.debian.org/>

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Junio C Hamano]

Yann Dirson <ydirson@altern.org> writes:

> I'm not sure I understand what you're saying.

Please don't; I was asleep when I typed it -- sorry.

I was hoping fsck was doing the right thing for a very low level
tool -- verify commit objects itself, without relying on the
object parser machinery which does funky things like grafts.

Apparently it doesn't -- which is mostly good.  You can safely
remove commits you wanted to discard by grafting it away like
your test did, and you can keep unrelated history you grafted to
the back of your real root commit.

For the "clone without propagating grafts" issue, I think we
need to have a way to communicate grafts across repositories.
As you say, grafts is a local policy issue, but when you start
cloning you _are_ sharing that local policy across repositories.
Futzing fsck to honor and ignore grafts at the same time sounds
like a band-aid to me.

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Yann Dirson]

On Fri, May 19, 2006 at 12:20:45AM +0200, Yann Dirson wrote:
> On Thu, May 18, 2006 at 02:46:16PM -0700, Junio C Hamano wrote:
> > Linus Torvalds <torvalds@osdl.org> writes:
> > 
> > > Is it/does it?
> > >
> > > I'd assume that if you have a graft, you _want_ the history to be hidden 
> > > and pruned. 
> > >
> > > That's how you'd drop history, if you wanted to do it on purpose.
> > 
> > I haven't looked at what the test does, but I think he is
> > talking about the opposite.  fsck by design does not honor
> > grafts, and if you grafted a history back to your true root
> > commit, that "older" history will be lost.
> 
> I'm not sure I understand what you're saying.  AFACT fsck does not
> ignore grafts: if a rev is not accessible from heads because of a
> graft, prune drops it, and fsck does not see a problem.
> 
> Linus, I understand your point, but the current situation is
> problematic: a graft does not get propagated by cg-clone (and I
> suppose not by git-clone or git-fetch either), so cloning a tree which
> has undergone such a pruning operation results in an incomplete clone
> (indeed it is how I met the problem).  Since grafts are supposed to
> have local effect only (as far as I understand), I see it as a bad
> indication to have such a remote effect.

To make my point maybe more clear: if someone really wants to make a
graft permanent, wouldn't some history rewriting (eg. with
cg-admin-rewritehist, but see the patch I posted against it) be the
way to go, instead of relying on out-of-band transfer of graft data ?

-- 
Yann Dirson    <ydirson@altern.org> |
Debian-related: <dirson@debian.org> |   Support Debian GNU/Linux:
                                    |  Freedom, Power, Stability, Gratis
     http://ydirson.free.fr/        | Check <http://www.debian.org/>

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Junio C Hamano]

Yann Dirson <ydirson@altern.org> writes:

> To make my point maybe more clear: if someone really wants to make a
> graft permanent, wouldn't some history rewriting ... be the
> way to go,...

Yes.

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Yann Dirson]

On Thu, May 18, 2006 at 03:53:36PM -0700, Junio C Hamano wrote:
> Yann Dirson <ydirson@altern.org> writes:
> 
> > To make my point maybe more clear: if someone really wants to make a
> > graft permanent, wouldn't some history rewriting ... be the
> > way to go,...
> 
> Yes.

So if temporary usage is a typical use for grafts, don't we want to
protect people using them from pruning ?  I got no feedback to my
suggestion of changing the default behaviour, even to say it was a bad
idea :)

Best regards,
-- 
Yann Dirson    <ydirson@altern.org> |
Debian-related: <dirson@debian.org> |   Support Debian GNU/Linux:
                                    |  Freedom, Power, Stability, Gratis
     http://ydirson.free.fr/        | Check <http://www.debian.org/>

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Jakub Narebski]

Yann Dirson wrote:

> On Thu, May 18, 2006 at 03:53:36PM -0700, Junio C Hamano wrote:
>> Yann Dirson <ydirson@altern.org> writes:
>> 
>> > To make my point maybe more clear: if someone really wants to make a
>> > graft permanent, wouldn't some history rewriting ... be the
>> > way to go,...
>> 
>> Yes.
> 
> So if temporary usage is a typical use for grafts, don't we want to
> protect people using them from pruning ?  I got no feedback to my
> suggestion of changing the default behaviour, even to say it was a bad
> idea :)

Perhaps prune should be conservative by default, and follow both grafts and
original parents, and use appropriate options to preserve or not preserve
grafts.

-- 
Jakub Narebski
Warsaw, Poland

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Linus Torvalds]

On Fri, 19 May 2006, Yann Dirson wrote:

> On Thu, May 18, 2006 at 03:53:36PM -0700, Junio C Hamano wrote:
> > Yann Dirson <ydirson@altern.org> writes:
> > 
> > > To make my point maybe more clear: if someone really wants to make a
> > > graft permanent, wouldn't some history rewriting ... be the
> > > way to go,...
> > 
> > Yes.
> 
> So if temporary usage is a typical use for grafts, don't we want to
> protect people using them from pruning ?  I got no feedback to my
> suggestion of changing the default behaviour, even to say it was a bad
> idea :)

I don't actually know how much grafts end up being used. Right now, the 
only really valid use I know about is to graft together the old kernel 
history kind of thing, and I suspect not a whole lot of people do that (I 
keep a separate kernel history tree around for when I need to look at it, 
and it doesn't happen all that often).

So I think the lack of feedback on the graft-related issue comes directly 
from that lack of graft usage. 

We _could_ decide that fsck should just follow the "real parents" and the 
grafts _both_. That's the safe thing to do by default. Possibly with a 
flag to say "prefer one over the other", or even a "prefer which-ever 
exists".

		Linus

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Yann Dirson]

On Fri, May 19, 2006 at 12:02:48PM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 19 May 2006, Yann Dirson wrote:
> 
> > On Thu, May 18, 2006 at 03:53:36PM -0700, Junio C Hamano wrote:
> > > Yann Dirson <ydirson@altern.org> writes:
> > > 
> > > > To make my point maybe more clear: if someone really wants to make a
> > > > graft permanent, wouldn't some history rewriting ... be the
> > > > way to go,...
> > > 
> > > Yes.
> > 
> > So if temporary usage is a typical use for grafts, don't we want to
> > protect people using them from pruning ?  I got no feedback to my
> > suggestion of changing the default behaviour, even to say it was a bad
> > idea :)
> 
> I don't actually know how much grafts end up being used. Right now, the 
> only really valid use I know about is to graft together the old kernel 
> history kind of thing, and I suspect not a whole lot of people do that (I 
> keep a separate kernel history tree around for when I need to look at it, 
> and it doesn't happen all that often).
> 
> So I think the lack of feedback on the graft-related issue comes directly 
> from that lack of graft usage. 

I take this as an incentive to share my use of the think :)

On several projects managed with CVS, I use a git mirror (maintained
with git-cvsimport for now) to prepare my sets of patches with stgit,
before committing them to cvs (through git-cvsexportcommit).  In this
context, since merges are not recorded in cvs, and cvs insists that
all branches must derive from the trunk, I use grafts to:

	1. record merges
	2. cause git to believe that the trunk derives from the vendor
	   branch
	3. hide those pseudo revisions cvs adds to rcs files saying
	   "file was initially added to branch foo"

It is the latter use which caused the loss previously mentionned.  It
could have been avoided by making cvsimport, or more likely cvsps more
clever wrt this case.


> We _could_ decide that fsck should just follow the "real parents" and the 
> grafts _both_. That's the safe thing to do by default. Possibly with a 
> flag to say "prefer one over the other", or even a "prefer which-ever 
> exists".

I'm not sure I see how "prefer which-ever exists" would be useful - do
you have anything precise in mind ?

Best regards,
-- 
Yann Dirson    <ydirson@altern.org> |
Debian-related: <dirson@debian.org> |   Support Debian GNU/Linux:
                                    |  Freedom, Power, Stability, Gratis
     http://ydirson.free.fr/        | Check <http://www.debian.org/>

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Linus Torvalds]

On Fri, 19 May 2006, Yann Dirson wrote:
> 
> > We _could_ decide that fsck should just follow the "real parents" and the 
> > grafts _both_. That's the safe thing to do by default. Possibly with a 
> > flag to say "prefer one over the other", or even a "prefer which-ever 
> > exists".
> 
> I'm not sure I see how "prefer which-ever exists" would be useful - do
> you have anything precise in mind ?

It would be a "once you've pruned one or the other, don't complain any 
more about the fact that it doesn't exist" flag.

		Linus

Re: [PATCH] [BUG] Add a test to check git-prune does not throw away revs hidden by a graft.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Fri, 19 May 2006, Yann Dirson wrote:
>
>> On Thu, May 18, 2006 at 03:53:36PM -0700, Junio C Hamano wrote:
>> > Yann Dirson <ydirson@altern.org> writes:
>> > 
>> > > To make my point maybe more clear: if someone really wants to make a
>> > > graft permanent, wouldn't some history rewriting ... be the
>> > > way to go,...
>> > 
>> > Yes.
>> 
>> So if temporary usage is a typical use for grafts, don't we want to
>> protect people using them from pruning ?  I got no feedback to my
>> suggestion of changing the default behaviour, even to say it was a bad
>> idea :)

I just gave a terse "Yes" because I agree with Yann that if
really a permanent history rewriting is needed it should be done
by history rewriting not with graft (I do not necessarily
encourage people to rewrite history but if somebody wants to,
that is).

> We _could_ decide that fsck should just follow the "real parents" and the 
> grafts _both_. That's the safe thing to do by default. Possibly with a 
> flag to say "prefer one over the other", or even a "prefer which-ever 
> exists".

I agree with everything Linus said about the current grafts
usage.  My vote for fsck is to make it default to follow both by
default for safety, and perhaps an optional --ignore-graft
flag.

Re: [PATCH] [BUG] Add a test to check git-prune does not throw awayrevs hidden by a graft.

[David Lang]

On Fri, 19 May 2006, Linus Torvalds wrote:

> On Fri, 19 May 2006, Yann Dirson wrote:
>
>> On Thu, May 18, 2006 at 03:53:36PM -0700, Junio C Hamano wrote:
>>> Yann Dirson <ydirson@altern.org> writes:
>>>
>>>> To make my point maybe more clear: if someone really wants to make a
>>>> graft permanent, wouldn't some history rewriting ... be the
>>>> way to go,...
>>>
>>> Yes.
>>
>> So if temporary usage is a typical use for grafts, don't we want to
>> protect people using them from pruning ?  I got no feedback to my
>> suggestion of changing the default behaviour, even to say it was a bad
>> idea :)
>
> I don't actually know how much grafts end up being used. Right now, the
> only really valid use I know about is to graft together the old kernel
> history kind of thing, and I suspect not a whole lot of people do that (I
> keep a separate kernel history tree around for when I need to look at it,
> and it doesn't happen all that often).
>
> So I think the lack of feedback on the graft-related issue comes directly
> from that lack of graft usage.

if/when shallow clones become available I would expect to see graft useage 
climb significantly

David Lang

-- 
There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies.
  -- C.A.R. Hoare