On Tue, Mar 29, 2022 at 08:59:24AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> writes:
>
> > I'm not sure... if we did:
> >
> > --- 8< ---
> >
> > diff --git a/setup.c b/setup.c
> > index 04ce33cdcd..fa54c9baa4 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -565,2 +565,4 @@ static enum extension_result handle_extension(const char *var,
> > return EXTENSION_OK;
> > + } else if (!strcmp(ext, "cruftpacks")) {
> > + return EXTENSION_OK;
> > }
> >
> > --- >8 ---
> >
> > but nothing more, then a hypothetical `extensions.cruftPacks` could be
> > used to prevent older writers in a mixed version environment. But if you
> > don't have or care about older versions of Git, you can avoid setting it
> > altogether.
>
> Smells like "unsafe by default, but you can opt into safety", which
> is backwards, isn't it?
I see it a little differently. The default (not writing cruft packs at
all) is safe, even in a mixed-version environment. If a user (a) wants
to use cruft packs, and (b) has older versions of Git also gc'ing the
repository, and (c) can't get rid of them, _then_ an opt-in extension
would make it impossible for those older versions to interact with the
repository.
I still can't shake the feeling that this is a pretty fringe and
timing-dependent scenario, which at worst keeps too many unreachable
objects around.
But I think this in conjunction with the already opt-in nature of cruft
packs would be a nice way to create safeguards for the situation
Jonathan described. There may be a simpler way, but I'm not sure I see
it (i.e., if you control whether or not `--cruft` is passed when
doing maintenance with newer versions of Git, but not whether older
versions are running around doing their own maintenance, then an
extension would be necessary to lock the old versions out).
> > But cruft packs are already guarded in two places:
> >
> > - `git repack` won't write a cruft pack unless given the `--cruft`
> > flag (i.e., `git repack -A` doesn't suddenly start generating cruft
> > packs upon upgrade).
> >
> > - `git gc` won't write cruft packs unless the `gc.cruftPacks`
> > configuration is set, or `--cruft` is given as a flag.
>
> Hmph, OK. So individuals can sort-of protect from hurting
> themselves by refraining from running these with --cruft or writing
> --cruft in their maintenance scripts. An organization that wants to
> let the more adventurous types to early opt-in can prepare two
> versions of the maintenance scripts they distribute to their users,
> one with and the other without --cruft, and use the mechanism they
> use for gradual rollouts to control the population. Perhaps that
> would make sufficient protection? I dunno.
>
> Jonathan, what do you think?
I'm confused: if newer versions of Git are writing cruft packs, then
having the older versions gc'ing in the same repository runs into the
same scenario Jonathan originally describes.
The thing I think Jonathan seeks to prevent is older versions of Git
gc'ing a repo that has cruft packs. I think I may need you to clarify a
little, sorry :-(.
> > I'd be curious what Jonathan and others think of that approach (which,
> > to be clear, is what this series already implements). We could make it
> > clear to say:
> >
> > If you have mixed versions of Git which both repack a repository
> > (either manually or by auto-GC / background maintenance), consider
> > leaving `gc.cruftPacks` unset and avoiding passing `--cruft` as a
> > command-line argument to `git repack` and `git gc`, since doing so
> > can lead to [...]
>
> That message is (depending on what comes in [...]) much more helpful
> than just throwing a word "mtime" out and letting the reader figure
> out the rest ;-)
Yes, totally agreed.
Thanks,
Taylor
