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

This week's nits...

I found this harder to read than the previous patch, but I think it's
mostly because the existing code is already a bit tangled.  I think the
second item below is worth fixing, though.


Jeff King [off-list ref] writes:

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

+	off_t to_write;
+	int fd;
+
+	if (!is_pack_valid(reuse_packfile))
+		return 0;
+
+	fd = git_open_noatime(reuse_packfile->pack_name);
+	if (fd < 0)
+		return 0;
+
+	if (lseek(fd, sizeof(struct pack_header), SEEK_SET) == -1) {
+		close(fd);
+		return 0;
+	}

You do an error return if any of the syscalls in this routine fails, but
there is only one caller and it immediately dies:

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

+
+	if (reuse_packfile_offset < 0)
+		reuse_packfile_offset = reuse_packfile->pack_size - 20;
+
+	to_write = reuse_packfile_offset - sizeof(struct pack_header);
+
+	while (to_write) {
+		int read_pack = xread(fd, buffer, sizeof(buffer));
+
+		if (read_pack <= 0) {
+			close(fd);
+			return 0;

Similar to the above, but this one may also clobber the 'errno' during
close(), which can lead to misleading messages.

+		}
+
+		if (read_pack > to_write)
+			read_pack = to_write;
+
+		sha1write(f, buffer, read_pack);

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

+		to_write -= read_pack;
+	}
+
+	close(fd);
+	written += reuse_packfile_objects;
+	return reuse_packfile_offset - sizeof(struct pack_header);
+}

[...]

-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)
 {

[...]

-	for (p = packed_git; p; p = p->next) {
-		off_t offset = find_pack_entry_one(sha1, p);
-		if (offset) {
-			if (!found_pack) {
-				if (!is_pack_valid(p)) {
-					warning("packfile %s cannot be accessed", p->pack_name);
-					continue;
+	if (!found_pack) {
+		for (p = packed_git; p; p = p->next) {
+			off_t offset = find_pack_entry_one(sha1, p);
+			if (offset) {
+				if (!found_pack) {
+					if (!is_pack_valid(p)) {
+						warning("packfile %s cannot be accessed", p->pack_name);
+						continue;
+					}
+					found_offset = offset;
+					found_pack = p;
 				}
-				found_offset = offset;
-				found_pack = p;
+				if (exclude)
+					break;
+				if (incremental)
+					return 0;
+				if (local && !p->pack_local)
+					return 0;
+				if (ignore_packed_keep && p->pack_local && p->pack_keep)
+					return 0;
 			}
-			if (exclude)
-				break;
-			if (incremental)
-				return 0;
-			if (local && !p->pack_local)
-				return 0;
-			if (ignore_packed_keep && p->pack_local && p->pack_keep)
-				return 0;
 		}
 	}

This function makes my head spin, and you're indenting it yet another
level.

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

-- 
Thomas Rast
tr@thomasrast.ch

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

quoted

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

quoted

-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