* Re: [PATCH v3 3/5] config: report config parse errors using cb
From: Taylor Blau @ 2023-10-23 19:29 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, jonathantanmy, calvinwan, glencbz, gitster
In-Reply-To: <a888045c04d27864edf5751ea8641fdba596779c.1695330852.git.steadmon@google.com>
On Thu, Sep 21, 2023 at 02:17:22PM -0700, Josh Steadmon wrote:
> diff --git a/bundle-uri.c b/bundle-uri.c
> index f93ca6a486..856bffdcad 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -237,9 +237,7 @@ int bundle_uri_parse_config_format(const char *uri,
> struct bundle_list *list)
> {
> int result;
> - struct config_parse_options opts = {
> - .error_action = CONFIG_ERROR_ERROR,
> - };
> + struct config_parse_options opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
I'm nit-picking, but I find this parameterized initializer macro to be a
little unusual w.r.t our usual conventions.
In terms of "usual conventions," I'm thinking about STRING_LIST_INIT_DUP
versus STRING_LIST_INIT_NODUP (as opposed to something like
STRING_LIST_INIT(DUP) or STRING_LIST_INIT(NODUP)).
Since there are only two possible values (the ones corresponding to
error() and die()) I wonder if something like CP_OPTS_INIT_ERROR and
CP_OPTS_INIT_DIE might be more appropriate. If you don't like either of
those, I'd suggest making the initializer a function instead of a
parameterized macro.
> if (!list->baseURI) {
> struct strbuf baseURI = STRBUF_INIT;
> diff --git a/config.c b/config.c
> index ff138500a2..0c4f1a2874 100644
> --- a/config.c
> +++ b/config.c
> @@ -55,7 +55,6 @@ struct config_source {
> enum config_origin_type origin_type;
> const char *name;
> const char *path;
> - enum config_error_action default_error_action;
> int linenr;
> int eof;
> size_t total_len;
> @@ -185,13 +184,15 @@ static int handle_path_include(const struct key_value_info *kvi,
> }
>
> if (!access_or_die(path, R_OK, 0)) {
> + struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
> +
> if (++inc->depth > MAX_INCLUDE_DEPTH)
> die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
> !kvi ? "<unknown>" :
> kvi->filename ? kvi->filename :
> "the command line");
> ret = git_config_from_file_with_options(git_config_include, path, inc,
> - kvi->scope, NULL);
> + kvi->scope, &config_opts);
...OK, so using the CONFIG_ERROR_DIE variant seems like the right choice
here because git_config_from_file_with_options() calls
do_config_from_file() which sets its default_error_action as
CONFIG_ERROR_DIE.
> static uintmax_t get_unit_factor(const char *end)
> @@ -2023,7 +2052,6 @@ static int do_config_from_file(config_fn_t fn,
> top.origin_type = origin_type;
> top.name = name;
> top.path = path;
> - top.default_error_action = CONFIG_ERROR_DIE;
> top.do_fgetc = config_file_fgetc;
> top.do_ungetc = config_file_ungetc;
> top.do_ftell = config_file_ftell;
> @@ -2037,8 +2065,10 @@ static int do_config_from_file(config_fn_t fn,
> static int git_config_from_stdin(config_fn_t fn, void *data,
> enum config_scope scope)
> {
> + struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
> +
> return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
> - data, scope, NULL);
> + data, scope, &config_opts);
Same here.
> int git_config_from_file_with_options(config_fn_t fn, const char *filename,
> @@ -2061,8 +2091,10 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
>
> int git_config_from_file(config_fn_t fn, const char *filename, void *data)
> {
> + struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
> +
> return git_config_from_file_with_options(fn, filename, data,
> - CONFIG_SCOPE_UNKNOWN, NULL);
> + CONFIG_SCOPE_UNKNOWN, &config_opts);
> }
And here.
> @@ -2098,6 +2129,7 @@ int git_config_from_blob_oid(config_fn_t fn,
> char *buf;
> unsigned long size;
> int ret;
> + struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
>
> buf = repo_read_object_file(repo, oid, &type, &size);
> if (!buf)
> @@ -2108,7 +2140,7 @@ int git_config_from_blob_oid(config_fn_t fn,
> }
>
> ret = git_config_from_mem(fn, CONFIG_ORIGIN_BLOB, name, buf, size,
> - data, scope, NULL);
> + data, scope, &config_opts);
> free(buf);
This one uses git_config_from_mem(), which sets the default error action
to "CONFIG_ERROR_ERROR", so this transformation looks correct.
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Jacob Stopak @ 2023-10-23 19:29 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Dragan Simic, git
In-Reply-To: <ZTa4iqe0lqn/Yg5L@ugly>
On Mon, Oct 23, 2023 at 08:16:42PM +0200, Oswald Buddenhagen wrote:
> On Mon, Oct 23, 2023 at 10:30:54AM -0700, Jacob Stopak wrote:
> > On Mon, Oct 23, 2023 at 04:34:15PM +0200, Dragan Simic wrote:
> > > On 2023-10-23 12:52, Oswald Buddenhagen wrote:
> > > > i for one think that it would be a perfectly valid experiment to go
> > > > all-in and beyond with jacob's proposal - _and make it the default_
> > >
> > > I'd never support that, FWIW.
> >
> > FWIW, I'd _never suggest_ that.
> >
> why, though?
> doing that would extend the feature's reach about two orders of magnitude
> among newbies, which is where it matters most.
To be honest it never even popped into my head to contemplate that and
how the user experience might be impacted by making it the default. I
assume the big distinction is "would it help more users than it would
annoy"?
I always saw this feature as a helper to be invoked when the user is in need
as opposed to a default, similar to the -n dry run option on some commands.
For brand new users, I can see what you mean since they would likely not
know the --table format exists unless being instructed by someone else.
It would be kindof a shame for this capability to exist but not be taken
advantage of by the folks who need it most - the newbies running "git
status" literally for the very first time.
But the main drawback is that although the --table for status does provide
some visual clarity and tangibility, the status command doesn't actually
_do_ anything, so the arrows showing how changes move around don't apply.
Those arrows showing how things move only really apply to "simulating"
(dry runs) for specific commands like add, restore, rm, commit, stash,
etc, so making the --table proposal a default status output would still
miss those scenarios.
However, now that I'm thinking about it maybe it could somehow be included
in the Hints feature? I honestly don't know exactly when the hints are
currently invoked or how much detail they go into, but what just popped
into my head is kindof a "universal dry run" option, which would show the
user the --table format hint when they invoke an applicable command, and
prompt them if they actually want to run it.
>
> > I very much value Git's current usage and wouldn't dream to make this
> > the default.
> >
> making the default output format somewhat more verbose wouldn't really
> "change the usage", though. and being able to permanently get rid of it with
> a single command should alleviate any _reasonable_ concerns about habit
> disruption.
>
> regards
It's a good point too that people surprised or annoyed or disgusted by the
change of a longstanding status output format could just disable the
configuration with a single config command...
^ permalink raw reply
* [PATCH v2] doc/git-bisect: clarify `git bisect run` syntax
From: cousteau via GitGitGadget @ 2023-10-23 19:23 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Patrick Steinhardt, Javier Mora, cousteau,
Javier Mora
In-Reply-To: <pull.1602.git.1698004968582.gitgitgadget@gmail.com>
From: Javier Mora <cousteaulecommandant@gmail.com>
The description of the `git bisect run` command syntax at the beginning
of the manpage is `git bisect run <cmd>...`, which isn't quite clear
about what `<cmd>` is or what the `...` mean; one could think that it is
the whole (quoted) command line with all arguments in a single string,
or that it supports multiple commands, or that it doesn't accept
commands with arguments at all.
Change to `git bisect run <cmd> [<arg>...]` to clarify the syntax,
in both the manpage and the `git bisect -h` command output.
Additionally, change `--term-{new,bad}` et al to `--term-(new|bad)`
for consistency with the synopsis syntax conventions.
Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
---
doc/git-bisect: clarify git bisect run syntax
I saw someone in IRC wondering about the syntax for git bisect run for a
command with arguments, and found that its short description at the
beginning of the manpage is not very clear (although it gets clarified
later when it is properly described). It describes the syntax as git
bisect run <cmd>... which is a bit confusing; it should say git bisect
run <cmd> [<arg>...], otherwise it somehow looks like you have to "enter
one or more commands", and that each command is a single argument.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1602%2Fcousteaulecommandant%2Fman-git-bisect-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1602/cousteaulecommandant/man-git-bisect-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1602
Range-diff vs v1:
1: ce4c60a6f4f ! 1: 8de70bb060e doc/git-bisect: clarify `git bisect run` syntax
@@ Commit message
or that it supports multiple commands, or that it doesn't accept
commands with arguments at all.
- Change to `git bisect run <cmd> [<arg>...]` to clarify the syntax.
+ Change to `git bisect run <cmd> [<arg>...]` to clarify the syntax,
+ in both the manpage and the `git bisect -h` command output.
+
+ Additionally, change `--term-{new,bad}` et al to `--term-(new|bad)`
+ for consistency with the synopsis syntax conventions.
Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
## Documentation/git-bisect.txt ##
+@@ Documentation/git-bisect.txt: DESCRIPTION
+ The command takes various subcommands, and different options depending
+ on the subcommand:
+
+- git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]
++ git bisect start [--term-(new|bad)=<term-new> --term-(old|good)=<term-old>]
+ [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
+ git bisect (bad|new|<term-new>) [<rev>]
+ git bisect (good|old|<term-old>) [<rev>...]
@@ Documentation/git-bisect.txt: on the subcommand:
git bisect (visualize|view)
git bisect replay <logfile>
@@ Documentation/git-bisect.txt: on the subcommand:
git bisect help
This command uses a binary search algorithm to find which commit in
+
+ ## builtin/bisect.c ##
+@@ builtin/bisect.c: static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
+ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
+
+ #define BUILTIN_GIT_BISECT_START_USAGE \
+- N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]" \
++ N_("git bisect start [--term-(new|bad)=<term> --term-(old|good)=<term>]" \
+ " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--]" \
+ " [<pathspec>...]")
+ #define BUILTIN_GIT_BISECT_STATE_USAGE \
+@@ builtin/bisect.c: static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
+ #define BUILTIN_GIT_BISECT_LOG_USAGE \
+ "git bisect log"
+ #define BUILTIN_GIT_BISECT_RUN_USAGE \
+- N_("git bisect run <cmd>...")
++ N_("git bisect run <cmd> [<arg>...]")
+
+ static const char * const git_bisect_usage[] = {
+ BUILTIN_GIT_BISECT_START_USAGE,
Documentation/git-bisect.txt | 4 ++--
builtin/bisect.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 7872dba3aef..191b4a42b6d 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -16,7 +16,7 @@ DESCRIPTION
The command takes various subcommands, and different options depending
on the subcommand:
- git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]
+ git bisect start [--term-(new|bad)=<term-new> --term-(old|good)=<term-old>]
[--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
git bisect (bad|new|<term-new>) [<rev>]
git bisect (good|old|<term-old>) [<rev>...]
@@ -26,7 +26,7 @@ on the subcommand:
git bisect (visualize|view)
git bisect replay <logfile>
git bisect log
- git bisect run <cmd>...
+ git bisect run <cmd> [<arg>...]
git bisect help
This command uses a binary search algorithm to find which commit in
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 65478ef40f5..35938b05fd1 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -26,7 +26,7 @@ static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
#define BUILTIN_GIT_BISECT_START_USAGE \
- N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]" \
+ N_("git bisect start [--term-(new|bad)=<term> --term-(old|good)=<term>]" \
" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--]" \
" [<pathspec>...]")
#define BUILTIN_GIT_BISECT_STATE_USAGE \
@@ -46,7 +46,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
#define BUILTIN_GIT_BISECT_LOG_USAGE \
"git bisect log"
#define BUILTIN_GIT_BISECT_RUN_USAGE \
- N_("git bisect run <cmd>...")
+ N_("git bisect run <cmd> [<arg>...]")
static const char * const git_bisect_usage[] = {
BUILTIN_GIT_BISECT_START_USAGE,
base-commit: ceadf0f3cf51550166a387ec8508bb55e7883057
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 04/11] t: convert tests to not write references via the filesystem
From: Junio C Hamano @ 2023-10-23 19:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <ZTZ8ATohRe7GVu5D@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> OK, the original checks "if a random garbage file, which may not
>> necessarily be a ref, exists at $n_dir, we cannot create a ref at
>> $n_dir/fixes, due to D/F conflict" more directly, but as long as our
>> intention is to enforce the D/F restriction across different ref
>> backends [*], creating a ref at $n_dir and making sure $n_dir/fixes
>> cannot be created is an equivalent check that is better (because it
>> can be applied for other backends).
>>
>> Side note: there is no fundamental need to, though, and there
>> are cases where being able to have the 'seen' branch and
>> 'seen/ps/ref-test-tools' branches at the same time is
>> beneficial---packed-refs and ref-table backends would not
>> have such an inherent limitation, but they can of course be
>> castrated to match what files-backend can(not) do.
>
> I think initially it is beneficial to keep any such restriction and cut
> back new backends to match them, even though it's more work.
Note that the same thing can be said for "Can I have Main and main
branches?". Loose refs on systems with case-sensitive filesystem
are not penalized, though.
In any case, I think we are in agreement.
>> I trust that this will be corrected to use some wrapper around "git
>> symbolic-ref" (or an equivalent for it as a test-tool subcommand) in
>> some future patch, if not in this series?
>
> Yup, this is getting fixed in a subsequent patch. I had two different
> options to structure this series:
> ...
> There were two reasons why I didn't like the first iteration:
Yup. I tend to agree with the choice and criteria you made and used
here.
^ permalink raw reply
* Re: [PATCH 02/11] t: allow skipping expected object ID in `ref-store update-ref`
From: Junio C Hamano @ 2023-10-23 19:06 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <ZTZ7-oMqek8kQqhJ@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> Good.
>>
>> Even better would be to make the old one optional, though.
>
> I was also a bit torn when writing this. We could of course make the
> behaviour conditional on whether `argc` is 4 or 5. But I wasn't quite
> sure how important it is to provide a nice UI for this test helper, and
> we don't have `argc` readily available. It's not hard to count them
> manually, but until now I was under the impression that the test helpers
> only need to be "good enough".
Yup, good enough would probably be good enough in this case, I agree
;-)
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Dragan Simic @ 2023-10-23 19:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Stopak, Oswald Buddenhagen, git
In-Reply-To: <xmqqfs21noxx.fsf@gitster.g>
On 2023-10-23 21:01, Junio C Hamano wrote:
> Jacob Stopak <jacob@initialcommit.io> writes:
>
>> Git-Sim is a visual dry-run tool for Git that creates images
>> simulating
>> what the corresponding Git command will do, without actually making
>> any
>> change to the underlying repo state. Another important aspect is that
>> command syntax mimics Git's exactly - so to simulate any Git command,
>> like:
>
> Ah, OK, now I see where your "--table" is coming from ;-).
> "git-sim" was exactly what I thought about when I saw it, and I did
> not know that "--table" came from the same set of brain cells.
>
> One thing that nobody seems to have raised that disturbs me is that
> even though there may be educational value in having such a
> "feature", having to carry the extra code to implement in Git incurs
> extra cost. I was reasonably happy when I saw that "git-sim" was
> done as a totally separate entity exactly for this reason.
That's exactly why I already wrote that the original author of the table
patches, if those would become accepted, should commit in advance to
maintaining that as the new git feature.
^ permalink raw reply
* Re: [PATCH v3 0/3] rebase refactoring
From: Junio C Hamano @ 2023-10-23 19:02 UTC (permalink / raw)
To: Phillip Wood; +Cc: Oswald Buddenhagen, git
In-Reply-To: <4f8616d1-35f2-418d-9d28-b230ca45090d@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 20/10/2023 23:07, Junio C Hamano wrote:
>> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> broken out of the bigger series, as the aggregation just unnecessarily holds it
>>> up.
>>>
>>> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>>>
>>> Oswald Buddenhagen (3):
>>> rebase: simplify code related to imply_merge()
>>> rebase: handle --strategy via imply_merge() as well
>>> rebase: move parse_opt_keep_empty() down
>>>
>>> builtin/rebase.c | 44 ++++++++++++++------------------------------
>>> 1 file changed, 14 insertions(+), 30 deletions(-)
>> Looking quite straight-forward and I didn't see anythihng
>> potentially controversial.
>
> Yes they look good, thanks Oswald
Thanks, both. The topic has already been merged to 'next'.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Junio C Hamano @ 2023-10-23 19:01 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Dragan Simic, Oswald Buddenhagen, git
In-Reply-To: <ZTatzlzCkPOW3Rn7.jacob@initialcommit.io>
Jacob Stopak <jacob@initialcommit.io> writes:
> Git-Sim is a visual dry-run tool for Git that creates images simulating
> what the corresponding Git command will do, without actually making any
> change to the underlying repo state. Another important aspect is that
> command syntax mimics Git's exactly - so to simulate any Git command, like:
Ah, OK, now I see where your "--table" is coming from ;-).
"git-sim" was exactly what I thought about when I saw it, and I did
not know that "--table" came from the same set of brain cells.
One thing that nobody seems to have raised that disturbs me is that
even though there may be educational value in having such a
"feature", having to carry the extra code to implement in Git incurs
extra cost. I was reasonably happy when I saw that "git-sim" was
done as a totally separate entity exactly for this reason.
THanks.
^ permalink raw reply
* Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
From: Jeff King @ 2023-10-23 18:58 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Taylor Blau, git, Elijah Newren, Eric W. Biederman,
Junio C Hamano
In-Reply-To: <ZTY6kTZT-ni16usH@tanuki>
On Mon, Oct 23, 2023 at 11:19:13AM +0200, Patrick Steinhardt wrote:
> > + case SOURCE_INCORE:
> > + assert(source->read <= source->size);
>
> Is there any guideline around when to use `assert()` vs `BUG()`? I think
> that this assertion here is quite critical, because when it does not
> hold we can end up performing out-of-bounds reads and writes. But as
> asserts are typically missing in non-debug builds, this safeguard would
> not do anything for our end users, right?
I don't think we have a written guideline. My philosophy is: always use
BUG(), because you will never be surprised that the assertion was not
compiled in (and I think compiling without assertions is almost
certainly premature optimization, especially given the way we tend to
use them).
-Peff
^ permalink raw reply
* Re: [PATCH v2] upload-pack: add tracing for fetches
From: Jeff King @ 2023-10-23 18:55 UTC (permalink / raw)
To: Robert Coup via GitGitGadget; +Cc: git, Taylor Blau, Robert Coup
In-Reply-To: <pull.1598.v2.git.1697577168128.gitgitgadget@gmail.com>
On Tue, Oct 17, 2023 at 09:12:47PM +0000, Robert Coup via GitGitGadget wrote:
> Information on how users are accessing hosted repositories can be
> helpful to server operators. For example, being able to broadly
> differentiate between fetches and initial clones; the use of shallow
> repository features; or partial clone filters.
>
> a29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)
> added some information on have counts to fetch-pack itself to help
> diagnose negotiation; but from a git-upload-pack (server) perspective,
> there's no means of accessing such information without using
> GIT_TRACE_PACKET to examine the protocol packets.
>
> Improve this by emitting a Trace2 JSON event from upload-pack with
> summary information on the contents of a fetch request.
>
> * haves, wants, and want-ref counts can help determine (broadly) between
> fetches and clones, and the use of single-branch, etc.
> * shallow clone depth, tip counts, and deepening options.
> * any partial clone filter type.
>
> Signed-off-by: Robert Coup <robert@coup.net.nz>
> ---
> upload-pack: add tracing for fetches
>
>
> Changes since V1
> ================
>
> * Don't generate the JSON event unless Trace2 is active.
> * Code style fix.
Thanks, the first bullet point there addressed my only concern.
The rest looks good to me overall. I think it is a useful feature to
have (as Taylor mentioned, GitHub has something similar via custom
code), but it has been long enough since I have operated a server that I
don't have opinions on what specific items should or should not be
included. :)
-Peff
^ permalink raw reply
* Re: [PATCH v3 5/5] config-parse: split library out of config.[c|h]
From: Jonathan Tan @ 2023-10-23 18:53 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git, calvinwan, glencbz, gitster
In-Reply-To: <e59ca992d0566f793a07d78f3e65219b51239492.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> From: Glen Choo <chooglen@google.com>
>
> The config parsing machinery (besides "include" directives) is usable by
> programs other than Git - it works with any file written in Git config
> syntax (IOW it doesn't rely on 'core' Git features like a repository),
> and as of the series ending at 6e8e7981eb (config: pass source to
> config_parser_event_fn_t, 2023-06-28), it no longer relies on global
> state. Thus, we can and should start turning it into a library other
> programs can use.
Checking this with --color-moved looks good, but we'll need to take
another look once my comments about the earlier patches have been
addressed.
^ permalink raw reply
* Re: [PATCH v3 4/5] config.c: accept config_parse_options in git_config_from_stdin
From: Jonathan Tan @ 2023-10-23 18:52 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git, calvinwan, glencbz, gitster
In-Reply-To: <49d4b649919e5661daa2113c2f5d674c5cd8dd0e.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> diff --git a/config.c b/config.c
> index 0c4f1a2874..50188f469a 100644
> --- a/config.c
> +++ b/config.c
> @@ -2063,12 +2063,11 @@ static int do_config_from_file(config_fn_t fn,
> }
>
> static int git_config_from_stdin(config_fn_t fn, void *data,
> - enum config_scope scope)
> + enum config_scope scope,
> + const struct config_parse_options *config_opts)
> {
> - struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
> -
> return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
> - data, scope, &config_opts);
> + data, scope, config_opts);
> }
>
> int git_config_from_file_with_options(config_fn_t fn, const char *filename,
> @@ -2303,7 +2302,8 @@ int config_with_options(config_fn_t fn, void *data,
> * regular lookup sequence.
> */
> if (config_source && config_source->use_stdin) {
> - ret = git_config_from_stdin(fn, data, config_source->scope);
> + ret = git_config_from_stdin(fn, data, config_source->scope,
> + &opts->parse_options);
> } else if (config_source && config_source->file) {
> ret = git_config_from_file_with_options(fn, config_source->file,
> data, config_source->scope,
Does this change the behavior of stdin config parsing from "die" to
"silent" (since there is no event emitting callback)? The only user of
stdin parsing seems to be builtin/config.c, so maybe a corresponding
change needs to be made there.
^ permalink raw reply
* Re: [PATCH 0/3] some send-email --compose fixes
From: Jeff King @ 2023-10-23 18:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqil71otsa.fsf@gitster.g>
On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:
> > So here's the fix in a cleaned up form, guided by my own comments from
> > earlier. ;) I think this is actually all orthogonal to the patch you are
> > working on, so yours could either go on top or just be applied
> > separately.
> >
> > [1/3]: doc/send-email: mention handling of "reply-to" with --compose
> > [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
> > [3/3]: send-email: handle to/cc/bcc from --compose message
>
> Nice.
>
> With the approach suggested to move the validation down to where the
> necessary addresses are already all defined, Michael observed "whoa,
> why am I getting stringified array ref?". If that is the only issue
> in the approach, queuing these three patches first and then have
> Michael's fix on top of them sounds like the cleanest thing to do.
I don't think it is even an issue in Michael's approach. I'd have to see
his patch and how he tested it to be sure, but I suspect he was simply
being extra careful to test nearby behavior and stumbled upon the
ARRAY() bug. But the bug was there long before either of his patches.
> Will queue on top of v2.42.0 to help those who may want to backport
> these to the maintenance track.
So I think you could take my series on top of master (or 2.42.0), and
eventually target 'master'. The bug it fixes is from 2017, so not
urgent. The reading of "to" headers is a new feature.
But the fix to move the validation around should probably go directly
onto a8022c5f7b (send-email: expose header information to
git-send-email's sendemail-validate hook, 2023-04-19) for use on maint.
I guess maybe it is not that urgent anymore, as that regression is in
v2.41, and we would not release anything along that maint track anymore,
though.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-23 18:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqedhpotmt.fsf@gitster.g>
On Fri, Oct 20, 2023 at 02:45:30PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > - the handling for to/cc/bcc is totally broken.
>
> It is good to see another evidence that "--compose" is probably not
> as often as used as we thought. With enough bugs discovered,
> perhaps someday we can declare "it cannot be that the feature is
> used in the wild, without anybody getting hit by these bugs---let's
> deprecate and eventually remove it" ;-)
I'm not sure if that is evidence or not. The to/cc/bcc feature was just
never implemented. The commit from 2017 made it more broken than saying
"not yet implemented", but that may only be an indication that nobody
wants it or tried to use it.
I dunno. As I noted, the same feature exists when reading the
cover-letter from a set of format-patch files. And of course it is
implemented using totally separate code (in pre_process_file). One
possible cleanup would be to unify those two, but I'm sure there would
be behavior changes. Some of them perhaps good (e.g., it looks like
pre_process_file is more careful about rfc2047 handling) and some of
them I'm not so sure of (e.g., support for --header-cmd in the --compose
letter).
I think an interested person could champion such changes, but I am not
that interested in send-email (I don't use it, and some of its code is
pretty ancient and gross). My goal was to fix the bug I saw with minimal
regression (I waffled even on my patch 2).
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/5] config: split out config_parse_options
From: Taylor Blau @ 2023-10-23 18:46 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Josh Steadmon, git, calvinwan, glencbz, gitster
In-Reply-To: <20231023175217.984090-1-jonathantanmy@google.com>
On Mon, Oct 23, 2023 at 10:52:17AM -0700, Jonathan Tan wrote:
> Josh Steadmon <steadmon@google.com> writes:
> > From: Glen Choo <chooglen@google.com>
> >
> > "struct config_options" is a disjoint set of options used by the config
> > parser (e.g. event listeners) and options used by config_with_options()
> > (e.g. to handle includes, choose which config files to parse).
>
> Can this sentence be reworded? In particular, "disjoint" is a word
> normally applied to two or more sets (meaning that they have no elements
> in common), but here it is used for only one.
The pedant in me agrees with you. I do think that the sentence reads a
little awkwardly. Perhaps instead:
"struct config_options" has members which serve two distinct
purposes. There are a set of members used by the configuration parse
(e.g. event listeners). There is also a set used by
config_with_options() (e.g to handle includes, choose which config
files to parse).
> Everything else looks good, and the reasoning (some functions only use
> a subset of the fields, and this subset is easily explained conceptually
> as those related to parsing) makes sense.
Yup.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 3/5] config: report config parse errors using cb
From: Jonathan Tan @ 2023-10-23 18:41 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git, calvinwan, glencbz, gitster
In-Reply-To: <a888045c04d27864edf5751ea8641fdba596779c.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> From: Glen Choo <chooglen@google.com>
>
> In a subsequent commit, config parsing will become its own library, and
> it's likely that the caller will want flexibility in handling errors
> (instead of being limited to the error handling we have in-tree).
>
> Move the Git-specific error handling into a config_parser_event_fn_t
> that responds to config errors, and make git_parse_source() always
> return -1 (careful inspection shows that it was always returning -1
> already). This makes CONFIG_ERROR_SILENT obsolete since that is
> equivalent to not specifying an error event listener. Also, remove
> CONFIG_ERROR_UNSET and the config_source 'default', since all callers
> are now expected to specify the error handling they want.
I think this has to be better explained. So:
- There is already a config_parser_event_fn_t that can be configured
by a user to receive emitted config events. This callback can return
negative to halt further config parsing.
- Currently, it is git_parse_source() that detects when an error
occurs, and it emits a CONFIG_EVENT_ERROR and either dies, prints
an error, or swallows the error depending on error_action; no
matter what error_action is, it halts config parsing, as one would
expect. This commit moves the die/print/swallow handling to a
config_parser_event_fn_t that will see the CONFIG_EVENT_ERROR and die/
print/swallow.
- This new config_parser_event_fn_t does not need to swallow, since
that's the same as not passing in a callback. So it just needs to die/
print.
> @@ -1039,6 +1042,29 @@ static int do_event(struct config_source *cs, enum config_event_t type,
> return 0;
> }
>
> +static int do_event_and_flush(struct config_source *cs,
> + enum config_event_t type,
> + struct parse_event_data *data)
> +{
> + int maybe_ret;
> +
> + if ((maybe_ret = flush_event(cs, type, data)) < 1)
> + return maybe_ret;
> +
> + start_event(cs, type, data);
> +
> + if ((maybe_ret = flush_event(cs, type, data)) < 1)
> + return maybe_ret;
> +
> + /*
> + * Not actually EOF, but this indicates we don't have a valid event
> + * to flush next time around.
> + */
> + data->previous_type = CONFIG_EVENT_EOF;
> +
> + return 0;
> +}
A lot of this function only makes sense if the type is ERROR, so maybe
rename this as flush_and_emit_error() (and don't take in a type). As
it is, right now there is some confusion about how you can flush (I'm
referring to the second flush) with the same type as what you passed
to start_event().
Also, I don't think we should set data->previous_type here. Instead
there should be a comment saying that if you're emitting ERROR, you
should halt config parsing. The return value here is useless too (it
signals whether we should halt config parsing, but the caller should
always halt, so we don't need to return anything).
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-23 18:40 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <ZTJaVzt75r0iHPzR@ugly>
On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
> On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:
> > But one thing that gives me pause is that the neither before or after
> > this patch do we handle continuation lines like:
> >
> > Subject: this is the beginning
> > and this is more subject
> >
> > And it would probably be a lot easier to add when storing the headers in
> > a hash (it's not impossible to do it the other way, but you basically
> > have to delay processing each line with a small state machine).
> >
> that seems like a rather significant point, doesn't it?
Maybe. It depends on whether anybody is interested in adding
continuation support. Nobody has in the previous 18 years, and nobody
has asked for it.
> > So another option is to just fix the individual bugs separately.
> >
> ... so that seems preferable to me, given that the necessary fixes seem
> rather trivial.
They're not too bad. Probably:
1. lc() the keys we put into the hash
2. match to/cc/bcc and dereference their arrays
3. maybe handle 'body' separately from headers to avoid confusion
But there may be other similar bugs lurking. One I didn't mention: the
hash-based version randomly reorders headers!
> > I guess "readable" is up for debate here, but I find the inline handling
> > a lot easier to follow
> >
> any particular reason for that?
For the reasons I gave in the commit message: namely that the matching
and logic is in one place and doesn't need to be duplicated (e.g., the
special handling of to/cc/bcc, which caused a bug here).
> > (and it's half as many lines; most of the diffstat is the new tests).
>
> > - if ($parsed_email{'From'}) {
> > - $sender = delete($parsed_email{'From'});
> > - }
>
> this verbosity could be cut down somewhat using just
>
> $sender = delete($parsed_email{'From'});
>
> and if the value can be pre-set and needs to be preserved,
>
> $sender = delete($parsed_email{'From'}) // $sender;
>
> but this seems kind of counter-productive legibility-wise.
We do need to avoid overwriting the pre-set value. The "//" one would
work, but we support perl versions old enough that they don't have it.
-Peff
^ permalink raw reply
* Re: [PATCH v2] upload-pack: add tracing for fetches
From: Robert Coup @ 2023-10-23 18:28 UTC (permalink / raw)
To: Taylor Blau, Jeff King; +Cc: git, Robert Coup via GitGitGadget
In-Reply-To: <pull.1598.v2.git.1697577168128.gitgitgadget@gmail.com>
Hi Jeff & Taylor,
Sorry to nag, but would it be possible to give this v2 patch a
look-over? Would be good to get it progressed.
Thanks,
Rob :)
On Tue, 17 Oct 2023 at 22:12, Robert Coup via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Changes since V1
> ================
>
> * Don't generate the JSON event unless Trace2 is active.
> * Code style fix.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Oswald Buddenhagen @ 2023-10-23 18:16 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Dragan Simic, git
In-Reply-To: <ZTatzlzCkPOW3Rn7.jacob@initialcommit.io>
On Mon, Oct 23, 2023 at 10:30:54AM -0700, Jacob Stopak wrote:
>On Mon, Oct 23, 2023 at 04:34:15PM +0200, Dragan Simic wrote:
>> On 2023-10-23 12:52, Oswald Buddenhagen wrote:
>> > i for one think that it would be a perfectly valid experiment to go
>> > all-in and beyond with jacob's proposal - _and make it the default_
>>
>> I'd never support that, FWIW.
>
>FWIW, I'd _never suggest_ that.
>
why, though?
doing that would extend the feature's reach about two orders of
magnitude among newbies, which is where it matters most.
>I very much value Git's current usage and wouldn't dream to make this
>the default.
>
making the default output format somewhat more verbose wouldn't really
"change the usage", though. and being able to permanently get rid of it
with a single command should alleviate any _reasonable_ concerns about
habit disruption.
regards
^ permalink raw reply
* Re: [PATCH v3 2/5] config: split do_event() into start and flush operations
From: Jonathan Tan @ 2023-10-23 18:05 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git, calvinwan, glencbz, gitster
In-Reply-To: <8a1463c223497fca2fd3f11a54db5d7e52d1d08a.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> +static void start_event(struct config_source *cs, enum config_event_t type,
> + struct parse_event_data *data)
> +{
> + data->previous_type = type;
> + data->previous_offset = get_corrected_offset(cs, type);
> +}
It's a pity that get_corrected_offset() has to be called twice (once
here and once below) but I think that's the best we can do given how the
code is laid out (and I can't think of a better code layout either).
> +static int flush_event(struct config_source *cs, enum config_event_t type,
> + struct parse_event_data *data)
One thing confusing here is that the "type" is not what's being flushed,
but used to change details about how we flush. Technically all we need
is is_whitespace_type and is_eof_type, but that's clumsier to code. I
think the best we can do is add some documentation to this function,
maybe 'Flush the event started by a prior start_event(), if one exists.
The type of the event being flushed is not "type" but the type that was
passed to the prior start_event(); "type" here may merely change how the
flush is performed' or something like that.
> +{
> + if (!data->opts || !data->opts->event_fn)
> + return 0;
> +
> + if (type == CONFIG_EVENT_WHITESPACE &&
> + data->previous_type == type)
> + return 0;
>
> if (data->previous_type != CONFIG_EVENT_EOF &&
> data->opts->event_fn(data->previous_type, data->previous_offset,
> - offset, cs, data->opts->event_fn_data) < 0)
> + get_corrected_offset(cs, type), cs,
> + data->opts->event_fn_data) < 0)
> return -1;
Another confusing point here is how EOF is used both to mean
"start_event() was never called" and a true EOF. I think for now it's
best to just document this where we define CONFIG_EVENT_EOF.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Dragan Simic @ 2023-10-23 17:59 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Oswald Buddenhagen, git
In-Reply-To: <ZTatzlzCkPOW3Rn7.jacob@initialcommit.io>
On 2023-10-23 19:30, Jacob Stopak wrote:
> On Mon, Oct 23, 2023 at 04:34:15PM +0200, Dragan Simic wrote:
>> On 2023-10-23 12:52, Oswald Buddenhagen wrote:
>>> i for one think that it would be a perfectly valid experiment to go
>>> all-in and beyond with jacob's proposal - _and make it the default_
>>> (when the output is a tty). more advanced users who feel annoyed
>>> would
>>> be expected to opt out of it via configuration, as they are for the
>>> advice messages. because it's really the same idea, only thought
>>> bigger.
>>
>> I'd never support that, FWIW.
>
> FWIW, I'd _never suggest_ that. I very much value Git's current usage
> and wouldn't dream to make this the default. This proposal is for an
> optional flag to help users who would benefit from it, nothing more,
> nothing less. Speculating on user motives to classify them into 2 broad
> categories in order to prove the feature isn't helpful misses the point
> that there is a (relatively large IMO) subset of users who would
> benefit
> from it.
>
> As an optional flag, experienced users wouldn't bat an eyelash, and the
> type of users who installed my tool could use the flag on and off until
> they feel confident enough to drop it. But it is always there in case
> they need a refresher.
Perhaps my broad classification of git users wasn't that great, and I'm
actually happy for being wrong there.
Just to clarify, in general I'd support the inclusion of the table
output formatting, but I'd need to test it in detail before saying a
definite yes. It would also need to be more feature-complete, and the
original author of the patches should commit to maintaining it as the
new git feature. Having different options available can't hurt, IMHO.
However, having that as the default is something I'll never support.
All this is just my opinion.
^ permalink raw reply
* Re: [PATCH v3 1/5] config: split out config_parse_options
From: Jonathan Tan @ 2023-10-23 17:52 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git, calvinwan, glencbz, gitster
In-Reply-To: <fa55b7836f112cb7c7ab9b80e745b9969421c768.1695330852.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> From: Glen Choo <chooglen@google.com>
>
> "struct config_options" is a disjoint set of options used by the config
> parser (e.g. event listeners) and options used by config_with_options()
> (e.g. to handle includes, choose which config files to parse).
Can this sentence be reworded? In particular, "disjoint" is a word
normally applied to two or more sets (meaning that they have no elements
in common), but here it is used for only one.
Everything else looks good, and the reasoning (some functions only use
a subset of the fields, and this subset is easily explained conceptually
as those related to parsing) makes sense.
^ permalink raw reply
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-23 17:52 UTC (permalink / raw)
To: phillip.wood
Cc: git, Junio C Hamano, Christian Couder, Charvi Mendiratta,
Marc Branchaud, Johannes Sixt
In-Reply-To: <a85c80eb-65ab-4b8c-ba94-de71516da5ef@gmail.com>
On Mon, Oct 23, 2023 at 05:01:02PM +0100, Phillip Wood wrote:
>On 23/10/2023 14:00, Oswald Buddenhagen wrote:
>> +unless "fixup -c" is used. In the latter case, the message is
>> obtained
>> +only from the "fixup -c" commit (having more than one of these is
>> +incorrect).
>
>This change is incorrect - it is perfectly fine to have more than one
>"fixup -c" command. In that case we use the message of the commit of the
>final "fixup -c" command.
>
i know that this is the case, see the previous thread (which i failed to
link by header, cf.
https://lore.kernel.org/all/20231020092707.917514-1-oswald.buddenhagen@gmx.de/T/#u
).
>One case where there can be multiple "fixup -c" commands is when a
>commit has been reworded several times via "git commit
>--fixup=reword:<commit>" and the user runs "git rebase --autosquash"
>
a cleaner solution would be recognizing the situation and not generating
these contradicting commands in the first place. of course that would be
more complexity, but it would also allow catching accidental use.
of course i can go back to documenting the status quo, but it seems kind
of wrong.
>In the case of
>
>pick A
>fixup -C B
>
>don't we keep the authorship from A and just use the commit message from B?
>
uhm. we clearly do. that means i was given incorrect advice in
https://lore.kernel.org/all/YjXRM5HiRizZ035p@ugly/T/#u (and so the
thread is still looking for a resolution) ...
regards
^ permalink raw reply
* Re: [PATCH] doc/git-bisect: clarify `git bisect run` syntax
From: Junio C Hamano @ 2023-10-23 17:46 UTC (permalink / raw)
To: Javier Mora
Cc: Patrick Steinhardt, Eric Sunshine, cousteau via GitGitGadget, git
In-Reply-To: <CAH1-q0hrfROfQROXGoCfde4MFkEjxjSMneDcqLO1pqYpe+bN9g@mail.gmail.com>
Javier Mora <cousteaulecommandant@gmail.com> writes:
>> The output of `git bisect -h` suffers the same problem. Perhaps this
>> patch can fix that, as well?
>
> Certainly possible. Probably best if I put that on a second patch
> though (i.e. a separate commit). Or should I just squash everything
> together?
In this case, a single patch is the way to go; otherwise we will
(tentatively) be in an inconsistent state after applying one until
the other gets applied.
> There are still multiple .po files containing the old string, I guess
> I don't need to touch those?
Correct.
> Speaking of which, looking at the .po files I've found that there's
> also a `git bisect--helper` command; I don't know if that's relevant
> nor how to modify that.
bisect--helper has been retired but most of the messages used by it
should have been in use by bisect proper, so only the "this message
appears here" comments may be wrong.
In any case, touching po/ is not in the scope of this isolated fix.
The i18n group has their own workflows to update the files there,
and those touching the code and docs should not have to touch them
in general.
>> I wonder if we should eventually move these into the
>> proper SYNOPSIS section.
>
> Seems reasonable. I was actually wondering about that.
But not as a part of this isolated fix.
^ permalink raw reply
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Taylor Blau @ 2023-10-23 17:40 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Kristoffer Haugsbakk, git, stolee
In-Reply-To: <ZTZDqToqcsDiS5AP@tanuki>
On Mon, Oct 23, 2023 at 11:58:01AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 18, 2023 at 10:28:40PM +0200, Kristoffer Haugsbakk wrote:
> >
> > Factor out code that retrieves the global config file so that we can use
> > it in `gc.c` as well.
> >
> > Use the old name from the previous commit since this function acts
> > functionally the same as `git_system_config` but for “global”.
> >
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
> > builtin/config.c | 25 ++-----------------------
> > config.c | 24 ++++++++++++++++++++++++
> > config.h | 1 +
> > 3 files changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/builtin/config.c b/builtin/config.c
> > index 6fff2655816..df06b766fad 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> > }
> >
> > if (use_global_config) {
> > - char *user_config, *xdg_config;
> > -
> > - git_global_config_paths(&user_config, &xdg_config);
> > - if (!user_config)
> > - /*
> > - * It is unknown if HOME/.gitconfig exists, so
> > - * we do not know if we should write to XDG
> > - * location; error out even if XDG_CONFIG_HOME
> > - * is set and points at a sane location.
> > - */
> > - die(_("$HOME not set"));
> > -
> > + given_config_source.file = git_global_config();
> > given_config_source.scope = CONFIG_SCOPE_GLOBAL;
> > -
> > - if (access_or_warn(user_config, R_OK, 0) &&
> > - xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> > - given_config_source.file = xdg_config;
> > - free(user_config);
> > - } else {
> > - given_config_source.file = user_config;
> > - free(xdg_config);
> > - }
> > - }
> > - else if (use_system_config) {
> > + } else if (use_system_config) {
> > given_config_source.file = git_system_config();
> > given_config_source.scope = CONFIG_SCOPE_SYSTEM;
> > } else if (use_local_config) {
> > diff --git a/config.c b/config.c
> > index d2cdda96edd..2ff766c56ff 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -2111,6 +2111,30 @@ char *git_system_config(void)
> > return system_config;
> > }
> >
> > +char *git_global_config(void)
> > +{
> > + char *user_config, *xdg_config;
> > +
> > + git_global_config_paths(&user_config, &xdg_config);
> > + if (!user_config)
> > + /*
> > + * It is unknown if HOME/.gitconfig exists, so
> > + * we do not know if we should write to XDG
>
> Nit: we don't know about the intent of the caller, so they may not want
> to write to the file but only read it.
I was going to suggest that we allow the caller to pass in the flags
that they wish for git_global_config() to pass down to access(2), but
was surprised to see that we always use R_OK.
But thinking on it for a moment longer, I realized that we don't care
about write-level permissions for the config, since we want to instead
open $GIT_DIR/config.lock for writing, and then rename() it into place,
meaning we only care about whether or not we have write permissions on
$GIT_DIR itself.
I think in the existing location of this code, the "if we should write"
portion of the comment is premature, since we don't know for sure
whether or not we are writing. So I'd be fine with leaving it as-is, but
changing the comment seems easy enough to do...
> > + * location; error out even if XDG_CONFIG_HOME
> > + * is set and points at a sane location.
> > + */
> > + die(_("$HOME not set"));
>
> Is it sensible to `die()` here in this new function that behaves more
> like a library function? I imagine it would be more sensible to indicate
> the error to the user and let them handle it accordingly.
Agreed.
Thanks,
Taylor
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox