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

From: Thomas Rast <tr@thomasrast.ch>
Date: Dec 7, 2013, 15:47 UTC
Message-ID: <87zjock6if.fsf@linux-1gf2.Speedport_W723_V_Typ_A_1_00_098>
In-Reply-To: <20131114124510.GK10757@sigill.intra.peff.net>

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 <peff@peff.net> 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.

Show 14 quoted lines

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

Show 12 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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

[...]

Show 6 quoted lines

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

[...]

Show 40 quoted lines

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

Show 22 quoted lines

>  	entry = packlist_alloc(&to_pack, sha1, index_pos);
> -	entry->hash = hash;
> +	entry->hash = name_hash;
>  	if (type)
>  		entry->type = type;
>  	if (exclude)
>  		entry->preferred_base = 1;
>  	else
>  		nr_result++;
> +
> +	if (flags & OBJECT_ENTRY_NO_TRY_DELTA)
> +		entry->no_try_delta = 1;
> +
>  	if (found_pack) {
>  		entry->in_pack = found_pack;
>  		entry->in_pack_offset = found_offset;
> @@ -859,10 +932,21 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
>  
>  	display_progress(progress_state, to_pack.nr_objects);
>  
> +	return 1;
> +}

-- 
Thomas Rast
tr@thomasrast.ch