From: Taylor Blau <me@ttaylorr.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] repack: add config to skip updating server info
Date: Fri, 11 Mar 2022 11:20:41 -0500 [thread overview]
Message-ID: <Yit22Xcs6iF4MVB7@nand.local> (raw)
In-Reply-To: <659d5528df56f6b9aece6b1f3c4e2e5a4ae04e1e.1646996936.git.ps@pks.im>
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
next prev parent reply other threads:[~2022-03-11 16:20 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-11 11:09 [PATCH] repack: add config to skip updating server info Patrick Steinhardt
2022-03-11 16:20 ` Taylor Blau [this message]
2022-03-14 7:19 ` Patrick Steinhardt
2022-03-14 7:42 ` [PATCH v2 0/2] " Patrick Steinhardt
2022-03-14 7:42 ` [PATCH v2 1/2] repack: refactor to avoid double-negation of update-server-info Patrick Steinhardt
2022-03-14 7:42 ` [PATCH v2 2/2] repack: add config to skip updating server info Patrick Steinhardt
2022-03-14 22:26 ` 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yit22Xcs6iF4MVB7@nand.local \
--to=me@ttaylorr.com \
--cc=git@vger.kernel.org \
--cc=ps@pks.im \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).