[PATCH 1/2] repack: point out a bug handling stale shallow info

[PATCH 1/2] repack: point out a bug handling stale shallow info

[Johannes Schindelin via GitGitGadget]

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(+)

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 943231af9..561485d31 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,4 +186,31 @@ 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_done
-- 
gitgitgadget

[PATCH 2/2] repack -ad: prune the list of shallow commits

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

While it is true that we never add unreachable commits into pack files
intentionally (as `git repack`'s documentation states), we must not
forget that a `git fetch --prune` (or even a `git fetch` when a ref was
force-pushed in the meantime) can make a commit unreachable that was
reachable before.

Therefore it is not safe to assume that a `git repack -adlf` will keep
unreachable commits alone (under the assumption that they had not been
packed in the first place).

This is particularly important to keep in mind when looking at the
`.git/shallow` file: if any commits listed in that file become
unreachable, it is not a problem, but if they go missing, it *is* a
problem. One symptom of this problem is that a deepening fetch may now
fail with

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

To avoid this problem, let's prune the shallow list in `git repack` when
the `-d` option is passed, unless `-A` is passed, too (which would force
the now-unreachable objects to be turned into loose objects instead of
being deleted).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/repack.c         | 4 ++++
 t/t5537-fetch-shallow.sh | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 6c636e159..45f321b23 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -444,6 +444,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		if (!quiet && isatty(2))
 			opts |= PRUNE_PACKED_VERBOSE;
 		prune_packed_objects(opts);
+
+		if (!(pack_everything & LOOSEN_UNREACHABLE) &&
+		    is_repository_shallow())
+			prune_shallow(0);
 	}
 
 	if (!no_update_server_info)
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 561485d31..d32ba20f9 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,7 +186,7 @@ EOF
 	test_cmp expect actual
 '
 
-test_expect_failure '.git/shallow is edited by repack' '
+test_expect_success '.git/shallow is edited by repack' '
 	git init shallow-server &&
 	test_commit -C shallow-server A &&
 	test_commit -C shallow-server B &&
-- 
gitgitgadget

[PATCH 0/2] repack -ad: fix after `fetch --prune` in a shallow repository

[Johannes Schindelin via GitGitGadget]

Under certain circumstances, commits that were reachable can be made unreachable, e.g. via `git fetch --prune`. These commits might have been packed already, in which case `git repack -adlf` will just drop them without giving them the usual grace period before an eventual `git prune` (via `git gc`) prunes them.

This is a problem when the `shallow` file still lists them, which is the reason why `git prune` edits that file. And with the proposed changes, `git repack -ad` will now do the same.

Reported by Alejandro Pauly.

Johannes Schindelin (2):
  repack: point out a bug handling stale shallow info
  repack -ad: prune the list of shallow commits

 builtin/repack.c         |  4 ++++
 t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)


base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/9
-- 
gitgitgadget

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Jeff King]

On Thu, Jul 12, 2018 at 12:23:28AM +0200, Johannes Schindelin via GitGitGadget wrote:

> This is particularly important to keep in mind when looking at the
> `.git/shallow` file: if any commits listed in that file become
> unreachable, it is not a problem, but if they go missing, it *is* a
> problem. One symptom of this problem is that a deepening fetch may now
> fail with
> 
> 	fatal: error in object: unshallow <commit-hash>
> 
> To avoid this problem, let's prune the shallow list in `git repack` when
> the `-d` option is passed, unless `-A` is passed, too (which would force
> the now-unreachable objects to be turned into loose objects instead of
> being deleted).

I'm not sure if this covers all cases:

 - even with "-A", we may still drop objects subject to
   --unpack-unreachable. So if your pack has an old mtime (e.g., because
   you haven't packed in a while) I think you'd see the same problem.

 - if you use "-adk", we'd keep all objects, and this pruning would not
   be necessary

> diff --git a/builtin/repack.c b/builtin/repack.c
> index 6c636e159..45f321b23 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -444,6 +444,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		if (!quiet && isatty(2))
>  			opts |= PRUNE_PACKED_VERBOSE;
>  		prune_packed_objects(opts);
> +
> +		if (!(pack_everything & LOOSEN_UNREACHABLE) &&
> +		    is_repository_shallow())
> +			prune_shallow(0);
>  	}

I understand how this solves your immediate problem, but it feels like a
weird layering violation (which I think is a result of existing layering
violations ;) ).

I.e., it seems unexpected that "git repack" is going to tweak your
shallow lists. If we were designing from scratch, the sane behavior
seems to me to be:

  1. Shallow pruning should be its own separate command (distinct from
     either repacking or loose object pruning), and should be triggered
     as part of a normal git-gc.

  AND ONE OF:

  2a. Objects mentions in the shallow file are important, and therefore
      _are_ considered reachable on their own. Neither repack nor prune
      needs to know or care.

  OR

  2b. It's OK for shallow objects to be missing, and the shallow code
      should be more resilient to missing objects. Neither repack nor
      prune needs to know or care.

I don't know how hard it would be to get there from the current code
state, though.

-Peff

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Johannes Schindelin]

Hi Peff,

On Fri, 13 Jul 2018, Jeff King wrote:

> On Thu, Jul 12, 2018 at 12:23:28AM +0200, Johannes Schindelin via
> GitGitGadget wrote:
> 
> > This is particularly important to keep in mind when looking at the
> > `.git/shallow` file: if any commits listed in that file become
> > unreachable, it is not a problem, but if they go missing, it *is* a
> > problem. One symptom of this problem is that a deepening fetch may now
> > fail with
> > 
> > 	fatal: error in object: unshallow <commit-hash>
> > 
> > To avoid this problem, let's prune the shallow list in `git repack` when
> > the `-d` option is passed, unless `-A` is passed, too (which would force
> > the now-unreachable objects to be turned into loose objects instead of
> > being deleted).
> 
> I'm not sure if this covers all cases:
> 
>  - even with "-A", we may still drop objects subject to
>    --unpack-unreachable. So if your pack has an old mtime (e.g., because
>    you haven't packed in a while) I think you'd see the same problem.
> 
>  - if you use "-adk", we'd keep all objects, and this pruning would not
>    be necessary

Sure. I can add those cases.

> > diff --git a/builtin/repack.c b/builtin/repack.c
> > index 6c636e159..45f321b23 100644
> > --- a/builtin/repack.c
> > +++ b/builtin/repack.c
> > @@ -444,6 +444,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> >  		if (!quiet && isatty(2))
> >  			opts |= PRUNE_PACKED_VERBOSE;
> >  		prune_packed_objects(opts);
> > +
> > +		if (!(pack_everything & LOOSEN_UNREACHABLE) &&
> > +		    is_repository_shallow())
> > +			prune_shallow(0);
> >  	}
> 
> I understand how this solves your immediate problem, but it feels like a
> weird layering violation (which I think is a result of existing layering
> violations ;) ).

Okay, but please don't punish me for those existing layering violations by
forcing me to address them, instead of a quick bug fix for a very real bug
that was reported to me privately, and that I would like to see fixed
relatively quickly.

> I.e., it seems unexpected that "git repack" is going to tweak your
> shallow lists. If we were designing from scratch, the sane behavior
> seems to me to be:
> 
>   1. Shallow pruning should be its own separate command (distinct from
>      either repacking or loose object pruning), and should be triggered
>      as part of a normal git-gc.

I fail to see the value in a separate command.

And: `git gc` already calls `git prune`, which *does* prune the shallow
list.

>   AND ONE OF:
> 
>   2a. Objects mentions in the shallow file are important, and therefore
>       _are_ considered reachable on their own. Neither repack nor prune
>       needs to know or care.

If we were to do that, we would never be able to gc any stale shallow
commits.

That would be rather bad a design, don't you agree?

>   OR
> 
>   2b. It's OK for shallow objects to be missing, and the shallow code
>       should be more resilient to missing objects. Neither repack nor
>       prune needs to know or care.

That would be possible. Kind of like saying: we do have this list of
shallow commits, but oh, BTW, it is likely incorrect, so we painstakingly
verify for every entry during every fetch and push that those commits
objects are still there.

It looks to me like a rather bad design, too, as the whole idea of having
a `git gc` is to update such information *then*.

Sadly, we also allow `git repack` to drop objects, and it is really the
responsibility of a command that drops objects to update things like the
`shallow` list. Because that is exactly the time when its contents can go
stale.

Ciao,
Dscho

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Jeff King]

On Sat, Jul 14, 2018 at 11:56:57PM +0200, Johannes Schindelin wrote:

> > > To avoid this problem, let's prune the shallow list in `git repack` when
> > > the `-d` option is passed, unless `-A` is passed, too (which would force
> > > the now-unreachable objects to be turned into loose objects instead of
> > > being deleted).
> > 
> > I'm not sure if this covers all cases:
> > 
> >  - even with "-A", we may still drop objects subject to
> >    --unpack-unreachable. So if your pack has an old mtime (e.g., because
> >    you haven't packed in a while) I think you'd see the same problem.
> > 
> >  - if you use "-adk", we'd keep all objects, and this pruning would not
> >    be necessary
> 
> Sure. I can add those cases.

Thanks.

> > I understand how this solves your immediate problem, but it feels like a
> > weird layering violation (which I think is a result of existing layering
> > violations ;) ).
> 
> Okay, but please don't punish me for those existing layering violations by
> forcing me to address them, instead of a quick bug fix for a very real bug
> that was reported to me privately, and that I would like to see fixed
> relatively quickly.

I'm OK with having a partial fix, or one that fixes immediate pain
without doing a big cleanup, as long as it doesn't make anything _worse_
in a user-visible way. And that's really my question: is pruning here
going to bite people unexpectedly (not rhetorical -- I really don't
know)?

For instance, at GitHub we do not ever run "git gc", but have our
maintenance regimen that builds around "git repack". I don't think this
patch will matter to us either way, because we would never have a
shallow repository in the first place. But I'm wondering if people in a
similar situation might.

I'm also not entirely sure if people _care_ if their shallow list is
pruned. Maybe it's not a big deal, and should just be quietly cleaned
up.

And I know that you're probably coming at it from the opposite angle.
Somebody isn't running git-gc but doing a "repack -adl" and they _do_
want these shallow files cleaned up (because their repo is broken after
dropping the objects). I just want to make sure we don't regress some
other case.

> > I.e., it seems unexpected that "git repack" is going to tweak your
> > shallow lists. If we were designing from scratch, the sane behavior
> > seems to me to be:
> > 
> >   1. Shallow pruning should be its own separate command (distinct from
> >      either repacking or loose object pruning), and should be triggered
> >      as part of a normal git-gc.
> 
> I fail to see the value in a separate command.

The value of a separate command is that you can run those other commands
without triggering this behavior. I actually think "git prune" does too
much already (e.g., imagine that I do not ever want to prune objects,
but I _do_ want to get rid of tmp_pack and tmp_obj cruft; what command
do I run?). But that is definitely not a new problem. And I'm OK with
not fixing it for now. My main concern is that we are using the presence
of that mistake to justify making it again.

> And: `git gc` already calls `git prune`, which *does* prune the shallow
> list.

Right, I was trying to propose that each individual cleanup step which
_could_ be done independently should be done so, but that "gc" should
continue to do them all.

I think in the case of git-prune and prune_shallow(), it's a little
tricky, though. It looks like prune_shallow() relies on somebody having
marked the SEEN flag, which implies doing a full reachability walk. So
it's really amortizing the work already being done by prune.

Speaking of which, I don't see how your patch can work as-is. The repack
command does not do such a walk, so it has no idea which commits are
SEEN or not, and will delete all of them! Try this:

  [shallow of clone of git.git (or any repo)]
  $ git clone --no-local --depth=1 /path/to/git tmp
  ...
  $ cd tmp

  [we have a commit]
  $ git log --oneline -1
  de46fca (grafted, HEAD) repack -ad: prune the list of shallow commits

  [repacking with existing git is fine]
  $ git repack -adl
  ...
  $ git log --oneline -1
  de46fca (grafted, HEAD) repack -ad: prune the list of shallow commits

  [repacking with your patch empties the shallow file, which
   makes the repository look corrupt]
  $ /path/to/git repack -adl
  $ git log --oneline -1
  error: Could not read a2df50675979af639cf9490f7fc9b86fa18fd907
  fatal: Failed to traverse parents of commit de46fca5b43f47f3c5c6f9232a17490d39fc80b1

So either we have to do a reachability walk (which is expensive), or we
have to rely on some other command (like prune) that is doing a
reachability walk and reuse that work.

That also probably means my "separate command" suggestion is a bad one.
If we're trying to amortize work, then we're better to have a single
command with options to enable/disable certain operations (so in theory
I could use "git prune --no-prune-objects" to just clean up tempfiles,
and users of "repack -adl" could do something similar to _just_ prune
the shallow if they really didn't want to delete loose objects.

> >   AND ONE OF:
> > 
> >   2a. Objects mentions in the shallow file are important, and therefore
> >       _are_ considered reachable on their own. Neither repack nor prune
> >       needs to know or care.
> 
> If we were to do that, we would never be able to gc any stale shallow
> commits.
> 
> That would be rather bad a design, don't you agree?

I had meant for this to be coupled with my (1), which would presumably
remove unreachable commits from the shallow list in a separate step
(based on what criteria, I don't know -- again, I'm not really a shallow
user).

> >   2b. It's OK for shallow objects to be missing, and the shallow code
> >       should be more resilient to missing objects. Neither repack nor
> >       prune needs to know or care.
> 
> That would be possible. Kind of like saying: we do have this list of
> shallow commits, but oh, BTW, it is likely incorrect, so we painstakingly
> verify for every entry during every fetch and push that those commits
> objects are still there.

Obviously verifying reachability on each fetch is a bad idea. But my
understanding of the shallow list is that it says "this is a graft point
where we do not have any of the parents". If we find that we do not have
such an object, would it be OK to quietly ignore that? We used to claim
"we do not have the parents of X", but now we know "well, we do not have
X anymore either".

Again, I may be showing my ignorance of the shallow code here.

> It looks to me like a rather bad design, too, as the whole idea of having
> a `git gc` is to update such information *then*.

Right, I think "git gc" is absolutely the place to do such cleanups. My
only complaint was that having it as part of repack may be unexpected.

-Peff

[PATCH v2 0/2] repack -ad: fix after `fetch --prune` in a shallow repository

[Johannes Schindelin via GitGitGadget]

Under certain circumstances, commits that were reachable can be made unreachable, e.g. via `git fetch --prune`. These commits might have been packed already, in which case `git repack -adlf` will just drop them without giving them the usual grace period before an eventual `git prune` (via `git gc`) prunes them.

This is a problem when the `shallow` file still lists them, which is the reason why `git prune` edits that file. And with the proposed changes, `git repack -ad` will now do the same.

Reported by Alejandro Pauly.

Changes since v1:

- Also trigger `prune_shallow()` when `--unpack-unreachable=<approxidate>` was passed to `git repack`.
- No need to trigger `prune_shallow()` when `git repack` was called with `-k`.

Johannes Schindelin (2):
  repack: point out a bug handling stale shallow info
  repack -ad: prune the list of shallow commits

 builtin/repack.c         |  6 ++++++
 t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)


base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/9

Range-diff vs v1:

 1:  d2be40131 = 1:  d2be40131 repack: point out a bug handling stale shallow info
 2:  b4e01a963 ! 2:  c7ee6e008 repack -ad: prune the list of shallow commits
     @@ -23,7 +23,8 @@
          To avoid this problem, let's prune the shallow list in `git repack` when
          the `-d` option is passed, unless `-A` is passed, too (which would force
          the now-unreachable objects to be turned into loose objects instead of
     -    being deleted).
     +    being deleted). Additionally, e also need to take `--keep-reachable` and
     +    `--unpack-unreachable=<date>` into account.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ -35,7 +36,9 @@
       			opts |= PRUNE_PACKED_VERBOSE;
       		prune_packed_objects(opts);
      +
     -+		if (!(pack_everything & LOOSEN_UNREACHABLE) &&
     ++		if (!keep_unreachable &&
     ++		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
     ++		     unpack_unreachable) &&
      +		    is_repository_shallow())
      +			prune_shallow(0);
       	}

-- 
gitgitgadget

[PATCH v2 1/2] repack: point out a bug handling stale shallow info

[Johannes Schindelin via GitGitGadget]

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(+)

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 943231af9..561485d31 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,4 +186,31 @@ 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_done
-- 
gitgitgadget

[PATCH v2 2/2] repack -ad: prune the list of shallow commits

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

While it is true that we never add unreachable commits into pack files
intentionally (as `git repack`'s documentation states), we must not
forget that a `git fetch --prune` (or even a `git fetch` when a ref was
force-pushed in the meantime) can make a commit unreachable that was
reachable before.

Therefore it is not safe to assume that a `git repack -adlf` will keep
unreachable commits alone (under the assumption that they had not been
packed in the first place).

This is particularly important to keep in mind when looking at the
`.git/shallow` file: if any commits listed in that file become
unreachable, it is not a problem, but if they go missing, it *is* a
problem. One symptom of this problem is that a deepening fetch may now
fail with

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

To avoid this problem, let's prune the shallow list in `git repack` when
the `-d` option is passed, unless `-A` is passed, too (which would force
the now-unreachable objects to be turned into loose objects instead of
being deleted). Additionally, e also need to take `--keep-reachable` and
`--unpack-unreachable=<date>` into account.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/repack.c         | 6 ++++++
 t/t5537-fetch-shallow.sh | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 6c636e159..4caf57221 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -444,6 +444,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		if (!quiet && isatty(2))
 			opts |= PRUNE_PACKED_VERBOSE;
 		prune_packed_objects(opts);
+
+		if (!keep_unreachable &&
+		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
+		     unpack_unreachable) &&
+		    is_repository_shallow())
+			prune_shallow(0);
 	}
 
 	if (!no_update_server_info)
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 561485d31..d32ba20f9 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,7 +186,7 @@ EOF
 	test_cmp expect actual
 '
 
-test_expect_failure '.git/shallow is edited by repack' '
+test_expect_success '.git/shallow is edited by repack' '
 	git init shallow-server &&
 	test_commit -C shallow-server A &&
 	test_commit -C shallow-server B &&
-- 
gitgitgadget

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm OK with having a partial fix, or one that fixes immediate pain
> without doing a big cleanup, as long as it doesn't make anything _worse_
> in a user-visible way. And that's really my question: is pruning here
> going to bite people unexpectedly (not rhetorical -- I really don't
> know)?

Yeah, that matches the general guideline I follow when reviewing a
patch that claims to make existing things better.  And I do not
think I can explain to a third person why pruning here is a good
idea and won't cause problems, after seeing these patches and
the discussion from the sideline.

> Right, I think "git gc" is absolutely the place to do such cleanups. My
> only complaint was that having it as part of repack may be unexpected.

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Duy Nguyen]

On Fri, Jul 13, 2018 at 10:19 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> While it is true that we never add unreachable commits into pack files
> intentionally (as `git repack`'s documentation states), we must not
> forget that a `git fetch --prune` (or even a `git fetch` when a ref was
> force-pushed in the meantime) can make a commit unreachable that was
> reachable before.
>
> Therefore it is not safe to assume that a `git repack -adlf` will keep
> unreachable commits alone (under the assumption that they had not been
> packed in the first place).
>
> This is particularly important to keep in mind when looking at the
> `.git/shallow` file: if any commits listed in that file become
> unreachable, it is not a problem, but if they go missing, it *is* a
> problem. One symptom of this problem is that a deepening fetch may now
> fail with
>
>         fatal: error in object: unshallow <commit-hash>
>

Could you elaborate a bit more? I don't quite see the connection
between the reachability in the first two paragraphs and .git/shallow
in the third one. I'm guessing we drop objects in between because
"they go missing"? Where? How does prune_shallow() in prune.c play any
role in this (if it does)?
-- 
Duy

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Duy Nguyen]

On Tue, Jul 17, 2018 at 6:39 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Fri, Jul 13, 2018 at 10:19 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > While it is true that we never add unreachable commits into pack files
> > intentionally (as `git repack`'s documentation states), we must not
> > forget that a `git fetch --prune` (or even a `git fetch` when a ref was
> > force-pushed in the meantime) can make a commit unreachable that was
> > reachable before.
> >
> > Therefore it is not safe to assume that a `git repack -adlf` will keep
> > unreachable commits alone (under the assumption that they had not been
> > packed in the first place).
> >
> > This is particularly important to keep in mind when looking at the
> > `.git/shallow` file: if any commits listed in that file become
> > unreachable, it is not a problem, but if they go missing, it *is* a
> > problem. One symptom of this problem is that a deepening fetch may now
> > fail with
> >
> >         fatal: error in object: unshallow <commit-hash>
> >
>
> Could you elaborate a bit more?

Never mind. The problem is described in another patch.. sigh.. I
understand you want to flip that "failure" to "success" but personally
I don't think it worth it to look back in history and have "what?"
moments like when I read this patch alone.
-- 
Duy

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Duy Nguyen]

On Mon, Jul 16, 2018 at 7:38 PM Jeff King <peff@peff.net> wrote:
> in a user-visible way. And that's really my question: is pruning here
> going to bite people unexpectedly (not rhetorical -- I really don't
> know)?

If shallow points are no longer reachable, removing them should not
bite anybody, I think.

> For instance, at GitHub we do not ever run "git gc", but have our
> maintenance regimen that builds around "git repack". I don't think this
> patch will matter to us either way, because we would never have a
> shallow repository in the first place. But I'm wondering if people in a
> similar situation might.
>
> I'm also not entirely sure if people _care_ if their shallow list is
> pruned. Maybe it's not a big deal, and should just be quietly cleaned
> up.
>
> And I know that you're probably coming at it from the opposite angle.
> Somebody isn't running git-gc but doing a "repack -adl" and they _do_
> want these shallow files cleaned up (because their repo is broken after
> dropping the objects). I just want to make sure we don't regress some
> other case.
>
> > > I.e., it seems unexpected that "git repack" is going to tweak your
> > > shallow lists. If we were designing from scratch, the sane behavior
> > > seems to me to be:
> > >
> > >   1. Shallow pruning should be its own separate command (distinct from
> > >      either repacking or loose object pruning), and should be triggered
> > >      as part of a normal git-gc.
> >
> > I fail to see the value in a separate command.
>
> The value of a separate command is that you can run those other commands
> without triggering this behavior. I actually think "git prune" does too
> much already (e.g., imagine that I do not ever want to prune objects,
> but I _do_ want to get rid of tmp_pack and tmp_obj cruft; what command
> do I run?). But that is definitely not a new problem. And I'm OK with
> not fixing it for now. My main concern is that we are using the presence
> of that mistake to justify making it again.
>
> > And: `git gc` already calls `git prune`, which *does* prune the shallow
> > list.
>
> Right, I was trying to propose that each individual cleanup step which
> _could_ be done independently should be done so, but that "gc" should
> continue to do them all.
>
> I think in the case of git-prune and prune_shallow(), it's a little
> tricky, though. It looks like prune_shallow() relies on somebody having
> marked the SEEN flag, which implies doing a full reachability walk.

Sorry, my bad for hiding this SEEN flag deep in library code like this
in eab3296c7e (prune: clean .git/shallow after pruning objects -
2013-12-05) But in my defense I didn't realize the horror of sharing
object flags a year later and added the "object flag allocation" in
object.h

> So it's really amortizing the work already being done by prune.
>
> Speaking of which, I don't see how your patch can work as-is. The repack
> command does not do such a walk, so it has no idea which commits are
> SEEN or not, and will delete all of them! Try this:
>
>   [shallow of clone of git.git (or any repo)]
>   $ git clone --no-local --depth=1 /path/to/git tmp
>   ...
>   $ cd tmp
>
>   [we have a commit]
>   $ git log --oneline -1
>   de46fca (grafted, HEAD) repack -ad: prune the list of shallow commits
>
>   [repacking with existing git is fine]
>   $ git repack -adl
>   ...
>   $ git log --oneline -1
>   de46fca (grafted, HEAD) repack -ad: prune the list of shallow commits
>
>   [repacking with your patch empties the shallow file, which
>    makes the repository look corrupt]
>   $ /path/to/git repack -adl
>   $ git log --oneline -1
>   error: Could not read a2df50675979af639cf9490f7fc9b86fa18fd907
>   fatal: Failed to traverse parents of commit de46fca5b43f47f3c5c6f9232a17490d39fc80b1
>
> So either we have to do a reachability walk (which is expensive), or we
> have to rely on some other command (like prune) that is doing a
> reachability walk and reuse that work.

Since "git prune" took care of loose objects, if we're going to delete
some redundant packs, we can perform a reasonably cheap "reachability"
test in repack, I think. We have the list of new packs from
pack-objects which should contain all reachable objects (and even some
unreachable ones). If we traverse all of their idx files and mark as
SEEN, then whatever shallow points that are not marked SEEN _and_ not
loose objects could be safely removed.

> > >   2b. It's OK for shallow objects to be missing, and the shallow code
> > >       should be more resilient to missing objects. Neither repack nor
> > >       prune needs to know or care.
> >
> > That would be possible. Kind of like saying: we do have this list of
> > shallow commits, but oh, BTW, it is likely incorrect, so we painstakingly
> > verify for every entry during every fetch and push that those commits
> > objects are still there.
>
> Obviously verifying reachability on each fetch is a bad idea. But my
> understanding of the shallow list is that it says "this is a graft point
> where we do not have any of the parents". If we find that we do not have
> such an object, would it be OK to quietly ignore that? We used to claim
> "we do not have the parents of X", but now we know "well, we do not have
> X anymore either".
>
> Again, I may be showing my ignorance of the shallow code here.

I don't see any problem with this either, but then I've not worked on
shallow code for quite some time. The only potential problem is where
we do this check. If we check (and drop) shallow points when we read
.git/info/shallow, then when we write it down some time in the future
we accidentally prune them. Which might be ok but I feel safer when we
only prune at one place (prune/gc/repack) where we can be more careful
and can do more testing.

If we read the shallow list as-is, then just not send "shallow" lines
in fetch-pack for shallow points that do not exist, I don't see a
problem, but it may be a bit more work and we could get to some
confusing state where upload-pack gives us the same shallow point that
we have but ignore because we don't have objects behind it. Hm...
actually I might see a problem :)
-- 
Duy

Re: [PATCH v2 2/2] repack -ad: prune the list of shallow commits

[Eric Sunshine]

On Tue, Jul 17, 2018 at 9:51 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> While it is true that we never add unreachable commits into pack files
> intentionally (as `git repack`'s documentation states), we must not
> forget that a `git fetch --prune` (or even a `git fetch` when a ref was
> force-pushed in the meantime) can make a commit unreachable that was
> reachable before.
>
> Therefore it is not safe to assume that a `git repack -adlf` will keep
> unreachable commits alone (under the assumption that they had not been
> packed in the first place).
>
> This is particularly important to keep in mind when looking at the
> `.git/shallow` file: if any commits listed in that file become
> unreachable, it is not a problem, but if they go missing, it *is* a
> problem. One symptom of this problem is that a deepening fetch may now
> fail with
>
>         fatal: error in object: unshallow <commit-hash>
>
> To avoid this problem, let's prune the shallow list in `git repack` when
> the `-d` option is passed, unless `-A` is passed, too (which would force
> the now-unreachable objects to be turned into loose objects instead of
> being deleted). Additionally, e also need to take `--keep-reachable` and

s/, e/, we/

> `--unpack-unreachable=<date>` into account.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Re: [PATCH v2 0/2] repack -ad: fix after `fetch --prune` in a shallow repository

[Jeff King]

On Tue, Jul 17, 2018 at 06:51:39AM -0700, Johannes Schindelin via GitGitGadget wrote:

> Under certain circumstances, commits that were reachable can be made
> unreachable, e.g. via `git fetch --prune`. These commits might have
> been packed already, in which case `git repack -adlf` will just drop
> them without giving them the usual grace period before an eventual
> `git prune` (via `git gc`) prunes them.
> 
> This is a problem when the `shallow` file still lists them, which is
> the reason why `git prune` edits that file. And with the proposed
> changes, `git repack -ad` will now do the same.
> 
> Reported by Alejandro Pauly.
> 
> Changes since v1:
> 
> - Also trigger `prune_shallow()` when `--unpack-unreachable=<approxidate>` was passed to `git repack`.
> - No need to trigger `prune_shallow()` when `git repack` was called with `-k`.

I think you might have missed the bigger problem I pointed at, as it was
buried deep within my later reply. Try applying this patch on top, which
tests the opposite case (reachable shallow commits are retained):

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index d32ba20f9d..911e457ae1 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,17 +186,20 @@ EOF
 	test_cmp expect actual
 '
 
-test_expect_success '.git/shallow is edited by repack' '
+test_expect_success 'set up shallow server' '
 	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 -C shallow-server checkout master
+'
 
+test_expect_success 'repack drops unreachable objects from .git/shallow' '
+	test_when_finished "rm -rf shallow-client" &&
 	git clone --depth=1 --no-tags --no-single-branch \
 		"file://$PWD/shallow-server" shallow-client &&
 
@@ -213,4 +216,13 @@ test_expect_success '.git/shallow is edited by repack' '
 		origin "+refs/heads/*:refs/remotes/origin/*"
 '
 
+test_expect_success 'repack keeps reachable objects in .git/shallow' '
+	test_when_finished "rm -rf shallow-client" &&
+	git clone --depth=1 --no-tags --no-single-branch \
+		"file://$PWD/shallow-server" shallow-client &&
+
+	git -C shallow-client repack -adfl &&
+	grep $d shallow-client/.git/shallow
+'
+
 test_done

Re: [PATCH v2 0/2] repack -ad: fix after `fetch --prune` in a shallow repository

[Jeff King]

On Tue, Jul 17, 2018 at 03:15:31PM -0400, Jeff King wrote:

> > - Also trigger `prune_shallow()` when `--unpack-unreachable=<approxidate>` was passed to `git repack`.
> > - No need to trigger `prune_shallow()` when `git repack` was called with `-k`.
> 
> I think you might have missed the bigger problem I pointed at, as it was
> buried deep within my later reply. Try applying this patch on top, which
> tests the opposite case (reachable shallow commits are retained):

By the way, I notice that the patches themselves are cc'd to you, but
the cover letter isn't. So my reply went only to gitgitgadget, which
(AFAIK) has not yet learned to work as a mail-to-comment gateway.

So I'm copying this to you directly to make sure you see it, but also
because I'm not sure if the gitgitgadget cc behavior is intentional or
not.

-Peff

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Jeff King]

On Tue, Jul 17, 2018 at 07:28:07PM +0200, Duy Nguyen wrote:

> On Mon, Jul 16, 2018 at 7:38 PM Jeff King <peff@peff.net> wrote:
> > in a user-visible way. And that's really my question: is pruning here
> > going to bite people unexpectedly (not rhetorical -- I really don't
> > know)?
> 
> If shallow points are no longer reachable, removing them should not
> bite anybody, I think.

I slept on this to see if I could brainstorm any other ways.

The only thing I really came up with is that removing shallows is racy
with respect to the reachability check.  For instance, if "prune" runs
at the same as an incoming shallow fetch, we'll compute the reachability
without holding the shallow lock. Somebody else may write an entry for
an object we've never heard of (because it just showed up), and we'd
erase it, making the repository appear corrupt.

But note that I used "prune" in the above example, because this bug
already exists. So probably we're not changing anything material here.
Though if we wanted to fix it, I think it would require holding the
shallow lock during the reachability analysis, which is probably not
something that repack would want to do. Unlike prune, it then does
a lot of other expensive operations, like delta compression and writing
out the pack, before it would get to the shallow prune.

Although even holding the lock for the duration of "prune" is more than
we need. Shallow commits must be commits, so we don't need to do a full
graph walk (and in my experience there's often an order-of-magnitude
difference between the two; even more so if we're using Stolee's
commit-graph cache).

> > I think in the case of git-prune and prune_shallow(), it's a little
> > tricky, though. It looks like prune_shallow() relies on somebody having
> > marked the SEEN flag, which implies doing a full reachability walk.
> 
> Sorry, my bad for hiding this SEEN flag deep in library code like this
> in eab3296c7e (prune: clean .git/shallow after pruning objects -
> 2013-12-05) But in my defense I didn't realize the horror of sharing
> object flags a year later and added the "object flag allocation" in
> object.h

Sort of an aside to the patch under discussion, but I think it may make
sense for prune_shallow() to take a callback function for determining
whether a commit is reachable.

I have an old patch that teaches git-prune to lazily do the reachability
check, since in many cases "git repack" will have just packed all of the
loose objects. But it just occurred to me that this patch is totally
broken with respect to prune_shallow(), because it would not set the
SEEN flag (I've literally been running with it for years, which goes to
show how often I use the shallow feature).

And if we were to have repack do a prune_shallow(), it may want to use a
different method than traversing and marking each object SEEN.

> > So either we have to do a reachability walk (which is expensive), or we
> > have to rely on some other command (like prune) that is doing a
> > reachability walk and reuse that work.
> 
> Since "git prune" took care of loose objects, if we're going to delete
> some redundant packs, we can perform a reasonably cheap "reachability"
> test in repack, I think. We have the list of new packs from
> pack-objects which should contain all reachable objects (and even some
> unreachable ones). If we traverse all of their idx files and mark as
> SEEN, then whatever shallow points that are not marked SEEN _and_ not
> loose objects could be safely removed.

Hmm. I don't immediately see any reason that would not work with the
current code. But I am a little uncomfortable adding these sorts of
subtle dependencies and assumptions. It feels like we're building a
house of cards.

> I don't see any problem with this either, but then I've not worked on
> shallow code for quite some time. The only potential problem is where
> we do this check. If we check (and drop) shallow points when we read
> .git/info/shallow, then when we write it down some time in the future
> we accidentally prune them. Which might be ok but I feel safer when we
> only prune at one place (prune/gc/repack) where we can be more careful
> and can do more testing.
> 
> If we read the shallow list as-is, then just not send "shallow" lines
> in fetch-pack for shallow points that do not exist, I don't see a
> problem, but it may be a bit more work and we could get to some
> confusing state where upload-pack gives us the same shallow point that
> we have but ignore because we don't have objects behind it. Hm...
> actually I might see a problem :)

Yeah, I agree if we were to do this it would definitely be in a
"read-only" way: let the current command skip those grafts for its
operation, but do not impact the on-disk shallow file. Then races or
other assumptions can at worst impact that operation, and not cause a
lasting corruption (and in particular I think we'd be subject to the
race I described at the start of this mail).

-Peff

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Duy Nguyen]

On Tue, Jul 17, 2018 at 9:41 PM Jeff King <peff@peff.net> wrote:
> I slept on this to see if I could brainstorm any other ways.
>
> ..
>
> Sort of an aside to the patch under discussion, but I think it may make
> sense for prune_shallow() to take a callback function for determining
> whether a commit is reachable.
>
> I have an old patch that teaches git-prune to lazily do the reachability
> check, since in many cases "git repack" will have just packed all of the
> loose objects. But it just occurred to me that this patch is totally
> broken with respect to prune_shallow(), because it would not set the
> SEEN flag (I've literally been running with it for years, which goes to
> show how often I use the shallow feature).
>
> And if we were to have repack do a prune_shallow(), it may want to use a
> different method than traversing and marking each object SEEN.

All of this sounds good. The only thing I'd like to do a bit
differently is to pass the reachable commits in prune_shallow() as a
commit-slab instead of taking a callback function. I'll refactor this
code, move prune_shallow() to a separate command prune-shallow and do
the locking thing.

Johannes I could take over this bug too (since I introduced the
shallow pruning and missed this part) if you are busy, but if you want
it to be done fast, I can't help. I still have a list of things to go
through.
-- 
Duy

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Jeff King]

On Wed, Jul 18, 2018 at 07:31:40PM +0200, Duy Nguyen wrote:

> > Sort of an aside to the patch under discussion, but I think it may make
> > sense for prune_shallow() to take a callback function for determining
> > whether a commit is reachable.
> >
> > I have an old patch that teaches git-prune to lazily do the reachability
> > check, since in many cases "git repack" will have just packed all of the
> > loose objects. But it just occurred to me that this patch is totally
> > broken with respect to prune_shallow(), because it would not set the
> > SEEN flag (I've literally been running with it for years, which goes to
> > show how often I use the shallow feature).
> >
> > And if we were to have repack do a prune_shallow(), it may want to use a
> > different method than traversing and marking each object SEEN.
> 
> All of this sounds good. The only thing I'd like to do a bit
> differently is to pass the reachable commits in prune_shallow() as a
> commit-slab instead of taking a callback function. I'll refactor this
> code, move prune_shallow() to a separate command prune-shallow and do
> the locking thing.

I think using a slab is much nicer than the current global-struct flags.
But it's not as flexible as a callback, for two reasons:

  - in the lazy case, the caller might not even have loaded the slab
    yet. On the other hand, it might be sufficient to just be broad
    there, and just always pre-populate the slab when
    is_repository_shallow(), before we even call into prune_shallow().
    If we have _any_ entries in the shallow file, we'd need to compute
    reachability.

  - it precludes any optimizations that compute partial reachability.
    Asking "is XYZ reachable" is a much easier question to answer than
    "show me the full reachability graph." At the least, it lets you
    stop the graph traversal early. And with generation numbers, it can
    even avoid traversing down unproductive segments of the graph.

I think it's OK to switch to a slab for now if you don't want to do the
callback thing. But I think a callback is probably long-term the right
thing (and I actually think it may be _less_ work to implement, since
then prune would probably be OK sticking with the global struct flags).

-Peff

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Duy Nguyen]

On Wed, Jul 18, 2018 at 7:45 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jul 18, 2018 at 07:31:40PM +0200, Duy Nguyen wrote:
>
> > > Sort of an aside to the patch under discussion, but I think it may make
> > > sense for prune_shallow() to take a callback function for determining
> > > whether a commit is reachable.
> > >
> > > I have an old patch that teaches git-prune to lazily do the reachability
> > > check, since in many cases "git repack" will have just packed all of the
> > > loose objects. But it just occurred to me that this patch is totally
> > > broken with respect to prune_shallow(), because it would not set the
> > > SEEN flag (I've literally been running with it for years, which goes to
> > > show how often I use the shallow feature).
> > >
> > > And if we were to have repack do a prune_shallow(), it may want to use a
> > > different method than traversing and marking each object SEEN.
> >
> > All of this sounds good. The only thing I'd like to do a bit
> > differently is to pass the reachable commits in prune_shallow() as a
> > commit-slab instead of taking a callback function. I'll refactor this
> > code, move prune_shallow() to a separate command prune-shallow and do
> > the locking thing.
>
> I think using a slab is much nicer than the current global-struct flags.
> But it's not as flexible as a callback, for two reasons:
>
>   - in the lazy case, the caller might not even have loaded the slab
>     yet. On the other hand, it might be sufficient to just be broad
>     there, and just always pre-populate the slab when
>     is_repository_shallow(), before we even call into prune_shallow().
>     If we have _any_ entries in the shallow file, we'd need to compute
>     reachability.
>
>   - it precludes any optimizations that compute partial reachability.
>     Asking "is XYZ reachable" is a much easier question to answer than
>     "show me the full reachability graph." At the least, it lets you
>     stop the graph traversal early. And with generation numbers, it can
>     even avoid traversing down unproductive segments of the graph.

Both good points. Callback it is then.
-- 
Duy

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Johannes Schindelin]

Hi Junio,

On Tue, 17 Jul 2018, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I'm OK with having a partial fix, or one that fixes immediate pain
> > without doing a big cleanup, as long as it doesn't make anything _worse_
> > in a user-visible way. And that's really my question: is pruning here
> > going to bite people unexpectedly (not rhetorical -- I really don't
> > know)?
> 
> Yeah, that matches the general guideline I follow when reviewing a
> patch that claims to make existing things better.  And I do not
> think I can explain to a third person why pruning here is a good
> idea and won't cause problems, after seeing these patches and
> the discussion from the sideline.

It is very easy to explain: `git repack` can drop unreachable commits
without further warning, making the corresponding entries in
`.git/shallow` invalid, which causes serious problems when deepening the
branches.

The solution is easy: drop also the now-invalid entries in `.git/shallow`
after dropping unreachable commits unceremoniously.

While I am sympathetic to Peff's attempt to make this a bit more general,
I think he overthinks it, and we do not even need to complexify the code
for now.

Ciao,
Dscho

Re: [PATCH v2 0/2] repack -ad: fix after `fetch --prune` in a shallow repository

[Johannes Schindelin]

Hi Peff,

On Tue, 17 Jul 2018, Jeff King wrote:

> On Tue, Jul 17, 2018 at 03:15:31PM -0400, Jeff King wrote:
> 
> > > - Also trigger `prune_shallow()` when `--unpack-unreachable=<approxidate>` was passed to `git repack`.
> > > - No need to trigger `prune_shallow()` when `git repack` was called with `-k`.
> > 
> > I think you might have missed the bigger problem I pointed at, as it was
> > buried deep within my later reply. Try applying this patch on top, which
> > tests the opposite case (reachable shallow commits are retained):
> 
> By the way, I notice that the patches themselves are cc'd to you, but
> the cover letter isn't. So my reply went only to gitgitgadget, which
> (AFAIK) has not yet learned to work as a mail-to-comment gateway.
> 
> So I'm copying this to you directly to make sure you see it, but also
> because I'm not sure if the gitgitgadget cc behavior is intentional or
> not.

It is one of the existing TODOs to Cc: the person who hit "/submit". So I
am aware of it, and it is recorded in the TODO.md file.

But as the GitHub user might not be associated with a public email, this
will need an internal map that will be extended to whoever is in the list
of allowed users.

Ciao,
Dscho

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Johannes Schindelin]

Hi,

On Tue, 17 Jul 2018, Duy Nguyen wrote:

> On Tue, Jul 17, 2018 at 6:39 PM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Fri, Jul 13, 2018 at 10:19 PM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >
> > > While it is true that we never add unreachable commits into pack files
> > > intentionally (as `git repack`'s documentation states), we must not
> > > forget that a `git fetch --prune` (or even a `git fetch` when a ref was
> > > force-pushed in the meantime) can make a commit unreachable that was
> > > reachable before.
> > >
> > > Therefore it is not safe to assume that a `git repack -adlf` will keep
> > > unreachable commits alone (under the assumption that they had not been
> > > packed in the first place).
> > >
> > > This is particularly important to keep in mind when looking at the
> > > `.git/shallow` file: if any commits listed in that file become
> > > unreachable, it is not a problem, but if they go missing, it *is* a
> > > problem. One symptom of this problem is that a deepening fetch may now
> > > fail with
> > >
> > >         fatal: error in object: unshallow <commit-hash>
> > >
> >
> > Could you elaborate a bit more?
> 
> Never mind. The problem is described in another patch.. sigh.. I
> understand you want to flip that "failure" to "success" but personally
> I don't think it worth it to look back in history and have "what?"
> moments like when I read this patch alone.

The split was not meant for your benefit, but for the benefit of verifying
that I indeed fixed a problem.

I don't think it is a wise idea to squash them together.

Ciao,
Dscho

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 17 Jul 2018, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > I'm OK with having a partial fix, or one that fixes immediate pain
>> > without doing a big cleanup, as long as it doesn't make anything _worse_
>> > in a user-visible way. And that's really my question: is pruning here
>> > going to bite people unexpectedly (not rhetorical -- I really don't
>> > know)?
>> 
>> Yeah, that matches the general guideline I follow when reviewing a
>> patch that claims to make existing things better.  And I do not
>> think I can explain to a third person why pruning here is a good
>> idea and won't cause problems, after seeing these patches and
>> the discussion from the sideline.
>
> It is very easy to explain: `git repack` can drop unreachable commits
> without further warning, making the corresponding entries in
> `.git/shallow` invalid, which causes serious problems when deepening the
> branches.

That explains why you wrote the patch very clearly.

> The solution is easy: drop also the now-invalid entries in `.git/shallow`
> after dropping unreachable commits unceremoniously.

Sorry, but I do not think I can relay that as an explanation why it
won't cause problems to a third person.

The entries in shallow file says that history behind them may not
exist in the repository due to its shallowness but history after
them are supposed to be traversable (otherwise we have a repository
corruption).  It is true that an entry that itself no longer exists
in this repository should not be in shallow file, as the presence of
that entry breaks that promise the file is making---that commit
ought to exist and it is safe to traverse down to it, so keeping the
entry in the file is absolutely a wrong thing to do.

But that does not automatically mean that just simply removing it
makes the resulting repository good, does it?  Wouldn't the solution
for that corruption be to set a new entry to stop history traversal
before reaching that (now-missing) commit?  If your solution and
explanatoin were like that, then I can understand why it won't cause
problems, because the resulting repository would be shallower than
it originally was, as if you cloned with a smaller depth, but it is
not corrupt.

Perhaps your rationale is that by trading one shape of corrupt
repository (where a commit that does not even exist is in the
shallow file, breaking the early start-up sequence when it tries to
read the entries) with another shape of corrupt repsitory (where
shallow does not completely tell where to stop, and traversing the
history can eventually hit a missing commit because no entry in the
shallow file stops such a traversal), at least deepening fetch can
start (instead of dying while trying to see how shallow the
repository currently is) and that can be used to recover the corrupt
repository back into a usable state?  If that is the justification,
I can fully buy that, but that is not what I am hearing in your easy
to explain answer, so I am still puzzled.

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sorry, but I do not think I can relay that as an explanation why it
> won't cause problems to a third person.

OK, ignore this. I was being stupid.

> The entries in shallow file says that history behind them may not
> exist in the repository due to its shallowness but history after
> them are supposed to be traversable (otherwise we have a repository
> corruption).  It is true that an entry that itself no longer exists
> in this repository should not be in shallow file, as the presence of
> that entry breaks that promise the file is making---that commit
> ought to exist and it is safe to traverse down to it, so keeping the
> entry in the file is absolutely a wrong thing to do.
>
> But that does not automatically mean that just simply removing it
> makes the resulting repository good, does it?  Wouldn't the solution
> for that corruption be to set a new entry to stop history traversal
> before reaching that (now-missing) commit?

The above is overly pessimistic and worried about an impossible
situation, I would think.  The reason why a commit that used to be
in the shallow file is being pruned during a "repack" is because it
has become unreachable.  By definition, no future history traversal
that wants to enumerate reachable commits needs to be stopped from
finding that commits that are older than this commit being pruned
are missing by having this in the shallow list.  If there is a ref
or a reflog entry from which such a problematic traversal starts at,
we wouldn't be pruing this commit in the first place, because the
commit has not become unreachable yet.

So a repository does not become corrupt by pruning the commit *and*
removing it from the shallow file at the same time.

Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

[Jeff King]

On Fri, Jul 20, 2018 at 02:30:23AM -0700, Junio C Hamano wrote:

> > The entries in shallow file says that history behind them may not
> > exist in the repository due to its shallowness but history after
> > them are supposed to be traversable (otherwise we have a repository
> > corruption).  It is true that an entry that itself no longer exists
> > in this repository should not be in shallow file, as the presence of
> > that entry breaks that promise the file is making---that commit
> > ought to exist and it is safe to traverse down to it, so keeping the
> > entry in the file is absolutely a wrong thing to do.
> >
> > But that does not automatically mean that just simply removing it
> > makes the resulting repository good, does it?  Wouldn't the solution
> > for that corruption be to set a new entry to stop history traversal
> > before reaching that (now-missing) commit?
> 
> The above is overly pessimistic and worried about an impossible
> situation, I would think.  The reason why a commit that used to be
> in the shallow file is being pruned during a "repack" is because it
> has become unreachable.  By definition, no future history traversal
> that wants to enumerate reachable commits needs to be stopped from
> finding that commits that are older than this commit being pruned
> are missing by having this in the shallow list.  If there is a ref
> or a reflog entry from which such a problematic traversal starts at,
> we wouldn't be pruing this commit in the first place, because the
> commit has not become unreachable yet.
> 
> So a repository does not become corrupt by pruning the commit *and*
> removing it from the shallow file at the same time.

Right. I think a lot of this is rethinking how shallow pruning works,
too, which is not something Dscho is trying to change. The simplest
argument (which I think Dscho has made elsewhere, too) is: this is
necessary in the current shallow code when dropping objects. We do it
therefore from prune, but miss the case when git-repack is run itself
outside of git-gc.

I do still think the gc/prune architecture is a bit muddled, but at this
point in the discussion I feel OK saying that people running "git repack
-ad" would not be upset to have their shallows pruned.

But the patch is still not OK as-is because prune_shallow() requires the
SEEN flag on each reachable object struct, which we have not set in the
repack process (hence the failing test I posted earlier).  So we need a
solution for that, which may impact ideas about how the call works.
E.g., some possible solutions are:

 - teach pack-objects to optionally trigger the shallow prune based on
   its internal walk

 - have repack use the just-completed pack as a hint about reachability

 - introduce a mechanism to trigger the shallow prune based on a
   commit-only reachability check, and run that from repack (or from gc
   and document that it must be run if you are using repack as a manual
   gc replacement)

I'm not advocating any particular solution there, but just showing that
there's an array of them (and probably more that I didn't mention).

-Peff

[PATCH v3 0/3] repack -ad: fix after fetch --prune in a shallow repository

[Johannes Schindelin via GitGitGadget]

Under certain circumstances, commits that were reachable can be made
unreachable, e.g. via git fetch --prune. These commits might have been
packed already, in which case git repack -adlf will just drop them without
giving them the usual grace period before an eventual git prune (via git gc)
prunes them.

This is a problem when the shallow file still lists them, which is the
reason why git prune edits that file. And with the proposed changes, git
repack -ad will now do the same.

Reported by Alejandro Pauly.

Changes since v2:

 * Fixed a typo in the last commit message.
 * Added an explanation to the last commit message why we do not simply skip
   non-existing shallow commits at fetch time.
 * Introduced a new, "quick prune" mode where prune_shallow() does not try
   to drop unreachable commits, but only non-existing ones.
 * Rebased to current master because there were too many merge conflicts for
   my liking otherwise.

Changes since v1:

 * Also trigger prune_shallow() when --unpack-unreachable=<approxidate> was
   passed to git repack.
 * No need to trigger prune_shallow() when git repack was called with -k.

Johannes Schindelin (3):
  repack: point out a bug handling stale shallow info
  shallow: offer to prune only non-existing entries
  repack -ad: prune the list of shallow commits

 builtin/prune.c          |  2 +-
 builtin/repack.c         |  6 ++++++
 commit.h                 |  2 +-
 shallow.c                | 22 +++++++++++++++++-----
 t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++
 5 files changed, 52 insertions(+), 7 deletions(-)


base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/9

Range-diff vs v2:

 1:  d2be40131 ! 1:  ed8559b91 repack: point out a bug handling stale shallow info
     @@ -48,4 +48,6 @@
      +		origin "+refs/heads/*:refs/remotes/origin/*"
      +'
      +
     - test_done
     + . "$TEST_DIRECTORY"/lib-httpd.sh
     + start_httpd
     + 
 -:  --------- > 2:  f085eb4f7 shallow: offer to prune only non-existing entries
 2:  c7ee6e008 ! 3:  1f9ff57d5 repack -ad: prune the list of shallow commits
     @@ -2,15 +2,19 @@
      
          repack -ad: prune the list of shallow commits
      
     -    While it is true that we never add unreachable commits into pack files
     -    intentionally (as `git repack`'s documentation states), we must not
     -    forget that a `git fetch --prune` (or even a `git fetch` when a ref was
     +    `git repack` can drop unreachable commits without further warning,
     +    making the corresponding entries in `.git/shallow` invalid, which causes
     +    serious problems when deepening the branches.
     +
     +    One scenario where unreachable commits are dropped by `git repack` is
     +    when a `git fetch --prune` (or even a `git fetch` when a ref was
          force-pushed in the meantime) can make a commit unreachable that was
          reachable before.
      
          Therefore it is not safe to assume that a `git repack -adlf` will keep
          unreachable commits alone (under the assumption that they had not been
     -    packed in the first place).
     +    packed in the first place, which is an assumption at least some of Git's
     +    code seems to make).
      
          This is particularly important to keep in mind when looking at the
          `.git/shallow` file: if any commits listed in that file become
     @@ -23,8 +27,16 @@
          To avoid this problem, let's prune the shallow list in `git repack` when
          the `-d` option is passed, unless `-A` is passed, too (which would force
          the now-unreachable objects to be turned into loose objects instead of
     -    being deleted). Additionally, e also need to take `--keep-reachable` and
     -    `--unpack-unreachable=<date>` into account.
     +    being deleted). Additionally, we also need to take `--keep-reachable`
     +    and `--unpack-unreachable=<date>` into account.
     +
     +    Note: an alternative solution discussed during the review of this patch
     +    was to teach `git fetch` to simply ignore entries in .git/shallow if the
     +    corresponding commits do not exist locally. A quick test, however,
     +    revealed that the .git/shallow file is written during a shallow *clone*,
     +    in which case the commits do not exist, either, but the "shallow" line
     +    *does* need to be sent. Therefore, this approach would be a lot more
     +    finicky than the approach presented by the this patch.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ -32,15 +44,15 @@
      --- a/builtin/repack.c
      +++ b/builtin/repack.c
      @@
     - 		if (!quiet && isatty(2))
     + 		if (!po_args.quiet && isatty(2))
       			opts |= PRUNE_PACKED_VERBOSE;
       		prune_packed_objects(opts);
      +
      +		if (!keep_unreachable &&
      +		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
      +		     unpack_unreachable) &&
     -+		    is_repository_shallow())
     -+			prune_shallow(0);
     ++		    is_repository_shallow(the_repository))
     ++			prune_shallow(0, 1);
       	}
       
       	if (!no_update_server_info)

-- 
gitgitgadget

[PATCH v3 1/3] repack: point out a bug handling stale shallow info

[Johannes Schindelin via GitGitGadget]

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(+)

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
 
-- 
gitgitgadget

[PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The `prune_shallow()` function wants a full reachability check to be
completed before it goes to work, to ensure that all unreachable entries
are removed from the shallow file.

However, in the upcoming patch we do not even want to go that far. We
really only need to remove entries corresponding to pruned commits, i.e.
to commits that no longer exist.

Let's support that use case.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/prune.c |  2 +-
 commit.h        |  2 +-
 shallow.c       | 22 +++++++++++++++++-----
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/builtin/prune.c b/builtin/prune.c
index 41230f821..6d6ab6cf1 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	free(s);
 
 	if (is_repository_shallow(the_repository))
-		prune_shallow(show_only);
+		prune_shallow(show_only, 0);
 
 	return 0;
 }
diff --git a/commit.h b/commit.h
index 1d260d62f..ff34447ab 100644
--- a/commit.h
+++ b/commit.h
@@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,
 					   uint32_t **used,
 					   int *ref_status);
 extern int delayed_reachability_test(struct shallow_info *si, int c);
-extern void prune_shallow(int show_only);
+extern void prune_shallow(int show_only, int quick_prune);
 extern struct trace_key trace_shallow;
 
 extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
diff --git a/shallow.c b/shallow.c
index 732e18d54..0a2671bc2 100644
--- a/shallow.c
+++ b/shallow.c
@@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)
 
 #define SEEN_ONLY 1
 #define VERBOSE   2
+#define QUICK_PRUNE 4
 
 struct write_shallow_data {
 	struct strbuf *out;
@@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 	const char *hex = oid_to_hex(&graft->oid);
 	if (graft->nr_parent != -1)
 		return 0;
-	if (data->flags & SEEN_ONLY) {
+	if (data->flags & QUICK_PRUNE) {
+		struct commit *c = lookup_commit(the_repository, &graft->oid);
+		if (!c || parse_commit(c))
+			return 0;
+	} else if (data->flags & SEEN_ONLY) {
 		struct commit *c = lookup_commit(the_repository, &graft->oid);
 		if (!c || !(c->object.flags & SEEN)) {
 			if (data->flags & VERBOSE)
@@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)
 
 /*
  * mark_reachable_objects() should have been run prior to this and all
- * reachable commits marked as "SEEN".
+ * reachable commits marked as "SEEN", except when quick_prune is non-zero,
+ * in which case lines are excised from the shallow file if they refer to
+ * commits that do not exist (any longer).
  */
-void prune_shallow(int show_only)
+void prune_shallow(int show_only, int quick_prune)
 {
 	struct lock_file shallow_lock = LOCK_INIT;
 	struct strbuf sb = STRBUF_INIT;
+	unsigned flags = SEEN_ONLY;
 	int fd;
 
+	if (quick_prune)
+		flags |= QUICK_PRUNE;
+
 	if (show_only) {
-		write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);
+		flags |= VERBOSE;
+		write_shallow_commits_1(&sb, 0, NULL, flags);
 		strbuf_release(&sb);
 		return;
 	}
@@ -388,7 +400,7 @@ void prune_shallow(int show_only)
 				       git_path_shallow(the_repository),
 				       LOCK_DIE_ON_ERROR);
 	check_shallow_file_for_update(the_repository);
-	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
+	if (write_shallow_commits_1(&sb, 0, NULL, flags)) {
 		if (write_in_full(fd, sb.buf, sb.len) < 0)
 			die_errno("failed to write to %s",
 				  get_lock_file_path(&shallow_lock));
-- 
gitgitgadget

[PATCH v3 3/3] repack -ad: prune the list of shallow commits

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

`git repack` can drop unreachable commits without further warning,
making the corresponding entries in `.git/shallow` invalid, which causes
serious problems when deepening the branches.

One scenario where unreachable commits are dropped by `git repack` is
when a `git fetch --prune` (or even a `git fetch` when a ref was
force-pushed in the meantime) can make a commit unreachable that was
reachable before.

Therefore it is not safe to assume that a `git repack -adlf` will keep
unreachable commits alone (under the assumption that they had not been
packed in the first place, which is an assumption at least some of Git's
code seems to make).

This is particularly important to keep in mind when looking at the
`.git/shallow` file: if any commits listed in that file become
unreachable, it is not a problem, but if they go missing, it *is* a
problem. One symptom of this problem is that a deepening fetch may now
fail with

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

To avoid this problem, let's prune the shallow list in `git repack` when
the `-d` option is passed, unless `-A` is passed, too (which would force
the now-unreachable objects to be turned into loose objects instead of
being deleted). Additionally, we also need to take `--keep-reachable`
and `--unpack-unreachable=<date>` into account.

Note: an alternative solution discussed during the review of this patch
was to teach `git fetch` to simply ignore entries in .git/shallow if the
corresponding commits do not exist locally. A quick test, however,
revealed that the .git/shallow file is written during a shallow *clone*,
in which case the commits do not exist, either, but the "shallow" line
*does* need to be sent. Therefore, this approach would be a lot more
finicky than the approach presented by the this patch.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/repack.c         | 6 ++++++
 t/t5537-fetch-shallow.sh | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index c6a7943d5..9217fc832 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		if (!po_args.quiet && isatty(2))
 			opts |= PRUNE_PACKED_VERBOSE;
 		prune_packed_objects(opts);
+
+		if (!keep_unreachable &&
+		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
+		     unpack_unreachable) &&
+		    is_repository_shallow(the_repository))
+			prune_shallow(0, 1);
 	}
 
 	if (!no_update_server_info)
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 2d0031703..777c9d1dc 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,7 +186,7 @@ EOF
 	test_cmp expect actual
 '
 
-test_expect_failure '.git/shallow is edited by repack' '
+test_expect_success '.git/shallow is edited by repack' '
 	git init shallow-server &&
 	test_commit -C shallow-server A &&
 	test_commit -C shallow-server B &&
-- 
gitgitgadget

Re: [PATCH v3 0/3] repack -ad: fix after fetch --prune in a shallow repository

[Johannes Schindelin]

Hi,

On Mon, 22 Oct 2018, Johannes Schindelin via GitGitGadget wrote:

> Under certain circumstances, commits that were reachable can be made
> unreachable, e.g. via git fetch --prune. These commits might have been
> packed already, in which case git repack -adlf will just drop them without
> giving them the usual grace period before an eventual git prune (via git gc)
> prunes them.
> 
> This is a problem when the shallow file still lists them, which is the
> reason why git prune edits that file. And with the proposed changes, git
> repack -ad will now do the same.
> 
> Reported by Alejandro Pauly.
> 
> Changes since v2:
> 
>  * Fixed a typo in the last commit message.
>  * Added an explanation to the last commit message why we do not simply skip
>    non-existing shallow commits at fetch time.
>  * Introduced a new, "quick prune" mode where prune_shallow() does not try
>    to drop unreachable commits, but only non-existing ones.

BTW this was supposed to address Peff's concern that the SEEN flag would
not be marking all reachable shallow commits at the time when
`cmd_repack()` calls `prune_shallow()`, i.e. the last concern about this
stop-gap patch series.

Ciao,
Dscho

>  * Rebased to current master because there were too many merge conflicts for
>    my liking otherwise.
> 
> Changes since v1:
> 
>  * Also trigger prune_shallow() when --unpack-unreachable=<approxidate> was
>    passed to git repack.
>  * No need to trigger prune_shallow() when git repack was called with -k.
> 
> Johannes Schindelin (3):
>   repack: point out a bug handling stale shallow info
>   shallow: offer to prune only non-existing entries
>   repack -ad: prune the list of shallow commits
> 
>  builtin/prune.c          |  2 +-
>  builtin/repack.c         |  6 ++++++
>  commit.h                 |  2 +-
>  shallow.c                | 22 +++++++++++++++++-----
>  t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++
>  5 files changed, 52 insertions(+), 7 deletions(-)
> 
> 
> base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/9
> 
> Range-diff vs v2:
> 
>  1:  d2be40131 ! 1:  ed8559b91 repack: point out a bug handling stale shallow info
>      @@ -48,4 +48,6 @@
>       +		origin "+refs/heads/*:refs/remotes/origin/*"
>       +'
>       +
>      - test_done
>      + . "$TEST_DIRECTORY"/lib-httpd.sh
>      + start_httpd
>      + 
>  -:  --------- > 2:  f085eb4f7 shallow: offer to prune only non-existing entries
>  2:  c7ee6e008 ! 3:  1f9ff57d5 repack -ad: prune the list of shallow commits
>      @@ -2,15 +2,19 @@
>       
>           repack -ad: prune the list of shallow commits
>       
>      -    While it is true that we never add unreachable commits into pack files
>      -    intentionally (as `git repack`'s documentation states), we must not
>      -    forget that a `git fetch --prune` (or even a `git fetch` when a ref was
>      +    `git repack` can drop unreachable commits without further warning,
>      +    making the corresponding entries in `.git/shallow` invalid, which causes
>      +    serious problems when deepening the branches.
>      +
>      +    One scenario where unreachable commits are dropped by `git repack` is
>      +    when a `git fetch --prune` (or even a `git fetch` when a ref was
>           force-pushed in the meantime) can make a commit unreachable that was
>           reachable before.
>       
>           Therefore it is not safe to assume that a `git repack -adlf` will keep
>           unreachable commits alone (under the assumption that they had not been
>      -    packed in the first place).
>      +    packed in the first place, which is an assumption at least some of Git's
>      +    code seems to make).
>       
>           This is particularly important to keep in mind when looking at the
>           `.git/shallow` file: if any commits listed in that file become
>      @@ -23,8 +27,16 @@
>           To avoid this problem, let's prune the shallow list in `git repack` when
>           the `-d` option is passed, unless `-A` is passed, too (which would force
>           the now-unreachable objects to be turned into loose objects instead of
>      -    being deleted). Additionally, e also need to take `--keep-reachable` and
>      -    `--unpack-unreachable=<date>` into account.
>      +    being deleted). Additionally, we also need to take `--keep-reachable`
>      +    and `--unpack-unreachable=<date>` into account.
>      +
>      +    Note: an alternative solution discussed during the review of this patch
>      +    was to teach `git fetch` to simply ignore entries in .git/shallow if the
>      +    corresponding commits do not exist locally. A quick test, however,
>      +    revealed that the .git/shallow file is written during a shallow *clone*,
>      +    in which case the commits do not exist, either, but the "shallow" line
>      +    *does* need to be sent. Therefore, this approach would be a lot more
>      +    finicky than the approach presented by the this patch.
>       
>           Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>       
>      @@ -32,15 +44,15 @@
>       --- a/builtin/repack.c
>       +++ b/builtin/repack.c
>       @@
>      - 		if (!quiet && isatty(2))
>      + 		if (!po_args.quiet && isatty(2))
>        			opts |= PRUNE_PACKED_VERBOSE;
>        		prune_packed_objects(opts);
>       +
>       +		if (!keep_unreachable &&
>       +		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
>       +		     unpack_unreachable) &&
>      -+		    is_repository_shallow())
>      -+			prune_shallow(0);
>      ++		    is_repository_shallow(the_repository))
>      ++			prune_shallow(0, 1);
>        	}
>        
>        	if (!no_update_server_info)
> 
> -- 
> gitgitgadget
> 

Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info

[Junio C Hamano]

"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

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Junio C Hamano]

Jonathan, do you see any issues with the use of lookup_commit() in
this change wrt lazy clone?  I am wondering what happens when the
commit in question is at, an immediate parent of, or an immediate
child of a promisor object.  I _think_ this change won't make it
worse for two features in playing together, but thought that it
would be better to double check.

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The `prune_shallow()` function wants a full reachability check to be
> completed before it goes to work, to ensure that all unreachable entries
> are removed from the shallow file.
>
> However, in the upcoming patch we do not even want to go that far. We
> really only need to remove entries corresponding to pruned commits, i.e.
> to commits that no longer exist.
>
> Let's support that use case.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/prune.c |  2 +-
>  commit.h        |  2 +-
>  shallow.c       | 22 +++++++++++++++++-----
>  3 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 41230f821..6d6ab6cf1 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
>  	free(s);
>  
>  	if (is_repository_shallow(the_repository))
> -		prune_shallow(show_only);
> +		prune_shallow(show_only, 0);
>  
>  	return 0;
>  }
> diff --git a/commit.h b/commit.h
> index 1d260d62f..ff34447ab 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,
>  					   uint32_t **used,
>  					   int *ref_status);
>  extern int delayed_reachability_test(struct shallow_info *si, int c);
> -extern void prune_shallow(int show_only);
> +extern void prune_shallow(int show_only, int quick_prune);
>  extern struct trace_key trace_shallow;
>  
>  extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
> diff --git a/shallow.c b/shallow.c
> index 732e18d54..0a2671bc2 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)
>  
>  #define SEEN_ONLY 1
>  #define VERBOSE   2
> +#define QUICK_PRUNE 4
>  
>  struct write_shallow_data {
>  	struct strbuf *out;
> @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
>  	const char *hex = oid_to_hex(&graft->oid);
>  	if (graft->nr_parent != -1)
>  		return 0;
> -	if (data->flags & SEEN_ONLY) {
> +	if (data->flags & QUICK_PRUNE) {
> +		struct commit *c = lookup_commit(the_repository, &graft->oid);
> +		if (!c || parse_commit(c))
> +			return 0;
> +	} else if (data->flags & SEEN_ONLY) {
>  		struct commit *c = lookup_commit(the_repository, &graft->oid);
>  		if (!c || !(c->object.flags & SEEN)) {
>  			if (data->flags & VERBOSE)
> @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)
>  
>  /*
>   * mark_reachable_objects() should have been run prior to this and all
> - * reachable commits marked as "SEEN".
> + * reachable commits marked as "SEEN", except when quick_prune is non-zero,
> + * in which case lines are excised from the shallow file if they refer to
> + * commits that do not exist (any longer).
>   */
> -void prune_shallow(int show_only)
> +void prune_shallow(int show_only, int quick_prune)
>  {
>  	struct lock_file shallow_lock = LOCK_INIT;
>  	struct strbuf sb = STRBUF_INIT;
> +	unsigned flags = SEEN_ONLY;
>  	int fd;
>  
> +	if (quick_prune)
> +		flags |= QUICK_PRUNE;
> +
>  	if (show_only) {
> -		write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);
> +		flags |= VERBOSE;
> +		write_shallow_commits_1(&sb, 0, NULL, flags);
>  		strbuf_release(&sb);
>  		return;
>  	}
> @@ -388,7 +400,7 @@ void prune_shallow(int show_only)
>  				       git_path_shallow(the_repository),
>  				       LOCK_DIE_ON_ERROR);
>  	check_shallow_file_for_update(the_repository);
> -	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
> +	if (write_shallow_commits_1(&sb, 0, NULL, flags)) {
>  		if (write_in_full(fd, sb.buf, sb.len) < 0)
>  			die_errno("failed to write to %s",
>  				  get_lock_file_path(&shallow_lock));

Re: [PATCH v3 3/3] repack -ad: prune the list of shallow commits

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/builtin/repack.c b/builtin/repack.c
> index c6a7943d5..9217fc832 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		if (!po_args.quiet && isatty(2))
>  			opts |= PRUNE_PACKED_VERBOSE;
>  		prune_packed_objects(opts);
> +
> +		if (!keep_unreachable &&
> +		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
> +		     unpack_unreachable) &&
> +		    is_repository_shallow(the_repository))
> +			prune_shallow(0, 1);

The logic looks correct (and the reasoning behind it, which was
explained in the log message, was sound).  prune_shallow(0, 1)
however is not easily understandable.

There are only two callsites of this function after these three
patches, and the areas of the code that have these calls are in
relatively quiescent parts in the whole tree, so it shouldn't be too
distracting to do an update to make the function take a flag word,
so that we can make callsites more immediately obvious, i.e.

	prune_shallow(PRUNE_SHALLOW_QUICK)

It of course can be left as a low-hanging fruit loose-end.

> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index 2d0031703..777c9d1dc 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -186,7 +186,7 @@ EOF
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure '.git/shallow is edited by repack' '
> +test_expect_success '.git/shallow is edited by repack' '
>  	git init shallow-server &&
>  	test_commit -C shallow-server A &&
>  	test_commit -C shallow-server B &&

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Johannes Schindelin]

Hi Junio & Jonathan,

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

> Jonathan, do you see any issues with the use of lookup_commit() in
> this change wrt lazy clone?  I am wondering what happens when the
> commit in question is at, an immediate parent of, or an immediate
> child of a promisor object.  I _think_ this change won't make it
> worse for two features in playing together, but thought that it
> would be better to double check.

Good point.

Instinctively, I would say that no promised object can be a shallow
commit. The entire idea of a shallow commit is that it *is* present, but
none of its parents.

Also, I would claim that the shallow feature does not make sense with lazy
clones, as lazy clones offer a superset of shallow clone's functionality.

However, I am curious whether there is a better way to check for the
presence of a local commit? Do we have an API function for that, that I
missed? (I do not really want to parse the commit, after all, just verify
that it is not pruned.)

Thanks,
Dscho

> 
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The `prune_shallow()` function wants a full reachability check to be
> > completed before it goes to work, to ensure that all unreachable entries
> > are removed from the shallow file.
> >
> > However, in the upcoming patch we do not even want to go that far. We
> > really only need to remove entries corresponding to pruned commits, i.e.
> > to commits that no longer exist.
> >
> > Let's support that use case.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/prune.c |  2 +-
> >  commit.h        |  2 +-
> >  shallow.c       | 22 +++++++++++++++++-----
> >  3 files changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/builtin/prune.c b/builtin/prune.c
> > index 41230f821..6d6ab6cf1 100644
> > --- a/builtin/prune.c
> > +++ b/builtin/prune.c
> > @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
> >  	free(s);
> >  
> >  	if (is_repository_shallow(the_repository))
> > -		prune_shallow(show_only);
> > +		prune_shallow(show_only, 0);
> >  
> >  	return 0;
> >  }
> > diff --git a/commit.h b/commit.h
> > index 1d260d62f..ff34447ab 100644
> > --- a/commit.h
> > +++ b/commit.h
> > @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,
> >  					   uint32_t **used,
> >  					   int *ref_status);
> >  extern int delayed_reachability_test(struct shallow_info *si, int c);
> > -extern void prune_shallow(int show_only);
> > +extern void prune_shallow(int show_only, int quick_prune);
> >  extern struct trace_key trace_shallow;
> >  
> >  extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
> > diff --git a/shallow.c b/shallow.c
> > index 732e18d54..0a2671bc2 100644
> > --- a/shallow.c
> > +++ b/shallow.c
> > @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)
> >  
> >  #define SEEN_ONLY 1
> >  #define VERBOSE   2
> > +#define QUICK_PRUNE 4
> >  
> >  struct write_shallow_data {
> >  	struct strbuf *out;
> > @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
> >  	const char *hex = oid_to_hex(&graft->oid);
> >  	if (graft->nr_parent != -1)
> >  		return 0;
> > -	if (data->flags & SEEN_ONLY) {
> > +	if (data->flags & QUICK_PRUNE) {
> > +		struct commit *c = lookup_commit(the_repository, &graft->oid);
> > +		if (!c || parse_commit(c))
> > +			return 0;
> > +	} else if (data->flags & SEEN_ONLY) {
> >  		struct commit *c = lookup_commit(the_repository, &graft->oid);
> >  		if (!c || !(c->object.flags & SEEN)) {
> >  			if (data->flags & VERBOSE)
> > @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)
> >  
> >  /*
> >   * mark_reachable_objects() should have been run prior to this and all
> > - * reachable commits marked as "SEEN".
> > + * reachable commits marked as "SEEN", except when quick_prune is non-zero,
> > + * in which case lines are excised from the shallow file if they refer to
> > + * commits that do not exist (any longer).
> >   */
> > -void prune_shallow(int show_only)
> > +void prune_shallow(int show_only, int quick_prune)
> >  {
> >  	struct lock_file shallow_lock = LOCK_INIT;
> >  	struct strbuf sb = STRBUF_INIT;
> > +	unsigned flags = SEEN_ONLY;
> >  	int fd;
> >  
> > +	if (quick_prune)
> > +		flags |= QUICK_PRUNE;
> > +
> >  	if (show_only) {
> > -		write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);
> > +		flags |= VERBOSE;
> > +		write_shallow_commits_1(&sb, 0, NULL, flags);
> >  		strbuf_release(&sb);
> >  		return;
> >  	}
> > @@ -388,7 +400,7 @@ void prune_shallow(int show_only)
> >  				       git_path_shallow(the_repository),
> >  				       LOCK_DIE_ON_ERROR);
> >  	check_shallow_file_for_update(the_repository);
> > -	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
> > +	if (write_shallow_commits_1(&sb, 0, NULL, flags)) {
> >  		if (write_in_full(fd, sb.buf, sb.len) < 0)
> >  			die_errno("failed to write to %s",
> >  				  get_lock_file_path(&shallow_lock));
> 

Re: [PATCH v3 3/3] repack -ad: prune the list of shallow commits

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/builtin/repack.c b/builtin/repack.c
> > index c6a7943d5..9217fc832 100644
> > --- a/builtin/repack.c
> > +++ b/builtin/repack.c
> > @@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> >  		if (!po_args.quiet && isatty(2))
> >  			opts |= PRUNE_PACKED_VERBOSE;
> >  		prune_packed_objects(opts);
> > +
> > +		if (!keep_unreachable &&
> > +		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
> > +		     unpack_unreachable) &&
> > +		    is_repository_shallow(the_repository))
> > +			prune_shallow(0, 1);
> 
> The logic looks correct (and the reasoning behind it, which was
> explained in the log message, was sound).  prune_shallow(0, 1)
> however is not easily understandable.
> 
> There are only two callsites of this function after these three
> patches, and the areas of the code that have these calls are in
> relatively quiescent parts in the whole tree, so it shouldn't be too
> distracting to do an update to make the function take a flag word,
> so that we can make callsites more immediately obvious, i.e.
> 
> 	prune_shallow(PRUNE_SHALLOW_QUICK)
> 
> It of course can be left as a low-hanging fruit loose-end.

I almost did that, but then decided not to clutter the previous patch with
this change (and global constant).

Having said that, since you also had this idea, I will make that change.
It will read nicer, you are right.

Ciao,
Dscho

> 
> > diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> > index 2d0031703..777c9d1dc 100755
> > --- a/t/t5537-fetch-shallow.sh
> > +++ b/t/t5537-fetch-shallow.sh
> > @@ -186,7 +186,7 @@ EOF
> >  	test_cmp expect actual
> >  '
> >  
> > -test_expect_failure '.git/shallow is edited by repack' '
> > +test_expect_success '.git/shallow is edited by repack' '
> >  	git init shallow-server &&
> >  	test_commit -C shallow-server A &&
> >  	test_commit -C shallow-server B &&
> 

Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info

[Johannes Schindelin]

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
> 
> 

Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info

[Johannes Schindelin]

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 v4 0/3] repack -ad: fix after fetch --prune in a shallow repository

[Johannes Schindelin via GitGitGadget]

Under certain circumstances, commits that were reachable can be made
unreachable, e.g. via git fetch --prune. These commits might have been
packed already, in which case git repack -adlf will just drop them without
giving them the usual grace period before an eventual git prune (via git gc)
prunes them.

This is a problem when the shallow file still lists them, which is the
reason why git prune edits that file. And with the proposed changes, git
repack -ad will now do the same.

Reported by Alejandro Pauly.

Changes since v3:

 * Made the regression test less confusing with regards to tags that might
   need to be dereferenced.
 * Introduced new global constants PRUNE_SHOW_ONLY and PRUNE_QUICK instead
   of extending the signature of prune_shallow() with Boolean parameters.
 * While at it, renamed the file-local constant from QUICK_PRUNE to QUICK.
 * Replaced the lookup_commit() && parse_commit() cadence (that wants to
   test for the existence of a commit) to has_object_file(), for ease of
   readability, and also to make it more obvious how to add a call to 
   is_promisor_object() if we ever figure out that that would be necessary.

Changes since v2:

 * Fixed a typo in the last commit message.
 * Added an explanation to the last commit message why we do not simply skip
   non-existing shallow commits at fetch time.
 * Introduced a new, "quick prune" mode where prune_shallow() does not try
   to drop unreachable commits, but only non-existing ones.
 * Rebased to current master because there were too many merge conflicts for
   my liking otherwise.

Changes since v1:

 * Also trigger prune_shallow() when --unpack-unreachable=<approxidate> was
   passed to git repack.
 * No need to trigger prune_shallow() when git repack was called with -k.

Johannes Schindelin (3):
  repack: point out a bug handling stale shallow info
  shallow: offer to prune only non-existing entries
  repack -ad: prune the list of shallow commits

 builtin/prune.c          |  2 +-
 builtin/repack.c         |  6 ++++++
 commit.h                 |  4 +++-
 shallow.c                | 23 +++++++++++++++++------
 t/t5537-fetch-shallow.sh | 27 +++++++++++++++++++++++++++
 5 files changed, 54 insertions(+), 8 deletions(-)


base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-9/dscho/shallow-and-fetch-prune-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/9

Range-diff vs v3:

 1:  ed8559b91 ! 1:  d9720bd25 repack: point out a bug handling stale shallow info
     @@ -29,7 +29,7 @@
      +	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)" &&
     ++	d="$(git -C shallow-server rev-parse --verify D^0)" &&
      +	git -C shallow-server checkout master &&
      +
      +	git clone --depth=1 --no-tags --no-single-branch \
     @@ -43,7 +43,7 @@
      +	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-server branch branch-orig $d &&
      +	git -C shallow-client fetch --prune --depth=2 \
      +		origin "+refs/heads/*:refs/remotes/origin/*"
      +'
 2:  f085eb4f7 ! 2:  18308c13e shallow: offer to prune only non-existing entries
     @@ -12,6 +12,10 @@
      
          Let's support that use case.
      
     +    Rather than extending the signature of `prune_shallow()` to accept
     +    another Boolean, let's turn it into a bit field and declare constants,
     +    for readability.
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
      diff --git a/builtin/prune.c b/builtin/prune.c
     @@ -22,7 +26,7 @@
       
       	if (is_repository_shallow(the_repository))
      -		prune_shallow(show_only);
     -+		prune_shallow(show_only, 0);
     ++		prune_shallow(show_only ? PRUNE_SHOW_ONLY : 0);
       
       	return 0;
       }
     @@ -35,7 +39,9 @@
       					   int *ref_status);
       extern int delayed_reachability_test(struct shallow_info *si, int c);
      -extern void prune_shallow(int show_only);
     -+extern void prune_shallow(int show_only, int quick_prune);
     ++#define PRUNE_SHOW_ONLY 1
     ++#define PRUNE_QUICK 2
     ++extern void prune_shallow(unsigned options);
       extern struct trace_key trace_shallow;
       
       extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
     @@ -47,7 +53,7 @@
       
       #define SEEN_ONLY 1
       #define VERBOSE   2
     -+#define QUICK_PRUNE 4
     ++#define QUICK 4
       
       struct write_shallow_data {
       	struct strbuf *out;
     @@ -56,9 +62,8 @@
       	if (graft->nr_parent != -1)
       		return 0;
      -	if (data->flags & SEEN_ONLY) {
     -+	if (data->flags & QUICK_PRUNE) {
     -+		struct commit *c = lookup_commit(the_repository, &graft->oid);
     -+		if (!c || parse_commit(c))
     ++	if (data->flags & QUICK) {
     ++		if (!has_object_file(&graft->oid))
      +			return 0;
      +	} else if (data->flags & SEEN_ONLY) {
       		struct commit *c = lookup_commit(the_repository, &graft->oid);
     @@ -74,18 +79,19 @@
      + * commits that do not exist (any longer).
        */
      -void prune_shallow(int show_only)
     -+void prune_shallow(int show_only, int quick_prune)
     ++void prune_shallow(unsigned options)
       {
       	struct lock_file shallow_lock = LOCK_INIT;
       	struct strbuf sb = STRBUF_INIT;
      +	unsigned flags = SEEN_ONLY;
       	int fd;
       
     -+	if (quick_prune)
     -+		flags |= QUICK_PRUNE;
     -+
     - 	if (show_only) {
     +-	if (show_only) {
      -		write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);
     ++	if (options & PRUNE_QUICK)
     ++		flags |= QUICK;
     ++
     ++	if (options & PRUNE_SHOW_ONLY) {
      +		flags |= VERBOSE;
      +		write_shallow_commits_1(&sb, 0, NULL, flags);
       		strbuf_release(&sb);
 3:  1f9ff57d5 ! 3:  2858bc886 repack -ad: prune the list of shallow commits
     @@ -52,7 +52,7 @@
      +		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
      +		     unpack_unreachable) &&
      +		    is_repository_shallow(the_repository))
     -+			prune_shallow(0, 1);
     ++			prune_shallow(PRUNE_QUICK);
       	}
       
       	if (!no_update_server_info)

-- 
gitgitgadget

[PATCH v4 1/3] repack: point out a bug handling stale shallow info

[Johannes Schindelin via GitGitGadget]

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(+)

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 7045685e2..686fdc928 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^0)" &&
+	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 &&
+	git -C shallow-client fetch --prune --depth=2 \
+		origin "+refs/heads/*:refs/remotes/origin/*"
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
gitgitgadget

[PATCH v4 2/3] shallow: offer to prune only non-existing entries

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The `prune_shallow()` function wants a full reachability check to be
completed before it goes to work, to ensure that all unreachable entries
are removed from the shallow file.

However, in the upcoming patch we do not even want to go that far. We
really only need to remove entries corresponding to pruned commits, i.e.
to commits that no longer exist.

Let's support that use case.

Rather than extending the signature of `prune_shallow()` to accept
another Boolean, let's turn it into a bit field and declare constants,
for readability.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/prune.c |  2 +-
 commit.h        |  4 +++-
 shallow.c       | 23 +++++++++++++++++------
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/builtin/prune.c b/builtin/prune.c
index 41230f821..1ec9ddd75 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	free(s);
 
 	if (is_repository_shallow(the_repository))
-		prune_shallow(show_only);
+		prune_shallow(show_only ? PRUNE_SHOW_ONLY : 0);
 
 	return 0;
 }
diff --git a/commit.h b/commit.h
index 1d260d62f..b80d6fdb5 100644
--- a/commit.h
+++ b/commit.h
@@ -249,7 +249,9 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,
 					   uint32_t **used,
 					   int *ref_status);
 extern int delayed_reachability_test(struct shallow_info *si, int c);
-extern void prune_shallow(int show_only);
+#define PRUNE_SHOW_ONLY 1
+#define PRUNE_QUICK 2
+extern void prune_shallow(unsigned options);
 extern struct trace_key trace_shallow;
 
 extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
diff --git a/shallow.c b/shallow.c
index 732e18d54..02fdbfc55 100644
--- a/shallow.c
+++ b/shallow.c
@@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)
 
 #define SEEN_ONLY 1
 #define VERBOSE   2
+#define QUICK 4
 
 struct write_shallow_data {
 	struct strbuf *out;
@@ -261,7 +262,10 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 	const char *hex = oid_to_hex(&graft->oid);
 	if (graft->nr_parent != -1)
 		return 0;
-	if (data->flags & SEEN_ONLY) {
+	if (data->flags & QUICK) {
+		if (!has_object_file(&graft->oid))
+			return 0;
+	} else if (data->flags & SEEN_ONLY) {
 		struct commit *c = lookup_commit(the_repository, &graft->oid);
 		if (!c || !(c->object.flags & SEEN)) {
 			if (data->flags & VERBOSE)
@@ -371,16 +375,23 @@ void advertise_shallow_grafts(int fd)
 
 /*
  * mark_reachable_objects() should have been run prior to this and all
- * reachable commits marked as "SEEN".
+ * reachable commits marked as "SEEN", except when quick_prune is non-zero,
+ * in which case lines are excised from the shallow file if they refer to
+ * commits that do not exist (any longer).
  */
-void prune_shallow(int show_only)
+void prune_shallow(unsigned options)
 {
 	struct lock_file shallow_lock = LOCK_INIT;
 	struct strbuf sb = STRBUF_INIT;
+	unsigned flags = SEEN_ONLY;
 	int fd;
 
-	if (show_only) {
-		write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);
+	if (options & PRUNE_QUICK)
+		flags |= QUICK;
+
+	if (options & PRUNE_SHOW_ONLY) {
+		flags |= VERBOSE;
+		write_shallow_commits_1(&sb, 0, NULL, flags);
 		strbuf_release(&sb);
 		return;
 	}
@@ -388,7 +399,7 @@ void prune_shallow(int show_only)
 				       git_path_shallow(the_repository),
 				       LOCK_DIE_ON_ERROR);
 	check_shallow_file_for_update(the_repository);
-	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
+	if (write_shallow_commits_1(&sb, 0, NULL, flags)) {
 		if (write_in_full(fd, sb.buf, sb.len) < 0)
 			die_errno("failed to write to %s",
 				  get_lock_file_path(&shallow_lock));
-- 
gitgitgadget

[PATCH v4 3/3] repack -ad: prune the list of shallow commits

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

`git repack` can drop unreachable commits without further warning,
making the corresponding entries in `.git/shallow` invalid, which causes
serious problems when deepening the branches.

One scenario where unreachable commits are dropped by `git repack` is
when a `git fetch --prune` (or even a `git fetch` when a ref was
force-pushed in the meantime) can make a commit unreachable that was
reachable before.

Therefore it is not safe to assume that a `git repack -adlf` will keep
unreachable commits alone (under the assumption that they had not been
packed in the first place, which is an assumption at least some of Git's
code seems to make).

This is particularly important to keep in mind when looking at the
`.git/shallow` file: if any commits listed in that file become
unreachable, it is not a problem, but if they go missing, it *is* a
problem. One symptom of this problem is that a deepening fetch may now
fail with

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

To avoid this problem, let's prune the shallow list in `git repack` when
the `-d` option is passed, unless `-A` is passed, too (which would force
the now-unreachable objects to be turned into loose objects instead of
being deleted). Additionally, we also need to take `--keep-reachable`
and `--unpack-unreachable=<date>` into account.

Note: an alternative solution discussed during the review of this patch
was to teach `git fetch` to simply ignore entries in .git/shallow if the
corresponding commits do not exist locally. A quick test, however,
revealed that the .git/shallow file is written during a shallow *clone*,
in which case the commits do not exist, either, but the "shallow" line
*does* need to be sent. Therefore, this approach would be a lot more
finicky than the approach presented by the this patch.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/repack.c         | 6 ++++++
 t/t5537-fetch-shallow.sh | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index c6a7943d5..acd43c75d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -549,6 +549,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		if (!po_args.quiet && isatty(2))
 			opts |= PRUNE_PACKED_VERBOSE;
 		prune_packed_objects(opts);
+
+		if (!keep_unreachable &&
+		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
+		     unpack_unreachable) &&
+		    is_repository_shallow(the_repository))
+			prune_shallow(PRUNE_QUICK);
 	}
 
 	if (!no_update_server_info)
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 686fdc928..6faf17e17 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -186,7 +186,7 @@ EOF
 	test_cmp expect actual
 '
 
-test_expect_failure '.git/shallow is edited by repack' '
+test_expect_success '.git/shallow is edited by repack' '
 	git init shallow-server &&
 	test_commit -C shallow-server A &&
 	test_commit -C shallow-server B &&
-- 
gitgitgadget

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Johannes Schindelin]

Hi,

On Wed, 24 Oct 2018, Johannes Schindelin wrote:

> On Wed, 24 Oct 2018, Junio C Hamano wrote:
> 
> > Jonathan, do you see any issues with the use of lookup_commit() in
> > this change wrt lazy clone?  I am wondering what happens when the
> > commit in question is at, an immediate parent of, or an immediate
> > child of a promisor object.  I _think_ this change won't make it
> > worse for two features in playing together, but thought that it
> > would be better to double check.
> 
> Good point.
> 
> Instinctively, I would say that no promised object can be a shallow
> commit. The entire idea of a shallow commit is that it *is* present, but
> none of its parents.
> 
> Also, I would claim that the shallow feature does not make sense with lazy
> clones, as lazy clones offer a superset of shallow clone's functionality.
> 
> However, I am curious whether there is a better way to check for the
> presence of a local commit? Do we have an API function for that, that I
> missed? (I do not really want to parse the commit, after all, just verify
> that it is not pruned.)

Okay, I looked around a bit. It seems that there is an
`is_promisor_object(oid)` function in `pu` to see whether an object was
promised. If need be (and I am still not convinced that there is a need),
then we can always add a call to that function to the condition.

Coming back to my question whether there is a better way to check for the
presence of a local commit, I figured that I can use `has_object_file()`
instead of looking up and parsing the commit, as this code does not really
need to verify that the shallow entry refers to a commit, but only that it
refers to a local object.

So I'll send another iteration shortly, with this diff applied to 2/3:

-- snip --
diff --git a/shallow.c b/shallow.c
index 9c3faa8af243..02fdbfc554c4 100644
--- a/shallow.c
+++ b/shallow.c
@@ -263,8 +263,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 	if (graft->nr_parent != -1)
 		return 0;
 	if (data->flags & QUICK) {
-		struct commit *c = lookup_commit(the_repository, &graft->oid);
-		if (!c || parse_commit(c))
+		if (!has_object_file(&graft->oid))
 			return 0;
 	} else if (data->flags & SEEN_ONLY) {
 		struct commit *c = lookup_commit(the_repository, &graft->oid);
--snap --

Ciao,
Dscho

> 
> > 
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> > 
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >
> > > The `prune_shallow()` function wants a full reachability check to be
> > > completed before it goes to work, to ensure that all unreachable entries
> > > are removed from the shallow file.
> > >
> > > However, in the upcoming patch we do not even want to go that far. We
> > > really only need to remove entries corresponding to pruned commits, i.e.
> > > to commits that no longer exist.
> > >
> > > Let's support that use case.
> > >
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > >  builtin/prune.c |  2 +-
> > >  commit.h        |  2 +-
> > >  shallow.c       | 22 +++++++++++++++++-----
> > >  3 files changed, 19 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/builtin/prune.c b/builtin/prune.c
> > > index 41230f821..6d6ab6cf1 100644
> > > --- a/builtin/prune.c
> > > +++ b/builtin/prune.c
> > > @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
> > >  	free(s);
> > >  
> > >  	if (is_repository_shallow(the_repository))
> > > -		prune_shallow(show_only);
> > > +		prune_shallow(show_only, 0);
> > >  
> > >  	return 0;
> > >  }
> > > diff --git a/commit.h b/commit.h
> > > index 1d260d62f..ff34447ab 100644
> > > --- a/commit.h
> > > +++ b/commit.h
> > > @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,
> > >  					   uint32_t **used,
> > >  					   int *ref_status);
> > >  extern int delayed_reachability_test(struct shallow_info *si, int c);
> > > -extern void prune_shallow(int show_only);
> > > +extern void prune_shallow(int show_only, int quick_prune);
> > >  extern struct trace_key trace_shallow;
> > >  
> > >  extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
> > > diff --git a/shallow.c b/shallow.c
> > > index 732e18d54..0a2671bc2 100644
> > > --- a/shallow.c
> > > +++ b/shallow.c
> > > @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct repository *r)
> > >  
> > >  #define SEEN_ONLY 1
> > >  #define VERBOSE   2
> > > +#define QUICK_PRUNE 4
> > >  
> > >  struct write_shallow_data {
> > >  	struct strbuf *out;
> > > @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
> > >  	const char *hex = oid_to_hex(&graft->oid);
> > >  	if (graft->nr_parent != -1)
> > >  		return 0;
> > > -	if (data->flags & SEEN_ONLY) {
> > > +	if (data->flags & QUICK_PRUNE) {
> > > +		struct commit *c = lookup_commit(the_repository, &graft->oid);
> > > +		if (!c || parse_commit(c))
> > > +			return 0;
> > > +	} else if (data->flags & SEEN_ONLY) {
> > >  		struct commit *c = lookup_commit(the_repository, &graft->oid);
> > >  		if (!c || !(c->object.flags & SEEN)) {
> > >  			if (data->flags & VERBOSE)
> > > @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)
> > >  
> > >  /*
> > >   * mark_reachable_objects() should have been run prior to this and all
> > > - * reachable commits marked as "SEEN".
> > > + * reachable commits marked as "SEEN", except when quick_prune is non-zero,
> > > + * in which case lines are excised from the shallow file if they refer to
> > > + * commits that do not exist (any longer).
> > >   */
> > > -void prune_shallow(int show_only)
> > > +void prune_shallow(int show_only, int quick_prune)
> > >  {
> > >  	struct lock_file shallow_lock = LOCK_INIT;
> > >  	struct strbuf sb = STRBUF_INIT;
> > > +	unsigned flags = SEEN_ONLY;
> > >  	int fd;
> > >  
> > > +	if (quick_prune)
> > > +		flags |= QUICK_PRUNE;
> > > +
> > >  	if (show_only) {
> > > -		write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);
> > > +		flags |= VERBOSE;
> > > +		write_shallow_commits_1(&sb, 0, NULL, flags);
> > >  		strbuf_release(&sb);
> > >  		return;
> > >  	}
> > > @@ -388,7 +400,7 @@ void prune_shallow(int show_only)
> > >  				       git_path_shallow(the_repository),
> > >  				       LOCK_DIE_ON_ERROR);
> > >  	check_shallow_file_for_update(the_repository);
> > > -	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
> > > +	if (write_shallow_commits_1(&sb, 0, NULL, flags)) {
> > >  		if (write_in_full(fd, sb.buf, sb.len) < 0)
> > >  			die_errno("failed to write to %s",
> > >  				  get_lock_file_path(&shallow_lock));
> > 
> 

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Jonathan Tan]

> On Wed, 24 Oct 2018, Johannes Schindelin wrote:
> 
> > On Wed, 24 Oct 2018, Junio C Hamano wrote:
> > 
> > > Jonathan, do you see any issues with the use of lookup_commit() in
> > > this change wrt lazy clone?  I am wondering what happens when the
> > > commit in question is at, an immediate parent of, or an immediate
> > > child of a promisor object.  I _think_ this change won't make it
> > > worse for two features in playing together, but thought that it
> > > would be better to double check.
> > 
> > Good point.
> > 
> > Instinctively, I would say that no promised object can be a shallow
> > commit. The entire idea of a shallow commit is that it *is* present, but
> > none of its parents.

I'm envisioning a client repo with a single branch, cloned both with
--depth=1 and with --filter=<foo>, that has just fetched to the same
branch also with --depth=1 resulting in a fast-forward from A to B.

If A is B's parent, then A would be known to be promised. (Incidentally,
the problem is greater in current Git, because for performance reasons,
we do not check promisor status when lazily fetching - so it doesn't
matter here whether an object is known to be promised or not.)

When pruning shallow and checking the existence of A, this would trigger
a fetch for A, which would download all commits and trees reachable from
it.

It sounds safer to me to use the fast approach in this patch when the
repository is not partial, and stick to the slow approach when it is.

> > However, I am curious whether there is a better way to check for the
> > presence of a local commit? Do we have an API function for that, that I
> > missed? (I do not really want to parse the commit, after all, just verify
> > that it is not pruned.)
> 
> Okay, I looked around a bit. It seems that there is an
> `is_promisor_object(oid)` function in `pu` to see whether an object was
> promised. If need be (and I am still not convinced that there is a need),
> then we can always add a call to that function to the condition.

I don't think there is a need for is_promisor_object() either - as
described above, it doesn't completely solve the problem.

> Coming back to my question whether there is a better way to check for the
> presence of a local commit, I figured that I can use `has_object_file()`
> instead of looking up and parsing the commit, as this code does not really
> need to verify that the shallow entry refers to a commit, but only that it
> refers to a local object.

Note that has_object_file() also triggers the lazy fetch if needed, but
I agree that it's better because you don't really need to parse the
commit.

There is the possibility of just checking for loose objects (which does
not trigger any lazy fetches), which works for builtin/prune since it
only prunes loose objects, but doesn't work in the general case, I
guess.

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Johannes Schindelin]

Hi Jonathan,

On Thu, 25 Oct 2018, Jonathan Tan wrote:

> > On Wed, 24 Oct 2018, Johannes Schindelin wrote:
> > 
> > Coming back to my question whether there is a better way to check for
> > the presence of a local commit, I figured that I can use
> > `has_object_file()` instead of looking up and parsing the commit, as
> > this code does not really need to verify that the shallow entry refers
> > to a commit, but only that it refers to a local object.
> 
> Note that has_object_file() also triggers the lazy fetch if needed, but
> I agree that it's better because you don't really need to parse the
> commit.

Thanks for confirming.

So even better would be to use `is_promisor_object(oid) ||
has_object_file(oid)`, right?

This is something that is probably not even needed: as I mentioned, the
shallow commits are *expected* to be local. It should not ever happen that
they are fetched.

> There is the possibility of just checking for loose objects (which does
> not trigger any lazy fetches), which works for builtin/prune since it
> only prunes loose objects, but doesn't work in the general case, I
> guess.

In the test case I added, the commit object is actually packed. And then,
because it became unreachable, it is dropped.

If I only checked for loose objects here, the shallow line would already
be removed when the commit gets packed, which would be the wrong thing to
do.

In short: thank you for confirming that the current version of the patch
seems to be the best we can do for now.

Ciao,
Dscho

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Jonathan Tan]

> Thanks for confirming.
> 
> So even better would be to use `is_promisor_object(oid) ||
> has_object_file(oid)`, right?
> 
> This is something that is probably not even needed: as I mentioned, the
> shallow commits are *expected* to be local. It should not ever happen that
> they are fetched.

That would help, but I don't think it would help in the "fast-forward
from A to B where A is B's parent" case I describe in [1].

My suggestion was:

> It sounds safer to me to use the fast approach in this patch when the
> repository is not partial, and stick to the slow approach when it is.

which can be done by replacing "prune_shallow(0, 1)" in patch 3 with
"prune_shallow(0, !repository_format_partial_clone)", possibly with a comment
that the fast method checks object existence for each shallow line directly,
which is undesirable when the repository is a partial clone.
(repository_format_partial_clone is non-NULL with the name of the promisor
remote if the repository is a partial clone, and NULL otherwise).

[1] https://public-inbox.org/git/20181025185459.206127-1-jonathantanmy@google.com/

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Johannes Schindelin]

Hi Jonathan,

On Fri, 26 Oct 2018, Jonathan Tan wrote:

> > So even better would be to use `is_promisor_object(oid) ||
> > has_object_file(oid)`, right?
> > 
> > This is something that is probably not even needed: as I mentioned,
> > the shallow commits are *expected* to be local. It should not ever
> > happen that they are fetched.
> 
> That would help, but I don't think it would help in the "fast-forward
> from A to B where A is B's parent" case I describe in [1].

I don't think that that analysis is correct. It assumes that there could
be a promised commit that is also listed as shallow. I do not think that
is a possible scenario.

And even if it were, why would asking for a promised commit object
download not only that object but also all of its trees and all of its
ancestors? That's not how I understood the idea of the lazy clone: I
understood promised objects to be downloaded on demand, individually.

> My suggestion was:
> 
> > It sounds safer to me to use the fast approach in this patch when the
> > repository is not partial, and stick to the slow approach when it is.
> 
> which can be done by replacing "prune_shallow(0, 1)" in patch 3 with
> "prune_shallow(0, !repository_format_partial_clone)", possibly with a comment
> that the fast method checks object existence for each shallow line directly,
> which is undesirable when the repository is a partial clone.

I am afraid that that would re-introduce the bug pointed out by Peff: you
*really* would need to traverse all reachable commits to use the non-fast
pruning method. And we simply don't.

Ciao,
Dscho

> (repository_format_partial_clone is non-NULL with the name of the promisor
> remote if the repository is a partial clone, and NULL otherwise).
> 
> [1] https://public-inbox.org/git/20181025185459.206127-1-jonathantanmy@google.com/
>