* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Patrick Steinhardt @ 2024-03-07 6:31 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git
In-Reply-To: <ZekAWSqr9qb8FIAD@nand.local>
[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]
On Wed, Mar 06, 2024 at 06:46:33PM -0500, Taylor Blau wrote:
> On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > > - `git config --get-urlmatch` -> `git config get-urlmatch`.
> >
> > ... is a Meh to me, personally. I'd not actively push it
> > enthusiastically, but I'd passively accept its existence.
>
> I don't have strong feelings about this, but I wonder if `--urlmatch`
> (or `--url-match`) might be an argument to the "get" mode of this
> sub-command instead. Something like `git config get --urlmatch` feels
> much more natural to me than `git config get-urlmatch`.
Agreed. I allude to this somewhere in the patch series that I only see
this as a first incremental step, and that some of the subcommands
really should be converted to become options eventually. Specifically:
- `git config get-urlmatch` -> `git config get --urlmatch`
- `git config get-regexp` -> `git config get --regexp`
- `git config get-all` -> `git config get --all`
- `git config set-all` -> `git config set --all`
- `git config unset-all` -> `git config unset --all`
I didn't yet do it as part of this patch series because I didn't want to
make functional changes at the same time (well, except of course for the
introduction of subcommands). But if we all agree that this patch series
is something we want _and_ that we don't want to have an intermediate
step with the above somewhat weird subcommands then I would be happy to
pile onto the series.
I wouldn't think that keeping this intermediate step would be too bad
though. While we would eventually omit the above subcommands, the infra
to keep them around needs to stay anyway to support old syntax like
`--get-all`. Thus, we can keep the subcommands themselves almost for
free even if we eventually decide to hide them and replace them with
flags.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles
From: Patrick Steinhardt @ 2024-03-07 6:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqedcntj06.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]
On Wed, Mar 06, 2024 at 08:36:25AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> Since the tempfile is now being created through the tempfile API, I
> >> think the file mode can be set directly through `mks_tempfile_m()`
> >> instead of creating the tempfile and then using chmod. Just something I
> >> thought to mention.
> >
> > Unfortunately not. The problem is that `mks_tempfile_m()` will munge
> > passed-in permissions via "core.sharedRepository", but we already pre
> > calculated the target mode in `config.default_permissions`. Thus, the
> > result would have wrong permissions if we used `mks_tempfile_m()`.
>
> I somehow found that default_permissions thing always disturbing.
>
> Even if we keep a separate mechanism for determining the file
> permission (perhaps in order to give ourselves a better separation
> as "an independent library" from the rest of Git), shouldn't the
> permission setting that is computed by the mechanism and stored in
> config.default_permissions be consistent with the permission the
> rest of git computes based on core.sharedRepository?
It is consistent. The problem is rather that `mks_tempfile_m()` takes a
mode as input, but still ends up applying the umask to that mode. Thus,
using that function without a subsequent call to chmod(3P) would end up
mishandling "core.sharedRepository".
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter
From: Patrick Steinhardt @ 2024-03-07 6:00 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZRQ6sYLBqjWiMXGS7rcv8pJ7jSEneiJ8ZcHtALrrcwMeg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 407 bytes --]
On Wed, Mar 06, 2024 at 08:13:39AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > When creating a new reflog iterator, we first have to reload the stack
> > that the iterator is being created. This is done so that any concurrent
>
> Nit: s/created./created for.
Indeed, thanks. I've fixed this locally and will wait for more comments
before sending a v2.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Elijah Newren @ 2024-03-07 5:36 UTC (permalink / raw)
To: Stefan Haller
Cc: Junio C Hamano, git, Derrick Stolee, Phillip Wood,
Christian Couder
In-Reply-To: <845ced9a-1f35-4100-a1ff-4243db2ba34f@haller-berlin.de>
On Wed, Mar 6, 2024 at 1:00 PM Stefan Haller <lists@haller-berlin.de> wrote:
>
> On 06.03.24 03:57, Elijah Newren wrote:
>
> > 1) What if there is a branch that is "just a copy" of one of the
> > branches earlier in the "stack"? Since it's "just a copy", shouldn't
> > it be excluded for similar reasons to what you are arguing? And, if
> > so, which branch is the copy?
>
> This is a good point, but in my experience it's a lot more rare. Maybe
> I'm looking at all this just from my own experience, and there might be
> other usecases that are very different from mine, but as far as I am
> concerned, copies of branches are not long-lived.
> There is no point in having two branches point at the same commit.
But isn't that what you're doing?
> When I create a copy of a
> branch, I do that only to rebase the copy somewhere else _immediately_,
> leaving the original branch where it was.
If it is inherently tied like this, why not create the new branch
immediately after the rebase (with active_branch@{1} as the start
point), instead of creating it immediately before?
> Which means that I encounter
> copied branches only at the top of the stack, not in the middle. Which
> means that I'm fine with keeping the current behavior of "rebase
> --update-ref" to update both copies of that middle-of-the-stack branch,
> because it never happens in practice for me.
You've really lost me here; are you saying you're fine changing the
design to add inherent edgecase bugs to the code because those edge
cases "never happen in practice for me"? I've spent a lot of time
dealing with built up cruft in git from partial solutions and fixes
that overlooked subsets of relevant testcases, so I'm not a fan of
that statement and in particular the last two words of it. Perhaps
I'm reading it wrong, and if so I apologize, but it triggered unhappy
memories of mine from merge-recursive.c and dir.c and elsewhere.
> > 2) Further, a "stack", to me at least, suggests a linear history
> > without branching (i.e. each commit has at most one parent _and_ at
> > most one child among the commits in the stack). I designed `git
> > replay` to handle diverging histories (i.e. rebasing multiple branches
> > that _might_ share a subset of common history but none necessarily
> > need to fully contain the others, though perhaps the branches do share
> > some other contained branches), and I want it to handle replaying
> > merges as well. While `git rebase --update-refs` is absolutely
> > limited to "stacks", and thus your argument might make sense in the
> > context of `git rebase`, since you are bringing `git replay` into the
> > mix, it needs to apply beyond a stack of commits. It's not clear to
> > me how to genericize your suggestions to handle cases other than a
> > simple stack of commits, though.
>
> I don't see a contradiction here. I don't tend to do this in practice,
> but I can totally imagine a tree of stacked branches that share some
> common base branches in the beginning and then diverge into different
> branches from there. It's true that "rebase --update-refs", when told to
> rebase one of the leaf branches, will destroy this tree because it pulls
> the base branches away from under the other leaf branches, but this is
> unrelated to my proposal, it has this problem today already. And it's
> awesome that git replay has a way to avoid this by rebasing the whole
> tree at once, keeping everything intact. Still, I don't see what's bad
> about excluding branches that point at the same commits as the leaf
> branches it is told to rebase when using "replay --contains".
By "leaf branches", do you mean (a) those commits explicitly mentioned
on the command line for being replayed, (b) only the subset of the
branches mentioned on the command line which aren't an ancestor of
another commit being replayed, or (c) something else?
> (I suppose
> what I'm suggesting is to treat "--contains" to mean "is included in the
> half-open interval from base to tip" of the revision range you are
> rebasing, rather than the closed interval.)
"half-open interval"? That to me again implies a simple stack, which
since we're trying to address the more general case, makes me more
confused rather than less.
Let me re-ask my question another way. If someone runs
git replay --onto A --contained ^B ^C D E F
when branches G, H, & I are in the revision range of "^B ^C D E F",
with G in particular pointing where D does and H pointing where E
does, and E contains D in its history, and F contains commits that are
in neither D nor E, how do I figure out which of D-I should be
updated?
> Maybe I should make this more explicit again: I'm not trying to solve
> the problem of making a copy of a stack of branches, and rebasing that
> copy somewhere else. I think this can't be solved except by making
> branch stacks a new concept in git, which I'm not sure we want to do.
Oh, I hadn't even thought of that. Yeah, that'd be even more complex.
> > 3) This is mostly covered in (1) and (2), but to be explicit: `git
> > replay` is completely against the HEAD-is-special assumptions that are
> > pervasive within `git rebase`, and your problem is entirely phrased as
> > HEAD-is-special due to your call out of "the current branch". Is your
> > argument limited to such special cases? (If so, it might still be
> > valid for `git rebase`, of course.)
>
> No, I don't think I need HEAD to be special. "The thing that I'm
> rebasing" is special, and it is always HEAD for git rebase, but it can
> be something else for replay.
But what exactly should that something else be? I still don't
understand what that is from your explanation so far.
> > 4a) `git replay` does what Junio suggests naturally, since it doesn't
> > update the refs but instead gives commands which can be fed to `git
> > update-ref --stdin`. Thus, users can inspect the output of `git
> > replay` and only perform the updates they want (by feeding a subset of
> > the lines to update-ref --stdin).
>
> At this point I probably need to explain that I'm rarely using the
> command line. I'm a user and co-maintainer of lazygit, and I want to
> make lazygit work in such a way that "it does the right thing" in as
> many cases as possible.
...and I'm pointing out that `git replay` has the necessary tools to
enable you to do so. Unlike `git rebase --update-refs` it doesn't
automatically update the branches, but just creates the new commits
and tells you what it could update each branch to, in a format that
you can pass along to another tool to actually do the updates of the
branches. As such, you can write your tool to take that output, pick
out the bits you like, and only pass those bits along so that only
some of the branches are updated.
> > 4b) For `git replay`, --contained is just syntactic sugar -- it isn't
> > necessary. git replay will allow you to list multiple branches that
> > you want replayed, so you can specify which branches are relevant to
> > you.
>
> That's great, even if it means that I have to redo some of the work that
> --contains would already do for me, just because I want a slightly
> different behavior.
Right, but I thought you were maintaining lazygit, meaning that
programming it to select the branches you want is a one time cost?
Something like `git log --format=%D --decorate-refs=refs/heads/
${base}..HEAD^1 | grep -v ^$`, plus adding in the current branch,
right?
Or is the concern with this suggestion the performance hit you'd take
(which admittedly might be a problem with this solution, since you
walk the commits an extra time)?
> > 4c) For `git rebase --update-refs`, you can add `--interactive` and
> > then delete the `update-ref` line(s) corresponding to the refs you
> > don't want updated.
>
> Yes, that's what I always do today to work around the problem. It's just
> easy to forget, and I find it annoying that I have to take this extra
> step every time.
And if you forget, then after the rebase it's trivial to move the
updated branch back to where you want it, right?
git branch -f ${copy_branch_name} ${current_branch_name}@{1}
In fact, that's probably easier than making the rebase interactive,
and should be easier to remember since you only ever create these
branches precisely when you want to do a rebase.
> One last remark: whenever I describe my use case involving copies of
> branches, people tell me not to do that, use detached heads instead, or
> other ways to achieve what I want. But then I don't understand why my
> proposal would make a difference for them. If you don't use copied
> branches, then why do you care whether "rebase --update-refs" or "replay
> --contained" moves those copies or not? I still haven't heard a good
> argument for why the current behavior is desirable, except for the one
> example of a degenerate stack that Phillip Wood described in [1].
The current behavior is easy to describe and explain to users, and
generalizes nicely to cases of replaying multiple diverging and
converging branches.
To me, the behavior you're proposing doesn't seem to share either of
those qualities, at least not as you've explained it so far.
But, perhaps that's because I still don't really understand your
usecase. I'm trying to, and it's possible I could be convinced there
is a proposal here that is easy to explain to users and generalizes
nicely. My way of attempting to get that out is to make
counter-proposals and ask questions as a way of teasing out what your
usecase is and what a refined proposal might be. Currently, it seems
there are two trivial alternative solutions that would solve this
problem more cleanly (namely, either creating the branch after the
fact instead of beforehand, or simply updating the branch after the
fact)...but maybe I'm still just missing something?
^ permalink raw reply
* Re: [PATCH v2] userdiff: better method/property matching for C#
From: Junio C Hamano @ 2024-03-07 2:11 UTC (permalink / raw)
To: Steven Jeuris via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Jeff King,
Steven Jeuris, Steven Jeuris
In-Reply-To: <pull.1682.v2.git.git.1709756493673.gitgitgadget@gmail.com>
"Steven Jeuris via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Steven Jeuris <steven.jeuris@3shape.com>
>
> - Support multi-line methods by not requiring closing parenthesis.
> - Support multiple generics (comma was missing before).
> - Add missing `foreach`, `lock` and `fixed` keywords to skip over.
> - Remove `instanceof` keyword, which isn't C#.
> - Also detect non-method keywords not positioned at the start of a line.
> - Added tests; none existed before.
>
> The overall strategy is to focus more on what isn't expected for
> method/property definitions, instead of what is, but is fully optional.
>
Roughly in other words, we assume that any file the end user throws
at us is a well formed program, so instead of enumerating all valid
keywords and limit the match exactly to them, use a pattern that
would match valid keywords (both currently known ones, and anything
the language might add in the future that we do not know about), to
match with anything syntactically plausible to be a definition?
It does make sense to start by assuming that the end user data is a
valid C# program.
> Signed-off-by: Steven Jeuris <steven.jeuris@gmail.com>
> ---
> diff --git a/userdiff.c b/userdiff.c
> index e399543823b..5a9e8a0ef55 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -89,12 +89,18 @@ PATTERNS("cpp",
> "|\\.[0-9][0-9]*([Ee][-+]?[0-9]+)?[fFlL]?"
> "|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->\\*?|\\.\\*|<=>"),
> PATTERNS("csharp",
> - /* Keywords */
> - "!^[ \t]*(do|while|for|if|else|instanceof|new|return|switch|case|throw|catch|using)\n"
> - /* Methods and constructors */
> - "^[ \t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe|async)[ \t]+)*[][<>@.~_[:alnum:]]+[ \t]+[<>@._[:alnum:]]+[ \t]*\\(.*\\))[ \t]*$\n"
> + /*
> + * Jump over keywords not used by methods which can be followed by parentheses without special characters in between,
> + * making them look like methods.
> + */
Overly long comments (I'll wrap them while queuing).
> + "!(^|[ \t]+)(do|while|for|foreach|if|else|new|default|return|switch|case|throw|catch|using|lock|fixed)([ \t(]+|$)\n"
> + /* Methods/constructors:
> + * the strategy is to identify a minimum of two groups (any combination of keywords/type/name),
> + * without intermediate or final characters which can't be part of method definitions before the opening parenthesis.
> + */
> + "^[ \t]*(([][[:alnum:]@_<>.,]*[^=:{ \t][ \t]+[][[:alnum:]@_<>.,]*)+\\([^;]*)$\n"
> /* Properties */
> - "^[ \t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe)[ \t]+)*[][<>@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+)[ \t]*$\n"
> + "^[ \t]*((([][[:alnum:]@_<>.,]+)[ \t]+[][[:alnum:]@_]*)+[^=:;,()]*)$\n"
> /* Type definitions */
> "^[ \t]*(((static|public|internal|private|protected|new|unsafe|sealed|abstract|partial)[ \t]+)*(class|enum|interface|struct|record)[ \t]+.*)$\n"
> /* Namespace */
>
> base-commit: f41f85c9ec8d4d46de0fd5fded88db94d3ec8c11
^ permalink raw reply
* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Dragan Simic @ 2024-03-07 0:31 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, Patrick Steinhardt, git
In-Reply-To: <ZekAWSqr9qb8FIAD@nand.local>
Hello all,
On 2024-03-07 00:46, Taylor Blau wrote:
> On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > - `git config foo.bar` -> `git config get foo.bar`
>> >
>> > - `git config foo.bar value` -> `git config set foo.bar value`
>>
>> I actually have been perfectly OK with the above two, but I agree
>> that ...
>
> Same here, though I think that we are probably both biased by many
> years
> of familiarity with the existing syntax.
Ditto. Though, having "get" and "set" commands will be nice, making it
all more self descriptive.
>> > - `git config foo.bar value value-pattern` -> `git config set-all
>> > foo.bar value value-pattern`
>>
>> ... this was less than discoverable, and would be a good update.
>> This one ...
>
> Agreed.
Also agreed. Having the point below in mind, perhaps we could actually
end up with "set --all" instead of "set-all".
>> > - `git config --get-urlmatch` -> `git config get-urlmatch`.
>>
>> ... is a Meh to me, personally. I'd not actively push it
>> enthusiastically, but I'd passively accept its existence.
>
> I don't have strong feelings about this, but I wonder if `--urlmatch`
> (or `--url-match`) might be an argument to the "get" mode of this
> sub-command instead. Something like `git config get --urlmatch` feels
> much more natural to me than `git config get-urlmatch`.
Good point. I'd vote for having "get --urlmatch" or "get --url-match",
because it feels more natural to me, it doesn't "clog up" the command
space, and such an approach, in general, allows git-config(1) to be
expanded later easier with more new arguments for the existing commands.
^ permalink raw reply
* RE: Bug in Git
From: rsbecker @ 2024-03-07 0:22 UTC (permalink / raw)
To: 'Victor Porton', git
In-Reply-To: <0d741b90-8307-40cf-b0b3-163203651a57@gmail.com>
On Wednesday, March 6, 2024 6:52 PM, Victor Porton wrote:
>Recently updated Ubuntu 23.10:
>
>I tried it several times - the same result.
>
>$ git clone https://github.com/dfinity/motoko Cloning into 'motoko'...
>remote: Enumerating objects: 104759, done.
>remote: Counting objects: 100% (5702/5702), done.
>remote: Compressing objects: 100% (2333/2333), done.
>error: RPC failed; curl 92 HTTP/2 stream 5 was not closed cleanly:
>CANCEL (err 8)
>error: 1600 bytes of body are still expected
>fetch-pack: unexpected disconnect while reading sideband packet
>fatal: early EOF
>fatal: fetch-pack: invalid index-pack output
What version of git? Do you have a firewall or proxy between your machine and github? This works correctly on 2.42.1 with no proxy.
--Randall
^ permalink raw reply
* [Feature request] git config - includeif - resolve symlinks using realpath
From: Lamy Geier @ 2024-03-07 0:20 UTC (permalink / raw)
To: git
Hello
It will be useful if the path in includeIf "gitdir" uses the resolved path.
For example, in the following snippet, ~/Documents/Git/Wiki is symlinked
to several different locations. When using git from other locations, the
following won't get included. I verified this using `git config --list
--show-origin --show-scope`
```
[includeIf "gitdir:~/Documents/Git/Wiki/"]
path = ~/Documents/Git/Wiki/.gitconfig
```
Feature request: Please resolve the path, before using it.
--
Thanks and Regards
Lamy
^ permalink raw reply
* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Taylor Blau @ 2024-03-07 0:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
In-Reply-To: <xmqq1q8mq5oo.fsf@gitster.g>
On Wed, Mar 06, 2024 at 03:52:23PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > I don't have strong feelings about this, but I wonder if `--urlmatch`
> > (or `--url-match`) might be an argument to the "get" mode of this
> > sub-command instead. Something like `git config get --urlmatch` feels
> > much more natural to me than `git config get-urlmatch`.
>
> I like that, too.
>
> "--get-regexp" may also be a good candidate to be folded into the
> base verb "get", with "--regexp" option to tweak what kind of key is
> used.
Yes, definitely.
> Could "--get-color" and "--get-colorbool" become verb "get" with
> "--type=color"? The other parameters they get are somewhat
> different from "get", so that may not work very well, I guess.
I'm somewhat embarrassed to not have a better suggestion, since I
vaguely remember working on the `--type` option as one of my first
contributions to the project.
Indeed: fb0dc3bac13 (builtin/config.c: support `--type=<type>` as
preferred alias for `--<type>`, 2018-04-18). ;-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 5/8] builtin/config: track subcommands by action
From: Taylor Blau @ 2024-03-07 0:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <e39774d6495767c6505999bdc33110678139e347.1709724089.git.ps@pks.im>
On Wed, Mar 06, 2024 at 12:31:50PM +0100, Patrick Steinhardt wrote:
> ---
> builtin/config.c | 207 +++++++++++++++++++++++------------------------
> 1 file changed, 99 insertions(+), 108 deletions(-)
This is definitely easier (for me, at least) to view with:
$ git show --color-moved --color-moved-ws=ignore-all-space
which makes clearer that this change does not introduce or change any
existing behavior.
> static struct option builtin_config_options[] = {
> OPT_GROUP(N_("Config file location")),
> OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
> @@ -851,20 +868,20 @@ static struct option builtin_config_options[] = {
> OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")),
> OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")),
> OPT_GROUP(N_("Action")),
> - OPT_CMDMODE(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET),
> - OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL),
> - OPT_CMDMODE(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP),
> [...]
Got it, this whole hunk is changing "&actions" to "&action", and nothing
else.
> @@ -976,33 +993,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> key_delim = '\n';
> }
>
> - if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
> - error(_("--get-color and variable type are incoherent"));
> - usage_builtin_config();
> - }
> -
> - if (actions == 0)
> + if (action == ACTION_NONE) {
> switch (argc) {
> - case 1: actions = ACTION_GET; break;
> - case 2: actions = ACTION_SET; break;
> - case 3: actions = ACTION_SET_ALL; break;
> + case 1: action = ACTION_GET; break;
> + case 2: action = ACTION_SET; break;
> + case 3: action = ACTION_SET_ALL; break;
Wondering aloud a little bit... is it safe to set us to "ACTION_SET",
for example, if we have exactly two arguments? On the one hand, it seems
like the answer is yes. But on the other hand, if we were invoked as:
$ git config ste foo
We would get something like:
$ git config ste foo
error: key does not contain a section: ste
which is sensible. It would be nice to say something more along the
lines of "oops, you probably meant 'set' instead of 'ste'". I think you
could claim that the error on the user's part is one of:
- (using the new style 'git config') misspelling "set" as "ste", and
thus trying to invoke a non-existent sub-command
- (using the old style) trying to set the key "ste", which does not
contain a section name, and thus is not a valid configuration key
I have no idea which is more likely, and I think that there isn't a good
way to distinguish between the two at all. So I think the error message
is OK as-is, but let me know if you have other thoughts.
> default:
> usage_builtin_config();
> }
> + }
> + if (action <= ACTION_NONE || action >= ARRAY_SIZE(subcommands_by_action))
> + BUG("invalid action %d", action);
> + subcommand = subcommands_by_action[action];
> +
> + if (type && (subcommand == cmd_config_get_color ||
> + subcommand == cmd_config_get_colorbool)) {
I don't think there's anything wrong with using the function-pointer
equality here to figure out which subcommand we're in, but I wonder if
it might be cleaner to write this as:
if (type && (action == ACTION_GET_COLOR ||
action == ACTION_GET_COLORBOOL)) {
...
> - if (actions == ACTION_LIST) {
> - return cmd_config_list(argc, argv, prefix);
> - } else if (actions == ACTION_EDIT) {
> - return cmd_config_edit(argc, argv, prefix);
> - } else if (actions == ACTION_SET) {
> - return cmd_config_set(argc, argv, prefix);
> - } else if (actions == ACTION_SET_ALL) {
> - return cmd_config_set_all(argc, argv, prefix);
> - } else if (actions == ACTION_ADD) {
> - return cmd_config_add(argc, argv, prefix);
> - } else if (actions == ACTION_REPLACE_ALL) {
> - return cmd_config_replace_all(argc, argv, prefix);
> - } else if (actions == ACTION_GET) {
> - return cmd_config_get(argc, argv, prefix);
> - } else if (actions == ACTION_GET_ALL) {
> - return cmd_config_get_all(argc, argv, prefix);
> - } else if (actions == ACTION_GET_REGEXP) {
> - return cmd_config_get_regexp(argc, argv, prefix);
> - } else if (actions == ACTION_GET_URLMATCH) {
> - return cmd_config_get_urlmatch(argc, argv, prefix);
> - } else if (actions == ACTION_UNSET) {
> - return cmd_config_unset(argc, argv, prefix);
> - } else if (actions == ACTION_UNSET_ALL) {
> - return cmd_config_unset_all(argc, argv, prefix);
> - } else if (actions == ACTION_RENAME_SECTION) {
> - return cmd_config_rename_section(argc, argv, prefix);
> - } else if (actions == ACTION_REMOVE_SECTION) {
> - return cmd_config_remove_section(argc, argv, prefix);
> - } else if (actions == ACTION_GET_COLOR) {
> - return cmd_config_get_color(argc, argv, prefix);
> - } else if (actions == ACTION_GET_COLORBOOL) {
> - return cmd_config_get_colorbool(argc, argv, prefix);
> - }
> -
> - BUG("invalid action");
> + return subcommand(argc, argv, prefix);
Very nice.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 3/8] builtin/config: use `OPT_CMDMODE()` to specify modes
From: Taylor Blau @ 2024-03-06 23:52 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <41e5bf1d6aa35a32f961b7f9d82a70781674eed0.1709724089.git.ps@pks.im>
On Wed, Mar 06, 2024 at 12:31:42PM +0100, Patrick Steinhardt wrote:
> The git-config(1) command has various different modes which are
> accessible via e.g. `--get-urlmatch` or `--unset-all`. These modes are
> declared with `OPT_BIT()`, which causes two minor issues:
>
> - The respective modes also have a negated form `--no-get-urlmatch`,
> which is unintended.
>
> - We have to manually handle exclusiveness of the modes.
>
> Switch these options to instead use `OPT_CMDMODE()`, which is made
> exactly for this usecase. Remove the now-unneeded check that only a
> single mode is given, which is now handled by the parse-options
> interface.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> + OPT_CMDMODE(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET),
> + OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL),
> + OPT_CMDMODE(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP),
> + OPT_CMDMODE(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
Expanding a little on my reply to Junio later on in the thread, I
suspect that these would translate into "get", "get --all", "get
--regexp", and "get --urlmatch", respectively.
> + OPT_CMDMODE(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL),
> + OPT_CMDMODE(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
> + OPT_CMDMODE(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET),
> + OPT_CMDMODE(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL),
Same with this one turning into "unset --all".
> + OPT_CMDMODE(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
> + OPT_CMDMODE(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
> + OPT_CMDMODE('l', "list", &actions, N_("list all"), ACTION_LIST),
> + OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
> + OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
> + OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
These two could probably join the other "get-xyz" modes, and similarly
become "get --color", and "get --colorbool", respectively. It looks like
that's where they lived originally... perhaps I'm missing something as
to why they were moved.
> OPT_GROUP(N_("Type")),
> OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type),
> OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
> @@ -767,10 +767,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> usage_builtin_config();
> }
>
> - if (HAS_MULTI_BITS(actions)) {
> - error(_("only one action at a time"));
> - usage_builtin_config();
> - }
If you apply just this hunk, I would have expected it to break t1300,
but it doesn't appear to. In fact, it looks like the test suite doesn't
seem to mind, either, which indicates a gap in our test coverage...
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 31c3878687..2d1bc1e27e 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2612,4 +2612,17 @@ test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such
> grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
> '
>
> +test_expect_success 'negated mode causes failure' '
> + test_must_fail git config --no-get 2>err &&
> + grep "unknown option \`no-get${SQ}" err
> +'
> +
> +test_expect_success 'specifying multiple modes causes failure' '
> + cat >expect <<-EOF &&
> + error: options ${SQ}--get-all${SQ} and ${SQ}--get${SQ} cannot be used together
> + EOF
> + test_must_fail git config --get --get-all 2>err &&
> + test_cmp expect err
> +'
But this gap is rectified by this hunk here. Thank you!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Junio C Hamano @ 2024-03-06 23:52 UTC (permalink / raw)
To: Taylor Blau; +Cc: Patrick Steinhardt, git
In-Reply-To: <ZekAWSqr9qb8FIAD@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> I don't have strong feelings about this, but I wonder if `--urlmatch`
> (or `--url-match`) might be an argument to the "get" mode of this
> sub-command instead. Something like `git config get --urlmatch` feels
> much more natural to me than `git config get-urlmatch`.
I like that, too.
"--get-regexp" may also be a good candidate to be folded into the
base verb "get", with "--regexp" option to tweak what kind of key is
used.
Could "--get-color" and "--get-colorbool" become verb "get" with
"--type=color"? The other parameters they get are somewhat
different from "get", so that may not work very well, I guess.
^ permalink raw reply
* Bug in Git
From: Victor Porton @ 2024-03-06 23:51 UTC (permalink / raw)
To: git
Recently updated Ubuntu 23.10:
I tried it several times - the same result.
$ git clone https://github.com/dfinity/motoko
Cloning into 'motoko'...
remote: Enumerating objects: 104759, done.
remote: Counting objects: 100% (5702/5702), done.
remote: Compressing objects: 100% (2333/2333), done.
error: RPC failed; curl 92 HTTP/2 stream 5 was not closed cleanly:
CANCEL (err 8)
error: 1600 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output
^ permalink raw reply
* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Taylor Blau @ 2024-03-06 23:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
In-Reply-To: <xmqqil1zs327.fsf@gitster.g>
On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > - `git config foo.bar` -> `git config get foo.bar`
> >
> > - `git config foo.bar value` -> `git config set foo.bar value`
>
> I actually have been perfectly OK with the above two, but I agree
> that ...
Same here, though I think that we are probably both biased by many years
of familiarity with the existing syntax.
> > - `git config foo.bar value value-pattern` -> `git config set-all
> > foo.bar value value-pattern`
>
> ... this was less than discoverable, and would be a good update.
> This one ...
Agreed.
> > - `git config --get-urlmatch` -> `git config get-urlmatch`.
>
> ... is a Meh to me, personally. I'd not actively push it
> enthusiastically, but I'd passively accept its existence.
I don't have strong feelings about this, but I wonder if `--urlmatch`
(or `--url-match`) might be an argument to the "get" mode of this
sub-command instead. Something like `git config get --urlmatch` feels
much more natural to me than `git config get-urlmatch`.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
From: Junio C Hamano @ 2024-03-06 23:00 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Josh Steadmon, Jeff Hostetler via GitGitGadget, git,
Jeff Hostetler
In-Reply-To: <f1c1847f-49fa-5573-55f4-7cca401df401@jeffhostetler.com>
Jeff Hostetler <git@jeffhostetler.com> writes:
>>> Reviewed-by: Josh Steadmon <steadmon@google.com>
>> Let's see what JeffH says about this. I agree with you that making
>> some stuff redundant in [Patch 3/4] and fixing the redundancy in
>> this step does feel somewhat roundabout way of doing this.
>> Thanks.
>>
>
> Sure we can merge them. That's fine. I can send a V4 or if you want
> to just squash them together that's fine.
Let's have a v4 describing the change for combined 3&4 in your
words, with Josh's Reviewed-by: already added to the trailers.
Thanks, both of you.
^ permalink raw reply
* Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
From: Jeff Hostetler @ 2024-03-06 22:54 UTC (permalink / raw)
To: Junio C Hamano, Josh Steadmon
Cc: Jeff Hostetler via GitGitGadget, git, Jeff Hostetler
In-Reply-To: <xmqqwmqfowfo.fsf@gitster.g>
On 3/6/24 4:57 PM, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
>> On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:
>>> From: Jeff Hostetler <jeffhostetler@github.com>
>>>
>>> Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()"
>>> and "trace2_cmd_list_env_vars()", we don't need to explicitly call them.
>>>
>>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
>>> ---
>>> git.c | 6 ------
>>> 1 file changed, 6 deletions(-)
>>>
>>> diff --git a/git.c b/git.c
>>> index 7068a184b0a..a769d72ab8f 100644
>>> --- a/git.c
>>> +++ b/git.c
>>> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
>>> strvec_pushv(&child.args, (*argv) + 1);
>>>
>>> trace2_cmd_alias(alias_command, child.args.v);
>>> - trace2_cmd_list_config();
>>> - trace2_cmd_list_env_vars();
>>> trace2_cmd_name("_run_shell_alias_");
>>>
>>> ret = run_command(&child);
>>> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
>>> COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
>>>
>>> trace2_cmd_alias(alias_command, new_argv);
>>> - trace2_cmd_list_config();
>>> - trace2_cmd_list_env_vars();
>>>
>>> *argv = new_argv;
>>> *argcp += count - 1;
>>> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>>>
>>> trace_argv_printf(argv, "trace: built-in: git");
>>> trace2_cmd_name(p->cmd);
>>> - trace2_cmd_list_config();
>>> - trace2_cmd_list_env_vars();
>>>
>>> validate_cache_entries(the_repository->index);
>>> status = p->fn(argc, argv, prefix);
>>> --
>>> gitgitgadget
>>>
>>
>> I'd personally prefer to see this squashed into Patch 3, but I don't
>> feel too strongly about it. Either way, the series LGTM.
>>
>> Reviewed-by: Josh Steadmon <steadmon@google.com>
>
> Let's see what JeffH says about this. I agree with you that making
> some stuff redundant in [Patch 3/4] and fixing the redundancy in
> this step does feel somewhat roundabout way of doing this.
>
> Thanks.
>
Sure we can merge them. That's fine. I can send a V4 or if you want
to just squash them together that's fine.
Jeff
^ permalink raw reply
* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Kristoffer Haugsbakk @ 2024-03-06 22:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <cover.1709724089.git.ps@pks.im>
Wow, this is great. I’ve always found git-config a bit hard to use.
Thanks for this.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
From: Junio C Hamano @ 2024-03-06 21:57 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jeff Hostetler via GitGitGadget, git, Jeff Hostetler
In-Reply-To: <ZejkVOVQBZhLVfHW@google.com>
Josh Steadmon <steadmon@google.com> writes:
> On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhostetler@github.com>
>>
>> Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()"
>> and "trace2_cmd_list_env_vars()", we don't need to explicitly call them.
>>
>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
>> ---
>> git.c | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/git.c b/git.c
>> index 7068a184b0a..a769d72ab8f 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
>> strvec_pushv(&child.args, (*argv) + 1);
>>
>> trace2_cmd_alias(alias_command, child.args.v);
>> - trace2_cmd_list_config();
>> - trace2_cmd_list_env_vars();
>> trace2_cmd_name("_run_shell_alias_");
>>
>> ret = run_command(&child);
>> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
>> COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
>>
>> trace2_cmd_alias(alias_command, new_argv);
>> - trace2_cmd_list_config();
>> - trace2_cmd_list_env_vars();
>>
>> *argv = new_argv;
>> *argcp += count - 1;
>> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>>
>> trace_argv_printf(argv, "trace: built-in: git");
>> trace2_cmd_name(p->cmd);
>> - trace2_cmd_list_config();
>> - trace2_cmd_list_env_vars();
>>
>> validate_cache_entries(the_repository->index);
>> status = p->fn(argc, argv, prefix);
>> --
>> gitgitgadget
>>
>
> I'd personally prefer to see this squashed into Patch 3, but I don't
> feel too strongly about it. Either way, the series LGTM.
>
> Reviewed-by: Josh Steadmon <steadmon@google.com>
Let's see what JeffH says about this. I agree with you that making
some stuff redundant in [Patch 3/4] and fixing the redundancy in
this step does feel somewhat roundabout way of doing this.
Thanks.
^ permalink raw reply
* Re: [PATCH 5/8] builtin/config: track subcommands by action
From: Jean-Noël AVILA @ 2024-03-06 21:54 UTC (permalink / raw)
To: git, Patrick Steinhardt
In-Reply-To: <e39774d6495767c6505999bdc33110678139e347.1709724089.git.ps@pks.im>
Le mercredi 6 mars 2024, 12:31:50 CET Patrick Steinhardt a écrit :
> Part of `cmd_config()` is a rather unwieldy switch statement that
> invokes the correct subcommand function based on which action has been
> requested by the user. Now that we have converted actions to be tracked
> via a `OPT_CMDMODE()`, we know that the `actions` variable will
> only ever have at most one bit set. This allows us to convert the
> variable to use an `enum` instead, and thus to create an array that maps
> from this newly introduced `enum` to the corresponding subcommand
> function.
>
> Refactor the code to do so. Besides allowing us to get rid of the giant
> switch statement, this refactoring will also make it easier to introduce
> proper subcommands to git-config(1).
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/config.c | 207 +++++++++++++++++++++++------------------------
> 1 file changed, 99 insertions(+), 108 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index a6ab9b8204..0d58397ef5 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -20,6 +20,26 @@ static const char *const builtin_config_usage[] = {
> NULL
> };
>
> +enum config_action {
> + ACTION_NONE,
> + ACTION_GET,
> + ACTION_GET_ALL,
> + ACTION_GET_REGEXP,
> + ACTION_REPLACE_ALL,
> + ACTION_ADD,
> + ACTION_UNSET,
> + ACTION_UNSET_ALL,
> + ACTION_RENAME_SECTION,
> + ACTION_REMOVE_SECTION,
> + ACTION_LIST,
> + ACTION_EDIT,
> + ACTION_SET,
> + ACTION_SET_ALL,
> + ACTION_GET_COLOR,
> + ACTION_GET_COLORBOOL,
> + ACTION_GET_URLMATCH,
> +};
> +
> static char *key;
> static regex_t *key_regexp;
> static const char *value_pattern;
> @@ -33,10 +53,12 @@ static char delim = '=';
> static char key_delim = ' ';
> static char term = '\n';
>
> +static parse_opt_subcommand_fn *subcommand;
> +static enum config_action action = ACTION_NONE;
> static int use_global_config, use_system_config, use_local_config;
> static int use_worktree_config;
> static struct git_config_source given_config_source;
> -static int actions, type;
> +static int type;
> static char *default_value;
> static int end_nul;
> static int respect_includes_opt = -1;
> @@ -46,30 +68,6 @@ static int show_scope;
> static int fixed_value;
> static int config_flags;
>
> -#define ACTION_GET (1<<0)
> -#define ACTION_GET_ALL (1<<1)
> -#define ACTION_GET_REGEXP (1<<2)
> -#define ACTION_REPLACE_ALL (1<<3)
> -#define ACTION_ADD (1<<4)
> -#define ACTION_UNSET (1<<5)
> -#define ACTION_UNSET_ALL (1<<6)
> -#define ACTION_RENAME_SECTION (1<<7)
> -#define ACTION_REMOVE_SECTION (1<<8)
> -#define ACTION_LIST (1<<9)
> -#define ACTION_EDIT (1<<10)
> -#define ACTION_SET (1<<11)
> -#define ACTION_SET_ALL (1<<12)
> -#define ACTION_GET_COLOR (1<<13)
> -#define ACTION_GET_COLORBOOL (1<<14)
> -#define ACTION_GET_URLMATCH (1<<15)
> -
> -/*
> - * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than
> - * one line of output and which should therefore be paged.
> - */
> -#define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
> - ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
> -
> #define TYPE_BOOL 1
> #define TYPE_INT 2
> #define TYPE_BOOL_OR_INT 3
> @@ -842,6 +840,25 @@ static int cmd_config_get_colorbool(int argc, const char **argv, const char *pre
> return get_colorbool(argv[0], argc == 2);
> }
>
> +static parse_opt_subcommand_fn *subcommands_by_action[] = {
> + [ACTION_LIST] = cmd_config_list,
> + [ACTION_EDIT] = cmd_config_edit,
> + [ACTION_SET] = cmd_config_set,
> + [ACTION_SET_ALL] = cmd_config_set_all,
> + [ACTION_ADD] = cmd_config_add,
> + [ACTION_REPLACE_ALL] = cmd_config_replace_all,
> + [ACTION_GET] = cmd_config_get,
> + [ACTION_GET_ALL] = cmd_config_get_all,
> + [ACTION_GET_REGEXP] = cmd_config_get_regexp,
> + [ACTION_GET_URLMATCH] = cmd_config_get_urlmatch,
> + [ACTION_UNSET] = cmd_config_unset,
> + [ACTION_UNSET_ALL] = cmd_config_unset_all,
> + [ACTION_RENAME_SECTION] = cmd_config_rename_section,
> + [ACTION_REMOVE_SECTION] = cmd_config_remove_section,
> + [ACTION_GET_COLOR] = cmd_config_get_color,
> + [ACTION_GET_COLORBOOL] = cmd_config_get_colorbool,
> +};
> +
> static struct option builtin_config_options[] = {
> OPT_GROUP(N_("Config file location")),
> OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
> @@ -851,20 +868,20 @@ static struct option builtin_config_options[] = {
> OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")),
> OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")),
> OPT_GROUP(N_("Action")),
> - OPT_CMDMODE(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET),
> - OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL),
> - OPT_CMDMODE(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP),
> - OPT_CMDMODE(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
> - OPT_CMDMODE(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL),
> - OPT_CMDMODE(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
> - OPT_CMDMODE(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET),
> - OPT_CMDMODE(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL),
> - OPT_CMDMODE(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
> - OPT_CMDMODE(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
> - OPT_CMDMODE('l', "list", &actions, N_("list all"), ACTION_LIST),
> - OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
> - OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
> - OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
> + OPT_CMDMODE(0, "get", &action, N_("get value: name [value-pattern]"), ACTION_GET),
I guess value-pattern is a placeholder. So for uniformity with the style guidelines, it should be formatted <value-pattern>.
This also applies to other placeholders below.
> + OPT_CMDMODE(0, "get-all", &action, N_("get all values: key [value-pattern]"), ACTION_GET_ALL),
> + OPT_CMDMODE(0, "get-regexp", &action, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP),
> + OPT_CMDMODE(0, "get-urlmatch", &action, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
> + OPT_CMDMODE(0, "replace-all", &action, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL),
> + OPT_CMDMODE(0, "add", &action, N_("add a new variable: name value"), ACTION_ADD),
> + OPT_CMDMODE(0, "unset", &action, N_("remove a variable: name [value-pattern]"), ACTION_UNSET),
> + OPT_CMDMODE(0, "unset-all", &action, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL),
> + OPT_CMDMODE(0, "rename-section", &action, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
> + OPT_CMDMODE(0, "remove-section", &action, N_("remove a section: name"), ACTION_REMOVE_SECTION),
> + OPT_CMDMODE('l', "list", &action, N_("list all"), ACTION_LIST),
> + OPT_CMDMODE('e', "edit", &action, N_("open an editor"), ACTION_EDIT),
> + OPT_CMDMODE(0, "get-color", &action, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
> + OPT_CMDMODE(0, "get-colorbool", &action, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
> OPT_GROUP(N_("Type")),
> OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type),
> OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
> @@ -976,33 +993,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> key_delim = '\n';
> }
>
^ permalink raw reply
* Re: [PATCH 19/22] t/t8*: merge "grep | sed" pipelines
From: Junio C Hamano @ 2024-03-06 21:47 UTC (permalink / raw)
To: Beat Bolli; +Cc: Todd Zullinger, Beat Bolli, git
In-Reply-To: <6571138d-16b0-4c59-b6fe-a256b1c7e37a@drbeat.li>
Beat Bolli <dev+git@drbeat.li> writes:
> On 06.03.24 03:17, Todd Zullinger wrote:
>> Junio C Hamano wrote:
>>> Isn't -E a GNUism?
>>>
>>> At least,
>>>
>>> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
>>>
>>> does not seem to have it (we may need to fix t6030 to rid its only
>>> existing use).
>> I _thought_ that -r was the GNUism. The GNU sed-4.8 manpage
>> says:
>> -E, -r, --regexp-extended
>> use extended regular expressions in the script
>> (for portability use POSIX -E).
>> That doesn't mean the man page is right, of course. :)
>> https://www.austingroupbugs.net/view.php?id=528 suggests
>> that -E has been adopted and, importanly, is more widely
> ...
> At least macOS Ventura and later supports "sed -E". I can't say what
> the more exotic platforms (NonStop?) have.
More exotic platforms may lag behind, and it is a very valid
concern. Also, the bug #528 does talk about -E being accepted, but
wasn't it accepted for Issue 8, which is not finalized yet, no?
So it seems it may be prudent to stick to BRE if we were to squash
these pipelines into a single sed invocation, at least for a few
years.
Thanks.
^ permalink raw reply
* Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
From: Josh Steadmon @ 2024-03-06 21:47 UTC (permalink / raw)
To: Jeff Hostetler via GitGitGadget; +Cc: git, Jeff Hostetler
In-Reply-To: <e8528715ebf97c12622c2e73f914ab4228a0927c.1709566808.git.gitgitgadget@gmail.com>
On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()"
> and "trace2_cmd_list_env_vars()", we don't need to explicitly call them.
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
> git.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/git.c b/git.c
> index 7068a184b0a..a769d72ab8f 100644
> --- a/git.c
> +++ b/git.c
> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
> strvec_pushv(&child.args, (*argv) + 1);
>
> trace2_cmd_alias(alias_command, child.args.v);
> - trace2_cmd_list_config();
> - trace2_cmd_list_env_vars();
> trace2_cmd_name("_run_shell_alias_");
>
> ret = run_command(&child);
> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
> COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
>
> trace2_cmd_alias(alias_command, new_argv);
> - trace2_cmd_list_config();
> - trace2_cmd_list_env_vars();
>
> *argv = new_argv;
> *argcp += count - 1;
> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>
> trace_argv_printf(argv, "trace: built-in: git");
> trace2_cmd_name(p->cmd);
> - trace2_cmd_list_config();
> - trace2_cmd_list_env_vars();
>
> validate_cache_entries(the_repository->index);
> status = p->fn(argc, argv, prefix);
> --
> gitgitgadget
>
I'd personally prefer to see this squashed into Patch 3, but I don't
feel too strongly about it. Either way, the series LGTM.
Reviewed-by: Josh Steadmon <steadmon@google.com>
^ permalink raw reply
* Re: [PATCH 6/8] builtin/config: introduce subcommands
From: Karthik Nayak @ 2024-03-06 21:38 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <f79c0f94e415de5c1c7b4120af5270fe4900d825.1709724089.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> @@ -910,6 +930,20 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> {
> given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
>
> + /*
> + * This is somewhat hacky: we first parse the command line while
> + * keeping all args intact in order to determine whether a subcommand
> + * has been specified. If so, we re-parse it a second time, but this
> + * time we drop KEEP_ARGV0. This is so that we don't munge the command
> + * line in case no subcommand was given, which would otherwise confuse
> + * us when parsing the implicit modes.
> + */
> + argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage,
> + PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_ARGV0|PARSE_OPT_KEEP_UNKNOWN_OPT);
> + if (subcommand)
> + argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage,
> + PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_UNKNOWN_OPT);
> +
I wonder if we can drop the PARSE_OPT_SUBCOMMAND_OPTIONAL in the second
iteration to make it stricter. But this is OK.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 09/22] t/t4*: avoid redundant uses of cat
From: Beat Bolli @ 2024-03-06 20:58 UTC (permalink / raw)
To: Eric Sunshine, Beat Bolli; +Cc: git, Junio C Hamano
In-Reply-To: <CAPig+cQ13SANfReDNxVRPmS2TbK+0ZU4mT6g1rw9kWJHzH-j2w@mail.gmail.com>
On 06.03.24 02:08, Eric Sunshine wrote:
> On Tue, Mar 5, 2024 at 4:31 PM Beat Bolli <bb@drbeat.li> wrote:
>> diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
>> @@ -232,7 +232,7 @@ keep_only_cr () {
>> test_expect_success 'external diff with autocrlf = true' '
>> test_config core.autocrlf true &&
>> GIT_EXTERNAL_DIFF=./fake-diff.sh git diff &&
>> - test $(wc -l < crlfed.txt) = $(cat crlfed.txt | keep_only_cr | wc -c)
>> + test $(wc -l < crlfed.txt) = $(keep_only_cr <crlfed.txt | wc -c)
>> '
>
> Could also fix the style problem (drop whitespace after existing `<`
> operator) while here, but not at all worth a reroll.
>
>> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
>> @@ -786,7 +786,7 @@ test_expect_success 'am takes patches from a Pine mailbox' '
>> git checkout first &&
>> - cat pine patch1 | git am &&
>> + git am pine patch1 &&
>
> As with Junio, the semantic change made here concerned me.
I was even more on the fence about this hunk than the others, but then
the test was about 'am takes patches from a Pine mailbox', not
specifically about reading a Pine mailbox from stdin. But I can drop
this hunk in v2.
Cheers, Beat
^ permalink raw reply
* Re: [PATCH 19/22] t/t8*: merge "grep | sed" pipelines
From: Beat Bolli @ 2024-03-06 21:03 UTC (permalink / raw)
To: Todd Zullinger, Junio C Hamano; +Cc: Beat Bolli, git
In-Reply-To: <ZefSPAiTLrRVeXm6@pobox.com>
On 06.03.24 03:17, Todd Zullinger wrote:
> Junio C Hamano wrote:
>> Isn't -E a GNUism?
>>
>> At least,
>>
>> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
>>
>> does not seem to have it (we may need to fix t6030 to rid its only
>> existing use).
>
> I _thought_ that -r was the GNUism. The GNU sed-4.8 manpage
> says:
>
> -E, -r, --regexp-extended
> use extended regular expressions in the script
> (for portability use POSIX -E).
>
> That doesn't mean the man page is right, of course. :)
>
> https://www.austingroupbugs.net/view.php?id=528 suggests
> that -E has been adopted and, importanly, is more widely
> supported than -r (if we were considering using that rather
> than rewriting this to not use ERE syntax). MacOS in
> particular supports -E but not -r, according to that link.
>
> It seems like the documentation hasn't quite caught up to
> reality yet, perhaps?
>
At least macOS Ventura and later supports "sed -E". I can't say what the
more exotic platforms (NonStop?) have.
^ permalink raw reply
* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Stefan Haller @ 2024-03-06 21:00 UTC (permalink / raw)
To: Elijah Newren, Junio C Hamano
Cc: git, Derrick Stolee, Phillip Wood, Christian Couder
In-Reply-To: <CABPp-BGO2ftEMHJDrf6yg3J4AfpKn=rpf_5Wt_WAS+Hi70KqPQ@mail.gmail.com>
On 06.03.24 03:57, Elijah Newren wrote:
> 1) What if there is a branch that is "just a copy" of one of the
> branches earlier in the "stack"? Since it's "just a copy", shouldn't
> it be excluded for similar reasons to what you are arguing? And, if
> so, which branch is the copy?
This is a good point, but in my experience it's a lot more rare. Maybe
I'm looking at all this just from my own experience, and there might be
other usecases that are very different from mine, but as far as I am
concerned, copies of branches are not long-lived. There is no point in
having two branches point at the same commit. When I create a copy of a
branch, I do that only to rebase the copy somewhere else _immediately_,
leaving the original branch where it was. Which means that I encounter
copied branches only at the top of the stack, not in the middle. Which
means that I'm fine with keeping the current behavior of "rebase
--update-ref" to update both copies of that middle-of-the-stack branch,
because it never happens in practice for me.
> 2) Further, a "stack", to me at least, suggests a linear history
> without branching (i.e. each commit has at most one parent _and_ at
> most one child among the commits in the stack). I designed `git
> replay` to handle diverging histories (i.e. rebasing multiple branches
> that _might_ share a subset of common history but none necessarily
> need to fully contain the others, though perhaps the branches do share
> some other contained branches), and I want it to handle replaying
> merges as well. While `git rebase --update-refs` is absolutely
> limited to "stacks", and thus your argument might make sense in the
> context of `git rebase`, since you are bringing `git replay` into the
> mix, it needs to apply beyond a stack of commits. It's not clear to
> me how to genericize your suggestions to handle cases other than a
> simple stack of commits, though.
I don't see a contradiction here. I don't tend to do this in practice,
but I can totally imagine a tree of stacked branches that share some
common base branches in the beginning and then diverge into different
branches from there. It's true that "rebase --update-refs", when told to
rebase one of the leaf branches, will destroy this tree because it pulls
the base branches away from under the other leaf branches, but this is
unrelated to my proposal, it has this problem today already. And it's
awesome that git replay has a way to avoid this by rebasing the whole
tree at once, keeping everything intact. Still, I don't see what's bad
about excluding branches that point at the same commits as the leaf
branches it is told to rebase when using "replay --contains". (I suppose
what I'm suggesting is to treat "--contains" to mean "is included in the
half-open interval from base to tip" of the revision range you are
rebasing, rather than the closed interval.)
Maybe I should make this more explicit again: I'm not trying to solve
the problem of making a copy of a stack of branches, and rebasing that
copy somewhere else. I think this can't be solved except by making
branch stacks a new concept in git, which I'm not sure we want to do.
> 3) This is mostly covered in (1) and (2), but to be explicit: `git
> replay` is completely against the HEAD-is-special assumptions that are
> pervasive within `git rebase`, and your problem is entirely phrased as
> HEAD-is-special due to your call out of "the current branch". Is your
> argument limited to such special cases? (If so, it might still be
> valid for `git rebase`, of course.)
No, I don't think I need HEAD to be special. "The thing that I'm
rebasing" is special, and it is always HEAD for git rebase, but it can
be something else for replay.
> 4a) `git replay` does what Junio suggests naturally, since it doesn't
> update the refs but instead gives commands which can be fed to `git
> update-ref --stdin`. Thus, users can inspect the output of `git
> replay` and only perform the updates they want (by feeding a subset of
> the lines to update-ref --stdin).
At this point I probably need to explain that I'm rarely using the
command line. I'm a user and co-maintainer of lazygit, and I want to
make lazygit work in such a way that "it does the right thing" in as
many cases as possible.
> 4b) For `git replay`, --contained is just syntactic sugar -- it isn't
> necessary. git replay will allow you to list multiple branches that
> you want replayed, so you can specify which branches are relevant to
> you.
That's great, even if it means that I have to redo some of the work that
--contains would already do for me, just because I want a slightly
different behavior.
> 4c) For `git rebase --update-refs`, you can add `--interactive` and
> then delete the `update-ref` line(s) corresponding to the refs you
> don't want updated.
Yes, that's what I always do today to work around the problem. It's just
easy to forget, and I find it annoying that I have to take this extra
step every time.
One last remark: whenever I describe my use case involving copies of
branches, people tell me not to do that, use detached heads instead, or
other ways to achieve what I want. But then I don't understand why my
proposal would make a difference for them. If you don't use copied
branches, then why do you care whether "rebase --update-refs" or "replay
--contained" moves those copies or not? I still haven't heard a good
argument for why the current behavior is desirable, except for the one
example of a degenerate stack that Phillip Wood described in [1].
-Stefan
[1] <https://public-inbox.org/git/
98548a5b-7d30-543b-b943-fd48d8926a33@gmail.com/>
^ 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