Re: [PATCH v3 11/21] pack-objects: use bitmaps when packing objects

[Jeff King <peff@peff.net>]

On Sat, Dec 07, 2013 at 04:47:20PM +0100, Thomas Rast wrote:

> > +static off_t write_reused_pack(struct sha1file *f)
> > +{
> > +	uint8_t buffer[8192];
> 
> We usually just call this 'unsigned char'.  I can see why this would be
> more portable, but git would already fall apart badly on an architecture
> where char is not 8 bits.

I think it's worth switching just for consistency with the rest of git.
Fixed.

> } +			packfile_size = write_reused_pack(f);
> } +			if (!packfile_size)
> } +				die_errno("failed to re-use existing pack");
> 
> So if you just died here, when the error happens, you could take the
> chance to tell the user _which_ syscall failed.

Yeah, agreed (and especially the fact that we may get bogus errno
values). Fixed.

> Not your fault, but sha1write() is an odd function -- it purportedly is
> 
>   int sha1write(struct sha1file *f, const void *buf, unsigned int count);
> 
> but it can only return 0.  This goes back all the way to c38138c
> (git-pack-objects: write the pack files with a SHA1 csum, 2005-06-26).

It looks like there's exactly one site that checks its return value, and
it's just to die. We should drop the return value from sha1write
entirely to make it clear that it dies on error. But that's orthogonal
to this series.

> > -static int add_object_entry(const unsigned char *sha1, enum object_type type,
> > -			    const char *name, int exclude)
> > +static int add_object_entry_1(const unsigned char *sha1, enum object_type type,
> > +			      int flags, uint32_t name_hash,
> > +			      struct packed_git *found_pack, off_t found_offset)
> [...]
> This function makes my head spin, and you're indenting it yet another
> level.

Yeah. In addition, the use of the "flags" here is somewhat questionable.
We use them for internal values in the call from add_object_entry to
add_object_entry_1. But we also pass the latter as a traversal callback,
meaning that what it would get in "flags" is totally different. It's not
actually a bug in the current code, since the bitmap traversal always
passes empty flags, but it's still rather confusing.

> If it's not too much work, can you split it into the three parts that it
> really is?  IIUC it boils down to
> 
>   do we have this already?
>       possibly apply 'exclude', then return
>   are we coming from a call path that doesn't tell us which pack to take
>   it from?
>       find _all_ instances in packs
>       check if any of them are local .keep packs
>           if so, return
>   construct a packlist entry to taste

I did this split. By itself, I was on the fence, as there are actually
some interdependencies between the three parts that make it hairy.

But then I realized that instead of making the weird relationship
between add_object_entry and add_object_entry_1, we can simply make a
new function that composes the functions differently.

So we get:

+static int add_object_entry_from_bitmap(const unsigned char *sha1,
+					enum object_type type,
+					int flags, uint32_t name_hash,
+					struct packed_git *pack, off_t offset)
+{
+	uint32_t index_pos;
+
+	if (have_duplicate_entry(sha1, 0, &index_pos))
+		return 0;
+
+	create_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack, offset);
+
+	display_progress(progress_state, to_pack.nr_objects);
+	return 1;
+}
[...]
+	traverse_bitmap_commit_list(&add_object_entry_from_bitmap);

which makes much more sense.

-Peff