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

shejialuo [off-list ref] writes:

quoted

quoted

+static void fsck_refs(void)
+{
+	struct child_process refs_verify = CHILD_PROCESS_INIT;
+	child_process_init(&refs_verify);
+	refs_verify.git_cmd = 1;
+	strvec_pushl(&refs_verify.args, "refs", "verify", NULL);
+	if (verbose)
+		strvec_push(&refs_verify.args, "--verbose");
+	if (check_strict)
+		strvec_push(&refs_verify.args, "--strict");
+
+	if (run_command(&refs_verify))
+		errors_found |= ERROR_REFS;
+}

Okay. I think that it's sensible to execute this as part of git-fsck(1).
But do we want to provide an option to disable this new check, as well?

It does feel a bit like opening a can of worms, though. None of the
other checks have trivial ways to disable them, and git-fsck(1) is
gaining more and more checks. So if we can disable ref checks, we may
also want to have options to disable checks for objects, connectivity,
reverse indices, indices, commit graphs and whatnot. In other words, in
my opinion we need to think a bit bigger and design a proper UI around
this.

But I don't think that should happen as part of this commit series, as
it is already big enough. So either we just accept this patch as-is. Or
we evict it from this series and handle it in the future together with
all the other taks that one may potentially want to disable.

I'd rather pick option two.

After talking with Patrick offline, we decide to drop this patch. At
current, we should put this change slowly for the user. Because many
people use "git-fsck(1)", currently we don't have a way to disable ref
checks by default. It's a little beyond this series.

We may consider later.

Hmph, I am fine with the approach to take it slower.

BUT.

Here is what the diffstat for the whole thing in the updated round
v15 looked like.  Do most of these changes outside refs/ still
needed if we do not give any way to even optionally enable it via
"fsck" for those who feel adventurous?  Should we still be touching
fsck.[ch] and other fsck related infrastructure in the series?

 Documentation/fsck-msgids.txt |   6 ++
 Documentation/git-refs.txt    |  13 +++++
 builtin/fsck.c                |  17 +++---
 builtin/mktag.c               |   3 +-
 builtin/refs.c                |  34 +++++++++++
 fsck.c                        | 127 +++++++++++++++++++++++++++++++++---------
 fsck.h                        |  76 +++++++++++++++++++------
 object-file.c                 |   9 ++-
 refs.c                        |   5 ++
 refs.h                        |   8 +++
 refs/debug.c                  |  11 ++++
 refs/files-backend.c          | 116 +++++++++++++++++++++++++++++++++++++-
 refs/packed-backend.c         |   8 +++
 refs/refs-internal.h          |   6 ++
 refs/reftable-backend.c       |   8 +++
 t/t0602-reffiles-fsck.sh      |  92 ++++++++++++++++++++++++++++++
 16 files changed, 480 insertions(+), 59 deletions(-)

Hmph, I am fine with the approach to take it slower.

BUT.

Here is what the diffstat for the whole thing in the updated round
v15 looked like.  Do most of these changes outside refs/ still
needed if we do not give any way to even optionally enable it via
"fsck" for those who feel adventurous?  Should we still be touching
fsck.[ch] and other fsck related infrastructure in the series?

From my current standing, regardless of whether we run the "git refs
verify" subprocess inside "git-fsck(1)", we must change the fsck
infrastructure illustrated by the following:

From the development of this series, we can know the main problem is
that fsck error message is highly coupled with the object checks. Even
if we don't perform "git refs verify" in "git-fsck(1)", we still need to
follow the fsck msg framework when executing "git refs verify". We
cannot avoid this.

The content we change for "fsck.[ch]" is mainly the fsck msg part. We do
not change anything about the objects.

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.

 Documentation/fsck-msgids.txt |   6 ++
 Documentation/git-refs.txt    |  13 +++++
 builtin/fsck.c                |  17 +++---
 builtin/mktag.c               |   3 +-
 builtin/refs.c                |  34 +++++++++++
 fsck.c                        | 127 +++++++++++++++++++++++++++++++++---------
 fsck.h                        |  76 +++++++++++++++++++------
 object-file.c                 |   9 ++-
 refs.c                        |   5 ++
 refs.h                        |   8 +++
 refs/debug.c                  |  11 ++++
 refs/files-backend.c          | 116 +++++++++++++++++++++++++++++++++++++-
 refs/packed-backend.c         |   8 +++
 refs/refs-internal.h          |   6 ++
 refs/reftable-backend.c       |   8 +++
 t/t0602-reffiles-fsck.sh      |  92 ++++++++++++++++++++++++++++++
 16 files changed, 480 insertions(+), 59 deletions(-)