* Re: [DISCUSS] Introducing Rust into the Git project
From: Trevor Gross @ 2024-01-11 20:07 UTC (permalink / raw)
To: rsbecker; +Cc: brian m. carlson, Taylor Blau, Junio C Hamano, Dragan Simic, git
In-Reply-To: <00a801da443d$b1539670$13fac350$@nexbridge.com>
On Wed, Jan 10, 2024 at 10:24 PM <rsbecker@nexbridge.com> wrote:
>
> There are a number of issues for porting gcc (and Go). The list is fairly long, but the summary of what I encountered directly (on the last funded effort of 3) is:
> 1. There are C syntax constructs required to do anything useful (required for access to the OS API) on NonStop that are not in gcc. I can hand code the parser for that, but it would take time.
> 2. The Big Endian x86 architecture is weird to gcc and making that work is not easy.
> 3. There is no assembler on NonStop.
> 4. The ELF header is very different from standard.
> 5. The symbol table structure is radically different, so debugging would be (nearly) impossible or impractical. gdb was ported to account for the platform differences.
> 6. The linkage structure is similar but different from standard.
> 7. The external fixup structure is radically different.
> 8. The loader does not work the same way, so there are required sections of the ELF files on NonStop that are not generated by gcc.
>
> There are more, but I just did not get to the point if hitting them. Part of my own issue is that I have expertise in parsing and semantic passes of compilers, but my code generation skills are not where I want them to be for taking on this effort. Our last funded attempt had a code generation expert and he gave up in frustration.
>
> If I was hired on to do this, it might have a chance, but at an estimate (not mine) of 4-5 person years for a gcc port, best case, my $DAYJOB will not permit it.
>
> If gcc could be ported to NonStop, it would solve so many problems. I have heard of numerous failed efforts beyond what was officially funded by various companies, so this is considered a high-risk project.
Out of curiosity - does the Tandem compiler (assuming that is the
correct name) have a backend that is usable as a library or via an IR?
If so, maybe it would be possible to write a rustc_codegen_tandem
backend like the three that exist (rustc_codegen_{llvm,gcc,cranelift}
at [1]. GCC and cranelift are still under development). This way you
sidestep a lot of the codegen-specific problems listed above.
I am, of course, not suggesting this as a solution for git and am sure
you would rather have GCC support. But I wonder how feasible this
would be if Rust on NonStop is desired at some point.
[1]: https://github.com/rust-lang/rust/tree/062e7c6a951c1e4f33c0a6f6761755949cde15ec/compiler
^ permalink raw reply
* Re: Add support for `git rebase -no-edit`
From: Chaitanya Tata @ 2024-01-11 20:07 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZaBHooq6ORZ3TTkL@nand.local>
Cheers,
Chaitanya.
On Fri, Jan 12, 2024 at 1:25 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> Hi Chaitanya,
>
> On Thu, Jan 11, 2024 at 10:55:47PM +0530, Chaitanya Tata wrote:
> > Hi,
> >
> > I have a feature request to add `--no-edit` option to `git rebase`
> > like we do for `git commit`.
> > The workflow I typically follow is:
> >
> > * `git commit -a --fixup=XXX`
> > * `git rebase -i HEAD~15 --autosquash`
> >
> > But it requires closing the editor without any changes. I can
> > workaround this using the `GIT_EDITOR` option, see [1]. But it would
> > be good to have this built-in.
>
> The easiest workaround would be setting GIT_EDITOR=true, which matches
> the recommendation in [1].
Thanks for the quick response.
>
> Short of that, you can't do a non-interactive rebase, since we rely on
> the todo list generated by interactive rebases in order for
> `--autosquash` to work.
IIUC, creating a todo list needs access to the file used in interactive
rebase?
> Presumably plumbing in a new `--[no-]edit` option would be fairly
> straightforward, and once that is done, the change boils down to just:
I am bit confused by this as this seems to contradict above statement,
I guess the below patch will only work without `--autosquash`? Sorry, not
familiar with git internals.
>
> index 3cc88d8a80..5235a003f2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6169,7 +6169,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> struct todo_list new_todo = TODO_LIST_INIT;
> struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
> struct object_id oid = onto->object.oid;
> - int res;
> + int res = 0;
>
> repo_find_unique_abbrev_r(r, shortonto, &oid,
> DEFAULT_ABBREV);
> @@ -6197,8 +6197,9 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> return error(_("nothing to do"));
> }
>
> - res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> - shortonto, flags);
> + if (!opts->edit)
> + res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> + shortonto, flags);
> if (res == -1)
> return -1;
> else if (res == -2) {
>
> > [1] - https://stackoverflow.com/a/45783848
>
> Thanks,
> Taylor
^ permalink raw reply
* [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action
From: Michael Lohmann @ 2024-01-11 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, mi.al.lohmann, phillip.wood123, phillip.wood, wanja.hentze
In-Reply-To: <xmqqsf33fy3t.fsf@gitster.g>
This is done to avoid having to keep the char values in sync in
different places and also to get compiler warnings on non-exhaustive
switches.
In the rebase `action` enum there is the enumeration constant
`ACTION_NONE` which is not particularly descriptive, since it seems to
imply that no action should be taken. Instead it signals a start of a
revert/cherry-pick process, so here `ACTION_START` was chosen.
Co-authored-by: Wanja Henze <wanja.hentze@bevuta.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
On 11. Jan 2024, at 20:37, Junio C Hamano <gitster@pobox.com> wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> > I think ACTION_NONE was intended to covey that the user did not pass
> > one of the OPT_CMDMODE() options like "--continue" as there isn't a
> > "--start" option. I don't have a strong opinion between "_NONE" and
> > "_START".
>
> I agree with you why NONE is called as such. If "revert" does not
> take "--start" (I do not remember offhand), I would think it would
> be better to follow suit.
My point was that yes, it might be in sync with what the user passes in
as arguments, but when I followed the code and saw lots of references to
ACTION_NONE I was puzzled, since my intuition of that name was that
_no action_ should be taken (which did not make sense to me).
So the (provocative) question is: Do we want to keep the variable name
in sync with some input parameters, or rather with the real action that
should be taken?
(Depending on the outcome of this discussion I would also prepare a
patch renaming it in builtin/rebase.c)
What do you think about this version which keeps the
`if (cmd != ACTION_START)` in favour of the `goto` and instead of the
constant if/else checks for the `verify_opt_compatible` (with the
`assert` at the last one) here is one version with a
`get_cmd_flag`-function (I am not that happy with the name...) that has
a `switch` and it has a runtime error handling with `BUG`.
I think it is the most concise of the options so far.
Ciao
Michael
builtin/revert.c | 65 +++++++++++++++++++++++++++---------------------
1 file changed, 37 insertions(+), 28 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 89821bab95..891aa1d720 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -20,6 +20,14 @@
* Copyright (c) 2005 Junio C Hamano
*/
+enum action {
+ ACTION_START = 0,
+ ACTION_CONTINUE,
+ ACTION_SKIP,
+ ACTION_ABORT,
+ ACTION_QUIT,
+};
+
static const char * const revert_usage[] = {
N_("git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>..."),
N_("git revert (--continue | --skip | --abort | --quit)"),
@@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = {
NULL
};
+static char* get_cmd_optionname(enum action cmd)
+{
+ switch (cmd) {
+ case ACTION_CONTINUE: return "--continue";
+ case ACTION_SKIP: return "--skip";
+ case ACTION_ABORT: return "--abort";
+ case ACTION_QUIT: return "--quit";
+ case ACTION_START: BUG("no commandline flag for ACTION_START");
+ }
+}
+
static const char *action_name(const struct replay_opts *opts)
{
return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
@@ -85,12 +104,12 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
const char *cleanup_arg = NULL;
- int cmd = 0;
+ enum action cmd = ACTION_START;
struct option base_options[] = {
- OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
- OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
- OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
- OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'),
+ OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT),
+ OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE),
+ OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT),
+ OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP),
OPT_CLEANUP(&cleanup_arg),
OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
@@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
}
/* Check for incompatible command line arguments */
- if (cmd) {
- char *this_operation;
- if (cmd == 'q')
- this_operation = "--quit";
- else if (cmd == 'c')
- this_operation = "--continue";
- else if (cmd == 's')
- this_operation = "--skip";
- else {
- assert(cmd == 'a');
- this_operation = "--abort";
- }
-
- verify_opt_compatible(me, this_operation,
+ if (cmd != ACTION_START)
+ verify_opt_compatible(me, get_cmd_optionname(cmd),
"--no-commit", opts->no_commit,
"--signoff", opts->signoff,
"--mainline", opts->mainline,
@@ -168,7 +175,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
"--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
"--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
NULL);
- }
if (!opts->strategy && opts->default_strategy) {
opts->strategy = opts->default_strategy;
@@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
"--edit", opts->edit > 0,
NULL);
- if (cmd) {
- opts->revs = NULL;
- } else {
+ if (cmd == ACTION_START) {
struct setup_revision_opt s_r_opt;
opts->revs = xmalloc(sizeof(*opts->revs));
repo_init_revisions(the_repository, opts->revs, NULL);
@@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
+ } else {
+ opts->revs = NULL;
}
if (argc > 1)
@@ -210,19 +216,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
free(options);
- if (cmd == 'q') {
+ switch (cmd) {
+ case ACTION_QUIT: {
int ret = sequencer_remove_state(opts);
if (!ret)
remove_branch_state(the_repository, 0);
return ret;
}
- if (cmd == 'c')
+ case ACTION_CONTINUE:
return sequencer_continue(the_repository, opts);
- if (cmd == 'a')
+ case ACTION_ABORT:
return sequencer_rollback(the_repository, opts);
- if (cmd == 's')
+ case ACTION_SKIP:
return sequencer_skip(the_repository, opts);
- return sequencer_pick_revisions(the_repository, opts);
+ case ACTION_START:
+ return sequencer_pick_revisions(the_repository, opts);
+ }
}
int cmd_revert(int argc, const char **argv, const char *prefix)
--
2.39.3 (Apple Git-145)
^ permalink raw reply related
* Re: Add support for `git rebase -no-edit`
From: Taylor Blau @ 2024-01-11 19:55 UTC (permalink / raw)
To: Chaitanya Tata; +Cc: git
In-Reply-To: <CADA7-FOE_81ze8hdaRGLPbipihnvFcEYfp9uwnPxPVxDepG4nA@mail.gmail.com>
Hi Chaitanya,
On Thu, Jan 11, 2024 at 10:55:47PM +0530, Chaitanya Tata wrote:
> Hi,
>
> I have a feature request to add `--no-edit` option to `git rebase`
> like we do for `git commit`.
> The workflow I typically follow is:
>
> * `git commit -a --fixup=XXX`
> * `git rebase -i HEAD~15 --autosquash`
>
> But it requires closing the editor without any changes. I can
> workaround this using the `GIT_EDITOR` option, see [1]. But it would
> be good to have this built-in.
The easiest workaround would be setting GIT_EDITOR=true, which matches
the recommendation in [1].
Short of that, you can't do a non-interactive rebase, since we rely on
the todo list generated by interactive rebases in order for
`--autosquash` to work.
Presumably plumbing in a new `--[no-]edit` option would be fairly
straightforward, and once that is done, the change boils down to just:
index 3cc88d8a80..5235a003f2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6169,7 +6169,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
struct todo_list new_todo = TODO_LIST_INIT;
struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
struct object_id oid = onto->object.oid;
- int res;
+ int res = 0;
repo_find_unique_abbrev_r(r, shortonto, &oid,
DEFAULT_ABBREV);
@@ -6197,8 +6197,9 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
return error(_("nothing to do"));
}
- res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
- shortonto, flags);
+ if (!opts->edit)
+ res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
+ shortonto, flags);
if (res == -1)
return -1;
else if (res == -2) {
> [1] - https://stackoverflow.com/a/45783848
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH v2] builtin/revert.c: refactor using an enum for cmd-action
From: Junio C Hamano @ 2024-01-11 19:50 UTC (permalink / raw)
To: Michael Lohmann; +Cc: phillip.wood123, git, phillip.wood, wanja.hentze
In-Reply-To: <20240111174727.55189-1-mi.al.lohmann@gmail.com>
Michael Lohmann <mi.al.lohmann@gmail.com> writes:
> struct option base_options[] = {
> - OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
> - OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
> - OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> - OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'),
> + OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT),
> + OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE),
> + OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT),
> + OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP),
> OPT_CLEANUP(&cleanup_arg),
> OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
> OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
I actually do not terribly mind reusing the single letter option
(e.g. 'x' in "-x") and the command mode, as it is descriptive
enough. The argument that an enum allows compilers warn about
non-exhausitve switches is valid if we have such a switch.
> + switch (cmd) {
> + case ACTION_START:
> + goto skip_opt_compatibility_verification;
> + case ACTION_QUIT:
> + this_operation = "--quit";
> + break;
> + case ACTION_CONTINUE:
> + this_operation = "--continue";
> + break;
> + case ACTION_SKIP:
> + this_operation = "--skip";
> + break;
> + case ACTION_ABORT:
> + this_operation = "--abort";
> + break;
> }
>
> + verify_opt_compatible(me, this_operation,
And indeed the if/elseif cascade in the original is easier to ensure
its exhaustiveness by turning it into a switch.
HOWEVER.
If I were writing this patch, I would rather do it like so:
this_operation = NULL;
switch (cmd) {
case 'q':
this_operation = '--quit";
break;
...
case 'a':
this_operation = '--abort";
break;
default: /* everything else is compatible with others */
break;
}
if (this_operation)
verify_opt_compatible(me, this_operation, ...);
Use of enum is optional; I just didn't like too much churning to
illustrate a single idea (here, the single idea being "switch is
more appropriate then if/else cascade in this case"), and I think
this is easier to read than with enums [*].
Side note: After all the single letter option names are
meant to be mnemonic. "case 'q'" is just as descriptive as
"case ACTION_QUIT" in the context of this switch statement.
HTH.
^ permalink raw reply
* Re: [PATCH] builtin/revert.c: refactor using an enum for cmd-action
From: Junio C Hamano @ 2024-01-11 19:37 UTC (permalink / raw)
To: Phillip Wood; +Cc: Michael Lohmann, git, Wanja Henze
In-Reply-To: <1ed0e249-dab9-4cf3-9b76-c8797b97c9c5@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I think ACTION_NONE was intended to covey that the user did not pass
> one of the OPT_CMDMODE() options like "--continue" as there isn't a
> "--start" option. I don't have a strong opinion between "_NONE" and
> "_START".
I agree with you why NONE is called as such. If "revert" does not
take "--start" (I do not remember offhand), I would think it would
be better to follow suit.
^ permalink raw reply
* Re: [PATCH 2/2] t5541: generalize reference locking
From: Junio C Hamano @ 2024-01-11 18:47 UTC (permalink / raw)
To: Jeff King; +Cc: Justin Tobler via GitGitGadget, git, Justin Tobler
In-Reply-To: <20240111072828.GD48154@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> From: Justin Tobler <jltobler@gmail.com>
>>
>> Some tests set up reference locks by directly creating the lockfile.
>> While this works for the files reference backend, reftable reference
>> locks operate differently and are incompatible with this approach.
>> Generalize reference locking by preparing a reference transaction.
>
> As with the first patch, I think we could use d/f conflicts to get the
> same effect. Perhaps something like this:
Thanks for a great alternative. I agree that avoiding fifo indeed
is a better way to go.
^ permalink raw reply
* Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph
From: Junio C Hamano @ 2024-01-11 18:35 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, git, Scott Leggett
In-Reply-To: <20240111075313.GF48154@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 10, 2024 at 08:38:18AM -0800, Junio C Hamano wrote:
>
>> > It should be easy-ish to iterate through the slab and look at the
>> > commits that are mentioned in it. Though maybe not? Each commit knows
>> > its slab-id, but I'm not sure if we have a master list of commits to go
>> > the other way.
>>
>> We have table of all in-core objects, don't we?
>
> Oh, duh. Yes, we could iterate over obj_hash. I do think the "on demand"
> version I showed later in the message is better, though, as the work
> both scales with the number of affected commits (rather than the total
> number of objects) and can be done lazily (so callers that are not buggy
> pay no price at all).
Yeah, it is far more desirable than scanning obj_hash if we can do
the right thing on lazily.
> So what if we just tried harder to look it up in the graph file (rather
> than the slab) when we see COMMIT_NOT_FROM_GRAPH? And indeed, we even
> have a function to do this already!
;-)
> but:
> ...
> I somehow sniped myself into thinking about it more, but that has only
> reinforced my feeling that I'm afraid to touch it. ;)
Thanks for a nice summary.
^ permalink raw reply
* [PATCH v2] builtin/revert.c: refactor using an enum for cmd-action
From: Michael Lohmann @ 2024-01-11 17:47 UTC (permalink / raw)
To: phillip.wood123; +Cc: git, mi.al.lohmann, phillip.wood, wanja.hentze
In-Reply-To: <1ed0e249-dab9-4cf3-9b76-c8797b97c9c5@gmail.com>
This is done to avoid having to keep the char values in sync in
different places and also to get compiler warnings on non-exhaustive
switches.
In the rebase `action` enum there is the enumeration constant
`ACTION_NONE` which is not particularly descriptive, since it seems to
imply that no action should be taken. Instead it signals a start of a
revert/cherry-pick process, so here `ACTION_START` was chosen.
Co-authored-by: Wanja Henze <wanja.hentze@bevuta.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Hi Phillip!
Thanks for the feedback!
On 11/01/2024 16:57, Phillip Wood wrote:
> I can see the attraction of using an exhaustive switch() here but as
> we don't want to do anything in the _START case it gets a bit messy
> with the extra conditional below. I wonder if we'd be better to
> replace "if (cmd) {" with "if (cmd != REVERT_ACTION_START) {".
> Alternatively if you want to stick with the switch then declaring
> "this_operation" at the beginning of the function would mean you can
> get rid of the new "{}" block.
> The extra indentation here is unfortunate as some of the lines are
> rather long already. In the current code it is clear that we only call
> verify_opt_compatible() when cmd is non-nul, I think it would be
> clearer to use "if (cmd != REVERT_ACTION_START)" here.
Totally agreed - an alternative to the `if` would be a `goto` (see this
version of the patch). This would keep the benefit of the exhaustive
switch, but I don't know if that would fit the style used in this
project... (at least it is a jump forward...)
Changes compared to the previous patch:
- initialize `this_operation` pointer with NULL instead of 0
- drop "REVERT_" prefix of enum and its members
- declare `this_operation` at the toplevel to get rid of codeblock
- skip the verify_opt_compatible in case of ACTION_START with a `goto`
Best wishes
Michael
builtin/revert.c | 90 ++++++++++++++++++++++++++++--------------------
1 file changed, 53 insertions(+), 37 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 89821bab95..19e6653f99 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -20,6 +20,14 @@
* Copyright (c) 2005 Junio C Hamano
*/
+enum action {
+ ACTION_START = 0,
+ ACTION_CONTINUE,
+ ACTION_SKIP,
+ ACTION_ABORT,
+ ACTION_QUIT,
+};
+
static const char * const revert_usage[] = {
N_("git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>..."),
N_("git revert (--continue | --skip | --abort | --quit)"),
@@ -85,12 +93,13 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
const char *cleanup_arg = NULL;
- int cmd = 0;
+ char *this_operation = NULL;
+ enum action cmd = ACTION_START;
struct option base_options[] = {
- OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
- OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
- OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
- OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'),
+ OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT),
+ OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE),
+ OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT),
+ OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP),
OPT_CLEANUP(&cleanup_arg),
OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
@@ -144,32 +153,36 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
}
/* Check for incompatible command line arguments */
- if (cmd) {
- char *this_operation;
- if (cmd == 'q')
- this_operation = "--quit";
- else if (cmd == 'c')
- this_operation = "--continue";
- else if (cmd == 's')
- this_operation = "--skip";
- else {
- assert(cmd == 'a');
- this_operation = "--abort";
- }
-
- verify_opt_compatible(me, this_operation,
- "--no-commit", opts->no_commit,
- "--signoff", opts->signoff,
- "--mainline", opts->mainline,
- "--strategy", opts->strategy ? 1 : 0,
- "--strategy-option", opts->xopts.nr ? 1 : 0,
- "-x", opts->record_origin,
- "--ff", opts->allow_ff,
- "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
- "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
- NULL);
+ switch (cmd) {
+ case ACTION_START:
+ goto skip_opt_compatibility_verification;
+ case ACTION_QUIT:
+ this_operation = "--quit";
+ break;
+ case ACTION_CONTINUE:
+ this_operation = "--continue";
+ break;
+ case ACTION_SKIP:
+ this_operation = "--skip";
+ break;
+ case ACTION_ABORT:
+ this_operation = "--abort";
+ break;
}
+ verify_opt_compatible(me, this_operation,
+ "--no-commit", opts->no_commit,
+ "--signoff", opts->signoff,
+ "--mainline", opts->mainline,
+ "--strategy", opts->strategy ? 1 : 0,
+ "--strategy-option", opts->xopts.nr ? 1 : 0,
+ "-x", opts->record_origin,
+ "--ff", opts->allow_ff,
+ "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
+ "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
+ NULL);
+
+skip_opt_compatibility_verification:
if (!opts->strategy && opts->default_strategy) {
opts->strategy = opts->default_strategy;
opts->default_strategy = NULL;
@@ -183,9 +196,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
"--edit", opts->edit > 0,
NULL);
- if (cmd) {
- opts->revs = NULL;
- } else {
+ if (cmd == ACTION_START) {
struct setup_revision_opt s_r_opt;
opts->revs = xmalloc(sizeof(*opts->revs));
repo_init_revisions(the_repository, opts->revs, NULL);
@@ -198,6 +209,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
+ } else {
+ opts->revs = NULL;
}
if (argc > 1)
@@ -210,19 +223,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
free(options);
- if (cmd == 'q') {
+ switch (cmd) {
+ case ACTION_QUIT: {
int ret = sequencer_remove_state(opts);
if (!ret)
remove_branch_state(the_repository, 0);
return ret;
}
- if (cmd == 'c')
+ case ACTION_CONTINUE:
return sequencer_continue(the_repository, opts);
- if (cmd == 'a')
+ case ACTION_ABORT:
return sequencer_rollback(the_repository, opts);
- if (cmd == 's')
+ case ACTION_SKIP:
return sequencer_skip(the_repository, opts);
- return sequencer_pick_revisions(the_repository, opts);
+ case ACTION_START:
+ return sequencer_pick_revisions(the_repository, opts);
+ }
}
int cmd_revert(int argc, const char **argv, const char *prefix)
--
2.39.3 (Apple Git-145)
^ permalink raw reply related
* Add support for `git rebase -no-edit`
From: Chaitanya Tata @ 2024-01-11 17:25 UTC (permalink / raw)
To: git
Hi,
I have a feature request to add `--no-edit` option to `git rebase`
like we do for `git commit`.
The workflow I typically follow is:
* `git commit -a --fixup=XXX`
* `git rebase -i HEAD~15 --autosquash`
But it requires closing the editor without any changes. I can
workaround this using the `GIT_EDITOR` option, see [1]. But it would
be good to have this built-in.
Thoughts?
[1] - https://stackoverflow.com/a/45783848
Cheers,
Chaitanya.
^ permalink raw reply
* Re: [PATCH 2/3] t7450: test submodule urls
From: Victoria Dye @ 2024-01-11 17:23 UTC (permalink / raw)
To: Junio C Hamano, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <xmqqttnmfarm.fsf@gitster.g>
Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +#define TEST_TOOL_CHECK_URL_USAGE \
>> + "test-tool submodule check-url <url>"
>> +static const char *submodule_check_url_usage[] = {
>> + TEST_TOOL_CHECK_URL_USAGE,
>> + NULL
>> +};
>
> Granted, the entry that follows this new one already uses the same
> pattern, but TEST_TOOL_CHECK_URL_USAGE being used only once here and
> nowhere else, with its name almost as long as the value it expands to,
> I found it unnecessarily verbose and confusing.
This is only used once because I missed the second place it should be used
(in 'submodule_usage[]'). It's still somewhat verbose, but once I fix that
it'll at least have the benefit of avoiding some duplication.
^ permalink raw reply
* Re: [DISCUSS] Introducing Rust into the Git project
From: Elijah Newren @ 2024-01-11 16:57 UTC (permalink / raw)
To: Dragan Simic; +Cc: Taylor Blau, git
In-Reply-To: <f5b9a57b6e2b513f1d79a93c6f0ccf45@manjaro.org>
Hi Dragan,
On Wed, Jan 10, 2024 at 9:39 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2024-01-11 01:33, Elijah Newren wrote:
> > On Wed, Jan 10, 2024 at 1:57 PM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >>
> >> Thus, Git should probably follow the same approach of not converting
> >> the
> >> already existing code
> >
> > I disagree with this. I saw significant performance improvements
> > through converting some existing Git code to Rust. Granted, it was
> > only a small amount of code, but the performance benefits I saw
> > suggested we'd see more by also doing similar conversions elsewhere.
> > (Note that I kept the old C code and then conditionally compiled
> > either Rust or C versions of what I was converting.)
>
> Well, it's also possible that improving the old C code could also result
> in some performance improvements. Thus, quite frankly, I don't see that
> as a valid argument to rewrite some existing C code in Rust.
Yes, and I've made many performance improvements in the C code in git.
Sometimes I make some of the code 5% or 20% faster. Sometimes 1-3
orders of magnitude faster. Once over 60 orders of magnitude
faster.[1] Look around in git's history; I've done a fair amount of
performance stuff.
And I'm specifically arguing that I feel limited in some of the
performance work that can be done by remaining in C. Part of my
reason for interest in Rust is exactly because I think it can help us
improve performance in ways that are far more difficult to achieve in
C. And this isn't just guesswork, I've done some trials with it.
Further, I even took the time to document some of these reasons
elsewhere in this thread[2]. Arguing that some performance
improvements can be done in C is thus entirely missing the point.
If you want to dismiss the performance angle of argument for Rust, you
should take the time to address the actual reasons raised for why it
could make it easier to improve performance relative to continuing in
C.
Also, as a heads up since you seem to be relatively new to the list:
your position will probably carry more weight with others if you take
the time to understand, acknowledge, and/or address counterpoints of
the other party. It is certainly fine to simply express some concerns
without doing so (Randall and Patrick did a good job of this in this
thread), but when you simply assert that the benefits others point out
simply don't exist (e.g. your "Quite frankly, that would _only_
complicate things and cause fragmentation." (emphasis added) from your
first email in this thread[3], and which this latest email of yours
somewhat looks like as well), others may well start applying a
discount to any positions you state. Granted, it's totally up to you,
but I'm just giving a hint about how I think you might be able to be
more persuasive.
Hope that helps,
Elijah
[1] A couple examples: 6a5fb966720 ("Change default merge backend from
recursive to ort", 2021-08-04) and 8d92fb29270 ("dir: replace
exponential algorithm with a linear one", 2020-04-01)
[2] Footnote 6 of
https://lore.kernel.org/git/CABPp-BFOmwV-xBtjvtenb6RFz9wx2VWVpTeho0k=D8wsCCVwqQ@mail.gmail.com/
[3] https://lore.kernel.org/git/b2651b38a4f7edaf1c5ffee72af00e46@manjaro.org/
^ permalink raw reply
* Re: [PATCH] builtin/revert.c: refactor using an enum for cmd-action
From: Phillip Wood @ 2024-01-11 16:57 UTC (permalink / raw)
To: Michael Lohmann, git; +Cc: Wanja Henze
In-Reply-To: <20240111080417.59346-1-mi.al.lohmann@gmail.com>
Hi Michael
On 11/01/2024 08:04, Michael Lohmann wrote:
> This is done to avoid having to keep the char values in sync in
> different places and also to get compiler warnings on non-exhaustive
> switches.
I think this is a reasonable change, thanks for working on it.
> The newly introduced `revert_action`-enum aligns with the
> builtin/rebase.c `action`-enum though the name `revert_action` is chosen
> to better differentiate it from `replay_opts->action` with a different
> function. For rebase the equivalent of the latter is
> `rebase_options->type` and `rebase_options->action` corresponds to the
> `cmd` variable in revert.c.
>
> In the rebase `action` enum there is the enumeration constant
> `ACTION_NONE` which is not particularly descriptive, since it seems to
> imply that no action should be taken. Instead it signals a start of a
> revert/cherry-pick process, so here `REVERT_ACTION_START` was chosen.
I think ACTION_NONE was intended to covey that the user did not pass one
of the OPT_CMDMODE() options like "--continue" as there isn't a
"--start" option. I don't have a strong opinion between "_NONE" and
"_START".
> +enum revert_action {
> + REVERT_ACTION_START = 0,
> + REVERT_ACTION_CONTINUE,
> + REVERT_ACTION_SKIP,
> + REVERT_ACTION_ABORT,
> + REVERT_ACTION_QUIT,
> +};
The "REVERT_" prefix is a bit unfortunate as this is used by cherry-pick
as well but it does match the filename. As this enum is only used in
this file I'd be quite happy to drop the "REVERT_" prefix. (I don't
think we need to go messing with the "action" member of struct
replay_opts to do that)
> /* Check for incompatible command line arguments */
> - if (cmd) {
> - char *this_operation;
> - if (cmd == 'q')
> + {
> + char *this_operation = 0;
style note: we use "NULL" rather than "0" when initializing pointers.
Ideally this would be a "const char *" as we assign string literals but
that is not a new problem with this patch.
> + switch (cmd) {
> + case REVERT_ACTION_START:
> + break;
I can see the attraction of using an exhaustive switch() here but as we
don't want to do anything in the _START case it gets a bit messy with
the extra conditional below. I wonder if we'd be better to replace "if
(cmd) {" with "if (cmd != REVERT_ACTION_START) {". Alternatively if you
want to stick with the switch then declaring "this_operation" at the
beginning of the function would mean you can get rid of the new "{}" block.
> + case REVERT_ACTION_QUIT:
> this_operation = "--quit";
> - else if (cmd == 'c')
> + break;
> + case REVERT_ACTION_CONTINUE:
> this_operation = "--continue";
> - else if (cmd == 's')
> + break;
> + case REVERT_ACTION_SKIP:
> this_operation = "--skip";
> - else {
> - assert(cmd == 'a'); > + break;
> + case REVERT_ACTION_ABORT:
> this_operation = "--abort";
> + break;
> }
>
> - verify_opt_compatible(me, this_operation,
> - "--no-commit", opts->no_commit,
> - "--signoff", opts->signoff,
> - "--mainline", opts->mainline,
> - "--strategy", opts->strategy ? 1 : 0,
> - "--strategy-option", opts->xopts.nr ? 1 : 0,
> - "-x", opts->record_origin,
> - "--ff", opts->allow_ff,
> - "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
> - "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
> - NULL);
> + if (this_operation)
The extra indentation here is unfortunate as some of the lines are
rather long already. In the current code it is clear that we only call
verify_opt_compatible() when cmd is non-nul, I think it would be clearer
to use "if (cmd != REVERT_ACTION_START)" here.
> + verify_opt_compatible(me, this_operation,
> + "--no-commit", opts->no_commit,
> + "--signoff", opts->signoff,
> + "--mainline", opts->mainline,
> + "--strategy", opts->strategy ? 1 : 0,
> + "--strategy-option", opts->xopts.nr ? 1 : 0,
> + "-x", opts->record_origin,
> + "--ff", opts->allow_ff,
> + "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
> + "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
> + NULL);
> }
> [...]
> - if (cmd) {
> - opts->revs = NULL;
> - } else {
> + if (cmd == REVERT_ACTION_START) {
I was momentarily confused by this change but you're reversing the
conditional. I agree that the result is an improvement.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 2/3] t7450: test submodule urls
From: Victoria Dye @ 2024-01-11 16:54 UTC (permalink / raw)
To: Jeff King, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <20240110103812.GB16674@coredump.intra.peff.net>
Jeff King wrote:
> On Tue, Jan 09, 2024 at 05:53:36PM +0000, Victoria Dye via GitGitGadget wrote:
>
>> +#define TEST_TOOL_CHECK_URL_USAGE \
>> + "test-tool submodule check-url <url>"
>
> I don't think this command-line "<url>" mode works at all. Your
> underlying function can handle either stdin or arguments:
...
> All of this is inherited from the existing check_name() code, which I
> think has all of the same bugs. The test scripts all just use the stdin
> mode, so they don't notice. It's not too hard to fix, but maybe it's
> worth just ripping out the unreachable code.
Thanks for pointing out those issues, I think removing the command line
input mode is the way to go. The description of the 'check_name()' mentions
that the stdin mode was "primarily intended for testing". But as 85321a346b5
(submodule--helper: move "check-name" to a test-tool, 2022-09-01) pointed
out, 'check_name()' was never used outside of tests anyway, so whatever use
case was imagined for the command line mode never seemed to have existed.
Combine that with the fact that the command line mode is so different from
the stdin mode (non-zero exit code for invalid names, prints nothing vs.
zero exit code, prints valid names), there don't seem to be any real
downsides to removing the unused code.
>
> -Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] t7501: add tests for --include and --only
From: phillip.wood123 @ 2024-01-11 16:33 UTC (permalink / raw)
To: Ghanshyam Thakkar, git; +Cc: christian.couder, gitster
In-Reply-To: <20240110163622.51182-4-shyamthakkar001@gmail.com>
On 10/01/2024 16:35, Ghanshyam Thakkar wrote:
I don't have much to add to what Junio said, I've just left one comment
below
> +test_expect_success '--only excludes staged changes' '
> + echo content >file &&
> + echo content >baz &&
> + git add baz &&
> + git commit --only -m "file" file &&
> +
> + git diff --name-only >actual &&
> + test_must_be_empty actual &&
> + git diff --name-only --staged >actual &&
> + test_grep "baz" actual
using test_grep feels a bit weak here, I think it would be better to use
test_cmp to ensure that there are no other staged changes.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3 2/2] t7501: add tests for --amend --signoff
From: phillip.wood123 @ 2024-01-11 16:30 UTC (permalink / raw)
To: Ghanshyam Thakkar, git; +Cc: christian.couder, gitster
In-Reply-To: <20240110163622.51182-6-shyamthakkar001@gmail.com>
On 10/01/2024 16:35, Ghanshyam Thakkar wrote:
> Add tests for amending the commit to add Signed-off-by trailer. And
> also to check if it does not add another trailer if one already exists.
>
> Currently, there are tests for --signoff separately in t7501, however,
> they are not tested with --amend.
>
> Therefore, these tests belong with other similar tests of --amend in
> t7501-commit-basic-functionality.
>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
This version looks good, thanks for re-rolling. I have left one comment
below but it is not worth re-rolling just for that.
> +test_expect_success 'amend commit to add signoff' '
> +
> + test_commit "msg" file content &&
> + git commit --amend --signoff &&
> + test_commit_message HEAD <<-EOF
> + msg
> +
> + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
> + EOF
> +
> +'
If you do happen re-roll I think we could happily lose the empty line
before the closing "'" in this test and the next.
Best Wishes
Phillip
^ permalink raw reply
* Re: [GSOC][RFC] Add more builtin patterns for userdiff, as Mircroproject.
From: Sergius Nyah @ 2024-01-11 15:42 UTC (permalink / raw)
To: Christian Couder; +Cc: git, gitster, newren, l.s.r
In-Reply-To: <CAP8UFD0YicYXrYgHdvcrWps+cMa=Dp9Ob7SfYLMXQKLp7-B+7w@mail.gmail.com>
On Thu, Jan 11, 2024 at 4:11 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Thu, Jan 11, 2024 at 3:23 PM Sergius Nyah <sergiusnyah@gmail.com> wrote:
> >
> > ---------- Forwarded message ---------
> > From: Christian Couder <christian.couder@gmail.com>
> > Date: Thu, Jan 11, 2024 at 11:51 AM
> > Subject: Re: [GSOC][RFC] Add more builtin patterns for userdiff, as
> > Mircroproject.
> > To: Sergius Nyah <sergiusnyah@gmail.com>
> >
> >
> > On Thu, Jan 11, 2024 at 11:48 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
> > >
> > > On Wed, Jan 10, 2024 at 9:10 PM Sergius Nyah <sergiusnyah@gmail.com> wrote:
> >
> > > > > Okay then. I will add support for shell, as it was primarily suggested
> > > > > in the idea itself.
> > >
> > > As Bash is a shell and is already supported, I am not sure which kind
> > > of shell you want to support. There are other shells or kinds of shell
> > > with different syntax that might be worth supporting, but then you
> > > might want to tell us which shell or which new kind of shell.
> > >
> > > Thanks!
> >
> > I've definitely learned one more thing. Thank you!
> > Also, after taking another keen look at the Patterns defined in userdiff.c,
> > In accordance with what you've said, I realized that Kotlin isn't
> > supported yet.
>
> It is actually already supported too:
>
> $ grep -i -n kotlin userdiff.c
> 186:PATTERNS("kotlin",
>
> To help you a bit, you can get the list of already supported languages using:
>
> $ perl -ne 'print "$1\n" if m/PATTERNS\(\"(\w+)\"/ or
> m/IPATTERN\(\"(\w+)\"/' <userdiff.c
Thank you for these. $ perl -ne 'print "$1\n" if
m/IPATTERN\(\"(\w+)\"/' userdiff.c works too.
> ada
> bash
> bibtex
> cpp
> csharp
> css
> dts
> elixir
> fortran
> fountain
> golang
> html
> java
> kotlin
> markdown
> matlab
> objc
> pascal
> perl
> php
> python
> ruby
> rust
> scheme
> tex
Great! JavaScript, despite its widespread use, isn't implemented, so I
will take it.
Thank you!
^ permalink raw reply
* Re: [GSOC][RFC] Add more builtin patterns for userdiff, as Mircroproject.
From: Christian Couder @ 2024-01-11 15:11 UTC (permalink / raw)
To: Sergius Nyah; +Cc: git, gitster, newren, l.s.r
In-Reply-To: <CANAnif-hWovPnRmSy0pnEJnA274vwiSAkRexstGQdQJ4u-5+pw@mail.gmail.com>
On Thu, Jan 11, 2024 at 3:23 PM Sergius Nyah <sergiusnyah@gmail.com> wrote:
>
> ---------- Forwarded message ---------
> From: Christian Couder <christian.couder@gmail.com>
> Date: Thu, Jan 11, 2024 at 11:51 AM
> Subject: Re: [GSOC][RFC] Add more builtin patterns for userdiff, as
> Mircroproject.
> To: Sergius Nyah <sergiusnyah@gmail.com>
>
>
> On Thu, Jan 11, 2024 at 11:48 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > On Wed, Jan 10, 2024 at 9:10 PM Sergius Nyah <sergiusnyah@gmail.com> wrote:
>
> > > > Okay then. I will add support for shell, as it was primarily suggested
> > > > in the idea itself.
> >
> > As Bash is a shell and is already supported, I am not sure which kind
> > of shell you want to support. There are other shells or kinds of shell
> > with different syntax that might be worth supporting, but then you
> > might want to tell us which shell or which new kind of shell.
> >
> > Thanks!
>
> I've definitely learned one more thing. Thank you!
> Also, after taking another keen look at the Patterns defined in userdiff.c,
> In accordance with what you've said, I realized that Kotlin isn't
> supported yet.
It is actually already supported too:
$ grep -i -n kotlin userdiff.c
186:PATTERNS("kotlin",
To help you a bit, you can get the list of already supported languages using:
$ perl -ne 'print "$1\n" if m/PATTERNS\(\"(\w+)\"/ or
m/IPATTERN\(\"(\w+)\"/' <userdiff.c
ada
bash
bibtex
cpp
csharp
css
dts
elixir
fortran
fountain
golang
html
java
kotlin
markdown
matlab
objc
pascal
perl
php
python
ruby
rust
scheme
tex
^ permalink raw reply
* Fwd: [GSOC][RFC] Add more builtin patterns for userdiff, as Mircroproject.
From: Sergius Nyah @ 2024-01-11 14:23 UTC (permalink / raw)
To: Christian Couder; +Cc: git, gitster, newren, l.s.r
In-Reply-To: <CAP8UFD3e=Zv2wkx5tswCz05Vwn3vD68Vw-TD6SoENWK+norYsw@mail.gmail.com>
---------- Forwarded message ---------
From: Christian Couder <christian.couder@gmail.com>
Date: Thu, Jan 11, 2024 at 11:51 AM
Subject: Re: [GSOC][RFC] Add more builtin patterns for userdiff, as
Mircroproject.
To: Sergius Nyah <sergiusnyah@gmail.com>
On Thu, Jan 11, 2024 at 11:48 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Jan 10, 2024 at 9:10 PM Sergius Nyah <sergiusnyah@gmail.com> wrote:
> > > Okay then. I will add support for shell, as it was primarily suggested
> > > in the idea itself.
>
> As Bash is a shell and is already supported, I am not sure which kind
> of shell you want to support. There are other shells or kinds of shell
> with different syntax that might be worth supporting, but then you
> might want to tell us which shell or which new kind of shell.
>
> Thanks!
I've definitely learned one more thing. Thank you!
Also, after taking another keen look at the Patterns defined in userdiff.c,
In accordance with what you've said, I realized that Kotlin isn't
supported yet.
So, I'd begin working on it.
Thanks!
By the way I realized that you replied privately to me instead of
keeping the mailing list in Cc: This made me reply privately to you.
Thank you for the correction Christian. Noted!
I think it would be better if the discussions were all public as this
way others could give you their opinion on this.
Definitely True.
^ permalink raw reply
* [PATCH] git-p4: stop reaching into the refdb
From: Patrick Steinhardt @ 2024-01-11 13:47 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]
The git-p4 tool creates a bunch of temporary branches that share a
common prefix "refs/git-p4-tmp/". These branches get cleaned up via
git-update-ref(1) after the import has finished. Once done, we try to
manually remove the now supposedly-empty ".git/refs/git-p4-tmp/"
directory.
This last step can fail in case there still are any temporary branches
around that we failed to delete because `os.rmdir()` refuses to delete a
non-empty directory. It can thus be seen as kind of a sanity check to
verify that we really did delete all temporary branches. Another failure
mode though is when the directory didn't exist in the first place, which
can be the case when using an alternate ref backend like the upcoming
"reftable" backend.
Convert the code to instead use git-for-each-ref(1) to verify that there
are no more temporary branches around. This works alright with alternate
ref backends while retaining the sanity check that we really did prune
all temporary branches.
This is a modification in behaviour for the "files" backend because the
empty directory does not get deleted anymore. But arguably we should not
care about such implementation details of the ref backend anyway, and
this should not cause any user-visible change in behaviour.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
git-p4.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index 0eb3bb4c47..3ea1c405e5 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -4251,7 +4251,8 @@ def run(self, args):
if self.tempBranches != []:
for branch in self.tempBranches:
read_pipe(["git", "update-ref", "-d", branch])
- os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), self.tempBranchLocation))
+ if len(read_pipe(["git", "for-each-ref", self.tempBranchLocation])) > 0:
+ die("There are unexpected temporary branches")
# Create a symbolic ref p4/HEAD pointing to p4/<branch> to allow
# a convenient shortcut refname "p4".
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH] branch: error description when deleting a not fully merged branch
From: Rubén Justo @ 2024-01-11 13:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqbk9tcc57.fsf@gitster.g>
On 10-ene-2024 09:48:52, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> > The error message we show when the user tries to delete a not fully
> > merged branch describes the error and gives a hint to the user:
> >
> > error: the branch 'foo' is not fully merged.
> > If you are sure you want to delete it, run 'git branch -D foo'.
> >
> > Let's move the hint part so that it takes advantage of the advice
> > machinery:
> >
> > error: the branch 'foo' is not fully merged
> > hint: If you are sure you want to delete it, run 'git branch -D foo'
> > hint: Disable this message with "git config advice.forceDeleteBranch false"
>
> This is probably one sensible step forward, so let's queue it as-is.
Thanks.
> But with reservations for longer-term future direction. Stepping
> back a bit, when 'foo' is not fully merged and the user used "branch
> -d" on it, is it sensible for us to suggest use of "branch -D"
Maybe the user hits here because he's doing "branch -d" and so I would
find a more clear suggestion: "branch -d foo -f".
Or to be more generic, not suggesting a command line but a description
that explains how to use "the force" ... :) sorry for the joke
Anyway, I think you mean to suggest a less destructive approach. Which
is fine by me.
> Especially now this is a "hint" to help less experienced folks, it
> may be helpful to suggest how the user can answer "If you are sure
> you want to delete" part. As this knows what unique commits on the
> branch being deleted are about to be lost, one way to do so may be
> to tell the user about them ("you are about to lose 'branch: error
> description when deleting a not fully merged branch' and other 47
> commits that are not merged the target branch 'main'", for example).
That's an interesting idea. Maybe the hint becomes more informative
than a simple advice ... maybe a more-verbose error is needed ... just
thinking out loud ...
>
> Another possibility is to suggest merging the branch into the
> target, instead of suggesting a destructive "deletion", but I
> suspect that it goes too far second-guessing the end-user intention.
>
> Thanks.
Thank you.
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Stefan Haller @ 2024-01-11 13:28 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20240110110842.GD16674@coredump.intra.peff.net>
On 10.01.24 12:08, Jeff King wrote:
> On Mon, Jan 08, 2024 at 10:20:00AM -0800, Junio C Hamano wrote:
>
>> An obvious alternative is to have .lazygit directory next to .git directory
>> which would give you a bigger separation, which can cut both ways.
>
> Just to spell out one of those ways: unlike ".git", we will happily
> check out ".lazygit" from an untrusted remote repository. That may be a
> feature if you want to be able to share project-specific config, or it
> might be a terrible security vulnerability if lazygit config files can
> trigger arbitrary code execution.
Unless you don't version it and add it to .gitignore instead, which (I
suppose) is what most people do with their .vscode/settings.json, for
example.
-Stefan
^ permalink raw reply
* RE: [DISCUSS] Introducing Rust into the Git project
From: rsbecker @ 2024-01-11 13:07 UTC (permalink / raw)
To: 'Elijah Newren'
Cc: 'Taylor Blau', 'Junio C Hamano',
'Dragan Simic', git
In-Reply-To: <CABPp-BGmXw0NQ8yBaMiVXHiKr0-Y_jkZWmJB1CG_oc4UGxt_gA@mail.gmail.com>
On Thursday, January 11, 2024 12:06 AM, Elijah Newren wrote:
>On Wed, Jan 10, 2024 at 6:57 PM <rsbecker@nexbridge.com> wrote:
>>
>> On Wednesday, January 10, 2024 9:21 PM, Elijah Newren wrote:
>> >On Wed, Jan 10, 2024 at 5:44 PM <rsbecker@nexbridge.com> wrote:
>> >>
>> >> On Wednesday, January 10, 2024 7:59 PM, Elijah Newren wrote:
>> >[...]
>> >> >Would you be okay with the following alternative: requiring that
>> >> >all Rust code be optional for now?
>> >> >
>> >> >(In other words, allow you to build with USE_RUST=0, or something
>> >> >like that. And then we have both a Rust and a C implementation of
>> >> >anything that is required for backward compatibility, while any
>> >> >new Rust-only stuff would not be included in your build.)
>> >>
>> >> To address the immediate above, I assume this means that platform
>> >> maintainers will be responsible for developing non-portable
>> >> implementations that duplicate Rust functionality
>> >
>> >This doesn't at all sound like what I thought I said. The whole
>> >proposal was so that folks like NonStop could continue using Git with
>> >no more work than setting
>> >USE_RUST=0 at build time.
>> >
>> >Why do you feel you'd need to duplicate any functionality?
>>
>> I think I misunderstood. What I took from this is that all new functionality would
>be in Rust, which would require a custom implementation in C for platforms that did
>not have Rust available - if that is even practical. Did I get that wrong?
>
>I think you somehow missed the word optional?
>
>I did say that new functionality should be allowed to be Rust only (unlike existing
>functionality), but I'm not sure how you leaped to assuming that all new
>functionality would be in Rust. Further, I also don't understand why you jump to
>assuming that all new functionality needs to be supported on all platforms. The
>point of the word "optional" in my proposal is that it is not required. So, say, if git-
>replay is in Rust, well you've never had git-replay before in any release, so you
>haven't lost any functionality by it being implemented in Rust. And existing things
>(merge, cherry-pick, rebase, etc.) continue working with C-only code. But you may
>have one less optional addition.
>
>At least that was _my_ proposal -- that Rust be optional for now. It does differ from
>what I think Taylor was originally proposing, but that's why I brought it up as an
>alternative proposal.
Thank you for the clarification.
^ permalink raw reply
* [PATCH v2 3/3] branch: make the advice to force-deleting a conditional one
From: Rubén Justo @ 2024-01-11 12:40 UTC (permalink / raw)
To: Git List, Junio C Hamano
In-Reply-To: <4aedc15c-4b3f-4f5e-abea-581b501600f8@gmail.com>
The error message we show when the user tries to delete a not fully
merged branch describes the error and gives a hint to the user:
error: the branch 'foo' is not fully merged.
If you are sure you want to delete it, run 'git branch -D foo'.
Let's move the hint part so that it is displayed using the advice
machinery:
error: the branch 'foo' is not fully merged
hint: If you are sure you want to delete it, run 'git branch -D foo'
hint: Disable this message with "git config advice.forceDeleteBranch false"
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
Documentation/config/advice.txt | 3 +++
advice.c | 1 +
advice.h | 1 +
builtin/branch.c | 8 +++++---
4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index e0deaf3144..25c0917524 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -45,6 +45,9 @@ advice.*::
Advice shown when linkgit:git-fetch[1] takes a long time
to calculate forced updates after ref updates, or to warn
that the check is disabled.
+ forceDeleteBranch::
+ Advice shown when a user tries to delete a not fully merged
+ branch without the force option set.
ignoredHook::
Advice shown if a hook is ignored because the hook is not
set as executable.
diff --git a/advice.c b/advice.c
index 03322caba0..f6e4c2f302 100644
--- a/advice.c
+++ b/advice.c
@@ -47,6 +47,7 @@ static struct {
[ADVICE_DETACHED_HEAD] = { "detachedHead", 1 },
[ADVICE_DIVERGING] = { "diverging", 1 },
[ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 },
+ [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch", 1 },
[ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 },
[ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 },
[ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity", 1 },
diff --git a/advice.h b/advice.h
index 74d44d1156..9d4f49ae38 100644
--- a/advice.h
+++ b/advice.h
@@ -21,6 +21,7 @@ enum advice_type {
ADVICE_DETACHED_HEAD,
ADVICE_DIVERGING,
ADVICE_FETCH_SHOW_FORCED_UPDATES,
+ ADVICE_FORCE_DELETE_BRANCH,
ADVICE_GRAFT_FILE_DEPRECATED,
ADVICE_IGNORED_HOOK,
ADVICE_IMPLICIT_IDENTITY,
diff --git a/builtin/branch.c b/builtin/branch.c
index 0a32d1b6c8..cfb63cce5f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -24,6 +24,7 @@
#include "ref-filter.h"
#include "worktree.h"
#include "help.h"
+#include "advice.h"
#include "commit-reach.h"
static const char * const builtin_branch_usage[] = {
@@ -190,9 +191,10 @@ static int check_branch_commit(const char *branchname, const char *refname,
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("the branch '%s' is not fully merged.\n"
- "If you are sure you want to delete it, "
- "run 'git branch -D %s'"), branchname, branchname);
+ error(_("the branch '%s' is not fully merged"), branchname);
+ advise_if_enabled(ADVICE_FORCE_DELETE_BRANCH,
+ _("If you are sure you want to delete it, "
+ "run 'git branch -D %s'"), branchname);
return -1;
}
return 0;
--
2.42.0
^ permalink raw reply related
* [PATCH v2 2/3] advice: fix an unexpected leading space
From: Rubén Justo @ 2024-01-11 12:40 UTC (permalink / raw)
To: Git List, Junio C Hamano
In-Reply-To: <4aedc15c-4b3f-4f5e-abea-581b501600f8@gmail.com>
This space was introduced, presumably unintentionally, in b3b18d1621
(advice: revamp advise API, 2020-03-02)
I notice this space due to confuse diff outputs while doing some
changes to enum advice_type.
As a reference, a recent change we have to that enum is:
$ git show 35f0383
[ ... ]
diff --git a/advice.h b/advice.h
index 0f584163f5..2affbe1426 100644
--- a/advice.h
+++ b/advice.h
@@ -49,6 +49,7 @@ struct string_list;
ADVICE_UPDATE_SPARSE_PATH,
ADVICE_WAITING_FOR_EDITOR,
ADVICE_SKIPPED_CHERRY_PICKS,
+ ADVICE_WORKTREE_ADD_ORPHAN,
};
Note the hunk header, instead of a much more expected:
@@ -49,6 +49,7 @@ enum advice_type
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
advice.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/advice.h b/advice.h
index 9396bcdcf1..74d44d1156 100644
--- a/advice.h
+++ b/advice.h
@@ -10,7 +10,7 @@ struct string_list;
* Add the new config variable to Documentation/config/advice.txt.
* Call advise_if_enabled to print your advice.
*/
- enum advice_type {
+enum advice_type {
ADVICE_ADD_EMBEDDED_REPO,
ADVICE_ADD_EMPTY_PATHSPEC,
ADVICE_ADD_IGNORED_FILE,
--
2.42.0
^ permalink raw reply related
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