Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too

[Junio C Hamano <gitster@pobox.com>]

Kirill Smelkov <kirr@nexedi.com> writes:

> > From: Kirill Smelkov <kirr@nexedi.com>
> Subject: [PATCH 1/2] pack-objects: Make sure use_bitmap_index is not active under
>  --local or --honor-pack-keep
>
> Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
> are two codepaths in pack-objects: with & without using bitmap
> reachability index.
>
> However add_object_entry_from_bitmap(), despite its non-bitmapped
> counterpart add_object_entry(), in no way does check for whether --local
> or --honor-pack-keep should be respected. In non-bitmapped codepath this
> is handled in want_object_in_pack(), but bitmapped codepath has simply
> no such checking at all.
>
> The bitmapped codepath however was allowing to pass --local and
> --honor-pack-keep and bitmap indices were still used under such
> conditions - potentially giving wrong output (including objects from
> non-local or .keep'ed pack).
>
> Instead of fixing bitmapped codepath to respect those options, since
> currently no one actually need or use them in combination with bitmaps,
> let's just force use_bitmap_index=0 when any of --local or
> --honor-pack-keep are used and add appropriate comment about
> not-checking for those in add_object_entry_from_bitmap()
>
> Suggested-by: Jeff King <peff@peff.net>
> ---
>  builtin/pack-objects.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 15866d7..d7cf782 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1055,6 +1055,12 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
>  	if (have_duplicate_entry(sha1, 0, &index_pos))
>  		return 0;
>  
> +	/*
> +	 * for simplicity we always want object to be in pack, as
> +	 * use_bitmap_index codepath assumes neither --local nor --honor-pack-keep
> +	 * is active.
> +	 */

I am not sure this comment is useful to readers.

Unless the readers are comparing add_object_entry() and this
function and wondering why this side lacks a check here, iow, when
they are merely following from a caller of this function through
this function down to its callee to understand what goes on, this
comment would not help them and only confuse them.

If we were to say something to help those who are comparing these
two functions, I think we should be more explicit, i.e.

    The caller disables use-bitmap-index when --local or
    --honor-pack-keep options are in effect because bitmap code is
    not prepared to handle them.  Because the control does not reach
    here if these options are in effect, the check with
    want_object_in_pack() to skip objects is not done.

or something like that.

Or is the rest of the bitmap codepath prepared to handle these
options and it is just the matter of adding the missing check with
want_object_in_pack() here to make it work correctly?

>  	create_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack, offset);
>  
>  	display_progress(progress_state, nr_result);
> @@ -2776,6 +2782,15 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  	if (!use_internal_rev_list || !pack_to_stdout || is_repository_shallow())
>  		use_bitmap_index = 0;
>  
> +	/*
> +	 * "lazy" reasons not to use bitmaps; it is easier to reason about when
> +	 * neither --local nor --honor-pack-keep is in action, and so far no one
> +	 * needed nor implemented such support yet.
> +	 */

Justifying comment like this is a good idea, but the comment above
does not make it very clear that this is a correctness fix, i.e. if
we do not disable, the code will do a wrong thing.

The other logic to disable use of bitmap we can see in the
pre-context would also benefit from some description as to why;
6b8fda2d (pack-objects: use bitmaps when packing objects,
2013-12-21) didn't do a very good job in that---the reason is not
clear in its log message, either.

> +	if (local || ignore_packed_keep)
> +		use_bitmap_index = 0;
> +
> +

I see one extra blank line here ;-)

>  	if (pack_to_stdout || !rev_list_all)
>  		write_bitmap_index = 0;

Thanks.