| Message ID | 659d5528df56f6b9aece6b1f3c4e2e5a4ae04e1e.1646996936.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | repack: add config to skip updating server info | expand |
On Fri, Mar 11, 2022 at 12:09:30PM +0100, Patrick Steinhardt wrote: > diff --git a/Documentation/config/repack.txt b/Documentation/config/repack.txt > index 9c413e177e..22bfc26afc 100644 > --- a/Documentation/config/repack.txt > +++ b/Documentation/config/repack.txt > @@ -25,3 +25,6 @@ repack.writeBitmaps:: > space and extra time spent on the initial repack. This has > no effect if multiple packfiles are created. > Defaults to true on bare repos, false otherwise. > + > +repack.updateServerInfo:: > + If set to false, git-repack will not run git-update-server-info. Can you clarify here what the default value of this config variable is, and how it interacts with repack's `-n` flag? E.g., something along the lines of: repack.updateServerInfo:: If set to false, linkgit:git-repack[1] will not run linkgit:git-update-serve-info[1]. Defaults to true. Can be overridden when true by the `-n` option of linkgit:git-repack[1]. Perhaps a little verbose, but I think it leaves less ambiguity about what this new configuration variable is for. > diff --git a/builtin/repack.c b/builtin/repack.c > index da1e364a75..3baa993da2 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -22,6 +22,7 @@ static int delta_base_offset = 1; > static int pack_kept_objects = -1; > static int write_bitmaps = -1; > static int use_delta_islands; > +static int no_update_server_info = 0; Not the fault of this patch, but I wonder if this would be less confusing if we stored `update_server_info` instead of `no_update_server_info`. If you have time, I think it may be worth a preparatory patch at the beginning to swap the two. > +test_expect_success 'updates server info by default' ' > + git init repo && > + test_when_finished "rm -rf repo" && > + test_commit -C repo message && > + test_path_is_missing repo/.git/objects/info/packs && > + test_path_is_missing repo/.git/info/refs && > + git -C repo repack && > + test_path_is_file repo/.git/objects/info/packs && > + test_path_is_file repo/.git/info/refs > +' I wonder if this and the below tests might be cleaned up with a pair of helper functions, perhaps: test_server_info_present () { test_path_is_file .git/objects/info/packs && test_path_is_file .git/info/refs } test_server_info_missing () { test_path_is_missing .git/objects/info/packs && test_path_is_missing .git/info/refs } t7700 has a mix of styles, but it may shorten some of the lines to use a subshell that is changed into the repo directory, e.g., the test above would become: test_expect_success 'updates server info by default' ' git init repo && test_when_finished "rm -fr repo" && ( test_commit message && test_server_info_missing && git repack && test_server_info_present ) ' which reads a little more easily to me. It would be nice to avoid creating the sub-repos at all, perhaps by removing these files ourselves in between tests. > +test_expect_success '-n skips updating server info' ' > +test_expect_success 'repack.updateServerInfo=true updates server info' ' > +test_expect_success 'repack.updateServerInfo=false skips updating server info' ' > +test_expect_success '-n overrides repack.updateServerInfo=true' ' Great, these four and the above together cover all of the cases I think we'd be interested in. Thanks, Taylor
On Fri, Mar 11, 2022 at 11:20:41AM -0500, Taylor Blau wrote: > On Fri, Mar 11, 2022 at 12:09:30PM +0100, Patrick Steinhardt wrote: > > diff --git a/Documentation/config/repack.txt b/Documentation/config/repack.txt > > index 9c413e177e..22bfc26afc 100644 > > --- a/Documentation/config/repack.txt > > +++ b/Documentation/config/repack.txt > > @@ -25,3 +25,6 @@ repack.writeBitmaps:: > > space and extra time spent on the initial repack. This has > > no effect if multiple packfiles are created. > > Defaults to true on bare repos, false otherwise. > > + > > +repack.updateServerInfo:: > > + If set to false, git-repack will not run git-update-server-info. > > Can you clarify here what the default value of this config variable is, > and how it interacts with repack's `-n` flag? E.g., something along the > lines of: > > repack.updateServerInfo:: > If set to false, linkgit:git-repack[1] will not run > linkgit:git-update-serve-info[1]. Defaults to true. Can be > overridden when true by the `-n` option of > linkgit:git-repack[1]. > > Perhaps a little verbose, but I think it leaves less ambiguity about > what this new configuration variable is for. Makes sense. > > diff --git a/builtin/repack.c b/builtin/repack.c > > index da1e364a75..3baa993da2 100644 > > --- a/builtin/repack.c > > +++ b/builtin/repack.c > > @@ -22,6 +22,7 @@ static int delta_base_offset = 1; > > static int pack_kept_objects = -1; > > static int write_bitmaps = -1; > > static int use_delta_islands; > > +static int no_update_server_info = 0; > > Not the fault of this patch, but I wonder if this would be less > confusing if we stored `update_server_info` instead of > `no_update_server_info`. If you have time, I think it may be worth a > preparatory patch at the beginning to swap the two. [snip] I indeed first had a look at how to do this, but didn't find a negated `OPT_BOOL()`. I had another look now though, and it seems like this is typically solved via `OPT_NEGBIT()`. Thanks! Patrick
diff --git a/Documentation/config/repack.txt b/Documentation/config/repack.txt index 9c413e177e..22bfc26afc 100644 --- a/Documentation/config/repack.txt +++ b/Documentation/config/repack.txt @@ -25,3 +25,6 @@ repack.writeBitmaps:: space and extra time spent on the initial repack. This has no effect if multiple packfiles are created. Defaults to true on bare repos, false otherwise. + +repack.updateServerInfo:: + If set to false, git-repack will not run git-update-server-info. diff --git a/builtin/repack.c b/builtin/repack.c index da1e364a75..3baa993da2 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -22,6 +22,7 @@ static int delta_base_offset = 1; static int pack_kept_objects = -1; static int write_bitmaps = -1; static int use_delta_islands; +static int no_update_server_info = 0; static char *packdir, *packtmp_name, *packtmp; static const char *const git_repack_usage[] = { @@ -54,6 +55,10 @@ static int repack_config(const char *var, const char *value, void *cb) use_delta_islands = git_config_bool(var, value); return 0; } + if (strcmp(var, "repack.updateserverinfo") == 0) { + no_update_server_info = !git_config_bool(var, value); + return 0; + } return git_default_config(var, value, cb); } @@ -620,7 +625,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix) const char *unpack_unreachable = NULL; int keep_unreachable = 0; struct string_list keep_pack_list = STRING_LIST_INIT_NODUP; - int no_update_server_info = 0; struct pack_objects_args po_args = {NULL}; int geometric_factor = 0; int write_midx = 0; diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 5922fb5bdd..f049120022 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -381,4 +381,51 @@ test_expect_success TTY '--quiet disables progress' ' test_must_be_empty stderr ' +test_expect_success 'updates server info by default' ' + git init repo && + test_when_finished "rm -rf repo" && + test_commit -C repo message && + test_path_is_missing repo/.git/objects/info/packs && + test_path_is_missing repo/.git/info/refs && + git -C repo repack && + test_path_is_file repo/.git/objects/info/packs && + test_path_is_file repo/.git/info/refs +' + +test_expect_success '-n skips updating server info' ' + git init repo && + test_when_finished "rm -rf repo" && + test_commit -C repo message && + git -C repo repack -n && + test_path_is_missing repo/.git/objects/info/packs && + test_path_is_missing repo/.git/info/refs +' + +test_expect_success 'repack.updateServerInfo=true updates server info' ' + git init repo && + test_when_finished "rm -rf repo" && + test_commit -C repo message && + git -C repo -c repack.updateServerInfo=true repack && + test_path_is_file repo/.git/objects/info/packs && + test_path_is_file repo/.git/info/refs +' + +test_expect_success 'repack.updateServerInfo=false skips updating server info' ' + git init repo && + test_when_finished "rm -rf repo" && + test_commit -C repo message && + git -C repo -c repack.updateServerInfo=false repack && + test_path_is_missing repo/.git/objects/info/packs && + test_path_is_missing repo/.git/info/refs +' + +test_expect_success '-n overrides repack.updateServerInfo=true' ' + git init repo && + test_when_finished "rm -rf repo" && + test_commit -C repo message && + git -C repo -c repack.updateServerInfo=true repack -n && + test_path_is_missing repo/.git/objects/info/packs && + test_path_is_missing repo/.git/info/refs +' + test_done
By default, git-repack(1) will update server info that is required by the dumb HTTP transport. This can be skipped by passing the `-n` flag, but what we're noticably missing is a config option to permanently disable updating this information. Add a new option "repack.updateServerInfo" which can be used to disable the logic. Most hosting providers have turned off the dumb HTTP protocol anyway, and on the client-side it woudln't typically be useful either. Giving a persistent way to disable this feature thus makes quite some sense to avoid wasting compute cycles and storage. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/config/repack.txt | 3 +++ builtin/repack.c | 6 ++++- t/t7700-repack.sh | 47 +++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-)