On Mon, Feb 03, 2025 at 09:40:43AM +0100, Patrick Steinhardt wrote:
> On Thu, Jan 30, 2025 at 12:08:22PM +0800, shejialuo wrote:
> > diff --git a/builtin/fsck.c b/builtin/fsck.c
> > index 7a4dcb0716..9a8613d07f 100644
> > --- a/builtin/fsck.c
> > +++ b/builtin/fsck.c
> > @@ -905,6 +905,34 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
> > return res;
> > }
> >
> > +static void fsck_refs(struct repository *r)
> > +{
> > + struct child_process refs_verify = CHILD_PROCESS_INIT;
> > + struct progress *progress = NULL;
> > + uint64_t progress_num = 1;
> > +
> > + if (show_progress)
> > + progress = start_progress(r, _("Checking ref database"),
> > + progress_num);
>
> Hm. I don't really think that this progress meter adds anything right
> now. It only shows either 0 or 1, so it basically only tells you when
> you're done. And that is something that the user can tell without a
> progress meter.
>
You are correct in the functionality part. Actually, my very initial
implementation is what you have said. I simply used the following way to
indicate the user that we are going to check ref database.
fprintf_ln(stderr, _("Checking ref database"));
However, it will break a test in "t/t1050-large.sh::fsck large blobs". I
cite the shell script below:
test_expect_success 'fsck large blobs' '
git fsck 2>err &&
test_must_be_empty err
'
> > +
> > + if (verbose)
> > + fprintf_ln(stderr, _("Checking ref database"));
> > +
That's the reason why we need to use `verbose` to control the behavior
here. Put it futhermore, We either use `process` or `verbose` to print
the message to the user. This is a pattern widely used in "git-fsck(1)".
For example "builtin/fsck.c::fsck_object_dir", we have the following
code:
if (verbose)
fprintf_ln(stderr, _("Checking object directory"));
if (show_progress)
progress = start_progress(the_repository,
_("Checking object directories"), 256);
So, that's why I use progress here. We need this to print the
information to the user. I have also tried to print to the stdout like
the following
fprintf_ln(stdout, _("Checking ref database"));
It will also break the test.
> > + 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;
> > +
> > + display_progress(progress, 1);
> > + stop_progress(&progress);
> > +}
> > +
> > static char const * const fsck_usage[] = {
> > N_("git fsck [--tags] [--root] [--unreachable] [--cache] [--no-reflogs]\n"
> > " [--[no-]full] [--strict] [--verbose] [--lost-found]\n"
> > @@ -970,6 +998,8 @@ int cmd_fsck(int argc,
> > git_config(git_fsck_config, &fsck_obj_options);
> > prepare_repo_settings(the_repository);
> >
> > + fsck_refs(the_repository);
>
> I think there needs to be a way to disable this. How about we add an
> option `--[no-]references` to do so? I was briefly wondering whether we
> also want to have `--only-references`, but if a user wants to do that
> they can simply execute `git refs verify` directly.
>
Good idea, let me improve this in the next version.
Thanks,
Jialuo
> Patrick
