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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 14, 2018, 21:56 UTC
Message-ID: <nycvar.QRO.7.76.6.1807142351100.75@tvgsbejvaqbjf.bet>
In-Reply-To: <20180713203140.GB17670@sigill.intra.peff.net>

Hi Peff,

On Fri, 13 Jul 2018, Jeff King wrote:

Show 24 quoted lines

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

Show 17 quoted lines

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

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

Show 5 quoted 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?

Show 5 quoted lines

>   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