* [PATCH 11/15] find multi-byte comment chars in unterminated buffers
From: Jeff King @ 2024-03-07 9:26 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Dragan Simic,
Kristoffer Haugsbakk, Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
As with the previous patch, we need to swap out single-byte matching for
something like starts_with() to match all bytes of a multi-byte comment
character. But for cases where the buffer is not NUL-terminated (and we
instead have an explicit size or end pointer), it's not safe to use
starts_with(), as it might walk off the end of the buffer.
Let's introduce a new starts_with_mem() that does the same thing but
also accepts the length of the "haystack" str and makes sure not to walk
past it.
Note that in most cases the existing code did not need a length check at
all, since it was written in a way that knew we had at least one byte
available (and that was all we checked). So I had to read each one to
find the appropriate bounds. The one exception is sequencer.c's
add_commented_lines(), where we can actually get rid of the length
check. Just like starts_with(), our starts_with_mem() handles an empty
haystack variable by not matching (assuming a non-empty prefix).
A few notes on the implementation of starts_with_mem():
- it would be equally correct to take an "end" pointer (and indeed,
many of the callers have this and have to subtract to come up with
the length). I think taking a ptr/size combo is a more usual
interface for our codebase, though, and has the added benefit that
the function signature makes it harder to mix up the three
parameters.
- we could obviously build starts_with() on top of this by passing
strlen(str) as the length. But it's possible that starts_with() is a
relatively hot code path, and it should not pay that penalty (it can
generally return an answer proportional to the size of the prefix,
not the whole string).
- it naively feels like xstrncmpz() should be able to do the same
thing, but that's not quite true. If you pass the length of the
haystack buffer, then strncmp() finds that a shorter prefix string
is "less than" than the haystack, even if the haystack starts with
the prefix. If you pass the length of the prefix, then you risk
reading past the end of the haystack if it is shorter than the
prefix. So I think we really do need a new function.
Signed-off-by: Jeff King <peff@peff.net>
---
Arguably starts_with() and this new function should both be inlined,
like we do for skip_prefix(), but I think that's out of scope for this
series.
And it's possible I was simply too dumb to figure out xstrncmpz() here.
I'm waiting for René to show up and tell me how to do it. ;)
IMHO this is the trickiest commit of the whole series, as it would be
easy to get the length computations subtly wrong.
commit.c | 3 ++-
sequencer.c | 4 ++--
strbuf.c | 11 +++++++++++
strbuf.h | 1 +
trailer.c | 4 ++--
5 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/commit.c b/commit.c
index ef679a0b93..531a666cba 100644
--- a/commit.c
+++ b/commit.c
@@ -1796,7 +1796,8 @@ size_t ignored_log_message_bytes(const char *buf, size_t len)
else
next_line++;
- if (buf[bol] == comment_line_char || buf[bol] == '\n') {
+ if (starts_with_mem(buf + bol, cutoff - bol, comment_line_str) ||
+ buf[bol] == '\n') {
/* is this the first of the run of comments? */
if (!boc)
boc = bol;
diff --git a/sequencer.c b/sequencer.c
index 991a2dbe96..664986e3b2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1840,7 +1840,7 @@ static int is_fixup_flag(enum todo_command command, unsigned flag)
static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
{
const char *s = str;
- while (len > 0 && s[0] == comment_line_char) {
+ while (starts_with_mem(s, len, comment_line_str)) {
size_t count;
const char *n = memchr(s, '\n', len);
if (!n)
@@ -2562,7 +2562,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
/* left-trim */
bol += strspn(bol, " \t");
- if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
+ if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
item->command = TODO_COMMENT;
item->commit = NULL;
item->arg_offset = bol - buf;
diff --git a/strbuf.c b/strbuf.c
index 7c8f582127..291bdc2a65 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -24,6 +24,17 @@ int istarts_with(const char *str, const char *prefix)
return 0;
}
+int starts_with_mem(const char *str, size_t len, const char *prefix)
+{
+ const char *end = str + len;
+ for (; ; str++, prefix++) {
+ if (!*prefix)
+ return 1;
+ else if (str == end || *str != *prefix)
+ return 0;
+ }
+}
+
int skip_to_optional_arg_default(const char *str, const char *prefix,
const char **arg, const char *def)
{
diff --git a/strbuf.h b/strbuf.h
index 58dddf2777..3156d6ea8c 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -673,6 +673,7 @@ char *xstrfmt(const char *fmt, ...);
int starts_with(const char *str, const char *prefix);
int istarts_with(const char *str, const char *prefix);
+int starts_with_mem(const char *str, size_t len, const char *prefix);
/*
* If the string "str" is the same as the string in "prefix", then the "arg"
diff --git a/trailer.c b/trailer.c
index fe18faf6c5..f59c90b4b5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
/* The first paragraph is the title and cannot be trailers */
for (s = buf; s < buf + len; s = next_line(s)) {
- if (s[0] == comment_line_char)
+ if (starts_with_mem(s, buf + len - s, comment_line_str))
continue;
if (is_blank_line(s))
break;
@@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
const char **p;
ssize_t separator_pos;
- if (bol[0] == comment_line_char) {
+ if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
non_trailer_lines += possible_continuation_lines;
possible_continuation_lines = 0;
continue;
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 10/15] find multi-byte comment chars in NUL-terminated strings
From: Jeff King @ 2024-03-07 9:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
Several parts of the code need to identify lines that begin with the
comment character, and do so with a simple byte equality check. As part
of the transition to handling multi-byte characters, we need to match
all of the bytes. For cases where we are looking in a NUL-terminated
string, we can just use starts_with(), which checks all of the
characters in comment_line_str.
Note that we can drop the "line.len" check in wt-status.c's
read_rebase_todolist(). The starts_with() function handles the case of
an empty haystack buffer (it will always return false for a non-empty
prefix).
Signed-off-by: Jeff King <peff@peff.net>
---
I think the main way these hunks could be wrong is if the buffer is not
in fact NUL-terminated. In most cases we're working with a strbuf,
though.
add-patch.c | 2 +-
sequencer.c | 2 +-
trailer.c | 2 +-
wt-status.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 4a10237d50..d599ca53e1 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1139,7 +1139,7 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
for (i = 0; i < s->buf.len; ) {
size_t next = find_next_line(&s->buf, i);
- if (s->buf.buf[i] != comment_line_char)
+ if (!starts_with(s->buf.buf + i, comment_line_str))
strbuf_add(&s->plain, s->buf.buf + i, next - i);
i = next;
}
diff --git a/sequencer.c b/sequencer.c
index 241e185f87..991a2dbe96 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2003,7 +2003,7 @@ static int update_squash_messages(struct repository *r,
return error(_("could not read '%s'"),
rebase_path_squash_msg());
- eol = buf.buf[0] != comment_line_char ?
+ eol = !starts_with(buf.buf, comment_line_str) ?
buf.buf : strchrnul(buf.buf, '\n');
strbuf_addf(&header, "%s ", comment_line_str);
diff --git a/trailer.c b/trailer.c
index ef9df4af55..fe18faf6c5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1013,7 +1013,7 @@ static void parse_trailers(struct trailer_info *info,
for (i = 0; i < info->trailer_nr; i++) {
int separator_pos;
char *trailer = info->trailers[i];
- if (trailer[0] == comment_line_char)
+ if (starts_with(trailer, comment_line_str))
continue;
separator_pos = find_separator(trailer, separators);
if (separator_pos >= 1) {
diff --git a/wt-status.c b/wt-status.c
index b66c30775b..084bfc584f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1382,7 +1382,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)
git_path("%s", fname));
}
while (!strbuf_getline_lf(&line, f)) {
- if (line.len && line.buf[0] == comment_line_char)
+ if (starts_with(line.buf, comment_line_str))
continue;
strbuf_trim(&line);
if (!line.len)
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 09/15] prefer comment_line_str to comment_line_char for printing
From: Jeff King @ 2024-03-07 9:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
As part of our transition to multi-byte comment characters, we should
use the string variable rather than the historical character variable.
All of the sites adjusted here are just swapping out "%c" for "%s" in
format strings, or strbuf_addch() for strbuf_addstr(). The type system
and printf-attribute give the compiler enough information to make sure
our formats and variable changes all match (especially important for
cases where the format string is defined far away from its use, like
prepare_to_commit() in commit.c).
Signed-off-by: Jeff King <peff@peff.net>
---
add-patch.c | 4 ++--
builtin/branch.c | 4 ++--
builtin/commit.c | 12 ++++++------
builtin/merge.c | 4 ++--
builtin/tag.c | 8 ++++----
fmt-merge-msg.c | 2 +-
sequencer.c | 20 ++++++++++----------
wt-status.c | 10 +++++-----
8 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 7390677795..4a10237d50 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1114,10 +1114,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
"To remove '%c' lines, make them ' ' lines "
"(context).\n"
"To remove '%c' lines, delete them.\n"
- "Lines starting with %c will be removed.\n"),
+ "Lines starting with %s will be removed.\n"),
s->mode->is_reverse ? '+' : '-',
s->mode->is_reverse ? '-' : '+',
- comment_line_char);
+ comment_line_str);
strbuf_commented_addf(&s->buf, comment_line_str, "%s",
_(s->mode->edit_hunk_hint));
/*
diff --git a/builtin/branch.c b/builtin/branch.c
index 8904a1e5d9..1cdcae8454 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -670,8 +670,8 @@ static int edit_branch_description(const char *branch_name)
strbuf_commented_addf(&buf, comment_line_str,
_("Please edit the description for the branch\n"
" %s\n"
- "Lines starting with '%c' will be stripped.\n"),
- branch_name, comment_line_char);
+ "Lines starting with '%s' will be stripped.\n"),
+ branch_name, comment_line_str);
write_file_buf(edit_description(), buf.buf, buf.len);
strbuf_reset(&buf);
if (launch_editor(edit_description(), &buf, NULL)) {
diff --git a/builtin/commit.c b/builtin/commit.c
index d8abbe48b1..8519a004d0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -910,18 +910,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
struct ident_split ci, ai;
const char *hint_cleanup_all = allow_empty_message ?
_("Please enter the commit message for your changes."
- " Lines starting\nwith '%c' will be ignored.\n") :
+ " Lines starting\nwith '%s' will be ignored.\n") :
_("Please enter the commit message for your changes."
- " Lines starting\nwith '%c' will be ignored, and an empty"
+ " Lines starting\nwith '%s' will be ignored, and an empty"
" message aborts the commit.\n");
const char *hint_cleanup_space = allow_empty_message ?
_("Please enter the commit message for your changes."
" Lines starting\n"
- "with '%c' will be kept; you may remove them"
+ "with '%s' will be kept; you may remove them"
" yourself if you want to.\n") :
_("Please enter the commit message for your changes."
" Lines starting\n"
- "with '%c' will be kept; you may remove them"
+ "with '%s' will be kept; you may remove them"
" yourself if you want to.\n"
"An empty message aborts the commit.\n");
if (whence != FROM_COMMIT) {
@@ -945,12 +945,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
fprintf(s->fp, "\n");
if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)
- status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_all, comment_line_char);
+ status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_all, comment_line_str);
else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
if (whence == FROM_COMMIT && !merge_contains_scissors)
wt_status_add_cut_line(s->fp);
} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
- status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_space, comment_line_char);
+ status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_space, comment_line_str);
/*
* These should never fail because they come from our own
diff --git a/builtin/merge.c b/builtin/merge.c
index 6d048fb628..ba4308883f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -821,7 +821,7 @@ static const char scissors_editor_comment[] =
N_("An empty message aborts the commit.\n");
static const char no_scissors_editor_comment[] =
-N_("Lines starting with '%c' will be ignored, and an empty message aborts\n"
+N_("Lines starting with '%s' will be ignored, and an empty message aborts\n"
"the commit.\n");
static void write_merge_heads(struct commit_list *);
@@ -861,7 +861,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
_(scissors_editor_comment));
else
strbuf_commented_addf(&msg, comment_line_str,
- _(no_scissors_editor_comment), comment_line_char);
+ _(no_scissors_editor_comment), comment_line_str);
}
if (signoff)
append_signoff(&msg, ignored_log_message_bytes(msg.buf, msg.len), 0);
diff --git a/builtin/tag.c b/builtin/tag.c
index 1c708785bf..721d07a589 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -158,11 +158,11 @@ static int do_sign(struct strbuf *buffer)
static const char tag_template[] =
N_("\nWrite a message for tag:\n %s\n"
- "Lines starting with '%c' will be ignored.\n");
+ "Lines starting with '%s' will be ignored.\n");
static const char tag_template_nocleanup[] =
N_("\nWrite a message for tag:\n %s\n"
- "Lines starting with '%c' will be kept; you may remove them"
+ "Lines starting with '%s' will be kept; you may remove them"
" yourself if you want to.\n");
static int git_tag_config(const char *var, const char *value,
@@ -292,10 +292,10 @@ static void create_tag(const struct object_id *object, const char *object_ref,
strbuf_addch(&buf, '\n');
if (opt->cleanup_mode == CLEANUP_ALL)
strbuf_commented_addf(&buf, comment_line_str,
- _(tag_template), tag, comment_line_char);
+ _(tag_template), tag, comment_line_str);
else
strbuf_commented_addf(&buf, comment_line_str,
- _(tag_template_nocleanup), tag, comment_line_char);
+ _(tag_template_nocleanup), tag, comment_line_str);
write_or_die(fd, buf.buf, buf.len);
strbuf_release(&buf);
}
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 79e8aad086..ae201e21db 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -321,7 +321,7 @@ static void credit_people(struct strbuf *out,
skip_prefix(me, them->items->string, &me) &&
starts_with(me, " <")))
return;
- strbuf_addf(out, "\n%c %s ", comment_line_char, label);
+ strbuf_addf(out, "\n%s %s ", comment_line_str, label);
add_people_count(out, them);
}
diff --git a/sequencer.c b/sequencer.c
index 032e213a3f..241e185f87 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -663,7 +663,7 @@ void append_conflicts_hint(struct index_state *istate,
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
strbuf_addch(msgbuf, '\n');
wt_status_append_cut_line(msgbuf);
- strbuf_addch(msgbuf, comment_line_char);
+ strbuf_addstr(msgbuf, comment_line_str);
}
strbuf_addch(msgbuf, '\n');
@@ -1946,7 +1946,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
(starts_with(body, "squash!") || starts_with(body, "fixup!"))))
commented_len = commit_subject_length(body);
- strbuf_addf(buf, "\n%c ", comment_line_char);
+ strbuf_addf(buf, "\n%s ", comment_line_str);
strbuf_addf(buf, _(nth_commit_msg_fmt),
++opts->current_fixup_count + 1);
strbuf_addstr(buf, "\n\n");
@@ -2006,7 +2006,7 @@ static int update_squash_messages(struct repository *r,
eol = buf.buf[0] != comment_line_char ?
buf.buf : strchrnul(buf.buf, '\n');
- strbuf_addf(&header, "%c ", comment_line_char);
+ strbuf_addf(&header, "%s ", comment_line_str);
strbuf_addf(&header, _(combined_commit_msg_fmt),
opts->current_fixup_count + 2);
strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
@@ -2032,9 +2032,9 @@ static int update_squash_messages(struct repository *r,
repo_unuse_commit_buffer(r, head_commit, head_message);
return error(_("cannot write '%s'"), rebase_path_fixup_msg());
}
- strbuf_addf(&buf, "%c ", comment_line_char);
+ strbuf_addf(&buf, "%s ", comment_line_str);
strbuf_addf(&buf, _(combined_commit_msg_fmt), 2);
- strbuf_addf(&buf, "\n%c ", comment_line_char);
+ strbuf_addf(&buf, "\n%s ", comment_line_str);
strbuf_addstr(&buf, is_fixup_flag(command, flag) ?
_(skip_first_commit_msg_str) :
_(first_commit_msg_str));
@@ -2056,7 +2056,7 @@ static int update_squash_messages(struct repository *r,
if (command == TODO_SQUASH || is_fixup_flag(command, flag)) {
res = append_squash_message(&buf, body, command, opts, flag);
} else if (command == TODO_FIXUP) {
- strbuf_addf(&buf, "\n%c ", comment_line_char);
+ strbuf_addf(&buf, "\n%s ", comment_line_str);
strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),
++opts->current_fixup_count + 1);
strbuf_addstr(&buf, "\n\n");
@@ -5659,8 +5659,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
oid_to_hex(&commit->object.oid),
oneline.buf);
if (is_empty)
- strbuf_addf(&buf, " %c empty",
- comment_line_char);
+ strbuf_addf(&buf, " %s empty",
+ comment_line_str);
FLEX_ALLOC_STR(entry, string, buf.buf);
oidcpy(&entry->entry.oid, &commit->object.oid);
@@ -5750,7 +5750,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
entry = oidmap_get(&state.commit2label, &commit->object.oid);
if (entry)
- strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
+ strbuf_addf(out, "\n%s Branch %s\n", comment_line_str, entry->string);
else
strbuf_addch(out, '\n');
@@ -5887,7 +5887,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
oid_to_hex(&commit->object.oid));
pretty_print_commit(&pp, commit, out);
if (is_empty)
- strbuf_addf(out, " %c empty", comment_line_char);
+ strbuf_addf(out, " %s empty", comment_line_str);
strbuf_addch(out, '\n');
}
if (skipped_commit)
diff --git a/wt-status.c b/wt-status.c
index 6b81f5349c..b66c30775b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -70,7 +70,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
strbuf_vaddf(&sb, fmt, ap);
if (!sb.len) {
if (s->display_comment_prefix) {
- strbuf_addch(&sb, comment_line_char);
+ strbuf_addstr(&sb, comment_line_str);
if (!trail)
strbuf_addch(&sb, ' ');
}
@@ -85,7 +85,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
strbuf_reset(&linebuf);
if (at_bol && s->display_comment_prefix) {
- strbuf_addch(&linebuf, comment_line_char);
+ strbuf_addstr(&linebuf, comment_line_str);
if (*line != '\n' && *line != '\t')
strbuf_addch(&linebuf, ' ');
}
@@ -1090,7 +1090,7 @@ size_t wt_status_locate_end(const char *s, size_t len)
const char *p;
struct strbuf pattern = STRBUF_INIT;
- strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
+ strbuf_addf(&pattern, "\n%s %s", comment_line_str, cut_line);
if (starts_with(s, pattern.buf + 1))
len = 0;
else if ((p = strstr(s, pattern.buf)))
@@ -1214,8 +1214,8 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
"%s%.*s", comment_line_string,
(int)(ep - cp), cp);
if (s->display_comment_prefix)
- color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%c",
- comment_line_char);
+ color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%s",
+ comment_line_str);
else
fputs("\n", s->fp);
strbuf_release(&sb);
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 08/15] strbuf: accept a comment string for strbuf_add_commented_lines()
From: Jeff King @ 2024-03-07 9:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
As part of our transition to multi-byte comment characters, let's take a
NUL-terminated string pointer for strbuf_add_commented_lines() rather
than a single character.
All of the callers have to be adjusted; most can just pass
comment_line_str rather than comment_line_char.
And now our "cheat" in strbuf_commented_addf() can go away, as we can
take the full string from it.
Signed-off-by: Jeff King <peff@peff.net>
---
This could also be squashed into the previous patch. I wasn't sure if it
would be more overwhelming to have so many changes intermingled, or if
the "cheat" / "uncheat" back-and-forth would be too confusing. Pick your
poison.
builtin/notes.c | 8 ++++----
builtin/stripspace.c | 2 +-
fmt-merge-msg.c | 6 +++---
rebase-interactive.c | 6 +++---
sequencer.c | 8 ++++----
strbuf.c | 16 +++-------------
strbuf.h | 2 +-
wt-status.c | 4 ++--
8 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 5223a3f350..1a67f01d00 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -179,7 +179,7 @@ static void write_commented_object(int fd, const struct object_id *object)
if (strbuf_read(&buf, show.out, 0) < 0)
die_errno(_("could not read 'show' output"));
- strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_char);
+ strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
write_or_die(fd, cbuf.buf, cbuf.len);
strbuf_release(&cbuf);
@@ -207,10 +207,10 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
copy_obj_to_fd(fd, old_note);
strbuf_addch(&buf, '\n');
- strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_char);
+ strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_str);
strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)),
- comment_line_char);
- strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_char);
+ comment_line_str);
+ strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_str);
write_or_die(fd, buf.buf, buf.len);
write_commented_object(fd, object);
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 434ac490cb..e5626e5126 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -13,7 +13,7 @@ static void comment_lines(struct strbuf *buf)
size_t len;
msg = strbuf_detach(buf, &len);
- strbuf_add_commented_lines(buf, msg, len, comment_line_char);
+ strbuf_add_commented_lines(buf, msg, len, comment_line_str);
free(msg);
}
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 66e47449a0..79e8aad086 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -510,7 +510,7 @@ static void fmt_tag_signature(struct strbuf *tagbuf,
if (sig->len) {
strbuf_addch(tagbuf, '\n');
strbuf_add_commented_lines(tagbuf, sig->buf, sig->len,
- comment_line_char);
+ comment_line_str);
}
}
@@ -557,7 +557,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
strbuf_add_commented_lines(&tagline,
origins.items[first_tag].string,
strlen(origins.items[first_tag].string),
- comment_line_char);
+ comment_line_str);
strbuf_insert(&tagbuf, 0, tagline.buf,
tagline.len);
strbuf_release(&tagline);
@@ -566,7 +566,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
strbuf_add_commented_lines(&tagbuf,
origins.items[i].string,
strlen(origins.items[i].string),
- comment_line_char);
+ comment_line_str);
fmt_tag_signature(&tagbuf, &sig, buf, len);
}
strbuf_release(&payload);
diff --git a/rebase-interactive.c b/rebase-interactive.c
index affc93a8e4..c343e16fcd 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -78,7 +78,7 @@ void append_todo_help(int command_count,
shortrevisions, shortonto, command_count);
}
- strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_char);
+ strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);
if (get_missing_commit_check_level() == MISSING_COMMIT_CHECK_ERROR)
msg = _("\nDo not remove any line. Use 'drop' "
@@ -87,7 +87,7 @@ void append_todo_help(int command_count,
msg = _("\nIf you remove a line here "
"THAT COMMIT WILL BE LOST.\n");
- strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_char);
+ strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);
if (edit_todo)
msg = _("\nYou are editing the todo file "
@@ -98,7 +98,7 @@ void append_todo_help(int command_count,
msg = _("\nHowever, if you remove everything, "
"the rebase will be aborted.\n\n");
- strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_char);
+ strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);
}
int edit_todo_list(struct repository *r, struct todo_list *todo_list,
diff --git a/sequencer.c b/sequencer.c
index 852c3f9f4e..032e213a3f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1851,7 +1851,7 @@ static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
s += count;
len -= count;
}
- strbuf_add_commented_lines(buf, s, len, comment_line_char);
+ strbuf_add_commented_lines(buf, s, len, comment_line_str);
}
/* Does the current fixup chain contain a squash command? */
@@ -1950,7 +1950,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
strbuf_addf(buf, _(nth_commit_msg_fmt),
++opts->current_fixup_count + 1);
strbuf_addstr(buf, "\n\n");
- strbuf_add_commented_lines(buf, body, commented_len, comment_line_char);
+ strbuf_add_commented_lines(buf, body, commented_len, comment_line_str);
/* buf->buf may be reallocated so store an offset into the buffer */
fixup_off = buf->len;
strbuf_addstr(buf, body + commented_len);
@@ -2041,7 +2041,7 @@ static int update_squash_messages(struct repository *r,
strbuf_addstr(&buf, "\n\n");
if (is_fixup_flag(command, flag))
strbuf_add_commented_lines(&buf, body, strlen(body),
- comment_line_char);
+ comment_line_str);
else
strbuf_addstr(&buf, body);
@@ -2061,7 +2061,7 @@ static int update_squash_messages(struct repository *r,
++opts->current_fixup_count + 1);
strbuf_addstr(&buf, "\n\n");
strbuf_add_commented_lines(&buf, body, strlen(body),
- comment_line_char);
+ comment_line_str);
} else
return error(_("unknown command: %d"), command);
repo_unuse_commit_buffer(r, commit, message);
diff --git a/strbuf.c b/strbuf.c
index 76d02e0920..7c8f582127 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -359,13 +359,9 @@ static void add_lines(struct strbuf *out,
}
void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
- size_t size, char comment_prefix)
+ size_t size, const char *comment_prefix)
{
- char prefix[2];
-
- prefix[0] = comment_prefix;
- prefix[1] = '\0';
- add_lines(out, prefix, buf, size, 1);
+ add_lines(out, comment_prefix, buf, size, 1);
}
void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix,
@@ -379,13 +375,7 @@ void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix,
strbuf_vaddf(&buf, fmt, params);
va_end(params);
- /*
- * TODO Our commented_lines helper does not yet understand
- * comment strings. But since we know that the strings are
- * always single-char, we can cheat for the moment, and
- * fix this later.
- */
- strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_prefix[0]);
+ strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_prefix);
if (incomplete_line)
sb->buf[--sb->len] = '\0';
diff --git a/strbuf.h b/strbuf.h
index b128ca539a..58dddf2777 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -288,7 +288,7 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
*/
void strbuf_add_commented_lines(struct strbuf *out,
const char *buf, size_t size,
- char comment_prefix);
+ const char *comment_prefix);
/**
diff --git a/wt-status.c b/wt-status.c
index 2be2eb094c..6b81f5349c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1028,7 +1028,7 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom
if (s->display_comment_prefix) {
size_t len;
summary_content = strbuf_detach(&summary, &len);
- strbuf_add_commented_lines(&summary, summary_content, len, comment_line_char);
+ strbuf_add_commented_lines(&summary, summary_content, len, comment_line_str);
free(summary_content);
}
@@ -1104,7 +1104,7 @@ void wt_status_append_cut_line(struct strbuf *buf)
const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
strbuf_commented_addf(buf, comment_line_str, "%s", cut_line);
- strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
+ strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_str);
}
void wt_status_add_cut_line(FILE *fp)
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 07/15] strbuf: accept a comment string for strbuf_commented_addf()
From: Jeff King @ 2024-03-07 9:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
As part of our transition to multi-byte comment characters, let's take a
NUL-terminated string pointer for strbuf_commented_addf() rather than a
single character.
All of the callers have to be adjusted, but they can just pass
comment_line_str rather than comment_line_char.
Note that we rely on strbuf_add_commented_lines() under the hood, so
we'll cheat a bit to squeeze our string into a single character (for now
the two are equivalent, and we'll address this TODO in the next patch).
Signed-off-by: Jeff King <peff@peff.net>
---
add-patch.c | 8 ++++----
builtin/branch.c | 2 +-
builtin/merge.c | 8 ++++----
builtin/tag.c | 4 ++--
rebase-interactive.c | 2 +-
sequencer.c | 4 ++--
strbuf.c | 10 ++++++++--
strbuf.h | 2 +-
wt-status.c | 2 +-
9 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..7390677795 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1105,11 +1105,11 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
size_t i;
strbuf_reset(&s->buf);
- strbuf_commented_addf(&s->buf, comment_line_char,
+ strbuf_commented_addf(&s->buf, comment_line_str,
_("Manual hunk edit mode -- see bottom for "
"a quick guide.\n"));
render_hunk(s, hunk, 0, 0, &s->buf);
- strbuf_commented_addf(&s->buf, comment_line_char,
+ strbuf_commented_addf(&s->buf, comment_line_str,
_("---\n"
"To remove '%c' lines, make them ' ' lines "
"(context).\n"
@@ -1118,13 +1118,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
s->mode->is_reverse ? '+' : '-',
s->mode->is_reverse ? '-' : '+',
comment_line_char);
- strbuf_commented_addf(&s->buf, comment_line_char, "%s",
+ strbuf_commented_addf(&s->buf, comment_line_str, "%s",
_(s->mode->edit_hunk_hint));
/*
* TRANSLATORS: 'it' refers to the patch mentioned in the previous
* messages.
*/
- strbuf_commented_addf(&s->buf, comment_line_char,
+ strbuf_commented_addf(&s->buf, comment_line_str,
_("If it does not apply cleanly, you will be "
"given an opportunity to\n"
"edit again. If all lines of the hunk are "
diff --git a/builtin/branch.c b/builtin/branch.c
index c03c0407d1..8904a1e5d9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -667,7 +667,7 @@ static int edit_branch_description(const char *branch_name)
exists = !read_branch_desc(&buf, branch_name);
if (!buf.len || buf.buf[buf.len-1] != '\n')
strbuf_addch(&buf, '\n');
- strbuf_commented_addf(&buf, comment_line_char,
+ strbuf_commented_addf(&buf, comment_line_str,
_("Please edit the description for the branch\n"
" %s\n"
"Lines starting with '%c' will be stripped.\n"),
diff --git a/builtin/merge.c b/builtin/merge.c
index 935c8a57dd..6d048fb628 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -852,15 +852,15 @@ static void prepare_to_commit(struct commit_list *remoteheads)
strbuf_addch(&msg, '\n');
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
wt_status_append_cut_line(&msg);
- strbuf_commented_addf(&msg, comment_line_char, "\n");
+ strbuf_commented_addf(&msg, comment_line_str, "\n");
}
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg, comment_line_str,
_(merge_editor_comment));
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg, comment_line_str,
_(scissors_editor_comment));
else
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg, comment_line_str,
_(no_scissors_editor_comment), comment_line_char);
}
if (signoff)
diff --git a/builtin/tag.c b/builtin/tag.c
index 07327d3c04..1c708785bf 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -291,10 +291,10 @@ static void create_tag(const struct object_id *object, const char *object_ref,
struct strbuf buf = STRBUF_INIT;
strbuf_addch(&buf, '\n');
if (opt->cleanup_mode == CLEANUP_ALL)
- strbuf_commented_addf(&buf, comment_line_char,
+ strbuf_commented_addf(&buf, comment_line_str,
_(tag_template), tag, comment_line_char);
else
- strbuf_commented_addf(&buf, comment_line_char,
+ strbuf_commented_addf(&buf, comment_line_str,
_(tag_template_nocleanup), tag, comment_line_char);
write_or_die(fd, buf.buf, buf.len);
strbuf_release(&buf);
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 6dfc33e4e3..affc93a8e4 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -71,7 +71,7 @@ void append_todo_help(int command_count,
if (!edit_todo) {
strbuf_addch(buf, '\n');
- strbuf_commented_addf(buf, comment_line_char,
+ strbuf_commented_addf(buf, comment_line_str,
Q_("Rebase %s onto %s (%d command)",
"Rebase %s onto %s (%d commands)",
command_count),
diff --git a/sequencer.c b/sequencer.c
index 6a1b7b200e..852c3f9f4e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -667,11 +667,11 @@ void append_conflicts_hint(struct index_state *istate,
}
strbuf_addch(msgbuf, '\n');
- strbuf_commented_addf(msgbuf, comment_line_char, "Conflicts:\n");
+ strbuf_commented_addf(msgbuf, comment_line_str, "Conflicts:\n");
for (i = 0; i < istate->cache_nr;) {
const struct cache_entry *ce = istate->cache[i++];
if (ce_stage(ce)) {
- strbuf_commented_addf(msgbuf, comment_line_char,
+ strbuf_commented_addf(msgbuf, comment_line_str,
"\t%s\n", ce->name);
while (i < istate->cache_nr &&
!strcmp(ce->name, istate->cache[i]->name))
diff --git a/strbuf.c b/strbuf.c
index e9b6127e76..76d02e0920 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -368,7 +368,7 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
add_lines(out, prefix, buf, size, 1);
}
-void strbuf_commented_addf(struct strbuf *sb, char comment_prefix,
+void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix,
const char *fmt, ...)
{
va_list params;
@@ -379,7 +379,13 @@ void strbuf_commented_addf(struct strbuf *sb, char comment_prefix,
strbuf_vaddf(&buf, fmt, params);
va_end(params);
- strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_prefix);
+ /*
+ * TODO Our commented_lines helper does not yet understand
+ * comment strings. But since we know that the strings are
+ * always single-char, we can cheat for the moment, and
+ * fix this later.
+ */
+ strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_prefix[0]);
if (incomplete_line)
sb->buf[--sb->len] = '\0';
diff --git a/strbuf.h b/strbuf.h
index dc4710adbb..b128ca539a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -379,7 +379,7 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
* blank to the buffer.
*/
__attribute__((format (printf, 3, 4)))
-void strbuf_commented_addf(struct strbuf *sb, char comment_prefix, const char *fmt, ...);
+void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix, const char *fmt, ...);
__attribute__((format (printf,2,0)))
void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
diff --git a/wt-status.c b/wt-status.c
index b5a29083df..2be2eb094c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1103,7 +1103,7 @@ void wt_status_append_cut_line(struct strbuf *buf)
{
const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
- strbuf_commented_addf(buf, comment_line_char, "%s", cut_line);
+ strbuf_commented_addf(buf, comment_line_str, "%s", cut_line);
strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
}
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 06/15] strbuf: accept a comment string for strbuf_stripspace()
From: Jeff King @ 2024-03-07 9:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
As part of our transition to multi-byte comment characters, let's take a
NUL-terminated string pointer for strbuf_stripspace(), rather than a
single character. We can continue to support its feature of ignoring
comments by accepting a NULL pointer (as opposed to the current behavior
of a NUL byte).
All of the callers have to be adjusted, but they can all just pass
comment_line_str (or NULL).
Inside the function we detect comments by comparing the first byte of a
line to the comment character. We'll adjust that to use starts_with(),
which will match multiple bytes (though for now, of course, we still
only allow a single byte, so it's academic).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/branch.c | 2 +-
builtin/notes.c | 2 +-
builtin/rebase.c | 2 +-
builtin/stripspace.c | 2 +-
builtin/tag.c | 2 +-
rebase-interactive.c | 2 +-
sequencer.c | 6 +++---
strbuf.c | 6 +++---
strbuf.h | 4 ++--
9 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5f..c03c0407d1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -678,7 +678,7 @@ static int edit_branch_description(const char *branch_name)
strbuf_release(&buf);
return -1;
}
- strbuf_stripspace(&buf, comment_line_char);
+ strbuf_stripspace(&buf, comment_line_str);
strbuf_addf(&name, "branch.%s.description", branch_name);
if (buf.len || exists)
diff --git a/builtin/notes.c b/builtin/notes.c
index caf20fd5bd..5223a3f350 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -223,7 +223,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
die(_("please supply the note contents using either -m or -F option"));
}
if (d->stripspace)
- strbuf_stripspace(&d->buf, comment_line_char);
+ strbuf_stripspace(&d->buf, comment_line_str);
}
}
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6ead9465a4..bf78402129 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -204,7 +204,7 @@ static int edit_todo_file(unsigned flags)
if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
return error_errno(_("could not read '%s'."), todo_file);
- strbuf_stripspace(&todo_list.buf, comment_line_char);
+ strbuf_stripspace(&todo_list.buf, comment_line_str);
res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);
if (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,
NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..434ac490cb 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -59,7 +59,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
if (mode == STRIP_DEFAULT || mode == STRIP_COMMENTS)
strbuf_stripspace(&buf,
- mode == STRIP_COMMENTS ? comment_line_char : '\0');
+ mode == STRIP_COMMENTS ? comment_line_str : NULL);
else
comment_lines(&buf);
diff --git a/builtin/tag.c b/builtin/tag.c
index 19a7e06bf4..07327d3c04 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -310,7 +310,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,
if (opt->cleanup_mode != CLEANUP_NONE)
strbuf_stripspace(buf,
- opt->cleanup_mode == CLEANUP_ALL ? comment_line_char : '\0');
+ opt->cleanup_mode == CLEANUP_ALL ? comment_line_str : NULL);
if (!opt->message_given && !buf->len)
die(_("no tag message?"));
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d9718409b3..6dfc33e4e3 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -130,7 +130,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
return -2;
- strbuf_stripspace(&new_todo->buf, comment_line_char);
+ strbuf_stripspace(&new_todo->buf, comment_line_str);
if (initial && new_todo->buf.len == 0)
return -3;
diff --git a/sequencer.c b/sequencer.c
index f49a871ac0..6a1b7b200e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1152,7 +1152,7 @@ void cleanup_message(struct strbuf *msgbuf,
strbuf_setlen(msgbuf, wt_status_locate_end(msgbuf->buf, msgbuf->len));
if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE)
strbuf_stripspace(msgbuf,
- cleanup_mode == COMMIT_MSG_CLEANUP_ALL ? comment_line_char : '\0');
+ cleanup_mode == COMMIT_MSG_CLEANUP_ALL ? comment_line_str : NULL);
}
/*
@@ -1184,7 +1184,7 @@ int template_untouched(const struct strbuf *sb, const char *template_file,
return 0;
strbuf_stripspace(&tmpl,
- cleanup_mode == COMMIT_MSG_CLEANUP_ALL ? comment_line_char : '\0');
+ cleanup_mode == COMMIT_MSG_CLEANUP_ALL ? comment_line_str : NULL);
if (!skip_prefix(sb->buf, tmpl.buf, &start))
start = sb->buf;
strbuf_release(&tmpl);
@@ -1557,7 +1557,7 @@ static int try_to_commit(struct repository *r,
if (cleanup != COMMIT_MSG_CLEANUP_NONE)
strbuf_stripspace(msg,
- cleanup == COMMIT_MSG_CLEANUP_ALL ? comment_line_char : '\0');
+ cleanup == COMMIT_MSG_CLEANUP_ALL ? comment_line_str : NULL);
if ((flags & EDIT_MSG) && message_is_empty(msg, cleanup)) {
res = 1; /* run 'git commit' to display error message */
goto out;
diff --git a/strbuf.c b/strbuf.c
index a33aed6c07..e9b6127e76 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1001,10 +1001,10 @@ static size_t cleanup(char *line, size_t len)
*
* If last line does not have a newline at the end, one is added.
*
- * Pass a non-NUL comment_prefix to skip every line starting
+ * Pass a non-NULL comment_prefix to skip every line starting
* with it.
*/
-void strbuf_stripspace(struct strbuf *sb, char comment_prefix)
+void strbuf_stripspace(struct strbuf *sb, const char *comment_prefix)
{
size_t empties = 0;
size_t i, j, len, newlen;
@@ -1018,7 +1018,7 @@ void strbuf_stripspace(struct strbuf *sb, char comment_prefix)
len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
if (comment_prefix && len &&
- sb->buf[i] == comment_prefix) {
+ starts_with(sb->buf + i, comment_prefix)) {
newlen = 0;
continue;
}
diff --git a/strbuf.h b/strbuf.h
index 860fcec5fb..dc4710adbb 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -513,11 +513,11 @@ int strbuf_getcwd(struct strbuf *sb);
int strbuf_normalize_path(struct strbuf *sb);
/**
- * Strip whitespace from a buffer. If comment_prefix is non-NUL,
+ * Strip whitespace from a buffer. If comment_prefix is non-NULL,
* then lines beginning with that character are considered comments,
* thus removed.
*/
-void strbuf_stripspace(struct strbuf *buf, char comment_prefix);
+void strbuf_stripspace(struct strbuf *buf, const char *comment_prefix);
static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
{
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 05/15] environment: store comment_line_char as a string
From: Jeff King @ 2024-03-07 9:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
We'd like to eventually support multi-byte comment prefixes, but the
comment_line_char variable is referenced in many spots, making the
transition difficult.
Let's start by storing the character in a NUL-terminated string. That
will let us switch code over incrementally to the string format, and we
can easily support the existing code with a macro wrapper (since we'll
continue to allow only a single-byte prefix, this will behave
identically).
Once all references to the "char" variable have been converted, we can
drop it and enable longer strings.
We'll still have to touch all of the spots that create or set the
variable in this patch, but there are only a few (reading the config,
and the "auto" character selector).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/commit.c | 4 ++--
config.c | 2 +-
environment.c | 2 +-
environment.h | 3 ++-
4 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index d496980421..d8abbe48b1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -685,7 +685,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
const char *p;
if (!memchr(sb->buf, candidates[0], sb->len)) {
- comment_line_char = candidates[0];
+ comment_line_str = xstrfmt("%c", candidates[0]);
return;
}
@@ -706,7 +706,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
if (!*p)
die(_("unable to select a comment character that is not used\n"
"in the current commit message"));
- comment_line_char = *p;
+ comment_line_str = xstrfmt("%c", *p);
}
static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
diff --git a/config.c b/config.c
index 3cfeb3d8bd..e12ea68f24 100644
--- a/config.c
+++ b/config.c
@@ -1566,7 +1566,7 @@ static int git_default_core_config(const char *var, const char *value,
else if (!strcasecmp(value, "auto"))
auto_comment_line_char = 1;
else if (value[0] && !value[1]) {
- comment_line_char = value[0];
+ comment_line_str = xstrfmt("%c", value[0]);
auto_comment_line_char = 0;
} else
return error(_("core.commentChar should only be one ASCII character"));
diff --git a/environment.c b/environment.c
index 90632a39bc..0a9f5db407 100644
--- a/environment.c
+++ b/environment.c
@@ -110,7 +110,7 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT;
* The character that begins a commented line in user-editable file
* that is subject to stripspace.
*/
-char comment_line_char = '#';
+const char *comment_line_str = "#";
int auto_comment_line_char;
/* Parallel index stat data preload? */
diff --git a/environment.h b/environment.h
index e5351c9dd9..3496474cce 100644
--- a/environment.h
+++ b/environment.h
@@ -8,7 +8,8 @@ struct strvec;
* The character that begins a commented line in user-editable file
* that is subject to stripspace.
*/
-extern char comment_line_char;
+#define comment_line_char (comment_line_str[0])
+extern const char *comment_line_str;
extern int auto_comment_line_char;
/*
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 04/15] strbuf: avoid shadowing global comment_line_char name
From: Jeff King @ 2024-03-07 9:19 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
Several comment-related strbuf functions take a comment_line_char
parameter. There's also a global comment_line_char variable, which is
closely related (most callers pass it in as this parameter). Let's avoid
shadowing the global name. This makes it more obvious that we're not
using the global value, and it will be especially helpful as we refactor
the global in future patches (in particular, any macro trickery wouldn't
work because the preprocessor doesn't respect scope).
We'll use "comment_prefix". That should be descriptive enough, and as a
bonus is more neutral with respect to the "char" type (since we'll
eventually swap it out for a string).
Signed-off-by: Jeff King <peff@peff.net>
---
strbuf.c | 16 ++++++++--------
strbuf.h | 8 ++++----
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index ca80a2c77e..a33aed6c07 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -359,16 +359,16 @@ static void add_lines(struct strbuf *out,
}
void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
- size_t size, char comment_line_char)
+ size_t size, char comment_prefix)
{
char prefix[2];
- prefix[0] = comment_line_char;
+ prefix[0] = comment_prefix;
prefix[1] = '\0';
add_lines(out, prefix, buf, size, 1);
}
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
+void strbuf_commented_addf(struct strbuf *sb, char comment_prefix,
const char *fmt, ...)
{
va_list params;
@@ -379,7 +379,7 @@ void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
strbuf_vaddf(&buf, fmt, params);
va_end(params);
- strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_char);
+ strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_prefix);
if (incomplete_line)
sb->buf[--sb->len] = '\0';
@@ -1001,10 +1001,10 @@ static size_t cleanup(char *line, size_t len)
*
* If last line does not have a newline at the end, one is added.
*
- * Pass a non-NUL comment_line_char to skip every line starting
+ * Pass a non-NUL comment_prefix to skip every line starting
* with it.
*/
-void strbuf_stripspace(struct strbuf *sb, char comment_line_char)
+void strbuf_stripspace(struct strbuf *sb, char comment_prefix)
{
size_t empties = 0;
size_t i, j, len, newlen;
@@ -1017,8 +1017,8 @@ void strbuf_stripspace(struct strbuf *sb, char comment_line_char)
eol = memchr(sb->buf + i, '\n', sb->len - i);
len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
- if (comment_line_char && len &&
- sb->buf[i] == comment_line_char) {
+ if (comment_prefix && len &&
+ sb->buf[i] == comment_prefix) {
newlen = 0;
continue;
}
diff --git a/strbuf.h b/strbuf.h
index e959caca87..860fcec5fb 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -288,7 +288,7 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
*/
void strbuf_add_commented_lines(struct strbuf *out,
const char *buf, size_t size,
- char comment_line_char);
+ char comment_prefix);
/**
@@ -379,7 +379,7 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
* blank to the buffer.
*/
__attribute__((format (printf, 3, 4)))
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char, const char *fmt, ...);
+void strbuf_commented_addf(struct strbuf *sb, char comment_prefix, const char *fmt, ...);
__attribute__((format (printf,2,0)))
void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
@@ -513,11 +513,11 @@ int strbuf_getcwd(struct strbuf *sb);
int strbuf_normalize_path(struct strbuf *sb);
/**
- * Strip whitespace from a buffer. If comment_line_char is non-NUL,
+ * Strip whitespace from a buffer. If comment_prefix is non-NUL,
* then lines beginning with that character are considered comments,
* thus removed.
*/
-void strbuf_stripspace(struct strbuf *buf, char comment_line_char);
+void strbuf_stripspace(struct strbuf *buf, char comment_prefix);
static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
{
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 03/15] commit: refactor base-case of adjust_comment_line_char()
From: Jeff King @ 2024-03-07 9:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
When core.commentChar is set to "auto", we check a set of candidate
characters against the proposed buffer to see which if any can be used
without ambiguity. But before we do that, we optimize for the common
case that the default "#" is fine by just seeing if it is present in the
buffer at all.
The way we do this is a bit subtle, though: we assign the candidate
character to comment_line_char preemptively, then check if it works, and
return if it does. The subtle part is that sometimes setting
comment_line_char is important (after we return, the important outcome
is the fact that we have set the variable) and sometimes it is useless
(if our optimization fails, we go on to do the more careful checks and
eventually assign something else instead).
To make it more clear what is happening (and to make further refactoring
of comment_line_char easier), let's check our candidate character
directly, and then assign as part of returning if it worked out.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/commit.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 6d1fa71676..d496980421 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -684,9 +684,10 @@ static void adjust_comment_line_char(const struct strbuf *sb)
char *candidate;
const char *p;
- comment_line_char = candidates[0];
- if (!memchr(sb->buf, comment_line_char, sb->len))
+ if (!memchr(sb->buf, candidates[0], sb->len)) {
+ comment_line_char = candidates[0];
return;
+ }
p = sb->buf;
candidate = strchr(candidates, *p);
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 02/15] strbuf: avoid static variables in strbuf_add_commented_lines()
From: Jeff King @ 2024-03-07 9:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
In strbuf_add_commented_lines(), we have to convert the single-byte
comment_line_char into a string to pass to add_lines(). We cache the
created string using a static-local variable. But this makes the
function non-reentrant, and it's doubtful that this provides any real
performance benefit given that we know the string always contains a
single character.
So let's just create it from scratch each time, and to give the compiler
the maximal opportunity to make it fast we'll ditch the over-complicated
xsnprintf() and just assign directly into the array.
Signed-off-by: Jeff King <peff@peff.net>
---
In the long run we'll end up just passing in the comment-string that the
caller gives us, so this patch could arguably be dropped until that
point.
strbuf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index 689d8acd5e..ca80a2c77e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -361,10 +361,10 @@ static void add_lines(struct strbuf *out,
void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
size_t size, char comment_line_char)
{
- static char prefix[2];
+ char prefix[2];
- if (prefix[0] != comment_line_char)
- xsnprintf(prefix, sizeof(prefix), "%c", comment_line_char);
+ prefix[0] = comment_line_char;
+ prefix[1] = '\0';
add_lines(out, prefix, buf, size, 1);
}
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 01/15] strbuf: simplify comment-handling in add_lines() helper
From: Jeff King @ 2024-03-07 9:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
In strbuf_add_commented_lines(), we prepare two strings with potential
prefixes: one with just the comment char, and one with an additional
space. In the add_lines() helper, we use the one without the extra space
for blank lines or lines starting with a tab.
While passing in two separate prefixes to the helper is very flexible,
it's more flexibility than we actually use (or are likely to use, since
the rules inside add_lines() only make sense if "prefix2" is a variant
of "prefix1" without the extra space). And setting up the two strings
makes refactoring in strbuf_add_commented_lines() awkward.
Instead, let's pass in a single string, and just let add_lines() add the
extra space to the result as appropriate.
We do still need to pass in a flag to trigger this behavior. The helper
is shared by strbuf_add_lines(), which passes in a NULL "prefix2" to
inhibit this extra handling.
Signed-off-by: Jeff King <peff@peff.net>
---
strbuf.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index 7827178d8e..689d8acd5e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -340,18 +340,17 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
}
static void add_lines(struct strbuf *out,
- const char *prefix1,
- const char *prefix2,
- const char *buf, size_t size)
+ const char *prefix,
+ const char *buf, size_t size,
+ int space_after_prefix)
{
while (size) {
- const char *prefix;
const char *next = memchr(buf, '\n', size);
next = next ? (next + 1) : (buf + size);
- prefix = ((prefix2 && (buf[0] == '\n' || buf[0] == '\t'))
- ? prefix2 : prefix1);
strbuf_addstr(out, prefix);
+ if (space_after_prefix && buf[0] != '\n' && buf[0] != '\t')
+ strbuf_addch(out, ' ');
strbuf_add(out, buf, next - buf);
size -= next - buf;
buf = next;
@@ -362,14 +361,11 @@ static void add_lines(struct strbuf *out,
void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
size_t size, char comment_line_char)
{
- static char prefix1[3];
- static char prefix2[2];
+ static char prefix[2];
- if (prefix1[0] != comment_line_char) {
- xsnprintf(prefix1, sizeof(prefix1), "%c ", comment_line_char);
- xsnprintf(prefix2, sizeof(prefix2), "%c", comment_line_char);
- }
- add_lines(out, prefix1, prefix2, buf, size);
+ if (prefix[0] != comment_line_char)
+ xsnprintf(prefix, sizeof(prefix), "%c", comment_line_char);
+ add_lines(out, prefix, buf, size, 1);
}
void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
@@ -750,7 +746,7 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
void strbuf_add_lines(struct strbuf *out, const char *prefix,
const char *buf, size_t size)
{
- add_lines(out, prefix, NULL, buf, size);
+ add_lines(out, prefix, buf, size, 0);
}
void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s)
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 0/15] allow multi-byte core.commentChar
From: Jeff King @ 2024-03-07 9:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240306080804.GB4099518@coredump.intra.peff.net>
On Wed, Mar 06, 2024 at 03:08:04AM -0500, Jeff King wrote:
> For a more readable series, I'd guess it would make sense to introduce
> comment_line_str as a separate variable (but continue to enforce the
> single-char rule), convert the easy cases en masse, the tricky cases one
> by one, and then finally drop comment_line_char entirely. At which point
> the config rules can be lifted to allow multi-byte strings.
I ended up cleaning this up. Like I said, this isn't something I'm
personally that interested in. But it just seemed like a wart that this
one spot could not handle multi-byte characters that all the cool kids
are using in their prompts etc these days.
Plus it was kind of an interesting puzzle for how to lay out the
refactoring to make each step self-consistent. At the very least, I
think the first couple of cleanups are worth it even if we do not see
the whole thing through. ;)
It obviously nullifies kh/doc-commentchar-is-a-byte, which is in 'next'.
Sadly "git merge" does not find a conflict with the documentation update
in patch 15, so we'll have to remember to pick up one topic or the
other.
I'm using U+00BB as my commentChar for now to see if any bugs show up,
but I expect I'll get sick of it after a few days.
[01/15]: strbuf: simplify comment-handling in add_lines() helper
[02/15]: strbuf: avoid static variables in strbuf_add_commented_lines()
[03/15]: commit: refactor base-case of adjust_comment_line_char()
[04/15]: strbuf: avoid shadowing global comment_line_char name
These four are cleanups that could be taken independently.
[05/15]: environment: store comment_line_char as a string
This one preps us for incrementally moving code over to the new
system.
[06/15]: strbuf: accept a comment string for strbuf_stripspace()
[07/15]: strbuf: accept a comment string for strbuf_commented_addf()
[08/15]: strbuf: accept a comment string for strbuf_add_commented_lines()
[09/15]: prefer comment_line_str to comment_line_char for printing
[10/15]: find multi-byte comment chars in NUL-terminated strings
[11/15]: find multi-byte comment chars in unterminated buffers
[12/15]: sequencer: handle multi-byte comment characters when writing todo list
[13/15]: wt-status: drop custom comment-char stringification
These ones are the actual transition.
[14/15]: environment: drop comment_line_char compatibility macro
[15/15]: config: allow multi-byte core.commentChar
And then we tie it off by dropping the now-unused bits and loosening
the config logic.
Documentation/config/core.txt | 4 ++-
add-patch.c | 14 +++++-----
builtin/branch.c | 8 +++---
builtin/commit.c | 19 +++++++-------
builtin/merge.c | 12 ++++-----
builtin/notes.c | 10 ++++----
builtin/rebase.c | 2 +-
builtin/stripspace.c | 4 +--
builtin/tag.c | 14 +++++-----
commit.c | 3 ++-
config.c | 6 ++---
environment.c | 2 +-
environment.h | 2 +-
fmt-merge-msg.c | 8 +++---
rebase-interactive.c | 10 ++++----
sequencer.c | 48 ++++++++++++++++++-----------------
strbuf.c | 47 ++++++++++++++++++----------------
strbuf.h | 9 ++++---
t/t0030-stripspace.sh | 5 ++++
t/t7507-commit-verbose.sh | 10 ++++++++
t/t7508-status.sh | 4 ++-
trailer.c | 6 ++---
wt-status.c | 31 +++++++++-------------
23 files changed, 149 insertions(+), 129 deletions(-)
^ permalink raw reply
* [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code
From: Jeff King @ 2024-03-07 8:56 UTC (permalink / raw)
To: git; +Cc: brian m. carlson
In-Reply-To: <20240307084735.GA2072130@coredump.intra.peff.net>
Git's transport-helper code has always sent "option object-format\n",
and never provided the "true" or "algorithm" arguments. While the
"algorithm" request is something we might need or want to eventually
support, it probably makes sense for now to document the actual
behavior, especially as it has been in place for several years, since
8b85ee4f47 (transport-helper: implement object-format extensions,
2020-05-25).
Signed-off-by: Jeff King <peff@peff.net>
---
As I discussed in patch 1, remote-curl does handle the "true" thing
correctly. And that's really the helper that matters in practice (it's
possible some third party helper is looking for the explicit "true", but
presumably they'd have reported their confusion to the list). So we
could probably just start tacking on the "true" in transport-helper.c
and leave that part of the documentation untouched.
I'm less sure of the specific-algorithm thing, just because it seems
like remote-curl would never make use of it anyway (preferring instead
to match whatever algorithm is used by the http remote). But maybe there
are pending interoperability plans that depend on this?
I guess it would not hurt to leave it in place even if transport-helper
never produces it. On the other hand, any helper which advertises the
"object-format" capability is supposed to support it, and without the
transport-helper side being implemented, I don't know how any helper
program can claim that.
Documentation/gitremote-helpers.txt | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 07c8439a6f..12dffbf383 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -542,13 +542,10 @@ set by Git if the remote helper has the 'option' capability.
transaction. If successful, all refs will be updated, or none will. If the
remote side does not support this capability, the push will fail.
-'option object-format' {'true'|algorithm}::
- If 'true', indicate that the caller wants hash algorithm information
+'option object-format'::
+ Indicate that the caller wants hash algorithm information
to be passed back from the remote. This mode is used when fetching
refs.
-+
-If set to an algorithm, indicate that the caller wants to interact with
-the remote side using that algorithm.
SEE ALSO
--------
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 1/2] t5801: fix object-format handling in git-remote-testgit
From: Jeff King @ 2024-03-07 8:51 UTC (permalink / raw)
To: git; +Cc: brian m. carlson
In-Reply-To: <20240307084735.GA2072130@coredump.intra.peff.net>
Our fake remote helper tries to handle the object-format capability,
courtesy of 3716d50dd5 (remote-testgit: adapt for object-format,
2020-06-19). But its parsing isn't quite right; it expects to receive
"option object-format true", but the transport-helper code just sends
"option object-format" with no value.
As a result, we never set the $object_format variable to "true". And
worse, because $val is used unquoted, this confuses the shell's "test"
command, which prints something like:
.../git/t/t5801/git-remote-testgit: 150: test: =: unexpected operator
It all turns out to be harmless, though, because we never look at
$object_format after that!
The Git-side behavior comes from 8b85ee4f47 (transport-helper: implement
object-format extensions, 2020-05-25). It is a bit unlike other "option"
variables, which always say "true" or "false". But in this case, there's
not really any need to do so. As I understand it from that commit, the
sequence is something like:
1. the remote helper in its capabilities list says "object-format" to
tell Git that it understands the object-format option.
2. Git then tells the helper "option object-format" to tell it that it
too understands object-formats.
3. when the remote helper lists refs, it sends a special
":object-format" line that tells Git which object format it is
using. But it presumably should only do this if we found out that
the other side supports object-formats in step (2).
So let's improve our remote-testgit helper a bit:
- when we see an object-format line, just set object_format=true;
that's the only useful thing to take away from it
- make sure that object_format is set before sending the special
":object-format" line. Since we're always testing against a version
of Git recent enough to have sent us the object-format option, this
is mostly a noop. But it confirms that the transport-helper code is
correctly sending us the option (if we fail to send the line, then
the test will fail when run with GIT_TEST_DEFAULT_HASH=sha256).
Signed-off-by: Jeff King <peff@peff.net>
---
The only other helper we ship that knows about object-format is
remote-curl. And there it _does_ expect "true" or an algorithm.
Curiously the "true" thing works because the remote-curl code silently
rewrites "option foo" to be the same as "option foo true". And even
though it understands receiving a specific algorithm, I'm not sure it
would do anything useful (whatever the caller says is generally
overwritten by the info/refs response).
So I dunno.
t/t5801/git-remote-testgit | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
index 1544d6dc6b..b348608847 100755
--- a/t/t5801/git-remote-testgit
+++ b/t/t5801/git-remote-testgit
@@ -25,6 +25,7 @@ GIT_DIR="$url/.git"
export GIT_DIR
force=
+object_format=
mkdir -p "$dir"
@@ -56,7 +57,8 @@ do
echo
;;
list)
- echo ":object-format $(git rev-parse --show-object-format=storage)"
+ test -n "$object_format" &&
+ echo ":object-format $(git rev-parse --show-object-format=storage)"
git for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/'
head=$(git symbolic-ref HEAD)
echo "@$head HEAD"
@@ -142,7 +144,7 @@ do
echo "ok"
;;
object-format)
- test $val = "true" && object_format="true" || object_format=
+ object_format=true
echo "ok"
;;
*)
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [RFC/PATCH 0/2] some transport-helper "option object-format" confusion
From: Jeff King @ 2024-03-07 8:47 UTC (permalink / raw)
To: git; +Cc: brian m. carlson
I happened to be looking at the output of t5801 for an unrelated
problem, and I noticed our git-remote-testgit spewing a bunch of shell
errors. It turns out that its expectations do not quite match what the
transport-helper code produces.
This series brings the test and documentation in line with how the
transport-helper code behaves. But I'm not sure if we should be going
the other way (see the comments on patch 2 especially), and bringing the
transport-helper code in line with the others. Hence the RFC.
[1/2]: t5801: fix object-format handling in git-remote-testgit
[2/2]: doc/gitremote-helpers: match object-format option docs to code
Documentation/gitremote-helpers.txt | 7 ++-----
t/t5801/git-remote-testgit | 6 ++++--
2 files changed, 6 insertions(+), 7 deletions(-)
-Peff
^ permalink raw reply
* [PATCH] doc/gitremote-helpers: fix missing single-quote
From: Jeff King @ 2024-03-07 8:43 UTC (permalink / raw)
To: git
The formatting around "option push-option" was missing its closing
quote, leading to the output having a stray opening quote, rather than
rendering the item in italics (as we do for all of the other options in
the list).
Signed-off-by: Jeff King <peff@peff.net>
---
Just happened to notice this while looking at the rendered manpage for a
different option.
Documentation/gitremote-helpers.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index ed8da428c9..07c8439a6f 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -526,7 +526,7 @@ set by Git if the remote helper has the 'option' capability.
'option pushcert' {'true'|'false'}::
GPG sign pushes.
-'option push-option <string>::
+'option push-option' <string>::
Transmit <string> as a push option. As the push option
must not contain LF or NUL characters, the string is not encoded.
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Elijah Newren @ 2024-03-07 8:22 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Stefan Haller, Derrick Stolee, Phillip Wood, Christian Couder,
git
In-Reply-To: <25aa1b8d-79f6-4b33-be22-735a867367c0@app.fastmail.com>
Hi,
On Wed, Mar 6, 2024 at 11:59 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
>
> On Tue, Mar 5, 2024, at 08:40, Stefan Haller wrote:
> > Coming back to this after almost a year, I can say that I'm still
> > running into this problem relatively frequently, and it is annoying
> > every single time. Excluding refs pointing at the current head from
> > being updated, as proposed above, would be a big usability improvement
> > for me.
>
> Sounds like a ref-stash command is in order…
A what?
> # I want a new branch
> git checkout -b new
> # But I don’t want it to be affected by the next rebase
This doesn't make any sense; rebase always operates on the current
branch, and Stefan wasn't asking for anything otherwise. He was just
concerned that with --update-refs, one of the other branches it also
operated on was one he didn't want it to operate on.
Perhaps you meant
git branch new
for your first command?
> git ref-stash push
> git rebase [...]
> # Now I’m done: put the ref back where it was
> git ref-stash pop
Leaving aside questions about how ref-stash is supposed to interact
with each and every other command out there, and how it's supposed to
know which branches it's operating on when you do pushes and pops...
Why do we need to invent a new command, when we already have the
reflog? You could drop both ref-stash commands, and instead just have
a
git branch -f new new@{1}
at the end (assuming of course "git branch new" was used instead of
your "git checkout -b new", as I suggested earlier) to put "new" back
to where it was before the rebase. That's fewer commands.
Or, even simpler, drop the initial branch creation and both ref-stash
commands by just not creating the branch until after the rebase.
That'd make the entire set of commands just be:
git rebase --update-refs [...]
git branch new current_branch@{1}
Plus, either solution works today and needs no new changes.
^ permalink raw reply
* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Kristoffer Haugsbakk @ 2024-03-07 7:59 UTC (permalink / raw)
To: Stefan Haller
Cc: Derrick Stolee, Elijah Newren, Phillip Wood, Christian Couder,
git
In-Reply-To: <354f9fed-567f-42c8-9da9-148a5e223022@haller-berlin.de>
On Tue, Mar 5, 2024, at 08:40, Stefan Haller wrote:
> Coming back to this after almost a year, I can say that I'm still
> running into this problem relatively frequently, and it is annoying
> every single time. Excluding refs pointing at the current head from
> being updated, as proposed above, would be a big usability improvement
> for me.
Sounds like a ref-stash command is in order…
# I want a new branch
git checkout -b new
# But I don’t want it to be affected by the next rebase
git ref-stash push
git rebase [...]
# Now I’m done: put the ref back where it was
git ref-stash pop
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 8/8] Documentation/git-config: update to new-style syntax
From: Patrick Steinhardt @ 2024-03-07 7:33 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cQevQKb8YA1Yf-t=AHg11P7i848gELx2kPc39g5cOWbqw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]
On Thu, Mar 07, 2024 at 01:57:41AM -0500, Eric Sunshine wrote:
> On Wed, Mar 6, 2024 at 6:32 AM Patrick Steinhardt <ps@pks.im> wrote:
> > Update our documentation of git-config(1) to stop mentioning the old
> > syntax while starting to mention the new syntax. Remove the help
> > mismatch in t0450 so that we start to ensure that the manpage and
> > builtin synopsis match.
> > [...]
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
> I'd like to push back on the part of this patch which completely
> eradicates mention of the old syntax. Doing so makes it difficult for
> people to figure out the meaning of git-config commands which they run
> across in blogs and existing tooling. For instance, the following
> recommendation is commonly encountered in the wild:
>
> git config --global user.name "Your Name"
> git config --global user.email "youremail@yourdomain.com"
>
> Typically, we instead retain the old syntax (or options or whatever)
> in the documentation so that people who want to learn can learn, but
> we rewrite it to make it clear that it is deprecated, and explicitly
> point the reader at the modern replacement command or option (or
> whatever).
>
> So, perhaps you can have a SYNOPSIS section for the deprecated usage,
> as well as a "DEPRECATED OPTIONS" section (or such), so that we don't
> leave readers entirely in the dark.
Fair point indeed. I will update this patch accordingly, thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 6/8] builtin/config: introduce subcommands
From: Patrick Steinhardt @ 2024-03-07 7:14 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZSg35atoTsBVtWj_j94n2Mt8otzfw3DYLYS9FCbNgp_Xg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]
On Wed, Mar 06, 2024 at 01:38:25PM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > @@ -910,6 +930,20 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> > {
> > given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
> >
> > + /*
> > + * This is somewhat hacky: we first parse the command line while
> > + * keeping all args intact in order to determine whether a subcommand
> > + * has been specified. If so, we re-parse it a second time, but this
> > + * time we drop KEEP_ARGV0. This is so that we don't munge the command
> > + * line in case no subcommand was given, which would otherwise confuse
> > + * us when parsing the implicit modes.
> > + */
> > + argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage,
> > + PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_ARGV0|PARSE_OPT_KEEP_UNKNOWN_OPT);
> > + if (subcommand)
> > + argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage,
> > + PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_UNKNOWN_OPT);
> > +
>
> I wonder if we can drop the PARSE_OPT_SUBCOMMAND_OPTIONAL in the second
> iteration to make it stricter. But this is OK.
We can't due to a restriction in the parse-options interface: when
subcommands are present, then PARSE_OPT_KEEP_UNKNOWN_OPT requires us to
also pass PARSE_OPT_SUBCOMMAND_OPTIONAL. Otherwise we trigger a `BUG()`.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/8] builtin/config: use `OPT_CMDMODE()` to specify modes
From: Patrick Steinhardt @ 2024-03-07 7:02 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZekBznq74P3QVsSC@nand.local>
[-- Attachment #1: Type: text/plain, Size: 3134 bytes --]
On Wed, Mar 06, 2024 at 06:52:46PM -0500, Taylor Blau wrote:
> On Wed, Mar 06, 2024 at 12:31:42PM +0100, Patrick Steinhardt wrote:
> > The git-config(1) command has various different modes which are
> > accessible via e.g. `--get-urlmatch` or `--unset-all`. These modes are
> > declared with `OPT_BIT()`, which causes two minor issues:
> >
> > - The respective modes also have a negated form `--no-get-urlmatch`,
> > which is unintended.
> >
> > - We have to manually handle exclusiveness of the modes.
> >
> > Switch these options to instead use `OPT_CMDMODE()`, which is made
> > exactly for this usecase. Remove the now-unneeded check that only a
> > single mode is given, which is now handled by the parse-options
> > interface.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
> > + OPT_CMDMODE(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET),
> > + OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL),
> > + OPT_CMDMODE(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP),
> > + OPT_CMDMODE(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
>
> Expanding a little on my reply to Junio later on in the thread, I
> suspect that these would translate into "get", "get --all", "get
> --regexp", and "get --urlmatch", respectively.
Yup.
> > + OPT_CMDMODE(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL),
> > + OPT_CMDMODE(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
> > + OPT_CMDMODE(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET),
> > + OPT_CMDMODE(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL),
>
> Same with this one turning into "unset --all".
Yup.
> > + OPT_CMDMODE(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
> > + OPT_CMDMODE(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
> > + OPT_CMDMODE('l', "list", &actions, N_("list all"), ACTION_LIST),
> > + OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
> > + OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
> > + OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
>
> These two could probably join the other "get-xyz" modes, and similarly
> become "get --color", and "get --colorbool", respectively. It looks like
> that's where they lived originally... perhaps I'm missing something as
> to why they were moved.
Yes, although I think putting it into the `--type` parameter might be
even better.
I'm still a bit torn on whether we should do this all as part of this
patch series or as part of a follow-up patch series. But if anybody
feels strongly one way or the other I'm happy to adapt accordingly.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 8/8] Documentation/git-config: update to new-style syntax
From: Eric Sunshine @ 2024-03-07 6:57 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <5ff2bf4a2286431a3b3048e0ea04d6551006d0d7.1709724089.git.ps@pks.im>
On Wed, Mar 6, 2024 at 6:32 AM Patrick Steinhardt <ps@pks.im> wrote:
> Update our documentation of git-config(1) to stop mentioning the old
> syntax while starting to mention the new syntax. Remove the help
> mismatch in t0450 so that we start to ensure that the manpage and
> builtin synopsis match.
> [...]
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
I'd like to push back on the part of this patch which completely
eradicates mention of the old syntax. Doing so makes it difficult for
people to figure out the meaning of git-config commands which they run
across in blogs and existing tooling. For instance, the following
recommendation is commonly encountered in the wild:
git config --global user.name "Your Name"
git config --global user.email "youremail@yourdomain.com"
Typically, we instead retain the old syntax (or options or whatever)
in the documentation so that people who want to learn can learn, but
we rewrite it to make it clear that it is deprecated, and explicitly
point the reader at the modern replacement command or option (or
whatever).
So, perhaps you can have a SYNOPSIS section for the deprecated usage,
as well as a "DEPRECATED OPTIONS" section (or such), so that we don't
leave readers entirely in the dark.
^ permalink raw reply
* Re: [PATCH 5/8] builtin/config: track subcommands by action
From: Patrick Steinhardt @ 2024-03-07 6:37 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git
In-Reply-To: <6044285.lOV4Wx5bFT@cayenne>
[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]
On Wed, Mar 06, 2024 at 10:54:01PM +0100, Jean-Noël AVILA wrote:
> Le mercredi 6 mars 2024, 12:31:50 CET Patrick Steinhardt a écrit :
[snip]
> > @@ -851,20 +868,20 @@ static struct option builtin_config_options[] = {
> > OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")),
> > OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")),
> > OPT_GROUP(N_("Action")),
> > - OPT_CMDMODE(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET),
> > - OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL),
> > - OPT_CMDMODE(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP),
> > - OPT_CMDMODE(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
> > - OPT_CMDMODE(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL),
> > - OPT_CMDMODE(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
> > - OPT_CMDMODE(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET),
> > - OPT_CMDMODE(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL),
> > - OPT_CMDMODE(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
> > - OPT_CMDMODE(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
> > - OPT_CMDMODE('l', "list", &actions, N_("list all"), ACTION_LIST),
> > - OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
> > - OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
> > - OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
> > + OPT_CMDMODE(0, "get", &action, N_("get value: name [value-pattern]"), ACTION_GET),
>
> I guess value-pattern is a placeholder. So for uniformity with the
> style guidelines, it should be formatted <value-pattern>. This also
> applies to other placeholders below.
Ah, indeed, thanks for catching this!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/8] builtin/config: track subcommands by action
From: Patrick Steinhardt @ 2024-03-07 6:36 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZekF4aqq+NUf52go@nand.local>
[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]
On Wed, Mar 06, 2024 at 07:10:09PM -0500, Taylor Blau wrote:
> On Wed, Mar 06, 2024 at 12:31:50PM +0100, Patrick Steinhardt wrote:
> > @@ -976,33 +993,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> > key_delim = '\n';
> > }
> >
> > - if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
> > - error(_("--get-color and variable type are incoherent"));
> > - usage_builtin_config();
> > - }
> > -
> > - if (actions == 0)
> > + if (action == ACTION_NONE) {
> > switch (argc) {
> > - case 1: actions = ACTION_GET; break;
> > - case 2: actions = ACTION_SET; break;
> > - case 3: actions = ACTION_SET_ALL; break;
> > + case 1: action = ACTION_GET; break;
> > + case 2: action = ACTION_SET; break;
> > + case 3: action = ACTION_SET_ALL; break;
>
> Wondering aloud a little bit... is it safe to set us to "ACTION_SET",
> for example, if we have exactly two arguments? On the one hand, it seems
> like the answer is yes. But on the other hand, if we were invoked as:
>
> $ git config ste foo
>
> We would get something like:
>
> $ git config ste foo
> error: key does not contain a section: ste
>
> which is sensible. It would be nice to say something more along the
> lines of "oops, you probably meant 'set' instead of 'ste'". I think you
> could claim that the error on the user's part is one of:
>
> - (using the new style 'git config') misspelling "set" as "ste", and
> thus trying to invoke a non-existent sub-command
>
> - (using the old style) trying to set the key "ste", which does not
> contain a section name, and thus is not a valid configuration key
>
> I have no idea which is more likely, and I think that there isn't a good
> way to distinguish between the two at all. So I think the error message
> is OK as-is, but let me know if you have other thoughts.
The transition period will be a tad weird, I agree. I think initially
I'd prefer to keep the old behaviour just to ensure we don't regress
anything, but after a couple of releases I think we should revisit this
and become more aggressive about using the new style.
> > default:
> > usage_builtin_config();
> > }
> > + }
> > + if (action <= ACTION_NONE || action >= ARRAY_SIZE(subcommands_by_action))
> > + BUG("invalid action %d", action);
> > + subcommand = subcommands_by_action[action];
> > +
> > + if (type && (subcommand == cmd_config_get_color ||
> > + subcommand == cmd_config_get_colorbool)) {
> I don't think there's anything wrong with using the function-pointer
> equality here to figure out which subcommand we're in, but I wonder if
> it might be cleaner to write this as:
>
> if (type && (action == ACTION_GET_COLOR ||
> action == ACTION_GET_COLORBOOL)) {
> ...
The reason I didn't is that we don't always have an action set once we
support subcommands. We can of course figure it out by walking the array
of functions pointers to find the one corresponding to this action. But
I figured it's easier to just use function pointers exclusively.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Patrick Steinhardt @ 2024-03-07 6:31 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git
In-Reply-To: <ZekAWSqr9qb8FIAD@nand.local>
[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]
On Wed, Mar 06, 2024 at 06:46:33PM -0500, Taylor Blau wrote:
> On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > > - `git config --get-urlmatch` -> `git config get-urlmatch`.
> >
> > ... is a Meh to me, personally. I'd not actively push it
> > enthusiastically, but I'd passively accept its existence.
>
> I don't have strong feelings about this, but I wonder if `--urlmatch`
> (or `--url-match`) might be an argument to the "get" mode of this
> sub-command instead. Something like `git config get --urlmatch` feels
> much more natural to me than `git config get-urlmatch`.
Agreed. I allude to this somewhere in the patch series that I only see
this as a first incremental step, and that some of the subcommands
really should be converted to become options eventually. Specifically:
- `git config get-urlmatch` -> `git config get --urlmatch`
- `git config get-regexp` -> `git config get --regexp`
- `git config get-all` -> `git config get --all`
- `git config set-all` -> `git config set --all`
- `git config unset-all` -> `git config unset --all`
I didn't yet do it as part of this patch series because I didn't want to
make functional changes at the same time (well, except of course for the
introduction of subcommands). But if we all agree that this patch series
is something we want _and_ that we don't want to have an intermediate
step with the above somewhat weird subcommands then I would be happy to
pile onto the series.
I wouldn't think that keeping this intermediate step would be too bad
though. While we would eventually omit the above subcommands, the infra
to keep them around needs to stay anyway to support old syntax like
`--get-all`. Thus, we can keep the subcommands themselves almost for
free even if we eventually decide to hide them and replace them with
flags.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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