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

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

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