On 5/6/25 3:39 PM, Derrick Stolee wrote:
On 5/2/25 7:21 PM, Taylor Blau wrote:
On Mon, Mar 24, 2025 at 03:22:38PM +0000, Derrick Stolee via GitGitGadget wrote:
+static int add_objects_by_path(const char *path,
+ struct oid_array *oids,
+ enum object_type type,
+ void *data)
+{
+ struct object_entry **delta_list;
+ size_t oe_start = to_pack.nr_objects;
+ size_t oe_end;
+ unsigned int sub_list_size;
Could you help me understand why sub_list_size is an unsigned int and
not a size_t here?
This is based on a similar "list_size" used in ll_find_deltas(), but
since we are starting from scratch here it would be better to use
size_t.
Sorry for the delay, but the find_deltas() method called in both
places expects unsigned ints, so we are stuck with this until we
decide to upgrade it in both places.
+ unsigned int *processed = data;
+
+ /*
+ * First, add all objects to the packing data, including the ones
+ * marked UNINTERESTING (translated to 'exclude') as they can be
+ * used as delta bases.
+ */
+ for (size_t i = 0; i < oids->nr; i++) {
+ int exclude;
+ struct object_info oi = OBJECT_INFO_INIT;
+ struct object_id *oid = &oids->oid[i];
+
+ /* Skip objects that do not exist locally. */
+ if (exclude_promisor_objects &&
+ oid_object_info_extended(the_repository, oid, &oi,
+ OBJECT_INFO_FOR_PREFETCH) < 0)
+ continue;
+
+ exclude = !is_oid_interesting(the_repository, oid);
+
+ if (exclude && !thin)
+ continue;
...here we only skip calling add_object_entry() if the object is
excluded (which lookup_object() returning NULL would cause above) *and*
we're not using thin packs. That makes sense, since if we have a missing
object, but we're using thin packs, then it's OK to skip.
So I think all of this looks good. However, I find the interface here a
little awkward. The function is called "is_oid_interesting()", but it
determines that fact by checking whether or not the object has the
UNINTERSTING bit set on its flag, and then negating the result. But then
we negate it again here at the caller to obtain whether or not we should
"exclude" the object.
I was wondering if there were other callers of is_oid_interesting() that
don't negate its result. But it doesn't look like there are (at least in
this patch). I wonder if it would be cleaner just to inline this instead
of having the result negated at multiple levels.
Double-checked now and there seem to be no other callers, so this will be
renamed to the opposite case.
+ return 0;
+}
+
+static void get_object_list_path_walk(struct rev_info *revs)
+{
+ struct path_walk_info info = PATH_WALK_INFO_INIT;
+ unsigned int processed = 0;
+
+ info.revs = revs;
+ info.path_fn = add_objects_by_path;
+ info.path_fn_data = &processed;
+ revs->tag_objects = 1;
What is the purpose of setting revs->tag_objects to 1 here? Do we need
to restore it back to its original value before returning? A comment
here would be useful, I think.
I think this is not necessary and should be removed.
Thanks,
-Stolee
