[v3,1/3] repack: point out a bug handling stale shallow info
diff mbox series

Message ID ed8559b91b7a3b51a5950d62e78fc726ed5b44c2.1540245934.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • repack -ad: fix after fetch --prune in a shallow repository
Related show

Commit Message

Junio C Hamano via GitGitGadget Oct. 22, 2018, 10:05 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

A `git fetch --prune` can turn previously-reachable objects unreachable,
even commits that are in the `shallow` list. A subsequent `git repack
-ad` will then unceremoniously drop those unreachable commits, and the
`shallow` list will become stale. This means that when we try to fetch
with a larger `--depth` the next time, we may end up with:

	fatal: error in object: unshallow <commit-hash>

Reported by Alejandro Pauly.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Junio C Hamano Oct. 24, 2018, 3:39 a.m. UTC | #1
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> A `git fetch --prune` can turn previously-reachable objects unreachable,
> even commits that are in the `shallow` list. A subsequent `git repack
> -ad` will then unceremoniously drop those unreachable commits, and the
> `shallow` list will become stale. This means that when we try to fetch
> with a larger `--depth` the next time, we may end up with:
>
> 	fatal: error in object: unshallow <commit-hash>

Nicely analysed and described.  One minor thing nagged me in the
implementation but it is not a big issue.

> +...
> +	d="$(git -C shallow-server rev-parse --verify D)" &&
> +	git -C shallow-server checkout master &&
> +
> +...
> +	! grep $d shallow-client/.git/shallow &&

We know D (and $d) is not a tag, but perhaps the place that assigns
to $d (way above) can do the rev-parse of D^0, not D, to make it
more clear what is going on, especially given that...

> +	git -C shallow-server branch branch-orig D^0 &&

... this does that D^0 thing here to makes us wonder if D needs
unwrapping before using it as a commit (not commit-ish). 

If we did so, we could use $d here instead of D^0.

> +	git -C shallow-client fetch --prune --depth=2 \
> +		origin "+refs/heads/*:refs/remotes/origin/*"
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd
Johannes Schindelin Oct. 24, 2018, 8:12 a.m. UTC | #2
Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > +...
> > +	d="$(git -C shallow-server rev-parse --verify D)" &&
> > +	git -C shallow-server checkout master &&
> > +
> > +...
> > +	! grep $d shallow-client/.git/shallow &&
> 
> We know D (and $d) is not a tag,


Actually, it is... the `test_commit D` creates that tag, and that is what
I use here.

> but perhaps the place that assigns to $d (way above) can do the
> rev-parse of D^0, not D, to make it more clear what is going on,
> especially given that...

... that the `grep` really wants to test for the absence of the *commit*,
not the *tag* in .git/shallow?

Yes, you are right ;-)

So why did my test do the right thing, if it looked at a tag, but did not
dereference it via ^0? The answer is: the `test_commit` function creates
light-weight tags, i.e. no tag objects. And therefore, the $d^0 you found
below, that's just confusing.

I will change it so that the `rev-parse` call uses ^0 (even if it is
technically not necessary), to avoid said confusion.

Thanks,
Dscho

> 
> > +	git -C shallow-server branch branch-orig D^0 &&
> 
> ... this does that D^0 thing here to makes us wonder if D needs
> unwrapping before using it as a commit (not commit-ish). 
> 
> If we did so, we could use $d here instead of D^0.
> 
> > +	git -C shallow-client fetch --prune --depth=2 \
> > +		origin "+refs/heads/*:refs/remotes/origin/*"
> > +'
> > +
> >  . "$TEST_DIRECTORY"/lib-httpd.sh
> >  start_httpd
> 
>
Johannes Schindelin Oct. 24, 2018, 8:38 a.m. UTC | #3
Hi Junio,

me again. I was wrong.

On Wed, 24 Oct 2018, Johannes Schindelin wrote:

> On Wed, 24 Oct 2018, Junio C Hamano wrote:
> 
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> > 
> > > +...
> > > +	d="$(git -C shallow-server rev-parse --verify D)" &&
> > > +	git -C shallow-server checkout master &&
> > > +
> > > +...
> > > +	! grep $d shallow-client/.git/shallow &&
> > 
> > We know D (and $d) is not a tag,
> 
> Actually, it is... the `test_commit D` creates that tag, and that is what
> I use here.
> 
> > but perhaps the place that assigns to $d (way above) can do the
> > rev-parse of D^0, not D, to make it more clear what is going on,
> > especially given that...
> 
> ... that the `grep` really wants to test for the absence of the *commit*,
> not the *tag* in .git/shallow?
> 
> Yes, you are right ;-)
> 
> So why did my test do the right thing, if it looked at a tag, but did not
> dereference it via ^0? The answer is: the `test_commit` function creates
> light-weight tags, i.e. no tag objects. And therefore, the $d^0 you found
> below, that's just confusing.

What I was referring to was the call

	test_must_fail git -C shallow-client rev-parse --verify $d^0

However, here we *have* to append ^0, otherwise `rev-parse --verify` will
(and quite confusingly so) *succeed*. I *repeatedly* fall into that trap
that `git rev-parse --verify 0000000000000000000000000000000000000000`
will succeed. Why? Because that is a valid 40-digit hex string. Not
because the object exists. Because it does not.

So I managed to confuse myself again into believing that I only need to
append ^0 to the earlier rev-parse call, but can remove it from this one,
when in reality, I have to append it to both. In the first case, to avoid
having to think about dereferencing a tag, in the second case, to force
rev-parse to look for the object.

Ciao,
Dscho

> 
> I will change it so that the `rev-parse` call uses ^0 (even if it is
> technically not necessary), to avoid said confusion.
> 
> Thanks,
> Dscho
> 
> > 
> > > +	git -C shallow-server branch branch-orig D^0 &&
> > 
> > ... this does that D^0 thing here to makes us wonder if D needs
> > unwrapping before using it as a commit (not commit-ish). 
> > 
> > If we did so, we could use $d here instead of D^0.
> > 
> > > +	git -C shallow-client fetch --prune --depth=2 \
> > > +		origin "+refs/heads/*:refs/remotes/origin/*"
> > > +'
> > > +
> > >  . "$TEST_DIRECTORY"/lib-httpd.sh
> > >  start_httpd
> > 
> > 
>

Patch
diff mbox series

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 7045685e2..2d0031703 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,6 +186,33 @@  EOF
 	test_cmp expect actual
 '
 
+test_expect_failure '.git/shallow is edited by repack' '
+	git init shallow-server &&
+	test_commit -C shallow-server A &&
+	test_commit -C shallow-server B &&
+	git -C shallow-server checkout -b branch &&
+	test_commit -C shallow-server C &&
+	test_commit -C shallow-server E &&
+	test_commit -C shallow-server D &&
+	d="$(git -C shallow-server rev-parse --verify D)" &&
+	git -C shallow-server checkout master &&
+
+	git clone --depth=1 --no-tags --no-single-branch \
+		"file://$PWD/shallow-server" shallow-client &&
+
+	: now remove the branch and fetch with prune &&
+	git -C shallow-server branch -D branch &&
+	git -C shallow-client fetch --prune --depth=1 \
+		origin "+refs/heads/*:refs/remotes/origin/*" &&
+	git -C shallow-client repack -adfl &&
+	test_must_fail git -C shallow-client rev-parse --verify $d^0 &&
+	! grep $d shallow-client/.git/shallow &&
+
+	git -C shallow-server branch branch-orig D^0 &&
+	git -C shallow-client fetch --prune --depth=2 \
+		origin "+refs/heads/*:refs/remotes/origin/*"
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd