* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Junio C Hamano @ 2023-09-18 17:11 UTC (permalink / raw)
To: Phillip Wood; +Cc: René Scharfe, Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <0bf56c65-e59f-4290-8160-cce141f692d5@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
>> - resume->mode = RESUME_SHOW_PATCH;
>> + resume->mode_int = RESUME_SHOW_PATCH;
>> resume->sub_mode = new_value;
>> return 0;
>> }
>
> Having "mode" and "mode_int" feels a bit fragile as only "mode_int" is
> valid while parsing the options but then we want to use "mode". I
> wonder if we could get Oswald's idea of using callbacks working in a
> reasonably ergonomic way with a couple of macros. We could add an new
> OPTION_SET_ENUM member to "enum parse_opt_type" that would take a
> setter function as well as the usual void *value. To set the value it
> would pass the value pointer and an integer value to the setter
> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and take
> the name of the enum as well as the integer value we want to set for
> that option. The name of the enum would be used to generate the name
> of the setter callback which would be defined with another macro. The
> macro to generate the setter would look like
>
> #define MAKE_CMDMODE_SETTER(name) \
> static void parse_cmdmode_ ## name (void * var, int value) {
> enum name *p = var;
> *p = value;
> }
Ah, OK. So that's how you defeat "how the size and alignment of an
enum mixes well with int is not known and depends on particular enum
type". It is a tad sad that this relies on "void *", which means
that the caller of parse_cmdmode_resume_type cannot be forced by the
compilers to pass "enum resume_type *" to the function, though. And
that is probably inevitable with the design as .enum_setter needs to
be of a single type, and the member in the "struct option" that
points at the destination variable must be "void *" as it has to
be capable of pointing at various different enum types.
> ...
> Then in builtin/am.c at the top level we'd add
>
> MAKE_CMDMODE_SETTER(resume_type)
>
> and change the option definitions to look like
>
> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
Yup, that is ergonomic and corrects "The shape of a particular enum
may not match 'int'" issue nicely. I do not know how severe the
problem is that it is not quite type safe that we cannot enforce
resume_type is the same as typeof(resume.mode) here, though.
Thanks.
^ permalink raw reply
* Re: [PATCH] diff --stat: add config option to limit filename width
From: Junio C Hamano @ 2023-09-18 16:38 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <7aceb7db8d3f4b569564ffd9d1e2e368@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Just checking, do you want me to perform any improvements to this
> patch, so you can have it pulled into one of your trees?
I do not think of any outstanding issues I spotted on this change.
Thanks for pinging.
^ permalink raw reply
* Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on
From: Junio C Hamano @ 2023-09-18 16:35 UTC (permalink / raw)
To: Josip Sokcevic; +Cc: jonathantanmy, git, git
In-Reply-To: <CAJiyOiidpD5eNvxVV8rZP7Xej4_8bBfgTOG75T8PVsmvwya3hg@mail.gmail.com>
Josip Sokcevic <sokcevic@google.com> writes:
> I see you created a new set of patches in a separate thread, so I'll
> start those tests and report back there.
Thanks.
^ permalink raw reply
* [PATCH] git-send-email.perl: avoid printing undef when validating addresses
From: Taylor Blau @ 2023-09-18 16:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Bagas Sanjaya
In-Reply-To: <ZQhI5fMhDE82awpE@debian.me>
When validating email addresses with `extract_valid_address_or_die()`,
we print out a helpful error message when the given input does not
contain a valid email address.
However, the pre-image of this patch looks something like:
my $address = shift;
$address = extract_valid_address($address):
die sprintf(__("..."), $address) if !$address;
which fails when given a bogus email address by trying to use $address
(which is undef) in a sprintf() expansion, like so:
$ git.compile send-email --to="pi <pi@pi>" /tmp/x/*.patch --force
Use of uninitialized value $address in sprintf at /home/ttaylorr/src/git/git-send-email line 1175.
error: unable to extract a valid address from:
This regression dates back to e431225569 (git-send-email: remove invalid
addresses earlier, 2012-11-22), but became more noticeable in a8022c5f7b
(send-email: expose header information to git-send-email's
sendemail-validate hook, 2023-04-19), which validates SMTP headers in
the sendemail-validate hook.
Avoid trying to format an undef by storing the given and cleaned address
separately. After applying this fix, the error contains the invalid
email address, and the warning disappears:
$ git.compile send-email --to="pi <pi@pi>" /tmp/x/*.patch --force
error: unable to extract a valid address from: pi <pi@pi>
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
git-send-email.perl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 897cea6564..288ea1ae80 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1166,10 +1166,10 @@ sub extract_valid_address {
sub extract_valid_address_or_die {
my $address = shift;
- $address = extract_valid_address($address);
+ my $valid_address = extract_valid_address($address);
die sprintf(__("error: unable to extract a valid address from: %s\n"), $address)
- if !$address;
- return $address;
+ if !$valid_address;
+ return $valid_address;
}
sub validate_address {
--
2.42.0.217.g5402a90ddb
^ permalink raw reply related
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: Junio C Hamano @ 2023-09-18 16:17 UTC (permalink / raw)
To: René Scharfe; +Cc: Taylor Blau, Git List, Jeff King
In-Reply-To: <2349e897-9e0d-4341-86fc-9da117a1eb48@web.de>
René Scharfe <l.s.r@web.de> writes:
> It reduces the memory footprint, but only slightly. Saving a few bytes
> for objects with less than a hundred instances total doesn't seem worth
> the downsides.
It makes it impossible to use the both at the same time, which is a
bigger (than reduced memory) advantage. Otherwise, we would be
tempted to consider that having "void *value" and "int value_int"
next to each other and allow them to coexist may be a good solution
for a narrow corner case (please see at the end of the message you
are responding to).
As you said, use of union has its downsides that may contradict the
objective of the larger picture this topic draws.
Thanks.
^ permalink raw reply
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
From: Mark Levedahl @ 2023-09-18 16:25 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin; +Cc: me, git
In-Reply-To: <xmqqpm2fmq2d.fsf@gitster.g>
On 9/18/23 11:58, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> + set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
>>> + return [_open_stdout_stderr $cmd]
>> This looks so much nicer than the original code.
>>
>> Thank you,
>> Johannes
> Yup, looking good.
Thanks. BTW, my commit message at "Furthermore, since v2.36 git exposes
its hook exection machinery via" needs
s/exection/execution/
Should I resend?
Mark
^ permalink raw reply
* Re: [PATCH 1/2] diff-merges: improve --diff-merges documentation
From: Sergey Organov @ 2023-09-18 16:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqcyymly5m.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>>> It is more like that `-p` does not imply `-m` (which used to mean
>>> "consider showing the comparison between parent(s) and the child,
>>> even for merge commits"), even though newer options like `-c`,
>>> `--cc` and others do imply `-m` (simply because they do not make
>>> much sense if they are not allowed to work on merges) that may make
>>> new people confused.
>>
>> No, neither --cc nor -c imply -m.
>
> I was only trying to help you polish the text you added to explain
> what you called the "legacy feature" to reflect the reason behind
> that legacy. As you obviously were not there back then when I made
> "--cc" imply "-m" while keeping "-p" not to imply "-m".
Your help is appreciated, yet unfortunately I still can't figure how to
improve the text based on your advice.
Your "I made --cc imply -m" does not explain why later, when you made
--cc imply -p (did you, or was it somebody else?), you didn't make -m
imply -p at the same time, and then "while keeping -p not to imply -m"
sounds out of place as we rather try to figure why "-m not implies -p".
The "--c imply -m" part of the help raises yet another question: if --cc
implied -m, why it was not -m that was made to imply -p instead of --cc
(and -c)? Then both --cc and -c would imply -p automatically as a
side-effect of implication of -p by -m (do not confuse with agreed
non-implication of -m by -p), and then all the relevant options were
consistent. This consideration renders current situation more surprising
instead of clarifying it, I'm afraid.
"-p does not imply -m" fact is fine with me and is not the cause of user
confusion I'm trying to address. How does it help us to explain why "-m
does not imply -p" though?
[...]
>
> Given that I, together with Linus, invented "--cc" and "-c", taking
> inspiration from how Paul Mackerras showed a merge in his 'gitk'
> tool, and made the design decision not to require "-m" to get the
> output in the format they specify when the "git log" traversal shows
> merge commits, I do not know what to say when you repeat that "--cc"
> does not imply "-m". It simply is not true.
I keep saying "--cc does not imply -m" because it does not seem to,
unless you either use some vague meaning of "imply", or mean some other
"-m", not the one used in "git log". Please check:
$ cd src/git
$ git --version
git version 2.42.0.111.gd814540bb75b
$ git describe
v2.42.0-111-gd814540bb75b
$ git log 74a2e88700efc -n1 -p --cc > diff.actual
$ git log 74a2e88700efc -n1 -p --cc -m > diff.expected
$ cmp diff.expected diff.actual
diff.expected diff.actual differ: byte 706, line 18
$
This test tells us that "--c" is not the same as "--cc -m", that for me
in turn reads "--cc does not imply -m", and that's what I continue to
say.
>
> I think this is the second time you claimed the above after I
> explained the same to you, if I am not mistaken. If you do not want
> to be corrected, that is fine, and I'll stop wasting my time trying
> to correct you.
I'd love to be corrected, but I think I carefully checked my grounds
before saying that --cc does not imply -m, please consider:
1. "--cc implies -m" is not documented. Please point to the
documentation in case I missed it.
2. Git does not behave as if "--cc implied -m", see the test-case above.
If it's neither documented nor matches actual behavior, it's not there,
at least from the POV of random user, to whom my original clarification
of "why -m does not imply -p?" has been addressed.
On top of that, I even can't figure why we argue about it in the first
place, as it seems to be irrelevant to the issue at hand: explain why -m
does not imply -p?
>
> But I still have to make sure that you (or anybody else) do not
> spread misinformation to other users by writing incorrect statements
> in documentation patches.
I'm all against spreading misinformation, and try my best to avoid it
myself. I still fail to see what misinformation, exactly, you find in
this particular explanation by me:
" Note: This option [`-m`] not implying `-p` is legacy feature that is
preserved for the sake of backward compatibility. "
That's exactly what I figured out from a lot of discussions over my
multiple attempts to make `-m` behave more usefully. Is it that "legacy
feature" somehow sounds offensive, or what?
As, despite your help, I fail to come up with better edition of the
note, please, if you feel like it, suggest your own variant of
explanation to the user why `-m` is left inconsistent with the rest of
diff for merges options, provided current matching documentation reads
roughly like this (from more recent options to oldest):
--remrege-diff: produces "remerge" output. Implies -p.
--cc: produces dense combined output. Implies -p.
-c: produces combined output. Implies -p.
-m: produces separate output, provided -p is given as well (?!).
and so why
git log -m
surprisingly has no visible effect, and then the user needs to
type:
git log -m -p
That's all I wanted to explain to the user in a few words with the note
you argue against.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v3] revision: add `--ignore-missing-links` user option
From: Junio C Hamano @ 2023-09-18 15:56 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, me
In-Reply-To: <CAOLa=ZQ9ZRmYe5b6R9ZTTpDCzb2L0UmxkeBujb2kOMSeFuwJGA@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>> Given the above "we merely surface a feature that already exists and
>> supported to be used by the end users from the command line" claim ...
>>
>> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
>> > index ff715d6918..5239d83c76 100644
>> > --- a/builtin/rev-list.c
>> > +++ b/builtin/rev-list.c
>> > @@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,
>> > {
>> > struct rev_list_info *info = cb_data;
>> > if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {
>> > - finish_object__ma(obj);
>> > + if (!info->revs->ignore_missing_links)
>> > + finish_object__ma(obj);
>> > return 1;
>> > }
>>
>> ... this hunk is a bit unexpected. As a low-level plumbing command,
>> shouldn't it be left to the user who gives --ignore-missing-links
>> from their command line to specify how the missing "obj" here should
>> be dealt with by giving the "--missing=<foo>" option? While giving
>> "allow-promisor" may not make much sense, "--missing=allow-any" may
>> of course make sense (it is the same as hardcoding the decision not
>> to call finish_object__ma() at all), and so may "--missing=print".
>>
>
> This is to be expected, in my opinion. In terms of revision.c and
> setting the `revs->ignore_missing_links` bit, the traversal will
> go throw all objects (commits and otherwise) and call
> `show_commit` or `show_object` on them.
Yes. And the user can choose how to handle such an object here by
telling finish_object__ma() with the --missing=<how> option, so
letting them do so, instead of robbing the choice from them, would
be a more flexible design here, right?
> if a commit is
> missing, git-rev-list(1) will still barf an error, but this error
OK, yeah, I do see the need for setting the ignore-missing-links bit
for what you are doing, and --missing and --ignore-missing-links are
orthogonal options. Getting rid of the hardcoded skipping of
finish_object__ma() would make sense from this angle, too.
Thanks.
^ permalink raw reply
* Re: [PATCH v5] merge-tree: add -X strategy option
From: Junio C Hamano @ 2023-09-18 16:08 UTC (permalink / raw)
To: Phillip Wood; +Cc: Izzy via GitGitGadget, git, Elijah Newren, Jeff King, Izzy
In-Reply-To: <67638fd7-ad63-4e20-87e1-bef121fef197@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> ...
> This is the right place to call parse_merge_opt() but I think we
> should first check that the user has requested a real merge rather
> than a trivial merge.
>
> if (xopts.nr && o.mode == MODE_TRIVIAL)
> die(_("--trivial-merge is incompatible with all other options"));
>
> Otherwise if the user passes in invalid strategy option to a trivial
> merge they'll get an error about an invalid strategy option rather
> than being told --strategy-option is not supported with
> --trivial-merge.
I presume that another issue with not having that compatibility
checking with "--trivial" would be when the user passes a valid
strategy option and it would be silently ignored.
> Ideally there would be a preparatory patch that moves the switch
> statement that is below the "if(o.use_stdin)" block up to this point
> so we'd always have set o.mode before checking if it is a trivial
> merge. (I think you'd to change the code slightly when it is moved to
> add a check for o.use_stdin)
That sounds very good.
Thanks for a good "stepping back a bit" review. Highly appreciated.
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Phillip Wood @ 2023-09-18 13:33 UTC (permalink / raw)
To: René Scharfe, Junio C Hamano; +Cc: Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <6dc558c6-f78c-4d9c-8444-498de8e4d22a@web.de>
Hi René
On 18/09/2023 10:28, René Scharfe wrote:
> Here's a version that preserves the enums by using additional int
> variables just for the parsing phase. No tricks. The diff is long, but
> most changes aren't particularly complicated and the resulting code is
> not that ugly. Except for builtin/am.c perhaps, which changes the
> command mode value using a callback as well.
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 202040b62e..00930e2152 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2270,13 +2270,14 @@ enum resume_type {
>
> struct resume_mode {
> enum resume_type mode;
> + int mode_int;
> enum show_patch_type sub_mode;
> };
>
> static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
> {
> int *opt_value = opt->value;
> - struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
> + struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
>
> /*
> * Please update $__git_showcurrentpatch in git-completion.bash
> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
> "--show-current-patch", arg);
> }
>
> - if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
> + if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
> return error(_("options '%s=%s' and '%s=%s' "
> "cannot be used together"),
> "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>
> - resume->mode = RESUME_SHOW_PATCH;
> + resume->mode_int = RESUME_SHOW_PATCH;
> resume->sub_mode = new_value;
> return 0;
> }
Having "mode" and "mode_int" feels a bit fragile as only "mode_int" is
valid while parsing the options but then we want to use "mode". I wonder
if we could get Oswald's idea of using callbacks working in a reasonably
ergonomic way with a couple of macros. We could add an new
OPTION_SET_ENUM member to "enum parse_opt_type" that would take a setter
function as well as the usual void *value. To set the value it would
pass the value pointer and an integer value to the setter function. We
could change OPT_CMDMODE to use OPTION_SET_ENUM and take the name of the
enum as well as the integer value we want to set for that option. The
name of the enum would be used to generate the name of the setter
callback which would be defined with another macro. The macro to
generate the setter would look like
#define MAKE_CMDMODE_SETTER(name) \
static void parse_cmdmode_ ## name (void * var, int value) {
enum name *p = var;
*p = value;
}
then OPT_CMDMODE would look like
#define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \
.type = OPTION_SET_ENUM, \
.short_name = (s), \
.long_name = (l), \
.value = (v), \
.help = (h), \
.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
.defval = (i), \
.enum_setter = parse_cmdmode_ ## n,
}
#define OPT_CMDMODE(s, l, v, h, n, i) OPT_CMDMODE_F(s, l, v, h, n, i, 0)
Then in builtin/am.c at the top level we'd add
MAKE_CMDMODE_SETTER(resume_type)
and change the option definitions to look like
OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
Best Wishes
Phillip
> @@ -2316,7 +2317,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
> int binary = -1;
> int keep_cr = -1;
> int patch_format = PATCH_FORMAT_UNKNOWN;
> - struct resume_mode resume = { .mode = RESUME_FALSE };
> + struct resume_mode resume = { .mode_int = RESUME_FALSE };
> int in_progress;
> int ret = 0;
>
> @@ -2387,27 +2388,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)
> PARSE_OPT_NOARG),
> OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
> N_("override error message when patch failure occurs")),
> - OPT_CMDMODE(0, "continue", &resume.mode,
> + OPT_CMDMODE(0, "continue", &resume.mode_int,
> N_("continue applying patches after resolving a conflict"),
> RESUME_RESOLVED),
> - OPT_CMDMODE('r', "resolved", &resume.mode,
> + OPT_CMDMODE('r', "resolved", &resume.mode_int,
> N_("synonyms for --continue"),
> RESUME_RESOLVED),
> - OPT_CMDMODE(0, "skip", &resume.mode,
> + OPT_CMDMODE(0, "skip", &resume.mode_int,
> N_("skip the current patch"),
> RESUME_SKIP),
> - OPT_CMDMODE(0, "abort", &resume.mode,
> + OPT_CMDMODE(0, "abort", &resume.mode_int,
> N_("restore the original branch and abort the patching operation"),
> RESUME_ABORT),
> - OPT_CMDMODE(0, "quit", &resume.mode,
> + OPT_CMDMODE(0, "quit", &resume.mode_int,
> N_("abort the patching operation but keep HEAD where it is"),
> RESUME_QUIT),
> - { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
> + { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode_int,
> "(diff|raw)",
> N_("show the patch being applied"),
> PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> parse_opt_show_current_patch, RESUME_SHOW_PATCH },
> - OPT_CMDMODE(0, "allow-empty", &resume.mode,
> + OPT_CMDMODE(0, "allow-empty", &resume.mode_int,
> N_("record the empty patch as an empty commit"),
> RESUME_ALLOW_EMPTY),
> OPT_BOOL(0, "committer-date-is-author-date",
> @@ -2439,6 +2440,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
> am_load(&state);
>
> argc = parse_options(argc, argv, prefix, options, usage, 0);
> + resume.mode = resume.mode_int;
>
> if (binary >= 0)
> fprintf_ln(stderr, _("The -b/--binary option has been a no-op for long time, and\n"
> diff --git a/builtin/help.c b/builtin/help.c
> index dc1fbe2b98..d76f88d544 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -51,6 +51,7 @@ static enum help_action {
> HELP_ACTION_CONFIG_FOR_COMPLETION,
> HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
> } cmd_mode;
> +static int cmd_mode_int;
>
> static const char *html_path;
> static int verbose = 1;
> @@ -59,7 +60,7 @@ static int exclude_guides;
> static int show_external_commands = -1;
> static int show_aliases = -1;
> static struct option builtin_help_options[] = {
> - OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
> + OPT_CMDMODE('a', "all", &cmd_mode_int, N_("print all available commands"),
> HELP_ACTION_ALL),
> OPT_BOOL(0, "external-commands", &show_external_commands,
> N_("show external commands in --all")),
> @@ -72,19 +73,19 @@ static struct option builtin_help_options[] = {
> HELP_FORMAT_INFO),
> OPT__VERBOSE(&verbose, N_("print command description")),
>
> - OPT_CMDMODE('g', "guides", &cmd_mode, N_("print list of useful guides"),
> + OPT_CMDMODE('g', "guides", &cmd_mode_int, N_("print list of useful guides"),
> HELP_ACTION_GUIDES),
> - OPT_CMDMODE(0, "user-interfaces", &cmd_mode,
> + OPT_CMDMODE(0, "user-interfaces", &cmd_mode_int,
> N_("print list of user-facing repository, command and file interfaces"),
> HELP_ACTION_USER_INTERFACES),
> - OPT_CMDMODE(0, "developer-interfaces", &cmd_mode,
> + OPT_CMDMODE(0, "developer-interfaces", &cmd_mode_int,
> N_("print list of file formats, protocols and other developer interfaces"),
> HELP_ACTION_DEVELOPER_INTERFACES),
> - OPT_CMDMODE('c', "config", &cmd_mode, N_("print all configuration variable names"),
> + OPT_CMDMODE('c', "config", &cmd_mode_int, N_("print all configuration variable names"),
> HELP_ACTION_CONFIG),
> - OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
> + OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode_int, "",
> HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
> - OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
> + OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode_int, "",
> HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
>
> OPT_END(),
> @@ -640,6 +641,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, builtin_help_options,
> builtin_help_usage, 0);
> parsed_help_format = help_format;
> + cmd_mode = cmd_mode_int;
>
> if (cmd_mode != HELP_ACTION_ALL &&
> (show_external_commands >= 0 ||
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 209d2dc0d5..c64b38614a 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -346,7 +346,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> int i, full_tree = 0;
> int full_name = !prefix || !*prefix;
> read_tree_fn_t fn = NULL;
> - enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
> + enum ls_tree_cmdmode cmdmode;
> + int cmdmode_int = MODE_DEFAULT;
> int null_termination = 0;
> struct ls_tree_options options = { 0 };
> const struct option ls_tree_options[] = {
> @@ -358,13 +359,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> LS_SHOW_TREES),
> OPT_BOOL('z', NULL, &null_termination,
> N_("terminate entries with NUL byte")),
> - OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
> + OPT_CMDMODE('l', "long", &cmdmode_int, N_("include object size"),
> MODE_LONG),
> - OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
> + OPT_CMDMODE(0, "name-only", &cmdmode_int, N_("list only filenames"),
> MODE_NAME_ONLY),
> - OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
> + OPT_CMDMODE(0, "name-status", &cmdmode_int, N_("list only filenames"),
> MODE_NAME_STATUS),
> - OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
> + OPT_CMDMODE(0, "object-only", &cmdmode_int, N_("list only objects"),
> MODE_OBJECT_ONLY),
> OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
> OPT_BOOL(0, "full-tree", &full_tree,
> @@ -384,6 +385,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, ls_tree_options,
> ls_tree_usage, 0);
> options.null_termination = null_termination;
> + cmdmode = cmdmode_int;
>
> if (full_tree)
> prefix = NULL;
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 50cb85751f..6dbe57f0ac 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1061,6 +1061,7 @@ static int check_exec_cmd(const char *cmd)
> int cmd_rebase(int argc, const char **argv, const char *prefix)
> {
> struct rebase_options options = REBASE_OPTIONS_INIT;
> + int action;
> const char *branch_name;
> int ret, flags, total_argc, in_progress = 0;
> int keep_base = 0;
> @@ -1116,18 +1117,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> OPT_BIT(0, "no-ff", &options.flags,
> N_("cherry-pick all commits, even if unchanged"),
> REBASE_FORCE),
> - OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
> + OPT_CMDMODE(0, "continue", &action, N_("continue"),
> ACTION_CONTINUE),
> - OPT_CMDMODE(0, "skip", &options.action,
> + OPT_CMDMODE(0, "skip", &action,
> N_("skip current patch and continue"), ACTION_SKIP),
> - OPT_CMDMODE(0, "abort", &options.action,
> + OPT_CMDMODE(0, "abort", &action,
> N_("abort and check out the original branch"),
> ACTION_ABORT),
> - OPT_CMDMODE(0, "quit", &options.action,
> + OPT_CMDMODE(0, "quit", &action,
> N_("abort but keep HEAD where it is"), ACTION_QUIT),
> - OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
> + OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
> "during an interactive rebase"), ACTION_EDIT_TODO),
> - OPT_CMDMODE(0, "show-current-patch", &options.action,
> + OPT_CMDMODE(0, "show-current-patch", &action,
> N_("show the patch file being applied or merged"),
> ACTION_SHOW_CURRENT_PATCH),
> OPT_CALLBACK_F(0, "apply", &options, NULL,
> @@ -1233,10 +1234,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.type != REBASE_UNSPECIFIED)
> in_progress = 1;
>
> + action = options.action;
> total_argc = argc;
> argc = parse_options(argc, argv, prefix,
> builtin_rebase_options,
> builtin_rebase_usage, 0);
> + options.action = action;
>
> if (preserve_merges_selected)
> die(_("--preserve-merges was replaced by --rebase-merges\n"
> diff --git a/builtin/replace.c b/builtin/replace.c
> index da59600ad2..205c337ad3 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -554,12 +554,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
> MODE_CONVERT_GRAFT_FILE,
> MODE_REPLACE
> } cmdmode = MODE_UNSPECIFIED;
> + int cmdmode_int = cmdmode;
> struct option options[] = {
> - OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
> - OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
> - OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
> - OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
> - OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
> + OPT_CMDMODE('l', "list", &cmdmode_int, N_("list replace refs"), MODE_LIST),
> + OPT_CMDMODE('d', "delete", &cmdmode_int, N_("delete replace refs"), MODE_DELETE),
> + OPT_CMDMODE('e', "edit", &cmdmode_int, N_("edit existing object"), MODE_EDIT),
> + OPT_CMDMODE('g', "graft", &cmdmode_int, N_("change a commit's parents"), MODE_GRAFT),
> + OPT_CMDMODE(0, "convert-graft-file", &cmdmode_int, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
> OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"),
> PARSE_OPT_NOCOMPLETE),
> OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")),
> @@ -572,6 +573,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
>
> argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
>
> + cmdmode = cmdmode_int;
> if (!cmdmode)
> cmdmode = argc ? MODE_REPLACE : MODE_LIST;
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 7b700a9fb1..e8efa0e7ac 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -33,13 +33,14 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> {
> struct strbuf buf = STRBUF_INIT;
> enum stripspace_mode mode = STRIP_DEFAULT;
> + int mode_int = mode;
> int nongit;
>
> const struct option options[] = {
> - OPT_CMDMODE('s', "strip-comments", &mode,
> + OPT_CMDMODE('s', "strip-comments", &mode_int,
> N_("skip and remove all lines starting with comment character"),
> STRIP_COMMENTS),
> - OPT_CMDMODE('c', "comment-lines", &mode,
> + OPT_CMDMODE('c', "comment-lines", &mode_int,
> N_("prepend comment character and space to each line"),
> COMMENT_LINES),
> OPT_END()
> @@ -49,6 +50,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> if (argc)
> usage_with_options(stripspace_usage, options);
>
> + mode = mode_int;
> if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> setup_git_directory_gently(&nongit);
> git_config(git_default_config, NULL);
> diff --git a/parse-options.h b/parse-options.h
> index 5e7475bd2d..349c3fca04 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -262,7 +262,7 @@ struct option {
> .type = OPTION_SET_INT, \
> .short_name = (s), \
> .long_name = (l), \
> - .value = (v), \
> + .value_int = (v), \
> .help = (h), \
> .flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
> .defval = (i), \
> --
> 2.42.0
^ permalink raw reply
* Re: [PATCH v2] git-gui - re-enable use of hook scripts
From: Junio C Hamano @ 2023-09-18 16:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Mark Levedahl, me, git
In-Reply-To: <260b62dc-d45b-58fa-85c4-ffbd981a1c84@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Sounds good. FWIW I ran a couple experiments here, too:
>
> % file pathtype "C:/foo"
> absolute
> % file pathtype ".git/hooks"
> relative
> % file pathtype ".git\\hooks"
> relative
> % file pathtype "/foo"
> volumerelative
> % file pathtype "foo"
> relative
>
> The problem, therefore, is that `file pathtype` does not discern between a
> bare file name and a relative path. The proposed patch looks correct to
> me.
>
> Thank you,
> Johannes
Yup, the other "run hooks in a more modern way using 'git hook'"
patch is the right solution for the immediate breakage, but it still
cannot remove this sanitize_command_line proc as we have other users
and use cases where we want to use the sanitized $PATH search, so
this fix is still needed.
Thanks for a quick review on both patches.
^ permalink raw reply
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
From: Junio C Hamano @ 2023-09-18 15:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Mark Levedahl, me, git
In-Reply-To: <a6998d64-32a7-80b6-f75c-d983ac6130dd@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> + set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
>> + return [_open_stdout_stderr $cmd]
>
> This looks so much nicer than the original code.
>
> Thank you,
> Johannes
Yup, looking good.
^ permalink raw reply
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
From: Johannes Schindelin @ 2023-09-18 15:27 UTC (permalink / raw)
To: Mark Levedahl; +Cc: gitster, me, git
In-Reply-To: <20230917192431.101775-1-mlevedahl@gmail.com>
Hi Mark,
On Sun, 17 Sep 2023, Mark Levedahl wrote:
> git-gui currently runs some hooks directly using its own code written
> before 2010, long predating git v2.9 that added the core.hooksPath
> configuration to override the assumed location at $GIT_DIR/hooks. Thus,
> git-gui looks for and runs hooks including prepare-commit-msg,
> commit-msg, pre-commit, post-commit, and post-checkout from
> $GIT_DIR/hooks, regardless of configuration. Commands (e.g., git-merge)
> that git-gui invokes directly do honor core.hooksPath, meaning the
> overall behaviour is inconsistent.
>
> Furthermore, since v2.36 git exposes its hook exection machinery via
> git-hook run, eliminating the need for others to maintain code
> duplicating that functionality. Using git-hook will both fix git-gui's
> current issues on hook configuration and (presumably) reduce the
> maintenance burden going forward. So, teach git-gui to use git-hook.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
> git-gui.sh | 27 ++-------------------------
> 1 file changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 8603437..3e5907a 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -661,31 +661,8 @@ proc git_write {args} {
> }
>
> proc githook_read {hook_name args} {
> - set pchook [gitdir hooks $hook_name]
> - lappend args 2>@1
> -
> - # On Windows [file executable] might lie so we need to ask
> - # the shell if the hook is executable. Yes that's annoying.
> - #
> - if {[is_Windows]} {
> - upvar #0 _sh interp
> - if {![info exists interp]} {
> - set interp [_which sh]
> - }
> - if {$interp eq {}} {
> - error "hook execution requires sh (not in PATH)"
> - }
> -
> - set scr {if test -x "$1";then exec "$@";fi}
> - set sh_c [list $interp -c $scr $interp $pchook]
> - return [_open_stdout_stderr [concat $sh_c $args]]
> - }
> -
> - if {[file executable $pchook]} {
> - return [_open_stdout_stderr [concat [list $pchook] $args]]
> - }
> -
> - return {}
> + set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
> + return [_open_stdout_stderr $cmd]
This looks so much nicer than the original code.
Thank you,
Johannes
> }
>
> proc kill_file_process {fd} {
> --
> 2.41.0.99.19
>
>
^ permalink raw reply
* Re: [PATCH v2] git-gui - re-enable use of hook scripts
From: Johannes Schindelin @ 2023-09-18 15:26 UTC (permalink / raw)
To: Mark Levedahl; +Cc: gitster, me, git
In-Reply-To: <20230916210131.78593-1-mlevedahl@gmail.com>
Hi,
On Sat, 16 Sep 2023, Mark Levedahl wrote:
> Earlier, commit aae9560a introduced search in $PATH to find executables
> before running them, avoiding an issue where on Windows a same named
> file in the current directory can be executed in preference to anything
> in a directory in $PATH. This search is intended to find an absolute
> path for a bare executable ( e.g, a function "foo") by finding the first
> instance of "foo" in a directory given in $PATH, and this search works
> correctly. The search is explicitly avoided for an executable named
> with an absolute path (e.g., /bin/sh), and that works as well.
>
> Unfortunately, the search is also applied to commands named with a
> relative path. A hook script (or executable) $HOOK is usually located
> relative to the project directory as .git/hooks/$HOOK. The search for
> this will generally fail as that relative path will (probably) not exist
> on any directory in $PATH. This means that git hooks in general now fail
> to run. Considerable mayhem could occur should a directory on $PATH be
> git controlled. If such a directory includes .git/hooks/$HOOK, that
> repository's $HOOK will be substituted for the one in the current
> project, with unknown consequences.
>
> This lookup failure also occurs in worktrees linked to a remote .git
> directory using git-new-workdir. However, a worktree using a .git file
> pointing to a separate git directory apparently avoids this: in that
> case the hook command is resolved to an absolute path before being
> passed down to the code introduced in aae9560a.
>
> Fix this by replacing the test for an "absolute" pathname to a check for
> a command name having more than one pathname component. This limits the
> search and absolute pathname resolution to bare commands. The new test
> uses tcl's "file split" command. Experiments on Linux and Windows, using
> tclsh, show that command names with relative and absolute paths always
> give at least two components, while a bare command gives only one.
>
> Linux: puts [file split {foo}] ==> foo
> Linux: puts [file split {/foo}] ==> / foo
> Linux: puts [file split {.git/foo}] ==> .git foo
> Windows: puts [file split {foo}] ==> foo
> Windows: puts [file split {c:\foo}] ==> c:/ foo
> Windows: puts [file split {.git\foo}] ==> .git foo
>
> The above results show the new test limits search and replacement
> to bare commands on both Linux and Windows.
Sounds good. FWIW I ran a couple experiments here, too:
% file pathtype "C:/foo"
absolute
% file pathtype ".git/hooks"
relative
% file pathtype ".git\\hooks"
relative
% file pathtype "/foo"
volumerelative
% file pathtype "foo"
relative
The problem, therefore, is that `file pathtype` does not discern between a
bare file name and a relative path. The proposed patch looks correct to
me.
Thank you,
Johannes
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
> git-gui.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 8bc8892..8603437 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {
> set i $from_index
> while {$i < [llength $command_line]} {
> set cmd [lindex $command_line $i]
> - if {[file pathtype $cmd] ne "absolute"} {
> + if {[llength [file split $cmd]] < 2} {
> set fullpath [_which $cmd]
> if {$fullpath eq ""} {
> throw {NOT-FOUND} "$cmd not found in PATH"
> --
> 2.41.0.99.19
>
>
^ permalink raw reply
* [REGRESSION] uninitialized value $address in git send-email
From: Bagas Sanjaya @ 2023-09-18 12:56 UTC (permalink / raw)
To: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson
Cc: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]
Hi,
Recently when I was submitting doc fixes to linux-doc mailing list [1]
using git-send-email(1), I got perl-related error:
```
Use of uninitialized value $address in sprintf at /home/bagas/.app/git/dist/v2.42.0/libexec/git-core/git-send-email line 1172.
error: unable to extract a valid address from:
```
My linux.git clone has sendemail-validate hook which uses patatt (from b4
package). The hook is:
```
#!/bin/sh
# installed by patatt install-hook
patatt sign --hook "${1}"
```
This issue occurs on Git v2.41.0 but not in v2.40.0. Bisecting, the culprit is
commit a8022c5f7b67 (send-email: expose header information to git-send-email's
sendemail-validate hook, 2023-04-19). Emily's earlier report [2] also points to
the same culprit, but with different bug.
I triggered this issue on patch series with cover letter. To reproduce:
1. Clone git.git repo, then branch off:
```
$ git clone https://github.com/git/git.git && cd git
$ git checkout -b test
```
2. Make two dummy signed-off commits:
```
$ echo test > test && git add test && git commit -s -m "test"
$ echo "test test" >> test && git commit -a -s -m "test test"
```
3. Generate patch series:
```
$ mkdir /tmp/test
$ git format-patch -o /tmp/test --cover-letter main
```
4. Send the series to dummy address:
```
$ git send-email --to="pi <pi@pi>" /tmp/test/*.patch
```
git-send-email(1) trips on the cover letter since there is no recipient
addresses detected. It also errored out on patches without Signed-off-by
trailer. When the command should have been succeeded, I expected that it
asked me whether to send each patch or not.
My system runs Debian testing (trixie/sid) with perl 5.36.0.
Thanks.
[1]: https://lore.kernel.org/linux-doc/20230918093240.29824-1-bagasdotme@gmail.com/
[2]: https://lore.kernel.org/git/CAJoAoZ=GGgjGOeaeo6RFBO7=6msdRf-Ze6XcnL04K5ugupLUJA@mail.gmail.com/
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: Kristoffer Haugsbakk @ 2023-09-18 11:34 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Taylor Blau, René Scharfe, Git List, Jeff King,
Junio C Hamano
In-Reply-To: <ZP+UgvIon1lrIFa+@ugly>
On Tue, Sep 12, 2023, at 00:28, Oswald Buddenhagen wrote:
> i'd go the opposite way and make it an anonymous union.
> that would require c11, though. imo not exactly an outrageous
> proposition in 2023, but ...
Moving to C11 would get pushback because not all platforms
that people care about support that compiler.[1]
[1] https://lore.kernel.org/git/004601d8ed6b$13a2f580$3ae8e080$@nexbridge.com/
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: Oswald Buddenhagen @ 2023-09-18 10:28 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Taylor Blau, Git List, Jeff King
In-Reply-To: <2349e897-9e0d-4341-86fc-9da117a1eb48@web.de>
On Mon, Sep 18, 2023 at 11:53:19AM +0200, René Scharfe wrote:
>Am 11.09.23 um 21:19 schrieb Junio C Hamano:
>> Yup, that does cross my mind, even though I would have used
>>
>> union {
>> void *void_ptr;
>> int *int_ptr;
>> } value;
>>
>> or something without a rather meaningless 'u'.
>
>OK, but I neglected to ask what we would get out of throwing different
>types into the same bin.
>It complicates type safety by making it impossible for the parser to
>distinguish the used type. [...]
>
this is somewhat ironic, given that using a union has some semantic
value by illustrating that the fields are mutually exclusive.
but i'm not sure that the checking is really important here, given that
the initializers are inside centralized macros anyway.
regards
^ permalink raw reply
* Re: [PATCH v3] revision: add `--ignore-missing-links` user option
From: Karthik Nayak @ 2023-09-18 10:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, me
In-Reply-To: <xmqqfs3fe08e.fsf@gitster.g>
On Fri, Sep 15, 2023 at 8:54 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > From: Karthik Nayak <karthik.188@gmail.com>
> >
> > The revision backend is used by multiple porcelain commands such as
> > git-rev-list(1) and git-log(1). The backend currently supports ignoring
> > missing links by setting the `ignore_missing_links` bit. This allows the
> > revision walk to skip any objects links which are missing. Expose this
> > bit via an `--ignore-missing-links` user option.
>
> Given the above "we merely surface a feature that already exists and
> supported to be used by the end users from the command line" claim ...
>
> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index ff715d6918..5239d83c76 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,
> > {
> > struct rev_list_info *info = cb_data;
> > if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {
> > - finish_object__ma(obj);
> > + if (!info->revs->ignore_missing_links)
> > + finish_object__ma(obj);
> > return 1;
> > }
>
> ... this hunk is a bit unexpected. As a low-level plumbing command,
> shouldn't it be left to the user who gives --ignore-missing-links
> from their command line to specify how the missing "obj" here should
> be dealt with by giving the "--missing=<foo>" option? While giving
> "allow-promisor" may not make much sense, "--missing=allow-any" may
> of course make sense (it is the same as hardcoding the decision not
> to call finish_object__ma() at all), and so may "--missing=print".
>
This is to be expected, in my opinion. In terms of revision.c and setting the
`revs->ignore_missing_links` bit, the traversal will go throw all
objects (commits
and otherwise) and call `show_commit` or `show_object` on them.
Here there is a difference for commits and non-commit objects.
1. Commit objects: commits are parsed in revision.c and after that the
`show_commit`
function is called only when the object is available.
2. Non-commit objects: while trees are parsed in revision.c, blobs are
never parsed and
hence, ` show_object` can be called on missing blobs. This is left to
the user to handle. In
our case, we error out in `rev-list.c`, which is not what we want when using the
`--ignore-missing-links` option. Hence, this addition.
There is an argument to be made around compatibility between the
`--missing` option
and `--ignore-missing-links` option, but since the former only works
with non-commit objects
I think the latter should be independent, and also the latter is about
ignoring all missing links.
I also don't think the user should again specify what to do with
missing links by adding
`--missing=allow-any` as `--ignore-missing-links` is a superset of it.
> Stepping back a bit, with "--missing=print", is this change still
> needed? The missing objects discovered will be shown at the end,
> with the setting, no?
>
The main difference is that the `--missing` options works entirely
with non-commit
objects (I'm assuming this was built with promisor notes in mind). So
if a commit is
missing, git-rev-list(1) will still barf an error, but this error
handling is not in
`builtin/rev-list.c` rather is in a layer above in `revision.c`. So it
wouldn't be trivial for
the `--missing` option to support missing commit links. So that's why we expose
`--ignore-missing-links` which ensures any kind of object (commits
included) if missing,
is ignored.
Thanks for the review!
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Oswald Buddenhagen @ 2023-09-18 10:10 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Jeff King, Git List
In-Reply-To: <6dc558c6-f78c-4d9c-8444-498de8e4d22a@web.de>
On Mon, Sep 18, 2023 at 11:28:31AM +0200, René Scharfe wrote:
>@@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
> "--show-current-patch", arg);
> }
>
>- if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>+ if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>
this illustrates why i don't quite like the approach: the context
determines which variable to use.
my idea would be to introduce a new type OPTION_SET_ENUM which would
also use the callback field. one could even adjust the data type and
elide the callback when c23 mode (or more specifically, the enum size
feature) is detected.
> return error(_("options '%s=%s' and '%s=%s' "
> "cannot be used together"),
> "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>
totally on a tangent: the argument order is bogus here.
and the line wrapping is also funny.
regards
^ permalink raw reply
* Re: [PATCH v5] merge-tree: add -X strategy option
From: Phillip Wood @ 2023-09-18 9:53 UTC (permalink / raw)
To: Izzy via GitGitGadget, git; +Cc: Elijah Newren, Jeff King, Izzy
In-Reply-To: <pull.1565.v5.git.1694853437494.gitgitgadget@gmail.com>
On 16/09/2023 09:37, Izzy via GitGitGadget wrote:
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically resolving conflicts.
I think adding a merge strategy option to merge-tree is a good idea, but
have you tested this with anything apart from -Xours or -Xtheirs? It
looks to me like those are the only two that this patch supports. If you
look at parse_merge_opt() in merge-recursive.c you will see that there
are many other options. In order to support all the merge options I
think this patch needs a bit of refactoring.
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0de42aecf4b..97d0fe6c952 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> static int real_merge(struct merge_tree_options *o,
> @@ -439,6 +441,8 @@ static int real_merge(struct merge_tree_options *o,
>
> init_merge_options(&opt, the_repository);
>
> + opt.recursive_variant = o->merge_options.recursive_variant;
> +
Rather that copying across individual members I think you should
initialize o->merge_options properly in cmd_merge_tree() by calling
init_merge_options() and then use o->merge_options instead of "opt" in
this function. That way all the strategy options will be supported.
> opt.show_rename_progress = 0;
>
> opt.branch1 = branch1;
> @@ -513,6 +517,7 @@ static int real_merge(struct merge_tree_options *o,
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> struct merge_tree_options o = { .show_messages = -1 };
> + struct strvec xopts = STRVEC_INIT;
> int expected_remaining_argc;
> int original_argc;
> const char *merge_base = NULL;
> @@ -548,6 +553,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> &merge_base,
> N_("commit"),
> N_("specify a merge-base for the merge")),
> + OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> + N_("option for selected merge strategy")),
> OPT_END()
> };
>
> @@ -556,6 +563,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
You should add
init_merge_options(&o.merge_options);
here to ensure it is properly initialized.
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
This is the right place to call parse_merge_opt() but I think we should
first check that the user has requested a real merge rather than a
trivial merge.
if (xopts.nr && o.mode == MODE_TRIVIAL)
die(_("--trivial-merge is incompatible with all other options"));
Otherwise if the user passes in invalid strategy option to a trivial
merge they'll get an error about an invalid strategy option rather than
being told --strategy-option is not supported with --trivial-merge.
Ideally there would be a preparatory patch that moves the switch
statement that is below the "if(o.use_stdin)" block up to this point so
we'd always have set o.mode before checking if it is a trivial merge. (I
think you'd to change the code slightly when it is moved to add a check
for o.use_stdin)
Best Wishes
Phillip
> + for (int x = 0; x < xopts.nr; x++)
> + if (parse_merge_opt(&o.merge_options, xopts.v[x]))
> + die(_("unknown strategy option: -X%s"), xopts.v[x]);
> +
> /* Handle --stdin */
> if (o.use_stdin) {
> struct strbuf buf = STRBUF_INIT;
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 250f721795b..b2c8a43fce3 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -22,6 +22,7 @@ test_expect_success setup '
> git branch side1 &&
> git branch side2 &&
> git branch side3 &&
> + git branch side4 &&
>
> git checkout side1 &&
> test_write_lines 1 2 3 4 5 6 >numbers &&
> @@ -46,6 +47,13 @@ test_expect_success setup '
> test_tick &&
> git commit -m rename-numbers &&
>
> + git checkout side4 &&
> + test_write_lines 0 1 2 3 4 5 >numbers &&
> + echo yo >greeting &&
> + git add numbers greeting &&
> + test_tick &&
> + git commit -m other-content-modifications &&
> +
> git switch --orphan unrelated &&
> >something-else &&
> git add something-else &&
> @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
> test_cmp expect actual
> '
>
> +test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
> + git checkout side1^0 &&
> +
> + # make sure merge conflict exists
> + test_must_fail git merge side4 &&
> + git merge --abort &&
> +
> + git merge -X ours side4 &&
> + git rev-parse HEAD^{tree} >expected &&
> +
> + git merge-tree -X ours side1 side4 >actual &&
> +
> + test_cmp expected actual
> +'
> +
> test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' '
> # Mis-spell with single "s" instead of double "s"
> test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&
>
> base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: René Scharfe @ 2023-09-18 9:53 UTC (permalink / raw)
To: Junio C Hamano, Taylor Blau; +Cc: Git List, Jeff King
In-Reply-To: <xmqq7cowv7pm.fsf@gitster.g>
Am 11.09.23 um 21:19 schrieb Junio C Hamano:
> Taylor Blau <me@ttaylorr.com> writes:
>
>> callback, something like:
>>
>> struct option {
>> /* ... */
>> union {
>> void *value;
>> int *value_int;
>> /* etc ... */
>> } u;
>> enum option_type t;
>> };
>>
>> where option_type has some value corresponding to "void *", another for
>> "int *", and so on.
>
> Yup, that does cross my mind, even though I would have used
>
> union {
> void *void_ptr;
> int *int_ptr;
> } value;
>
> or something without a rather meaningless 'u'.
OK, but I neglected to ask what we would get out of throwing different
types into the same bin. It complicates type safety by making it
impossible for the parser to distinguish the used type. This will
become relevant once all int options are converted and value_int can be
made mandatory for them. A named union also requires changing all
users.
It reduces the memory footprint, but only slightly. Saving a few bytes
for objects with less than a hundred instances total doesn't seem worth
the downsides.
René
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-09-18 9:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <xmqqa5tmau6e.fsf@gitster.g>
Am 16.09.23 um 19:45 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>>> True. Though I don't fully understand these warnings (why not then
>>> also warn about if without else?), but taking them away is a bit rude
>>> to those who care.
>>
>> I think losing warnings is unfortunate, but it's just one example.
>> We're losing the type information completely from the values.
>> ...
>>> Or to use an int to point to and then copy into a companion enum
>>> variable to after parsing, which would be my choice.
>>
>> Yeah, I had the same thought. I'm just not sure how to do that in a way
>> that isn't a pain for the callers.
>
> The discussion seems to have petered out around this point.
> What (if anything) do we want to do with this topic?
Here's a version that preserves the enums by using additional int
variables just for the parsing phase. No tricks. The diff is long, but
most changes aren't particularly complicated and the resulting code is
not that ugly. Except for builtin/am.c perhaps, which changes the
command mode value using a callback as well.
--- >8 ---
Subject: [PATCH v2 2/2] parse-options: use and require int pointer for OPT_CMDMODE
Some uses of OPT_CMDMODE provide a pointer to an enum. It is
dereferenced as an int pointer in parse-options.c::get_value(). The two
types are incompatible, though -- the storage size of an enum can vary
between platforms. C23 would allow us to specify the underlying type of
the diffenrent enums, making them compatible, but with C99 the easiest
safe option is to actually use int as the value type.
Convert the offending OPT_CMDMODE users to point to a new int value and
set the enum value after parsing. Switch to the typed value_int pointer
in the macro's definition to enforce that type for future users.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/am.c | 24 +++++++++++++-----------
builtin/help.c | 16 +++++++++-------
builtin/ls-tree.c | 12 +++++++-----
builtin/rebase.c | 15 +++++++++------
builtin/replace.c | 12 +++++++-----
builtin/stripspace.c | 6 ++++--
parse-options.h | 2 +-
7 files changed, 50 insertions(+), 37 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 202040b62e..00930e2152 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2270,13 +2270,14 @@ enum resume_type {
struct resume_mode {
enum resume_type mode;
+ int mode_int;
enum show_patch_type sub_mode;
};
static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
{
int *opt_value = opt->value;
- struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
+ struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
/*
* Please update $__git_showcurrentpatch in git-completion.bash
@@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
"--show-current-patch", arg);
}
- if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
+ if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
return error(_("options '%s=%s' and '%s=%s' "
"cannot be used together"),
"--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
- resume->mode = RESUME_SHOW_PATCH;
+ resume->mode_int = RESUME_SHOW_PATCH;
resume->sub_mode = new_value;
return 0;
}
@@ -2316,7 +2317,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
int binary = -1;
int keep_cr = -1;
int patch_format = PATCH_FORMAT_UNKNOWN;
- struct resume_mode resume = { .mode = RESUME_FALSE };
+ struct resume_mode resume = { .mode_int = RESUME_FALSE };
int in_progress;
int ret = 0;
@@ -2387,27 +2388,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)
PARSE_OPT_NOARG),
OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
N_("override error message when patch failure occurs")),
- OPT_CMDMODE(0, "continue", &resume.mode,
+ OPT_CMDMODE(0, "continue", &resume.mode_int,
N_("continue applying patches after resolving a conflict"),
RESUME_RESOLVED),
- OPT_CMDMODE('r', "resolved", &resume.mode,
+ OPT_CMDMODE('r', "resolved", &resume.mode_int,
N_("synonyms for --continue"),
RESUME_RESOLVED),
- OPT_CMDMODE(0, "skip", &resume.mode,
+ OPT_CMDMODE(0, "skip", &resume.mode_int,
N_("skip the current patch"),
RESUME_SKIP),
- OPT_CMDMODE(0, "abort", &resume.mode,
+ OPT_CMDMODE(0, "abort", &resume.mode_int,
N_("restore the original branch and abort the patching operation"),
RESUME_ABORT),
- OPT_CMDMODE(0, "quit", &resume.mode,
+ OPT_CMDMODE(0, "quit", &resume.mode_int,
N_("abort the patching operation but keep HEAD where it is"),
RESUME_QUIT),
- { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
+ { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode_int,
"(diff|raw)",
N_("show the patch being applied"),
PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
parse_opt_show_current_patch, RESUME_SHOW_PATCH },
- OPT_CMDMODE(0, "allow-empty", &resume.mode,
+ OPT_CMDMODE(0, "allow-empty", &resume.mode_int,
N_("record the empty patch as an empty commit"),
RESUME_ALLOW_EMPTY),
OPT_BOOL(0, "committer-date-is-author-date",
@@ -2439,6 +2440,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
am_load(&state);
argc = parse_options(argc, argv, prefix, options, usage, 0);
+ resume.mode = resume.mode_int;
if (binary >= 0)
fprintf_ln(stderr, _("The -b/--binary option has been a no-op for long time, and\n"
diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b98..d76f88d544 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -51,6 +51,7 @@ static enum help_action {
HELP_ACTION_CONFIG_FOR_COMPLETION,
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
} cmd_mode;
+static int cmd_mode_int;
static const char *html_path;
static int verbose = 1;
@@ -59,7 +60,7 @@ static int exclude_guides;
static int show_external_commands = -1;
static int show_aliases = -1;
static struct option builtin_help_options[] = {
- OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
+ OPT_CMDMODE('a', "all", &cmd_mode_int, N_("print all available commands"),
HELP_ACTION_ALL),
OPT_BOOL(0, "external-commands", &show_external_commands,
N_("show external commands in --all")),
@@ -72,19 +73,19 @@ static struct option builtin_help_options[] = {
HELP_FORMAT_INFO),
OPT__VERBOSE(&verbose, N_("print command description")),
- OPT_CMDMODE('g', "guides", &cmd_mode, N_("print list of useful guides"),
+ OPT_CMDMODE('g', "guides", &cmd_mode_int, N_("print list of useful guides"),
HELP_ACTION_GUIDES),
- OPT_CMDMODE(0, "user-interfaces", &cmd_mode,
+ OPT_CMDMODE(0, "user-interfaces", &cmd_mode_int,
N_("print list of user-facing repository, command and file interfaces"),
HELP_ACTION_USER_INTERFACES),
- OPT_CMDMODE(0, "developer-interfaces", &cmd_mode,
+ OPT_CMDMODE(0, "developer-interfaces", &cmd_mode_int,
N_("print list of file formats, protocols and other developer interfaces"),
HELP_ACTION_DEVELOPER_INTERFACES),
- OPT_CMDMODE('c', "config", &cmd_mode, N_("print all configuration variable names"),
+ OPT_CMDMODE('c', "config", &cmd_mode_int, N_("print all configuration variable names"),
HELP_ACTION_CONFIG),
- OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
+ OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode_int, "",
HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
- OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
+ OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode_int, "",
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
OPT_END(),
@@ -640,6 +641,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, builtin_help_options,
builtin_help_usage, 0);
parsed_help_format = help_format;
+ cmd_mode = cmd_mode_int;
if (cmd_mode != HELP_ACTION_ALL &&
(show_external_commands >= 0 ||
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 209d2dc0d5..c64b38614a 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -346,7 +346,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
int i, full_tree = 0;
int full_name = !prefix || !*prefix;
read_tree_fn_t fn = NULL;
- enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
+ enum ls_tree_cmdmode cmdmode;
+ int cmdmode_int = MODE_DEFAULT;
int null_termination = 0;
struct ls_tree_options options = { 0 };
const struct option ls_tree_options[] = {
@@ -358,13 +359,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
LS_SHOW_TREES),
OPT_BOOL('z', NULL, &null_termination,
N_("terminate entries with NUL byte")),
- OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
+ OPT_CMDMODE('l', "long", &cmdmode_int, N_("include object size"),
MODE_LONG),
- OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
+ OPT_CMDMODE(0, "name-only", &cmdmode_int, N_("list only filenames"),
MODE_NAME_ONLY),
- OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
+ OPT_CMDMODE(0, "name-status", &cmdmode_int, N_("list only filenames"),
MODE_NAME_STATUS),
- OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
+ OPT_CMDMODE(0, "object-only", &cmdmode_int, N_("list only objects"),
MODE_OBJECT_ONLY),
OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
OPT_BOOL(0, "full-tree", &full_tree,
@@ -384,6 +385,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, ls_tree_options,
ls_tree_usage, 0);
options.null_termination = null_termination;
+ cmdmode = cmdmode_int;
if (full_tree)
prefix = NULL;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..6dbe57f0ac 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1061,6 +1061,7 @@ static int check_exec_cmd(const char *cmd)
int cmd_rebase(int argc, const char **argv, const char *prefix)
{
struct rebase_options options = REBASE_OPTIONS_INIT;
+ int action;
const char *branch_name;
int ret, flags, total_argc, in_progress = 0;
int keep_base = 0;
@@ -1116,18 +1117,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
OPT_BIT(0, "no-ff", &options.flags,
N_("cherry-pick all commits, even if unchanged"),
REBASE_FORCE),
- OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
+ OPT_CMDMODE(0, "continue", &action, N_("continue"),
ACTION_CONTINUE),
- OPT_CMDMODE(0, "skip", &options.action,
+ OPT_CMDMODE(0, "skip", &action,
N_("skip current patch and continue"), ACTION_SKIP),
- OPT_CMDMODE(0, "abort", &options.action,
+ OPT_CMDMODE(0, "abort", &action,
N_("abort and check out the original branch"),
ACTION_ABORT),
- OPT_CMDMODE(0, "quit", &options.action,
+ OPT_CMDMODE(0, "quit", &action,
N_("abort but keep HEAD where it is"), ACTION_QUIT),
- OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
+ OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
"during an interactive rebase"), ACTION_EDIT_TODO),
- OPT_CMDMODE(0, "show-current-patch", &options.action,
+ OPT_CMDMODE(0, "show-current-patch", &action,
N_("show the patch file being applied or merged"),
ACTION_SHOW_CURRENT_PATCH),
OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1233,10 +1234,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.type != REBASE_UNSPECIFIED)
in_progress = 1;
+ action = options.action;
total_argc = argc;
argc = parse_options(argc, argv, prefix,
builtin_rebase_options,
builtin_rebase_usage, 0);
+ options.action = action;
if (preserve_merges_selected)
die(_("--preserve-merges was replaced by --rebase-merges\n"
diff --git a/builtin/replace.c b/builtin/replace.c
index da59600ad2..205c337ad3 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -554,12 +554,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
MODE_CONVERT_GRAFT_FILE,
MODE_REPLACE
} cmdmode = MODE_UNSPECIFIED;
+ int cmdmode_int = cmdmode;
struct option options[] = {
- OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
- OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
- OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
- OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
- OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
+ OPT_CMDMODE('l', "list", &cmdmode_int, N_("list replace refs"), MODE_LIST),
+ OPT_CMDMODE('d', "delete", &cmdmode_int, N_("delete replace refs"), MODE_DELETE),
+ OPT_CMDMODE('e', "edit", &cmdmode_int, N_("edit existing object"), MODE_EDIT),
+ OPT_CMDMODE('g', "graft", &cmdmode_int, N_("change a commit's parents"), MODE_GRAFT),
+ OPT_CMDMODE(0, "convert-graft-file", &cmdmode_int, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"),
PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")),
@@ -572,6 +573,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
+ cmdmode = cmdmode_int;
if (!cmdmode)
cmdmode = argc ? MODE_REPLACE : MODE_LIST;
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..e8efa0e7ac 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -33,13 +33,14 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
enum stripspace_mode mode = STRIP_DEFAULT;
+ int mode_int = mode;
int nongit;
const struct option options[] = {
- OPT_CMDMODE('s', "strip-comments", &mode,
+ OPT_CMDMODE('s', "strip-comments", &mode_int,
N_("skip and remove all lines starting with comment character"),
STRIP_COMMENTS),
- OPT_CMDMODE('c', "comment-lines", &mode,
+ OPT_CMDMODE('c', "comment-lines", &mode_int,
N_("prepend comment character and space to each line"),
COMMENT_LINES),
OPT_END()
@@ -49,6 +50,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
if (argc)
usage_with_options(stripspace_usage, options);
+ mode = mode_int;
if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
setup_git_directory_gently(&nongit);
git_config(git_default_config, NULL);
diff --git a/parse-options.h b/parse-options.h
index 5e7475bd2d..349c3fca04 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -262,7 +262,7 @@ struct option {
.type = OPTION_SET_INT, \
.short_name = (s), \
.long_name = (l), \
- .value = (v), \
+ .value_int = (v), \
.help = (h), \
.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
.defval = (i), \
--
2.42.0
^ permalink raw reply related
* [PATCH] git-gui - use git-hook, honor core.hooksPath
From: Mark Levedahl @ 2023-09-17 19:24 UTC (permalink / raw)
To: gitster, johannes.schindelin, me, git; +Cc: Mark Levedahl
In-Reply-To: <fa876f80-02dc-2c04-0db3-bf3f6269b427@gmail.com>
git-gui currently runs some hooks directly using its own code written
before 2010, long predating git v2.9 that added the core.hooksPath
configuration to override the assumed location at $GIT_DIR/hooks. Thus,
git-gui looks for and runs hooks including prepare-commit-msg,
commit-msg, pre-commit, post-commit, and post-checkout from
$GIT_DIR/hooks, regardless of configuration. Commands (e.g., git-merge)
that git-gui invokes directly do honor core.hooksPath, meaning the
overall behaviour is inconsistent.
Furthermore, since v2.36 git exposes its hook exection machinery via
git-hook run, eliminating the need for others to maintain code
duplicating that functionality. Using git-hook will both fix git-gui's
current issues on hook configuration and (presumably) reduce the
maintenance burden going forward. So, teach git-gui to use git-hook.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
git-gui.sh | 27 ++-------------------------
1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index 8603437..3e5907a 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -661,31 +661,8 @@ proc git_write {args} {
}
proc githook_read {hook_name args} {
- set pchook [gitdir hooks $hook_name]
- lappend args 2>@1
-
- # On Windows [file executable] might lie so we need to ask
- # the shell if the hook is executable. Yes that's annoying.
- #
- if {[is_Windows]} {
- upvar #0 _sh interp
- if {![info exists interp]} {
- set interp [_which sh]
- }
- if {$interp eq {}} {
- error "hook execution requires sh (not in PATH)"
- }
-
- set scr {if test -x "$1";then exec "$@";fi}
- set sh_c [list $interp -c $scr $interp $pchook]
- return [_open_stdout_stderr [concat $sh_c $args]]
- }
-
- if {[file executable $pchook]} {
- return [_open_stdout_stderr [concat [list $pchook] $args]]
- }
-
- return {}
+ set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
+ return [_open_stdout_stderr $cmd]
}
proc kill_file_process {fd} {
--
2.41.0.99.19
^ permalink raw reply related
* Re: [PATCH v2] git-gui - re-enable use of hook scripts
From: Mark Levedahl @ 2023-09-17 19:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: johannes.schindelin, me, git
In-Reply-To: <xmqqy1h5aisw.fsf@gitster.g>
On 9/16/23 17:51, Junio C Hamano wrote:
>
> Nice. Now we need to find a replacement maintainer for Git-gui ;-)
> In the meantime, I can queue this patch on top of what I updated
> git-gui part the last time with and merge it in.
>
> Thanks.
Thank you for help on this too. I retired some time ago, and stopped
using git much a decade ago. My popping up on the list was inspired by
cleaning out an old laptop and finding some old patches I thought would
be useful, especially as I'd helped Shawn create some of that old
git-gui/Cygwin code. My interest is unlikely to endure so I'm definitely
not a good candidate to maintain git-gui.
On this hook execution problem, looking further, I find using git-hook
run will fix some other issues in git-gui's hook handling, and that
would actually also patch around the problem we just fixed. So, another
patch follows, the commit message presumes the one fixing relative path
search remains. I would suggest keeping the one fixing the relative path
search regardless.
Mark
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-17 16:44 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git, Alexey Gladkov
In-Reply-To: <9203cd46-6a81-38e4-f191-da0b51e238c1@web.de>
René, sorry for late reply,
On 09/14, René Scharfe wrote:
>
> Am 13.09.23 um 11:46 schrieb Oleg Nesterov:
> >
> > I have another opinion. To me the 2nd "=..." marker does help to
> > understand the hit location. But this doesn't matter.
>
> You see it as another layer of information, as an annotation, an
> additional line containing meta-information. I saw them as context
> lines, i.e. lines from the original file shown in the original order
> without duplication, like - lines, with the only place for meta-
> information being the marker character itself.
Yes,
> > But without my patch, in this case I get
> >
> > TEST.c 1 void func1(struct pid *);
> > TEST.c 3 void func2(struct pid *pid)
> > TEST.c 5 use1(pid);
> > TEST.c 8 void func3(struct pid *pid)
> > TEST.c 10 use2(pid);
> >
> > because the output from git-grep
> >
> > $ git grep --untracked -pn pid TEST.c
> > TEST.c:1:void func1(struct pid *);
> > TEST.c:3:void func2(struct pid *pid)
> > TEST.c:5: use1(pid);
> > TEST.c:8:void func3(struct pid *pid)
> > TEST.c:10: use2(pid);
> >
> > doesn't have the "=..." markers at all.
>
> Sure, that's a problem. You could easily check whether a match is also
> a function line according to the default heuristic
Yes, but...
> there are some impressive regexes in userdiff.c
> and the script would have to figure out which language the file is
> configured to be for Git in the first place.
Yes, and this is what I'd like to avoid, I do not want to duplicate the
builtin_drivers[] logic.
> > in my editor without this patch, I get
> >
> > kernel/sys.c 224 sys_setpriority struct pid *pgrp;
[...snip...]
> Well, your script turns "SYSCALL_DEFINE3(setpriority, [...]" into
> "sys_setpriority" etc., so it is already knows a lot about function lines.
No, not a lot ;)
But yes sure, I can adapt this script to the current behaviour. In fact I
can even change it to not use "-p", the script can read the file backwards
itself (but of course I'd prefer to not do this).
Nevermind. Thanks for discussion. If I can't convince maintainers - lets
forget this patch. Although I will probably keep it (and another one I
had from the very begginning) for myself, it works for me.
> Can we use two markers, i.e. both : and =? No idea what that might break.
...
> So with the patch below this would look like this:
...
> kernel/sys.c#1073#SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
This works for me too. So please CC me if you ever push this change ;)
Thanks,
Oleg.
^ 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