Re: [PATCH 1/3] prepare_packed_git(): find more garbage

From: Jeff King <peff@peff.net>
Date: Dec 15, 2015, 23:09 UTC
Message-ID: <20151215230957.GA30353@sigill.intra.peff.net>
In-Reply-To: <1448518529-2659-1-git-send-email-dougk.ff7@gmail.com>

On Thu, Nov 26, 2015 at 12:15:29AM -0600, Doug Kelly wrote:

Show 14 quoted lines

> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index ba92919..5197b57 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -17,19 +17,15 @@ static off_t loose_size;
>  
>  static const char *bits_to_msg(unsigned seen_bits)
>  {
> -	switch (seen_bits) {
> -	case 0:
> -		return "no corresponding .idx or .pack";
> -	case PACKDIR_FILE_GARBAGE:
> +	if (seen_bits ==  PACKDIR_FILE_GARBAGE)
>  		return "garbage found";

It seems weird to use "==" on a bitfield. I think it is the case now that we would never see GARBAGE alongside anything else, but I wonder if we should future-proof that as:

  if (seen_bits & PACKDIR_FILE_GARBAGE)

Specifically, I am wondering what would happen if we had "foo.pack" and "foo.bogus", where we do not know about the latter at all.

Show 13 quoted lines

> -	case PACKDIR_FILE_PACK:
> +	else if (seen_bits & PACKDIR_FILE_PACK && !(seen_bits & PACKDIR_FILE_IDX))
>  		return "no corresponding .idx";
> -	case PACKDIR_FILE_IDX:
> +	else if (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & PACKDIR_FILE_PACK))
>  		return "no corresponding .pack";
> -	case PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:
> -	default:
> -		return NULL;
> -	}
> +	else if (seen_bits == 0 || !(seen_bits & (PACKDIR_FILE_IDX|PACKDIR_FILE_PACK)))
> +		return "no corresponding .idx or .pack";
> +	return NULL;

This bottom conditional is interesting. I understand the second half: we saw something pack-like, but there is not matching .idx or .pack at all (if we saw one but not the other, we would have caught it above).

But when will we get an empty seen_bits? What did we see that triggered this function, but didn't trigger a bit (even GARBAGE)?

I don't mind if the answer is "nothing, this is future-proofing", but am mostly curious.

Show 17 quoted lines

> diff --git a/sha1_file.c b/sha1_file.c
> index 3d56746..5f939e4 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,
>  	if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
>  		return;
>  
> +	if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
> +		return;
> +
> +	if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
> +		return;
> +
> +	if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
> +		return;
> +

It seems like we're enumerating a lot of cases here that will explode if we get even one more file type (e.g., we add "pack-XXX.foo" in the future). If I understand this function correctly, we're just trying to get rid of "boring" cases that do not need to be reported.

Isn't any case that has both a pack and an idx boring (no matter if it has a .bitmap or .keep)?

IOW, can these four conditionals just become:

  unsigned pack_with_idx = PACKDIR_FILE_PACK | PACKDIR_FILE_IDX;

  if ((seen_bits & pack_with_idx) == pack_with_idx)
	return;

-Peff