* [PATCH 04/10] trailer: delete obsolete formatting functions
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 79 -------------------------------------------------------
1 file changed, 79 deletions(-)
diff --git a/trailer.c b/trailer.c
index 315d90ee1ab..132f22b3dd7 100644
--- a/trailer.c
+++ b/trailer.c
@@ -144,24 +144,6 @@ static char last_non_space_char(const char *s)
return '\0';
}
-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
-{
- char c;
-
- if (!tok) {
- fprintf(outfile, "%s\n", val);
- return;
- }
-
- c = last_non_space_char(tok);
- if (!c)
- return;
- if (strchr(separators, c))
- fprintf(outfile, "%s%s\n", tok, val);
- else
- fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
-}
-
static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
{
struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
@@ -1142,67 +1124,6 @@ void trailer_info_release(struct trailer_info *info)
free(info->trailers);
}
-static void format_trailer_info(struct strbuf *out,
- const struct trailer_info *info,
- const char *msg,
- const struct process_trailer_options *opts)
-{
- size_t origlen = out->len;
- size_t i;
-
- /* If we want the whole block untouched, we can take the fast path. */
- if (!opts->only_trailers && !opts->unfold && !opts->filter &&
- !opts->separator && !opts->key_only && !opts->value_only &&
- !opts->key_value_separator) {
- strbuf_add(out, msg + info->trailer_block_start,
- info->trailer_block_end - info->trailer_block_start);
- return;
- }
-
- for (i = 0; i < info->trailer_nr; i++) {
- char *trailer = info->trailers[i];
- ssize_t separator_pos = find_separator(trailer, separators);
-
- if (separator_pos >= 1) {
- struct strbuf tok = STRBUF_INIT;
- struct strbuf val = STRBUF_INIT;
-
- parse_trailer(&tok, &val, NULL, trailer, separator_pos);
- if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
- if (opts->unfold)
- unfold_value(&val);
-
- if (opts->separator && out->len != origlen)
- strbuf_addbuf(out, opts->separator);
- if (!opts->value_only)
- strbuf_addbuf(out, &tok);
- if (!opts->key_only && !opts->value_only) {
- if (opts->key_value_separator)
- strbuf_addbuf(out, opts->key_value_separator);
- else
- strbuf_addstr(out, ": ");
- }
- if (!opts->key_only)
- strbuf_addbuf(out, &val);
- if (!opts->separator)
- strbuf_addch(out, '\n');
- }
- strbuf_release(&tok);
- strbuf_release(&val);
-
- } else if (!opts->only_trailers) {
- if (opts->separator && out->len != origlen) {
- strbuf_addbuf(out, opts->separator);
- }
- strbuf_addstr(out, trailer);
- if (opts->separator) {
- strbuf_rtrim(out);
- }
- }
- }
-
-}
-
void format_trailers_from_commit(const char *msg,
const struct process_trailer_options *opts,
struct strbuf *out)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 03/10] trailer: unify trailer formatting machinery
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Currently have two functions for formatting trailers exposed in
trailer.h:
void format_trailers(FILE *outfile, struct list_head *head,
const struct process_trailer_options *opts);
void format_trailers_from_commit(struct strbuf *out, const char *msg,
const struct process_trailer_options *opts);
and previously these functions, although similar enough (even taking the
same process_trailer_options struct pointer), did not build on each
other.
Make format_trailers_from_commit() rely on format_trailers(). Teach
format_trailers() to process trailers with the additional
process_trailer_options fields like opts->key_only which is only used by
format_trailers_from_commit() and not builtin/interpret-trailers.c.
This will allow us to delete the format_trailer_info() and
print_tok_val() functions in the next patch. They are not deleted here
in order to keep the diff small.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 5 +-
pretty.c | 2 +-
ref-filter.c | 2 +-
trailer.c | 105 +++++++++++++++++++++++++++++------
trailer.h | 21 +++----
5 files changed, 102 insertions(+), 33 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index adb74276281..934833a4645 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -140,6 +140,7 @@ static void interpret_trailers(const char *file,
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
+ struct strbuf trailer_block = STRBUF_INIT;
struct trailer_info info;
FILE *outfile = stdout;
@@ -170,7 +171,9 @@ static void interpret_trailers(const char *file,
}
/* Print trailer block. */
- format_trailers(outfile, &head, opts);
+ format_trailers(&head, opts, &trailer_block);
+ fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
+ strbuf_release(&trailer_block);
free_trailers(&head);
trailer_info_release(&info);
diff --git a/pretty.c b/pretty.c
index cf964b060cd..f0721a5214f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
goto trailer_out;
}
if (*arg == ')') {
- format_trailers_from_commit(sb, msg + c->subject_off, &opts);
+ format_trailers_from_commit(msg + c->subject_off, &opts, sb);
ret = arg - placeholder + 1;
}
trailer_out:
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1dfe..7fb13818686 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1985,7 +1985,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
struct strbuf s = STRBUF_INIT;
/* Format the trailer info according to the trailer_opts given */
- format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+ format_trailers_from_commit(subpos, &atom->u.contents.trailer_opts, &s);
v->s = strbuf_detach(&s, NULL);
} else if (atom->u.contents.option == C_BARE)
diff --git a/trailer.c b/trailer.c
index 0ce7e9079ca..315d90ee1ab 100644
--- a/trailer.c
+++ b/trailer.c
@@ -162,19 +162,6 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-void format_trailers(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts)
-{
- struct list_head *pos;
- struct trailer_item *item;
- list_for_each(pos, head) {
- item = list_entry(pos, struct trailer_item, list);
- if ((!opts->trim_empty || strlen(item->value) > 0) &&
- (!opts->only_trailers || item->token))
- print_tok_val(outfile, item->token, item->value);
- }
-}
-
static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
{
struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
@@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val)
strbuf_release(&out);
}
+void format_trailers(struct list_head *head,
+ const struct process_trailer_options *opts,
+ struct strbuf *out)
+{
+ struct list_head *pos;
+ struct trailer_item *item;
+ int need_separator = 0;
+
+ list_for_each(pos, head) {
+ item = list_entry(pos, struct trailer_item, list);
+ if (item->token) {
+ char c;
+
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ strbuf_addstr(&tok, item->token);
+ strbuf_addstr(&val, item->value);
+
+ /*
+ * Skip key/value pairs where the value was empty. This
+ * can happen from trailers specified without a
+ * separator, like `--trailer "Reviewed-by"` (no
+ * corresponding value).
+ */
+ if (opts->trim_empty && !strlen(item->value))
+ continue;
+
+ if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+ if (opts->unfold)
+ unfold_value(&val);
+
+ if (opts->separator && need_separator)
+ strbuf_addbuf(out, opts->separator);
+ if (!opts->value_only)
+ strbuf_addbuf(out, &tok);
+ if (!opts->key_only && !opts->value_only) {
+ if (opts->key_value_separator)
+ strbuf_addbuf(out, opts->key_value_separator);
+ else {
+ c = last_non_space_char(tok.buf);
+ if (c) {
+ if (!strchr(separators, c))
+ strbuf_addf(out, "%c ", separators[0]);
+ }
+ }
+ }
+ if (!opts->key_only)
+ strbuf_addbuf(out, &val);
+ if (!opts->separator)
+ strbuf_addch(out, '\n');
+
+ need_separator = 1;
+ }
+
+ strbuf_release(&tok);
+ strbuf_release(&val);
+ } else if (!opts->only_trailers) {
+ if (opts->separator && need_separator) {
+ strbuf_addbuf(out, opts->separator);
+ }
+ strbuf_addstr(out, item->value);
+ if (opts->separator)
+ strbuf_rtrim(out);
+ else
+ strbuf_addch(out, '\n');
+
+ need_separator = 1;
+ }
+
+ }
+}
+
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
@@ -1144,13 +1203,25 @@ static void format_trailer_info(struct strbuf *out,
}
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts)
+void format_trailers_from_commit(const char *msg,
+ const struct process_trailer_options *opts,
+ struct strbuf *out)
{
+ LIST_HEAD(head);
struct trailer_info info;
- trailer_info_get(&info, msg, opts);
- format_trailer_info(out, &info, msg, opts);
+ parse_trailers(&info, msg, &head, opts);
+
+ /* If we want the whole block untouched, we can take the fast path. */
+ if (!opts->only_trailers && !opts->unfold && !opts->filter &&
+ !opts->separator && !opts->key_only && !opts->value_only &&
+ !opts->key_value_separator) {
+ strbuf_add(out, msg + info.trailer_block_start,
+ info.trailer_block_end - info.trailer_block_start);
+ } else
+ format_trailers(&head, opts, out);
+
+ free_trailers(&head);
trailer_info_release(&info);
}
diff --git a/trailer.h b/trailer.h
index 0e4f0ece9b3..50f70556302 100644
--- a/trailer.h
+++ b/trailer.h
@@ -102,21 +102,16 @@ void trailer_info_release(struct trailer_info *info);
void trailer_config_init(void);
void free_trailers(struct list_head *trailers);
-void format_trailers(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts);
+void format_trailers(struct list_head *head,
+ const struct process_trailer_options *opts,
+ struct strbuf *out);
/*
- * Format the trailers from the commit msg "msg" into the strbuf "out".
- * Note two caveats about "opts":
- *
- * - this is primarily a helper for pretty.c, and not
- * all of the flags are supported.
- *
- * - this differs from format_trailers slightly in that we always format
- * only the trailer block itself, even if the "only_trailers" option is not
- * set.
+ * Convenience function to format the trailers from the commit msg "msg" into
+ * the strbuf "out". Reuses format_trailers internally.
*/
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts);
+void format_trailers_from_commit(const char *msg,
+ const struct process_trailer_options *opts,
+ struct strbuf *out);
/*
* An interface for iterating over the trailers found in a particular commit
--
gitgitgadget
^ permalink raw reply related
* [PATCH 02/10] trailer: include "trailer" term in API functions
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
These functions are exposed to clients and so they should include
"trailer" in their names for easier identification, just like all the
other functions already exposed by trailer.h.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 7 ++++---
trailer.c | 10 +++++-----
trailer.h | 10 +++++-----
3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 444f8fb70c9..adb74276281 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -143,7 +143,7 @@ static void interpret_trailers(const char *file,
struct trailer_info info;
FILE *outfile = stdout;
- ensure_configured();
+ trailer_config_init();
read_input_file(&sb, file);
@@ -169,9 +169,10 @@ static void interpret_trailers(const char *file,
process_trailers_lists(&head, &arg_head);
}
- print_all(outfile, &head, opts);
+ /* Print trailer block. */
+ format_trailers(outfile, &head, opts);
- free_all(&head);
+ free_trailers(&head);
trailer_info_release(&info);
/* Print the lines after the trailers as is */
diff --git a/trailer.c b/trailer.c
index 9d70c9946bd..0ce7e9079ca 100644
--- a/trailer.c
+++ b/trailer.c
@@ -162,8 +162,8 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-void print_all(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts)
+void format_trailers(FILE *outfile, struct list_head *head,
+ const struct process_trailer_options *opts)
{
struct list_head *pos;
struct trailer_item *item;
@@ -588,7 +588,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}
-void ensure_configured(void)
+void trailer_config_init(void)
{
if (configured)
return;
@@ -1023,7 +1023,7 @@ void parse_trailers(struct trailer_info *info,
}
}
-void free_all(struct list_head *head)
+void free_trailers(struct list_head *head)
{
struct list_head *pos, *p;
list_for_each_safe(pos, p, head) {
@@ -1041,7 +1041,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
size_t nr = 0, alloc = 0;
char **last = NULL;
- ensure_configured();
+ trailer_config_init();
end_of_log_message = find_end_of_log_message(str, opts->no_divider);
trailer_block_start = find_trailer_block_start(str, end_of_log_message);
diff --git a/trailer.h b/trailer.h
index b3e4a5e127d..0e4f0ece9b3 100644
--- a/trailer.h
+++ b/trailer.h
@@ -99,11 +99,11 @@ void trailer_info_get(struct trailer_info *info, const char *str,
void trailer_info_release(struct trailer_info *info);
-void ensure_configured(void);
-void print_all(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts);
-void free_all(struct list_head *head);
+void trailer_config_init(void);
+void free_trailers(struct list_head *trailers);
+void format_trailers(FILE *outfile, struct list_head *head,
+ const struct process_trailer_options *opts);
/*
* Format the trailers from the commit msg "msg" into the strbuf "out".
* Note two caveats about "opts":
@@ -111,7 +111,7 @@ void free_all(struct list_head *head);
* - this is primarily a helper for pretty.c, and not
* all of the flags are supported.
*
- * - this differs from process_trailers slightly in that we always format
+ * - this differs from format_trailers slightly in that we always format
* only the trailer block itself, even if the "only_trailers" option is not
* set.
*/
--
gitgitgadget
^ permalink raw reply related
* [PATCH 01/10] trailer: move process_trailers() to interpret-trailers.c
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
The interpret-trailers.c builtin is the only place we need to call
process_trailers(). As it stands, process_trailers() is inherently tied
to how the builtin behaves, so move its definition there.
Delete the corresponding declaration from trailer.h, which then forces
us to expose the working innards of that function. This enriches
trailer.h to include a more granular API, which can then be unit-tested
in the future (because process_trailers() by itself does too many things
to be able to be easily unit-tested).
Take this opportunity to demote some file-handling functions out of the
trailer API implementation, as these have nothing to do with trailers.
While we're at it, rename process_trailers() to interpret_trailers() in
the builtin for consistency with the existing cmd_interpret_trailers(),
which wraps around this function.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 98 +++++++++++++++++++++++++++-
trailer.c | 120 ++++-------------------------------
trailer.h | 20 +++++-
3 files changed, 126 insertions(+), 112 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 033bd1556cf..444f8fb70c9 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -9,6 +9,7 @@
#include "gettext.h"
#include "parse-options.h"
#include "string-list.h"
+#include "tempfile.h"
#include "trailer.h"
#include "config.h"
@@ -91,6 +92,99 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
return 0;
}
+static struct tempfile *trailers_tempfile;
+
+static FILE *create_in_place_tempfile(const char *file)
+{
+ struct stat st;
+ struct strbuf filename_template = STRBUF_INIT;
+ const char *tail;
+ FILE *outfile;
+
+ if (stat(file, &st))
+ die_errno(_("could not stat %s"), file);
+ if (!S_ISREG(st.st_mode))
+ die(_("file %s is not a regular file"), file);
+ if (!(st.st_mode & S_IWUSR))
+ die(_("file %s is not writable by user"), file);
+
+ /* Create temporary file in the same directory as the original */
+ tail = strrchr(file, '/');
+ if (tail)
+ strbuf_add(&filename_template, file, tail - file + 1);
+ strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
+
+ trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
+ strbuf_release(&filename_template);
+ outfile = fdopen_tempfile(trailers_tempfile, "w");
+ if (!outfile)
+ die_errno(_("could not open temporary file"));
+
+ return outfile;
+}
+
+static void read_input_file(struct strbuf *sb, const char *file)
+{
+ if (file) {
+ if (strbuf_read_file(sb, file, 0) < 0)
+ die_errno(_("could not read input file '%s'"), file);
+ } else {
+ if (strbuf_read(sb, fileno(stdin), 0) < 0)
+ die_errno(_("could not read from stdin"));
+ }
+}
+
+static void interpret_trailers(const char *file,
+ const struct process_trailer_options *opts,
+ struct list_head *new_trailer_head)
+{
+ LIST_HEAD(head);
+ struct strbuf sb = STRBUF_INIT;
+ struct trailer_info info;
+ FILE *outfile = stdout;
+
+ ensure_configured();
+
+ read_input_file(&sb, file);
+
+ if (opts->in_place)
+ outfile = create_in_place_tempfile(file);
+
+ parse_trailers(&info, sb.buf, &head, opts);
+
+ /* Print the lines before the trailers */
+ if (!opts->only_trailers)
+ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+
+ if (!opts->only_trailers && !info.blank_line_before_trailer)
+ fprintf(outfile, "\n");
+
+
+ if (!opts->only_input) {
+ LIST_HEAD(config_head);
+ LIST_HEAD(arg_head);
+ parse_trailers_from_config(&config_head);
+ parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
+ list_splice(&config_head, &arg_head);
+ process_trailers_lists(&head, &arg_head);
+ }
+
+ print_all(outfile, &head, opts);
+
+ free_all(&head);
+ trailer_info_release(&info);
+
+ /* Print the lines after the trailers as is */
+ if (!opts->only_trailers)
+ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+
+ if (opts->in_place)
+ if (rename_tempfile(&trailers_tempfile, file))
+ die_errno(_("could not rename temporary file to %s"), file);
+
+ strbuf_release(&sb);
+}
+
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
@@ -132,11 +226,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
if (argc) {
int i;
for (i = 0; i < argc; i++)
- process_trailers(argv[i], &opts, &trailers);
+ interpret_trailers(argv[i], &opts, &trailers);
} else {
if (opts.in_place)
die(_("no input file given for in-place editing"));
- process_trailers(NULL, &opts, &trailers);
+ interpret_trailers(NULL, &opts, &trailers);
}
new_trailers_clear(&trailers);
diff --git a/trailer.c b/trailer.c
index 3a0710a4583..9d70c9946bd 100644
--- a/trailer.c
+++ b/trailer.c
@@ -5,7 +5,6 @@
#include "string-list.h"
#include "run-command.h"
#include "commit.h"
-#include "tempfile.h"
#include "trailer.h"
#include "list.h"
/*
@@ -163,8 +162,8 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void print_all(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts)
+void print_all(FILE *outfile, struct list_head *head,
+ const struct process_trailer_options *opts)
{
struct list_head *pos;
struct trailer_item *item;
@@ -366,8 +365,8 @@ static int find_same_and_apply_arg(struct list_head *head,
return 0;
}
-static void process_trailers_lists(struct list_head *head,
- struct list_head *arg_head)
+void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head)
{
struct list_head *pos, *p;
struct arg_item *arg_tok;
@@ -589,7 +588,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}
-static void ensure_configured(void)
+void ensure_configured(void)
{
if (configured)
return;
@@ -719,7 +718,7 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
list_add_tail(&new_item->list, arg_head);
}
-static void parse_trailers_from_config(struct list_head *config_head)
+void parse_trailers_from_config(struct list_head *config_head)
{
struct arg_item *item;
struct list_head *pos;
@@ -735,8 +734,8 @@ static void parse_trailers_from_config(struct list_head *config_head)
}
}
-static void parse_trailers_from_command_line_args(struct list_head *arg_head,
- struct list_head *new_trailer_head)
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+ struct list_head *new_trailer_head)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -775,17 +774,6 @@ static void parse_trailers_from_command_line_args(struct list_head *arg_head,
free(cl_separators);
}
-static void read_input_file(struct strbuf *sb, const char *file)
-{
- if (file) {
- if (strbuf_read_file(sb, file, 0) < 0)
- die_errno(_("could not read input file '%s'"), file);
- } else {
- if (strbuf_read(sb, fileno(stdin), 0) < 0)
- die_errno(_("could not read from stdin"));
- }
-}
-
static const char *next_line(const char *str)
{
const char *nl = strchrnul(str, '\n');
@@ -1000,10 +988,10 @@ static void unfold_value(struct strbuf *val)
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
*/
-static void parse_trailers(struct trailer_info *info,
- const char *str,
- struct list_head *head,
- const struct process_trailer_options *opts)
+void parse_trailers(struct trailer_info *info,
+ const char *str,
+ struct list_head *head,
+ const struct process_trailer_options *opts)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -1035,7 +1023,7 @@ static void parse_trailers(struct trailer_info *info,
}
}
-static void free_all(struct list_head *head)
+void free_all(struct list_head *head)
{
struct list_head *pos, *p;
list_for_each_safe(pos, p, head) {
@@ -1044,88 +1032,6 @@ static void free_all(struct list_head *head)
}
}
-static struct tempfile *trailers_tempfile;
-
-static FILE *create_in_place_tempfile(const char *file)
-{
- struct stat st;
- struct strbuf filename_template = STRBUF_INIT;
- const char *tail;
- FILE *outfile;
-
- if (stat(file, &st))
- die_errno(_("could not stat %s"), file);
- if (!S_ISREG(st.st_mode))
- die(_("file %s is not a regular file"), file);
- if (!(st.st_mode & S_IWUSR))
- die(_("file %s is not writable by user"), file);
-
- /* Create temporary file in the same directory as the original */
- tail = strrchr(file, '/');
- if (tail)
- strbuf_add(&filename_template, file, tail - file + 1);
- strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
-
- trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
- strbuf_release(&filename_template);
- outfile = fdopen_tempfile(trailers_tempfile, "w");
- if (!outfile)
- die_errno(_("could not open temporary file"));
-
- return outfile;
-}
-
-void process_trailers(const char *file,
- const struct process_trailer_options *opts,
- struct list_head *new_trailer_head)
-{
- LIST_HEAD(head);
- struct strbuf sb = STRBUF_INIT;
- struct trailer_info info;
- FILE *outfile = stdout;
-
- ensure_configured();
-
- read_input_file(&sb, file);
-
- if (opts->in_place)
- outfile = create_in_place_tempfile(file);
-
- parse_trailers(&info, sb.buf, &head, opts);
-
- /* Print the lines before the trailers */
- if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
-
- if (!opts->only_trailers && !info.blank_line_before_trailer)
- fprintf(outfile, "\n");
-
-
- if (!opts->only_input) {
- LIST_HEAD(config_head);
- LIST_HEAD(arg_head);
- parse_trailers_from_config(&config_head);
- parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
- list_splice(&config_head, &arg_head);
- process_trailers_lists(&head, &arg_head);
- }
-
- print_all(outfile, &head, opts);
-
- free_all(&head);
- trailer_info_release(&info);
-
- /* Print the lines after the trailers as is */
- if (!opts->only_trailers)
- fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
-
- if (opts->in_place)
- if (rename_tempfile(&trailers_tempfile, file))
- die_errno(_("could not rename temporary file to %s"), file);
-
- strbuf_release(&sb);
-}
-
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
diff --git a/trailer.h b/trailer.h
index 1644cd05f60..b3e4a5e127d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -81,15 +81,29 @@ struct process_trailer_options {
#define PROCESS_TRAILER_OPTIONS_INIT {0}
-void process_trailers(const char *file,
- const struct process_trailer_options *opts,
- struct list_head *new_trailer_head);
+void parse_trailers_from_config(struct list_head *config_head);
+
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+ struct list_head *new_trailer_head);
+
+void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head);
+
+void parse_trailers(struct trailer_info *info,
+ const char *str,
+ struct list_head *head,
+ const struct process_trailer_options *opts);
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts);
void trailer_info_release(struct trailer_info *info);
+void ensure_configured(void);
+void print_all(FILE *outfile, struct list_head *head,
+ const struct process_trailer_options *opts);
+void free_all(struct list_head *head);
+
/*
* Format the trailers from the commit msg "msg" into the strbuf "out".
* Note two caveats about "opts":
--
gitgitgadget
^ permalink raw reply related
* [PATCH 00/10] Enrich Trailer API
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver
This patch series is the first 10 patches of a much larger series I've been
working. The main goal of this series is to enrich the API in trailer.h. The
larger series brings a number of additional code simplifications and
cleanups (exposing and fixing some bugs along the way), and builds on top of
this series. The goal of the larger series is to make the trailer interface
ready for unit testing. By "trailer API" I mean those functions exposed in
trailer.h.
Currently, we have "process_trailers()" in trailer.h which does many
different things (parse command-line arguments, create temporary files, etc)
that are independent of the concept of "trailers", whose interface trailer.h
defines. This makes unit-testing this function difficult. While there is no
technical reason why we couldn't write unit tests for the smaller functions
that are called within process_trailers(), doing so would involve testing
private ("static" in trailer.c) functions instead of the public functions
exposed in trailer.h which authoritatively define the API.
As an alternative to this patch series, we could keep trailer.h intact and
decide to unit-test the existing "trailer_info_get()" function which does
most of the trailer parsing work. However this function wouldn't be easy to
test either, because the resulting data structure merely contains the
unparsed "trailers" lines. So the unit test (if it wants to inspect the
result of parsing these lines) would have to invoke additional parsing
functions itself. And at that point it would not longer be a unit test in
the traditional sense, because it would be invoking multiple functions at
once.
This series breaks up "process_trailers()" into smaller pieces, exposing
many of the parts relevant to trailer-related processing in trailer.h. This
forces us to start writing unit tests for these now public functions, but
that is a good thing because those same unit tests should be easy to write
(due to their small(er) sizes), but also, because those unit tests will now
ensure some degree of stability across new versions of trailer.h (we will
start noticing when the behavior of any of these API functions change).
Another benefit is that more clients (perhaps those outside of Git in the
future) will be able to use the same trailer processing algorithms used by
the interpret-trailers builtin. For example, a web server may want to parse
trailers the same way that interpret-trailers would parse them, without
having to shell out to interpret-trailers. Importing the new (richer)
trailer.h file that this series promotes would help them achieve that goal.
This use case was the original motivation behind my work in this area of
Git.
Thanks to the aggressive refactoring in this series, I've been able to
identify and fix a couple bugs in our existing implementation. Those fixes
build on top of this series but were not included here, in order to keep
this series small.
Linus Arver (10):
trailer: move process_trailers() to interpret-trailers.c
trailer: include "trailer" term in API functions
trailer: unify trailer formatting machinery
trailer: delete obsolete formatting functions
sequencer: use the trailer iterator
trailer: make trailer_info struct private
trailer: spread usage of "trailer_block" language
trailer: prepare to move parse_trailers_from_command_line_args() to
builtin
trailer: move arg handling to interpret-trailers.c
trailer: delete obsolete argument handling code from API
builtin/interpret-trailers.c | 169 ++++++++--
builtin/shortlog.c | 7 +-
pretty.c | 2 +-
ref-filter.c | 2 +-
sequencer.c | 35 +--
trailer.c | 579 ++++++++++++++++-------------------
trailer.h | 106 ++++---
7 files changed, 482 insertions(+), 418 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1632%2Flistx%2Ftrailer-api-refactor-part-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1632
--
gitgitgadget
^ permalink raw reply
* Request fixing a vulnerability for git project
From: Reika Arakawa @ 2024-01-10 6:47 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: vuls@jpcert.or.jp
[-- Attachment #1.1: Type: text/plain, Size: 1601 bytes --]
To Whom it May Concern: git Product Development Manager,
We are a cybersecurity research team affiliated with NTT in Japan, and are engaged in research related to vulnerability detection, analysis, and modification.
We investigated git (https://github.com/git/git) released as OSS. A known vulnerability and known code were found, so please fix them with a patch. We hope to prevent cyber-attacks from occurring by working together with Japan's representative, JPCERT/CC (vuls@jpcert.or.jp<mailto:vuls@jpcert.or.jp>).
How to discover vulnerabilities:
We inspected all function codes for OSS software code using patch information and code information (2000-2023) for known vulnerabilities. We would like to inform you of the code information for which there is a high possibility that a public patch has not been applied with high severity. Vulnerability code information and countermeasures are listed in the attached file ‘git@@git_vulnerability_details.txt’.
Please note that in the real world, a vulnerable state may not necessarily be true, as it may become a vulnerable state under certain combinations of conditions in the real world.
Additional confirmation details:
We are planning to submit a paper on this content between January and March, and we would like to include the source code characteristics in the paper without revealing the OSS name or file name. Are there any concerns about this?
--
Best regards, NTT Social Informatics Laboratories（ https://www.rd.ntt/e/sil/）
E-mail: reika.arakawa@ntt.com<mailto:reika.arakawa@ntt.com>
[-- Attachment #1.2: Type: text/html, Size: 5350 bytes --]
[-- Attachment #2: git@@git_vulnerability_details.txt --]
[-- Type: text/plain, Size: 370 bytes --]
Software name：git@@git
Version tag：v2.43.0
How to fix: Search by CVE number, refer to the commit listed in reference on the NVD/CVE site, and fix it.
NVD:https://nvd.nist.gov/vuln/detail/CVE-2009-5155
â‘ git@@git/compat/regex/regcomp.c [parse_reg_exp]
・CVE-2009-5155，CWE-19_5.0_0
・git.savannah.gnu.org##gnulib_regcomp.c, [parse_reg_exp]
^ permalink raw reply
* Re: [PATCH 3/3] submodule-config.c: strengthen URL fsck check
From: Patrick Steinhardt @ 2024-01-10 6:33 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <893071530d3b77d6b72b7f69a6dfb9947579865e.1704822817.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]
On Tue, Jan 09, 2024 at 05:53:37PM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Update the validation of "curl URL" submodule URLs (i.e. those that specify
> an "http[s]" or "ftp[s]" protocol) in 'check_submodule_url()' to catch more
> invalid URLs. The existing validation using 'credential_from_url_gently()'
> parses certain URLs incorrectly, leading to invalid submodule URLs passing
> 'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote
> URLs in 'remote_get()' - correctly identifies the invalid URLs missed by
> 'credential_from_url_gently()'.
Okay, so we retain the wrong behavior of `credential_from_url_gently()`,
right? I wonder whether this can be abused in any way, doubly so because
the function gets invoked with untrusted input from the remote server
when we handle redirects in `http_request_reauth()`. But the redirect
URL we end up passing to `credential_from_url_gently()` would have to
contain a non-numeric port, and curl seemingly does not know to handle
those either.
Other callsites include fsck (which you're fixing) and the credential
store (which is entirely user-controlled). It would be great regardless
to fix the underlying bug in `credential_from_url_gently()` eventually
though. But I do not think that this has to be part this patch series
here, which is a strict improvement.
Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 5/5] completion: custom git-bisect terms
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ad80df6630..87cf7b2561 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1591,13 +1591,22 @@ _git_bisect ()
term_good=`__git bisect terms --term-good`
fi
- local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ # We will complete any custom terms, but still always complete the
+ # more usual bad/new/good/old because git bisect gives a good error
+ # message if these are given when not in use and that's better than
+ # silent refusal to complete if the user is confused.
+ #
+ # We want to recognize 'view' but not complete it, because it overlaps
+ # with 'visualize' too much and is just an alias for it.
+ #
+ local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ local all_subcommands="$completable_subcommands view"
- local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ local subcommand="$(__git_find_on_cmdline "$all_subcommands")"
if [ -z "$subcommand" ]; then
if [ -f "$__git_repo_path"/BISECT_START ]; then
- __gitcomp "$subcommands"
+ __gitcomp "$completable_subcommands"
else
__gitcomp "replay start"
fi
@@ -1615,7 +1624,7 @@ _git_bisect ()
;;
esac
;;
- visualize)
+ visualize|view)
case "$cur" in
-*)
__git_complete_log_opts
--
2.43.0
^ permalink raw reply related
* [PATCH v2 3/5] completion: move to maintain define-before-use
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 269 ++++++++++++-------------
1 file changed, 134 insertions(+), 135 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c16aded36c..63ca8082a4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1445,6 +1445,140 @@ _git_archive ()
__git_complete_file
}
+# Options that go well for log, shortlog and gitk
+__git_log_common_options="
+ --not --all
+ --branches --tags --remotes
+ --first-parent --merges --no-merges
+ --max-count=
+ --max-age= --since= --after=
+ --min-age= --until= --before=
+ --min-parents= --max-parents=
+ --no-min-parents --no-max-parents
+"
+# Options that go well for log and gitk (not shortlog)
+__git_log_gitk_options="
+ --dense --sparse --full-history
+ --simplify-merges --simplify-by-decoration
+ --left-right --notes --no-notes
+"
+# Options that go well for log and shortlog (not gitk)
+__git_log_shortlog_options="
+ --author= --committer= --grep=
+ --all-match --invert-grep
+"
+# Options accepted by log and show
+__git_log_show_options="
+ --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
+"
+
+__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
+
+__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
+__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
+
+# Check for only porcelain (i.e. not git-rev-list) option (not argument)
+# and selected option argument completions for git-log options and if any
+# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
+# and will be empty on return if no candidates are found.
+__git_complete_log_opts ()
+{
+ [ -z "$COMPREPLY" ] || return 1 # Precondition
+
+ local merge=""
+ if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ merge="--merge"
+ fi
+ case "$prev,$cur" in
+ -L,:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L,:*)
+ __git_complete_symbol --cur="${cur#:}" --sfx=":"
+ return
+ ;;
+ -G,*|-S,*)
+ __git_complete_symbol
+ return
+ ;;
+ esac
+ case "$cur" in
+ --pretty=*|--format=*)
+ __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
+ " "" "${cur#*=}"
+ return
+ ;;
+ --date=*)
+ __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
+ return
+ ;;
+ --decorate=*)
+ __gitcomp "full short no" "" "${cur##--decorate=}"
+ return
+ ;;
+ --diff-algorithm=*)
+ __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
+ return
+ ;;
+ --submodule=*)
+ __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
+ return
+ ;;
+ --ws-error-highlight=*)
+ __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
+ return
+ ;;
+ --no-walk=*)
+ __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
+ return
+ ;;
+ --diff-merges=*)
+ __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
+ return
+ ;;
+ --*)
+ __gitcomp "
+ $__git_log_common_options
+ $__git_log_shortlog_options
+ $__git_log_gitk_options
+ $__git_log_show_options
+ --root --topo-order --date-order --reverse
+ --follow --full-diff
+ --abbrev-commit --no-abbrev-commit --abbrev=
+ --relative-date --date=
+ --pretty= --format= --oneline
+ --show-signature
+ --cherry-mark
+ --cherry-pick
+ --graph
+ --decorate --decorate= --no-decorate
+ --walk-reflogs
+ --no-walk --no-walk= --do-walk
+ --parents --children
+ --expand-tabs --expand-tabs= --no-expand-tabs
+ $merge
+ $__git_diff_common_options
+ "
+ return
+ ;;
+ -L:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L:*)
+ __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
+ return
+ ;;
+ -G*)
+ __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
+ return
+ ;;
+ -S*)
+ __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
+ return
+ ;;
+ esac
+}
+
_git_bisect ()
{
__git_has_doubledash && return
@@ -2052,141 +2186,6 @@ _git_ls_tree ()
__git_complete_file
}
-# Options that go well for log, shortlog and gitk
-__git_log_common_options="
- --not --all
- --branches --tags --remotes
- --first-parent --merges --no-merges
- --max-count=
- --max-age= --since= --after=
- --min-age= --until= --before=
- --min-parents= --max-parents=
- --no-min-parents --no-max-parents
-"
-# Options that go well for log and gitk (not shortlog)
-__git_log_gitk_options="
- --dense --sparse --full-history
- --simplify-merges --simplify-by-decoration
- --left-right --notes --no-notes
-"
-# Options that go well for log and shortlog (not gitk)
-__git_log_shortlog_options="
- --author= --committer= --grep=
- --all-match --invert-grep
-"
-# Options accepted by log and show
-__git_log_show_options="
- --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
-"
-
-__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
-
-__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
-__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-
-
-# Check for only porcelain (i.e. not git-rev-list) option (not argument)
-# and selected option argument completions for git-log options and if any
-# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
-# and will be empty on return if no candidates are found.
-__git_complete_log_opts ()
-{
- [ -z "$COMPREPLY" ] || return 1 # Precondition
-
- local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
- fi
- case "$prev,$cur" in
- -L,:*:*)
- return # fall back to Bash filename completion
- ;;
- -L,:*)
- __git_complete_symbol --cur="${cur#:}" --sfx=":"
- return
- ;;
- -G,*|-S,*)
- __git_complete_symbol
- return
- ;;
- esac
- case "$cur" in
- --pretty=*|--format=*)
- __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
- " "" "${cur#*=}"
- return
- ;;
- --date=*)
- __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
- return
- ;;
- --decorate=*)
- __gitcomp "full short no" "" "${cur##--decorate=}"
- return
- ;;
- --diff-algorithm=*)
- __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
- return
- ;;
- --submodule=*)
- __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
- return
- ;;
- --ws-error-highlight=*)
- __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
- return
- ;;
- --no-walk=*)
- __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
- return
- ;;
- --diff-merges=*)
- __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
- return
- ;;
- --*)
- __gitcomp "
- $__git_log_common_options
- $__git_log_shortlog_options
- $__git_log_gitk_options
- $__git_log_show_options
- --root --topo-order --date-order --reverse
- --follow --full-diff
- --abbrev-commit --no-abbrev-commit --abbrev=
- --relative-date --date=
- --pretty= --format= --oneline
- --show-signature
- --cherry-mark
- --cherry-pick
- --graph
- --decorate --decorate= --no-decorate
- --walk-reflogs
- --no-walk --no-walk= --do-walk
- --parents --children
- --expand-tabs --expand-tabs= --no-expand-tabs
- $merge
- $__git_diff_common_options
- "
- return
- ;;
- -L:*:*)
- return # fall back to Bash filename completion
- ;;
- -L:*)
- __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
- return
- ;;
- -G*)
- __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
- return
- ;;
- -S*)
- __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
- return
- ;;
- esac
-}
-
_git_log ()
{
__git_has_doubledash && return
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/5] completion: git-log opts to bisect visualize
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
To do this the majority of _git_log has been factored out into the new
__git_complete_log_opts. This is needed because the visualize command
accepts git-log options but not rev arguments (they are fixed to the
commits under bisection).
__git_complete_log_opts has a precondition that COMPREPLY be empty. In
a completion context it doesn't seem advisable to implement
preconditions as noisy or hard failures, so instead it becomes a no-op
on violation. This should be detectable and quick to debug for devels,
without ever aggravating a user (besides completion failure).
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 30 +++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 15d22ff7d9..c16aded36c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1472,6 +1472,16 @@ _git_bisect ()
;;
esac
;;
+ visualize)
+ case "$cur" in
+ -*)
+ __git_complete_log_opts
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
esac
case "$subcommand" in
@@ -2074,10 +2084,14 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-_git_log ()
+
+# Check for only porcelain (i.e. not git-rev-list) option (not argument)
+# and selected option argument completions for git-log options and if any
+# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
+# and will be empty on return if no candidates are found.
+__git_complete_log_opts ()
{
- __git_has_doubledash && return
- __git_find_repo_path
+ [ -z "$COMPREPLY" ] || return 1 # Precondition
local merge=""
if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
@@ -2171,6 +2185,16 @@ _git_log ()
return
;;
esac
+}
+
+_git_log ()
+{
+ __git_has_doubledash && return
+ __git_find_repo_path
+
+ __git_complete_log_opts
+ [ -z "$COMPREPLY" ] || return
+
__git_complete_revlist
}
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/5] completion: improvements for git-bisect
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin, Junio C Hamano
In-Reply-To: <9e180f50-4bf4-4822-9b02-2a1b50114e09@smtp-relay.sendinblue.com>
Bring completion for bisect up to date with current features.
I didn't hear back on my previous RFC Patch for these improvements, and
ultimately decided that simply aborting special completion efforts and
doing nothing on precondition violation is the best policy, because devs
will likely notice and users won't be irritated more than necessary.
Besides that the series has just been cleaned up a tiny bit.
Britton Leo Kerin (5):
completion: complete new old actions, start opts
completion: git-log opts to bisect visualize
completion: move to maintain define-before-use
completion: custom git-bisect terms
completion: custom git-bisect terms
contrib/completion/git-completion.bash | 312 +++++++++++++++----------
1 file changed, 183 insertions(+), 129 deletions(-)
Range-diff against v1:
1: e16264bfb9 = 1: e16264bfb9 completion: complete new old actions, start opts
2: 90466cdfa5 ! 2: 130abe3460 completion: git-log opts to bisect visualize
@@ Commit message
accepts git-log options but not rev arguments (they are fixed to the
commits under bisection).
+ __git_complete_log_opts has a precondition that COMPREPLY be empty. In
+ a completion context it doesn't seem advisable to implement
+ preconditions as noisy or hard failures, so instead it becomes a no-op
+ on violation. This should be detectable and quick to debug for devels,
+ without ever aggravating a user (besides completion failure).
+
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
## contrib/completion/git-completion.bash ##
@@ contrib/completion/git-completion.bash: __git_diff_merges_opts="off none on firs
__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-_git_log ()
--{
++
++# Check for only porcelain (i.e. not git-rev-list) option (not argument)
++# and selected option argument completions for git-log options and if any
++# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
++# and will be empty on return if no candidates are found.
++__git_complete_log_opts ()
+ {
- __git_has_doubledash && return
- __git_find_repo_path
++ [ -z "$COMPREPLY" ] || return 1 # Precondition
-+# Find only porcelain (i.e. not git-rev-list) option (not argument) and
-+# selected option argument completions for git-log options and put them in
-+# COMPREPLY.
-+__git_complete_log_opts ()
-+{
local merge=""
if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
@@ contrib/completion/git-completion.bash: _git_log ()
return
;;
esac
-+
+}
+
+_git_log ()
3: 0edfb54dd5 ! 3: d659ace9c2 completion: move to maintain define-before-use
@@ contrib/completion/git-completion.bash: _git_archive ()
+__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
+__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
+
-+# Find only porcelain (i.e. not git-rev-list) option (not argument) and
-+# selected option argument completions for git-log options and put them in
-+# COMPREPLY.
++# Check for only porcelain (i.e. not git-rev-list) option (not argument)
++# and selected option argument completions for git-log options and if any
++# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
++# and will be empty on return if no candidates are found.
+__git_complete_log_opts ()
+{
++ [ -z "$COMPREPLY" ] || return 1 # Precondition
++
+ local merge=""
+ if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ merge="--merge"
@@ contrib/completion/git-completion.bash: _git_archive ()
+ return
+ ;;
+ esac
-+
+}
+
_git_bisect ()
@@ contrib/completion/git-completion.bash: _git_ls_tree ()
-__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-
-
--# Find only porcelain (i.e. not git-rev-list) option (not argument) and
--# selected option argument completions for git-log options and put them in
--# COMPREPLY.
+-# Check for only porcelain (i.e. not git-rev-list) option (not argument)
+-# and selected option argument completions for git-log options and if any
+-# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
+-# and will be empty on return if no candidates are found.
-__git_complete_log_opts ()
-{
+- [ -z "$COMPREPLY" ] || return 1 # Precondition
+-
- local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
@@ contrib/completion/git-completion.bash: _git_ls_tree ()
- return
- ;;
- esac
--
-}
-
_git_log ()
4: a8a730ffbe = 4: c5bee633b2 completion: custom git-bisect terms
5: 44d3a36e20 ! 5: 220650f0ba completion: recognize but do not complete 'view'
@@ Metadata
Author: Britton Leo Kerin <britton.kerin@gmail.com>
## Commit message ##
- completion: recognize but do not complete 'view'
-
- Completing it might annoy some existing users by creating completion
- ambiguity on 'v' and 'vi' without adding anything useful in terms of
- interface discovery/recall (because 'view' is just an alias anyway).
+ completion: custom git-bisect terms
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
@@ contrib/completion/git-completion.bash: _git_bisect ()
fi
- local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
++ # We will complete any custom terms, but still always complete the
++ # more usual bad/new/good/old because git bisect gives a good error
++ # message if these are given when not in use and that's better than
++ # silent refusal to complete if the user is confused.
++ #
+ # We want to recognize 'view' but not complete it, because it overlaps
+ # with 'visualize' too much and is just an alias for it.
++ #
+ local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ local all_subcommands="$completable_subcommands view"
6: 7b88549fbe < -: ---------- completion: add comment
--
2.43.0
^ permalink raw reply
* [PATCH v2 1/5] completion: complete new old actions, start opts
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 185b47d802..15d22ff7d9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1449,7 +1449,7 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad good skip reset visualize replay log run"
+ local subcommands="start bad new good old terms skip reset visualize replay log run help"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__git_find_repo_path
@@ -1462,7 +1462,20 @@ _git_bisect ()
fi
case "$subcommand" in
- bad|good|reset|skip|start)
+ start)
+ case "$cur" in
+ --*)
+ __gitcomp "--term-new --term-bad --term-old --term-good --first-parent --no-checkout"
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
+ esac
+
+ case "$subcommand" in
+ bad|new|good|old|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* [PATCH v2 4/5] completion: custom git-bisect terms
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 63ca8082a4..ad80df6630 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1583,10 +1583,19 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad new good old terms skip reset visualize replay log run help"
+ __git_find_repo_path
+
+ local term_bad term_good
+ if [ -f "$__git_repo_path"/BISECT_START ]; then
+ term_bad=`__git bisect terms --term-bad`
+ term_good=`__git bisect terms --term-good`
+ fi
+
+ local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+
local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
if [ -z "$subcommand" ]; then
- __git_find_repo_path
if [ -f "$__git_repo_path"/BISECT_START ]; then
__gitcomp "$subcommands"
else
@@ -1619,7 +1628,7 @@ _git_bisect ()
esac
case "$subcommand" in
- bad|new|good|old|reset|skip|start)
+ bad|new|"$term_bad"|good|old|"$term_good"|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* Problem in gitweb.cgi
From: Marcelo Roberto Jimenez @ 2024-01-10 0:43 UTC (permalink / raw)
To: git
Hi,
I have recently run into a problem with gitweb that was triggered by
my distribution, OpenSUSE Tumbleweed. Due to a misconfiguration of the
application AppArmour, "git instaweb" was having problems in accessing
the configuration file "/etc/gitweb-common.conf". That was half of the
problem and has been reported downstream here:
https://bugzilla.suse.com/show_bug.cgi?id=1218664
The other half of the problem is in gitweb.cgi itself. There is a
logic to select which configuration file is going to be used. That
logic is ok, although a bit unclear from the documentation. Reading
the code I understood that $GITWEB_CONFIG_COMMON (usually
/etc/gitweb-common.conf) should always be read if it exists, and
$GITWEB_CONFIG_SYSTEM (usually /etc/gitweb.conf) will never be read if
$GITWEB_CONFIG has been read before.
The problem is that $GITWEB_CONFIG_COMMON was never read, even though
the code that reads it was being called before the code that reads the
other two files. As I said before, the problem was caused by a lack of
permission in AppArmour, but gitweb should have been able to catch the
error. By not signaling it properly, users get lost because the
problem is a little tricky to debug.
After some "printf" debugging, I converged to this routine, line 728
of gitweb.cgi:
# read and parse gitweb config file given by its parameter.
# returns true on success, false on recoverable error, allowing
# to chain this subroutine, using first file that exists.
# dies on errors during parsing config file, as it is unrecoverable.
sub read_config_file {
my $filename = shift;
return unless defined $filename;
# die if there are errors parsing config file
if (-e $filename) {
do $filename;
die $@ if $@;
return 1;
}
return;
}
Perl uses two different variables to manage errors from a "do". One is
"$@", which is set in this case when do is unable to compile the file.
The other is $!, which is set in case "do" cannot read the file. By
printing the value of $! I found out that it was set to "Permission
denied". Since the script does not currently test for $!, the error
goes unnoticed.
I suppose a proper fix would be to put a line before the test for $@,
like "die $! if $!".
For the curious, I have a report explaining the full problem here, but
the part relevant to gitweb is in this e-mail:
https://stackoverflow.com/questions/77789216/problem-with-git-instaweb-on-opensuse-tumbleweed-etc-gitweb-common-conf-is-n
Best regards,
Marcelo.
^ permalink raw reply
* What's cooking in git.git (Jan 2024, #03; Tue, 9)
From: Junio C Hamano @ 2024-01-10 0:17 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* en/header-cleanup (2023-12-26) 12 commits
(merged to 'next' on 2023-12-28 at 1ccddc2a10)
+ treewide: remove unnecessary includes in source files
+ treewide: add direct includes currently only pulled in transitively
+ trace2/tr2_tls.h: remove unnecessary include
+ submodule-config.h: remove unnecessary include
+ pkt-line.h: remove unnecessary include
+ line-log.h: remove unnecessary include
+ http.h: remove unnecessary include
+ fsmonitor--daemon.h: remove unnecessary includes
+ blame.h: remove unnecessary includes
+ archive.h: remove unnecessary include
+ treewide: remove unnecessary includes in source files
+ treewide: remove unnecessary includes from header files
Remove unused header "#include".
source: <pull.1617.v2.git.1703351700.gitgitgadget@gmail.com>
* en/sparse-checkout-eoo (2023-12-26) 2 commits
(merged to 'next' on 2023-12-28 at 3ddf2ebab6)
+ sparse-checkout: be consistent with end of options markers
+ Merge branch 'jk/end-of-options' into jc/sparse-checkout-set-add-end-of-options
"git sparse-checkout (add|set) --[no-]cone --end-of-options" did
not handle "--end-of-options" correctly after a recent update.
source: <pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com>
* jc/archive-list-with-extra-args (2023-12-21) 1 commit
(merged to 'next' on 2023-12-28 at 2d5c20e67f)
+ archive: "--list" does not take further options
"git archive --list extra garbage" silently ignored excess command
line parameters, which has been corrected.
source: <xmqqmsu3mnix.fsf@gitster.g>
* jc/sparse-checkout-set-default-fix (2023-12-26) 1 commit
(merged to 'next' on 2023-12-28 at a558eccf8e)
+ sparse-checkout: use default patterns for 'set' only !stdin
"git sparse-checkout set" added default patterns even when the
patterns are being fed from the standard input, which has been
corrected.
source: <20231221065925.3234048-3-gitster@pobox.com>
* ml/doc-merge-updates (2023-12-20) 2 commits
(merged to 'next' on 2023-12-28 at 7a6329a219)
+ Documentation/git-merge.txt: use backticks for command wrapping
+ Documentation/git-merge.txt: fix reference to synopsis
Doc update.
source: <20231220195342.17590-1-mi.al.lohmann@gmail.com>
* rs/fast-import-simplify-mempool-allocation (2023-12-26) 1 commit
(merged to 'next' on 2023-12-28 at 16e6dd2a69)
+ fast-import: use mem_pool_calloc()
Code simplification.
source: <50c1f410-ca37-4c1c-a28b-3e9fad49f2b4@web.de>
* rs/mem-pool-improvements (2023-12-28) 2 commits
(merged to 'next' on 2023-12-28 at aa03d9441c)
+ mem-pool: simplify alignment calculation
+ mem-pool: fix big allocations
MemPool allocator fixes.
source: <fa89d269-1a23-4ed6-bebc-30c0b629f444@web.de>
--------------------------------------------------
[New Topics]
* ps/gitlab-ci-static-analysis (2024-01-08) 1 commit
- ci: add job performing static analysis on GitLab CI
GitLab CI update.
Will merge to 'next'.
source: <1536a5ef07ad24dafb5d685b40099882f89e6cc5.1703761005.git.ps@pks.im>
* ps/prompt-parse-HEAD-futureproof (2024-01-08) 2 commits
- git-prompt: stop manually parsing HEAD with unknown ref formats
- Merge branch 'ps/refstorage-extension' into ps/prompt-parse-HEAD-futureproof
(this branch uses ps/refstorage-extension.)
Futureproof command line prompt support (in contrib/).
Will merge to 'next'.
source: <ef4e36a5a40c369da138242a8fdc9e12a846613b.1704356313.git.ps@pks.im>
* ps/reftable-optimize-io (2024-01-08) 4 commits
- reftable/blocksource: use mmap to read tables
- reftable/stack: use stat info to avoid re-reading stack list
- reftable/stack: refactor reloading to use file descriptor
- reftable/stack: refactor stack reloading to have common exit path
Low-level I/O optimization for reftable.
Will merge to 'next'?
source: <cover.1704714575.git.ps@pks.im>
* rj/clarify-branch-doc-m (2024-01-08) 1 commit
- branch: clarify <oldbranch> term
Doc update.
Will merge to 'next'.
source: <d38e5a18-4d85-48f3-bc8b-8ca02ea683a4@gmail.com>
* tb/fetch-all-configuration (2024-01-08) 1 commit
- fetch: add new config option fetch.all
"git fetch" learned to pay attention to "fetch.all" configuration
variable, which pretends as if "--all" was passed from the command
line when no remote parameter was given.
Will merge to 'next'.
source: <20240108211832.47362-1-dev@tb6.eu>
* rj/advice-disable-how-to-disable (2024-01-09) 3 commits
- advice: allow disabling the automatic hint in advise_if_enabled()
- t/test-tool: handle -c <name>=<value> arguments
- t/test-tool: usage description
All conditional "advice" messages show how to turn them off, which
becomes repetitive. Add a configuration variable to omit the
instruction.
Expecting a reroll.
cf. <ZZ2QafUf/JxXYZU/@nand.local>
source: <7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com>
* vd/fsck-submodule-url-test (2024-01-09) 3 commits
- submodule-config.c: strengthen URL fsck check
- t7450: test submodule urls
- submodule-config.h: move check_submodule_url
Tighten URL checks fsck makes in a URL recorded for submodules.
Will merge to 'next'?
source: <pull.1635.git.1704822817.gitgitgadget@gmail.com>
--------------------------------------------------
[Cooking]
* ms/rebase-insnformat-doc-fix (2024-01-03) 1 commit
(merged to 'next' on 2024-01-04 at d68f2be39b)
+ Documentation: fix statement about rebase.instructionFormat
Docfix.
Will merge to 'master'.
source: <pull.1629.git.git.1704305663254.gitgitgadget@gmail.com>
* cp/git-flush-is-an-env-bool (2024-01-04) 1 commit
(merged to 'next' on 2024-01-04 at b435a96ce8)
+ write-or-die: make GIT_FLUSH a Boolean environment variable
Unlike other environment variables that took the usual
true/false/yes/no as well as 0/1, GIT_FLUSH only understood 0/1,
which has been corrected.
Will merge to 'master'.
source: <pull.1628.v3.git.1704363617842.gitgitgadget@gmail.com>
* sd/negotiate-trace-fix (2024-01-03) 1 commit
- push: region_leave trace for negotiate_using_fetch
Tracing fix.
Waiting for a review response.
cf. <xmqqbka27zu9.fsf@gitster.g>
source: <20240103224054.1940209-1-delmerico@google.com>
* sk/mingw-owner-check-error-message-improvement (2024-01-08) 1 commit
- mingw: give more details about unsafe directory's ownership
In addition to (rather cryptic) Security Identifiers, show username
and domain in the error message when we barf on mismatch between
the Git directory and the current user.
Will merge to 'next'?
source: <20240108173837.20480-2-soekkle@freenet.de>
* ib/rebase-reschedule-doc (2024-01-05) 1 commit
(merged to 'next' on 2024-01-08 at d451d1f760)
+ rebase: clarify --reschedule-failed-exec default
Doc update.
Will merge to 'master'.
source: <20240105011424.1443732-2-illia.bobyr@gmail.com>
* jk/commit-graph-slab-clear-fix (2024-01-05) 1 commit
(merged to 'next' on 2024-01-08 at f78c4fc296)
+ commit-graph: retain commit slab when closing NULL commit_graph
Clearing in-core repository (happens during e.g., "git fetch
--recurse-submodules" with commit graph enabled) made in-core
commit object in an inconsistent state by discarding the necessary
data from commit-graph too early, which has been corrected.
Will merge to 'master'.
source: <20240105054142.GA2035092@coredump.intra.peff.net>
* jk/index-pack-lsan-false-positive-fix (2024-01-05) 1 commit
(merged to 'next' on 2024-01-08 at 589ed65251)
+ index-pack: spawn threads atomically
Fix false positive reported by leak sanitizer.
Will merge to 'master'.
source: <20240105085034.GA3078476@coredump.intra.peff.net>
* cp/sideband-array-index-comment-fix (2023-12-28) 1 commit
(merged to 'next' on 2024-01-08 at f906bc86f1)
+ sideband.c: remove redundant 'NEEDSWORK' tag
In-code comment fix.
Will merge to 'master'.
source: <pull.1625.v4.git.1703750460527.gitgitgadget@gmail.com>
* ps/worktree-refdb-initialization (2024-01-08) 7 commits
- builtin/worktree: create refdb via ref backend
- worktree: expose interface to look up worktree by name
- builtin/worktree: move setup of commondir file earlier
- refs/files: skip creation of "refs/{heads,tags}" for worktrees
- setup: move creation of "refs/" into the files backend
- refs: prepare `refs_init_db()` for initializing worktree refs
- Merge branch 'ps/refstorage-extension' into ps/worktree-refdb-initialization
(this branch uses ps/refstorage-extension.)
Instead of manually creating refs/ hierarchy on disk upon a
creation of a secondary worktree, which is only usable via the
files backend, use the refs API to populate it.
source: <cover.1704705733.git.ps@pks.im>
* cp/apply-core-filemode (2023-12-26) 3 commits
- apply: code simplification
- apply: correctly reverse patch's pre- and post-image mode bits
- apply: ignore working tree filemode when !core.filemode
"git apply" on a filesystem without filemode support have learned
to take a hint from what is in the index for the path, even when
not working with the "--index" or "--cached" option, when checking
the executable bit match what is required by the preimage in the
patch.
Needs review.
source: <20231226233218.472054-1-gitster@pobox.com>
* jk/t1006-cat-file-objectsize-disk (2024-01-03) 2 commits
(merged to 'next' on 2024-01-03 at a492c6355c)
+ t1006: prefer shell loop to awk for packed object sizes
(merged to 'next' on 2023-12-28 at d82812e636)
+ t1006: add tests for %(objectsize:disk)
Test update.
Will merge to 'master'.
source: <20231221094722.GA570888@coredump.intra.peff.net>
source: <20240103090152.GB1866508@coredump.intra.peff.net>
* js/contributor-docs-updates (2023-12-27) 9 commits
(merged to 'next' on 2024-01-02 at 0e072117cd)
+ SubmittingPatches: hyphenate non-ASCII
+ SubmittingPatches: clarify GitHub artifact format
+ SubmittingPatches: clarify GitHub visual
+ SubmittingPatches: provide tag naming advice
+ SubmittingPatches: update extra tags list
+ SubmittingPatches: discourage new trailers
+ SubmittingPatches: drop ref to "What's in git.git"
+ CodingGuidelines: write punctuation marks
+ CodingGuidelines: move period inside parentheses
Doc update.
Will merge to 'master'.
source: <pull.1623.v3.git.1703739324.gitgitgadget@gmail.com>
* al/unit-test-ctype (2024-01-05) 1 commit
- unit-tests: rewrite t/helper/test-ctype.c as a unit test
Move test-ctype helper to the unit-test framework.
Almost there.
cf. <a087f57c-ce72-45c7-8182-f38d0aca9030@web.de>
cf. <33c81610-0958-49da-b702-ba8d96ecf1d3@gmail.com>
source: <20240105161413.10422-1-ach.lumap@gmail.com>
* bk/bisect-doc-fix (2023-12-27) 1 commit
- doc: use singular form of repeatable path arg
Synopsis fix.
Expecting a reroll.
source: <3d46bca1-96d4-43ba-a912-1f7c76942287@smtp-relay.sendinblue.com>
* ja/doc-placeholders-fix (2023-12-26) 2 commits
- doc: enforce placeholders in documentation
- doc: enforce dashes in placeholders
Docfix.
Needs review.
source: <pull.1626.git.1703539287.gitgitgadget@gmail.com>
* ps/refstorage-extension (2024-01-02) 13 commits
(merged to 'next' on 2024-01-08 at f9a034803b)
+ t9500: write "extensions.refstorage" into config
+ builtin/clone: introduce `--ref-format=` value flag
+ builtin/init: introduce `--ref-format=` value flag
+ builtin/rev-parse: introduce `--show-ref-format` flag
+ t: introduce GIT_TEST_DEFAULT_REF_FORMAT envvar
+ setup: introduce GIT_DEFAULT_REF_FORMAT envvar
+ setup: introduce "extensions.refStorage" extension
+ setup: set repository's formats on init
+ setup: start tracking ref storage format
+ refs: refactor logic to look up storage backends
+ worktree: skip reading HEAD when repairing worktrees
+ t: introduce DEFAULT_REPO_FORMAT prereq
+ Merge branch 'ps/clone-into-reftable-repository' into ps/refstorage-extension
(this branch is used by ps/prompt-parse-HEAD-futureproof and ps/worktree-refdb-initialization.)
Introduce a new extension "refstorage" so that we can mark a
repository that uses a non-default ref backend, like reftable.
Will merge to 'master'.
source: <cover.1703833818.git.ps@pks.im>
* ps/reftable-fixes-and-optims (2024-01-03) 9 commits
(merged to 'next' on 2024-01-08 at 167d7685f8)
+ reftable/merged: transfer ownership of records when iterating
+ reftable/merged: really reuse buffers to compute record keys
+ reftable/record: store "val2" hashes as static arrays
+ reftable/record: store "val1" hashes as static arrays
+ reftable/record: constify some parts of the interface
+ reftable/writer: fix index corruption when writing multiple indices
+ reftable/stack: do not auto-compact twice in `reftable_stack_add()`
+ reftable/stack: do not overwrite errors when compacting
+ Merge branch 'ps/reftable-fixes' into ps/reftable-fixes-and-optims
More fixes and optimizations to the reftable backend.
Will merge to 'master'.
source: <cover.1704262787.git.ps@pks.im>
* tb/multi-pack-verbatim-reuse (2023-12-14) 26 commits
(merged to 'next' on 2024-01-04 at 891ac0fa2c)
+ t/perf: add performance tests for multi-pack reuse
+ pack-bitmap: enable reuse from all bitmapped packs
+ pack-objects: allow setting `pack.allowPackReuse` to "single"
+ t/test-lib-functions.sh: implement `test_trace2_data` helper
+ pack-objects: add tracing for various packfile metrics
+ pack-bitmap: prepare to mark objects from multiple packs for reuse
+ pack-revindex: implement `midx_pair_to_pack_pos()`
+ pack-revindex: factor out `midx_key_to_pack_pos()` helper
+ midx: implement `midx_preferred_pack()`
+ git-compat-util.h: implement checked size_t to uint32_t conversion
+ pack-objects: include number of packs reused in output
+ pack-objects: prepare `write_reused_pack_verbatim()` for multi-pack reuse
+ pack-objects: prepare `write_reused_pack()` for multi-pack reuse
+ pack-objects: pass `bitmapped_pack`'s to pack-reuse functions
+ pack-objects: keep track of `pack_start` for each reuse pack
+ pack-objects: parameterize pack-reuse routines over a single pack
+ pack-bitmap: return multiple packs via `reuse_partial_packfile_from_bitmap()`
+ pack-bitmap: simplify `reuse_partial_packfile_from_bitmap()` signature
+ ewah: implement `bitmap_is_empty()`
+ pack-bitmap: pass `bitmapped_pack` struct to pack-reuse functions
+ midx: implement `midx_locate_pack()`
+ midx: implement `BTMP` chunk
+ midx: factor out `fill_pack_info()`
+ pack-bitmap: plug leak in find_objects()
+ pack-bitmap-write: deep-clear the `bb_commit` slab
+ pack-objects: free packing_data in more places
Streaming spans of packfile data used to be done only from a
single, primary, pack in a repository with multiple packfiles. It
has been extended to allow reuse from other packfiles, too.
Will merge to 'master'.
cf. <ZXurD1NTZ4TAs7WZ@nand.local>
source: <cover.1702592603.git.me@ttaylorr.com>
* jc/bisect-doc (2023-12-09) 1 commit
- bisect: document "terms" subcommand more fully
Doc update.
Needs review.
source: <xmqqzfyjmk02.fsf@gitster.g>
* jw/builtin-objectmode-attr (2023-12-28) 1 commit
(merged to 'next' on 2024-01-02 at 4c3784b3a1)
+ attr: add builtin objectmode values support
The builtin_objectmode attribute is populated for each path
without adding anything in .gitattributes files, which would be
useful in magic pathspec, e.g., ":(attr:builtin_objectmode=100755)"
to limit to executables.
Will merge to 'master'.
cf. <xmqq5y0ssknj.fsf@gitster.g>
source: <20231116054437.2343549-1-jojwang@google.com>
* tb/pair-chunk-expect (2023-11-10) 8 commits
- midx: read `OOFF` chunk with `pair_chunk_expect()`
- midx: read `OIDL` chunk with `pair_chunk_expect()`
- commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
- commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
- commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
- commit-graph: read `OIDL` chunk with `pair_chunk_expect()`
- chunk-format: introduce `pair_chunk_expect()` helper
- Merge branch 'jk/chunk-bounds-more' into HEAD
Further code clean-up.
Needs review.
source: <cover.1699569246.git.me@ttaylorr.com>
* tb/path-filter-fix (2023-10-18) 17 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
- commit-graph: ensure Bloom filters are read with consistent settings
- revision.c: consult Bloom filters for root commits
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Expecting a reroll?
cf. <20231023202212.GA5470@szeder.dev>
source: <cover.1697653929.git.me@ttaylorr.com>
* ak/color-decorate-symbols (2023-10-23) 7 commits
- log: add color.decorate.pseudoref config variable
- refs: exempt pseudorefs from pattern prefixing
- refs: add pseudorefs array and iteration functions
- log: add color.decorate.ref config variable
- log: add color.decorate.symbol config variable
- log: use designated inits for decoration_colors
- config: restructure color.decorate documentation
A new config for coloring.
Needs review.
source: <20231023221143.72489-1-andy.koppe@gmail.com>
* eb/hash-transition (2023-10-02) 30 commits
- t1016-compatObjectFormat: add tests to verify the conversion between objects
- t1006: test oid compatibility with cat-file
- t1006: rename sha1 to oid
- test-lib: compute the compatibility hash so tests may use it
- builtin/ls-tree: let the oid determine the output algorithm
- object-file: handle compat objects in check_object_signature
- tree-walk: init_tree_desc take an oid to get the hash algorithm
- builtin/cat-file: let the oid determine the output algorithm
- rev-parse: add an --output-object-format parameter
- repository: implement extensions.compatObjectFormat
- object-file: update object_info_extended to reencode objects
- object-file-convert: convert commits that embed signed tags
- object-file-convert: convert commit objects when writing
- object-file-convert: don't leak when converting tag objects
- object-file-convert: convert tag objects when writing
- object-file-convert: add a function to convert trees between algorithms
- object: factor out parse_mode out of fast-import and tree-walk into in object.h
- cache: add a function to read an OID of a specific algorithm
- tag: sign both hashes
- commit: export add_header_signature to support handling signatures on tags
- commit: convert mergetag before computing the signature of a commit
- commit: write commits for both hashes
- object-file: add a compat_oid_in parameter to write_object_file_flags
- object-file: update the loose object map when writing loose objects
- loose: compatibilty short name support
- loose: add a mapping between SHA-1 and SHA-256 for loose objects
- repository: add a compatibility hash algorithm
- object-names: support input of oids in any supported hash
- oid-array: teach oid-array to handle multiple kinds of oids
- object-file-convert: stubs for converting from one object format to another
Teach a repository to work with both SHA-1 and SHA-256 hash algorithms.
Needs review.
source: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>
* jx/remote-archive-over-smart-http (2023-12-14) 4 commits
- archive: support remote archive from stateless transport
- transport-helper: call do_take_over() in connect_helper
- transport-helper: call do_take_over() in process_connect
- transport-helper: no connection restriction in connect_helper
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
Needs review.
source: <cover.1702562879.git.zhiyou.jx@alibaba-inc.com>
* jx/sideband-chomp-newline-fix (2023-12-18) 3 commits
(merged to 'next' on 2024-01-04 at 1237898a22)
+ pkt-line: do not chomp newlines for sideband messages
+ pkt-line: memorize sideband fragment in reader
+ test-pkt-line: add option parser for unpack-sideband
Sideband demultiplexer fixes.
Will merge to 'master'.
source: <cover.1702823801.git.zhiyou.jx@alibaba-inc.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
Code clean-up.
Not ready to be reviewed yet.
source: <20230824205456.1231371-1-gitster@pobox.com>
--------------------------------------------------
[Discarded]
* jc/sparse-checkout-eoo (2023-12-21) 4 commits
. sparse-checkout: tighten add_patterns_from_input()
. sparse-checkout: use default patterns for 'set' only !stdin
. SQUASH??? end-of-options test
. sparse-checkout: take care of "--end-of-options" in set/add/check-rules
"git sparse-checkout (add|set) --[no-]cone --end-of-options" did
not handle "--end-of-options" correctly after a recent update.
Superseded by the en/sparse-checkout-eoo topic.
source: <20231221065925.3234048-1-gitster@pobox.com>
* tb/merge-tree-write-pack (2023-10-23) 5 commits
. builtin/merge-tree.c: implement support for `--write-pack`
. bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
. bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
. bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
. bulk-checkin: extract abstract `bulk_checkin_source`
"git merge-tree" learned "--write-pack" to record its result
without creating loose objects.
Broken when an object created during a merge is needed to continue merge
cf. <CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com>
cf. <ZZWOtnP2IHNldcy6@nand.local>
source: <cover.1698101088.git.me@ttaylorr.com>
^ permalink raw reply
* Re: Bug: Git spawns subprocesses on windows
From: brian m. carlson @ 2024-01-09 22:56 UTC (permalink / raw)
To: Domen Golob; +Cc: git
In-Reply-To: <CAF6dN-oEy-svp+6Bw_NAeOMrWc61ObcZ4Q2huVj9PPK1EQHquw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]
On 2024-01-09 at 15:33:43, Domen Golob wrote:
> Hello,
> the problem is exactly the same as what was reported here on stackoverflow:
> c# - Git for windows seems to create sub-processes that never die -
> Stack Overflow
> https://stackoverflow.com/questions/69579065/git-for-windows-seems-to-create-sub-processes-that-never-die
>
> Additionally I have found out that:
> - a Git for Windows subprocess is created for each repository and
> every time a git command is issued this process grows in size and it
> never dies.
> - you cannot delete the .git folder from the terminal, but it works
> via file explorer
> - deleting the .git folder kills the Git for windows process
> - creating changes in several repos creates multiple processes, and
> sometimes the process is created as a subprocess
You probably want to contact Git for Windows at
https://github.com/git-for-windows/git. The reason I suggest that is
that this is usually not a behaviour we see on Unix, and seems to be
Windows-specific.
I don't know if it's possible to see command-line arguments of processes
on Windows like it is with `ps` on Unix, but including that information
if possible will be very useful to the maintainers. Without knowing
that information, it's very unlikely that anyone will be able to help
you since we don't know what's going on.
There are some possibilities of what's going on here:
* git gc is running.
* git maintenance or the fsmonitor is configured and is running.
* You have a non-default antivirus or monitoring software that causes
processes to hang or not complete, so one of Git's subprocesses can't
exit. If you're using such software, we usually recommend completely
removing it completely (disabling it is usually not sufficient),
rebooting, and testing again to make sure it's not the problem.
* You have some other process which is running which spawns Git commands
(editors, content indexers, etc.).
* We actually have a bug and some process is not waited for correctly,
and zombie processes manifest differently on Windows than on Unix.
* Something else I haven't considered.
However, as I said, you'll probably want to contact the Git for Windows
folks through their issue tracker.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership
From: Junio C Hamano @ 2024-01-09 22:34 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Sören Krecker, sunshine, git
In-Reply-To: <e7594386-4a26-467d-bd27-8ac6268ad219@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> Because len_domain includes the terminating NUL for the domain part,
>> (*str)[len_domain-1] is that NUL, no? And that is what you want to
>> overwrite to make the two strings <d> <NUL> <u> <NUL> into a single
>> one <d> <slash> <u> <NUL>. So...
>
> But after a successful call, len_domain and len_user have been modified
> to contain the lengths of the names (not counting the NULs), ...
Ah, that is what I missed. Thanks.
^ permalink raw reply
* Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-09 22:32 UTC (permalink / raw)
To: Taylor Blau; +Cc: Rubén Justo, Git List
In-Reply-To: <ZZ2QafUf/JxXYZU/@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> On Tue, Jan 09, 2024 at 04:25:32PM +0100, Rubén Justo wrote:
>> Using advise_if_enabled() to display an advice will automatically
>> include instructions on how to disable the advice, along with the main
>> advice:
>>
>> hint: use --reapply-cherry-picks to include skipped commits
>> hint: Disable this message with "git config advice.skippedCherryPicks false"
>>
>> This may become distracting or "noisy" over time, while the user may
>> still want to receive the main advice.
>>
>> Let's have a switch to allow disabling this automatic advice.
>
> I reviewed this and had a couple of notes, mostly focused on what to
> call the new configuration option, and if we should be modifying the
> test-tool helpers to accept arbitrary configuration via command-line
> options.
>
> I think that we could reasonably drop the first two patches by
> imitating the existing style of t0018 more closely, but I don't feel all
> that strongly about it.
Even though I do not have a fundamental objection against test-tool
learning "-c key=val", I do not see a strong need for the first two
steps, either.
I actually have more problems with the primary change of this series
(in addition to that configuration knob is probably misnamed, as you
pointed out). How to disable the advice is an integral part of the
end-user experience about the conditional advice system. The idea
is to show it repeatedly, perhaps loud enough to be called "noisy",
so that the user learns to follow the suggestion given by the hint.
Until then, the user is expected to gradually learn to follow the
suggestion more and more, seeing the advice message less and less
often. If we rob the "how to disable THIS PARTICULAR message" and
gave only this line:
>> hint: use --reapply-cherry-picks to include skipped commits
it would become impossible to find how to disable it, once the user
get comfortable enough to pass --reapply-cherry-picks when it is
appropriate to do so. I do not think there is no quick way other
than grepping in the source to find that the user needs to disable
the skippedCherryPicks message (no, you can look at the output from
"git config --help" and find "skippedCherryPicks" if you know that
is the symbol to be found, but there is nothing to link the above
hint message to that particular help page entry).
I would understand if the proposed change were to change the
"advice.<key>" configuration variable from a Boolean to a tristate
(yes = default, no = disabled, always = give advice without
instruction), though. IOW, the message might look like so:
hint: use --reapply-cherry-picks to include skipped commits
hint: setting advice.skippedCherryPicks to 'false' disables this message
hint: and setting it to 'always' removes this additional instruction.
Then, those who find the hint always useful (because the particular
mistake the hint is given against is something the user commits very
rarely and the convenience of being reminded when it happens, which
is rare, outweigh the burden of learning what is suggested) may want
to set it to 'always' to accept that new choice.
But not the way the new feature is proposed.
Thanks.
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Sergey Organov @ 2024-01-09 22:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34v6gswv.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I think the current code makes "-n" take precedence, and ignores
> "-f".
To me it rather looks more like "-n" implies "-f", but then there is
"second -f" rule that makes things even more interesting:
"Git will refuse to modify untracked nested git repositories
(directories with a .git subdirectory) unless a second -f is given."
How do I figure what files will be deleted on
git clean -f -f
when "-n" behaves as you (or me) described? I.e., what
git clean -f -f -n
and
git clean -f -n
will output?
>
> Shouldn't it either
>
> (1) error out with "-n and -f cannot be used together", or
> (2) let "-n" and "-f" follow the usual "last one wins" rule?
>
> The latter may be logically cleaner but it is a change that breaks
> backward compatibility big time in a more dangerous direction, so it
> may not be desirable in practice, with too big a downside for a too
> little gain.
I agree (2) is too dangerous and surprising, and (1) is limiting: I
believe the user should be able to see what will be done on
git clean -f -f
by simply adding "-n" to the command-line.
So I figure I'd rather prefer yet another option:
(3) -n dry run: show what will be done once "-n" is removed.
This way, e.g.,
git clean
and
git clean -n
will produce exactly the same output with default configuration:
fatal: clean.requireForce defaults to true and neither -i, nor -f given; refusing to clean
and one will need to say, e.g.:
git clean -n -f
to get the list of files to be deleted with "git clean -f".
With (3) "-n" becomes orthogonal to "-f", resulting in predictable and
useful behavior.
BR,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH 3/3] submodule-config.c: strengthen URL fsck check
From: Junio C Hamano @ 2024-01-09 21:57 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <893071530d3b77d6b72b7f69a6dfb9947579865e.1704822817.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> Update the validation of "curl URL" submodule URLs (i.e. those that specify
> an "http[s]" or "ftp[s]" protocol) in 'check_submodule_url()' to catch more
> invalid URLs. The existing validation using 'credential_from_url_gently()'
> parses certain URLs incorrectly, leading to invalid submodule URLs passing
> 'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote
> URLs in 'remote_get()' - correctly identifies the invalid URLs missed by
> 'credential_from_url_gently()'.
>
> To catch more invalid cases, replace 'credential_from_url_gently()' with
> 'url_normalize()' followed by a 'url_decode()' and a check for newlines
> (mirroring 'check_url_component()' in the 'credential_from_url_gently()'
> validation).
Thanks. Left hand and right hand checking the same thing in
different ways and coming up with different result is never a happy
situation. Making sure we consistently use the same definition of
what the valid URLs are is a very welcome thing to do, of course.
> -test_expect_failure 'check urls' '
> +test_expect_success 'check urls' '
> cat >expect <<-\EOF &&
> ./bar/baz/foo.git
> https://example.com/foo.git
It is a bit unfortunate that from here we cannot tell which bogus
URLs in this test that were incorrectly accepted are now rejected.
Among the many bogus URLs in the input, we used to allow
http://example.com:test/foo.git
(we do not accept non-numeric representation of port numbers, so
http://example.com:http/foo.git would also be rejected), but with
this change, it is now rejected. All the other bogus ones are
rejected just as before this change.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] t7450: test submodule urls
From: Junio C Hamano @ 2024-01-09 21:38 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <cf7848edffca27931aad02c0652adf2715320d35.1704822817.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +#define TEST_TOOL_CHECK_URL_USAGE \
> + "test-tool submodule check-url <url>"
> +static const char *submodule_check_url_usage[] = {
> + TEST_TOOL_CHECK_URL_USAGE,
> + NULL
> +};
Granted, the entry that follows this new one already uses the same
pattern, but TEST_TOOL_CHECK_URL_USAGE being used only once here and
nowhere else, with its name almost as long as the value it expands to,
I found it unnecessarily verbose and confusing.
> #define TEST_TOOL_IS_ACTIVE_USAGE \
> "test-tool submodule is-active <name>"
> static const char *submodule_is_active_usage[] = {
> +typedef int (*check_fn_t)(const char *);
> +
> /*
> * Exit non-zero if any of the submodule names given on the command line is
> * invalid. If no names are given, filter stdin to print only valid names
> * (which is primarily intended for testing).
> */
OK. As long as each of the input lines are unique, we can use the
usual "does the actual output match the expected?" to test many of
them at once, and notice if there is an extra one in the output that
shouldn't have been emitted, or there is a missing one that should
have.
> -static int check_name(int argc, const char **argv)
> +static int check_submodule(int argc, const char **argv, check_fn_t check_fn)
> {
> if (argc > 1) {
> while (*++argv) {
> - if (check_submodule_name(*argv) < 0)
> + if (check_fn(*argv) < 0)
Quite nice way to reuse what we already have, thanks to [1/3].
^ permalink raw reply
* Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership
From: Johannes Sixt @ 2024-01-09 21:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sören Krecker, sunshine, git
In-Reply-To: <xmqq8r4ygtkd.fsf@gitster.g>
Am 09.01.24 um 21:06 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
>>> + &pe_use);
>>
>> At this point, the function fails, so len_user and len_domain contain
>> the required buffer size (including the trailing NUL).
>
> So (*str)[len_domain] would be the trailing NUL after the domain
> part in the next call? Or would that be (*str)[len_domain-1]? I am
> puzzled by off-by-one with your "including the trailing NUL" remark.
"Required buffer size" must count the trailing NUL. So, the NUL would be
at (*str)[len_domain-1].
>
>>> + /*
>>> + * Alloc needed space of the strings
>>> + */
>>> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
>
> This obviously assumes for domain 'd' and user 'u', we want "d/u" and
> len_domain must be 1+1 (including NUL) and len_user must be 1+1
> (including NUL). But then ...
So, this allocates the exact amount that is required to contain the
names with the trailing NULs: 1+1 plus 1+1 in this example.
>
>>> + translate_sid_to_user = LookupAccountSidA(NULL, sid,
>>> + (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);
>
> ... ((*str)+len_domain) is presumably the beginning of the user
> part, and (*str)+0) is where the domain part is to be stored.
Correct.
>
> Because len_domain includes the terminating NUL for the domain part,
> (*str)[len_domain-1] is that NUL, no? And that is what you want to
> overwrite to make the two strings <d> <NUL> <u> <NUL> into a single
> one <d> <slash> <u> <NUL>. So...
But after a successful call, len_domain and len_user have been modified
to contain the lengths of the names (not counting the NULs), so, here
the NUL is at (*str)[len_domain]...
>
>> At this point, if the function is successful, len_user and len_domain
>> contain the lengths of the names (without the trailing NUL).
>>
>>> + if (!translate_sid_to_user)
>>> + FREE_AND_NULL(*str);
>>> + else
>>> + (*str)[len_domain] = '/';
>
> ... this offset looks fishy to me. Am I off-by-one?
... and this offset is correct.
I followed the same train of thought and suspected an off-by-one error,
too, and was perplexed that I see a correct output. The documentation of
LookupAccountSid is unclear that the variables change values across the
(successful) call, but my tests verified that the change does happen.
>> Therefore, this overwrites the NUL after the domain name and so
>> concatenates the two names. Good.
>>
>> I found this by dumping the values of the variables, because the
>> documentation of LookupAccountSid is not clear about the values that the
>> variables receive in the success case.
>>
>>> + return translate_sid_to_user;
>>> +}
>>> +
>>
>> This patch looks good and works for me.
>>
>> Acked-by: Johannes Sixt <j6t@kdbg.org>
>>
>> Thank you!
>>
>> -- Hannes
-- Hannes
^ permalink raw reply
* what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Junio C Hamano @ 2024-01-09 20:20 UTC (permalink / raw)
To: git
I think the current code makes "-n" take precedence, and ignores
"-f". Shouldn't it either
(1) error out with "-n and -f cannot be used together", or
(2) let "-n" and "-f" follow the usual "last one wins" rule?
The latter may be logically cleaner but it is a change that breaks
backward compatibility big time in a more dangerous direction, so it
may not be desirable in practice, with too big a downside for a too
little gain.
^ permalink raw reply
* Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership
From: Junio C Hamano @ 2024-01-09 20:06 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Sören Krecker, sunshine, git
In-Reply-To: <d1e1a543-ab9c-4b1b-9f1d-3728e791df2e@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
>> + &pe_use);
>
> At this point, the function fails, so len_user and len_domain contain
> the required buffer size (including the trailing NUL).
So (*str)[len_domain] would be the trailing NUL after the domain
part in the next call? Or would that be (*str)[len_domain-1]? I am
puzzled by off-by-one with your "including the trailing NUL" remark.
>> + /*
>> + * Alloc needed space of the strings
>> + */
>> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
This obviously assumes for domain 'd' and user 'u', we want "d/u" and
len_domain must be 1+1 (including NUL) and len_user must be 1+1
(including NUL). But then ...
>> + translate_sid_to_user = LookupAccountSidA(NULL, sid,
>> + (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);
... ((*str)+len_domain) is presumably the beginning of the user
part, and (*str)+0) is where the domain part is to be stored.
Because len_domain includes the terminating NUL for the domain part,
(*str)[len_domain-1] is that NUL, no? And that is what you want to
overwrite to make the two strings <d> <NUL> <u> <NUL> into a single
one <d> <slash> <u> <NUL>. So...
> At this point, if the function is successful, len_user and len_domain
> contain the lengths of the names (without the trailing NUL).
>
>> + if (!translate_sid_to_user)
>> + FREE_AND_NULL(*str);
>> + else
>> + (*str)[len_domain] = '/';
... this offset looks fishy to me. Am I off-by-one?
> Therefore, this overwrites the NUL after the domain name and so
> concatenates the two names. Good.
>
> I found this by dumping the values of the variables, because the
> documentation of LookupAccountSid is not clear about the values that the
> variables receive in the success case.
>
>> + return translate_sid_to_user;
>> +}
>> +
>
> This patch looks good and works for me.
>
> Acked-by: Johannes Sixt <j6t@kdbg.org>
>
> Thank you!
>
> -- Hannes
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-09 19:57 UTC (permalink / raw)
To: Taylor Blau; +Cc: Rubén Justo, Git List
In-Reply-To: <ZZ2QGYBBmK8cSYBD@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> Looking at this test, I wonder why we don't imitate the existing style
> of:
>
> test_config advice.adviceOff false &&
> test-tool advise "This is a piece of advice" 2>actual &&
> test_cmp expect actual
>
> instead of teaching the test-tool helpers how to interpret `-c`
> arguments. Doing so would allow us to drop the first couple of patches
> in this series and simplify things a bit.
Thanks for a dose of sanity. I too got a bit too excited by a shiny
new toy ;-) Reusing the existing mechanism does make sense.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox