Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size
Jeff King writes: > On Wed, Jan 22, 2014 at 08:06:42PM -0500, Jeff King wrote: > >> But I think there is a subtle problem. Here (and elsewhere) we use the >> parsed value of "0" as a sentinel. I think that is OK for >> --max-pack-size, where "0" is not a reasonable value. But git-repack(1) >> says: >> >> --window-memory=0 makes memory usage unlimited, which is the default. >> >> What does: >> >> git config pack.windowMemory 256m >> git repack --window-memory=0 >> >> do? It should override the config, but I think it does not with your >> patch (nor with the current code). Using a string would fix that (though >> you could also fix it by using a different sentinel, like ULONG_MAX). > > Here is a series that does that (and fixes the other issue I found). It > would probably be nice to test these things, but checking that they > actually had an impact is tricky (how do you know that --window-memory > did the right thing?). > > [1/3]: repack: fix typo in max-pack-size option > [2/3]: repack: make parsed string options const-correct > [3/3]: repack: propagate pack-objects options as strings > > -Peff Sounds sensible; will chuck the hum-ulong series and replace with these patches. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size
On Wed, Jan 22, 2014 at 08:06:42PM -0500, Jeff King wrote: > But I think there is a subtle problem. Here (and elsewhere) we use the > parsed value of "0" as a sentinel. I think that is OK for > --max-pack-size, where "0" is not a reasonable value. But git-repack(1) > says: > > --window-memory=0 makes memory usage unlimited, which is the default. > > What does: > > git config pack.windowMemory 256m > git repack --window-memory=0 > > do? It should override the config, but I think it does not with your > patch (nor with the current code). Using a string would fix that (though > you could also fix it by using a different sentinel, like ULONG_MAX). Here is a series that does that (and fixes the other issue I found). It would probably be nice to test these things, but checking that they actually had an impact is tricky (how do you know that --window-memory did the right thing?). [1/3]: repack: fix typo in max-pack-size option [2/3]: repack: make parsed string options const-correct [3/3]: repack: propagate pack-objects options as strings -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size
On Wed, Jan 22, 2014 at 11:58:05AM -0800, Junio C Hamano wrote: > These quantities can be larger than an int. Use ulong to express > them like the underlying pack-objects does, and also parse them with > the human-friendly unit suffixes. Hrm. I think that is a valid strategy, but... > - int max_pack_size = 0; > + unsigned long max_pack_size = 0, window_memory = 0; Here we must use the correct C type... > - OPT_INTEGER(0, "window-memory", &window_memory, > + OPT_HUM_ULONG(0, "window-memory", &window_memory, And here use the correct parser... > if (window_memory) > - argv_array_pushf(&cmd_args, "--window-memory=%u", > window_memory); > + argv_array_pushf(&cmd_args, "--window-memory=%lu", > window_memory); And here use the correct format string... All of which must match what pack-objects does, or we risk a further break (though I do not guess it will change from ulong anytime soon). The original shell version worked because they were all strings. We do not care about the numeric value here, and are just forwarding the value along to pack-objects. Why not just use a string? The only advantage I can think of is that this gives us slightly earlier error detection for "git repack --window-memory=bogosity". But I think there is a subtle problem. Here (and elsewhere) we use the parsed value of "0" as a sentinel. I think that is OK for --max-pack-size, where "0" is not a reasonable value. But git-repack(1) says: --window-memory=0 makes memory usage unlimited, which is the default. What does: git config pack.windowMemory 256m git repack --window-memory=0 do? It should override the config, but I think it does not with your patch (nor with the current code). Using a string would fix that (though you could also fix it by using a different sentinel, like ULONG_MAX). > if (max_pack_size) > - argv_array_pushf(&cmd_args, "--max_pack_size=%u", > max_pack_size); > + argv_array_pushf(&cmd_args, "--max_pack_size=%lu", > max_pack_size); These underscores are interesting: $ git pack-objects --max_pack_size=256m error: unknown option `max_pack_size=256m' I get the feeling the test suite does not cover this feature very well. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] repack: accept larger window-memory and max-pack-size
These quantities can be larger than an int. Use ulong to express
them like the underlying pack-objects does, and also parse them with
the human-friendly unit suffixes.
Signed-off-by: Junio C Hamano
---
builtin/repack.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index a0ff5c7..8ce396b 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -130,9 +130,9 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
int pack_everything = 0;
int delete_redundant = 0;
char *unpack_unreachable = NULL;
- int window = 0, window_memory = 0;
+ int window = 0;
int depth = 0;
- int max_pack_size = 0;
+ unsigned long max_pack_size = 0, window_memory = 0;
int no_reuse_delta = 0, no_reuse_object = 0;
int no_update_server_info = 0;
int quiet = 0;
@@ -159,11 +159,11 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
N_("with -A, do not loosen objects older than
this")),
OPT_INTEGER(0, "window", &window,
N_("size of the window used for delta
compression")),
- OPT_INTEGER(0, "window-memory", &window_memory,
+ OPT_HUM_ULONG(0, "window-memory", &window_memory,
N_("same as the above, but limit memory size
instead of entries count")),
OPT_INTEGER(0, "depth", &depth,
N_("limits the maximum delta depth")),
- OPT_INTEGER(0, "max-pack-size", &max_pack_size,
+ OPT_HUM_ULONG(0, "max-pack-size", &max_pack_size,
N_("maximum size of each packfile")),
OPT_END()
};
@@ -187,11 +187,11 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
if (window)
argv_array_pushf(&cmd_args, "--window=%u", window);
if (window_memory)
- argv_array_pushf(&cmd_args, "--window-memory=%u",
window_memory);
+ argv_array_pushf(&cmd_args, "--window-memory=%lu",
window_memory);
if (depth)
argv_array_pushf(&cmd_args, "--depth=%u", depth);
if (max_pack_size)
- argv_array_pushf(&cmd_args, "--max_pack_size=%u",
max_pack_size);
+ argv_array_pushf(&cmd_args, "--max_pack_size=%lu",
max_pack_size);
if (no_reuse_delta)
argv_array_pushf(&cmd_args, "--no-reuse-delta");
if (no_reuse_object)
--
1.9-rc0-151-ga5225c0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
