From: Jeff King <peff@peff.net>
Date: Thu, 03 Nov 2011 03:33:25 GMT
Subject: Re: [PATCH] fsck: print progress
Message-ID: <20111103033325.GA10230@sigill.intra.peff.net>
In-Reply-To: <1320290513-8878-1-git-send-email-pclouds@gmail.com>

On Thu, Nov 03, 2011 at 10:21:53AM +0700, Nguyen Thai Ngoc Duy wrote:

> fsck is usually a long process and it would be nice if it prints
> progress from time to time.

The output looks good to me. Code looks sane overall, with one comment:

> +		for (i = 1, p = packed_git; p; p = p->next, i++) {
> +			if (show_progress) {
> +				char buf[32];
> +				snprintf(buf, sizeof(buf), "Verifying pack %d/%d",
> +					 i, nr_packs);
> +				if (open_pack_index(p))
> +					continue;
> +				progress = start_progress(buf, p->num_objects);
> +			}
>  			/* verify gives error messages itself */
> -			verify_pack(p);
> +			verify_pack(p, progress);
> +
> +			/*
> +			 * we do not stop progress here, let the next
> +			 * progress line overwrite the current one for
> +			 * the next pack.
> +			 */
> +		}
> +		stop_progress(&progress);

We're actually leaking some memory here, since stop_progress will also
free() the progress object and any associated resources. It's not a lot,
but it's kind of ugly.

Perhaps there should be a special version of stop_progress that handles
this better? Or perhaps we could even come up with a total object count
before starting.  I guess it would involve mapping each pack index
simultaneously, though by my reading of the code, I think we do that
anyway (the opened index is cached in the pack object).

-Peff