* Re: [PATCH v4] tests: modernize the test script t0010-racy-git.sh
From: Aryan Gupta @ 2024-03-07 14:23 UTC (permalink / raw)
To: Christian Couder
Cc: Aryan Gupta via GitGitGadget, git, Patrick Steinhardt [ ],
Michal Suchánek [ ], Jean-Noël AVILA [ ], Eric Sunshine
In-Reply-To: <CAP8UFD31udQB2e=+G-LpCevuS+JxQdWqwaq=5qvGEn21595faQ@mail.gmail.com>
On Thu, Mar 7, 2024 at 2:28 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Mar 6, 2024 at 10:46 AM Aryan Gupta via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Aryan Gupta <garyan447@gmail.com>
> >
> > Modernize the formatting of the test script to align with current
> > standards and improve its overall readability.
> >
> > Signed-off-by: Aryan Gupta <garyan447@gmail.com>
> > ---
> > [GSOC][PATCH] Modernize a test script
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v4
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v4
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1675
> >
> > Range-diff vs v3:
> >
> > 1: 05ee9e8a458 = 1: 14c7137baea tests: modernize the test script t0010-racy-git.sh
>
> This tells us that nothing changed in the patch since v3, so we can
> only wonder why you sent this v4.
>
> Did you fix some headers? Please explain.
>
Hey. Sorry for making a lot of mistakes in my emails.
I thought that there were some bugs in GGG due to which it sent some
headers which were not syntactically correct. So I tried sending it again.
And that was the whole purpose of this.
> > t/t0010-racy-git.sh | 31 +++++++++++++++----------------
> > 1 file changed, 15 insertions(+), 16 deletions(-)
>
> Otherwise, the patch looks good to me. Thanks.
^ permalink raw reply
* [PATCH v2] config: add --comment option to add a comment
From: Ralph Seichter via GitGitGadget @ 2024-03-07 15:15 UTC (permalink / raw)
To: git; +Cc: rsbecker, Ralph Seichter, Ralph Seichter
In-Reply-To: <pull.1681.git.1709532018372.gitgitgadget@gmail.com>
From: Ralph Seichter <github@seichter.de>
Introduce the ability to append comments to modifications
made using git-config. Example usage:
git config --comment "changed via script" \
--add safe.directory /home/alice/repo.git
based on the proposed patch, the output produced is:
[safe]
directory = /home/alice/repo.git #changed via script
* Motivation:
The use case which triggered my submitting this patch is
my need to distinguish between config entries made using
automation and entries made by a human. Automation can
add comments containing a URL pointing to explanations
for the change made, avoiding questions from users as to
why their config file was changed by a third party.
* Design considerations and implementation details:
The implementation ensures that a # character is always
prepended to the provided comment string, and that the
comment text is appended as a suffix to the changed key-
value-pair in the same line of text. Multiline comments
are deliberately not supported, because their usefulness
does not justifiy the possible problems they pose when
it comes to removing ML comments later.
* Target audience:
Regarding the intended consumers of the comments made:
They are aimed at humans who inspect or change their Git
config using a pager or editor. Comments are not meant
to be read or displayed by git-config at a later time.
Signed-off-by: Ralph Seichter <github@seichter.de>
---
config: add --comment option to add a comment
config: add --comment option to add a comment
Introduce the ability to append comments to modifications made using
git-config. Example usage:
git config --comment "changed via script" \
--add safe.directory /home/alice/repo.git
based on the proposed patch, the output produced is:
[safe]
directory = /home/alice/repo.git #changed via script
* Motivation: The use case which triggered my submitting this patch is
my need to distinguish between config entries made using automation
and entries made by a human. Automation can add comments containing a
URL pointing to explanations for the change made, avoiding questions
from users as to why their config file was changed by a third party.
* Design considerations and implementation details: The implementation
ensures that a # character is always prepended to the provided
comment string, and that the comment text is appended as a suffix to
the changed key- value-pair in the same line of text. Multiline
comments are deliberately not supported, because their usefulness
does not justifiy the possible problems they pose when it comes to
removing ML comments later.
* Target audience: Regarding the intended consumers of the comments
made: They are aimed at humans who inspect or change their Git config
using a pager or editor. Comments are not meant to be read or
displayed by git-config at a later time.
Changes since v1:
* Rewrite commit message to address reviewers' questions.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1681%2Frseichter%2Fissue-1680-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1681/rseichter/issue-1680-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1681
Range-diff vs v1:
1: d07cbb4bbf7 ! 1: 1e6ccc81685 Allow git-config to append a comment
@@ Metadata
Author: Ralph Seichter <github@seichter.de>
## Commit message ##
- Allow git-config to append a comment
+ config: add --comment option to add a comment
Introduce the ability to append comments to modifications
made using git-config. Example usage:
- git config --comment "I changed this. --A. Script" \
- --add safe.directory /home/alice/somerepo.git
+ git config --comment "changed via script" \
+ --add safe.directory /home/alice/repo.git
+
+ based on the proposed patch, the output produced is:
+
+ [safe]
+ directory = /home/alice/repo.git #changed via script
+
+ * Motivation:
+
+ The use case which triggered my submitting this patch is
+ my need to distinguish between config entries made using
+ automation and entries made by a human. Automation can
+ add comments containing a URL pointing to explanations
+ for the change made, avoiding questions from users as to
+ why their config file was changed by a third party.
+
+ * Design considerations and implementation details:
The implementation ensures that a # character is always
- prepended to the provided comment string.
+ prepended to the provided comment string, and that the
+ comment text is appended as a suffix to the changed key-
+ value-pair in the same line of text. Multiline comments
+ are deliberately not supported, because their usefulness
+ does not justifiy the possible problems they pose when
+ it comes to removing ML comments later.
+
+ * Target audience:
+
+ Regarding the intended consumers of the comments made:
+ They are aimed at humans who inspect or change their Git
+ config using a pager or editor. Comments are not meant
+ to be read or displayed by git-config at a later time.
Signed-off-by: Ralph Seichter <github@seichter.de>
Documentation/git-config.txt | 10 +++++++---
builtin/config.c | 21 ++++++++++++++-------
builtin/gc.c | 4 ++--
builtin/submodule--helper.c | 2 +-
builtin/worktree.c | 4 ++--
config.c | 21 +++++++++++++--------
config.h | 4 ++--
sequencer.c | 28 ++++++++++++++--------------
submodule-config.c | 2 +-
submodule.c | 2 +-
t/t1300-config.sh | 9 +++++++--
worktree.c | 4 ++--
12 files changed, 66 insertions(+), 45 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index dff39093b5e..ee8cd251b24 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,9 +9,9 @@ git-config - Get and set repository or global options
SYNOPSIS
--------
[verse]
-'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
-'git config' [<file-option>] [--type=<type>] --add <name> <value>
-'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value>
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>]
'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>]
'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>]
@@ -87,6 +87,10 @@ OPTIONS
values. This is the same as providing '^$' as the `value-pattern`
in `--replace-all`.
+--comment::
+ Append a comment to new or modified lines. A '#' character
+ will be automatically prepended to the value.
+
--get::
Get the value for a given key (optionally filtered by a regex
matching the value). Returns error code 1 if the key was not
diff --git a/builtin/config.c b/builtin/config.c
index b55bfae7d66..2aab3c0baf3 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -44,6 +44,7 @@ static struct config_options config_options;
static int show_origin;
static int show_scope;
static int fixed_value;
+static const char *comment;
#define ACTION_GET (1<<0)
#define ACTION_GET_ALL (1<<1)
@@ -173,6 +174,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")),
OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
+ OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")),
OPT_END(),
};
@@ -797,6 +799,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
usage_builtin_config();
}
+ if (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {
+ error(_("--comment is only applicable to add/set/replace operations"));
+ usage_builtin_config();
+ }
+
/* check usage of --fixed-value */
if (fixed_value) {
int allowed_usage = 0;
@@ -880,7 +887,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1], &default_kvi);
- ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
+ ret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value);
if (ret == CONFIG_NOTHING_SET)
error(_("cannot overwrite multiple values with a single value\n"
" Use a regexp, --add or --replace-all to change %s."), argv[0]);
@@ -891,7 +898,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
value = normalize_value(argv[0], argv[1], &default_kvi);
ret = git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], value, argv[2],
- flags);
+ comment, flags);
}
else if (actions == ACTION_ADD) {
check_write();
@@ -900,7 +907,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
ret = git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], value,
CONFIG_REGEX_NONE,
- flags);
+ comment, flags);
}
else if (actions == ACTION_REPLACE_ALL) {
check_write();
@@ -908,7 +915,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
value = normalize_value(argv[0], argv[1], &default_kvi);
ret = git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], value, argv[2],
- flags | CONFIG_FLAGS_MULTI_REPLACE);
+ comment, flags | CONFIG_FLAGS_MULTI_REPLACE);
}
else if (actions == ACTION_GET) {
check_argc(argc, 1, 2);
@@ -936,17 +943,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (argc == 2)
return git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], NULL, argv[1],
- flags);
+ NULL, flags);
else
return git_config_set_in_file_gently(given_config_source.file,
- argv[0], NULL);
+ argv[0], NULL, NULL);
}
else if (actions == ACTION_UNSET_ALL) {
check_write();
check_argc(argc, 1, 2);
return git_config_set_multivar_in_file_gently(given_config_source.file,
argv[0], NULL, argv[1],
- flags | CONFIG_FLAGS_MULTI_REPLACE);
+ NULL, flags | CONFIG_FLAGS_MULTI_REPLACE);
}
else if (actions == ACTION_RENAME_SECTION) {
check_write();
diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb5..342907f7bdb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
die(_("$HOME not set"));
rc = git_config_set_multivar_in_file_gently(
config_file, "maintenance.repo", maintpath,
- CONFIG_REGEX_NONE, 0);
+ CONFIG_REGEX_NONE, NULL, 0);
free(global_config_file);
if (rc)
@@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
if (!config_file)
die(_("$HOME not set"));
rc = git_config_set_multivar_in_file_gently(
- config_file, key, NULL, maintpath,
+ config_file, key, NULL, maintpath, NULL,
CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
free(global_config_file);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1e..e4e18adb575 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix,
submodule_to_gitdir(&sb, path);
strbuf_addstr(&sb, "/config");
- if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+ if (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))
die(_("failed to update remote for submodule '%s'"),
path);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9c76b62b02d..a20cc8820e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
bare &&
git_config_set_multivar_in_file_gently(
- to_file, "core.bare", NULL, "true", 0))
+ to_file, "core.bare", NULL, "true", NULL, 0))
error(_("failed to unset '%s' in '%s'"),
"core.bare", to_file);
if (!git_configset_get(&cs, "core.worktree") &&
git_config_set_in_file_gently(to_file,
- "core.worktree", NULL))
+ "core.worktree", NULL, NULL))
error(_("failed to unset '%s' in '%s'"),
"core.worktree", to_file);
diff --git a/config.c b/config.c
index 3cfeb3d8bd9..a22594eabd9 100644
--- a/config.c
+++ b/config.c
@@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key,
}
static ssize_t write_pair(int fd, const char *key, const char *value,
+ const char *comment,
const struct config_store_data *store)
{
int i;
@@ -3041,7 +3042,10 @@ static ssize_t write_pair(int fd, const char *key, const char *value,
strbuf_addch(&sb, value[i]);
break;
}
- strbuf_addf(&sb, "%s\n", quote);
+ if (comment)
+ strbuf_addf(&sb, "%s #%s\n", quote, comment);
+ else
+ strbuf_addf(&sb, "%s\n", quote);
ret = write_in_full(fd, sb.buf, sb.len);
strbuf_release(&sb);
@@ -3130,9 +3134,9 @@ static void maybe_remove_section(struct config_store_data *store,
}
int git_config_set_in_file_gently(const char *config_filename,
- const char *key, const char *value)
+ const char *key, const char *comment, const char *value)
{
- return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
+ return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, comment, 0);
}
void git_config_set_in_file(const char *config_filename,
@@ -3153,7 +3157,7 @@ int repo_config_set_worktree_gently(struct repository *r,
if (r->repository_format_worktree_config) {
char *file = repo_git_path(r, "config.worktree");
int ret = git_config_set_multivar_in_file_gently(
- file, key, value, NULL, 0);
+ file, key, value, NULL, NULL, 0);
free(file);
return ret;
}
@@ -3195,6 +3199,7 @@ void git_config_set(const char *key, const char *value)
int git_config_set_multivar_in_file_gently(const char *config_filename,
const char *key, const char *value,
const char *value_pattern,
+ const char *comment,
unsigned flags)
{
int fd = -1, in_fd = -1;
@@ -3245,7 +3250,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
free(store.key);
store.key = xstrdup(key);
if (write_section(fd, key, &store) < 0 ||
- write_pair(fd, key, value, &store) < 0)
+ write_pair(fd, key, value, comment, &store) < 0)
goto write_err_out;
} else {
struct stat st;
@@ -3399,7 +3404,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
if (write_section(fd, key, &store) < 0)
goto write_err_out;
}
- if (write_pair(fd, key, value, &store) < 0)
+ if (write_pair(fd, key, value, comment, &store) < 0)
goto write_err_out;
}
@@ -3444,7 +3449,7 @@ void git_config_set_multivar_in_file(const char *config_filename,
const char *value_pattern, unsigned flags)
{
if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
- value_pattern, flags))
+ value_pattern, NULL, flags))
return;
if (value)
die(_("could not set '%s' to '%s'"), key, value);
@@ -3467,7 +3472,7 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key,
int res = git_config_set_multivar_in_file_gently(file,
key, value,
value_pattern,
- flags);
+ NULL, flags);
free(file);
return res;
}
diff --git a/config.h b/config.h
index 5dba984f770..a85a8271696 100644
--- a/config.h
+++ b/config.h
@@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);
int git_config_expiry_date(timestamp_t *, const char *, const char *);
int git_config_color(char *, const char *, const char *);
-int git_config_set_in_file_gently(const char *, const char *, const char *);
+int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);
/**
* write config values to a specific config file, takes a key/value pair as
@@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *);
int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
void git_config_set_multivar(const char *, const char *, const char *, unsigned);
int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);
/**
* takes four parameters:
diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..4c91ca5a844 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts)
if (opts->no_commit)
res |= git_config_set_in_file_gently(opts_file,
- "options.no-commit", "true");
+ "options.no-commit", NULL, "true");
if (opts->edit >= 0)
- res |= git_config_set_in_file_gently(opts_file, "options.edit",
+ res |= git_config_set_in_file_gently(opts_file, "options.edit", NULL,
opts->edit ? "true" : "false");
if (opts->allow_empty)
res |= git_config_set_in_file_gently(opts_file,
- "options.allow-empty", "true");
+ "options.allow-empty", NULL, "true");
if (opts->allow_empty_message)
res |= git_config_set_in_file_gently(opts_file,
- "options.allow-empty-message", "true");
+ "options.allow-empty-message", NULL, "true");
if (opts->keep_redundant_commits)
res |= git_config_set_in_file_gently(opts_file,
- "options.keep-redundant-commits", "true");
+ "options.keep-redundant-commits", NULL, "true");
if (opts->signoff)
res |= git_config_set_in_file_gently(opts_file,
- "options.signoff", "true");
+ "options.signoff", NULL, "true");
if (opts->record_origin)
res |= git_config_set_in_file_gently(opts_file,
- "options.record-origin", "true");
+ "options.record-origin", NULL, "true");
if (opts->allow_ff)
res |= git_config_set_in_file_gently(opts_file,
- "options.allow-ff", "true");
+ "options.allow-ff", NULL, "true");
if (opts->mainline) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "%d", opts->mainline);
res |= git_config_set_in_file_gently(opts_file,
- "options.mainline", buf.buf);
+ "options.mainline", NULL, buf.buf);
strbuf_release(&buf);
}
if (opts->strategy)
res |= git_config_set_in_file_gently(opts_file,
- "options.strategy", opts->strategy);
+ "options.strategy", NULL, opts->strategy);
if (opts->gpg_sign)
res |= git_config_set_in_file_gently(opts_file,
- "options.gpg-sign", opts->gpg_sign);
+ "options.gpg-sign", NULL, opts->gpg_sign);
for (size_t i = 0; i < opts->xopts.nr; i++)
res |= git_config_set_multivar_in_file_gently(opts_file,
"options.strategy-option",
- opts->xopts.v[i], "^$", 0);
+ opts->xopts.v[i], "^$", NULL, 0);
if (opts->allow_rerere_auto)
res |= git_config_set_in_file_gently(opts_file,
- "options.allow-rerere-auto",
+ "options.allow-rerere-auto", NULL,
opts->allow_rerere_auto == RERERE_AUTOUPDATE ?
"true" : "false");
if (opts->explicit_cleanup)
res |= git_config_set_in_file_gently(opts_file,
- "options.default-msg-cleanup",
+ "options.default-msg-cleanup", NULL,
describe_cleanup_mode(opts->default_msg_cleanup));
return res;
}
diff --git a/submodule-config.c b/submodule-config.c
index 54130f6a385..11428b4adad 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value)
{
int ret;
- ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+ ret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value);
if (ret < 0)
/* Maybe the user already did that, don't error out here */
warning(_("Could not update .gitmodules entry %s"), key);
diff --git a/submodule.c b/submodule.c
index 213da79f661..86630932d09 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub)
submodule_name_to_gitdir(&config_path, the_repository, sub->name);
strbuf_addstr(&config_path, "/config");
- if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL))
+ if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL, NULL))
warning(_("Could not unset core.worktree setting in submodule '%s'"),
sub->path);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c38786870..daddaedd23c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' '
cat > expect << EOF
[section]
- penguin = very blue
Movie = BadPhysics
UPPERCASE = true
- penguin = kingpin
+ penguin = gentoo #Pygoscelis papua
+ disposition = peckish #find fish
[Sections]
WhatEver = Second
EOF
+test_expect_success 'append comments' '
+ git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
+ git config --comment="find fish" section.disposition peckish &&
+ test_cmp expect .git/config
+'
test_expect_success 'non-match result' 'test_cmp expect .git/config'
diff --git a/worktree.c b/worktree.c
index b02a05a74a3..cf5eea8c931 100644
--- a/worktree.c
+++ b/worktree.c
@@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
static int move_config_setting(const char *key, const char *value,
const char *from_file, const char *to_file)
{
- if (git_config_set_in_file_gently(to_file, key, value))
+ if (git_config_set_in_file_gently(to_file, key, NULL, value))
return error(_("unable to set %s in '%s'"), key, to_file);
- if (git_config_set_in_file_gently(from_file, key, NULL))
+ if (git_config_set_in_file_gently(from_file, key, NULL, NULL))
return error(_("unable to unset %s in '%s'"), key, from_file);
return 0;
}
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/1] rebase: teach `--exec` about `GIT_REBASE_BRANCH`
From: Kristoffer Haugsbakk @ 2024-03-07 15:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Phillip Wood
In-Reply-To: <xmqqo7buuce7.fsf@gitster.g>
On Mon, Mar 4, 2024, at 00:24, Junio C Hamano wrote:
> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>
>> The command fed to `--exec` might need some contextual information from
>> the branch name. But there is no convenient access to the branch name
>> that we were on before starting the rebase; rebase operates in detached
>> HEAD mode so we cannot ask for it directly. This means that we need to
>> parse something like this from the first line of `git branch --list`:
>>
>> (no branch, rebasing <branch>)
>>
>> This is a moderate amount of effort for something that git-rebase(1) can
>> store for us.
>>
>> To that end, teach `--exec` about an env. variable which stores the
>> branch name for the rebase-in-progress, if applicable.
>
> You seem to be saying that `git branch --list` output already
> contains the necessary information but it is shown in a hard to use
> format. Is the information given at least always accurate and
> reliable?
>
> Assuming it is, do you know where "git branch --list" gets that
> information when it says "(no branch, rebasing <branch>)"?
>
> git-rebase(1) is already storing information sufficient to let "git
> branch --list" to produce that information, and there are other ways
> to inspect that state ("git status" gives the same information but
> it also is in a "meant for humans" format).
>
> So, isn't it just the matter of surfacing the information that we
> are already recording and is already available in a fashion that is
> easier to use? For example, if "git status --porcelain=[version]"
> does not give the information, perhaps you can add a line or two to
> it, instead of duplicating the same information in two places?
>
> It comes from wt-status.c:wt_status_check_rebase() where state->branch
> is assigned to, by reading "$GIT_DIR/rebase-{apply,merge}/head-name".
Okay, thanks for the code directions and input (both). I’ll try to get
back to a rewrite on this topic in a while.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* [PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name'
From: Jeff Hostetler via GitGitGadget @ 2024-03-07 15:22 UTC (permalink / raw)
To: git; +Cc: Josh Steadmon, Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1679.git.1709566808.gitgitgadget@gmail.com>
Here is version 2 of this series. The only change from V1 is to combine the
last two commits as discussed.
Thanks Jeff
----------------------------------------------------------------------------
Some Git commands do not emit def_param events for interesting config and
environment variable settings. Let's fix that.
Builtin commands compiled into git.c have the normal control flow and emit a
cmd_name event and then def_param events for each interesting config and
environment variable. However, some special "query" commands, like
--exec-path, or some forms of alias expansion, emitted a cmd_name but did
not emit def_param events.
Also, special commands git-remote-https is built from remote-curl.c and
git-http-fetch is built from http-fetch.c and do not use the normal set up
in git.c. These emitted a cmd_name but not def_param events.
To minimize the footprint of this commit, move the calls to
trace2_cmd_list_config() and trace2_cmd_list_env_vars() into
trace2_cmd_name() so that we always get a set of def_param events when a
cmd_name event is generated.
Users can define local config settings on a repo to classify/name a repo
(e.g. "project-foo" vs "personal") and use the def_param feature to label
Trace2 data so that (a third-party) telemetry service does not collect data
on personal repos or so that telemetry from one work repo is distinguishable
from another work repo in database queries.
Jeff Hostetler (3):
t0211: demonstrate missing 'def_param' events for certain commands
trace2: avoid emitting 'def_param' set more than once
trace2: emit 'def_param' set with 'cmd_name' event
git.c | 6 --
t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++
trace2.c | 15 +++
3 files changed, 246 insertions(+), 6 deletions(-)
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1679%2Fjeffhostetler%2Falways-emit-def-param-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1679/jeffhostetler/always-emit-def-param-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1679
Range-diff vs v1:
1: b378b93242a = 1: b378b93242a t0211: demonstrate missing 'def_param' events for certain commands
2: 65068e97597 = 2: 65068e97597 trace2: avoid emitting 'def_param' set more than once
3: 9507184b4f1 ! 3: 178721cd4f0 trace2: emit 'def_param' set with 'cmd_name' event
@@ Commit message
the "trace2_cmd_name()" function to generate the set of 'def_param'
events.
- We can later remove explicit calls to "trace2_cmd_list_config()" and
- "trace2_cmd_list_env_vars()" in git.c.
+ Remove explicit calls to "trace2_cmd_list_config()" and
+ "trace2_cmd_list_env_vars()" in git.c since they are no longer needed.
+ Reviewed-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
+ ## git.c ##
+@@ git.c: static int handle_alias(int *argcp, const char ***argv)
+ strvec_pushv(&child.args, (*argv) + 1);
+
+ trace2_cmd_alias(alias_command, child.args.v);
+- trace2_cmd_list_config();
+- trace2_cmd_list_env_vars();
+ trace2_cmd_name("_run_shell_alias_");
+
+ ret = run_command(&child);
+@@ git.c: static int handle_alias(int *argcp, const char ***argv)
+ COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
+
+ trace2_cmd_alias(alias_command, new_argv);
+- trace2_cmd_list_config();
+- trace2_cmd_list_env_vars();
+
+ *argv = new_argv;
+ *argcp += count - 1;
+@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
+
+ trace_argv_printf(argv, "trace: built-in: git");
+ trace2_cmd_name(p->cmd);
+- trace2_cmd_list_config();
+- trace2_cmd_list_env_vars();
+
+ validate_cache_entries(the_repository->index);
+ status = p->fn(argc, argv, prefix);
+
## t/t0211-trace2-perf.sh ##
@@ t/t0211-trace2-perf.sh: test_expect_success 'expect def_params for normal builtin command' '
# Representative query command dispatched in handle_options()
4: e8528715ebf < -: ----------- trace2: remove unneeded calls to generate 'def_param' set
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 2/3] trace2: avoid emitting 'def_param' set more than once
From: Jeff Hostetler via GitGitGadget @ 2024-03-07 15:22 UTC (permalink / raw)
To: git; +Cc: Josh Steadmon, Jeff Hostetler, Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1679.v2.git.1709824949.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
During nested alias expansion it is possible for
"trace2_cmd_list_config()" and "trace2_cmd_list_env_vars()"
to be called more than once. This causes a full set of
'def_param' events to be emitted each time. Let's avoid
that.
Add code to those two functions to only emit them once.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 2 +-
trace2.c | 12 ++++++++++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 588c5bad033..7b353195396 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -470,7 +470,7 @@ test_expect_success 'expect def_params during shell alias expansion' '
grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
'
-test_expect_failure 'expect def_params during nested git alias expansion' '
+test_expect_success 'expect def_params during nested git alias expansion' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
diff --git a/trace2.c b/trace2.c
index f1e268bd159..facce641ef3 100644
--- a/trace2.c
+++ b/trace2.c
@@ -464,17 +464,29 @@ void trace2_cmd_alias_fl(const char *file, int line, const char *alias,
void trace2_cmd_list_config_fl(const char *file, int line)
{
+ static int emitted = 0;
+
if (!trace2_enabled)
return;
+ if (emitted)
+ return;
+ emitted = 1;
+
tr2_cfg_list_config_fl(file, line);
}
void trace2_cmd_list_env_vars_fl(const char *file, int line)
{
+ static int emitted = 0;
+
if (!trace2_enabled)
return;
+ if (emitted)
+ return;
+ emitted = 1;
+
tr2_list_env_vars_fl(file, line);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/3] t0211: demonstrate missing 'def_param' events for certain commands
From: Jeff Hostetler via GitGitGadget @ 2024-03-07 15:22 UTC (permalink / raw)
To: git; +Cc: Josh Steadmon, Jeff Hostetler, Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1679.v2.git.1709824949.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Some Git commands fail to emit 'def_param' events for interesting
config and environment variable settings.
Add unit tests to demonstrate this.
Most commands are considered "builtin" and are based upon git.c.
These typically do emit 'def_param' events. Exceptions are some of
the "query" commands, the "run-dashed" mechanism, and alias handling.
Commands built from remote-curl.c (instead of git.c), such as
"git-remote-https", do not emit 'def_param' events.
Likewise, "git-http-fetch" is built http-fetch.c and does not emit
them.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 231 insertions(+)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 290b6eaaab1..588c5bad033 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -287,4 +287,235 @@ test_expect_success 'unsafe URLs are redacted by default' '
grep "d0|main|def_param|.*|remote.origin.url:https://user:pwd@example.com" actual
'
+# Confirm that the requested command produces a "cmd_name" and a
+# set of "def_param" events.
+#
+try_simple () {
+ test_when_finished "rm prop.perf actual" &&
+
+ cmd=$1 &&
+ cmd_name=$2 &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ $cmd &&
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+ grep "d0|main|cmd_name|.*|$cmd_name" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual
+}
+
+# Representative mainstream builtin Git command dispatched
+# in run_builtin() in git.c
+#
+test_expect_success 'expect def_params for normal builtin command' '
+ try_simple "git version" "version"
+'
+
+# Representative query command dispatched in handle_options()
+# in git.c
+#
+test_expect_failure 'expect def_params for query command' '
+ try_simple "git --man-path" "_query_"
+'
+
+# remote-curl.c does not use the builtin setup in git.c, so confirm
+# that executables built from remote-curl.c emit def_params.
+#
+# Also tests the dashed-command handling where "git foo" silently
+# spawns "git-foo". Make sure that both commands should emit
+# def_params.
+#
+# Pass bogus arguments to remote-https and allow the command to fail
+# because we don't actually have a remote to fetch from. We just want
+# to see the run-dashed code run an executable built from
+# remote-curl.c rather than git.c. Confirm that we get def_param
+# events from both layers.
+#
+test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_might_fail env \
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git remote-http x y &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ grep "d0|main|cmd_name|.*|_run_dashed_" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ grep "d1|main|cmd_name|.*|remote-curl" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+# Similarly, `git-http-fetch` is not built from git.c so do a
+# trivial fetch so that the main git.c run-dashed code spawns
+# an executable built from http-fetch.c. Confirm that we get
+# def_param events from both layers.
+#
+test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_might_fail env \
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git http-fetch --stdin file:/// <<-EOF &&
+ EOF
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ grep "d0|main|cmd_name|.*|_run_dashed_" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ grep "d1|main|cmd_name|.*|http-fetch" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+# Historically, alias expansion explicitly emitted the def_param
+# events (independent of whether the command was a builtin, a Git
+# command or arbitrary shell command) so that it wasn't dependent
+# upon the unpeeling of the alias. Let's make sure that we preserve
+# the net effect.
+#
+test_expect_success 'expect def_params during git alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+
+ # We unpeel that and substitute "version" into "xxx" (giving
+ # "git version") and update the cmd_name event.
+ grep "d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_git_alias_)" actual &&
+
+ # These def_param events could be associated with either of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # The "git version" child sees a different cmd_name hierarchy.
+ # Also test the def_param (only for completeness).
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_git_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+test_expect_success 'expect def_params during shell alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "!git version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+
+ # We unpeel that and substitute "git version" for "git xxx" (as a
+ # shell command. Another cmd_name event is emitted as we unpeel.
+ grep "d0|main|cmd_name|.*|_run_shell_alias_ (_run_dashed_/_run_shell_alias_)" actual &&
+
+ # These def_param events could be associated with either of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # We get the following only because we used a git command for the
+ # shell command. In general, it could have been a shell script and
+ # we would see nothing.
+ #
+ # The child knows the cmd_name hierarchy so it includes it.
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_shell_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+test_expect_failure 'expect def_params during nested git alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "yyy" &&
+ test_config_global "alias.yyy" "version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and try to spawn "git-xxx"
+ # and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+ grep "d0|main|child_start|.*|.* class:dashed argv:\[git-xxx\]" actual &&
+
+ # We unpeel that and substitute "yyy" into "xxx" (giving "git yyy")
+ # and spawn "git-yyy" and the child will fail.
+ grep "d0|main|alias|.*|alias:xxx argv:\[yyy\]" actual &&
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_/_run_dashed_)" actual &&
+ grep "d0|main|child_start|.*|.* class:dashed argv:\[git-yyy\]" actual &&
+
+ # We unpeel that and substitute "version" into "xxx" (giving
+ # "git version") and update the cmd_name event.
+ grep "d0|main|alias|.*|alias:yyy argv:\[version\]" actual &&
+ grep "d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_dashed_/_run_git_alias_)" actual &&
+
+ # These def_param events could be associated with any of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual >actual.matches &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # However, we do not want them repeated each time we unpeel.
+ test_line_count = 1 actual.matches &&
+
+ # The "git version" child sees a different cmd_name hierarchy.
+ # Also test the def_param (only for completeness).
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_dashed_/_run_git_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/3] trace2: emit 'def_param' set with 'cmd_name' event
From: Jeff Hostetler via GitGitGadget @ 2024-03-07 15:22 UTC (permalink / raw)
To: git; +Cc: Josh Steadmon, Jeff Hostetler, Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1679.v2.git.1709824949.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Some commands do not cause a set of 'def_param' events to be emitted.
This includes "git-remote-https", "git-http-fetch", and various
"query" commands, like "git --man-path".
Since all of these commands do emit a 'cmd_name' event, add code to
the "trace2_cmd_name()" function to generate the set of 'def_param'
events.
Remove explicit calls to "trace2_cmd_list_config()" and
"trace2_cmd_list_env_vars()" in git.c since they are no longer needed.
Reviewed-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
git.c | 6 ------
t/t0211-trace2-perf.sh | 6 +++---
trace2.c | 3 +++
3 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/git.c b/git.c
index 7068a184b0a..a769d72ab8f 100644
--- a/git.c
+++ b/git.c
@@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
strvec_pushv(&child.args, (*argv) + 1);
trace2_cmd_alias(alias_command, child.args.v);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
trace2_cmd_name("_run_shell_alias_");
ret = run_command(&child);
@@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
trace2_cmd_alias(alias_command, new_argv);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
*argv = new_argv;
*argcp += count - 1;
@@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
trace_argv_printf(argv, "trace: built-in: git");
trace2_cmd_name(p->cmd);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
validate_cache_entries(the_repository->index);
status = p->fn(argc, argv, prefix);
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 7b353195396..13ef69b92f8 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -320,7 +320,7 @@ test_expect_success 'expect def_params for normal builtin command' '
# Representative query command dispatched in handle_options()
# in git.c
#
-test_expect_failure 'expect def_params for query command' '
+test_expect_success 'expect def_params for query command' '
try_simple "git --man-path" "_query_"
'
@@ -337,7 +337,7 @@ test_expect_failure 'expect def_params for query command' '
# remote-curl.c rather than git.c. Confirm that we get def_param
# events from both layers.
#
-test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
+test_expect_success 'expect def_params for remote-curl and _run_dashed_' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
@@ -366,7 +366,7 @@ test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
# an executable built from http-fetch.c. Confirm that we get
# def_param events from both layers.
#
-test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '
+test_expect_success 'expect def_params for http-fetch and _run_dashed_' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
diff --git a/trace2.c b/trace2.c
index facce641ef3..f894532d053 100644
--- a/trace2.c
+++ b/trace2.c
@@ -433,6 +433,9 @@ void trace2_cmd_name_fl(const char *file, int line, const char *name)
for_each_wanted_builtin (j, tgt_j)
if (tgt_j->pfn_command_name_fl)
tgt_j->pfn_command_name_fl(file, line, name, hierarchy);
+
+ trace2_cmd_list_config();
+ trace2_cmd_list_env_vars();
}
void trace2_cmd_mode_fl(const char *file, int line, const char *mode)
--
gitgitgadget
^ permalink raw reply related
* RE: [PATCH] Allow git-config to append a comment
From: Ralph Seichter @ 2024-03-07 15:26 UTC (permalink / raw)
To: rsbecker, 'Junio C Hamano', 'Ralph Seichter'
Cc: gitgitgadget, git
In-Reply-To: <098101da7096$cd773d40$6865b7c0$@nexbridge.com>
* rsbecker@nexbridge.com:
> While comments are permitted in .gitconfig files, I am not 100% sure
> that all stakeholders, particularly those who parse .gitconfig files
> in their own scripts outside of git - sure, it is their own
> responsibility, but this might be unexpected.
Comments are nothing new, and humans have added far crazier comments to
their Git config in the past. The patch ensures that a '#' precedes the
comments added using git-config, which is not guaranteed to happen when
Joe Random User manually edits config files.
I think that anybody incapable of reliably dealing with comments in
config files would already have fallen flat on his/her nose, regardless
of how those comments were made.
> I worry that this might unintentionally introduce incompatibilities
> into repository configurations.
Do you have an example?
-Ralph
^ permalink raw reply
* [PATCH] reftable/block: fix binary search over restart counter
From: Patrick Steinhardt @ 2024-03-07 15:26 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]
Records store their keys prefix-compressed. As many records will share a
common prefix (e.g. "refs/heads/"), this can end up saving quite a bit
of disk space. The downside of this is that it is not possible to just
seek into the middle of a block and consume the corresponding record
because it may depend on prefixes read from preceding records.
To help with this usecase, the reftable format writes every n'th record
without using prefix compression, which is called a "restart". The list
of restarts is stored at the end of each block so that a reader can
figure out entry points at which to read a full record without having to
read all preceding records.
This allows us to do a binary search over the records in a block when
searching for a particular key by iterating through the restarts until
we have found the section in which our record must be located. From
thereon we perform a linear search to locate the desired record.
This mechanism is broken though. In `block_reader_seek()` we call
`binsearch()` over the count of restarts in the current block. The
function we pass to compare records with each other computes the key at
the current index and then compares it to our search key by calling
`strbuf_cmp()`, returning its result directly. But `binsearch()` expects
us to return a truish value that indicates whether the current index is
smaller than the searched-for key. And unless our key exactly matches
the value at the restart counter we always end up returning a truish
value.
The consequence is that `binsearch()` essentially always returns 0,
indicacting to us that we must start searching right at the beginning of
the block. This works by chance because we now always do a linear scan
from the start of the block, and thus we would still end up finding the
desired record. But needless to say, this makes the optimization quite
useless.
Fix this bug by returning whether the current key is smaller than the
searched key. As the current behaviour was correct it is not possible to
write a test. Furthermore it is also not really possible to demonstrate
in a benchmark that this fix speeds up seeking records.
This may cause the reader to question whether this binary search makes
sense in the first place if it doesn't even help with performance. But
it would end up helping if we were to read a reftable with a much larger
block size. Blocks can be up to 16MB in size, in which case it will
become much more important to avoid the linear scan. We are not yet
ready to read or write such larger blocks though, so we have to live
without a benchmark demonstrating this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/reftable/block.c b/reftable/block.c
index 72eb73b380..3d7a7022e7 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -302,7 +302,7 @@ static int restart_key_less(size_t idx, void *args)
result = strbuf_cmp(&a->key, &rkey);
strbuf_release(&rkey);
- return result;
+ return result <= 0;
}
void block_iter_copy_from(struct block_iter *dest, struct block_iter *src)
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* RE: [PATCH] Allow git-config to append a comment
From: rsbecker @ 2024-03-07 15:40 UTC (permalink / raw)
To: 'Ralph Seichter', 'Junio C Hamano'; +Cc: gitgitgadget, git
In-Reply-To: <87h6higj1a.fsf@ra.horus-it.com>
On Thursday, March 7, 2024 10:26 AM, Ralph Seichter wrote:
>* rsbecker@nexbridge.com:
>
>> While comments are permitted in .gitconfig files, I am not 100% sure
>> that all stakeholders, particularly those who parse .gitconfig files
>> in their own scripts outside of git - sure, it is their own
>> responsibility, but this might be unexpected.
>
>Comments are nothing new, and humans have added far crazier comments to
their Git config in the past. The patch ensures that a '#'
>precedes the comments added using git-config, which is not guaranteed to
happen when Joe Random User manually edits config files.
>
>I think that anybody incapable of reliably dealing with comments in config
files would already have fallen flat on his/her nose,
>regardless of how those comments were made.
>
>> I worry that this might unintentionally introduce incompatibilities
>> into repository configurations.
>
>Do you have an example?
No example. This is a comment on "potential" changes to data that scripts
around git for automation purposes might use. My purpose is just to
highlight, for the purpose of reviewing the change, that there may be
unintended impacts, that's all. It may be useful to include comments in the
change notices and documentation pages that using this capability may impact
scripting. When a user manually puts in a comment, any breakages in their
scripts are 100% their issue. With git config moving comments around,
responsibility shifts to git - a.k.a., unintended consequences. I am not
asking that this change not happen - it is a good thing, but ensuring that
we communicate that this may cause breakages if external programs/scripts
read .gitconfig would be helpful. This also would need to be coordinated
with the libification efforts at some point.
^ permalink raw reply
* RE: [PATCH] Allow git-config to append a comment
From: Ralph Seichter @ 2024-03-07 15:57 UTC (permalink / raw)
To: rsbecker, 'Ralph Seichter', 'Junio C Hamano'
Cc: gitgitgadget, git
In-Reply-To: <09c501da70a5$bd2f4f40$378dedc0$@nexbridge.com>
* rsbecker@nexbridge.com:
> My purpose is just to highlight, for the purpose of reviewing the
> change, that there may be unintended impacts, that's all.
I see. Have you perhaps spotted a flaw in the patch which might cause
problems? If so, I'd like to address it.
> With git config moving comments around, responsibility shifts to git -
> a.k.a., unintended consequences.
It is important to note that my implementation does not engage in
"moving comments around" at all. It merely supports adding comment
suffixes to config lines *while they are created by git-config*. No
after-the-fact comment manipulation is done. There are no multiline
comments involved either, which I should have mentioned from the get-go.
The limited scope of my proposal is deliberate, and aimed at avoiding
possible problems based on the design alone.
-Ralph
^ permalink raw reply
* Re: [PATCH] reftable/block: fix binary search over restart counter
From: Patrick Steinhardt @ 2024-03-07 16:24 UTC (permalink / raw)
To: git
In-Reply-To: <a4312698cceab5f2438c9dd34465da21d719e256.1709825186.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3495 bytes --]
On Thu, Mar 07, 2024 at 04:26:38PM +0100, Patrick Steinhardt wrote:
> Records store their keys prefix-compressed. As many records will share a
> common prefix (e.g. "refs/heads/"), this can end up saving quite a bit
> of disk space. The downside of this is that it is not possible to just
> seek into the middle of a block and consume the corresponding record
> because it may depend on prefixes read from preceding records.
>
> To help with this usecase, the reftable format writes every n'th record
> without using prefix compression, which is called a "restart". The list
> of restarts is stored at the end of each block so that a reader can
> figure out entry points at which to read a full record without having to
> read all preceding records.
>
> This allows us to do a binary search over the records in a block when
> searching for a particular key by iterating through the restarts until
> we have found the section in which our record must be located. From
> thereon we perform a linear search to locate the desired record.
>
> This mechanism is broken though. In `block_reader_seek()` we call
> `binsearch()` over the count of restarts in the current block. The
> function we pass to compare records with each other computes the key at
> the current index and then compares it to our search key by calling
> `strbuf_cmp()`, returning its result directly. But `binsearch()` expects
> us to return a truish value that indicates whether the current index is
> smaller than the searched-for key. And unless our key exactly matches
> the value at the restart counter we always end up returning a truish
> value.
>
> The consequence is that `binsearch()` essentially always returns 0,
> indicacting to us that we must start searching right at the beginning of
> the block. This works by chance because we now always do a linear scan
> from the start of the block, and thus we would still end up finding the
> desired record. But needless to say, this makes the optimization quite
> useless.
>
> Fix this bug by returning whether the current key is smaller than the
> searched key. As the current behaviour was correct it is not possible to
> write a test. Furthermore it is also not really possible to demonstrate
> in a benchmark that this fix speeds up seeking records.
>
> This may cause the reader to question whether this binary search makes
> sense in the first place if it doesn't even help with performance. But
> it would end up helping if we were to read a reftable with a much larger
> block size. Blocks can be up to 16MB in size, in which case it will
> become much more important to avoid the linear scan. We are not yet
> ready to read or write such larger blocks though, so we have to live
> without a benchmark demonstrating this.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> reftable/block.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index 72eb73b380..3d7a7022e7 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -302,7 +302,7 @@ static int restart_key_less(size_t idx, void *args)
>
> result = strbuf_cmp(&a->key, &rkey);
> strbuf_release(&rkey);
> - return result;
> + return result <= 0;
> }
>
> void block_iter_copy_from(struct block_iter *dest, struct block_iter *src)
> --
> 2.44.0
>
Hum. I noticed that this causes a memory leak in our CI. I'll need to
investigate.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles
From: Junio C Hamano @ 2024-03-07 17:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <Zelb8ldHh4Lnlh7Z@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> It is consistent. The problem is rather that `mks_tempfile_m()` takes a
> mode as input, but still ends up applying the umask to that mode.
Ah, OK.
> Thus,
> using that function without a subsequent call to chmod(3P) would end up
> mishandling "core.sharedRepository".
It is understandable because this is meant for tempfile; you want to
create it, expect to use it yourself and not by others, before you
are done with it. There is no place in this sequence where you need
to open access up to those who share access to the repository.
The rest of the message is primarily for my own education.
I got a bit curious how other parts of the system does this. For
example, writing an loose object calls git_mkstemp_mode() with 0444
and then finalize_object_file() calls adjust_shared_perm() after it
renames the file to its final name at the end. A tight umask like
0700 may demote the original 0444 to 0400, but adjust_shared_perm()
expands the 0400 appropriately.
The index file is more interesting. We bypass the whole mkstemp
thing (as we know the final filename and add .lock after it), and
the call chain looks like this:
repo_refresh_and_write_index() ->
repo_hold_locked_index() ->
hold_lock_file_for_update() ->
lock_file() ->
create_tempfile_mode() ->
open()
activate_tempfile()
adjust_shared_perm()
refresh_index()
write_locked_index() ->
commit_locked_index() ->
commit_lock_file_to() ->
rename_tempfile().
After we create the file with open() and before returning to the
caller, hold_lock_file_for_update() already sets the permission
bits correctly, so the "committing" phase to rename the written
lockfile into its final place becomes merely a rename without any
futzing with permission bits.
So in short, for a file that we intend to create, write, and then
commit to the final name, we use at least two approaches:
- Let mkstemp_mode() do its thing, and fix the permission bits
later with adjust_shared_perm().
- Let hold_lock_file_for_update() take care of the permission bits.
I sense there might be some clean-up opportunities around here.
After all, lockfile is (or at least pretends to be) built on top of
tempfile, and it is for more permanent (as opposed to temporary)
files, but it somehow wasn't a good fit to wrap new tables in this
series?
Thanks.
^ permalink raw reply
* Re: [PATCH v4] tests: modernize the test script t0010-racy-git.sh
From: Junio C Hamano @ 2024-03-07 18:17 UTC (permalink / raw)
To: Aryan Gupta
Cc: Christian Couder, Aryan Gupta via GitGitGadget, git,
Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ], Eric Sunshine
In-Reply-To: <CAMbn=B73boxu1HDy2UHGz83wxnE7=udmbSv4nFsV+ngA0Bn0Sg@mail.gmail.com>
Aryan Gupta <garyan447@gmail.com> writes:
> I thought that there were some bugs in GGG due to which it sent some
> headers which were not syntactically correct. So I tried sending it again.
> And that was the whole purpose of this.
I was wondering about the same thing. I still see an unwanted "[ ]"
around Kristoffer's e-mail address that will break responding to the
message in your [PATCH v4] e-mail that can be seen at
https://lore.kernel.org/git/pull.1675.v4.git.1709716446874.gitgitgadget@gmail.com/raw
so, the experiment revealed that it did send some headers were
broken.
Thanks for a clarification.
^ permalink raw reply
* Re: [PATCH v4] tests: modernize the test script t0010-racy-git.sh
From: Junio C Hamano @ 2024-03-07 18:30 UTC (permalink / raw)
To: Aryan Gupta
Cc: Christian Couder, Aryan Gupta via GitGitGadget, git,
Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ], Eric Sunshine
In-Reply-To: <xmqqjzmdnbz4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I was wondering about the same thing. I still see an unwanted "[ ]"
> around Kristoffer's e-mail address that will break responding to the
> message in your [PATCH v4] e-mail that can be seen at
>
> https://lore.kernel.org/git/pull.1675.v4.git.1709716446874.gitgitgadget@gmail.com/raw
>
> so, the experiment revealed that it did send some headers were
> broken.
This does not necessarily mean GGG is broken. The majority of the
patches I see here from GGG are without these funny [square-bracket]
around addresses at all, and this was the only patch (or it is
possible that your other patches may have had the same issue; I do
not remember) with that problem. It might be caused by what you
feed GGG (e.g., the messages you give it in your pull request) that
caused GGG to hiccup, perhaps?
^ permalink raw reply
* Re: [PATCH v4] tests: modernize the test script t0010-racy-git.sh
From: Aryan Gupta @ 2024-03-07 18:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, Aryan Gupta via GitGitGadget, git,
Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ], Eric Sunshine
In-Reply-To: <xmqqfrx1nbde.fsf@gitster.g>
On Thu, Mar 7, 2024 at 7:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I was wondering about the same thing. I still see an unwanted "[ ]"
> > around Kristoffer's e-mail address that will break responding to the
> > message in your [PATCH v4] e-mail that can be seen at
> >
> > https://lore.kernel.org/git/pull.1675.v4.git.1709716446874.gitgitgadget@gmail.com/raw
> >
> > so, the experiment revealed that it did send some headers were
> > broken.
>
> This does not necessarily mean GGG is broken. The majority of the
> patches I see here from GGG are without these funny [square-bracket]
> around addresses at all, and this was the only patch (or it is
> possible that your other patches may have had the same issue; I do
> not remember) with that problem. It might be caused by what you
> feed GGG (e.g., the messages you give it in your pull request) that
> caused GGG to hiccup, perhaps?
>
Ohh yes. I think I got the problem. I have fixed it. Can I try sending
the patch again now?
^ permalink raw reply
* [PATCH] wt-status: Don't find scissors line beyond buf len
From: Florian Schmidt @ 2024-03-07 18:37 UTC (permalink / raw)
To: git
Cc: Florian Schmidt, Jonathan Davies, Phillip Wood, Junio C Hamano,
Denton Liu
Currently, if
(a) There is a "---" divider in a commit message,
(b) At some point beyond that divider, there is a cut-line (that is,
"# ------------------------ >8 ------------------------") in the
commit message,
(c) the user does not explicitly set the "no-divider" option,
then "git interpret-trailers" will hang indefinitively.
This is because when (a) is true, find_end_of_log_message() will invoke
ignored_log_message_bytes() with a len that is intended to make it
ignore the part of the commit message beyond the divider. However,
ignored_log_message_bytes() calls wt_status_locate_end(), and that
function ignores the length restriction when it tries to locate the cut
line. If it manages to find one, the returned cutoff value is greater
than len. At this point, ignored_log_message_bytes() goes into an
infinite loop, because it won't advance the string parsing beyond len,
but the exit condition expects to reach cutoff.
It seems sensible to expect that wt_status_locate_end() should honour
the length parameter passed in, and doing so fixes this issue.
Signed-off-by: Florian Schmidt <flosch@nutanix.com>
Reviewed-by: Jonathan Davies <jonathan.davies@nutanix.com>
---
Side remark: Since strstr() doesn't consider len, and will always search
up to a null byte, I now wonder whether it would be safer to create a
new strbuf that only contains the len bytes we want to operate on. If
anybody ever thinks they can pass a non-null-terminated string into
wt_status_locate_end() because they already provide a len parameter,
they will not have a good time. So it's that traded off against the
slightly higher overhead of creating yet another buffer and copying a
potentially large-ish commit message around.
t/t7513-interpret-trailers.sh | 14 ++++++++++++++
wt-status.c | 13 +++++++++----
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index ec9c6de114..3d3e13ccf8 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1935,4 +1935,18 @@ test_expect_success 'suppressing --- does not disable cut-line handling' '
test_cmp expected actual
'
+test_expect_success 'handling of --- lines in conjunction with cut-lines' '
+ echo "my-trailer: here" >expected &&
+
+ git interpret-trailers --parse >actual <<-\EOF &&
+ subject
+
+ my-trailer: here
+ ---
+ # ------------------------ >8 ------------------------
+ EOF
+
+ test_cmp expected actual
+'
+
test_done
diff --git a/wt-status.c b/wt-status.c
index b5a29083df..51a84575ed 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1089,14 +1089,19 @@ size_t wt_status_locate_end(const char *s, size_t len)
{
const char *p;
struct strbuf pattern = STRBUF_INIT;
+ size_t result = len;
strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
if (starts_with(s, pattern.buf + 1))
- len = 0;
- else if ((p = strstr(s, pattern.buf)))
- len = p - s + 1;
+ result = 0;
+ else if ((p = strstr(s, pattern.buf))) {
+ result = p - s + 1;
+ if (result > len) {
+ result = len;
+ }
+ }
strbuf_release(&pattern);
- return len;
+ return result;
}
void wt_status_append_cut_line(struct strbuf *buf)
--
2.42.0
^ permalink raw reply related
* Re: [PATCH] wt-status: Don't find scissors line beyond buf len
From: Junio C Hamano @ 2024-03-07 19:20 UTC (permalink / raw)
To: Florian Schmidt
Cc: git, Jonathan Davies, Phillip Wood, Denton Liu, Linus Arver
In-Reply-To: <20240307183743.219951-1-flosch@nutanix.com>
Florian Schmidt <flosch@nutanix.com> writes:
> Currently, if
> (a) There is a "---" divider in a commit message,
> (b) At some point beyond that divider, there is a cut-line (that is,
> "# ------------------------ >8 ------------------------") in the
> commit message,
> (c) the user does not explicitly set the "no-divider" option,
> then "git interpret-trailers" will hang indefinitively.
You do not have to say "Currently, if"; just "If" is sufficient.
Cf. Documentation/SubmittingPatches[[present-tense]]
> This is because when (a) is true, find_end_of_log_message() will invoke
> ignored_log_message_bytes() with a len that is intended to make it
> ignore the part of the commit message beyond the divider. However,
> ignored_log_message_bytes() calls wt_status_locate_end(), and that
> function ignores the length restriction when it tries to locate the cut
> line. If it manages to find one, the returned cutoff value is greater
> than len. At this point, ignored_log_message_bytes() goes into an
> infinite loop, because it won't advance the string parsing beyond len,
> but the exit condition expects to reach cutoff.
Good finding.
> It seems sensible to expect that wt_status_locate_end() should honour
> the length parameter passed in, and doing so fixes this issue.
Thanks. This is an ancient bug, not a retression from recent
changes to the trailer library [linusa CC'ed to save him from
wasting his time wondering if he broke anything].
> diff --git a/wt-status.c b/wt-status.c
> index b5a29083df..51a84575ed 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1089,14 +1089,19 @@ size_t wt_status_locate_end(const char *s, size_t len)
> {
> const char *p;
> struct strbuf pattern = STRBUF_INIT;
> + size_t result = len;
>
> strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
> if (starts_with(s, pattern.buf + 1))
> - len = 0;
> - else if ((p = strstr(s, pattern.buf)))
> - len = p - s + 1;
> + result = 0;
> + else if ((p = strstr(s, pattern.buf))) {
> + result = p - s + 1;
> + if (result > len) {
> + result = len;
> + }
> + }
> strbuf_release(&pattern);
> - return len;
> + return result;
> }
Looks correct, but we probably can make the fix a lot more isolated
into a single block, like the attached patch. How does this look?
wt-status.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git c/wt-status.c w/wt-status.c
index b5a29083df..511f37cfe0 100644
--- c/wt-status.c
+++ w/wt-status.c
@@ -1093,8 +1093,11 @@ size_t wt_status_locate_end(const char *s, size_t len)
strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
if (starts_with(s, pattern.buf + 1))
len = 0;
- else if ((p = strstr(s, pattern.buf)))
- len = p - s + 1;
+ else if ((p = strstr(s, pattern.buf))) {
+ int newlen = p - s + 1;
+ if (newlen < len)
+ len = newlen;
+ }
strbuf_release(&pattern);
return len;
}
^ permalink raw reply related
* Re: [PATCH] wt-status: Don't find scissors line beyond buf len
From: Junio C Hamano @ 2024-03-07 19:35 UTC (permalink / raw)
To: Florian Schmidt; +Cc: git, Jonathan Davies, Phillip Wood, Denton Liu
In-Reply-To: <20240307183743.219951-1-flosch@nutanix.com>
Florian Schmidt <flosch@nutanix.com> writes:
> Side remark: Since strstr() doesn't consider len, and will always search
> up to a null byte, I now wonder whether it would be safer to create a
> new strbuf that only contains the len bytes we want to operate on.
That is a valid concern in general, but does not seem to apply to
the current codebase. Thanks for being careful.
Two of the three callers of wt_status_locate_end() feed the pointer
into a piece of memory that is owned by strbuf, which guarantees
that the memory has an extra NUL to terminate it as a string even if
you did
strbuf buf = STRBUF_INIT;
strbuf_addch(&buf, 'A');
The other one is in commit.c:ignored_log_message_bytes() that still
takes <buf, len> as input, but again, two of its three callers call
it with a pointer that points at the beginning of memory held by an
instance of strbuf.
That leaves us trailer.c:find_end_of_log_message() the only one to
worry about, but it uses strlen() on the pointer before calling
ignored_log_message_bytes() so the region of the memory pointed at
by the pointer is assumed to be NUL-terminated already, and
presumably (I didn't follow the logic there too closely) the length
is also computed within that NUL-terminated string.
^ permalink raw reply
* Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers
From: René Scharfe @ 2024-03-07 19:41 UTC (permalink / raw)
To: Jeff King, git
Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307110809.GA3668372@coredump.intra.peff.net>
Am 07.03.24 um 12:08 schrieb Jeff King:
> On Thu, Mar 07, 2024 at 04:26:38AM -0500, Jeff King wrote:
>
>> IMHO this is the trickiest commit of the whole series, as it would be
>> easy to get the length computations subtly wrong.
>
> And sure enough...
>
>> diff --git a/trailer.c b/trailer.c
>> index fe18faf6c5..f59c90b4b5 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>>
>> /* The first paragraph is the title and cannot be trailers */
>> for (s = buf; s < buf + len; s = next_line(s)) {
>> - if (s[0] == comment_line_char)
>> + if (starts_with_mem(s, buf + len - s, comment_line_str))
>> continue;
>> if (is_blank_line(s))
>> break;
>> @@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>> const char **p;
>> ssize_t separator_pos;
>>
>> - if (bol[0] == comment_line_char) {
>> + if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
>> non_trailer_lines += possible_continuation_lines;
>> possible_continuation_lines = 0;
>> continue;
>
> This second hunk needs:
>
> diff --git a/trailer.c b/trailer.c
> index f59c90b4b5..fdb0b8137e 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
> const char **p;
> ssize_t separator_pos;
>
> - if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
> + if (starts_with_mem(bol, buf + len - bol, comment_line_str)) {
> non_trailer_lines += possible_continuation_lines;
> possible_continuation_lines = 0;
> continue;
>
> I was trying to bound the size based on the loop, which is:
>
> for (l = last_line(buf, len);
> l >= end_of_title;
> l = last_line(buf, l)) {
> const char *bol = buf + l;
>
> but I misread "end_of_title" as an upper bound, not a lower one. Which
> makes sense because we're iterating backwards over the lines. So I
> suppose we could bound it by the previous "bol" value. But in practice,
> your prefix won't cross such a boundary anyway, as it won't have a
> newline in it (maybe that's something we should enforce? I guess you
> could set core.commentChar to '\n' even before my series, which would be
> slightly insane).
>
> So just bounding ourselves to "buf + len" seems reasonable, as that
> makes sure we don't step outside the buffer passed into the function.
>
> Curiously, this was found by the sanitizer job in CI, where UBSan
> complains of integer overflow in the pointer computation. I had run with
> both ASan/UBSan locally, but just using gcc, which doesn't seem to find
> it (the CI job uses clang). So I'll that to my mental tally of "clang
> seems to be better with sanitizers".
>
> -Peff
^ permalink raw reply
* Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers
From: René Scharfe @ 2024-03-07 19:42 UTC (permalink / raw)
To: Jeff King, git
Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307092638.GK2080210@coredump.intra.peff.net>
Am 07.03.24 um 10:26 schrieb Jeff King:
> As with the previous patch, we need to swap out single-byte matching for
> something like starts_with() to match all bytes of a multi-byte comment
> character. But for cases where the buffer is not NUL-terminated (and we
> instead have an explicit size or end pointer), it's not safe to use
> starts_with(), as it might walk off the end of the buffer.
>
> Let's introduce a new starts_with_mem() that does the same thing but
> also accepts the length of the "haystack" str and makes sure not to walk
> past it.
>
> Note that in most cases the existing code did not need a length check at
> all, since it was written in a way that knew we had at least one byte
> available (and that was all we checked). So I had to read each one to
> find the appropriate bounds. The one exception is sequencer.c's
> add_commented_lines(), where we can actually get rid of the length
> check. Just like starts_with(), our starts_with_mem() handles an empty
> haystack variable by not matching (assuming a non-empty prefix).
>
> A few notes on the implementation of starts_with_mem():
>
> - it would be equally correct to take an "end" pointer (and indeed,
> many of the callers have this and have to subtract to come up with
> the length). I think taking a ptr/size combo is a more usual
> interface for our codebase, though, and has the added benefit that
> the function signature makes it harder to mix up the three
> parameters.
>
> - we could obviously build starts_with() on top of this by passing
> strlen(str) as the length. But it's possible that starts_with() is a
> relatively hot code path, and it should not pay that penalty (it can
> generally return an answer proportional to the size of the prefix,
> not the whole string).
>
> - it naively feels like xstrncmpz() should be able to do the same
> thing, but that's not quite true. If you pass the length of the
> haystack buffer, then strncmp() finds that a shorter prefix string
> is "less than" than the haystack, even if the haystack starts with
> the prefix. If you pass the length of the prefix, then you risk
> reading past the end of the haystack if it is shorter than the
> prefix. So I think we really do need a new function.
Yes. xstrncmpz() compares a NUL-terminated string and a length-limited
string. If you want to check whether the former is a prefix of the
latter then you need to stop comparing when reaching its NUL, and also
after exhausting the latter. So you need to take both lengths into
account:
int starts_with_mem(const char *str, size_t len, const char *prefix)
{
size_t prefixlen = strlen(prefix);
return prefixlen <= len && !xstrncmpz(prefix, str, prefixlen);
}
Using memcmp() here is equivalent and simpler:
int starts_with_mem(const char *str, size_t len, const char *prefix)
{
size_t prefixlen = strlen(prefix);
return prefixlen <= len && !memcmp(str, prefix, prefixlen);
}
And your version below avoids function calls and avoids traversing the
strings beyond their common prefix, of course.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Arguably starts_with() and this new function should both be inlined,
> like we do for skip_prefix(), but I think that's out of scope for this
> series.
Inlining would allow the compiler to unroll the loop for string
constants. I doubt it would do that for variables, as in the code
below.
Inlining the strlen()+memcmp() version above might allow the compiler
to push the strlen() call out of a loop.
Would any of that improve performance noticeably? For the call sites
below I doubt it. But it would probably increase the object text size.
> And it's possible I was simply too dumb to figure out xstrncmpz() here.
> I'm waiting for René to show up and tell me how to do it. ;)
Nah, it's not a good fit, as it requires the two strings to have the
same length.
>
> IMHO this is the trickiest commit of the whole series, as it would be
> easy to get the length computations subtly wrong.
>
> commit.c | 3 ++-
> sequencer.c | 4 ++--
> strbuf.c | 11 +++++++++++
> strbuf.h | 1 +
> trailer.c | 4 ++--
> 5 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index ef679a0b93..531a666cba 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1796,7 +1796,8 @@ size_t ignored_log_message_bytes(const char *buf, size_t len)
> else
> next_line++;
>
> - if (buf[bol] == comment_line_char || buf[bol] == '\n') {
> + if (starts_with_mem(buf + bol, cutoff - bol, comment_line_str) ||
> + buf[bol] == '\n') {
> /* is this the first of the run of comments? */
> if (!boc)
> boc = bol;
> diff --git a/sequencer.c b/sequencer.c
> index 991a2dbe96..664986e3b2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1840,7 +1840,7 @@ static int is_fixup_flag(enum todo_command command, unsigned flag)
> static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
> {
> const char *s = str;
> - while (len > 0 && s[0] == comment_line_char) {
> + while (starts_with_mem(s, len, comment_line_str)) {
> size_t count;
> const char *n = memchr(s, '\n', len);
> if (!n)
> @@ -2562,7 +2562,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
> /* left-trim */
> bol += strspn(bol, " \t");
>
> - if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
> + if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
If the strspn() call is safe (which it is, as the caller expects the
string to be NUL-terminated) then you could use starts_with() here and
avoid the length calculation. But that would also match
comment_line_str values that contain LF, which the _mem version does not
and that's better.
Not sure why lines that start with CR are considered comment lines,
though.
> item->command = TODO_COMMENT;
> item->commit = NULL;
> item->arg_offset = bol - buf;
> diff --git a/strbuf.c b/strbuf.c
> index 7c8f582127..291bdc2a65 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -24,6 +24,17 @@ int istarts_with(const char *str, const char *prefix)
> return 0;
> }
>
> +int starts_with_mem(const char *str, size_t len, const char *prefix)
> +{
> + const char *end = str + len;
> + for (; ; str++, prefix++) {
> + if (!*prefix)
> + return 1;
> + else if (str == end || *str != *prefix)
> + return 0;
> + }
> +}
So this checks whether a length-limited string has a prefix given as a
NUL-terminated string. I'd have called it mem_starts_with() and have
expected starts_with_mem() to check a NUL-terminated string for a
length-limited prefix (think !strncmp(str, prefix, prefixlen)).
> +
> int skip_to_optional_arg_default(const char *str, const char *prefix,
> const char **arg, const char *def)
> {
> diff --git a/strbuf.h b/strbuf.h
> index 58dddf2777..3156d6ea8c 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -673,6 +673,7 @@ char *xstrfmt(const char *fmt, ...);
>
> int starts_with(const char *str, const char *prefix);
> int istarts_with(const char *str, const char *prefix);
> +int starts_with_mem(const char *str, size_t len, const char *prefix);
>
> /*
> * If the string "str" is the same as the string in "prefix", then the "arg"
> diff --git a/trailer.c b/trailer.c
> index fe18faf6c5..f59c90b4b5 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>
> /* The first paragraph is the title and cannot be trailers */
> for (s = buf; s < buf + len; s = next_line(s)) {
> - if (s[0] == comment_line_char)
> + if (starts_with_mem(s, buf + len - s, comment_line_str))
> continue;
> if (is_blank_line(s))
Another case where starts_with() would be safe to use, as
is_blank_line() expects (and gets) a NUL-terminated string, but it would
allow matching comment_line_str values that contain LF.
> break;
> @@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
> const char **p;
> ssize_t separator_pos;
>
> - if (bol[0] == comment_line_char) {
> + if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
We're in the same buffer, so the above comment applies here as well.
> non_trailer_lines += possible_continuation_lines;
> possible_continuation_lines = 0;
> continue;
^ permalink raw reply
* Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers
From: René Scharfe @ 2024-03-07 19:47 UTC (permalink / raw)
To: Jeff King, git
Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <742ed8e6-d68b-47a0-8d57-731716c5365d@web.de>
Am 07.03.24 um 20:41 schrieb René Scharfe:
Sorry, sent too early.
> Am 07.03.24 um 12:08 schrieb Jeff King:
>> On Thu, Mar 07, 2024 at 04:26:38AM -0500, Jeff King wrote:
>>
>>> IMHO this is the trickiest commit of the whole series, as it would be
>>> easy to get the length computations subtly wrong.
>>
>> And sure enough...
>>
>>> diff --git a/trailer.c b/trailer.c
>>> index fe18faf6c5..f59c90b4b5 100644
>>> --- a/trailer.c
>>> +++ b/trailer.c
>>> @@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>>>
>>> /* The first paragraph is the title and cannot be trailers */
>>> for (s = buf; s < buf + len; s = next_line(s)) {
>>> - if (s[0] == comment_line_char)
>>> + if (starts_with_mem(s, buf + len - s, comment_line_str))
>>> continue;
>>> if (is_blank_line(s))
>>> break;
>>> @@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>>> const char **p;
>>> ssize_t separator_pos;
>>>
>>> - if (bol[0] == comment_line_char) {
>>> + if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
>>> non_trailer_lines += possible_continuation_lines;
>>> possible_continuation_lines = 0;
>>> continue;
>>
>> This second hunk needs:
>>
>> diff --git a/trailer.c b/trailer.c
>> index f59c90b4b5..fdb0b8137e 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>> const char **p;
>> ssize_t separator_pos;
>>
>> - if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
>> + if (starts_with_mem(bol, buf + len - bol, comment_line_str)) {
>> non_trailer_lines += possible_continuation_lines;
>> possible_continuation_lines = 0;
>> continue;
>>
>> I was trying to bound the size based on the loop, which is:
>>
>> for (l = last_line(buf, len);
>> l >= end_of_title;
>> l = last_line(buf, l)) {
>> const char *bol = buf + l;
>>
>> but I misread "end_of_title" as an upper bound, not a lower one. Which
>> makes sense because we're iterating backwards over the lines. So I
>> suppose we could bound it by the previous "bol" value. But in practice,
>> your prefix won't cross such a boundary anyway, as it won't have a
>> newline in it (maybe that's something we should enforce? I guess you
>> could set core.commentChar to '\n' even before my series, which would be
>> slightly insane).
>>
>> So just bounding ourselves to "buf + len" seems reasonable, as that
>> makes sure we don't step outside the buffer passed into the function.
If you don't want or expect LF in comment_line_str, better check it.
And if you do that, most callers of starts_with_mem() -- including this
one -- can use starts_with() instead, as mentioned in my reply to your
patch. Less calculations, less errors..
René
^ permalink raw reply
* [PATCH 0/3] format-patch: teach `--header-cmd`
From: Kristoffer Haugsbakk @ 2024-03-07 19:59 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Jeff King, Maxim Cournoyer
(most of this is from the main commit with some elaboration in parts)
Teach git-format-patch(1) `--header-cmd` (with negation) and the
accompanying config variable `format.headerCmd` which allows the user to
add extra headers per-patch.
§ Motivation
format-patch knows `--add-header`. However, that seems most useful for
series-wide headers; you cannot really control what the header is like
per patch or specifically for the cover letter. To that end, teach
format-patch a new option which runs a command that has access to the
hash of the current commit (if it is a code patch) and the patch count
which is used for the patch files that this command outputs. Also
include an environment variable which tells the version of this API so
that the command can detect and error out in case the API changes.
This is inspired by `--header-cmd` of git-send-email(1).
§ Discussion
One may already get the commit hash from the commit by looking at the message id created by format-patch:
Message-ID: <48c517ffb3dce2188aba5f0a2c1f4f9dc8df59f0.1709840255.git.code@khaugsbakk.name>
However that does not seem to be documented behavior, and relying on the
message id from various tools seems to have backfired before.[1]
It’s also more convenient to not have to parse any output from
format-patch.
One may also be interested in finding some information based on the
commit hash, not just simply to output the hash itself.
🔗 1: https://lore.kernel.org/git/20231106153214.s5abourejkuiwk64@pengutronix.de/
For example, the command could output a header for the current commit as
well as another header for the previously-published commits:
X-Commit-Hash: 97b53c04894578b23d0c650f69885f734699afc7
X-Previous-Commits:
4ad5d4190649dcb5f26c73a6f15ab731891b9dfd
d275d1d179b90592ddd7b5da2ae4573b3f7a37b7
402b7937951073466bf4527caffd38175391c7da
One can imagine that (1) these previous commit hashes were stored on every
commit rewrite and (2) the commits that had been published previously
were also stored. Then the command just needed the current commit hash
in order to look up this information.
Now interested parties can use this information to track where the
patches come from.
This information could of course be given between the
three-dash/three-hyphen line and the patch proper. However, the
hypoethetical project in question might prefer to use this part for
extra patch information written by the author and leave the above
information for tooling; this way the extra information does not need to
disturb the reader.
§ Demonstration
The above current/previous hash example is taken from:
https://lore.kernel.org/git/97b53c04894578b23d0c650f69885f734699afc7.1709670287.git.code@khaugsbakk.name/
§ CC
For patch “log-tree: take ownership of pointer”:
Cc: Jeff King <peff@peff.net>
For the git-send-email(1) `--header-cmd` topic:[1]
Cc: Maxim Cournoyer <maxim.cournoyer@gmail.com>
🔗 1: https://lore.kernel.org/git/20230423122744.4865-1-maxim.cournoyer@gmail.com/
Kristoffer Haugsbakk (3):
log-tree: take ownership of pointer
format-patch: teach `--header-cmd`
format-patch: check if header output looks valid
Documentation/config/format.txt | 5 ++
Documentation/git-format-patch.txt | 26 ++++++++++
builtin/log.c | 76 ++++++++++++++++++++++++++++++
log-tree.c | 18 ++++++-
revision.h | 2 +
t/t4014-format-patch.sh | 55 +++++++++++++++++++++
6 files changed, 180 insertions(+), 2 deletions(-)
--
2.44.0.169.gd259cac85a8
^ permalink raw reply
* [PATCH 1/3] log-tree: take ownership of pointer
From: Kristoffer Haugsbakk @ 2024-03-07 19:59 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709841147.git.code@khaugsbakk.name>
The MIME header handling started using string buffers in
d50b69b868d (log_write_email_headers: use strbufs, 2018-05-18). The
subject buffer is given to `extra_headers` without that variable taking
ownership; the commit “punts on that ownership” (in general, not just
for this buffer).
In an upcoming commit we will first assign `extra_headers` to the owned
pointer from another `strbuf`. In turn we need this variable to always
contain an owned pointer so that we can free it in the calling
function.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
log-tree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/log-tree.c b/log-tree.c
index 337b9334cdb..2eabd19962b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -519,7 +519,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
extra_headers ? extra_headers : "",
mime_boundary_leader, opt->mime_boundary,
mime_boundary_leader, opt->mime_boundary);
- extra_headers = subject_buffer.buf;
+ extra_headers = strbuf_detach(&subject_buffer, NULL);
if (opt->numbered_files)
strbuf_addf(&filename, "%d", opt->nr);
--
2.44.0.169.gd259cac85a8
^ permalink raw reply related
* [PATCH 2/3] format-patch: teach `--header-cmd`
From: Kristoffer Haugsbakk @ 2024-03-07 19:59 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709841147.git.code@khaugsbakk.name>
Teach git-format-patch(1) `--header-cmd` (with negation) and the
accompanying config variable `format.headerCmd` which allows the user to
add extra headers per-patch.
format-patch knows `--add-header`. However, that seems most useful for
series-wide headers; you cannot really control what the header is like
per patch or specifically for the cover letter. To that end, teach
format-patch a new option which runs a command that has access to the
hash of the current commit (if it is a code patch) and the patch count
which is used for the patch files that this command outputs. Also
include an environment variable which tells the version of this API so
that the command can detect and error out in case the API changes.
This is inspired by `--header-cmd` of git-send-email(1).
§ Discussion
The command can use the provided commit hash to provide relevant
information in the header. For example, the command could output a
header for the current commit as well as the previously-published
commits:
X-Commit-Hash: 97b53c04894578b23d0c650f69885f734699afc7
X-Previous-Commits:
4ad5d4190649dcb5f26c73a6f15ab731891b9dfd
d275d1d179b90592ddd7b5da2ae4573b3f7a37b7
402b7937951073466bf4527caffd38175391c7da
Now interested parties can use this information to track where the
patches come from.
This information could of course be given between the
three-dash/three-hyphen line and the patch proper. However, the project
might prefer to use this part for extra patch information written by the
author and leave the above information for tooling; this way the extra
information does not need to disturb the reader.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
Documentation/config/format.txt:
• I get the impression that `_` is the convention for placeholders now:
`_<cmd>_`
Documentation/config/format.txt | 5 ++++
Documentation/git-format-patch.txt | 26 ++++++++++++++++++
builtin/log.c | 43 ++++++++++++++++++++++++++++++
log-tree.c | 16 ++++++++++-
revision.h | 2 ++
t/t4014-format-patch.sh | 42 +++++++++++++++++++++++++++++
6 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index 7410e930e53..c184b865824 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -31,6 +31,11 @@ format.headers::
Additional email headers to include in a patch to be submitted
by mail. See linkgit:git-format-patch[1].
+format.headerCmd::
+ Command to run for each patch that should output RFC 2822 email
+ headers. Has access to some information per patch via
+ environment variables. See linkgit:git-format-patch[1].
+
format.to::
format.cc::
Additional recipients to include in a patch to be submitted
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 728bb3821c1..41c344902e9 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -303,6 +303,32 @@ feeding the result to `git send-email`.
`Cc:`, and custom) headers added so far from config or command
line.
+--[no-]header-cmd=<cmd>::
+ Run _<cmd>_ for each patch. _<cmd>_ should output valid RFC 2822
+ email headers. This can also be configured with
+ the configuration variable `format.headerCmd`. Can be turned off
+ with `--no-header-cmd`. This works independently of
+ `--[no-]add-header`.
++
+_<cmd>_ has access to these environment variables:
++
+ GIT_FP_HEADER_CMD_VERSION
++
+The version of this API. Currently `1`. _<cmd>_ may return exit code
+`2` in order to signal that it does not support the given version.
++
+ GIT_FP_HEADER_CMD_HASH
++
+The hash of the commit corresponding to the current patch. Not set if
+the current patch is the cover letter.
++
+ GIT_FP_HEADER_CMD_COUNT
++
+The current patch count. Increments for each patch.
++
+`git format-patch` will error out if _<cmd>_ returns a non-zero exit
+code.
+
--[no-]cover-letter::
In addition to the patches, generate a cover letter file
containing the branch description, shortlog and the overall diffstat. You can
diff --git a/builtin/log.c b/builtin/log.c
index db1808d7c13..eecbcdf1d6d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -43,10 +43,13 @@
#include "tmp-objdir.h"
#include "tree.h"
#include "write-or-die.h"
+#include "run-command.h"
#define MAIL_DEFAULT_WRAP 72
#define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
#define FORMAT_PATCH_NAME_MAX_DEFAULT 64
+#define HC_VERSION "1"
+#define HC_NOT_SUPPORTED 2
/* Set a default date-time format for git log ("log.date" config variable) */
static const char *default_date_mode = NULL;
@@ -902,6 +905,7 @@ static int auto_number = 1;
static char *default_attach = NULL;
+static const char *header_cmd = NULL;
static struct string_list extra_hdr = STRING_LIST_INIT_NODUP;
static struct string_list extra_to = STRING_LIST_INIT_NODUP;
static struct string_list extra_cc = STRING_LIST_INIT_NODUP;
@@ -1100,6 +1104,8 @@ static int git_format_config(const char *var, const char *value,
format_no_prefix = 1;
return 0;
}
+ if (!strcmp(var, "format.headercmd"))
+ return git_config_string(&header_cmd, var, value);
/*
* ignore some porcelain config which would otherwise be parsed by
@@ -1419,6 +1425,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
show_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);
strvec_clear(&other_arg);
}
+ free((char *)pp.after_subject);
}
static char *clean_message_id(const char *msg_id)
@@ -1865,6 +1872,35 @@ static void infer_range_diff_ranges(struct strbuf *r1,
}
}
+/* Returns an owned pointer */
+static char *header_cmd_output(struct rev_info *rev, const struct commit *cmit)
+{
+ struct child_process header_cmd_proc = CHILD_PROCESS_INIT;
+ struct strbuf output = STRBUF_INIT;
+ int res;
+
+ strvec_pushl(&header_cmd_proc.args, header_cmd, NULL);
+ if (cmit)
+ strvec_pushf(&header_cmd_proc.env, "GIT_FP_HEADER_CMD_HASH=%s",
+ oid_to_hex(&cmit->object.oid));
+ strvec_pushl(&header_cmd_proc.env,
+ "GIT_FP_HEADER_CMD_VERSION=" HC_VERSION, NULL);
+ strvec_pushf(&header_cmd_proc.env, "GIT_FP_HEADER_CMD_COUNT=%" PRIuMAX,
+ (uintmax_t)rev->nr);
+ res = capture_command(&header_cmd_proc, &output, 0);
+ if (res) {
+ if (res == HC_NOT_SUPPORTED)
+ die(_("header-cmd %s: returned exit "
+ "code %d; the command does not support "
+ "version " HC_VERSION),
+ header_cmd, HC_NOT_SUPPORTED);
+ else
+ die(_("header-cmd %s: failed with exit code %d"),
+ header_cmd, res);
+ }
+ return strbuf_detach(&output, NULL);
+}
+
int cmd_format_patch(int argc, const char **argv, const char *prefix)
{
struct commit *commit;
@@ -1955,6 +1991,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
OPT_GROUP(N_("Messaging")),
OPT_CALLBACK(0, "add-header", NULL, N_("header"),
N_("add email header"), header_callback),
+ OPT_STRING(0, "header-cmd", &header_cmd, N_("email"), N_("command that will be run to generate headers")),
OPT_STRING_LIST(0, "to", &extra_to, N_("email"), N_("add To: header")),
OPT_STRING_LIST(0, "cc", &extra_cc, N_("email"), N_("add Cc: header")),
OPT_CALLBACK_F(0, "from", &from, N_("ident"),
@@ -2321,6 +2358,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (cover_letter) {
if (thread)
gen_message_id(&rev, "cover");
+ if (header_cmd)
+ rev.pe_headers = header_cmd_output(&rev, NULL);
make_cover_letter(&rev, !!output_directory,
origin, nr, list, description_file, branch_name, quiet);
print_bases(&bases, rev.diffopt.file);
@@ -2330,6 +2369,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
/* interdiff/range-diff in cover-letter; omit from patches */
rev.idiff_oid1 = NULL;
rev.rdiff1 = NULL;
+ free((char *)rev.pe_headers);
}
rev.add_signoff = do_signoff;
@@ -2376,6 +2416,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
gen_message_id(&rev, oid_to_hex(&commit->object.oid));
}
+ if (header_cmd)
+ rev.pe_headers = header_cmd_output(&rev, commit);
if (output_directory &&
open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
die(_("failed to create output files"));
@@ -2402,6 +2444,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
}
if (output_directory)
fclose(rev.diffopt.file);
+ free((char *)rev.pe_headers);
}
stop_progress(&progress);
free(list);
diff --git a/log-tree.c b/log-tree.c
index 2eabd19962b..3ca383d099f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -469,12 +469,24 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
}
}
+static char *extra_and_pe_headers(const char *extra_headers, const char *pe_headers) {
+ struct strbuf all_headers = STRBUF_INIT;
+
+ if (extra_headers)
+ strbuf_addstr(&all_headers, extra_headers);
+ if (pe_headers) {
+ strbuf_addstr(&all_headers, pe_headers);
+ }
+ return strbuf_detach(&all_headers, NULL);
+}
+
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **extra_headers_p,
int *need_8bit_cte_p,
int maybe_multipart)
{
- const char *extra_headers = opt->extra_headers;
+ const char *extra_headers =
+ extra_and_pe_headers(opt->extra_headers, opt->pe_headers);
const char *name = oid_to_hex(opt->zero_commit ?
null_oid() : &commit->object.oid);
@@ -519,6 +531,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
extra_headers ? extra_headers : "",
mime_boundary_leader, opt->mime_boundary,
mime_boundary_leader, opt->mime_boundary);
+ free((char *)extra_headers);
extra_headers = strbuf_detach(&subject_buffer, NULL);
if (opt->numbered_files)
@@ -857,6 +870,7 @@ void show_log(struct rev_info *opt)
strbuf_release(&msgbuf);
free(ctx.notes_message);
+ free((char *)ctx.after_subject);
if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {
struct diff_queue_struct dq;
diff --git a/revision.h b/revision.h
index 94c43138bc3..eb36bdea36e 100644
--- a/revision.h
+++ b/revision.h
@@ -291,6 +291,8 @@ struct rev_info {
struct string_list *ref_message_ids;
int add_signoff;
const char *extra_headers;
+ /* per-email headers */
+ const char *pe_headers;
const char *log_reencode;
const char *subject_prefix;
int patch_name_max;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index e37a1411ee2..dfda21d4b2b 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -238,6 +238,48 @@ test_expect_failure 'configuration To: header (rfc2047)' '
grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs9
'
+test_expect_success '--header-cmd' '
+ write_script cmd <<-\EOF &&
+ printf "X-S: $GIT_FP_HEADER_CMD_HASH\n"
+ printf "X-V: $GIT_FP_HEADER_CMD_VERSION\n"
+ printf "X-C: $GIT_FP_HEADER_CMD_COUNT\n"
+ EOF
+ expect_sha1=$(git rev-parse side) &&
+ git format-patch --header-cmd=./cmd --stdout main..side >patch &&
+ grep "^X-S: $expect_sha1" patch &&
+ grep "^X-V: 1" patch &&
+ grep "^X-C: 3" patch
+'
+
+test_expect_success '--header-cmd with no output works' '
+ write_script cmd <<-\EOF &&
+ exit 0
+ EOF
+ git format-patch --header-cmd=./cmd --stdout main..side
+'
+
+test_expect_success '--header-cmd reports failed command' '
+ write_script cmd <<-\EOF &&
+ exit 1
+ EOF
+ cat > expect <<-\EOF &&
+ fatal: header-cmd ./cmd: failed with exit code 1
+ EOF
+ test_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&
+ test_cmp expect actual
+'
+
+test_expect_success '--header-cmd reports exit code 2' '
+ write_script cmd <<-\EOF &&
+ exit 2
+ EOF
+ cat > expect <<-\EOF &&
+ fatal: header-cmd ./cmd: returned exit code 2; the command does not support version 1
+ EOF
+ test_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&
+ test_cmp expect actual
+'
+
# check_patch <patch>: Verify that <patch> looks like a half-sane
# patch email to avoid a false positive with !grep
check_patch () {
--
2.44.0.169.gd259cac85a8
^ 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