Re: [PATCH 2/4] builtin/pack-objects.c: support `--max-pack-size` with `--cruft`

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 29, 2023, 17:52 UTC
Message-ID: <ZO4wZdaR/I06n+E8@nand.local>
In-Reply-To: <xmqqr0nld9u9.fsf@gitster.g>

On Tue, Aug 29, 2023 at 10:42:06AM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
>
> > When pack-objects learned the `--cruft` option back in b757353676
> > (builtin/pack-objects.c: --cruft without expiration, 2022-05-20), we
> > explicitly forbade `--cruft` with `--max-pack-size`.
> >
> > At the time, there was no specific rationale given in the patch for not
> > supporting the `--max-pack-size` option with `--cruft`. (As best I can
> > remember, it's because we were trying to push users towards only ever
> > having a single cruft pack, but I cannot be sure).
>
> I am reasonably sure it was the case but then I do not recall we
> ever discussing how the second cruft pack gets consolidated into one
> by combining it with the existing one.

Yeah. We write the combined cruft pack just like we would any other, and record each packed object's most recent mtime available from either:

  - a loose copy of that object, if one exists
  - the mtime of the .pack file for any packed copies of that object
    which may exist
  - the mtime of that object as recorded in an .mtimes file (if that
    file was packed as cruft).

Show 22 quoted lines

> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index 868efe7e0f..a046994a43 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -1190,8 +1190,7 @@ static void write_pack_file(void)
> >  		unsigned char hash[GIT_MAX_RAWSZ];
> >  		char *pack_tmp_name = NULL;
> >
> > -		if (pack_to_stdout)
> > -			f = hashfd_throughput(1, "<stdout>", progress_state);
> > +		if (pack_to_stdout) f = hashfd_throughput(1, "<stdout>", progress_state);
> >  		else
> >  			f = create_tmp_packfile(&pack_tmp_name);
>
> An unintended change, I am sure ;-)
>
> It is very surprising that absolutely no real change is needed to
> allow cruft packs to honor the settings, other than removing the
> seemingly artificial inter-option-compatibility roadblocks (all
> hunks for it omitted above as they were trivially obvious).  I am
> sure the first hunk to fold an "if" statement onto a single line is
> not what makes the feature to actually work ;-)

Hah, this made me laugh. Indeed, a whitespace change around this if-statement is not the make-or-break change we needed to make this feature work!

I'm happy to clean this up and resubmit it, but you may have already done so, in which case I'll leave this as-is.

Thanks, Taylor