Re: [PATCH v3 0/2] index-pack: fsck honor checks

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 26, 2024, 21:18 UTC
Message-ID: <xmqqfryjn686.fsf@gitster.g>
In-Reply-To: <pull.1658.v3.git.git.1706302749.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

>  1:  b3b3e8bd0bf ! 1:  cdf7fc7fe8a index-pack: test and document --strict=<msg>
>      @@ Metadata
>       Author: John Cai <johncai86@gmail.com>
>       
>        ## Commit message ##
>      -    index-pack: test and document --strict=<msg>
>      +    index-pack: test and document --strict=<msg-id>=<severity>...

Ah, I missed this one.  Nice spotting.

Show 8 quoted lines

>           5d477a334a (fsck (receive-pack): allow demoting errors to warnings,
>           2015-06-22) allowed a list of fsck msg to downgrade to be passed to
>      @@ Commit message
>           directly, (nor use index-pack for that matter) it is still useful to
>           document and test this feature.
>       
>      +    Reviewed-by: Christian Couder <christian.couder@gmail.com>
>           Signed-off-by: John Cai <johncai86@gmail.com>

I haven't seen Christian involved (by getting Cc'ed these patches, sending out review comments, or giving his Reviewed-by:) during these three rounds of this topic. I'll wait until I hear from him before queuing this, just to be safe.

Show 6 quoted lines

>      ++	Die, if the pack contains broken objects or links. An optional
>      ++	comma-separated list of `<msg-id>=<severity>` can be passed to change
>      ++	the severity of some possible issues, e.g.,
>      ++	 `--strict="missingEmail=ignore,badTagName=error"`. See the entry for the
>      ++	`fsck.<msg-id>` configuration options in linkgit:git-fsck[1] for more
>      ++	information on the possible values of `<msg-id>` and `<severity>`.

This is much better than the tentative text I tweaked.  Nice.

Show 5 quoted lines

>      ++--fsck-objects[=<msg-id>=<severity>...]::
>      ++	Die if the pack contains broken objects. If the pack contains a tree
>      ++	pointing to a .gitmodules blob that does not exist, prints the hash of
>      ++	that blob (for the caller to check) after the hash that goes into the
>      ++	name of the pack/idx file (see "Notes").

Not a new problem bit I have to wonder what happens if the pack contains many trees that point at different blobs for ".gitmodules" path and many of these blobs are not included in the packfile? Will the caller receive all of these blob object names so that they can be verified? The reference to the "Notes" only refer to the fact that usually a single hash value that is used in constructing the name of the packfile "pack-<Hashvalue>.pack" is emitted to the standard output, which is not wrong per se, but does not help readers very much wrt to understanding this.

[jc: dragging JTan into the thread, as this comes from his 5476e1ef (fetch-pack: print and use dangling .gitmodules, 2021-02-22)].

>        +
>      ++Unlike `--strict` however, don't choke on broken links. An optional

You'd need a comma on both sides of "however" used like this, I think.

In any case, I thought your original construction to have this "unlike" immediately after "die on broken objects" was far easier to follow.

Thanks.