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@xxxxxxxx> 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
> 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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html