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

[Jeff King <peff@peff.net>]

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

> 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.

> -	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.

> 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