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

From: John Cai <johncai86@gmail.com>
Date: Jan 26, 2024, 22:11 UTC
Message-ID: <BF772E83-2BFE-4652-A742-67FADF3D8FE2@gmail.com>
In-Reply-To: <xmqqfryjn686.fsf@gitster.g>

Hi Junio,

On 26 Jan 2024, at 16:18, Junio C Hamano wrote:

Show 25 quoted lines

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>>  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.
>
>>           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.

Christian was involved on an off-list review of this patch series. You can see it in [1].

1. https://gitlab.com/gitlab-org/git/-/merge_requests/88

Show 28 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.
>
>>      ++--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)].

sounds good, will wait for some clarification here

Show 6 quoted lines

>
>>        +
>>      ++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.

good catch

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

I'll reformulate this to be clearer. From the previous version I realized I didn't take into account the pre-existing "Die if the pack contains broken objects" block so I put it at the beginning. But now I think you're right in that the "Unlike..." comes too late.

>
> Thanks.