* [PATCH v2 01/12] builtin/verify-tag: refactor `cmd_verify_tag()`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-20 15:32 ` Junio C Hamano
2025-02-19 20:32 ` [PATCH v2 02/12] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
` (11 subsequent siblings)
12 siblings, 1 reply; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
Move `git_config()` call after `usage_with_options()` to avoid NULL `repo`
check.
When "-h" is passed to builtins using the RUN_SETUP macro, `repo` passed
by `run_builtin()` will be NULL. If we use the `repo` instead of the
global `the_repository` variable. We will have to switch from `git_config()`
to `repo_config()` which takes in `repo`. We must check for NULL `repo`
if `repo_config()` comes before `usage_with_options()`. Moving `git_config()`
after `usage_with_options()` eliminates this need, as `usage_with_options()`
exit before calling `repo_config()`.
This will be useful in the following patch which remove `the_repository`
global variable in favor of the `repo` passed by `run_builtin()`.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/verify-tag.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f6b97048a5..f0e7c2a2b5 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -35,8 +35,6 @@ int cmd_verify_tag(int argc,
OPT_END()
};
- git_config(git_default_config, NULL);
-
argc = parse_options(argc, argv, prefix, verify_tag_options,
verify_tag_usage, PARSE_OPT_KEEP_ARGV0);
if (argc <= i)
@@ -52,6 +50,8 @@ int cmd_verify_tag(int argc,
flags |= GPG_VERIFY_OMIT_STATUS;
}
+ git_config(git_default_config, NULL);
+
while (i < argc) {
struct object_id oid;
const char *name = argv[i++];
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH v2 01/12] builtin/verify-tag: refactor `cmd_verify_tag()`
2025-02-19 20:32 ` [PATCH v2 01/12] builtin/verify-tag: refactor `cmd_verify_tag()` Usman Akinyemi
@ 2025-02-20 15:32 ` Junio C Hamano
0 siblings, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2025-02-20 15:32 UTC (permalink / raw)
To: Usman Akinyemi
Cc: christian.couder, git, me, chriscool, johncai86, ps, shejialuo
Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
> Move `git_config()` call after `usage_with_options()` to avoid NULL `repo`
> check.
Two things and a half.
- This move of a single call is not something I see usually called
"refactor".
- The new call to git_config() should be chosen a bit more
carefully to be future-proof. It is harder to see with the
reduced context, but verbose and format.format has already been
referenced before the new place where you read the configuration
file, which would mean that you are making it impossible for
future developers to add configuration variables to give default
values to these two settings.
The same comment applies to potential new configuration variables
that may control how parse_options() would work. With this line of
conversion, we are closing the door for such configuration variables
(e.g., OPTION_FILENAME may want to understand "~/path" and if we had
user.homedirectory configuration variable, the value of the variable
should affect what file the string "~/path" given to the option
refers to).
> When "-h" is passed to builtins using the RUN_SETUP macro, `repo`
> passed by `run_builtin()` will be NULL. If we use the `repo`
> instead of the global `the_repository` variable. We will have to
> switch from `git_config()` to `repo_config()` which takes in
> `repo`.
I do not quite agree with this line of reasoning behind "will have
to switch". You need to realize that this is cmd_verify_tag() that
is designed to be ONLY called as the top-level command
implementation and not called from random places as a library
function. It is perfectly fine for it to work with the_repository
with git_config().
The helper functions that this functions call may be different
matter, though.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v2 02/12] builtin/verify-tag: stop using `the_repository`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 01/12] builtin/verify-tag: refactor `cmd_verify_tag()` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-20 15:43 ` Junio C Hamano
2025-02-19 20:32 ` [PATCH v2 03/12] builtin/verify-commit: refactor `cmd_verify_commit()` Usman Akinyemi
` (10 subsequent siblings)
12 siblings, 1 reply; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
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, without having to consult much of the
configuration file.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/verify-tag.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f0e7c2a2b5..0acdb364dd 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;
@@ -50,13 +49,13 @@ int cmd_verify_tag(int argc,
flags |= GPG_VERIFY_OMIT_STATUS;
}
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
while (i < 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;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH v2 02/12] builtin/verify-tag: stop using `the_repository`
2025-02-19 20:32 ` [PATCH v2 02/12] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
@ 2025-02-20 15:43 ` Junio C Hamano
2025-02-27 17:56 ` Usman Akinyemi
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2025-02-20 15:43 UTC (permalink / raw)
To: Usman Akinyemi
Cc: christian.couder, git, me, chriscool, johncai86, ps, shejialuo
Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
> @@ -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;
> @@ -50,13 +49,13 @@ int cmd_verify_tag(int argc,
> flags |= GPG_VERIFY_OMIT_STATUS;
> }
>
> - git_config(git_default_config, NULL);
> + repo_config(repo, git_default_config, NULL);
I seriously think that it is a horrible idea (but the previous step
of this series is hardly the first one that commits the same sin) to
move git_config() down only to deal with "repo might be NULL if run
outside a repository". We should stop making such changes, and we
should revert the changes we already made along that line, to solve
it differently.
Wouldn't it work much better if we teach repo_config() to allow repo
to be NULL to signal that we are outside any repository, and behave
the same way the current git_config() works when called outside a
repository? Even though the function is called repo_config(), it is
*NOT* limited to read from $GIT_DIR/config but does read from the
usual "repository configuration trumps per-user configuration which
trumps system-side configuration" cascade, so it is natural to skip
the repository configuration when called outside any repository but
read the other configuration sources, which should be what happens
when git_config() is called from outside the repository, no?
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH v2 02/12] builtin/verify-tag: stop using `the_repository`
2025-02-20 15:43 ` Junio C Hamano
@ 2025-02-27 17:56 ` Usman Akinyemi
2025-02-27 22:39 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-27 17:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: christian.couder, git, me, chriscool, johncai86, ps, shejialuo
On Thu, Feb 20, 2025 at 9:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>
Hi Junio.
> > @@ -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;
> > @@ -50,13 +49,13 @@ int cmd_verify_tag(int argc,
> > flags |= GPG_VERIFY_OMIT_STATUS;
> > }
> >
> > - git_config(git_default_config, NULL);
> > + repo_config(repo, git_default_config, NULL);
>
> I seriously think that it is a horrible idea (but the previous step
> of this series is hardly the first one that commits the same sin) to
> move git_config() down only to deal with "repo might be NULL if run
> outside a repository". We should stop making such changes, and we
> should revert the changes we already made along that line, to solve
> it differently.
>
Yeah, I agree with this after going through your comment on the other
patch. We should look for a better solution.
> Wouldn't it work much better if we teach repo_config() to allow repo
> to be NULL to signal that we are outside any repository, and behave
> the same way the current git_config() works when called outside a
> repository? Even though the function is called repo_config(), it is
> *NOT* limited to read from $GIT_DIR/config but does read from the
> usual "repository configuration trumps per-user configuration which
> trumps system-side configuration" cascade, so it is natural to skip
> the repository configuration when called outside any repository but
> read the other configuration sources, which should be what happens
> when git_config() is called from outside the repository, no?
Yeah, I was studying the config.c and config.h files to understand better
how all these functions work.
The git_config() when called outside the repository, uses the global
the_repository
variable basically called the repo_config(). It does not necessarily
handle any situation
when the repo was NULL. It always uses the global the_repository variable.
I do not think we want to handle the repo_config() the same as the point of
all these are to reduce/remove the use of the_repository global variable.
While going through the config.c I saw the read_very_early_config()
which read the config
from the system and global settings and does not require any repo
variable. I think, to teach
the repo_config() to allow NULL value, we could call the
read_very_early_config() whenever
the repo is NULL as we know, this happens outside the repository. I
sent a rfc patch for this
and it can viewed here :-
https://public-inbox.org/git/20250227175456.1129840-1-usmanakinyemi202@gmail.com/T/#u
Another approach which I was thinking about is having a local
repository variable inside
the repo_config() whenever the repo variable passed to it is NULL.
What do you think ?
Thanks.
>
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH v2 02/12] builtin/verify-tag: stop using `the_repository`
2025-02-27 17:56 ` Usman Akinyemi
@ 2025-02-27 22:39 ` Junio C Hamano
0 siblings, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2025-02-27 22:39 UTC (permalink / raw)
To: Usman Akinyemi
Cc: christian.couder, git, me, chriscool, johncai86, ps, shejialuo
Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
> While going through the config.c I saw the
> read_very_early_config() which read the config from the system and
> global settings and does not require any repo variable. I think,
> to teach the repo_config() to allow NULL value, we could call the
> read_very_early_config() whenever the repo is NULL as we know,
> this happens outside the repository.
Yeah, when I wrote the message you were responding to, I noticed
that do_git_config_sequence() is prepared to take NULL as repo (even
though it is a bit clunky interface; you need to futz with members
of the opts structure like opts->ignore_repo to have the code ignore
repo that is NULL). So a caller that calls config_with_options() with
no config_source and no repo should be a good candidate to reuse.
As we positively know that we are not in a repository in our case,
using read_very_early_config() when repo is NULL sounds like a
sensible thing to do, I would think.
Thanks.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v2 03/12] builtin/verify-commit: refactor `cmd_verify_commit()`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 01/12] builtin/verify-tag: refactor `cmd_verify_tag()` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 02/12] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 04/12] builtin/verify-commit: stop using `the_repository` Usman Akinyemi
` (9 subsequent siblings)
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
Move `git_config()` call after `usage_with_options()` to avoid NULL `repo`
check.
When "-h" is passed to builtins using the RUN_SETUP macro, `repo` passed
by `run_builtin()` will be NULL. If we use the `repo` instead of the
global `the_repository` variable. We will have to switch from `git_config()`
to `repo_config()` which takes in `repo`. We must check for NULL `repo`
if `repo_config()` comes before `usage_with_options()`. Moving `git_config()`
after `usage_with_options()` eliminates this need, as `usage_with_options()`
exit before calling `repo_config()`.
This will be useful in the following patch which remove `the_repository`
global variable in favor of the `repo` passed by `run_builtin()`.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/verify-commit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 779b7988ca..ae0c625777 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -64,8 +64,6 @@ int cmd_verify_commit(int argc,
OPT_END()
};
- git_config(git_default_config, NULL);
-
argc = parse_options(argc, argv, prefix, verify_commit_options,
verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
if (argc <= i)
@@ -74,6 +72,8 @@ int cmd_verify_commit(int argc,
if (verbose)
flags |= GPG_VERIFY_VERBOSE;
+ git_config(git_default_config, NULL);
+
/* sometimes the program was terminated because this signal
* was received in the process of writing the gpg input: */
signal(SIGPIPE, SIG_IGN);
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 04/12] builtin/verify-commit: stop using `the_repository`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (2 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 03/12] builtin/verify-commit: refactor `cmd_verify_commit()` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 05/12] builtin/send-pack: refactor `cmd_send_pack()` Usman Akinyemi
` (8 subsequent siblings)
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
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, without having to consult much of the
configuration file.
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 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index ae0c625777..037032d15c 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;
@@ -72,13 +71,13 @@ int cmd_verify_commit(int argc,
if (verbose)
flags |= GPG_VERIFY_VERBOSE;
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
/* sometimes the program was terminated because this signal
* 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;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 05/12] builtin/send-pack: refactor `cmd_send_pack()`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (3 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 04/12] builtin/verify-commit: stop using `the_repository` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 06/12] builtin/send-pack: stop using `the_repository` Usman Akinyemi
` (7 subsequent siblings)
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
Move `git_config()` call after `usage_with_options()` to avoid NULL `repo`
check.
When "-h" is passed to builtins using the RUN_SETUP macro, `repo` passed
by `run_builtin()` will be NULL. If we use the `repo` instead of the
global `the_repository` variable. We will have to switch from `git_config()`
to `repo_config()` which takes in `repo`. We must check for NULL `repo`
if `repo_config()` comes before `usage_with_options()`. Moving `git_config()`
after `usage_with_options()` eliminates this need, as `usage_with_options()`
exit before calling `repo_config()`.
This will be useful in the following patch which remove `the_repository`
global variable in favor of the `repo` passed by `run_builtin()`.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/send-pack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 8d461008e2..0848d23171 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -212,7 +212,6 @@ int cmd_send_pack(int argc,
OPT_END()
};
- git_config(send_pack_config, NULL);
argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
if (argc > 0) {
dest = argv[0];
@@ -222,6 +221,8 @@ int cmd_send_pack(int argc,
if (!dest)
usage_with_options(send_pack_usage, options);
+ git_config(send_pack_config, NULL);
+
args.verbose = verbose;
args.dry_run = dry_run;
args.send_mirror = send_mirror;
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 06/12] builtin/send-pack: stop using `the_repository`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (4 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 05/12] builtin/send-pack: refactor `cmd_send_pack()` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 07/12] builtin/pack-refs: refactor `cmd_pack_refs()` Usman Akinyemi
` (6 subsequent siblings)
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
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, without having to consult much of the
configuration file.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/send-pack.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 0848d23171..0ebfc98317 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;
@@ -221,7 +220,7 @@ int cmd_send_pack(int argc,
if (!dest)
usage_with_options(send_pack_usage, options);
- git_config(send_pack_config, NULL);
+ repo_config(repo, send_pack_config, NULL);
args.verbose = verbose;
args.dry_run = dry_run;
@@ -318,7 +317,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);
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 07/12] builtin/pack-refs: refactor `cmd_pack_refs()`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (5 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 06/12] builtin/send-pack: stop using `the_repository` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 08/12] builtin/pack-refs: stop using `the_repository` Usman Akinyemi
` (5 subsequent siblings)
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
Move `git_config()` call after `usage_with_options()` to avoid NULL `repo`
check.
When "-h" is passed to builtins using the RUN_SETUP macro, `repo` passed
by `run_builtin()` will be NULL. If we use the `repo` instead of the
global `the_repository` variable. We will have to switch from `git_config()`
to `repo_config()` which takes in `repo`. We must check for NULL `repo`
if `repo_config()` comes before `usage_with_options()`. Moving `git_config()`
after `usage_with_options()` eliminates this need, as `usage_with_options()`
exit before calling `repo_config()`.
This will be useful in the following patch which remove `the_repository`
global variable in favor of the `repo` passed by `run_builtin()`.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/pack-refs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 4fdd68880e..bd09366738 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -39,10 +39,11 @@ int cmd_pack_refs(int argc,
N_("references to exclude")),
OPT_END(),
};
- git_config(git_default_config, NULL);
+
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
+ git_config(git_default_config, NULL);
for_each_string_list_item(item, &option_excluded_refs)
add_ref_exclusion(pack_refs_opts.exclusions, item->string);
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 08/12] builtin/pack-refs: stop using `the_repository`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (6 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 07/12] builtin/pack-refs: refactor `cmd_pack_refs()` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 09/12] builtin/ls-files: " Usman Akinyemi
` (4 subsequent siblings)
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
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, without having to consult much of the
configuration file.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/pack-refs.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index bd09366738..200a5516b1 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;
@@ -43,7 +41,7 @@ int cmd_pack_refs(int argc,
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
for_each_string_list_item(item, &option_excluded_refs)
add_ref_exclusion(pack_refs_opts.exclusions, item->string);
@@ -53,7 +51,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);
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 09/12] builtin/ls-files: stop using `the_repository`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (7 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 08/12] builtin/pack-refs: stop using `the_repository` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 10/12] builtin/for-each-ref: refactor `cmd_for_each_ref()` Usman Akinyemi
` (3 subsequent siblings)
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
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, without having to consult much
of the configuration file.
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 ++++++++++++++++----------------
1 file changed, 16 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");
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 10/12] builtin/for-each-ref: refactor `cmd_for_each_ref()`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (8 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 09/12] builtin/ls-files: " Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:32 ` [PATCH v2 11/12] builtin/for-each-ref: stop using `the_repository` Usman Akinyemi
` (2 subsequent siblings)
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
Move `git_config()` call after `usage_with_options()` to avoid NULL `repo`
check.
When "-h" is passed to builtins using the RUN_SETUP macro, `repo` passed
by `run_builtin()` will be NULL. If we use the `repo` instead of the
global `the_repository` variable. We will have to switch from `git_config()`
to `repo_config()` which takes in `repo`. We must check for NULL `repo`
if `repo_config()` comes before `usage_with_options()`. Moving `git_config()`
after `usage_with_options()` eliminates this need, as `usage_with_options()`
exit before calling `repo_config()`.
This will be useful in the following patch which remove `the_repository`
global variable in favor of the `repo` passed by `run_builtin()`.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/for-each-ref.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 8085ebd8fe..649689c92c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -63,8 +63,6 @@ int cmd_for_each_ref(int argc,
format.format = "%(objectname) %(objecttype)\t%(refname)";
- git_config(git_default_config, NULL);
-
/* Set default (refname) sorting */
string_list_append(&sorting_options, "refname");
@@ -80,6 +78,7 @@ int cmd_for_each_ref(int argc,
if (verify_ref_format(&format))
usage_with_options(for_each_ref_usage, opts);
+ git_config(git_default_config, NULL);
sorting = ref_sorting_options(&sorting_options);
ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
filter.ignore_case = icase;
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 11/12] builtin/for-each-ref: stop using `the_repository`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (9 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 10/12] builtin/for-each-ref: refactor `cmd_for_each_ref()` Usman Akinyemi
@ 2025-02-19 20:32 ` Usman Akinyemi
2025-02-19 20:33 ` [PATCH v2 12/12] builtin/checkout-index: " Usman Akinyemi
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:32 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
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, without having to consult much of the
configuration file.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
builtin/for-each-ref.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 649689c92c..0f73f47fab 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;
@@ -78,7 +77,7 @@ int cmd_for_each_ref(int argc,
if (verify_ref_format(&format))
usage_with_options(for_each_ref_usage, opts);
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
sorting = ref_sorting_options(&sorting_options);
ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
filter.ignore_case = icase;
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH v2 12/12] builtin/checkout-index: stop using `the_repository`
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (10 preceding siblings ...)
2025-02-19 20:32 ` [PATCH v2 11/12] builtin/for-each-ref: stop using `the_repository` Usman Akinyemi
@ 2025-02-19 20:33 ` Usman Akinyemi
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
12 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-02-19 20:33 UTC (permalink / raw)
To: gitster, christian.couder, git; +Cc: me, chriscool, johncai86, ps, shejialuo
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, without having to consult much
of the configuration file.
Pass the repository 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 ++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index e30086c7d4..46035444eb 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 repository *repo, 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(repo->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 < repo->index->cache_nr) {
+ struct cache_entry *ce =repo->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 repository *repo, 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 < repo->index->cache_nr ; i++) {
+ struct cache_entry *ce = repo->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(repo->index);
+ ce = repo->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, 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, 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, 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;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 63+ messages in thread* [Outreachy PATCH v3 0/8] stop using the_repository global variable.
2025-02-19 20:32 ` [PATCH v2 00/12][Outreachy] " Usman Akinyemi
` (11 preceding siblings ...)
2025-02-19 20:33 ` [PATCH v2 12/12] builtin/checkout-index: " Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 14:35 ` [PATCH v3 1/8] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
` (8 more replies)
12 siblings, 9 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder; +Cc: gitster, johncai86, me, ps, shejialuo
Remove `the_repository` global variable in favor of the repository
argument that gets passed in builtin commands.
These sets of commands are commands that use only RUN_SETUP macro in "git.c".
Basically, When `-h` is passed to any of this command outside a Git repository,
the `run_builtin()` will call the `cmd_x()` function (where `x` is any
of the command from the sets of builtin commands that `the_repository` is removed
from) with `repo` set to NULL and then early in the function, `parse_options()`
or show_usage_with_options_if_asked() call will give the options help
and exit.
Some, functions also uses `the_repository` global internally, so, let's
let's refactor them and pass `struct repo` as one of the argument.
As 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 make it possible for us to savely replace
`git_config()` with `repo_config()`.
Changes since v2
================
- Remove preparatory patches that move the `git_config()` below the
`usage_with_options()`.
- Add a new preparatory patch that teach the `repo_config()` to accept
NULL value for `repo` variable.
- Add tests to each of the builtin function in this commit to ensure
`repo_config()` works as expected.
- Make changes to the commit messages.
Usman Akinyemi (8):
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/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/verify-commit.c | 13 +++++-----
builtin/verify-tag.c | 7 +++---
config.c | 4 +++
config.h | 3 +++
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 ++++++
16 files changed, 110 insertions(+), 61 deletions(-)
Range-diff versus v2:
1: a313f9afb7 < -: ---------- builtin/verify-tag: refactor `cmd_verify_tag()`
-: ---------- > 1: a5c69f3753 config: teach repo_config to allow `repo` to be NULL
2: f993160d90 ! 2: dfa0da4061 builtin/verify-tag: stop using `the_repository`
@@ Commit message
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, without having to consult much of the
- configuration file.
+ the options help and exit.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
@@ builtin/verify-tag.c: static const char * const verify_tag_usage[] = {
int i = 1, verbose = 0, had_error = 0;
unsigned flags = 0;
@@ builtin/verify-tag.c: int cmd_verify_tag(int argc,
- flags |= GPG_VERIFY_OMIT_STATUS;
- }
+ OPT_END()
+ };
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
- while (i < argc) {
+ argc = parse_options(argc, argv, prefix, verify_tag_options,
+ verify_tag_usage, PARSE_OPT_KEEP_ARGV0);
+@@ builtin/verify-tag.c: int cmd_verify_tag(int argc,
struct object_id oid;
const char *name = argv[i++];
@@ builtin/verify-tag.c: int cmd_verify_tag(int argc,
had_error = !!error("tag '%s' not found.", name);
continue;
}
+
+ ## t/t7030-verify-tag.sh ##
+@@ t/t7030-verify-tag.sh: 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 &&
3: f2f785190d < -: ---------- builtin/verify-commit: refactor `cmd_verify_commit()`
4: 5f8dc0c0ec ! 3: ade2d026cb builtin/verify-commit: stop using `the_repository`
@@ Commit message
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, without having to consult much of the
- configuration file.
+ 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.
@@ builtin/verify-commit.c: static int verify_commit(const char *name, unsigned fla
int i = 1, verbose = 0, had_error = 0;
unsigned flags = 0;
@@ builtin/verify-commit.c: int cmd_verify_commit(int argc,
- if (verbose)
- flags |= GPG_VERIFY_VERBOSE;
+ OPT_END()
+ };
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
- /* sometimes the program was terminated because this signal
+ argc = parse_options(argc, argv, prefix, verify_commit_options,
+ verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
+@@ builtin/verify-commit.c: int cmd_verify_commit(int argc,
* was received in the process of writing the gpg input: */
signal(SIGPIPE, SIG_IGN);
while (i < argc)
@@ builtin/verify-commit.c: int cmd_verify_commit(int argc,
had_error = 1;
return had_error;
}
+
+ ## t/t7510-signed-commit.sh ##
+@@ t/t7510-signed-commit.sh: 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
5: d79c3fc1ee < -: ---------- builtin/send-pack: refactor `cmd_send_pack()`
6: f280a9c387 ! 4: e3b58bc6cf builtin/send-pack: stop using `the_repository`
@@ Commit message
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, without having to consult much of the
- configuration file.
+ the options help and exit.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
@@ builtin/send-pack.c: static int send_pack_config(const char *k, const char *v,
struct refspec rs = REFSPEC_INIT_PUSH;
const char *remote_name = NULL;
@@ builtin/send-pack.c: int cmd_send_pack(int argc,
- if (!dest)
- usage_with_options(send_pack_usage, options);
+ OPT_END()
+ };
- git_config(send_pack_config, NULL);
+ repo_config(repo, send_pack_config, NULL);
-
- args.verbose = verbose;
- args.dry_run = dry_run;
+ argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
+ if (argc > 0) {
+ dest = argv[0];
@@ builtin/send-pack.c: int cmd_send_pack(int argc,
set_ref_status_for_push(remote_refs, args.send_mirror,
args.force_update);
@@ builtin/send-pack.c: int cmd_send_pack(int argc,
if (helper_status)
print_helper_status(remote_refs);
+
+ ## t/t5400-send-pack.sh ##
+@@ t/t5400-send-pack.sh: 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
7: df192a1b2c < -: ---------- builtin/pack-refs: refactor `cmd_pack_refs()`
8: 845cd1ea7c ! 5: b11e99627c builtin/pack-refs: stop using `the_repository`
@@ Commit message
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, without having to consult much of the
- configuration file.
+ the options help and exit.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
@@ builtin/pack-refs.c: static char const * const pack_refs_usage[] = {
struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;
struct string_list included_refs = STRING_LIST_INIT_NODUP;
@@ builtin/pack-refs.c: int cmd_pack_refs(int argc,
- if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
- usage_with_options(pack_refs_usage, opts);
-
+ N_("references to exclude")),
+ OPT_END(),
+ };
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
- for_each_string_list_item(item, &option_excluded_refs)
- add_ref_exclusion(pack_refs_opts.exclusions, item->string);
+ if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
+ usage_with_options(pack_refs_usage, opts);
@@ builtin/pack-refs.c: int cmd_pack_refs(int argc,
if (!pack_refs_opts.includes->nr)
@@ builtin/pack-refs.c: int cmd_pack_refs(int argc,
clear_ref_exclusions(&excludes);
string_list_clear(&included_refs, 0);
+
+ ## t/t0610-reftable-basics.sh ##
+@@ t/t0610-reftable-basics.sh: 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 &&
9: 2aff17b09c ! 6: 51c80f9273 builtin/ls-files: stop using `the_repository`
@@ Commit message
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, without having to consult much
- of the configuration file.
+ 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
@@ builtin/ls-files.c: int cmd_ls_files(int argc,
if (ps_matched && report_path_error(ps_matched, &pathspec)) {
fprintf(stderr, "Did you forget to 'git add'?\n");
+
+ ## t/t3004-ls-files-basic.sh ##
+@@ t/t3004-ls-files-basic.sh: 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 &&
10: 4cc9a4935a < -: ---------- builtin/for-each-ref: refactor `cmd_for_each_ref()`
11: 42c1a7ae74 ! 7: 63bb89291f builtin/for-each-ref: stop using `the_repository`
@@ Commit message
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, without having to consult much of the
- configuration file.
+ 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: static char const * const for_each_ref_usage[] = {
struct ref_sorting *sorting;
struct string_list sorting_options = STRING_LIST_INIT_DUP;
@@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc,
- if (verify_ref_format(&format))
- usage_with_options(for_each_ref_usage, opts);
+
+ format.format = "%(objectname) %(objecttype)\t%(refname)";
- git_config(git_default_config, NULL);
+ repo_config(repo, git_default_config, NULL);
- sorting = ref_sorting_options(&sorting_options);
- ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
- filter.ignore_case = icase;
+
+ /* Set default (refname) sorting */
+ string_list_append(&sorting_options, "refname");
+
+ ## t/t6300-for-each-ref.sh ##
+@@ t/t6300-for-each-ref.sh: 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 &&
12: 232cbef160 ! 8: 8dfe6b40c8 builtin/checkout-index: stop using `the_repository`
@@ Commit message
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, without having to consult much
- of the configuration file.
+ call will give the options help and exit.
Pass the repository available in the calling context to both `checkout_all()`
and `checkout_file()` to remove their dependency on the global
@@ builtin/checkout-index.c: int cmd_checkout_index(int argc,
die("Unable to write new index file");
return 0;
}
+
+ ## t/t2006-checkout-index-basic.sh ##
+@@ t/t2006-checkout-index-basic.sh: 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 [flat|nested] 63+ messages in thread* [PATCH v3 1/8] config: teach repo_config to allow `repo` to be NULL
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 17:53 ` Junio C Hamano
2025-03-06 14:35 ` [PATCH v3 2/8] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
` (7 subsequent siblings)
8 siblings, 1 reply; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, 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 | 3 +++
2 files changed, 7 insertions(+)
diff --git a/config.c b/config.c
index 36f76fafe5..c5181fd23b 100644
--- a/config.c
+++ b/config.c
@@ -2526,6 +2526,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..1e5b22dfc4 100644
--- a/config.h
+++ b/config.h
@@ -219,6 +219,9 @@ 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
+ * call read_early_config().
+ *
* 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] 63+ messages in thread* Re: [PATCH v3 1/8] config: teach repo_config to allow `repo` to be NULL
2025-03-06 14:35 ` [PATCH v3 1/8] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
@ 2025-03-06 17:53 ` Junio C Hamano
2025-03-07 1:33 ` Usman Akinyemi
2025-03-07 10:37 ` Phillip Wood
0 siblings, 2 replies; 63+ messages in thread
From: Junio C Hamano @ 2025-03-06 17:53 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, johncai86, me, ps, shejialuo,
Christian Couder
Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
> 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..1e5b22dfc4 100644
> --- a/config.h
> +++ b/config.h
> @@ -219,6 +219,9 @@ 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
> + * call read_early_config().
> + *
early or very early?
I am wondering if we should describe the effect we want out of the
design more prominently than the way we try to obtain the effect
here. In other words, instead of (rather, in addition to) saying
that we call helper X, wouldn't it be more helpful to future
developers why we call X, to convey the intent, so that they know
how to adjust when for example what X does change or X even
disappears? E.g.,
When repo==NULL, skip reading the per-repository
configuration file but still use the system- and globa-
configuration, by calling X. Note that this ignores
one-time configuration override "git -c var=val" given from
the command line. The only use case the feature to allow
passing repo==NULL was designed for 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).
That way, folks who are planning to update read_veriy_early_config()
so that it pays attention to the "git -c var=val" in the future will
be rest assured that they won't be breaking this caller with their
planned change.
Of course I didn't spend enough brainpower to make the above comment
more concise and to the point, which the final version should be,
but hopefully you got the idea.
Thanks.
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH v3 1/8] config: teach repo_config to allow `repo` to be NULL
2025-03-06 17:53 ` Junio C Hamano
@ 2025-03-07 1:33 ` Usman Akinyemi
2025-03-07 10:37 ` Phillip Wood
1 sibling, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 1:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, christian.couder, johncai86, me, ps, shejialuo,
Christian Couder
On Thu, Mar 6, 2025 at 11:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>
> > 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..1e5b22dfc4 100644
> > --- a/config.h
> > +++ b/config.h
> > @@ -219,6 +219,9 @@ 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
> > + * call read_early_config().
> > + *
>
> early or very early?
>
> I am wondering if we should describe the effect we want out of the
> design more prominently than the way we try to obtain the effect
> here. In other words, instead of (rather, in addition to) saying
> that we call helper X, wouldn't it be more helpful to future
> developers why we call X, to convey the intent, so that they know
> how to adjust when for example what X does change or X even
> disappears? E.g.,
>
> When repo==NULL, skip reading the per-repository
> configuration file but still use the system- and globa-
> configuration, by calling X. Note that this ignores
> one-time configuration override "git -c var=val" given from
> the command line. The only use case the feature to allow
> passing repo==NULL was designed for 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).
>
> That way, folks who are planning to update read_veriy_early_config()
> so that it pays attention to the "git -c var=val" in the future will
> be rest assured that they won't be breaking this caller with their
> planned change.
>
> Of course I didn't spend enough brainpower to make the above comment
> more concise and to the point, which the final version should be,
> but hopefully you got the idea.
Yeah, thanks Junio, I understand clearly. Your comment is well explained also.
I will refine it and add it in an updated patch.
Thank you.
>
> Thanks.
>
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH v3 1/8] config: teach repo_config to allow `repo` to be NULL
2025-03-06 17:53 ` Junio C Hamano
2025-03-07 1:33 ` Usman Akinyemi
@ 2025-03-07 10:37 ` Phillip Wood
1 sibling, 0 replies; 63+ messages in thread
From: Phillip Wood @ 2025-03-07 10:37 UTC (permalink / raw)
To: Junio C Hamano, Usman Akinyemi
Cc: git, christian.couder, johncai86, me, ps, shejialuo,
Christian Couder
On 06/03/2025 17:53, Junio C Hamano wrote:
> Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>
> I am wondering if we should describe the effect we want out of the
> design more prominently than the way we try to obtain the effect
> here. In other words, instead of (rather, in addition to) saying
> that we call helper X, wouldn't it be more helpful to future
> developers why we call X, to convey the intent, so that they know
> how to adjust when for example what X does change or X even
> disappears? E.g.,
>
> When repo==NULL, skip reading the per-repository
> configuration file but still use the system- and globa-
> configuration, by calling X. Note that this ignores
> one-time configuration override "git -c var=val" given from
> the command line. The only use case the feature to allow
> passing repo==NULL was designed for 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).
I think that would be very helpful as it makes it clear why we don't
care about config on the commandline for this series.
Best Wishes
Phillip
> That way, folks who are planning to update read_veriy_early_config()
> so that it pays attention to the "git -c var=val" in the future will
> be rest assured that they won't be breaking this caller with their
> planned change.
>
> Of course I didn't spend enough brainpower to make the above comment
> more concise and to the point, which the final version should be,
> but hopefully you got the idea.
>
> Thanks.
>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v3 2/8] builtin/verify-tag: stop using `the_repository`
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
2025-03-06 14:35 ` [PATCH v3 1/8] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 17:56 ` Junio C Hamano
2025-03-06 14:35 ` [PATCH v3 3/8] builtin/verify-commit: " Usman Akinyemi
` (6 subsequent siblings)
8 siblings, 1 reply; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, 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] 63+ messages in thread* Re: [PATCH v3 2/8] builtin/verify-tag: stop using `the_repository`
2025-03-06 14:35 ` [PATCH v3 2/8] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
@ 2025-03-06 17:56 ` Junio C Hamano
0 siblings, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2025-03-06 17:56 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, johncai86, me, ps, shejialuo,
Christian Couder
Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
> 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.
Makes sense.
> 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
> +'
OK. I am not sure if we want to insist that the "-h" invocation
exits with status 129, but changing it would be totaly outside the
topic.
Will queue.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v3 3/8] builtin/verify-commit: stop using `the_repository`
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
2025-03-06 14:35 ` [PATCH v3 1/8] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
2025-03-06 14:35 ` [PATCH v3 2/8] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 14:35 ` [PATCH v3 4/8] builtin/send-pack: " Usman Akinyemi
` (5 subsequent siblings)
8 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, 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] 63+ messages in thread* [PATCH v3 4/8] builtin/send-pack: stop using `the_repository`
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
` (2 preceding siblings ...)
2025-03-06 14:35 ` [PATCH v3 3/8] builtin/verify-commit: " Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 14:35 ` [PATCH v3 5/8] builtin/pack-refs: " Usman Akinyemi
` (4 subsequent siblings)
8 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, 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] 63+ messages in thread* [PATCH v3 5/8] builtin/pack-refs: stop using `the_repository`
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
` (3 preceding siblings ...)
2025-03-06 14:35 ` [PATCH v3 4/8] builtin/send-pack: " Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 14:35 ` [PATCH v3 6/8] builtin/ls-files: " Usman Akinyemi
` (3 subsequent siblings)
8 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, 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] 63+ messages in thread* [PATCH v3 6/8] builtin/ls-files: stop using `the_repository`
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
` (4 preceding siblings ...)
2025-03-06 14:35 ` [PATCH v3 5/8] builtin/pack-refs: " Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 17:59 ` Junio C Hamano
2025-03-06 14:35 ` [PATCH v3 7/8] builtin/for-each-ref: " Usman Akinyemi
` (2 subsequent siblings)
8 siblings, 1 reply; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, 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] 63+ messages in thread* [PATCH v3 7/8] builtin/for-each-ref: stop using `the_repository`
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
` (5 preceding siblings ...)
2025-03-06 14:35 ` [PATCH v3 6/8] builtin/ls-files: " Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 14:35 ` [PATCH v3 8/8] builtin/checkout-index: " Usman Akinyemi
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
8 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, 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] 63+ messages in thread* [PATCH v3 8/8] builtin/checkout-index: stop using `the_repository`
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
` (6 preceding siblings ...)
2025-03-06 14:35 ` [PATCH v3 7/8] builtin/for-each-ref: " Usman Akinyemi
@ 2025-03-06 14:35 ` Usman Akinyemi
2025-03-06 18:18 ` Junio C Hamano
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
8 siblings, 1 reply; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-06 14:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, 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 the repository 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..46035444eb 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 repository *repo, 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(repo->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 < repo->index->cache_nr) {
+ struct cache_entry *ce =repo->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 repository *repo, 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 < repo->index->cache_nr ; i++) {
+ struct cache_entry *ce = repo->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(repo->index);
+ ce = repo->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, 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, 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, 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] 63+ messages in thread* Re: [PATCH v3 8/8] builtin/checkout-index: stop using `the_repository`
2025-03-06 14:35 ` [PATCH v3 8/8] builtin/checkout-index: " Usman Akinyemi
@ 2025-03-06 18:18 ` Junio C Hamano
2025-03-07 1:15 ` Usman Akinyemi
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2025-03-06 18:18 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, johncai86, me, ps, shejialuo,
Christian Couder
Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
> 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 the repository available in the calling context to both `checkout_all()`
> and `checkout_file()` to remove their dependency on the global
> `the_repository` variable.
Hmph, if we are passing anything down to these code paths, I would
have expected that it would be an instance of "struct index_state".
Do these two helper functions need anything other than that from the
repository instance?
Other than that, I think this step does look great.
Will queue.
Thanks.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v3 8/8] builtin/checkout-index: stop using `the_repository`
2025-03-06 18:18 ` Junio C Hamano
@ 2025-03-07 1:15 ` Usman Akinyemi
2025-03-07 14:15 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 1:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, christian.couder, johncai86, me, ps, shejialuo,
Christian Couder
On Thu, Mar 6, 2025 at 11:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>
> > 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 the repository available in the calling context to both `checkout_all()`
> > and `checkout_file()` to remove their dependency on the global
> > `the_repository` variable.
>
> Hmph, if we are passing anything down to these code paths, I would
> have expected that it would be an instance of "struct index_state".
>
> Do these two helper functions need anything other than that from the
> repository instance?
No, they do not. They could possibly do in the future and is there any
reason why we might want to pass the "struct index_state" instead of
the whole "struct repository" ?
>
> Other than that, I think this step does look great.
>
> Will queue.
>
> Thanks.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v3 8/8] builtin/checkout-index: stop using `the_repository`
2025-03-07 1:15 ` Usman Akinyemi
@ 2025-03-07 14:15 ` Junio C Hamano
2025-03-07 19:41 ` Usman Akinyemi
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2025-03-07 14:15 UTC (permalink / raw)
To: Usman Akinyemi
Cc: git, christian.couder, johncai86, me, ps, shejialuo,
Christian Couder
Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>> Hmph, if we are passing anything down to these code paths, I would
>> have expected that it would be an instance of "struct index_state".
>>
>> Do these two helper functions need anything other than that from the
>> repository instance?
> No, they do not. They could possibly do in the future and is there any
> reason why we might want to pass the "struct index_state" instead of
> the whole "struct repository" ?
You are asking a wrong question.
When there are two things, A and B, where B is a piece of data with
a smaller scope that is contained within A, unless your function
needs to access parts of A that is impossible to access if you fed
it only B, limiting the interface to the smallest piece necessary
(in this case, B) is always the better design. A potential caller
that only has B and not A can still call your function that takes B
if you designed the API that way. If you insist that the caller
pass A (and infer which B to use as part of A), a potential caller
that only has B cannot call your function.
So, with that understanding of a general principle in mind, you need
a stronger justification to pass the larger object A that goes
against the best practice, saying "Even though we do not have to
pass A because only B is necessary, we pass A because ...". And you
just said there is no such reason why you need a repository and for
these functions an index_state is sufficient.
There are many functions that only need "struct index_state"
instance and obtains one from their callers. They do not have a
repository object, other than the one that is referred to from the
index_state (to keep track of which repository it originated from).
But this pointer is not designed to round-trip. There can be, and
there indeed are, code paths that use multiple index_state instances
associated with one repository. But a repository instance has only
one ".index" member. It means if you pass a repository, you are
making it impossible for your potential callers that has secondary
index_state instances.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v3 8/8] builtin/checkout-index: stop using `the_repository`
2025-03-07 14:15 ` Junio C Hamano
@ 2025-03-07 19:41 ` Usman Akinyemi
0 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 19:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, christian.couder, johncai86, me, ps, shejialuo,
Christian Couder
On Fri, Mar 7, 2025 at 7:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>
> >> Hmph, if we are passing anything down to these code paths, I would
> >> have expected that it would be an instance of "struct index_state".
> >>
> >> Do these two helper functions need anything other than that from the
> >> repository instance?
> > No, they do not. They could possibly do in the future and is there any
> > reason why we might want to pass the "struct index_state" instead of
> > the whole "struct repository" ?
>
> You are asking a wrong question.
>
> When there are two things, A and B, where B is a piece of data with
> a smaller scope that is contained within A, unless your function
> needs to access parts of A that is impossible to access if you fed
> it only B, limiting the interface to the smallest piece necessary
> (in this case, B) is always the better design. A potential caller
> that only has B and not A can still call your function that takes B
> if you designed the API that way. If you insist that the caller
> pass A (and infer which B to use as part of A), a potential caller
> that only has B cannot call your function.
>
> So, with that understanding of a general principle in mind, you need
> a stronger justification to pass the larger object A that goes
> against the best practice, saying "Even though we do not have to
> pass A because only B is necessary, we pass A because ...". And you
> just said there is no such reason why you need a repository and for
> these functions an index_state is sufficient.
>
> There are many functions that only need "struct index_state"
> instance and obtains one from their callers. They do not have a
> repository object, other than the one that is referred to from the
> index_state (to keep track of which repository it originated from).
>
> But this pointer is not designed to round-trip. There can be, and
> there indeed are, code paths that use multiple index_state instances
> associated with one repository. But a repository instance has only
> one ".index" member. It means if you pass a repository, you are
> making it impossible for your potential callers that has secondary
> index_state instances.
Thanks for the explanation.
I did not know or think about this before.
I will make changes to this in the next iteration.
Thank you.
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [Outreachy PATCH v4 0/8] stop using the_repository global variable.
2025-03-06 14:35 ` [Outreachy PATCH v3 0/8] stop using the_repository global variable Usman Akinyemi
` (7 preceding siblings ...)
2025-03-06 14:35 ` [PATCH v3 8/8] builtin/checkout-index: " Usman Akinyemi
@ 2025-03-07 23:34 ` Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 1/8] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
` (7 more replies)
8 siblings, 8 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:34 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123
Remove `the_repository` global variable in favor of the repository
argument that gets passed in builtin commands.
These sets of commands are commands that use only RUN_SETUP macro in "git.c".
Basically, When `-h` is passed to any of this command outside a Git repository,
the `run_builtin()` will call the `cmd_x()` function (where `x` is any
of the command from the sets of builtin commands that `the_repository` is removed
from) with `repo` set to NULL and then early in the function, `parse_options()`
or show_usage_with_options_if_asked() call will give the options help
and exit.
Some functions also uses `the_repository` global internally, so, let's
let's refactor them and pass `struct repo` as one of the argument. Some
functions only need an instance of "struct index_state", so, let's pass
it to such functions.
As 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 make it possible for us to savely replace
`git_config()` with `repo_config()`.
Changes since v3
================
- Add a comment to describe why we teach `repo_config()` to take NULL
repo value and what we intend to achieve.
- Pass "struct index_state" instead of repo in functions inside
"builtin/checkout-index.c"
- Fix some typo.
Usman Akinyemi (8):
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/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/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 ++++++
16 files changed, 116 insertions(+), 61 deletions(-)
Range-diff versus v3:
1: a5c69f3753 ! 1: f53677cbd6 config: teach repo_config to allow `repo` to be NULL
@@ config.h: void read_very_early_config(config_fn_t cb, void *data);
* value is left at the end).
*
+ * In cases where the repository variable is NULL, repo_config() will
-+ * call read_early_config().
++ * 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.
*/
2: dfa0da4061 = 2: 6560218b7a builtin/verify-tag: stop using `the_repository`
3: ade2d026cb = 3: 22681bad00 builtin/verify-commit: stop using `the_repository`
4: e3b58bc6cf = 4: a2a97b10a6 builtin/send-pack: stop using `the_repository`
5: b11e99627c = 5: b88e45e795 builtin/pack-refs: stop using `the_repository`
6: 51c80f9273 = 6: d976fab012 builtin/ls-files: stop using `the_repository`
7: 63bb89291f = 7: 6a44666310 builtin/for-each-ref: stop using `the_repository`
8: 8dfe6b40c8 ! 8: 677e088e55 builtin/checkout-index: stop using `the_repository`
@@ Commit message
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 `checkout_all()`
- and `checkout_file()` to remove their dependency on the global
- `the_repository` variable.
+ 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: static void write_tempfile_record(const char *name, co
}
-static int checkout_file(const char *name, const char *prefix)
-+static int checkout_file(struct repository *repo, 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(repo->index, name, namelen);
++ int pos = index_name_pos(index, name, namelen);
int has_same_name = 0;
int is_file = 0;
int is_skipped = 1;
@@ builtin/checkout-index.c: static int checkout_file(const char *name, const char
- while (pos <the_repository->index->cache_nr) {
- struct cache_entry *ce =the_repository->index->cache[pos];
-+ while (pos < repo->index->cache_nr) {
-+ struct cache_entry *ce =repo->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;
@@ builtin/checkout-index.c: static int checkout_file(const char *name, const char
}
-static int checkout_all(const char *prefix, int prefix_length)
-+static int checkout_all(struct repository *repo, 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 < repo->index->cache_nr ; i++) {
-+ struct cache_entry *ce = repo->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))
@@ builtin/checkout-index.c: static int checkout_all(const char *prefix, int prefix
if (ignore_skip_worktree) {
- ensure_full_index(the_repository->index);
- ce = the_repository->index->cache[i];
-+ ensure_full_index(repo->index);
-+ ce = repo->index->cache[i];
++ ensure_full_index(index);
++ ce = index->cache[i];
}
}
@@ builtin/checkout-index.c: int cmd_checkout_index(int argc,
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, p, prefix);
++ err |= checkout_file(repo->index, p, prefix);
free(p);
}
@@ builtin/checkout-index.c: int cmd_checkout_index(int argc,
}
p = prefix_path(prefix, prefix_length, buf.buf);
- err |= checkout_file(p, prefix);
-+ err |= checkout_file(repo, p, prefix);
++ err |= checkout_file(repo->index, p, prefix);
free(p);
}
strbuf_release(&unquoted);
@@ builtin/checkout-index.c: int cmd_checkout_index(int argc,
if (all)
- err |= checkout_all(prefix, prefix_length);
-+ err |= checkout_all(repo, prefix, prefix_length);
++ err |= checkout_all(repo->index, prefix, prefix_length);
if (pc_workers > 1)
err |= run_parallel_checkout(&state, pc_workers, pc_threshold,
--
2.48.1
^ permalink raw reply [flat|nested] 63+ messages in thread* [PATCH v4 1/8] config: teach repo_config to allow `repo` to be NULL
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
@ 2025-03-07 23:35 ` Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 2/8] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
` (6 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123,
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 36f76fafe5..c5181fd23b 100644
--- a/config.c
+++ b/config.c
@@ -2526,6 +2526,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] 63+ messages in thread* [PATCH v4 2/8] builtin/verify-tag: stop using `the_repository`
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 1/8] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
@ 2025-03-07 23:35 ` Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 3/8] builtin/verify-commit: " Usman Akinyemi
` (5 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123,
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] 63+ messages in thread* [PATCH v4 3/8] builtin/verify-commit: stop using `the_repository`
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 1/8] config: teach repo_config to allow `repo` to be NULL Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 2/8] builtin/verify-tag: stop using `the_repository` Usman Akinyemi
@ 2025-03-07 23:35 ` Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 4/8] builtin/send-pack: " Usman Akinyemi
` (4 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123,
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] 63+ messages in thread* [PATCH v4 4/8] builtin/send-pack: stop using `the_repository`
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
` (2 preceding siblings ...)
2025-03-07 23:35 ` [PATCH v4 3/8] builtin/verify-commit: " Usman Akinyemi
@ 2025-03-07 23:35 ` Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 5/8] builtin/pack-refs: " Usman Akinyemi
` (3 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123,
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] 63+ messages in thread* [PATCH v4 5/8] builtin/pack-refs: stop using `the_repository`
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
` (3 preceding siblings ...)
2025-03-07 23:35 ` [PATCH v4 4/8] builtin/send-pack: " Usman Akinyemi
@ 2025-03-07 23:35 ` Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 6/8] builtin/ls-files: " Usman Akinyemi
` (2 subsequent siblings)
7 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123,
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] 63+ messages in thread* [PATCH v4 6/8] builtin/ls-files: stop using `the_repository`
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
` (4 preceding siblings ...)
2025-03-07 23:35 ` [PATCH v4 5/8] builtin/pack-refs: " Usman Akinyemi
@ 2025-03-07 23:35 ` Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 7/8] builtin/for-each-ref: " Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 8/8] builtin/checkout-index: " Usman Akinyemi
7 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123,
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] 63+ messages in thread* [PATCH v4 7/8] builtin/for-each-ref: stop using `the_repository`
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
` (5 preceding siblings ...)
2025-03-07 23:35 ` [PATCH v4 6/8] builtin/ls-files: " Usman Akinyemi
@ 2025-03-07 23:35 ` Usman Akinyemi
2025-03-07 23:35 ` [PATCH v4 8/8] builtin/checkout-index: " Usman Akinyemi
7 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123,
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] 63+ messages in thread* [PATCH v4 8/8] builtin/checkout-index: stop using `the_repository`
2025-03-07 23:34 ` [Outreachy PATCH v4 0/8] stop using the_repository global variable Usman Akinyemi
` (6 preceding siblings ...)
2025-03-07 23:35 ` [PATCH v4 7/8] builtin/for-each-ref: " Usman Akinyemi
@ 2025-03-07 23:35 ` Usman Akinyemi
7 siblings, 0 replies; 63+ messages in thread
From: Usman Akinyemi @ 2025-03-07 23:35 UTC (permalink / raw)
To: git, christian.couder
Cc: gitster, johncai86, me, ps, shejialuo, phillip.wood123,
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] 63+ messages in thread