From: Derrick Stolee <stolee@gmail.com> To: Duy Nguyen <pclouds@gmail.com> Cc: "Git Mailing List" <git@vger.kernel.org>, "Stefan Beller" <sbeller@google.com>, "Derrick Stolee" <dstolee@microsoft.com>, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, "Jonathan Nieder" <jrnieder@gmail.com>, "Jonathan Tan" <jonathantanmy@google.com>, "Martin Fick" <mfick@codeaurora.org> Subject: Re: [PATCH 06/23] midx: struct midxed_git and 'read' subcommand Date: Wed, 20 Jun 2018 09:13:36 -0400 Message-ID: <3827f4ce-7f78-6566-d9c8-1830afb3c09c@gmail.com> (raw) In-Reply-To: <CACsJy8A3qVRXrQ9YiiN3Ggrcc5XFJWaWRrTbdO8E_48yRmzrqQ@mail.gmail.com> On 6/7/2018 1:54 PM, Duy Nguyen wrote: > On Thu, Jun 7, 2018 at 4:03 PM, Derrick Stolee <stolee@gmail.com> wrote: >> As we build the multi-pack-index feature by adding chunks at a time, >> we want to test that the data is being written correctly. >> >> Create struct midxed_git to store an in-memory representation of a > A word play on 'packed_git'? Amusing. Some more descriptive name would > be better though. midxed looks almost like random letters thrown > together. I'll use 'struct multi_pack_index'. > >> multi-pack-index and a memory-map of the binary file. Initialize this >> struct in load_midxed_git(object_dir). >> +static int read_midx_file(const char *object_dir) >> +{ >> + struct midxed_git *m = load_midxed_git(object_dir); >> + >> + if (!m) >> + return 0; > This looks like an error case, please don't just return zero, > typically used to say "success". I don't know if this command stays > "for debugging purposes" until the end. Of course in that case it does > not really matter. It is intended for debugging and testing. Generally, it is not an error to not have a MIDX in an object directory. >> +struct midxed_git *load_midxed_git(const char *object_dir) >> +{ >> + struct midxed_git *m; >> + int fd; >> + struct stat st; >> + size_t midx_size; >> + void *midx_map; >> + const char *midx_name = get_midx_filename(object_dir); > mem leak? This function returns allocated memory if I remember correctly. > >> + >> + fd = git_open(midx_name); >> + if (fd < 0) >> + return NULL; > do an error_errno() so we know what went wrong at least. > >> + if (fstat(fd, &st)) { >> + close(fd); >> + return NULL; > same here, we should know why fstat() fails. > >> + } >> + midx_size = xsize_t(st.st_size); >> + >> + if (midx_size < MIDX_MIN_SIZE) { >> + close(fd); >> + die("multi-pack-index file %s is too small", midx_name); > _() > > The use of die() should be discouraged though. Many people still try > (or wish) to libify code and new die() does not help. I think error() > here would be enough then you can return NULL. Or you can go fancier > and store the error string in a strbuf like refs code. > >> + } >> + >> + midx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0); >> + >> + m = xcalloc(1, sizeof(*m) + strlen(object_dir) + 1); >> + strcpy(m->object_dir, object_dir); >> + m->data = midx_map; >> + >> + m->signature = get_be32(m->data); >> + if (m->signature != MIDX_SIGNATURE) { >> + error("multi-pack-index signature %X does not match signature %X", >> + m->signature, MIDX_SIGNATURE); > _(). Maybe 0x%08x instead of %x > >> + goto cleanup_fail; >> + } >> + >> + m->version = *(m->data + 4); > m->data[4] instead? shorter and easier to understand. > > Same comment on "*(m->data + x)" and error() without _() for the rest. > >> + if (m->version != MIDX_VERSION) { >> + error("multi-pack-index version %d not recognized", >> + m->version); > _() >> + goto cleanup_fail; >> + } >> + >> + m->hash_version = *(m->data + 5); > m->data[5] > >> +cleanup_fail: >> + FREE_AND_NULL(m); >> + munmap(midx_map, midx_size); >> + close(fd); >> + exit(1); > It's bad enough that you die() but exit() in this code seems too much. > Please just return NULL and let the caller handle the error. Will do. > >> diff --git a/midx.h b/midx.h >> index 3a63673952..a1d18ed991 100644 >> --- a/midx.h >> +++ b/midx.h >> @@ -1,4 +1,13 @@ >> +#ifndef MIDX_H >> +#define MIDX_H >> + >> +#include "git-compat-util.h" >> #include "cache.h" >> +#include "object-store.h" > I don't really think you need object-store here (git-compat-util.h > too). "struct mixed_git;" would be enough for load_midxed_git > declaration below. > >> #include "packfile.h" >> >> +struct midxed_git *load_midxed_git(const char *object_dir); >> + >> int write_midx_file(const char *object_dir); >> + >> +#endif >> diff --git a/object-store.h b/object-store.h >> index d683112fd7..77cb82621a 100644 >> --- a/object-store.h >> +++ b/object-store.h >> @@ -84,6 +84,25 @@ struct packed_git { >> char pack_name[FLEX_ARRAY]; /* more */ >> }; >> >> +struct midxed_git { >> + struct midxed_git *next; > Do we really have multiple midx files? There is one per object directory currently, but you may have one locally and one in each of your alternates. I do need to double-check that we populate this list later in the series. (And I'll remove it from this commit and save it for when it is needed.) > >> + >> + int fd; >> + >> + const unsigned char *data; >> + size_t data_len; >> + >> + uint32_t signature; >> + unsigned char version; >> + unsigned char hash_version; >> + unsigned char hash_len; >> + unsigned char num_chunks; >> + uint32_t num_packs; >> + uint32_t num_objects; >> + >> + char object_dir[FLEX_ARRAY]; > Why do you need to keep object_dir when it could be easily retrieved > when the repo is available? > >> +}; >> + >> struct raw_object_store { >> /* >> * Path to the repository's object store.
next prev parent reply other threads:[~2018-06-20 13:13 UTC|newest] Thread overview: 192+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-06-07 14:03 [PATCH 00/23] Multi-pack-index (MIDX) Derrick Stolee 2018-06-07 14:03 ` [PATCH 01/23] midx: add design document Derrick Stolee 2018-06-11 19:04 ` Stefan Beller 2018-06-18 18:48 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 02/23] midx: add midx format details to pack-format.txt Derrick Stolee 2018-06-11 19:19 ` Stefan Beller 2018-06-18 19:01 ` Derrick Stolee 2018-06-18 19:41 ` Stefan Beller 2018-06-07 14:03 ` [PATCH 03/23] midx: add midx builtin Derrick Stolee 2018-06-07 17:20 ` Duy Nguyen 2018-06-18 19:23 ` Derrick Stolee 2018-06-11 21:02 ` Stefan Beller 2018-06-18 19:40 ` Derrick Stolee 2018-06-18 19:55 ` Stefan Beller 2018-06-18 19:58 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 04/23] midx: add 'write' subcommand and basic wiring Derrick Stolee 2018-06-07 17:27 ` Duy Nguyen 2018-06-07 14:03 ` [PATCH 05/23] midx: write header information to lockfile Derrick Stolee 2018-06-07 17:35 ` Duy Nguyen 2018-06-12 15:00 ` Duy Nguyen 2018-06-19 12:54 ` Derrick Stolee 2018-06-19 14:59 ` Duy Nguyen 2018-06-19 15:24 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 06/23] midx: struct midxed_git and 'read' subcommand Derrick Stolee 2018-06-07 17:54 ` Duy Nguyen 2018-06-20 13:13 ` Derrick Stolee [this message] 2018-06-07 18:31 ` Duy Nguyen 2018-06-20 13:33 ` Derrick Stolee 2018-06-20 15:07 ` Duy Nguyen 2018-06-20 16:39 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 07/23] midx: expand test data Derrick Stolee 2018-06-07 14:03 ` [PATCH 08/23] midx: read packfiles from pack directory Derrick Stolee 2018-06-07 18:03 ` Duy Nguyen 2018-06-20 16:33 ` [PATCH] packfile: generalize pack directory list Derrick Stolee 2018-06-07 14:03 ` [PATCH 09/23] midx: write pack names in chunk Derrick Stolee 2018-06-07 18:26 ` Duy Nguyen 2018-06-21 15:25 ` Derrick Stolee 2018-06-21 17:38 ` Junio C Hamano 2018-06-22 18:25 ` Derrick Stolee 2018-06-22 18:31 ` Junio C Hamano 2018-06-22 18:32 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 10/23] midx: write a lookup into the pack names chunk Derrick Stolee 2018-06-09 16:43 ` Duy Nguyen 2018-06-21 17:23 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 11/23] midx: sort and deduplicate objects from packfiles Derrick Stolee 2018-06-09 17:07 ` Duy Nguyen 2018-06-21 17:54 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 12/23] midx: write object ids in a chunk Derrick Stolee 2018-06-09 17:25 ` Duy Nguyen 2018-06-07 14:03 ` [PATCH 13/23] midx: write object id fanout chunk Derrick Stolee 2018-06-09 17:28 ` Duy Nguyen 2018-06-21 19:49 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 14/23] midx: write object offsets Derrick Stolee 2018-06-09 17:41 ` Duy Nguyen 2018-06-07 14:03 ` [PATCH 15/23] midx: create core.midx config setting Derrick Stolee 2018-06-07 14:03 ` [PATCH 16/23] midx: prepare midxed_git struct Derrick Stolee 2018-06-09 17:47 ` Duy Nguyen 2018-06-07 14:03 ` [PATCH 17/23] midx: read objects from multi-pack-index Derrick Stolee 2018-06-09 17:56 ` Duy Nguyen 2018-06-21 20:03 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 18/23] midx: use midx in abbreviation calculations Derrick Stolee 2018-06-09 18:01 ` Duy Nguyen 2018-06-22 18:38 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 19/23] midx: use existing midx when writing new one Derrick Stolee 2018-06-07 14:03 ` [PATCH 20/23] midx: use midx in approximate_object_count Derrick Stolee 2018-06-09 18:03 ` Duy Nguyen 2018-06-22 18:39 ` Derrick Stolee 2018-06-07 14:03 ` [PATCH 21/23] midx: prevent duplicate packfile loads Derrick Stolee 2018-06-09 18:05 ` Duy Nguyen 2018-06-07 14:03 ` [PATCH 22/23] midx: use midx to find ref-deltas Derrick Stolee 2018-06-07 14:03 ` [PATCH 23/23] midx: clear midx on repack Derrick Stolee 2018-06-09 18:13 ` Duy Nguyen 2018-06-22 18:44 ` Derrick Stolee 2018-06-07 14:06 ` [PATCH 00/23] Multi-pack-index (MIDX) Derrick Stolee 2018-06-07 14:45 ` Ævar Arnfjörð Bjarmason 2018-06-07 14:54 ` Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 00/24] " Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 01/24] multi-pack-index: add design document Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 02/24] multi-pack-index: add format details Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 03/24] multi-pack-index: add builtin Derrick Stolee 2018-06-25 19:15 ` Junio C Hamano 2018-06-25 14:34 ` [PATCH v2 04/24] multi-pack-index: add 'write' verb Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 05/24] midx: write header information to lockfile Derrick Stolee 2018-06-25 19:19 ` Junio C Hamano 2018-07-05 19:13 ` Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 06/24] multi-pack-index: load into memory Derrick Stolee 2018-06-25 19:38 ` Junio C Hamano 2018-07-05 14:19 ` Derrick Stolee 2018-07-05 18:58 ` Eric Sunshine 2018-07-06 19:20 ` Junio C Hamano 2018-06-25 14:34 ` [PATCH v2 07/24] multi-pack-index: expand test data Derrick Stolee 2018-06-25 19:45 ` Junio C Hamano 2018-06-25 14:34 ` [PATCH v2 08/24] packfile: generalize pack directory list Derrick Stolee 2018-06-25 19:57 ` Junio C Hamano 2018-06-25 14:34 ` [PATCH v2 09/24] multi-pack-index: read packfile list Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 10/24] multi-pack-index: write pack names in chunk Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 11/24] midx: read pack names into array Derrick Stolee 2018-06-25 23:52 ` Eric Sunshine 2018-06-25 14:34 ` [PATCH v2 12/24] midx: sort and deduplicate objects from packfiles Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 13/24] midx: write object ids in a chunk Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 14/24] midx: write object id fanout chunk Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 15/24] midx: write object offsets Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 16/24] config: create core.multiPackIndex setting Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 17/24] midx: prepare midxed_git struct Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 18/24] midx: read objects from multi-pack-index Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 19/24] midx: use midx in abbreviation calculations Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 20/24] midx: use existing midx when writing new one Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 21/24] midx: use midx in approximate_object_count Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 22/24] midx: prevent duplicate packfile loads Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 23/24] packfile: skip loading index if in multi-pack-index Derrick Stolee 2018-06-25 14:34 ` [PATCH v2 24/24] midx: clear midx on repack Derrick Stolee 2018-07-06 0:52 ` [PATCH v3 00/24] Multi-pack-index (MIDX) Derrick Stolee 2018-07-06 0:52 ` [PATCH v3 01/24] multi-pack-index: add design document Derrick Stolee 2018-07-06 0:52 ` [PATCH v3 02/24] multi-pack-index: add format details Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 03/24] multi-pack-index: add builtin Derrick Stolee 2018-07-06 3:54 ` Eric Sunshine 2018-07-06 0:53 ` [PATCH v3 04/24] multi-pack-index: add 'write' verb Derrick Stolee 2018-07-06 4:07 ` Eric Sunshine 2018-07-06 0:53 ` [PATCH v3 05/24] midx: write header information to lockfile Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 06/24] multi-pack-index: load into memory Derrick Stolee 2018-07-06 4:19 ` Eric Sunshine 2018-07-06 5:18 ` Eric Sunshine 2018-07-09 19:08 ` Junio C Hamano 2018-07-12 16:06 ` Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 07/24] multi-pack-index: expand test data Derrick Stolee 2018-07-06 4:36 ` Eric Sunshine 2018-07-06 5:20 ` Eric Sunshine 2018-07-12 14:10 ` Derrick Stolee 2018-07-12 18:02 ` Eric Sunshine 2018-07-12 18:06 ` Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 08/24] packfile: generalize pack directory list Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 09/24] multi-pack-index: read packfile list Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 10/24] multi-pack-index: write pack names in chunk Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 11/24] midx: read pack names into array Derrick Stolee 2018-07-06 4:58 ` Eric Sunshine 2018-07-06 0:53 ` [PATCH v3 12/24] midx: sort and deduplicate objects from packfiles Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 13/24] midx: write object ids in a chunk Derrick Stolee 2018-07-06 5:04 ` Eric Sunshine 2018-07-06 0:53 ` [PATCH v3 14/24] midx: write object id fanout chunk Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 15/24] midx: write object offsets Derrick Stolee 2018-07-06 5:27 ` Eric Sunshine 2018-07-12 16:33 ` Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 16/24] config: create core.multiPackIndex setting Derrick Stolee 2018-07-06 5:39 ` Eric Sunshine 2018-07-12 13:19 ` Derrick Stolee 2018-07-12 16:30 ` Derrick Stolee 2018-07-11 9:48 ` SZEDER Gábor 2018-07-12 13:01 ` Derrick Stolee 2018-07-12 13:31 ` SZEDER Gábor 2018-07-12 15:40 ` Derrick Stolee 2018-07-12 17:29 ` Junio C Hamano 2018-07-06 0:53 ` [PATCH v3 17/24] midx: prepare midxed_git struct Derrick Stolee 2018-07-06 5:41 ` Eric Sunshine 2018-07-06 0:53 ` [PATCH v3 18/24] midx: read objects from multi-pack-index Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 19/24] midx: use midx in abbreviation calculations Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 20/24] midx: use existing midx when writing new one Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 21/24] midx: use midx in approximate_object_count Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 22/24] midx: prevent duplicate packfile loads Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 23/24] packfile: skip loading index if in multi-pack-index Derrick Stolee 2018-07-06 0:53 ` [PATCH v3 24/24] midx: clear midx on repack Derrick Stolee 2018-07-06 5:52 ` Eric Sunshine 2018-07-12 19:39 ` [PATCH v4 00/23] Multi-pack-index (MIDX) Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 01/23] multi-pack-index: add design document Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 02/23] multi-pack-index: add format details Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 03/23] multi-pack-index: add builtin Derrick Stolee 2018-07-20 18:22 ` Junio C Hamano 2018-07-20 22:15 ` brian m. carlson 2018-07-20 22:28 ` Junio C Hamano 2018-07-12 19:39 ` [PATCH v4 04/23] multi-pack-index: add 'write' verb Derrick Stolee 2018-07-12 22:56 ` Eric Sunshine 2018-07-12 19:39 ` [PATCH v4 05/23] midx: write header information to lockfile Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 06/23] multi-pack-index: load into memory Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 07/23] t5319: expand test data Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 08/23] packfile: generalize pack directory list Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 09/23] multi-pack-index: read packfile list Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 10/23] multi-pack-index: write pack names in chunk Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 11/23] midx: read pack names into array Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 12/23] midx: sort and deduplicate objects from packfiles Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 13/23] midx: write object ids in a chunk Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 14/23] midx: write object id fanout chunk Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 15/23] midx: write object offsets Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 16/23] config: create core.multiPackIndex setting Derrick Stolee 2018-07-12 21:05 ` Junio C Hamano 2018-07-13 0:50 ` Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 17/23] midx: read objects from multi-pack-index Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 18/23] midx: use midx in abbreviation calculations Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 19/23] midx: use existing midx when writing new one Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 20/23] midx: use midx in approximate_object_count Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 21/23] midx: prevent duplicate packfile loads Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 22/23] packfile: skip loading index if in multi-pack-index Derrick Stolee 2018-07-12 19:39 ` [PATCH v4 23/23] midx: clear midx on repack Derrick Stolee 2018-07-12 21:11 ` [PATCH v4 00/23] Multi-pack-index (MIDX) Junio C Hamano
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3827f4ce-7f78-6566-d9c8-1830afb3c09c@gmail.com \ --to=stolee@gmail.com \ --cc=avarab@gmail.com \ --cc=dstolee@microsoft.com \ --cc=git@vger.kernel.org \ --cc=jonathantanmy@google.com \ --cc=jrnieder@gmail.com \ --cc=mfick@codeaurora.org \ --cc=pclouds@gmail.com \ --cc=sbeller@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
git@vger.kernel.org list mirror (unofficial, one of many) This inbox may be cloned and mirrored by anyone: git clone --mirror https://public-inbox.org/git git clone --mirror http://ou63pmih66umazou.onion/git git clone --mirror http://czquwvybam4bgbro.onion/git git clone --mirror http://hjrcffqmbrq6wope.onion/git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V1 git git/ https://public-inbox.org/git \ git@vger.kernel.org public-inbox-index git Example config snippet for mirrors. Newsgroups are available over NNTP: nntp://news.public-inbox.org/inbox.comp.version-control.git nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git nntp://news.gmane.io/gmane.comp.version-control.git note: .onion URLs require Tor: https://www.torproject.org/ code repositories for the project(s) associated with this inbox: https://80x24.org/mirrors/git.git AGPL code for this site: git clone https://public-inbox.org/public-inbox.git