Re: [PATCH 2/4] split-index; stop abusing the `base_oid` to strip the "link" extension
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 22, 2023, 21:24 UTC
- Message-ID
- <xmqqjzz8cwdp.fsf@gitster.g>
- In-Reply-To
- <f1897b880729b649ab24da14cbc3432d44b7c731.1679500859.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de> > > When a split-index is in effect, the `$GIT_DIR/index` file needs to > contain a "link" extension that contains all the information about the > split-index, including the information about the shared index. > ... > Let's stop zeroing out the `base_oid` to indicate that the "link" > extension should not be written.
Nicely explained.
> One might be tempted to simply call `discard_split_index()` instead, > under the assumption that Git decided to write a non-split index and > therefore the the `split_index` structure might no longer be wanted.
"the the".
Show 5 quoted lines
> +enum strip_extensions {
> + WRITE_ALL_EXTENSIONS = 0,
> + STRIP_ALL_EXTENSIONS = 1,
> + STRIP_LINK_EXTENSION_ONLY = 2
> +};We do not need to spell out the specific values for this enum; the users' (i.e. the callers of do_write_index()) sole requirement is for these symbols to have different values.
Also do we envision that (1) we would need to keep STRIP_LINK_ONLY to be with the largest value among the enum values, or (2) we would never add new value to the set? Otherwise let's end the last one with a trailing comma.
Looking at the way strip_extensions variable is used in do_write_index(), an alternative design might be to make it a set of bits (e.g. unsigned write_extension) and give one bit to each extension. But such a clean-up is better left outside the topic, I would imagine, as we do not have any need to skip an arbitrary set of extensions right now.
Show 6 quoted lines
> +/* > + * Write the Git index into a `.lock` file > + * > + * If `strip_link_extension` is non-zero, avoid writing any "link" extension > + * (used by the split-index feature). > + */
Not exposing "enum strip_extensions" to the caller of this function, like this patch does, is probably a very safe and sensible thing to do. We do not have a reason to allow its callers to (perhaps mistakenly) pass STRIP_ALL_EXTENSIONS to it.
Show 15 quoted lines
> static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,
> - unsigned flags)
> + unsigned flags, int strip_link_extension)
> {
> int ret;
> int was_full = istate->sparse_index == INDEX_EXPANDED;
> @@ -3185,7 +3197,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
> */
> trace2_region_enter_printf("index", "do_write_index", the_repository,
> "%s", get_lock_file_path(lock));
> - ret = do_write_index(istate, lock->tempfile, 0, flags);
> + ret = do_write_index(istate, lock->tempfile, strip_link_extension ? STRIP_LINK_EXTENSION_ONLY : 0, flags);
> trace2_region_leave_printf("index", "do_write_index", the_repository,
> "%s", get_lock_file_path(lock));
> OK.
Very nicely done.