Re: [PATCH v3 1/3] midx: teach "git multi-pack-index repack" honor "git repack" configurations

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2020, 16:51 UTC
Message-ID: <xmqq7dxlvypv.fsf@gitster.c.googlers.com>
In-Reply-To: <a925307d4c57506f5236e60dc1390998e186cf26.1589034270.git.gitgitgadget@gmail.com>

"Son Luong Ngoc via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

> From: Son Luong Ngoc <sluongng@gmail.com>
>
> Previously, when the "repack" subcommand of "git multi-pack-index" command
> creates new packfile(s), it does not call the "git repack" command but
> instead directly calls the "git pack-objects" command, and the
> configuration variables meant for the "git repack" command, like
> "repack.usedaeltabaseoffset", are ignored.

When we talk about the current state of the code (i.e. before applying this patch), we do not say "previously". It's not like you are complaining about a recent breakage, e.g. "previously X worked like this but since change Y, it instead works like that, which breaks Z".

> This patch ensured "git multi-pack-index" checks the configuration
> variables used by "git repack" and passes the corresponding options to
> the underlying "git pack-objects" command.

We write this part in imperative mood, as if we are giving an order to the codebase to "become like so". We do not give an observation about the patch or the author ("This patch does X, this patch also does Y", "I do X, I do Y").

Taking these two together, perhaps like:

    When the "repack" subcommand of "git multi-pack-index" command
    creates new packfile(s), it does not call the "git repack"
    command but instead directly calls the "git pack-objects"
    command, and the configuration variables meant for the "git
    repack" command, like "repack.usedaeltabaseoffset", are ignored.

    Check the configuration variables used by "git repack" ourselves
    in "git multi-index-pack" and pass the corresponding options to
    underlying "git pack-objects".

> Note that `repack.writeBitmaps` configuration is ignored, as the
> pack bitmap facility is useful only with a single packfile.

Good.

> +	int delta_base_offset = 1;
> +	int use_delta_islands = 0;

These give the default values for two configurations and over there builtin/repack.c has these lines:

    17	static int delta_base_offset = 1;
    18	static int pack_kept_objects = -1;
    19	static int write_bitmaps = -1;
    20	static int use_delta_islands;
    21	static char *packdir, *packtmp;

When somebody is tempted to update these to change the default used by "git repack", it should be easy to notice that such a change must be accompanied by a matching change to the lines you are introducing in this patch, or we'll be out of sync.

The easiest way to avoid such a problem may be to stop bypassing "git repack" and calling "pack-objects" ourselves. That is the reason why the configuration variables honored by "git repack" are ignored in this codepath in the first place. But that is not the approach we are taking, so we need a reasonable way to tell those who update this file and builtin/repack.c to make matching changes. At the very least, perhaps we should give a comment above these two lines in this file, e.g.

	/*
	 * when updating the default for these configuration
	 * variables in builtin/repack.c, these must be adjusted
	 * to match.
	 */
	int delta_base_offset = 1;
	int use_delta_islands = 0;

or something like that.

With that, the rest of the patch makes sense.

Thanks.