* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-10-03 8:49 UTC (permalink / raw)
To: phillip.wood, Junio C Hamano; +Cc: Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <000ff1b9-e7a5-4dd6-bc61-4b6761f66997@gmail.com>
Am 18.09.23 um 21:48 schrieb Phillip Wood:
> On 18/09/2023 18:11, Junio C Hamano wrote:
>>> 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.
>
> We could use gcc's __builtin_types_compatible_p() if we're prepared to have two definitions of OPT_CMDMODE_F
>
> #if defined(__GNUC__)
> #define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
> ...
> .defval (i) + \
> BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(enum n, __typeof__(v))), \
> }
> #else
> #define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
> ...
> .defval (i), \
> }
> #endif
>
> Best Wishes
>
> Phillip
That would work, but we can do a type check without a compiler builtin
by creating a function for that purpose. How about this?
--- >8 ---
Subject: [PATCH] parse-options: make OPT_CMDMODE type-safe
Some uses of OPT_CMDMODE point to an enum as value, but the option
parser dereferences it as an int pointer. Depending on the platform,
pointers to int and enums can be incompatible.
Add value accessor functions to struct option to dereference the pointer
safely, use them for PARSE_OPT_CMDMODE error detection and in the new
OPTION_SET_VALUE option type, add a typed version of OPT_CMDMODE named
OPT_CMDMODE_T, make OPT_CMDMODE only accept int and convert enum users
to declare the appropriate type.
The accessor functions for each type must be defined using
DEFINE_OPTION_VALUE_TYPE. OPT_CMDMODE needs the ones for int, so we
define them centrally in parse-options.h.
builtin/am.c::parse_opt_show_current_patch() uses the value pointer just
to calculate the offset of the enclosing struct resume_mode, making the
pointer type inconsequential. Use the correct type instead of int *
regardless, for consistency.
The enum used with OPT_CMDMODE in builtin/replace.c was anonymous. Give
it a name to allow specifying it in DEFINE_OPTION_VALUE_TYPE and then
OPT_CMDMODE_T.
Suggested-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Documentation/technical/api-parse-options.txt | 7 ++++
builtin/am.c | 32 ++++++++++------
builtin/help.c | 37 +++++++++++--------
builtin/ls-tree.c | 18 +++++----
builtin/rebase.c | 33 ++++++++++-------
builtin/replace.c | 37 ++++++++++++-------
builtin/stripspace.c | 14 ++++---
parse-options.c | 20 +++++++++-
parse-options.h | 36 ++++++++++++++++--
9 files changed, 159 insertions(+), 75 deletions(-)
diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 61fa6ee167..3958ab7c94 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -278,6 +278,13 @@ There are some macros to easily define options:
option has already set its value to the same `int_var`.
In new commands consider using subcommands instead.
+`OPT_CMDMODE_T(short, long, type_name, &enum_var, description, enum_val)`::
+ Same as `OPT_CMDMODE`, but allows specifying an enum variable
+ instead of an int.
+ Requires `DEFINE_OPTION_VALUE_TYPE(type_name, enum_type);` at
+ file scope to declare `type_name`; `enum_type` must be the type
+ of `enum_var`.
+
`OPT_SUBCOMMAND(long, &fn_ptr, subcommand_fn)`::
Define a subcommand. `subcommand_fn` is put into `fn_ptr` when
this subcommand is used.
diff --git a/builtin/am.c b/builtin/am.c
index 6655059a57..4642dc5099 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2268,6 +2268,8 @@ enum resume_type {
RESUME_ALLOW_EMPTY,
};
+DEFINE_OPTION_VALUE_TYPE(resume_type, enum resume_type);
+
struct resume_mode {
enum resume_type mode;
enum show_patch_type sub_mode;
@@ -2275,7 +2277,7 @@ struct resume_mode {
static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
{
- int *opt_value = opt->value;
+ enum resume_type *opt_value = opt->value;
struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
/*
@@ -2388,27 +2390,33 @@ 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_T(0, "continue", resume_type, &resume.mode,
N_("continue applying patches after resolving a conflict"),
RESUME_RESOLVED),
- OPT_CMDMODE('r', "resolved", &resume.mode,
+ OPT_CMDMODE_T('r', "resolved", resume_type, &resume.mode,
N_("synonyms for --continue"),
RESUME_RESOLVED),
- OPT_CMDMODE(0, "skip", &resume.mode,
+ OPT_CMDMODE_T(0, "skip", resume_type, &resume.mode,
N_("skip the current patch"),
RESUME_SKIP),
- OPT_CMDMODE(0, "abort", &resume.mode,
+ OPT_CMDMODE_T(0, "abort", resume_type, &resume.mode,
N_("restore the original branch and abort the patching operation"),
RESUME_ABORT),
- OPT_CMDMODE(0, "quit", &resume.mode,
+ OPT_CMDMODE_T(0, "quit", resume_type, &resume.mode,
N_("abort the patching operation but keep HEAD where it is"),
RESUME_QUIT),
- { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
- "(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,
+ {
+ .type = OPTION_CALLBACK,
+ .long_name = "show-current-patch",
+ OPTION_VALUE(resume_type, &resume.mode),
+ .argh = "(diff|raw)",
+ .help = N_("show the patch being applied"),
+ .flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG |
+ PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+ .callback = parse_opt_show_current_patch,
+ .defval = RESUME_SHOW_PATCH,
+ },
+ OPT_CMDMODE_T(0, "allow-empty", resume_type, &resume.mode,
N_("record the empty patch as an empty commit"),
RESUME_ALLOW_EMPTY),
OPT_BOOL(0, "committer-date-is-author-date",
diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b98..48437c8636 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -52,6 +52,8 @@ static enum help_action {
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
} cmd_mode;
+DEFINE_OPTION_VALUE_TYPE(help_action, enum help_action);
+
static const char *html_path;
static int verbose = 1;
static enum help_format help_format = HELP_FORMAT_NONE;
@@ -59,8 +61,8 @@ 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"),
- HELP_ACTION_ALL),
+ OPT_CMDMODE_T('a', "all", help_action, &cmd_mode,
+ N_("print all available commands"), HELP_ACTION_ALL),
OPT_BOOL(0, "external-commands", &show_external_commands,
N_("show external commands in --all")),
OPT_BOOL(0, "aliases", &show_aliases, N_("show aliases in --all")),
@@ -72,20 +74,23 @@ 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"),
- HELP_ACTION_GUIDES),
- OPT_CMDMODE(0, "user-interfaces", &cmd_mode,
- N_("print list of user-facing repository, command and file interfaces"),
- HELP_ACTION_USER_INTERFACES),
- OPT_CMDMODE(0, "developer-interfaces", &cmd_mode,
- 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"),
- HELP_ACTION_CONFIG),
- OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
- HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
- OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
- HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+ OPT_CMDMODE_T('g', "guides", help_action, &cmd_mode,
+ N_("print list of useful guides"), HELP_ACTION_GUIDES),
+ OPT_CMDMODE_T(0, "user-interfaces", help_action, &cmd_mode,
+ N_("print list of user-facing repository, command and file interfaces"),
+ HELP_ACTION_USER_INTERFACES),
+ OPT_CMDMODE_T(0, "developer-interfaces", help_action, &cmd_mode,
+ N_("print list of file formats, protocols and other developer interfaces"),
+ HELP_ACTION_DEVELOPER_INTERFACES),
+ OPT_CMDMODE_T('c', "config", help_action, &cmd_mode,
+ N_("print all configuration variable names"),
+ HELP_ACTION_CONFIG),
+ OPT_CMDMODE_T_F(0, "config-for-completion", help_action, &cmd_mode, "",
+ HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+ OPT_CMDMODE_T_F(0, "config-sections-for-completion",
+ help_action, &cmd_mode, "",
+ HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
+ PARSE_OPT_HIDDEN),
OPT_END(),
};
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 209d2dc0d5..2e7faf510d 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -339,6 +339,8 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {
},
};
+DEFINE_OPTION_VALUE_TYPE(cmdmode, enum ls_tree_cmdmode);
+
int cmd_ls_tree(int argc, const char **argv, const char *prefix)
{
struct object_id oid;
@@ -358,14 +360,14 @@ 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"),
- MODE_LONG),
- OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
- MODE_NAME_ONLY),
- OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
- MODE_NAME_STATUS),
- OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
- MODE_OBJECT_ONLY),
+ OPT_CMDMODE_T('l', "long", cmdmode, &cmdmode,
+ N_("include object size"), MODE_LONG),
+ OPT_CMDMODE_T(0, "name-only", cmdmode, &cmdmode,
+ N_("list only filenames"), MODE_NAME_ONLY),
+ OPT_CMDMODE_T(0, "name-status", cmdmode, &cmdmode,
+ N_("list only filenames"), MODE_NAME_STATUS),
+ OPT_CMDMODE_T(0, "object-only", cmdmode, &cmdmode,
+ 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,
N_("list entire tree; not just current directory "
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ed15accec9..50739327bd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -75,6 +75,8 @@ enum action {
ACTION_SHOW_CURRENT_PATCH
};
+DEFINE_OPTION_VALUE_TYPE(action, enum action);
+
static const char *action_names[] = {
"undefined",
"continue",
@@ -1116,20 +1118,23 @@ 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"),
- ACTION_CONTINUE),
- OPT_CMDMODE(0, "skip", &options.action,
- N_("skip current patch and continue"), ACTION_SKIP),
- OPT_CMDMODE(0, "abort", &options.action,
- N_("abort and check out the original branch"),
- ACTION_ABORT),
- OPT_CMDMODE(0, "quit", &options.action,
- N_("abort but keep HEAD where it is"), ACTION_QUIT),
- OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
- "during an interactive rebase"), ACTION_EDIT_TODO),
- OPT_CMDMODE(0, "show-current-patch", &options.action,
- N_("show the patch file being applied or merged"),
- ACTION_SHOW_CURRENT_PATCH),
+ OPT_CMDMODE_T(0, "continue", action, &options.action,
+ N_("continue"), ACTION_CONTINUE),
+ OPT_CMDMODE_T(0, "skip", action, &options.action,
+ N_("skip current patch and continue"),
+ ACTION_SKIP),
+ OPT_CMDMODE_T(0, "abort", action, &options.action,
+ N_("abort and check out the original branch"),
+ ACTION_ABORT),
+ OPT_CMDMODE_T(0, "quit", action, &options.action,
+ N_("abort but keep HEAD where it is"),
+ ACTION_QUIT),
+ OPT_CMDMODE_T(0, "edit-todo", action, &options.action,
+ N_("edit the todo list during an interactive rebase"),
+ ACTION_EDIT_TODO),
+ OPT_CMDMODE_T(0, "show-current-patch", action, &options.action,
+ N_("show the patch file being applied or merged"),
+ ACTION_SHOW_CURRENT_PATCH),
OPT_CALLBACK_F(0, "apply", &options, NULL,
N_("use apply strategies to rebase"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG,
diff --git a/builtin/replace.c b/builtin/replace.c
index da59600ad2..09aebc9de2 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -539,27 +539,36 @@ static int convert_graft_file(int force)
return -1;
}
+enum cmdmode {
+ MODE_UNSPECIFIED = 0,
+ MODE_LIST,
+ MODE_DELETE,
+ MODE_EDIT,
+ MODE_GRAFT,
+ MODE_CONVERT_GRAFT_FILE,
+ MODE_REPLACE
+};
+
+DEFINE_OPTION_VALUE_TYPE(cmdmode, enum cmdmode);
int cmd_replace(int argc, const char **argv, const char *prefix)
{
int force = 0;
int raw = 0;
const char *format = NULL;
- enum {
- MODE_UNSPECIFIED = 0,
- MODE_LIST,
- MODE_DELETE,
- MODE_EDIT,
- MODE_GRAFT,
- MODE_CONVERT_GRAFT_FILE,
- MODE_REPLACE
- } cmdmode = MODE_UNSPECIFIED;
+ enum cmdmode cmdmode = MODE_UNSPECIFIED;
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_T('l', "list", cmdmode, &cmdmode,
+ N_("list replace refs"), MODE_LIST),
+ OPT_CMDMODE_T('d', "delete", cmdmode, &cmdmode,
+ N_("delete replace refs"), MODE_DELETE),
+ OPT_CMDMODE_T('e', "edit", cmdmode, &cmdmode,
+ N_("edit existing object"), MODE_EDIT),
+ OPT_CMDMODE_T('g', "graft", cmdmode, &cmdmode,
+ N_("change a commit's parents"), MODE_GRAFT),
+ OPT_CMDMODE_T(0, "convert-graft-file", cmdmode, &cmdmode,
+ 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")),
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..82b9a95a4f 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -29,6 +29,8 @@ enum stripspace_mode {
COMMENT_LINES
};
+DEFINE_OPTION_VALUE_TYPE(mode, enum stripspace_mode);
+
int cmd_stripspace(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
@@ -36,12 +38,12 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
int nongit;
const struct option options[] = {
- OPT_CMDMODE('s', "strip-comments", &mode,
- N_("skip and remove all lines starting with comment character"),
- STRIP_COMMENTS),
- OPT_CMDMODE('c', "comment-lines", &mode,
- N_("prepend comment character and space to each line"),
- COMMENT_LINES),
+ OPT_CMDMODE_T('s', "strip-comments", mode, &mode,
+ N_("skip and remove all lines starting with comment character"),
+ STRIP_COMMENTS),
+ OPT_CMDMODE_T('c', "comment-lines", mode, &mode,
+ N_("prepend comment character and space to each line"),
+ COMMENT_LINES),
OPT_END()
};
diff --git a/parse-options.c b/parse-options.c
index e8e076c3a6..63a2247128 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -85,7 +85,7 @@ static enum parse_opt_result opt_command_mode_error(
if (that == opt ||
!(that->flags & PARSE_OPT_CMDMODE) ||
that->value != opt->value ||
- that->defval != *(int *)opt->value)
+ that->defval != opt->get_value(opt->value))
continue;
if (that->long_name)
@@ -122,7 +122,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
* is not a grave error, so let it pass.
*/
if ((opt->flags & PARSE_OPT_CMDMODE) &&
- *(int *)opt->value && *(int *)opt->value != opt->defval)
+ opt->get_value(opt->value) &&
+ opt->get_value(opt->value) != opt->defval)
return opt_command_mode_error(opt, all_opts, flags);
switch (opt->type) {
@@ -160,6 +161,10 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
*(int *)opt->value = unset ? 0 : opt->defval;
return 0;
+ case OPTION_SET_VALUE:
+ opt->set_value(opt->value, unset ? 0 : opt->defval);
+ return 0;
+
case OPTION_STRING:
if (unset)
*(const char **)opt->value = NULL;
@@ -483,11 +488,21 @@ static void parse_options_check(const struct option *opts)
if (opts->type == OPTION_SET_INT && !opts->defval &&
opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
optbug(opts, "OPTION_SET_INT 0 should not be negatable");
+ if (opts->type == OPTION_SET_VALUE && !opts->defval &&
+ opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
+ optbug(opts, "OPTION_SET_VALUE 0 should not be negatable");
+ if (opts->type == OPTION_SET_VALUE &&
+ (!opts->get_value || !opts->set_value))
+ optbug(opts, "OPTION_SET_VALUE requires accessors");
+ if ((opts->flags & PARSE_OPT_CMDMODE) &&
+ (!opts->get_value || !opts->set_value))
+ optbug(opts, "PARSE_OPT_CMDMODE requires accessors");
switch (opts->type) {
case OPTION_COUNTUP:
case OPTION_BIT:
case OPTION_NEGBIT:
case OPTION_SET_INT:
+ case OPTION_SET_VALUE:
case OPTION_NUMBER:
if ((opts->flags & PARSE_OPT_OPTARG) ||
!(opts->flags & PARSE_OPT_NOARG))
@@ -611,6 +626,7 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,
case OPTION_NEGBIT:
case OPTION_COUNTUP:
case OPTION_SET_INT:
+ case OPTION_SET_VALUE:
has_unset_form = 1;
break;
default:
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..764e7f7896 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -20,6 +20,7 @@ enum parse_opt_type {
OPTION_BITOP,
OPTION_COUNTUP,
OPTION_SET_INT,
+ OPTION_SET_VALUE,
/* options with arguments (usually) */
OPTION_STRING,
OPTION_INTEGER,
@@ -158,8 +159,34 @@ struct option {
parse_opt_ll_cb *ll_callback;
intptr_t extra;
parse_opt_subcommand_fn *subcommand_fn;
+ intptr_t (*get_value)(void *);
+ void (*set_value)(void *, intptr_t);
};
+#define DEFINE_OPTION_VALUE_TYPE(type_name, type) \
+static inline intptr_t type_name##__get(void *void_ptr) \
+{ \
+ type *ptr = void_ptr; \
+ return (intptr_t)*ptr; \
+} \
+static inline void type_name##__set(void *void_ptr, intptr_t value) \
+{ \
+ type *ptr = void_ptr; \
+ *ptr = (type)value; \
+} \
+static inline void *type_name##__check(type *ptr) \
+{ \
+ return ptr; \
+} \
+static inline void *type_name##__check(type *ptr)
+
+DEFINE_OPTION_VALUE_TYPE(int, int);
+
+#define OPTION_VALUE(type_name, v) \
+ .get_value = type_name##__get, \
+ .set_value = type_name##__set, \
+ .value = (1 ? (v) : type_name##__check(v))
+
#define OPT_BIT_F(s, l, v, h, b, f) { \
.type = OPTION_BIT, \
.short_name = (s), \
@@ -256,15 +283,18 @@ struct option {
.flags = PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, \
.defval = 1, \
}
-#define OPT_CMDMODE_F(s, l, v, h, i, f) { \
- .type = OPTION_SET_INT, \
+
+#define OPT_CMDMODE_T_F(s, l, t, v, h, i, f) { \
+ .type = OPTION_SET_VALUE, \
.short_name = (s), \
.long_name = (l), \
- .value = (v), \
+ OPTION_VALUE(t, (v)), \
.help = (h), \
.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
.defval = (i), \
}
+#define OPT_CMDMODE_T(s, l, t, v, h, i) OPT_CMDMODE_T_F(s, l, t, v, h, i, 0)
+#define OPT_CMDMODE_F(s, l, v, h, i, f) OPT_CMDMODE_T_F(s, l, int, v, h, i, f)
#define OPT_CMDMODE(s, l, v, h, i) OPT_CMDMODE_F(s, l, v, h, i, 0)
#define OPT_INTEGER(s, l, v, h) OPT_INTEGER_F(s, l, v, h, 0)
--
2.42.0
^ permalink raw reply related
* [PATCH] parse-options: drop unused parse_opt_ctx_t member
From: René Scharfe @ 2023-10-03 8:55 UTC (permalink / raw)
To: Git List
5c387428f1 (parse-options: don't emit "ambiguous option" for aliases,
2019-04-29) added "updated_options" to struct parse_opt_ctx_t, but it
has never been used. Remove it.
---
parse-options.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..4a66ec3bf5 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -459,7 +459,6 @@ struct parse_opt_ctx_t {
unsigned has_subcommands;
const char *prefix;
const char **alias_groups; /* must be in groups of 3 elements! */
- struct option *updated_options;
};
void parse_options_start(struct parse_opt_ctx_t *ctx,
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Oswald Buddenhagen @ 2023-10-03 9:38 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <d9defed8-4e7e-4b84-be3d-57155d973320@web.de>
On Tue, Oct 03, 2023 at 10:49:12AM +0200, René Scharfe wrote:
>Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:
>> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
>>> If we base it on type size then we're making assumptions that I
>>> find hard to justify.
>>>
>> the only one i can think of is signedness. i think this can be safely
>> ignored as long as we use only small positive integers.
>
>I don't fully understand the pointer-sign warning, so I'm not
>confident enough to silence it.
>
in theory, differently signed integers may have completely different
binary representations. but afaik, that only ever mattered for negative
numbers. and c++20 actually codifies two's complement, which was the
de-facto standard for decades already.
so in practice it just means that we may be assigning a value that is
outside the range of the actual type. but small positive values are
compatible between signed and unsiged types.
regards
^ permalink raw reply
* [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Isoken Ibizugbe @ 2023-10-03 10:19 UTC (permalink / raw)
To: git
Dear Git Community,
I hope this email finds you well. My name is Isoken Ibizugbe, and I am
writing to express my strong interest in joining the Git community as
a contributor to the FOSS project. I recently came across the project
description regarding "Moving existing tests to a unit testing
framework" and was particularly intrigued by the opportunity to be
part of this exciting endeavor.
As an aspiring software engineer, I have always admired the incredible
work done by the Git community in developing and maintaining this
widely-used version control system. The project's commitment to
fostering collaboration and innovation aligns perfectly with my values
and aspirations as a developer.
I understand that Christian Couder is the mentor for this project, and
I would be honored to have the opportunity to work under his guidance
and expertise. I would greatly appreciate any advice or direction he
can provide to help me get started on this journey.
I am eager to learn, collaborate with the community, and contribute
meaningfully to this project. Please let me know how I can formally
start my journey as a Git contributor and if there are any specific
guidelines or resources that you recommend for newcomers, as it was a
bit confusing process for me to join this mailing list.
Once again, thank you for your time and for providing an opportunity
for individuals like me to contribute to this remarkable project. I am
enthusiastic about the potential of this project and the journey
ahead.
Best regards,
Isoken
^ permalink raw reply
* [Outreachy] Move existing tests to a unit testing framework
From: Luma @ 2023-10-03 14:30 UTC (permalink / raw)
To: git
Hi;
My name is Luma, and I wanted to take a moment to introduce myself
and share some
insights on an essential aspect of avoiding pipes in git related
commands in test scripts.
I am an outreachy applicant for the December 2023 cohort and look
forward to learning from you.
One common practice in shell scripting is the use of pipes (|) to
chain multiple commands together.
While pipes are incredibly versatile and useful, excessive use of
them can lead to script complexity.
I plan to avoid overusing pipes in test scripts by: leveraging command
options, using temporary files
and using functions and variables to break down complex pipelines.
If you have any questions on pipes, I'm always here to learn and share
knowledge.
Best regards,
Luma.
^ permalink raw reply
* Re: batch-command wishlist [was: [TOPIC 02/12] Libification Goals and Progress]
From: Junio C Hamano @ 2023-10-03 16:15 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Taylor Blau
In-Reply-To: <20231003005251.M353509@dcvr>
Eric Wong <e@80x24.org> writes:
> Taylor Blau <me@ttaylorr.com> wrote:
>> * (Jonathan Tan) back to process isolation: is the short lifetime of the process
>> important?
>> * (Taylor Blau) seems like an impossible goal to be able to do multi-command
>> executions in a single process, the code is just not designed for it.
>
> -- Split out from https://lore.kernel.org/git/ZRrfN2lbg14IOLiK@nand.local/
> Thanks for posting in an accessible format for non-JS/video users.
>
>> * (Junio) is anybody using the `git cat-file --batch-command` mode that switches
>> between batch and batch-check.
This is not exactly what I asked about---I was asking about the use
of the "a long living process serves many requests" pattern ;-)
> But it would be nice if --batch-command grew more functionality:
>
> * ability to add/remove alternates
> * ability to specify a preferred alternate for a lookup[1]
> * detect unlinked packs/removed repos
To the third you would also want to notice an updated index, too.
> Not sure if cat-file is the place for it, but a persistent
> process to deal with:
>
> * `git config -f FILENAME ...' (especially --get-urlmatch --type=FOO)
> * approxidate parsing for other tools[2]
>
> Would also be nice...
"git daemon" ;-)?
^ permalink raw reply
* [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Taylor Blau @ 2023-10-03 16:27 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Patrick Steinhardt
When doing a `--geometric` repack, we make sure that the preferred pack
(if writing a MIDX) is the largest pack that we *didn't* repack. That
has the effect of keeping the preferred pack in sync with the pack
containing a majority of the repository's reachable objects.
But if the repository happens to double in size, we'll repack
everything. Here we don't specify any `--preferred-pack`, and instead
let the MIDX code choose.
In the past, that worked fine, since there would only be one pack to
choose from: the one we just wrote. But it's no longer necessarily the
case that there is one pack to choose from. It's possible that the
repository also has a cruft pack, too.
If the cruft pack happens to come earlier in lexical order (and has an
earlier mtime than any non-cruft pack), we'll pick that pack as
preferred. This makes it impossible to reuse chunks of the reachable
pack verbatim from pack-objects, so is sub-optimal.
Luckily, this is a somewhat rare circumstance to be in, since we would
have to repack the entire repository during a `--geometric` repack, and
the cruft pack would have to sort ahead of the pack we just created.
Note that this behavior is usually just a performance regression. But
it's possible it could be a correctness issue.
Suppose an object was duplicated among the cruft and non-cruft pack. The
MIDX will pick the one from the pack with the lowest mtime, which will
always be the cruft one. But if the non-cruft pack happens to sort
earlier in lexical order, we'll treat that one as preferred, but not all
duplicates will be resolved in favor of that pack.
So if we happened to have an object which appears in both packs
(e.g., due to a cruft object being freshened, causing it to appear
loose, and then repacking it via the `--geometric` repack) it's possible
the duplicate would be picked from the non-preferred pack.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
I've had this sitting in my patch queue for a while now. It's a
non-critical performance fix that avoids the repack/MIDX machinery from
ever choosing a cruft pack as preferred when writing a MIDX bitmap
without a given --preferred-pack.
There is no correctness issue here, but choosing a pack with few/no
reachable objects means that our pack reuse mechanism will rarely kick
in, resulting in performance degradation.
builtin/repack.c | 47 ++++++++++++++++++++++++++++++++++++++++-
t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 04770b15fe..a1a893d952 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -355,6 +355,18 @@ static struct generated_pack_data *populate_pack_exts(const char *name)
return data;
}
+static int has_pack_ext(const struct generated_pack_data *data,
+ const char *ext)
+{
+ int i;
+ for (i = 0; i < ARRAY_SIZE(exts); i++) {
+ if (strcmp(exts[i].name, ext))
+ continue;
+ return !!data->tempfiles[i];
+ }
+ BUG("unknown pack extension: '%s'", ext);
+}
+
static void repack_promisor_objects(const struct pack_objects_args *args,
struct string_list *names)
{
@@ -772,6 +784,7 @@ static void midx_included_packs(struct string_list *include,
static int write_midx_included_packs(struct string_list *include,
struct pack_geometry *geometry,
+ struct string_list *names,
const char *refs_snapshot,
int show_progress, int write_bitmaps)
{
@@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,
if (preferred)
strvec_pushf(&cmd.args, "--preferred-pack=%s",
pack_basename(preferred));
+ else if (names->nr) {
+ /* The largest pack was repacked, meaning that either
+ * one or two packs exist depending on whether the
+ * repository has a cruft pack or not.
+ *
+ * Select the non-cruft one as preferred to encourage
+ * pack-reuse among packs containing reachable objects
+ * over unreachable ones.
+ *
+ * (Note we could write multiple packs here if
+ * `--max-pack-size` was given, but any one of them
+ * will suffice, so pick the first one.)
+ */
+ for_each_string_list_item(item, names) {
+ struct generated_pack_data *data = item->util;
+ if (has_pack_ext(data, ".mtimes"))
+ continue;
+
+ strvec_pushf(&cmd.args, "--preferred-pack=pack-%s.pack",
+ item->string);
+ break;
+ }
+ } else {
+ /*
+ * No packs were kept, and no packs were written. The
+ * only thing remaining are .keep packs (unless
+ * --pack-kept-objects was given).
+ *
+ * Set the `--preferred-pack` arbitrarily here.
+ */
+ ;
+ }
if (refs_snapshot)
strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
@@ -1360,7 +1405,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct string_list include = STRING_LIST_INIT_NODUP;
midx_included_packs(&include, &existing, &names, &geometry);
- ret = write_midx_included_packs(&include, &geometry,
+ ret = write_midx_included_packs(&include, &geometry, &names,
refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
show_progress, write_bitmaps > 0);
diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh
index dc86ca8269..be3735dff0 100755
--- a/t/t7704-repack-cruft.sh
+++ b/t/t7704-repack-cruft.sh
@@ -372,4 +372,43 @@ test_expect_success '--max-cruft-size ignores non-local packs' '
)
'
+test_expect_success 'reachable packs are preferred over cruft ones' '
+ repo="cruft-preferred-packs" &&
+ git init "$repo" &&
+ (
+ cd "$repo" &&
+
+ # This test needs to exercise careful control over when a MIDX
+ # is and is not written. Unset the corresponding TEST variable
+ # accordingly.
+ sane_unset GIT_TEST_MULTI_PACK_INDEX &&
+
+ test_commit base &&
+ test_commit --no-tag cruft &&
+
+ non_cruft="$(echo base | git pack-objects --revs $packdir/pack)" &&
+ # Write a cruft pack which both (a) sorts ahead of the non-cruft
+ # pack in lexical order, and (b) has an older mtime to appease
+ # the MIDX preferred pack selection routine.
+ cruft="$(echo pack-$non_cruft.pack | git pack-objects --cruft $packdir/pack-A)" &&
+ test-tool chmtime -1000 $packdir/pack-A-$cruft.pack &&
+
+ test_commit other &&
+ git repack -d &&
+
+ git repack --geometric 2 -d --write-midx --write-bitmap-index &&
+
+ # After repacking, there are two packs left: one reachable one
+ # (which is the result of combining both of the existing two
+ # non-cruft packs), and one cruft pack.
+ find .git/objects/pack -type f -name "*.pack" >packs &&
+ test_line_count = 2 packs &&
+
+ # Make sure that the pack we just wrote is marked as preferred,
+ # not the cruft one.
+ pack="$(test-tool read-midx --preferred-pack $objdir)" &&
+ test_path_is_missing "$packdir/$(basename "$pack" ".idx").mtimes"
+ )
+'
+
test_done
--
2.42.0.311.ga7f7a0e966.dirty
^ permalink raw reply related
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Taylor Blau @ 2023-10-03 16:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com>
On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
> I've had this sitting in my patch queue for a while now. It's a
> non-critical performance fix that avoids the repack/MIDX machinery from
> ever choosing a cruft pack as preferred when writing a MIDX bitmap
> without a given --preferred-pack.
>
> There is no correctness issue here, but choosing a pack with few/no
> reachable objects means that our pack reuse mechanism will rarely kick
> in, resulting in performance degradation.
>
> builtin/repack.c | 47 ++++++++++++++++++++++++++++++++++++++++-
> t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++
> 2 files changed, 85 insertions(+), 1 deletion(-)
Oops, I should have mentioned that this is meant to be applied on top of
'tb/multi-cruft-pack' to reduce the conflict resolution burden. Sorry
about that.
Thanks,
Taylor
^ permalink raw reply
* [PATCH v2] git-status.txt: fix minor asciidoc format issue
From: cousteau via GitGitGadget @ 2023-10-03 16:33 UTC (permalink / raw)
To: git; +Cc: Javier Mora, cousteau, Javier Mora
In-Reply-To: <pull.1591.git.1696193527923.gitgitgadget@gmail.com>
From: Javier Mora <cousteaulecommandant@gmail.com>
The paragraph below the list of short option combinations
isn't correctly formatted, making the result hard to read.
Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
---
git-status.txt: minor asciidoc format correction
The paragraph below the list of short option combinations was hard to
read; turns out it wasn't correctly formatted in asciidoc.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1591%2Fcousteaulecommandant%2Fman-git-status-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1591/cousteaulecommandant/man-git-status-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1591
Range-diff vs v1:
1: b3c97ca9e0f ! 1: 811885a275f git-status.txt: fix minor asciidoc format issue
@@ Commit message
## Documentation/git-status.txt ##
@@ Documentation/git-status.txt: U U unmerged, both modified
- ....
Submodules have more state and instead report
+
- M the submodule has a different HEAD than
- recorded in the index
- m the submodule has modified content
- ? the submodule has untracked files
-+
+* 'M' = the submodule has a different HEAD than recorded in the index
+* 'm' = the submodule has modified content
+* '?' = the submodule has untracked files
-+
+
since modified content or untracked files in a submodule cannot be added
via `git add` in the superproject to prepare a commit.
-
Documentation/git-status.txt | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index b27d127b5e2..48f46eb2047 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -246,10 +246,9 @@ U U unmerged, both modified
Submodules have more state and instead report
- M the submodule has a different HEAD than
- recorded in the index
- m the submodule has modified content
- ? the submodule has untracked files
+* 'M' = the submodule has a different HEAD than recorded in the index
+* 'm' = the submodule has modified content
+* '?' = the submodule has untracked files
since modified content or untracked files in a submodule cannot be added
via `git add` in the superproject to prepare a commit.
base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
--
gitgitgadget
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Junio C Hamano @ 2023-10-03 16:33 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <871qecgpg1.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> I believe I've addressed this in details in my reply here:
> <87o7hok8dx.fsf@osv.gnss.ru>, and got no further objections from you
> since then, so I figure I'd ask to finally let the patch in.
You need to know that no response does not mean no objection. You
repeated why the less useful combination is what you want, but that
does not mean the combination deserves to squat on short-and-sweet
'd' and prevent others from coming up with a better use for it.
^ permalink raw reply
* Re: Is git-p4 maintained?
From: Junio C Hamano @ 2023-10-03 16:42 UTC (permalink / raw)
To: Yuri; +Cc: Git Mailing List
In-Reply-To: <2f9081b4-e34f-38b3-a557-021c54e4384c@tsoft.com>
Yuri <yuri@rawbw.com> writes:
> Is git-p4 maintained?
> Is there any chance for these problems to be fixed?
The last time anybody touched git-p4 was July last year, but
there is no dedicated area maintainer active even back then, as far
as I know (if not, please raise your hand).
But there may be folks willing to help. When I saw the patch[*] in
July last year, I was surprised to see it almost immediately got a
review to support it. Your post may attract attention from others
who are familiar with "git p4" and/or the problem you are describing
and you folks may be able to join forces to improve it.
Thanks.
[References]
* https://lore.kernel.org/git/pull.1285.git.git.1657267260405.gitgitgadget@gmail.com/
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Junio C Hamano @ 2023-10-03 17:15 UTC (permalink / raw)
To: René Scharfe; +Cc: phillip.wood, Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <6cb09270-04b9-456e-8d7e-97137e56e9e2@web.de>
René Scharfe <l.s.r@web.de> writes:
> +DEFINE_OPTION_VALUE_TYPE(resume_type, enum resume_type);
These are a bit annoying, but because we need a token that can be ## pasted
to form a valid identifier, we cannot help it.
> diff --git a/parse-options.c b/parse-options.c
> index e8e076c3a6..63a2247128 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -85,7 +85,7 @@ static enum parse_opt_result opt_command_mode_error(
> if (that == opt ||
> !(that->flags & PARSE_OPT_CMDMODE) ||
> that->value != opt->value ||
> - that->defval != *(int *)opt->value)
> + that->defval != opt->get_value(opt->value))
> continue;
So, instead of assuming the pointer stuffed in opt->value member can
be dereferenced as inteter pointer, we have the get_value method for
the option and invoke it to grab the value, and compare it with the
default value.
> @@ -122,7 +122,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
> * is not a grave error, so let it pass.
> */
> if ((opt->flags & PARSE_OPT_CMDMODE) &&
> - *(int *)opt->value && *(int *)opt->value != opt->defval)
> + opt->get_value(opt->value) &&
> + opt->get_value(opt->value) != opt->defval)
> return opt_command_mode_error(opt, all_opts, flags);
Likewise.
> @@ -160,6 +161,10 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
> *(int *)opt->value = unset ? 0 : opt->defval;
> return 0;
>
> + case OPTION_SET_VALUE:
> + opt->set_value(opt->value, unset ? 0 : opt->defval);
> + return 0;
Here we see the previous way in the precontext of this hunk that is
used for OPTION_SET_INT, but in the new type-safe-enum world order,
that uses OPTION_SET_VALUE, the set_value method should know what to
do with the pointer that is in opt->value.
> diff --git a/parse-options.h b/parse-options.h
> index 57a7fe9d91..764e7f7896 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -20,6 +20,7 @@ enum parse_opt_type {
> OPTION_BITOP,
> OPTION_COUNTUP,
> OPTION_SET_INT,
> + OPTION_SET_VALUE,
> /* options with arguments (usually) */
> OPTION_STRING,
> OPTION_INTEGER,
> @@ -158,8 +159,34 @@ struct option {
> parse_opt_ll_cb *ll_callback;
> intptr_t extra;
> parse_opt_subcommand_fn *subcommand_fn;
> + intptr_t (*get_value)(void *);
> + void (*set_value)(void *, intptr_t);
> };
OK.
> +#define DEFINE_OPTION_VALUE_TYPE(type_name, type) \
> +static inline intptr_t type_name##__get(void *void_ptr) \
> +{ \
> + type *ptr = void_ptr; \
> + return (intptr_t)*ptr; \
> +} \
> +static inline void type_name##__set(void *void_ptr, intptr_t value) \
> +{ \
> + type *ptr = void_ptr; \
> + *ptr = (type)value; \
> +} \
> +static inline void *type_name##__check(type *ptr) \
> +{ \
> + return ptr; \
> +} \
> +static inline void *type_name##__check(type *ptr)
Fun. So a typical pattern is that for "enum foo", the foo__get() is
created from the above template and becomes the .get_value method.
Copying from an earlier hunk, the get_value() method is used like
so:
> - that->defval != *(int *)opt->value)
> + that->defval != opt->get_value(opt->value))
We pass opt->value (which is void *) to foo__get(), we have a local
variable "enum foo *ptr" and assign it in there, and dereference it.
We used to dereference the pointer as if it were a pointer to an
integer, so the type of foo__get() could be "int", but because we
compare it with the .defval member, which is of type "intptr_t", the
return type of the get_value() method being "intptr_t" would make it
consistent here. I am not sure why defval need to be "intptr_t", and
for the purpose of this topic it would have been cleaner if it were
"int", but that is a tangent (probably somebody uses it as the default
value for a pointer variable and points it at some default object).
The setter is also reasonable. An earlier hunk used it like so:
> + opt->set_value(opt->value, unset ? 0 : opt->defval);
opt->value which is (void *) is assigned to "enum foo *ptr", and
using that pointer, "(enum foo)opt->defval" (or 0) is assinged
there. Pretty straight-forward.
> +DEFINE_OPTION_VALUE_TYPE(int, int);
> +
> +#define OPTION_VALUE(type_name, v) \
> + .get_value = type_name##__get, \
> + .set_value = type_name##__set, \
> + .value = (1 ? (v) : type_name##__check(v))
This is cute. foo__check() is declared to take "enum foo *" and
returns it as "void *", but because the condition to the ternary
operator is constant "true", it is discarded. The only expected
effect is to force the compiler to catch type errors when v is not
of type "enum foo *".
Unless it is "void *", I presume? Then foo__check() would be happy,
but typically OPTION_VALUE() is used as an implementation detail of
OPT_CMDMODE_T() and you are expected to say something like "&variable"
for "v" above, so it would be OK (because you cannot have a variable
of type "void").
Thanks for a fun read.
^ permalink raw reply
* Re: [PATCH] parse-options: drop unused parse_opt_ctx_t member
From: Eric Sunshine @ 2023-10-03 17:38 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <ebcaa9e1-d306-4c93-adec-3f35d7040531@web.de>
On Tue, Oct 3, 2023 at 4:55 AM René Scharfe <l.s.r@web.de> wrote:
> 5c387428f1 (parse-options: don't emit "ambiguous option" for aliases,
> 2019-04-29) added "updated_options" to struct parse_opt_ctx_t, but it
> has never been used. Remove it.
> ---
Missing sign-off.
^ permalink raw reply
* [PATCH 0/1] *** Avoid using Pipes ***
From: ach.lumap @ 2023-10-03 17:48 UTC (permalink / raw)
To: git; +Cc: christian.couder, achluma
From: achluma <achluma@gmail.com>
*** BLURB HERE ***
achluma (1):
t2400: avoid using pipes
t/t2400-worktree-add.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
--
2.41.0.windows.1
^ permalink raw reply
* [PATCH 1/1] t2400: avoid using pipes
From: ach.lumap @ 2023-10-03 17:48 UTC (permalink / raw)
To: git; +Cc: christian.couder, achluma
In-Reply-To: <20231003174853.1732-1-ach.lumap@gmail.com>
From: achluma <ach.lumap@gmail.com>
The exit code of the preceding command in a pipe is disregarded,
so it's advisable to refrain from relying on it. Instead, by
saving the output of a Git command to a file, we gain the
ability to examine the exit codes of both commands separately.
Signed-off-by: achluma <ach.lumap@gmail.com>
---
t/t2400-worktree-add.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index df4aff7825..7ead05bb98 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '
cd under-rebase &&
set_fake_editor &&
FAKE_LINES="edit 1" git rebase -i HEAD^ &&
- git worktree list | grep "under-rebase.*detached HEAD"
+ git worktree list >actual &&
+ grep "under-rebase.*detached HEAD" actual
)
'
@@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '
git bisect start &&
git bisect bad &&
git bisect good HEAD~2 &&
- git worktree list | grep "under-bisect.*detached HEAD" &&
+ git worktree list >actual &&
+ grep "under-bisect.*detached HEAD" actual &&
test_must_fail git worktree add new-bisect under-bisect &&
! test -d new-bisect
)
--
2.41.0.windows.1
^ permalink raw reply related
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-10-03 17:54 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <ZRvhEWHWn4nDynD0@ugly>
Am 03.10.23 um 11:38 schrieb Oswald Buddenhagen:
> On Tue, Oct 03, 2023 at 10:49:12AM +0200, René Scharfe wrote:
>> Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:
>>> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
>>>> If we base it on type size then we're making assumptions that
>>>> I find hard to justify.
>>>>
>>> the only one i can think of is signedness. i think this can be
>>> safely ignored as long as we use only small positive integers.
>>
>> I don't fully understand the pointer-sign warning, so I'm not
>> confident enough to silence it.
>>
> in theory, differently signed integers may have completely different
> binary representations. but afaik, that only ever mattered for
> negative numbers. and c++20 actually codifies two's complement, which
> was the de-facto standard for decades already. so in practice it just
> means that we may be assigning a value that is outside the range of
> the actual type. but small positive values are compatible between
> signed and unsiged types.
C++ is not relevant for Git, but C23 is going to to stop supporting
binary representations other than two's complement as well.
Still I don't feel comfortable overriding compiler warnings for
something pedestrian as a command line parser. No idea what other
assumptions are made in compilers around enums. I'd rather honor
the warnings and avoid any forcing or trickery if possible. Or at
least leave that to more capable hands.
René
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Sergey Organov @ 2023-10-03 17:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34yr3btn.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> I believe I've addressed this in details in my reply here:
>> <87o7hok8dx.fsf@osv.gnss.ru>, and got no further objections from you
>> since then, so I figure I'd ask to finally let the patch in.
>
> You need to know that no response does not mean no objection. You
> repeated why the less useful combination is what you want, but that
> does not mean the combination deserves to squat on short-and-sweet
> 'd' and prevent others from coming up with a better use for it.
Yep, but I've asked what's better use for -d than "get me diff"? Do you
really have an idea?
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH] parse-options: drop unused parse_opt_ctx_t member
From: René Scharfe @ 2023-10-03 18:00 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List
In-Reply-To: <CAPig+cRZ_KzyjWjm-G2qn+t-QA_=CL-tMvTSyZBKrmiHK3RQrg@mail.gmail.com>
Am 03.10.23 um 19:38 schrieb Eric Sunshine:
> On Tue, Oct 3, 2023 at 4:55 AM René Scharfe <l.s.r@web.de> wrote:
>> 5c387428f1 (parse-options: don't emit "ambiguous option" for aliases,
>> 2019-04-29) added "updated_options" to struct parse_opt_ctx_t, but it
>> has never been used. Remove it.
>> ---
>
> Missing sign-off.
Oops, thanks for catching that. Technically not necessary, I guess,
since the patch is trivial, but here it is:
Signed-off-by: René Scharfe <l.s.r@web.de>
^ permalink raw reply
* Re: [PATCH 1/1] t2400: avoid using pipes
From: Eric Sunshine @ 2023-10-03 18:01 UTC (permalink / raw)
To: ach.lumap; +Cc: git, christian.couder
In-Reply-To: <20231003174853.1732-2-ach.lumap@gmail.com>
On Tue, Oct 3, 2023 at 1:49 PM <ach.lumap@gmail.com> wrote:
> t2400: avoid using pipes
Pipes themselves are not necessarily problematic, and there are many
places in the test suite where they are legitimately used. Rather...
> The exit code of the preceding command in a pipe is disregarded,
> so it's advisable to refrain from relying on it. Instead, by
> saving the output of a Git command to a file, we gain the
> ability to examine the exit codes of both commands separately.
... as you correctly explain here, we don't want to lose the exit code
from the Git command. Thus, if you want to convey more information to
readers of `git log --oneline` (or other such commands), a better
subject for the patch might be:
t2400: avoid losing Git exit code
That minor comment aside (which is probably not worth a reroll), the
commit message properly explains why this change is desirable and the
patch itself looks good.
> Signed-off-by: achluma <ach.lumap@gmail.com>
> ---
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '
> cd under-rebase &&
> set_fake_editor &&
> FAKE_LINES="edit 1" git rebase -i HEAD^ &&
> - git worktree list | grep "under-rebase.*detached HEAD"
> + git worktree list >actual &&
Thanks for following the style guideline and omitting whitespace
between the redirection operator and the destination file.
> + grep "under-rebase.*detached HEAD" actual
> )
> '
>
> @@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '
> git bisect start &&
> git bisect bad &&
> git bisect good HEAD~2 &&
> - git worktree list | grep "under-bisect.*detached HEAD" &&
> + git worktree list >actual &&
> + grep "under-bisect.*detached HEAD" actual &&
> test_must_fail git worktree add new-bisect under-bisect &&
> ! test -d new-bisect
> )
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Oswald Buddenhagen @ 2023-10-03 18:24 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <88cb2db8-e5cb-470a-8060-7a1b898c91f9@web.de>
On Tue, Oct 03, 2023 at 07:54:28PM +0200, René Scharfe wrote:
>C++ is not relevant for Git, but C23 is going to to stop supporting
>binary representations other than two's complement as well.
>
it is relevant insofar as that every platform that comes with a recent
c++ compiler uses two's complement. this then applies to any language,
regardless of standard.
>Still I don't feel comfortable overriding compiler warnings for
>something pedestrian as a command line parser. No idea what other
>assumptions are made in compilers around enums.
>
i'm not sure what you're really worried about. if there was an actual
problem, we'd have noticed by now. as far as i'm concerned, the question
is only how to codify the status quo in the most elegant way. putting a
typecast into a macro certainly qualifies in my book.
regards
^ permalink raw reply
* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Eric Sunshine @ 2023-10-03 18:30 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003082107.3002173-1-stepnem@smrk.net>
On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
> Fix some typos, grammar or wording issues in the documentation
SubmittingPatches suggests: s/Fix/fix
Overall, these changes are welcome improvements. I've left a few minor
comments below which may or may not be actionable.
> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
> ---
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> @@ -393,8 +393,8 @@ mailing list{security-ml}, instead of the public mailing list.
> Learn to use format-patch and send-email if possible. These commands
> are optimized for the workflow of sending patches, avoiding many ways
> -your existing e-mail client that is optimized for "multipart/*" mime
> -type e-mails to corrupt and render your patches unusable.
> +your existing e-mail client (often optimized for "multipart/*" MIME
> +type e-mails) might render your patches unusable.
A subjective alternative would have been to use commas to set off the
newly-parenthesized comment. Not worth a reroll.
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> @@ -96,7 +96,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config
> This is useful for cases where you want to pass transitory
> -configuration options to git, but are doing so on OS's where
> +configuration options to git, but are doing so on OSes where
Since you're touching this anyhow, it could probably be made clearer
by simply spelling it out: "operating systems"
> other processes might be able to read your cmdline
> (e.g. `/proc/self/cmdline`), but not your environ
> (e.g. `/proc/self/environ`). That behavior is the default on
I wonder if "cmdline" and "environ" would be better spelled out, as
well, since the examples which immediately follow them give the
necessary context.
other processes might be able to read your command-line
(e.g. `/proc/self/cmdline`), but not your environment
(e.g. `/proc/self/environ`).
But perhaps that's outside the scope of this patch?
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> @@ -1151,7 +1151,7 @@ will be stored via placeholder `%P`.
> This attribute controls the length of conflict markers left in
> -the work tree file during a conflicted merge. Only setting to
> +the work tree file during a conflicted merge. Only setting
> the value to a positive integer has any meaningful effect.
Or, more simply:
Only a positive integer has a meaningful effect.
> diff --git a/contrib/README b/contrib/README
> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
> I expect that things that start their life in the contrib/ area
> -to graduate out of contrib/ once they mature, either by becoming
> +graduate out of contrib/ once they mature, either by becoming
You probably want to add a comma after "area".
Do we want to correct the formatting of this pathname:
...in the `contrib/` area...
...out of `contrib/` once...
or is that outside the scope of this patch?
> diff --git a/strbuf.h b/strbuf.h
> @@ -12,9 +12,9 @@
> - * strbuf's are meant to be used with all the usual C string and memory
> + * strbufs are meant to be used with all the usual C string and memory
Alternatively:
strbuf is meant to be used...
> * APIs. Given that the length of the buffer is known, it's often better to
> - * use the mem* functions than a str* one (memchr vs. strchr e.g.).
> + * use the mem* functions than a str* one (e.g., memchr vs. strchr).
> * Though, one has to be careful about the fact that str* functions often
> * stop on NULs and that strbufs may have embedded NULs.
Similar:
... and that a strbuf may have...
> diff --git a/t/README b/t/README
> @@ -262,8 +262,8 @@ The argument for --run, <test-selector>, is a list of description
> suite to include (or exclude, if negated) in the run. A range is two
> -numbers separated with a dash and matches a range of tests with both
> -ends been included. You may omit the first or the second number to
> +numbers separated with a dash and matches an inclusive range of tests
> +to run. You may omit the first or the second number to
Not the fault of this patch, but "matches" seems an odd choice.
Perhaps "specifies" would feel more natural.
> The argument to --run is split on commas into separate strings,
> @@ -579,11 +579,10 @@ This test harness library does the following things:
> -Here are some recommented styles when writing test case.
Do you want to fix the spelling error while you're here or is that
done in a later patch?
s/recommented/recommended/
> - - Keep test title the same line with test helper function itself.
> + - Keep test titles and helper function invocations on the same line.
This would be clearer if it was switched around. Either:
Keep the test_expect_* function call and test title on the same line.
or, more verbosely:
Place the test title on the same line as the test_expect_* call
which precedes it.
^ permalink raw reply
* Re: [PATCH 3/5] git-jump: admit to passing merge mode args to ls-files
From: Eric Sunshine @ 2023-10-03 18:33 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003082107.3002173-3-stepnem@smrk.net>
On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
> There's even an example of such usage in the README.
>
> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
> ---
> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> @@ -9,7 +9,7 @@ The <mode> parameter is one of:
>
> diff: elements are diff hunks. Arguments are given to diff.
>
> -merge: elements are merge conflicts. Arguments are ignored.
> +merge: elements are merge conflicts. Arguments are given to ls-files -u.
Should "ls-files -u" be formatted with backticks?
Arguments are passed to `git ls-files -u`.
^ permalink raw reply
* Re: [PATCH 1/1] t2400: avoid using pipes
From: Junio C Hamano @ 2023-10-03 18:42 UTC (permalink / raw)
To: Eric Sunshine; +Cc: ach.lumap, git, christian.couder
In-Reply-To: <CAPig+cSkZ_brRh_ijFRgz3sP9ou5se9-xeRg=C+cV3c3-v3Wtg@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Tue, Oct 3, 2023 at 1:49 PM <ach.lumap@gmail.com> wrote:
>> t2400: avoid using pipes
>
> Pipes themselves are not necessarily problematic, and there are many
> places in the test suite where they are legitimately used. Rather...
> ...
> readers of `git log --oneline` (or other such commands), a better
> subject for the patch might be:
>
> t2400: avoid losing Git exit code
>
> That minor comment aside (which is probably not worth a reroll), the
> commit message properly explains why this change is desirable and the
> patch itself looks good.
Thanks for writing and reviewing. Will queue.
^ permalink raw reply
* [PATCH 2/6] submodule--helper: return error from set-url when modifying failed
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
set-branch will return an error when setting the config fails so I don't
see why set-url shouldn't. Also skip the sync in this case.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
builtin/submodule--helper.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f376466a5e..e2175083a6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2890,39 +2890,41 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
static int module_set_url(int argc, const char **argv, const char *prefix)
{
- int quiet = 0;
+ int quiet = 0, ret;
const char *newurl;
const char *path;
char *config_name;
struct option options[] = {
OPT__QUIET(&quiet, N_("suppress output for setting url of a submodule")),
OPT_END()
};
const char *const usage[] = {
N_("git submodule set-url [--quiet] <path> <newurl>"),
NULL
};
const struct submodule *sub;
argc = parse_options(argc, argv, prefix, options, usage, 0);
if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
usage_with_options(usage, options);
sub = submodule_from_path(the_repository, null_oid(), path);
if (!sub)
die(_("no submodule mapping found in .gitmodules for path '%s'"),
path);
config_name = xstrfmt("submodule.%s.url", sub->name);
- config_set_in_gitmodules_file_gently(config_name, newurl);
+ ret = config_set_in_gitmodules_file_gently(config_name, newurl);
- repo_read_gitmodules (the_repository, 0);
- sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
+ if (!ret) {
+ repo_read_gitmodules(the_repository, 0);
+ sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
+ }
free(config_name);
- return 0;
+ return !!ret;
}
static int module_set_branch(int argc, const char **argv, const char *prefix)
--
2.42.0
^ permalink raw reply related
* [PATCH 5/6] t7419: Test that we correctly handle renamed submodules
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
Add the submodule again with an explicitly different name and path. Test
that calling set-branch modifies the correct .gitmodules entries. Make
sure we don't create a section named after the path instead of the name.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
t/t7419-submodule-set-branch.sh | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 3cd30865a7..a5d1bc5c54 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -38,7 +38,8 @@ test_expect_success 'submodule config cache setup' '
(cd super &&
git init &&
git submodule add ../submodule &&
- git commit -m "add submodule"
+ git submodule add --name thename ../submodule thepath &&
+ git commit -m "add submodules"
)
'
@@ -100,4 +101,31 @@ test_expect_success 'test submodule set-branch -d' '
)
'
+test_expect_success 'test submodule set-branch --branch with named submodule' '
+ (cd super &&
+ git submodule set-branch --branch topic thepath &&
+ test_cmp_config topic -f .gitmodules submodule.thename.branch &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.thepath.branch &&
+ git submodule update --remote &&
+ cat <<-\EOF >expect &&
+ b
+ EOF
+ git -C thepath show -s --pretty=%s >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'test submodule set-branch --default with named submodule' '
+ (cd super &&
+ git submodule set-branch --default thepath &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.thename.branch &&
+ git submodule update --remote &&
+ cat <<-\EOF >expect &&
+ a
+ EOF
+ git -C thepath show -s --pretty=%s >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
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