Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too
From: Junio C Hamano <hidden>
Date: 2016-07-27 20:40:45
Possibly related (same subject, not in this thread)
- 2016-07-29 · Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too · Kirill Smelkov <hidden>
- 2016-07-28 · Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too · Junio C Hamano <hidden>
- 2016-07-27 · Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too · Kirill Smelkov <hidden>
- 2016-07-25 · Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too · Jeff King <hidden>
- 2016-07-25 · Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too · Jeff King <hidden>
Kirill Smelkov [off-list ref] writes:
quoted hunk
quoted
From: Kirill Smelkov <redacted>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 <redacted> --- 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?
quoted hunk
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.