Re: [GSoC][PATCH v14 09/11] builtin/fsck: add `git-refs verify` child process

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4021 bytes --]

On Tue, Aug 06, 2024 at 08:42:23AM +0800, shejialuo wrote:
> On Mon, Aug 05, 2024 at 12:38:43PM -0700, Junio C Hamano wrote:
> > shejialuo <shejialuo@gmail.com> writes:
> > 
> > > I agree with you that it would be strange if we do not expose any
> > > interfaces for user who are adventurous. Actually we may simply add an
> > > option "--refs-experimental" or simply "--refs" to allow the users check
> > > ref consistency by using "git-fsck(1)".
> > >
> > > I guess the concern that Patrick cares about is that we ONLY make refs
> > > optional here, but do not provide options for other checks. It will be
> > > strange from this perspective.
> > 
> > I do not care about strange all that much.  I however care about new
> > complexity in the code, complexity that is not taken advantage of
> > and is not exercised.  
> > 
> > You said
> > 
> > > From the development of this series, we can know the main problem is
> > > that fsck error message is highly coupled with the object checks.
> > 
> > and even if it is true and we have problem in fsck code paths, we
> > cannot see if _your_ solution to that problem is a good one without
> > having the code that exercises your additional code.
> > 
> > But if "git refs verify" does exercise all the new code paths (and
> > the refactored code that existed before this series, sitting now in
> > different places), then I do not have to worry about it.  My question
> > was primarily to extract "even though we do not wire this up to fsck,
> > we already have another code paths that uses all these changes" out
> > of you.
> > 
> 
> I understand what you mean here. I can say that "git refs verify" only
> exercises a part of the new code paths. The main reason why this series
> changes a lot of "fsck.[ch]" is that I want to expose the
> "fsck_report_ref" interface to report refs-related errors. So I guess
> this part should be covered.
> 
> At the current implementation, we change the "fsck.[ch]" for the
> following three things:
> 
> 1. Refactor "report" to use "fsck_vreport"
> 2. Create "fsck_report_ref" for refs check.
> 3. Do some simple renames to distinguish between refs and objects.
> 
> We do cover the second case in "git refs verify". But sadly, the first
> case and third case are covered in "git-fsck(1)". So, "git refs verify"
> does not exercise the refactored code.
> 
> However, I am a little confused. Actually, in the implementation, refs
> check and objects check are independent.
> 
> I think we should wire up "git refs verify" to "git-fsck(1)". Because we
> have no way to exercise the above case 1 and 3. If we do not, we will
> bring a lot of complexity here.

I don't think that is necessary. Basically, what you are saying is that
we seem to be testing only the new refs-related reporting that you have
introduced via your refactorings, but not the preexisting objects
related functionality.

That isn't true though. The object-specific reporting functions that you
have refactored already had callers before, and it still has callers now
because you adapted those accordingly. You can assume that those callers
already had tests before your refactorings, and as no tests broken you
can be reasonably sure that your refactorings are sound for the object
related code.

Furthermore, you do exercise the new ref-related parts via a couple of
tests that exercise `git refs verify`. Consequently, all parts of the
refactoring are covered by either old or new tests, and we should be
good here.

So even though we do not exercise the ref-related parts via git-fsck(1),
it is still subjected to tests. Eventually, when we start calling `git
refs verify` in git-fsck(1), we'd introduce more tests that verify that
the integration of those two commands works as expected, as well.

So, to summarize: the refactored functionality is both used and tested
and I think it's sensible to defer the integration of git-fsck(1) and
git-refs(1).

Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]