* 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 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 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 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 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 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 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: [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 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: 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 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: [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
* [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: [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
* 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: [PATCH v3 0/5] config-parse: create config parsing library
From: Taylor Blau @ 2023-10-23 19:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Steadmon, git, jonathantanmy, calvinwan, glencbz
In-Reply-To: <xmqq34y9jho2.fsf@gitster.g>
On Tue, Oct 17, 2023 at 10:13:49AM -0700, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > Open questions:
> > - How do folks feel about the do_event() refactor in patches 2 & 3?
>
> I gave a quick re-read and found that the code after patch 2 made it
> easier to see how config.c::do_event() does its thing (even though
> the patch text of that exact step was somehow a bit hard to follow).
>
> However, the helper added by patch 3, do_event_and_flush(), that
> duplicates exactly what do_event() does, is hard to reason about, at
> least for me. It returns early without setting .previous_type to
> EOF and the value returned from the helper signals if that is the
> case (the two early return points both return what flush_event()
> gave us), but the only caller of the helper does not even inspect
> the return value, unlike all the callers of do_event(), which also
> looks a bit fishy.
I had similar thoughts while reviewing.
But I am not sure that I agree that this series is moving us in the
right direction necessarily. Or at least I am not convinced that
shipping the intermediate state is worth doing before we have callers
that could drop '#include "config.h"' for just the parser.
This feels like churn that does not yield a tangible pay-off, at least
in the sense that the refactoring and code movement delivers us
something that we can substantively use today.
I dunno.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2] doc/git-bisect: clarify `git bisect run` syntax
From: Eric Sunshine @ 2023-10-23 19:36 UTC (permalink / raw)
To: cousteau via GitGitGadget; +Cc: git, Patrick Steinhardt, Javier Mora
In-Reply-To: <pull.1602.v2.git.1698088990478.gitgitgadget@gmail.com>
On Mon, Oct 23, 2023 at 3:23 PM cousteau via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> doc/git-bisect: clarify `git bisect run` syntax
>
> 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.
Makes sense to fix this inconsistency, as well, though the patch
subject becomes a bit outdated with this addition.
> Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
> ---
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> @@ -16,7 +16,7 @@ DESCRIPTION
> - 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>...]
Upon first reading, I questioned whether changing <term> to <term-new>
and <term-old> adds value since the option names --term-new and
--term-old already provide enough context for the reader to understand
the generic placeholder <term>. However, then I noticed that the
following two lines are already referencing placeholders <term-new>
and <term-old>, so perhaps this change makes sense. But...
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> @@ -26,7 +26,7 @@ static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
> #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>]" \
...now we have an inconsistency again since this text just uses the
generic <term>. However, I haven't convinced myself that we need to
care about this inconsistency.
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Oswald Buddenhagen @ 2023-10-23 19:50 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <20231023184010.GA1537181@coredump.intra.peff.net>
On Mon, Oct 23, 2023 at 02:40:10PM -0400, Jeff King wrote:
>On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
>> 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.
>
dunno, it seems like a bug to me. so if i cared at all about this
functionality, i'd fix it just because. so at least it doesn't seem nice
to make it harder for a potential volunteer.
>> > 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
>
with the header keys lowercased, one could simply use BODY as the key
and be done with it.
>But there may be other similar bugs lurking.
>One I didn't mention: the
>hash-based version randomly reorders headers!
>
hmm, yeah, that would mean using Tie::IxHash if one wanted to do it
elegantly, at the cost of depending on another non-core module.
also, it means that another hash with non-lowercased versions of the
keys would have to be kept.
ok, that's stupid. it would be easier to just keep an additional array
of the original keys for iteration, and check the hash before emitting
them.
>> > 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).
>
from what i can see, there isn't really anything to "match", apart from
agreeing on the data structure (which the code partially failed to do,
but that's trivial enough). and layering/abstracting things is usually
considered a good thing, unless the cost/benefit ratio is completely
backwards.
>The "//" one would
>work, but we support perl versions old enough that they don't have it.
>
according to my grepping, that ship has sailed.
also, why _would_ you support such ancient perl versions? that makes
even less sense to me than supporting ancient c compilers.
regards
^ permalink raw reply
* Re: [PATCH v3 3/5] config: report config parse errors using cb
From: Junio C Hamano @ 2023-10-23 20:11 UTC (permalink / raw)
To: Taylor Blau; +Cc: Josh Steadmon, git, jonathantanmy, calvinwan, glencbz
In-Reply-To: <ZTbJqzWDyqkhc6L9@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
>> + 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)).
FWIW, I have always felt that the way STRING_LIST_INIT* was done was
quite ugly. The new pattern does look superiour, as long as (1) it
does not involve voodoo like token pasting, and (2) the parameters
passed does not grow. The latter is especially important as there
is no equivalent to designated initializers in C preprocessor macros.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 0/5] config-parse: create config parsing library
From: Junio C Hamano @ 2023-10-23 20:13 UTC (permalink / raw)
To: Taylor Blau; +Cc: Josh Steadmon, git, jonathantanmy, calvinwan, glencbz
In-Reply-To: <ZTbK3QTJYXxYj/M6@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> This feels like churn that does not yield a tangible pay-off, at least
> in the sense that the refactoring and code movement delivers us
> something that we can substantively use today.
>
> I dunno.
That matches something I felt but was too polite to say aloud ;-)
^ permalink raw reply
* Re: [PATCH v3] builtin/branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-23 20:17 UTC (permalink / raw)
To: Isoken June Ibizugbe, git; +Cc: Dragan Simic, Junio C Hamano
In-Reply-To: <20231023160656.4341-1-isokenjune@gmail.com>
On 23-oct-2023 17:06:56, Isoken June Ibizugbe wrote:
Just for reference, to avoid confusion, this is technically the fourth
version.
> As per the CodingGuidelines document, it is recommended that error messages
> such as die(), error() and warning(), should start with a lowercase letter
> and should not end with a period.
>
> This patch adjusts tests to match updated messages.
>
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
It is often convenient to include here, after this three dash line, a
description of the changes introduced in the new iteration.
In this particular case, a range-diff is very helpful.
This is the range-diff with v2:
1: a4e8bb1b4c ! 1: c4ae0c1cce builtin/branch.c: adjust error messages to coding guidelines
@@ Metadata
## Commit message ##
builtin/branch.c: adjust error messages to coding guidelines
- As per the CodingGuidelines document, it is recommended that a single-line
- message provided to error messages such as die(), error() and warning(),
- should start with a lowercase letter and should not end with a period.
- Also this patch fixes the tests broken by the changes.
+ As per the CodingGuidelines document, it is recommended that error messages
+ such as die(), error() and warning(), should start with a lowercase letter
+ and should not end with a period.
+
+ This patch adjusts tests to match updated messages.
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
As we can see, the only difference is in the commit message.
The new wording is better; it avoids the distraction introduced in v2.
And, as Dragan suggested about /the other v3/, the wrapping is correct.
The rest of the patch is equal to the previous iteration (v2), which
already seemed correct to me.
Thank you.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Oswald Buddenhagen @ 2023-10-23 20:19 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Dragan Simic, git
In-Reply-To: <ZTbJiIkIyXwWK8JP.jacob@initialcommit.io>
On Mon, Oct 23, 2023 at 12:29:12PM -0700, Jacob Stopak wrote:
>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.
>
you're too focused on the status quo of your own tool. :-)
there is really nothing that would speak against the real commands
reporting what they just *actually did*. this would seem rather helpful
for noobs and other insecure users.
if one really wanted, "you can also use this with --dry-run" could be
part of the hint that would say how to turn off the extra verbosity (or
just the hint itself, if one likes the verbosity).
one could even go one step further and put at least the destructive
commands into interactive/confirmation mode by default. but that's
probably a bridge too far, as it would be potentially habit-forming in a
bad way.
regards
^ permalink raw reply
* Re: [PATCH v3] builtin/branch.c: adjust error messages to coding guidelines
From: Junio C Hamano @ 2023-10-23 20:19 UTC (permalink / raw)
To: Rubén Justo; +Cc: Isoken June Ibizugbe, git, Dragan Simic
In-Reply-To: <14df596a-d0ef-46aa-97c6-3c9f0da1975f@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> The rest of the patch is equal to the previous iteration (v2), which
> already seemed correct to me.
>
> Thank you.
Thanks, all. Let's declare a victory and mark the topic to be
merged to 'next'.
^ permalink raw reply
* Re: [PATCH v4 00/17] bloom: changed-path Bloom filters v2 (& sundries)
From: SZEDER Gábor @ 2023-10-23 20:22 UTC (permalink / raw)
To: Taylor Blau
Cc: Junio C Hamano, git, Elijah Newren, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <ZTK4ZKESDVghzSH8@nand.local>
On Fri, Oct 20, 2023 at 01:27:00PM -0400, Taylor Blau wrote:
> On Wed, Oct 18, 2023 at 04:26:48PM -0700, Junio C Hamano wrote:
> > Taylor Blau <me@ttaylorr.com> writes:
> >
> > > (Rebased onto the tip of 'master', which is 3a06386e31 (The fifteenth
> > > batch, 2023-10-04), at the time of writing).
> >
> > Judging from 17/17 that has a free_commit_graph() call in
> > close_commit_graph(), that was merged in the eighteenth batch,
> > the above is probably untrue. I'll apply to the current master and
> > see how it goes instead.
>
> Worse than that, I sent this `--in-reply-to` the wrong thread :-<.
>
> Sorry about that, and indeed you are right that the correct base for
> this round should be a9ecda2788 (The eighteenth batch, 2023-10-13).
>
> I'm optimistic that with the amount of careful review that this topic
> has already received, that this round should do the trick.
Unfortunately, I can't share this optimism. This series still lacks
tests exercising the interaction of different versions of Bloom
filters and split commit graphs, and the one such test that I sent a
while ago demonstrates that it's still broken. And it's getting
worse: back then I didn't send the related test that merged
commit-graph layers containing different Bloom filter versions,
because happened to succeed even back then; but, alas, with this
series even that test fails.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Dragan Simic @ 2023-10-23 20:29 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Oswald Buddenhagen, git
In-Reply-To: <ZTbJiIkIyXwWK8JP.jacob@initialcommit.io>
On 2023-10-23 21:29, Jacob Stopak wrote:
> 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.
That's a good idea, having the tables and dry runs mentioned in a
well-placed hint that could, of course, be disabled through git
configuration.
^ 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