* [PATCH 0/9] remove unnecessary if statement
@ 2025-03-18 11:58 Usman Akinyemi
2025-03-18 11:58 ` [PATCH 1/9] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
` (9 more replies)
0 siblings, 10 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:58 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo
In an earlier patch[1] which has been merged to the master,
We checked `repo` is not NULL before making call to `repo_config()`.
Later, in another patch series[2] which has been merged to next,
`repo_config()` was taught to allow `repo` to be NULL.
So there is not need for checking if the `repo` is NULL before calling
repo_config() in the earlier patch.
Note, I have already sent the first 8 patches in [2] but, the 9th
patch depends on the first patch of that series.
[1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/
[2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/
Usman Akinyemi (9):
config: teach repo_config to allow `repo` to be NULL
builtin/verify-tag: stop using `the_repository`
builtin/verify-commit: stop using `the_repository`
builtin/send-pack: stop using `the_repository`
builtin/pack-refs: stop using `the_repository`
builtin/ls-files: stop using `the_repository`
builtin/for-each-ref: stop using `the_repository`
builtin/checkout-index: stop using `the_repository`
builtin/update-server-info: remove unnecessary if statement
builtin/checkout-index.c | 43 ++++++++++++++++-----------------
builtin/for-each-ref.c | 5 ++--
builtin/ls-files.c | 32 ++++++++++++------------
builtin/pack-refs.c | 8 +++---
builtin/send-pack.c | 7 +++---
builtin/update-server-info.c | 4 +--
builtin/verify-commit.c | 13 +++++-----
builtin/verify-tag.c | 7 +++---
config.c | 4 +++
config.h | 9 +++++++
t/t0610-reftable-basics.sh | 7 ++++++
t/t2006-checkout-index-basic.sh | 7 ++++++
t/t3004-ls-files-basic.sh | 7 ++++++
t/t5400-send-pack.sh | 7 ++++++
t/t6300-for-each-ref.sh | 7 ++++++
t/t7030-verify-tag.sh | 7 ++++++
t/t7510-signed-commit.sh | 7 ++++++
17 files changed, 118 insertions(+), 63 deletions(-)
--
2.48.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/9] config: teach repo_config to allow `repo` to be NULL
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
@ 2025-03-18 11:58 ` Usman Akinyemi
2025-03-20 7:21 ` Patrick Steinhardt
2025-03-18 11:58 ` [PATCH 2/9] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
` (8 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:58 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
The `repo` value can be NULL if a builtin command is run outside
any repository. The current implementation of `repo_config()` will
fail if `repo` is NULL.
If the `repo` is NULL the `repo_config()` can ignore the repository
configuration but it should read the other configuration sources like
the system-side configuration instead of failing.
Teach the `repo_config()` to allow `repo` to be NULL by calling the
`read_very_early_config()` which read config but only enumerate system
and global settings.
This will be useful in the following commits.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
config.c | 4 ++++
config.h | 9 +++++++++
2 files changed, 13 insertions(+)
diff --git a/config.c b/config.c
index 658569af08..e127afaa8f 100644
--- a/config.c
+++ b/config.c
@@ -2521,6 +2521,10 @@ void repo_config_clear(struct repository *repo)
void repo_config(struct repository *repo, config_fn_t fn, void *data)
{
+ if (!repo) {
+ read_very_early_config(fn, data);
+ return;
+ }
git_config_check_init(repo);
configset_iter(repo->config, fn, data);
}
diff --git a/config.h b/config.h
index 5c730c4f89..29a0277483 100644
--- a/config.h
+++ b/config.h
@@ -219,6 +219,15 @@ void read_very_early_config(config_fn_t cb, void *data);
* repo-specific one; by overwriting, the higher-priority repo-specific
* value is left at the end).
*
+ * In cases where the repository variable is NULL, repo_config() will
+ * skip the per-repository config but retain system and global configs
+ * by calling read_very_early_config() which also ignores one-time
+ * overrides like "git -c var=val". This is to support handling "git foo -h"
+ * (which lets git.c:run_builtin() to pass NULL and have the cmd_foo()
+ * call repo_config() before calling parse_options() to notice "-h", give
+ * help and exit) for a command that ordinarily require a repository
+ * so this limitation may be OK (but if needed you are welcome to fix it).
+ *
* Unlike git_config_from_file(), this function respects includes.
*/
void repo_config(struct repository *r, config_fn_t fn, void *);
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/9] builtin/verify-tag: stop using `the_repository`
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
2025-03-18 11:58 ` [PATCH 1/9] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
@ 2025-03-18 11:58 ` Usman Akinyemi
2025-03-20 7:21 ` Patrick Steinhardt
2025-03-18 11:58 ` [PATCH 3/9] builtin/verify-commit: " Usman Akinyemi
` (7 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:58 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
Remove the_repository global variable in favor of the repository
argument that gets passed in "builtin/verify-tag.c".
When `-h` is passed to the command outside a Git repository, the
`run_builtin()` will call the `cmd_verify_tag()` function with `repo` set
to NULL and then early in the function, `parse_options()` call will give
the options help and exit.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/verify-tag.c | 7 +++----
t/t7030-verify-tag.sh | 7 +++++++
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f6b97048a5..ed1c40338f 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -5,7 +5,6 @@
*
* Based on git-verify-tag.sh
*/
-#define USE_THE_REPOSITORY_VARIABLE
#include "builtin.h"
#include "config.h"
#include "gettext.h"
@@ -23,7 +22,7 @@ static const char * const verify_tag_usage[] = {
int cmd_verify_tag(int argc,
const char **argv,
const char *prefix,
- struct repository *repo UNUSED)
+ struct repository *repo)
{
int i = 1, verbose = 0, had_error = 0;
unsigned flags = 0;
@@ -35,7 +34,7 @@ int cmd_verify_tag(int argc,
OPT_END()
};
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
argc = parse_options(argc, argv, prefix, verify_tag_options,
verify_tag_usage, PARSE_OPT_KEEP_ARGV0);
@@ -56,7 +55,7 @@ int cmd_verify_tag(int argc,
struct object_id oid;
const char *name = argv[i++];
- if (repo_get_oid(the_repository, name, &oid)) {
+ if (repo_get_oid(repo, name, &oid)) {
had_error = !!error("tag '%s' not found.", name);
continue;
}
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 6f526c37c2..2c147072c1 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -7,6 +7,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
. "$TEST_DIRECTORY/lib-gpg.sh"
+test_expect_success GPG 'verify-tag does not crash with -h' '
+ test_expect_code 129 git verify-tag -h >usage &&
+ test_grep "[Uu]sage: git verify-tag " usage &&
+ test_expect_code 129 nongit git verify-tag -h >usage &&
+ test_grep "[Uu]sage: git verify-tag " usage
+'
+
test_expect_success GPG 'create signed tags' '
echo 1 >file && git add file &&
test_tick && git commit -m initial &&
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/9] builtin/verify-commit: stop using `the_repository`
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
2025-03-18 11:58 ` [PATCH 1/9] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
2025-03-18 11:58 ` [PATCH 2/9] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
@ 2025-03-18 11:58 ` Usman Akinyemi
2025-03-18 11:58 ` [PATCH 4/9] builtin/send-pack: " Usman Akinyemi
` (6 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:58 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
Remove the_repository global variable in favor of the repository
argument that gets passed in "builtin/verify-commit.c".
When `-h` is passed to the command outside a Git repository, the
`run_builtin()` will call the `cmd_verify_commit()` function with `repo`
set to NULL and then early in the function, `parse_options()` call will
give the options help and exit.
Pass the repository available in the calling context to `verify_commit()`
to remove it's dependency on the global `the_repository` variable.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/verify-commit.c | 13 ++++++-------
t/t7510-signed-commit.sh | 7 +++++++
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 779b7988ca..5f749a30da 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -5,7 +5,6 @@
*
* Based on git-verify-tag
*/
-#define USE_THE_REPOSITORY_VARIABLE
#include "builtin.h"
#include "config.h"
#include "gettext.h"
@@ -33,15 +32,15 @@ static int run_gpg_verify(struct commit *commit, unsigned flags)
return ret;
}
-static int verify_commit(const char *name, unsigned flags)
+static int verify_commit(struct repository *repo, const char *name, unsigned flags)
{
struct object_id oid;
struct object *obj;
- if (repo_get_oid(the_repository, name, &oid))
+ if (repo_get_oid(repo, name, &oid))
return error("commit '%s' not found.", name);
- obj = parse_object(the_repository, &oid);
+ obj = parse_object(repo, &oid);
if (!obj)
return error("%s: unable to read file.", name);
if (obj->type != OBJ_COMMIT)
@@ -54,7 +53,7 @@ static int verify_commit(const char *name, unsigned flags)
int cmd_verify_commit(int argc,
const char **argv,
const char *prefix,
- struct repository *repo UNUSED)
+ struct repository *repo)
{
int i = 1, verbose = 0, had_error = 0;
unsigned flags = 0;
@@ -64,7 +63,7 @@ int cmd_verify_commit(int argc,
OPT_END()
};
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
argc = parse_options(argc, argv, prefix, verify_commit_options,
verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
@@ -78,7 +77,7 @@ int cmd_verify_commit(int argc,
* was received in the process of writing the gpg input: */
signal(SIGPIPE, SIG_IGN);
while (i < argc)
- if (verify_commit(argv[i++], flags))
+ if (verify_commit(repo, argv[i++], flags))
had_error = 1;
return had_error;
}
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 0d2dd29fe6..39677e859a 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -8,6 +8,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
GNUPGHOME_NOT_USED=$GNUPGHOME
. "$TEST_DIRECTORY/lib-gpg.sh"
+test_expect_success GPG 'verify-commit does not crash with -h' '
+ test_expect_code 129 git verify-commit -h >usage &&
+ test_grep "[Uu]sage: git verify-commit " usage &&
+ test_expect_code 129 nongit git verify-commit -h >usage &&
+ test_grep "[Uu]sage: git verify-commit " usage
+'
+
test_expect_success GPG 'create signed commits' '
test_oid_cache <<-\EOF &&
header sha1:gpgsig
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/9] builtin/send-pack: stop using `the_repository`
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
` (2 preceding siblings ...)
2025-03-18 11:58 ` [PATCH 3/9] builtin/verify-commit: " Usman Akinyemi
@ 2025-03-18 11:58 ` Usman Akinyemi
2025-03-18 11:58 ` [PATCH 5/9] builtin/pack-refs: " Usman Akinyemi
` (5 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:58 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
Remove the_repository global variable in favor of the repository
argument that gets passed in "builtin/send-pack.c".
When `-h` is passed to the command outside a Git repository, the
`run_builtin()` will call the `cmd_send_pack()` function with `repo` set
to NULL and then early in the function, `parse_options()` call will give
the options help and exit.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/send-pack.c | 7 +++----
t/t5400-send-pack.sh | 7 +++++++
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 8d461008e2..c6e0e9d051 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#include "builtin.h"
#include "config.h"
#include "hex.h"
@@ -151,7 +150,7 @@ static int send_pack_config(const char *k, const char *v,
int cmd_send_pack(int argc,
const char **argv,
const char *prefix,
- struct repository *repo UNUSED)
+ struct repository *repo)
{
struct refspec rs = REFSPEC_INIT_PUSH;
const char *remote_name = NULL;
@@ -212,7 +211,7 @@ int cmd_send_pack(int argc,
OPT_END()
};
- git_config(send_pack_config, NULL);
+ repo_config(repo, send_pack_config, NULL);
argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
if (argc > 0) {
dest = argv[0];
@@ -317,7 +316,7 @@ int cmd_send_pack(int argc,
set_ref_status_for_push(remote_refs, args.send_mirror,
args.force_update);
- ret = send_pack(the_repository, &args, fd, conn, remote_refs, &extra_have);
+ ret = send_pack(repo, &args, fd, conn, remote_refs, &extra_have);
if (helper_status)
print_helper_status(remote_refs);
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 3f81f16e13..8f018d2f23 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -55,6 +55,13 @@ test_expect_success setup '
echo Rebase &&
git log'
+test_expect_success 'send-pack does not crash with -h' '
+ test_expect_code 129 git send-pack -h >usage &&
+ test_grep "[Uu]sage: git send-pack " usage &&
+ test_expect_code 129 nongit git send-pack -h >usage &&
+ test_grep "[Uu]sage: git send-pack " usage
+'
+
test_expect_success 'pack the source repository' '
git repack -a -d &&
git prune
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 5/9] builtin/pack-refs: stop using `the_repository`
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
` (3 preceding siblings ...)
2025-03-18 11:58 ` [PATCH 4/9] builtin/send-pack: " Usman Akinyemi
@ 2025-03-18 11:58 ` Usman Akinyemi
2025-03-18 11:58 ` [PATCH 6/9] builtin/ls-files: " Usman Akinyemi
` (4 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:58 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
Remove the_repository global variable in favor of the repository
argument that gets passed in "builtin/pack-refs.c".
When `-h` is passed to the command outside a Git repository, the
`run_builtin()` will call the `cmd_pack_refs()` function with `repo` set
to NULL and then early in the function, `parse_options()` call will give
the options help and exit.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/pack-refs.c | 8 +++-----
t/t0610-reftable-basics.sh | 7 +++++++
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 4fdd68880e..e47bae1c80 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "builtin.h"
#include "config.h"
#include "gettext.h"
@@ -15,7 +13,7 @@ static char const * const pack_refs_usage[] = {
int cmd_pack_refs(int argc,
const char **argv,
const char *prefix,
- struct repository *repo UNUSED)
+ struct repository *repo)
{
struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;
struct string_list included_refs = STRING_LIST_INIT_NODUP;
@@ -39,7 +37,7 @@ int cmd_pack_refs(int argc,
N_("references to exclude")),
OPT_END(),
};
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
@@ -52,7 +50,7 @@ int cmd_pack_refs(int argc,
if (!pack_refs_opts.includes->nr)
string_list_append(pack_refs_opts.includes, "refs/tags/*");
- ret = refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);
+ ret = refs_pack_refs(get_main_ref_store(repo), &pack_refs_opts);
clear_ref_exclusions(&excludes);
string_list_clear(&included_refs, 0);
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 4618ffc108..002a75dee8 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -14,6 +14,13 @@ export GIT_TEST_DEFAULT_REF_FORMAT
INVALID_OID=$(test_oid 001)
+test_expect_success 'pack-refs does not crash with -h' '
+ test_expect_code 129 git pack-refs -h >usage &&
+ test_grep "[Uu]sage: git pack-refs " usage &&
+ test_expect_code 129 nongit git pack-refs -h >usage &&
+ test_grep "[Uu]sage: git pack-refs " usage
+'
+
test_expect_success 'init: creates basic reftable structures' '
test_when_finished "rm -rf repo" &&
git init repo &&
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 6/9] builtin/ls-files: stop using `the_repository`
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
` (4 preceding siblings ...)
2025-03-18 11:58 ` [PATCH 5/9] builtin/pack-refs: " Usman Akinyemi
@ 2025-03-18 11:58 ` Usman Akinyemi
2025-03-18 11:58 ` [PATCH 7/9] builtin/for-each-ref: " Usman Akinyemi
` (3 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:58 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
Remove the_repository global variable in favor of the repository
argument that gets passed in "builtin/ls-files.c".
When `-h` is passed to the command outside a Git repository, the
`run_builtin()` will call the `cmd_ls_files()` function with `repo` set
to NULL and then early in the function, `show_usage_with_options_if_asked()`
call will give the options help and exit.
Pass the repository available in the calling context to both
`expand_objectsize()` and `show_ru_info()` to remove their
dependency on the global `the_repository` variable.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/ls-files.c | 32 ++++++++++++++++----------------
t/t3004-ls-files-basic.sh | 7 +++++++
2 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a4431429b7..70a377e9c0 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -6,7 +6,6 @@
* Copyright (C) Linus Torvalds, 2005
*/
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "builtin.h"
@@ -245,12 +244,13 @@ static void show_submodule(struct repository *superproject,
repo_clear(&subrepo);
}
-static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
+static void expand_objectsize(struct repository *repo, struct strbuf *line,
+ const struct object_id *oid,
const enum object_type type, unsigned int padded)
{
if (type == OBJ_BLOB) {
unsigned long size;
- if (oid_object_info(the_repository, oid, &size) < 0)
+ if (oid_object_info(repo, oid, &size) < 0)
die(_("could not get object info about '%s'"),
oid_to_hex(oid));
if (padded)
@@ -283,10 +283,10 @@ static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
else if (skip_prefix(format, "(objecttype)", &format))
strbuf_addstr(&sb, type_name(object_type(ce->ce_mode)));
else if (skip_prefix(format, "(objectsize:padded)", &format))
- expand_objectsize(&sb, &ce->oid,
+ expand_objectsize(repo, &sb, &ce->oid,
object_type(ce->ce_mode), 1);
else if (skip_prefix(format, "(objectsize)", &format))
- expand_objectsize(&sb, &ce->oid,
+ expand_objectsize(repo, &sb, &ce->oid,
object_type(ce->ce_mode), 0);
else if (skip_prefix(format, "(stage)", &format))
strbuf_addf(&sb, "%d", ce_stage(ce));
@@ -348,7 +348,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,
}
}
-static void show_ru_info(struct index_state *istate)
+static void show_ru_info(struct repository *repo, struct index_state *istate)
{
struct string_list_item *item;
@@ -370,7 +370,7 @@ static void show_ru_info(struct index_state *istate)
if (!ui->mode[i])
continue;
printf("%s%06o %s %d\t", tag_resolve_undo, ui->mode[i],
- repo_find_unique_abbrev(the_repository, &ui->oid[i], abbrev),
+ repo_find_unique_abbrev(repo, &ui->oid[i], abbrev),
i + 1);
write_name(path);
}
@@ -567,7 +567,7 @@ static int option_parse_exclude_standard(const struct option *opt,
int cmd_ls_files(int argc,
const char **argv,
const char *cmd_prefix,
- struct repository *repo UNUSED)
+ struct repository *repo)
{
int require_work_tree = 0, show_tag = 0, i;
char *max_prefix;
@@ -647,15 +647,15 @@ int cmd_ls_files(int argc,
show_usage_with_options_if_asked(argc, argv,
ls_files_usage, builtin_ls_files_options);
- prepare_repo_settings(the_repository);
- the_repository->settings.command_requires_full_index = 0;
+ prepare_repo_settings(repo);
+ repo->settings.command_requires_full_index = 0;
prefix = cmd_prefix;
if (prefix)
prefix_len = strlen(prefix);
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
- if (repo_read_index(the_repository) < 0)
+ if (repo_read_index(repo) < 0)
die("index file corrupt");
argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
@@ -724,7 +724,7 @@ int cmd_ls_files(int argc,
max_prefix = common_prefix(&pathspec);
max_prefix_len = get_common_prefix_len(max_prefix);
- prune_index(the_repository->index, max_prefix, max_prefix_len);
+ prune_index(repo->index, max_prefix, max_prefix_len);
/* Treat unmatching pathspec elements as errors */
if (pathspec.nr && error_unmatch)
@@ -748,13 +748,13 @@ int cmd_ls_files(int argc,
*/
if (show_stage || show_unmerged)
die(_("options '%s' and '%s' cannot be used together"), "ls-files --with-tree", "-s/-u");
- overlay_tree_on_index(the_repository->index, with_tree, max_prefix);
+ overlay_tree_on_index(repo->index, with_tree, max_prefix);
}
- show_files(the_repository, &dir);
+ show_files(repo, &dir);
if (show_resolve_undo)
- show_ru_info(the_repository->index);
+ show_ru_info(repo, repo->index);
if (ps_matched && report_path_error(ps_matched, &pathspec)) {
fprintf(stderr, "Did you forget to 'git add'?\n");
diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index a1078f8701..4034a5a59f 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -34,6 +34,13 @@ test_expect_success 'ls-files -h in corrupt repository' '
test_grep "[Uu]sage: git ls-files " broken/usage
'
+test_expect_success 'ls-files does not crash with -h' '
+ test_expect_code 129 git ls-files -h >usage &&
+ test_grep "[Uu]sage: git ls-files " usage &&
+ test_expect_code 129 nongit git ls-files -h >usage &&
+ test_grep "[Uu]sage: git ls-files " usage
+'
+
test_expect_success SYMLINKS 'ls-files with absolute paths to symlinks' '
mkdir subs &&
ln -s nosuch link &&
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 7/9] builtin/for-each-ref: stop using `the_repository`
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
` (5 preceding siblings ...)
2025-03-18 11:58 ` [PATCH 6/9] builtin/ls-files: " Usman Akinyemi
@ 2025-03-18 11:58 ` Usman Akinyemi
2025-03-18 11:59 ` [PATCH 8/9] builtin/checkout-index: " Usman Akinyemi
` (2 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:58 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
Remove the_repository global variable in favor of the repository
argument that gets passed in "builtin/for-each-ref.c".
When `-h` is passed to the command outside a Git repository, the
`run_builtin()` will call the `cmd_for_each_ref()` function with `repo`
set to NULL and then early in the function, `parse_options()` call will
give the options help and exit.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/for-each-ref.c | 5 ++---
t/t6300-for-each-ref.sh | 7 +++++++
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 8085ebd8fe..3d2207ec77 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
#include "builtin.h"
#include "commit.h"
#include "config.h"
@@ -20,7 +19,7 @@ static char const * const for_each_ref_usage[] = {
int cmd_for_each_ref(int argc,
const char **argv,
const char *prefix,
- struct repository *repo UNUSED)
+ struct repository *repo)
{
struct ref_sorting *sorting;
struct string_list sorting_options = STRING_LIST_INIT_DUP;
@@ -63,7 +62,7 @@ int cmd_for_each_ref(int argc,
format.format = "%(objectname) %(objecttype)\t%(refname)";
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
/* Set default (refname) sorting */
string_list_append(&sorting_options, "refname");
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index a5c7794385..9b4f4306c4 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -292,6 +292,13 @@ test_expect_success 'Check invalid atoms names are errors' '
test_must_fail git for-each-ref --format="%(INVALID)" refs/heads
'
+test_expect_success 'for-each-ref does not crash with -h' '
+ test_expect_code 129 git for-each-ref -h >usage &&
+ test_grep "[Uu]sage: git for-each-ref " usage &&
+ test_expect_code 129 nongit git for-each-ref -h >usage &&
+ test_grep "[Uu]sage: git for-each-ref " usage
+'
+
test_expect_success 'Check format specifiers are ignored in naming date atoms' '
git for-each-ref --format="%(authordate)" refs/heads &&
git for-each-ref --format="%(authordate:default) %(authordate)" refs/heads &&
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 8/9] builtin/checkout-index: stop using `the_repository`
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
` (6 preceding siblings ...)
2025-03-18 11:58 ` [PATCH 7/9] builtin/for-each-ref: " Usman Akinyemi
@ 2025-03-18 11:59 ` Usman Akinyemi
2025-03-18 11:59 ` [PATCH 9/9] builtin/update-server-info: remove unnecessary if statement Usman Akinyemi
2025-03-18 20:21 ` [PATCH 0/9] " Junio C Hamano
9 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:59 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
Remove the_repository global variable in favor of the repository
argument that gets passed in "builtin/checkout-index.c".
When `-h` is passed to the command outside a Git repository, the
`run_builtin()` will call the `cmd_checkout_index()` function with `repo`
set to NULL and then early in the function, `show_usage_with_options_if_asked()`
call will give the options help and exit.
Pass an instance of "struct index_state" available in the calling
context to both `checkout_all()` and `checkout_file()` to remove their
dependency on the global `the_repository` variable.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/checkout-index.c | 43 ++++++++++++++++-----------------
t/t2006-checkout-index-basic.sh | 7 ++++++
2 files changed, 28 insertions(+), 22 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index e30086c7d4..7f74bc702f 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -5,7 +5,6 @@
*
*/
-#define USE_THE_REPOSITORY_VARIABLE
#define DISABLE_SIGN_COMPARE_WARNINGS
#include "builtin.h"
@@ -68,10 +67,10 @@ static void write_tempfile_record(const char *name, const char *prefix)
}
}
-static int checkout_file(const char *name, const char *prefix)
+static int checkout_file(struct index_state *index, const char *name, const char *prefix)
{
int namelen = strlen(name);
- int pos = index_name_pos(the_repository->index, name, namelen);
+ int pos = index_name_pos(index, name, namelen);
int has_same_name = 0;
int is_file = 0;
int is_skipped = 1;
@@ -81,8 +80,8 @@ static int checkout_file(const char *name, const char *prefix)
if (pos < 0)
pos = -pos - 1;
- while (pos <the_repository->index->cache_nr) {
- struct cache_entry *ce =the_repository->index->cache[pos];
+ while (pos < index->cache_nr) {
+ struct cache_entry *ce = index->cache[pos];
if (ce_namelen(ce) != namelen ||
memcmp(ce->name, name, namelen))
break;
@@ -137,13 +136,13 @@ static int checkout_file(const char *name, const char *prefix)
return -1;
}
-static int checkout_all(const char *prefix, int prefix_length)
+static int checkout_all(struct index_state *index, const char *prefix, int prefix_length)
{
int i, errs = 0;
struct cache_entry *last_ce = NULL;
- for (i = 0; i < the_repository->index->cache_nr ; i++) {
- struct cache_entry *ce = the_repository->index->cache[i];
+ for (i = 0; i < index->cache_nr ; i++) {
+ struct cache_entry *ce = index->cache[i];
if (S_ISSPARSEDIR(ce->ce_mode)) {
if (!ce_skip_worktree(ce))
@@ -156,8 +155,8 @@ static int checkout_all(const char *prefix, int prefix_length)
* first entry inside the expanded sparse directory).
*/
if (ignore_skip_worktree) {
- ensure_full_index(the_repository->index);
- ce = the_repository->index->cache[i];
+ ensure_full_index(index);
+ ce = index->cache[i];
}
}
@@ -213,7 +212,7 @@ static int option_parse_stage(const struct option *opt,
int cmd_checkout_index(int argc,
const char **argv,
const char *prefix,
- struct repository *repo UNUSED)
+ struct repository *repo)
{
int i;
struct lock_file lock_file = LOCK_INIT;
@@ -253,19 +252,19 @@ int cmd_checkout_index(int argc,
show_usage_with_options_if_asked(argc, argv,
builtin_checkout_index_usage,
builtin_checkout_index_options);
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
prefix_length = prefix ? strlen(prefix) : 0;
- prepare_repo_settings(the_repository);
- the_repository->settings.command_requires_full_index = 0;
+ prepare_repo_settings(repo);
+ repo->settings.command_requires_full_index = 0;
- if (repo_read_index(the_repository) < 0) {
+ if (repo_read_index(repo) < 0) {
die("invalid cache");
}
argc = parse_options(argc, argv, prefix, builtin_checkout_index_options,
builtin_checkout_index_usage, 0);
- state.istate = the_repository->index;
+ state.istate = repo->index;
state.force = force;
state.quiet = quiet;
state.not_new = not_new;
@@ -285,8 +284,8 @@ int cmd_checkout_index(int argc,
*/
if (index_opt && !state.base_dir_len && !to_tempfile) {
state.refresh_cache = 1;
- state.istate = the_repository->index;
- repo_hold_locked_index(the_repository, &lock_file,
+ state.istate = repo->index;
+ repo_hold_locked_index(repo, &lock_file,
LOCK_DIE_ON_ERROR);
}
@@ -304,7 +303,7 @@ int cmd_checkout_index(int argc,
if (read_from_stdin)
die("git checkout-index: don't mix '--stdin' and explicit filenames");
p = prefix_path(prefix, prefix_length, arg);
- err |= checkout_file(p, prefix);
+ err |= checkout_file(repo->index, p, prefix);
free(p);
}
@@ -326,7 +325,7 @@ int cmd_checkout_index(int argc,
strbuf_swap(&buf, &unquoted);
}
p = prefix_path(prefix, prefix_length, buf.buf);
- err |= checkout_file(p, prefix);
+ err |= checkout_file(repo->index, p, prefix);
free(p);
}
strbuf_release(&unquoted);
@@ -334,7 +333,7 @@ int cmd_checkout_index(int argc,
}
if (all)
- err |= checkout_all(prefix, prefix_length);
+ err |= checkout_all(repo->index, prefix, prefix_length);
if (pc_workers > 1)
err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
@@ -344,7 +343,7 @@ int cmd_checkout_index(int argc,
return 1;
if (is_lock_file_locked(&lock_file) &&
- write_locked_index(the_repository->index, &lock_file, COMMIT_LOCK))
+ write_locked_index(repo->index, &lock_file, COMMIT_LOCK))
die("Unable to write new index file");
return 0;
}
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
index bac231b167..fedd2cc097 100755
--- a/t/t2006-checkout-index-basic.sh
+++ b/t/t2006-checkout-index-basic.sh
@@ -21,6 +21,13 @@ test_expect_success 'checkout-index -h in broken repository' '
test_grep "[Uu]sage" broken/usage
'
+test_expect_success 'checkout-index does not crash with -h' '
+ test_expect_code 129 git checkout-index -h >usage &&
+ test_grep "[Uu]sage: git checkout-index " usage &&
+ test_expect_code 129 nongit git checkout-index -h >usage &&
+ test_grep "[Uu]sage: git checkout-index " usage
+'
+
test_expect_success 'checkout-index reports errors (cmdline)' '
test_must_fail git checkout-index -- does-not-exist 2>stderr &&
test_grep not.in.the.cache stderr
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 9/9] builtin/update-server-info: remove unnecessary if statement
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
` (7 preceding siblings ...)
2025-03-18 11:59 ` [PATCH 8/9] builtin/checkout-index: " Usman Akinyemi
@ 2025-03-18 11:59 ` Usman Akinyemi
2025-03-20 7:21 ` Patrick Steinhardt
2025-03-18 20:21 ` [PATCH 0/9] " Junio C Hamano
9 siblings, 1 reply; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-18 11:59 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, phillip.wood123, ps, shejialuo,
Christian Couder
Since we already teach the `repo_config()` to allow `repo`
to be NULL, no need to check if `repo` is NULL before calling
`repo_config()`.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/update-server-info.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/update-server-info.c b/builtin/update-server-info.c
index d7467290a8..ba702d30ef 100644
--- a/builtin/update-server-info.c
+++ b/builtin/update-server-info.c
@@ -20,8 +20,8 @@ int cmd_update_server_info(int argc,
OPT_END()
};
- if (repo)
- repo_config(repo, git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
+
argc = parse_options(argc, argv, prefix, options,
update_server_info_usage, 0);
if (argc > 0)
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/9] remove unnecessary if statement
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
` (8 preceding siblings ...)
2025-03-18 11:59 ` [PATCH 9/9] builtin/update-server-info: remove unnecessary if statement Usman Akinyemi
@ 2025-03-18 20:21 ` Junio C Hamano
2025-03-19 3:53 ` Usman Akinyemi
9 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2025-03-18 20:21 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, johncai86, me, phillip.wood123, ps,
shejialuo
Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
> In an earlier patch[1] which has been merged to the master,
> We checked `repo` is not NULL before making call to `repo_config()`.
> Later, in another patch series[2] which has been merged to next,
> `repo_config()` was taught to allow `repo` to be NULL.
>
> So there is not need for checking if the `repo` is NULL before calling
> repo_config() in the earlier patch.
OK, that sounds good.
Are we confident that our half-hearted choice of "there is no repo,
so just do a very-early-config thing" is appropriate for any code
paths?
At least we should be perfectly happy with that choice applied to
all of these code paths touched by this series.
> Note, I have already sent the first 8 patches in [2] but, the 9th
> patch depends on the first patch of that series.
So, is this [v2 0/9] of ua/some-builtins-wo-the-repository?
I think that topic has long been merged to 'next', and it is way too
late to do a wholesale replacement like this.
> [1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/
> [2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/
>
> Usman Akinyemi (9):
> config: teach repo_config to allow `repo` to be NULL
> builtin/verify-tag: stop using `the_repository`
> builtin/verify-commit: stop using `the_repository`
> builtin/send-pack: stop using `the_repository`
> builtin/pack-refs: stop using `the_repository`
> builtin/ls-files: stop using `the_repository`
> builtin/for-each-ref: stop using `the_repository`
> builtin/checkout-index: stop using `the_repository`
> builtin/update-server-info: remove unnecessary if statement
>
> builtin/checkout-index.c | 43 ++++++++++++++++-----------------
> builtin/for-each-ref.c | 5 ++--
> builtin/ls-files.c | 32 ++++++++++++------------
> builtin/pack-refs.c | 8 +++---
> builtin/send-pack.c | 7 +++---
> builtin/update-server-info.c | 4 +--
> builtin/verify-commit.c | 13 +++++-----
> builtin/verify-tag.c | 7 +++---
> config.c | 4 +++
> config.h | 9 +++++++
> t/t0610-reftable-basics.sh | 7 ++++++
> t/t2006-checkout-index-basic.sh | 7 ++++++
> t/t3004-ls-files-basic.sh | 7 ++++++
> t/t5400-send-pack.sh | 7 ++++++
> t/t6300-for-each-ref.sh | 7 ++++++
> t/t7030-verify-tag.sh | 7 ++++++
> t/t7510-signed-commit.sh | 7 ++++++
> 17 files changed, 118 insertions(+), 63 deletions(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/9] remove unnecessary if statement
2025-03-18 20:21 ` [PATCH 0/9] " Junio C Hamano
@ 2025-03-19 3:53 ` Usman Akinyemi
0 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-19 3:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, christian.couder, johncai86, me, phillip.wood123, ps,
shejialuo
On Wed, Mar 19, 2025 at 1:51 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>
> > In an earlier patch[1] which has been merged to the master,
> > We checked `repo` is not NULL before making call to `repo_config()`.
> > Later, in another patch series[2] which has been merged to next,
> > `repo_config()` was taught to allow `repo` to be NULL.
> >
> > So there is not need for checking if the `repo` is NULL before calling
> > repo_config() in the earlier patch.
>
> OK, that sounds good.
>
> Are we confident that our half-hearted choice of "there is no repo,
> so just do a very-early-config thing" is appropriate for any code
> paths?
>
> At least we should be perfectly happy with that choice applied to
> all of these code paths touched by this series.
This is fine for the commands which use only RUN_SETUP. I also picked and
checked those code paths touched by this series to ensure they do what we intend
what we intend them to do. I mean, if there is any better option, that
would be cool.
>
> > Note, I have already sent the first 8 patches in [2] but, the 9th
> > patch depends on the first patch of that series.
>
> So, is this [v2 0/9] of ua/some-builtins-wo-the-repository?
>
> I think that topic has long been merged to 'next', and it is way too
> late to do a wholesale replacement like this.
It is not a replacement for that patch series but, the last patch here
9/9 depends
on the first patch in that series. That is why I sent everything together.
Thank you.
>
> > [1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/
> > [2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/
> >
> > Usman Akinyemi (9):
> > config: teach repo_config to allow `repo` to be NULL
> > builtin/verify-tag: stop using `the_repository`
> > builtin/verify-commit: stop using `the_repository`
> > builtin/send-pack: stop using `the_repository`
> > builtin/pack-refs: stop using `the_repository`
> > builtin/ls-files: stop using `the_repository`
> > builtin/for-each-ref: stop using `the_repository`
> > builtin/checkout-index: stop using `the_repository`
> > builtin/update-server-info: remove unnecessary if statement
> >
> > builtin/checkout-index.c | 43 ++++++++++++++++-----------------
> > builtin/for-each-ref.c | 5 ++--
> > builtin/ls-files.c | 32 ++++++++++++------------
> > builtin/pack-refs.c | 8 +++---
> > builtin/send-pack.c | 7 +++---
> > builtin/update-server-info.c | 4 +--
> > builtin/verify-commit.c | 13 +++++-----
> > builtin/verify-tag.c | 7 +++---
> > config.c | 4 +++
> > config.h | 9 +++++++
> > t/t0610-reftable-basics.sh | 7 ++++++
> > t/t2006-checkout-index-basic.sh | 7 ++++++
> > t/t3004-ls-files-basic.sh | 7 ++++++
> > t/t5400-send-pack.sh | 7 ++++++
> > t/t6300-for-each-ref.sh | 7 ++++++
> > t/t7030-verify-tag.sh | 7 ++++++
> > t/t7510-signed-commit.sh | 7 ++++++
> > 17 files changed, 118 insertions(+), 63 deletions(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/9] config: teach repo_config to allow `repo` to be NULL
2025-03-18 11:58 ` [PATCH 1/9] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
@ 2025-03-20 7:21 ` Patrick Steinhardt
2025-03-21 6:56 ` Usman Akinyemi
0 siblings, 1 reply; 17+ messages in thread
From: Patrick Steinhardt @ 2025-03-20 7:21 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, gitster, johncai86, me, phillip.wood123,
shejialuo, Christian Couder
On Tue, Mar 18, 2025 at 05:28:53PM +0530, Usman Akinyemi wrote:
> diff --git a/config.c b/config.c
> index 658569af08..e127afaa8f 100644
> --- a/config.c
> +++ b/config.c
> @@ -2521,6 +2521,10 @@ void repo_config_clear(struct repository *repo)
>
> void repo_config(struct repository *repo, config_fn_t fn, void *data)
> {
> + if (!repo) {
> + read_very_early_config(fn, data);
> + return;
> + }
I remember discussion that `read_very_early_config()` may not be a good
fit here. Most importantly, it ignores any configuration passed on the
command line, which I would think is very surprising behaviour. So
should we adapt this to instead manually call `config_with_options()`
with the expected bits set?
Patrick
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 9/9] builtin/update-server-info: remove unnecessary if statement
2025-03-18 11:59 ` [PATCH 9/9] builtin/update-server-info: remove unnecessary if statement Usman Akinyemi
@ 2025-03-20 7:21 ` Patrick Steinhardt
2025-03-21 7:00 ` Usman Akinyemi
0 siblings, 1 reply; 17+ messages in thread
From: Patrick Steinhardt @ 2025-03-20 7:21 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, gitster, johncai86, me, phillip.wood123,
shejialuo, Christian Couder
On Tue, Mar 18, 2025 at 05:29:01PM +0530, Usman Akinyemi wrote:
> Since we already teach the `repo_config()` to allow `repo`
> to be NULL, no need to check if `repo` is NULL before calling
> `repo_config()`.
I think it would be preferable to reorder this patch so that it comes
immediately after the one where you adapt `repo_config()`.
We also have a couple of additional sites where we call the function
conditionally:
- builtin/add.c
- builtin/difftool.c
- builtin/update-server-info.c
It would probably make sense to also adapt those.
Patrick
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/9] builtin/verify-tag: stop using `the_repository`
2025-03-18 11:58 ` [PATCH 2/9] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
@ 2025-03-20 7:21 ` Patrick Steinhardt
0 siblings, 0 replies; 17+ messages in thread
From: Patrick Steinhardt @ 2025-03-20 7:21 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, gitster, johncai86, me, phillip.wood123,
shejialuo, Christian Couder
On Tue, Mar 18, 2025 at 05:28:54PM +0530, Usman Akinyemi wrote:
> diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
> index 6f526c37c2..2c147072c1 100755
> --- a/t/t7030-verify-tag.sh
> +++ b/t/t7030-verify-tag.sh
> @@ -7,6 +7,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> . ./test-lib.sh
> . "$TEST_DIRECTORY/lib-gpg.sh"
>
> +test_expect_success GPG 'verify-tag does not crash with -h' '
> + test_expect_code 129 git verify-tag -h >usage &&
> + test_grep "[Uu]sage: git verify-tag " usage &&
> + test_expect_code 129 nongit git verify-tag -h >usage &&
> + test_grep "[Uu]sage: git verify-tag " usage
> +'
> +
> test_expect_success GPG 'create signed tags' '
> echo 1 >file && git add file &&
> test_tick && git commit -m initial &&
We have "t1517-outside-repo.sh". Maybe it would preferable to add tests
like these to that test suite instead?
Patrick
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/9] config: teach repo_config to allow `repo` to be NULL
2025-03-20 7:21 ` Patrick Steinhardt
@ 2025-03-21 6:56 ` Usman Akinyemi
0 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-21 6:56 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, christian.couder, gitster, johncai86, me, phillip.wood123,
shejialuo, Christian Couder
On Thu, Mar 20, 2025 at 12:51 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Mar 18, 2025 at 05:28:53PM +0530, Usman Akinyemi wrote:
> > diff --git a/config.c b/config.c
> > index 658569af08..e127afaa8f 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -2521,6 +2521,10 @@ void repo_config_clear(struct repository *repo)
> >
> > void repo_config(struct repository *repo, config_fn_t fn, void *data)
> > {
> > + if (!repo) {
> > + read_very_early_config(fn, data);
> > + return;
> > + }
>
> I remember discussion that `read_very_early_config()` may not be a good
> fit here. Most importantly, it ignores any configuration passed on the
> command line, which I would think is very surprising behaviour. So
> should we adapt this to instead manually call `config_with_options()`
> with the expected bits set?
Hi Patrick,
Sorry for the confusion.
There was a discussion about this here.
https://public-inbox.org/git/xmqqbjum2ayc.fsf@gitster.g/
https://public-inbox.org/git/xmqqcyeuhwqb.fsf@gitster.g/
Mainliy ignoring the configuration passed from cmdline for this purpose.
I was sending this patch again mainly for the 9/9 patch.
Thanks.
>
> Patrick
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 9/9] builtin/update-server-info: remove unnecessary if statement
2025-03-20 7:21 ` Patrick Steinhardt
@ 2025-03-21 7:00 ` Usman Akinyemi
0 siblings, 0 replies; 17+ messages in thread
From: Usman Akinyemi @ 2025-03-21 7:00 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, christian.couder, gitster, johncai86, me, phillip.wood123,
shejialuo, Christian Couder
On Thu, Mar 20, 2025 at 12:51 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Mar 18, 2025 at 05:29:01PM +0530, Usman Akinyemi wrote:
> > Since we already teach the `repo_config()` to allow `repo`
> > to be NULL, no need to check if `repo` is NULL before calling
> > `repo_config()`.
>
> I think it would be preferable to reorder this patch so that it comes
> immediately after the one where you adapt `repo_config()`.
>
> We also have a couple of additional sites where we call the function
> conditionally:
>
> - builtin/add.c
> - builtin/difftool.c
> - builtin/update-server-info.c
>
> It would probably make sense to also adapt those.
Yeah, it has caused a little confusion. Maybe, let's just ignore this
patch series.
The other series which introduces the first 8 patches of this has been
marked to be merged to master. I will resend the 9th patch(
builtin/update-server-info.c) then other functions like difftool.
Thank you.
>
> Patrick
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-03-21 7:00 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-18 11:58 [PATCH 0/9] remove unnecessary if statement Usman Akinyemi
2025-03-18 11:58 ` [PATCH 1/9] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
2025-03-20 7:21 ` Patrick Steinhardt
2025-03-21 6:56 ` Usman Akinyemi
2025-03-18 11:58 ` [PATCH 2/9] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
2025-03-20 7:21 ` Patrick Steinhardt
2025-03-18 11:58 ` [PATCH 3/9] builtin/verify-commit: " Usman Akinyemi
2025-03-18 11:58 ` [PATCH 4/9] builtin/send-pack: " Usman Akinyemi
2025-03-18 11:58 ` [PATCH 5/9] builtin/pack-refs: " Usman Akinyemi
2025-03-18 11:58 ` [PATCH 6/9] builtin/ls-files: " Usman Akinyemi
2025-03-18 11:58 ` [PATCH 7/9] builtin/for-each-ref: " Usman Akinyemi
2025-03-18 11:59 ` [PATCH 8/9] builtin/checkout-index: " Usman Akinyemi
2025-03-18 11:59 ` [PATCH 9/9] builtin/update-server-info: remove unnecessary if statement Usman Akinyemi
2025-03-20 7:21 ` Patrick Steinhardt
2025-03-21 7:00 ` Usman Akinyemi
2025-03-18 20:21 ` [PATCH 0/9] " Junio C Hamano
2025-03-19 3:53 ` Usman Akinyemi
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 read-only IMAP folder(s) and NNTP newsgroup(s).