Re: [PATCH v3 01/17] Documentation/technical: add cruft-packs.txt
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 30, 2022, 02:23 UTC
- Message-ID
- <YkO/P3xgGYmhAz2O@nand.local>
- In-Reply-To
- <xmqqa6d8yckj.fsf@gitster.g>
On Tue, Mar 29, 2022 at 08:59:24AM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> Taylor Blau <me@ttaylorr.com> 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).
Show 19 quoted lines
> > 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 :-(.
Show 13 quoted lines
> > 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