| Message ID | a808fbdf31afc9ad9ba0ab27ce889e5a2d1a01ae.1611098616.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | repack: support repacking into a geometric sequence | expand |
On 1/19/2021 6:24 PM, Taylor Blau wrote:
> 'git repack -g' will have to learn about unreachable loose objects that
This reference to the '-g' option is one patch too early. Perhaps
say
An upcoming patch will introduce geometric repacking. This will
require removing unreachable loose objects in a separate path
from the existing checks.
or similar?
Thanks,
-Stolee
On Wed, Jan 20, 2021 at 08:59:48AM -0500, Derrick Stolee wrote: > On 1/19/2021 6:24 PM, Taylor Blau wrote: > > 'git repack -g' will have to learn about unreachable loose objects that > > This reference to the '-g' option is one patch too early. Perhaps > say > > An upcoming patch will introduce geometric repacking. This will > require removing unreachable loose objects in a separate path > from the existing checks. > > or similar? Mmm. I had imagined that this would be read either in the context of this series, or by someone in the future long after 'git repack -g' had been introduced. I could see that it's confusing, though, and I do agree your wording makes clearer that the option doesn't exist yet. I'm happy to send a replacement or reroll if you feel strongly, but in either case I'll wait for a little more review first. Thanks, Taylor
On 1/20/2021 9:34 AM, Taylor Blau wrote: > On Wed, Jan 20, 2021 at 08:59:48AM -0500, Derrick Stolee wrote: >> On 1/19/2021 6:24 PM, Taylor Blau wrote: >>> 'git repack -g' will have to learn about unreachable loose objects that >> >> This reference to the '-g' option is one patch too early. Perhaps >> say >> >> An upcoming patch will introduce geometric repacking. This will >> require removing unreachable loose objects in a separate path >> from the existing checks. >> >> or similar? > > Mmm. I had imagined that this would be read either in the context of > this series, or by someone in the future long after 'git repack -g' had > been introduced. > > I could see that it's confusing, though, and I do agree your wording > makes clearer that the option doesn't exist yet. > > I'm happy to send a replacement or reroll if you feel strongly, but in > either case I'll wait for a little more review first. Definitely don't rush a re-roll for my nit-picks. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > On 1/19/2021 6:24 PM, Taylor Blau wrote: >> 'git repack -g' will have to learn about unreachable loose objects that > > This reference to the '-g' option is one patch too early. Perhaps > say > > An upcoming patch will introduce geometric repacking. This will > require removing unreachable loose objects in a separate path > from the existing checks. > > or similar? Yeah, sounds like a trivially obvious improvement to me. It does not matter to reviewers who are very well aware that the series is about adding "repack -g", but it may end up being confusing when somebody tries to see what commit the feature was added later when the help from the cover letter is not available.
diff --git a/builtin/repack.c b/builtin/repack.c index 279be11a16..664863111b 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -298,6 +298,27 @@ static void repack_promisor_objects(const struct pack_objects_args *args, #define ALL_INTO_ONE 1 #define LOOSEN_UNREACHABLE 2 +static void handle_loose_and_reachable(struct child_process *cmd, + const char *unpack_unreachable, + int pack_everything, + int keep_unreachable) +{ + if (unpack_unreachable) { + strvec_pushf(&cmd->args, + "--unpack-unreachable=%s", + unpack_unreachable); + strvec_push(&cmd->env_array, "GIT_REF_PARANOIA=1"); + } else if (pack_everything & LOOSEN_UNREACHABLE) { + strvec_push(&cmd->args, + "--unpack-unreachable"); + } else if (keep_unreachable) { + strvec_push(&cmd->args, "--keep-unreachable"); + strvec_push(&cmd->args, "--pack-loose-unreachable"); + } else { + strvec_push(&cmd->env_array, "GIT_REF_PARANOIA=1"); + } +} + int cmd_repack(int argc, const char **argv, const char *prefix) { struct child_process cmd = CHILD_PROCESS_INIT; @@ -414,22 +435,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix) repack_promisor_objects(&po_args, &names); - if (existing_packs.nr && delete_redundant) { - if (unpack_unreachable) { - strvec_pushf(&cmd.args, - "--unpack-unreachable=%s", - unpack_unreachable); - strvec_push(&cmd.env_array, "GIT_REF_PARANOIA=1"); - } else if (pack_everything & LOOSEN_UNREACHABLE) { - strvec_push(&cmd.args, - "--unpack-unreachable"); - } else if (keep_unreachable) { - strvec_push(&cmd.args, "--keep-unreachable"); - strvec_push(&cmd.args, "--pack-loose-unreachable"); - } else { - strvec_push(&cmd.env_array, "GIT_REF_PARANOIA=1"); - } - } + if (existing_packs.nr && delete_redundant) + handle_loose_and_reachable(&cmd, unpack_unreachable, + pack_everything, + keep_unreachable); } else { strvec_push(&cmd.args, "--unpacked"); strvec_push(&cmd.args, "--incremental");
'git repack -g' will have to learn about unreachable loose objects that need to be removed in a separate path from the existing checks. Extract that check into a function so it can be called from multiple places. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/repack.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)