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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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
>