repack -ad: fix after `fetch --prune` in a shallow repository

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

Quoted 6 lines · Johannes Schindelin via GitGitGadget

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

Quoted 6 lines

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

Quoted 9 lines

builtin/repack.c +1 -0

index 6c636e159..45f321b23 100644
@@ -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);
+

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 451

Quoted 4 lines

builtin/repack.c +3 -0

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

Quoted 7 lines

Quoted 4 lines

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:

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

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

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

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.

From: Johannes Schindelin <[email protected]>

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>

Quoted 19 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

Quoted 2 lines

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

Could you elaborate a bit more?

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.

Quoted 9 lines

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

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 <[email protected]>

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.

Quoted 2 lines

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

Quoted 3 lines · Jeff King

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.

Quoted 3 lines

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

Quoted 3 lines

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.

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

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.

Quoted 11 lines

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

Quoted 2 lines

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.

Quoted 11 lines · Duy Nguyen

Quoted 11 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 5 lines · Jeff King

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.

Quoted 6 lines · Jeff King

Quoted 2 lines

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

Quoted 25 lines · Duy Nguyen

Quoted 21 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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>

Quoted 1 line

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.

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · Jeff King

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

Quoted 5 lines

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.

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?

Quoted 9 lines

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.

Quoted 4 lines

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.

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 <[email protected]>

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

Signature · 1 line

gitgitgadget

From: Johannes Schindelin <[email protected]>

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>

t/t5537-fetch-shallow.sh +4 -0

+	git -C shallow-client fetch --prune --depth=2 \
+		origin "+refs/heads/*:refs/remotes/origin/*"
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/prune.c |  2 +-
 commit.h        |  2 +-
 shallow.c       | 22 +++++++++++++++++-----
 3 files changed, 19 insertions(+), 7 deletions(-)

builtin/prune.c +1 -1

index 41230f821..6d6ab6cf1 100644
@@ -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;
 }

commit.h +1 -1

index 1d260d62f..ff34447ab 100644
@@ -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);

shallow.c +17 -5

index 732e18d54..0a2671bc2 100644
@@ -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));

builtin/repack.c +0 -0

index c6a7943d5..9217fc832 100644

t/t5537-fetch-shallow.sh +1 -1

index 2d0031703..777c9d1dc 100755
@@ -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 &&

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" <[email protected]>

Quoted 106 lines · writes:

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/prune.c |  2 +-
 commit.h        |  2 +-
 shallow.c       | 22 +++++++++++++++++-----
 3 files changed, 19 insertions(+), 7 deletions(-)

builtin/prune.c +1 -1

index 41230f821..6d6ab6cf1 100644
@@ -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;
 }

commit.h +1 -1

index 1d260d62f..ff34447ab 100644
@@ -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);

shallow.c +17 -5

index 732e18d54..0a2671bc2 100644
@@ -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));

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

builtin/repack.c +0 -0

index c6a7943d5..9217fc832 100644

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 557

Quoted 9 lines

builtin/repack.c +6 -0

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

Quoted 14 lines

t/t5537-fetch-shallow.sh +1 -1

index 2d0031703..777c9d1dc 100755
@@ -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 &&

"Johannes Schindelin via GitGitGadget" <[email protected]>

t/t5537-fetch-shallow.sh EOF · line 209

Quoted 6 lines · writes:

+...

t/t5537-fetch-shallow.sh +5 -0

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

t/t5537-fetch-shallow.sh EOF · line 211

Quoted 2 lines

t/t5537-fetch-shallow.sh +1 -0

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

Quoted 6 lines

t/t5537-fetch-shallow.sh +4 -0

+	git -C shallow-client fetch --prune --depth=2 \
+		origin "+refs/heads/*:refs/remotes/origin/*"
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

Quoted 11 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>

t/t5537-fetch-shallow.sh EOF · line 209

Quoted 6 lines · writes:

+...

t/t5537-fetch-shallow.sh +5 -0

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

Quoted 3 lines

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

Quoted 16 lines

t/t5537-fetch-shallow.sh EOF · line 211

Quoted 2 lines

t/t5537-fetch-shallow.sh +1 -0

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

Quoted 6 lines

t/t5537-fetch-shallow.sh +4 -0

+	git -C shallow-client fetch --prune --depth=2 \
+		origin "+refs/heads/*:refs/remotes/origin/*"
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

Quoted 6 lines · 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.

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

Quoted 112 lines

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 106 lines · writes:

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/prune.c |  2 +-
 commit.h        |  2 +-
 shallow.c       | 22 +++++++++++++++++-----
 3 files changed, 19 insertions(+), 7 deletions(-)

builtin/prune.c +1 -1

index 41230f821..6d6ab6cf1 100644
@@ -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;
 }

commit.h +1 -1

index 1d260d62f..ff34447ab 100644
@@ -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);

shallow.c +17 -5

index 732e18d54..0a2671bc2 100644
@@ -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));

Quoted 10 lines · Junio C Hamano

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 7 lines · Johannes Schindelin

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.

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.

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.

Quoted 6 lines

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:

Quoted 2 lines

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/[email protected]/