* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Junio C Hamano @ 2023-10-29 23:53 UTC (permalink / raw)
To: Jeff King; +Cc: Kousik Sanagavarapu, Liam Beguin, git
In-Reply-To: <20231028021301.GA35796@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Oct 28, 2023 at 09:12:06AM +0900, Junio C Hamano wrote:
>
>> Grouping @gmail.com addresses do not smell all that useful, though.
>> ...
> One way you could directly use this is in shortlog, which these days
> lets you group by specific formats. So:
>
> git shortlog -ns --group=format:%aA
>
> is potentially useful.
Exactly. That is what I meant by "Grouping", and I agree with you
about "potentially" part, too ;-) Throwing all @gmail.com addresses
into a single bin would not be very useful.
> ... If we could spell it as
> %(authoremail:domain) that would remove the question. But given the
> existence of "%al", I'm not too sad to see another letter allocated to
> this purpose in the meantime.
Another line of thought is perhaps it is potentially useful to teach
the --format= machinery to be a bit more programmable, e.g. allowing
to compute a substring of an existing field %{%aE#*@} without having
to waste a letter each for the local part and domain part. But as I
already said, we are now talking about "postprocessing", and adding
complexity to our codebase only to have incomplete flexibility may
not be worth it. A more specific %(authoremail:localpart) and its
domain counterpart may be easier to explain and understand.
In any case, it is a bit too late to say "let's not waste the
precious single letter namespace to add useless features", as we
have come way too far, so I do not mind too much using a currently
unused letter $X for yet another author and committer trait.
^ permalink raw reply
* Re: [PATCH v4 1/2] t0091-bugreport: stop using i18ngrep
From: Junio C Hamano @ 2023-10-29 23:59 UTC (permalink / raw)
To: emilyshaffer; +Cc: git, Emily Shaffer
In-Reply-To: <20231026182231.3369370-2-nasamuffin@google.com>
emilyshaffer@google.com writes:
> From: Emily Shaffer <nasamuffin@google.com>
>
> Since e6545201ad (Merge branch 'ab/detox-config-gettext', 2021-04-13),
> test_i18ngrep is no longer required. Quit using it in the bugreport
> tests, since it's setting a bad example for tests added later.
>
> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
> ---
Makes sense. Thanks.
> t/t0091-bugreport.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index f6998269be..e1588f71b7 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -65,7 +65,7 @@ test_expect_success '--output-directory puts the report in the provided dir' '
>
> test_expect_success 'incorrect arguments abort with usage' '
> test_must_fail git bugreport --false 2>output &&
> - test_i18ngrep usage output &&
> + grep usage output &&
> test_path_is_missing git-bugreport-*
> '
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: make CMDMODE errors more precise
From: Junio C Hamano @ 2023-10-30 0:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <cefdba32-db0b-4f68-954e-9d31fc12b1a0@web.de>
René Scharfe <l.s.r@web.de> writes:
> Store the argument of PARSE_OPT_CMDMODE options of type OPTION_CALLBACK
> as well to allow taking over the responsibility for compatibility
> checking from the callback function. The next patch will use this
> capability to fix the messages for git am --show-current-patch.
>
> Use a linked list for storing the PARSE_OPT_CMDMODE variables. This
> somewhat outdated data structure is simple and suffices, as the number
> of elements per command is currently only zero or one. We do support
> multiple different command modes variables per command, but I don't
> expect that we'd ever use a significant number of them. Once we do we
> can switch to a hashmap.
Makes quite a lot of sense. I would have expected to see this in
the parse_options_check() function, where other sanity checks are
done, but I think the reason you added the call to its caller
because parse_options_check() does not take the context.
It is not like we want to perform a sanity check that is independent
from a particular invocation of parse_options() on the options[]
array only just once, and want to reuse the array that is known to
be sane multiple times. The check is called once for every
invocation, so with or without this change, I do not see a good
reason for parse_options_check() not to take context, though.
> +static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
> + const struct option *opt,
> + enum opt_parsed flags)
> +{
> + const char *arg = NULL;
> + enum parse_opt_result result = do_get_value(p, opt, flags, &arg);
> + struct parse_opt_cmdmode_list *elem = p->cmdmode_list;
> + char *opt_name, *other_opt_name;
> +
> + for (; elem; elem = elem->next) {
> + if (*elem->value_ptr == elem->value)
> + continue;
> +
> + if (elem->opt &&
> + (elem->opt->flags | opt->flags) & PARSE_OPT_CMDMODE)
> + break;
> +
> + elem->opt = opt;
> + elem->arg = arg;
> + elem->flags = flags;
> + elem->value = *elem->value_ptr;
> + }
> +
> + if (result || !elem)
> + return result;
> +
> + opt_name = optnamearg(opt, arg, flags);
> + other_opt_name = optnamearg(elem->opt, elem->arg, elem->flags);
> + error(_("%s is incompatible with %s"), opt_name, other_opt_name);
> + free(opt_name);
> + free(other_opt_name);
> + return -1;
> +}
Looks quite involved but the overhead is number of supported cmdmode
options per each command line option, and the problems outlined in
the proposed log message are worth addressing. OK.
> @@ -1006,6 +1041,11 @@ int parse_options(int argc, const char **argv,
> precompose_argv_prefix(argc, argv, NULL);
> free_preprocessed_options(real_options);
> free(ctx.alias_groups);
> + for (struct parse_opt_cmdmode_list *elem = ctx.cmdmode_list; elem;) {
> + struct parse_opt_cmdmode_list *next = elem->next;
> + free(elem);
> + elem = next;
> + }
> return parse_options_end(&ctx);
> }
OK.
^ permalink raw reply
* Re: [PATCH v3] bugreport: reject positional arguments
From: Junio C Hamano @ 2023-10-30 0:15 UTC (permalink / raw)
To: Phillip Wood
Cc: Eric Sunshine, emilyshaffer, git, Emily Shaffer, Sheik,
Dragan Simic
In-Reply-To: <3e15f266-c790-4b71-84b6-1328339425c1@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 5/5] ci: add support for GitLab CI
From: Junio C Hamano @ 2023-10-30 0:22 UTC (permalink / raw)
To: Phillip Wood; +Cc: Patrick Steinhardt, git, Oswald Buddenhagen
In-Reply-To: <0d889da1-7fd8-4e21-965f-6222e4433ecf@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I agree we don't want to support too many CI platforms but I think
> adding support for GitLab is good as it helps to stop us being too
> tied to GitHub Actions (which should make it easier if we ever need to
> transition to a different platform in the future) and provides an
> alternative for contributors who want to use a different platform.
> ...
> Having someone committed to on-going maintenance is great.
Yeah, it is great to see that stakeholder companies are helping Git
in ways they can.
Will queue.
^ permalink raw reply
* Re: [PATCH v3] bugreport: reject positional arguments
From: Junio C Hamano @ 2023-10-30 0:26 UTC (permalink / raw)
To: Phillip Wood
Cc: Eric Sunshine, emilyshaffer, git, Emily Shaffer, Sheik,
Dragan Simic
In-Reply-To: <3e15f266-c790-4b71-84b6-1328339425c1@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> It is rather unfortunate that test_i18ngrep was deprecated without
> providing an alternative that offers the same debugging
> experience.
The primary thing test_i18ngrep did was to _SKIP_ certain test that
looked for an expected string in "C" locale from the output when the
tests were run under a tainted localization mode. The tests that
looked for strings in messages that are *not* to be localized used
"grep". Tests that (unfortunately) had to match human-readable
messages had to work around the tainted localization test to use
test_i18ngrep.
> When test_i18ngrep fails it prints a message with the
> pattern and text that failed to match so it is easy to see where the
> test failed. If grep fails there is no output and so unless the test
> is run with "-x" it can be hard to see which command caused the test
> to fail.
We could rename test_i18ngrep to test_grep (and make test_i18ngrep
into a thin wrapper with warnings).
test_grep -e must-exist file &&
test_grep ! -e must-not-exist file
^ permalink raw reply
* Re: [PATCH v3] bugreport: reject positional arguments
From: Junio C Hamano @ 2023-10-30 0:33 UTC (permalink / raw)
To: Phillip Wood
Cc: Eric Sunshine, emilyshaffer, git, Emily Shaffer, Sheik,
Dragan Simic
In-Reply-To: <xmqqv8apez0o.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> It is rather unfortunate that test_i18ngrep was deprecated without
>> providing an alternative that offers the same debugging
>> experience.
>
> The primary thing test_i18ngrep did was to _SKIP_ certain test that
> looked for an expected string in "C" locale from the output when the
> tests were run under a tainted localization mode. The tests that
> looked for strings in messages that are *not* to be localized used
> "grep". Tests that (unfortunately) had to match human-readable
> messages had to work around the tainted localization test to use
> test_i18ngrep.
>
>> When test_i18ngrep fails it prints a message with the
>> pattern and text that failed to match so it is easy to see where the
>> test failed. If grep fails there is no output and so unless the test
>> is run with "-x" it can be hard to see which command caused the test
>> to fail.
>
> We could rename test_i18ngrep to test_grep (and make test_i18ngrep
> into a thin wrapper with warnings).
>
> test_grep -e must-exist file &&
> test_grep ! -e must-not-exist file
... as the only remaining part in test_18ngrep has no hack to work
around the tainted localization tests, so "was deprecated without"
is a bit too strong. There is nothing we have lost yet.
t/test-lib-functions.sh | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git c/t/test-lib-functions.sh w/t/test-lib-functions.sh
index 2f8868caa1..c50bc18861 100644
--- c/t/test-lib-functions.sh
+++ w/t/test-lib-functions.sh
@@ -1208,14 +1208,16 @@ test_cmp_bin () {
cmp "$@"
}
-# Wrapper for grep which used to be used for
-# GIT_TEST_GETTEXT_POISON=false. Only here as a shim for other
-# in-flight changes. Should not be used and will be removed soon.
+# Deprecated - do not use this in new code
test_i18ngrep () {
+ test_grep "$@"
+}
+
+test_grep () {
eval "last_arg=\${$#}"
test -f "$last_arg" ||
- BUG "test_i18ngrep requires a file to read as the last parameter"
+ BUG "test_grep requires a file to read as the last parameter"
if test $# -lt 2 ||
{ test "x!" = "x$1" && test $# -lt 3 ; }
^ permalink raw reply related
* Re: [RFC PATCH v2 1/6] status: add noob format from status.noob config
From: Junio C Hamano @ 2023-10-30 1:32 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git
In-Reply-To: <20231026224615.675172-2-jacob@initialcommit.io>
Jacob Stopak <jacob@initialcommit.io> writes:
> diff --git a/table.c b/table.c
> new file mode 100644
> index 0000000000..15600e117f
> --- /dev/null
> +++ b/table.c
Yuck, do we need an entirely new file? What trait are the things
that are thrown into this file together supposed to share [*]? It
is not very clear to me what the focus of this file is.
Side note: for example, stuff in wt-status.c are to compute
per-path status of the working tree and in-index files.
> @@ -0,0 +1,117 @@
> +#define USE_THE_INDEX_VARIABLE
I personally do not mind, but I suspect many people hate to see this
compatibility set of macros used in a newly written source file.
> +static const char *color(int slot, struct wt_status *s)
> +{
> + const char *c = "";
> + if (want_color(s->use_color))
> + c = s->color_palette[slot];
> + if (slot == WT_STATUS_ONBRANCH && color_is_nil(c))
> + c = s->color_palette[WT_STATUS_HEADER];
> + return c;
> +}
Do we need to duplicate this from other files? If this is about
"git status", perhaps some parts of this patch, the truly new things
(rather than what was copied, like this one) can be added to
wt-status.c instead of adding a new file with unclear focus?
> +static void build_table_border(struct strbuf *buf, int cols)
> +{
> + strbuf_reset(buf);
> + strbuf_addchars(buf, '-', cols);
> +}
This seems to be horizontal border; do we need a separate vertical
border?
> +static void build_table_entry(struct strbuf *buf, char *entry, int cols)
> +{
> + strbuf_reset(buf);
> + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
> + strbuf_addstr(buf, entry);
> +
> + /* Bump right padding if entry length is odd */
> + if (!(strlen(entry) % 2))
> + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2 + 1);
> + else
> + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
> +}
The code assumes that one byte in string "entry" occupies one and
only one display columns, which is so 20th centry assumption that
does not care about i18n. Often what takes 3 bytes in a UTF-8
string occupies 2 display columns, for example. In addition, if you
plan to color entries in the table, some substring would end up to
be 0-width. Your pathname may be so long that 1/3 of a window
width may not be sufficient to show it in its entirety, you might
need to show it truncated in the middle.
utf8.c has support for measuring the display width of UTF-8 string,
which is used elsewhere in our code. You may want to study it if
you want to do a "tabular" output. The code in diff.c that shows
diffstat has many gems to help what this code wants to do, including
measuring display columns of a string, chomping a long string to fit
in a desired display columns, etc., by using helpers defined in
utf8.c
A potential excuse I can think of to have these outside wt-status.c
and in a separate new file is to have a generic "table" layout
machinery that is independent from "git status" or what each column
of the table is showing (in other words, they may not be pathnames),
and reusable by other subcommands that want to show things in the
"table" layout. But even as a candidate for such a generic table
mechanism, the above falls far short by hardcoding that it can only
show 3-col table whose columns are evenly distributed and nothing
else.
> +static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
> +{
> + printf(_("|"));
> + color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", buf1->buf);
> + printf(_("|"));
> + color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%s", buf2->buf);
> + printf(_("|"));
> + color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%s", buf3->buf);
> + printf(_("|\n"));
> +}
How does the code deal with unknown display width of translated
version of "|" emitted here? Are you assuming that no matter how
these are translated, they will always occupy one display column
each?
> +void print_noob_status(struct wt_status *s)
> +{
> + struct winsize w;
> + int cols;
> + struct strbuf table_border = STRBUF_INIT;
> + struct strbuf table_col_entry_1 = STRBUF_INIT;
> + struct strbuf table_col_entry_2 = STRBUF_INIT;
> + struct strbuf table_col_entry_3 = STRBUF_INIT;
> + struct string_list_item *item;
> +
> + /* Get terminal width */
> + ioctl(STDOUT_FILENO, TIOCGWINSZ, &w);
> + cols = w.ws_col;
Let's not reinvent an incomplete solution before studying and
finding out what we already have in our codebase. Immediately after
you got tempted to type TIOCGWINSZ, you can "git grep" the codebase
for that particular constant to see if we already use it, as that is
one very reasonable way to achieve what this piece of code wants to
do (i.e. find out what the display width would be). You'd find
pager.c:term_columns() and also learned that we want to prepare for
the case where the ioctl() is not available.
> + build_table_entry(&table_col_entry_1, "Untracked files", cols);
> + build_table_entry(&table_col_entry_2, "Unstaged changes", cols);
> + build_table_entry(&table_col_entry_3, "Staging area", cols);
Shouldn't these three strings be translatable?
What shoudl happen when these labels are wider than cols/3?
> diff --git a/table.h b/table.h
> new file mode 100644
> index 0000000000..c9e8c386de
> --- /dev/null
> +++ b/table.h
> @@ -0,0 +1,6 @@
> +#ifndef TABLE_H
> +#define TABLE_H
> +
> +void print_noob_status(struct wt_status *s);
> +
> +#endif /* TABLE_H */
I am guessing that your plan is to add other "distim_noob_add()" and
other "noob" variant of operations for various Git subcommands here,
but I really do not think you want to add table.[ch] that has logic
for such random set of Git subcommands copied and tweaked from all
over the place, as the only trait being shared among them will
become "they are written by Jacob Stopak", that is not a very useful
grouping of the functions. It is not even "this file collects all
the code that produce tabular output from Git"---"git status -s"
already gives tabular output, for example, without using any of the
"I only want to draw a table with three columns of equal width"
logic. Adding code that are necessary to add yet another output
mode for "git status" directly to where various output modes of "git
status" are implemented, i.e. wt-status.c, and do similar changes for
each command would make more sense, I would think.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] status: add noob format from status.noob config
From: Dragan Simic @ 2023-10-30 1:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Stopak, git
In-Reply-To: <xmqqjzr4gaie.fsf@gitster.g>
On 2023-10-30 02:32, Junio C Hamano wrote:
> Jacob Stopak <jacob@initialcommit.io> writes:
>> diff --git a/table.h b/table.h
>> new file mode 100644
>> index 0000000000..c9e8c386de
>> --- /dev/null
>> +++ b/table.h
>> @@ -0,0 +1,6 @@
>> +#ifndef TABLE_H
>> +#define TABLE_H
>> +
>> +void print_noob_status(struct wt_status *s);
>> +
>> +#endif /* TABLE_H */
>
> I am guessing that your plan is to add other "distim_noob_add()" and
> other "noob" variant of operations for various Git subcommands here,
> but I really do not think you want to add table.[ch] that has logic
> for such random set of Git subcommands copied and tweaked from all
> over the place, as the only trait being shared among them will
> become "they are written by Jacob Stopak", that is not a very useful
> grouping of the functions. It is not even "this file collects all
> the code that produce tabular output from Git"---"git status -s"
> already gives tabular output, for example, without using any of the
> "I only want to draw a table with three columns of equal width"
> logic. Adding code that are necessary to add yet another output
> mode for "git status" directly to where various output modes of "git
> status" are implemented, i.e. wt-status.c, and do similar changes for
> each command would make more sense, I would think.
Furthermore, "extended" should perhaps be used instead of "noob"
throughout, to reflect the planned naming of the configuration option
values.
^ permalink raw reply
* Re: [PATCH v3] bugreport: reject positional arguments
From: Junio C Hamano @ 2023-10-30 1:59 UTC (permalink / raw)
To: Phillip Wood
Cc: Eric Sunshine, emilyshaffer, git, Emily Shaffer, Sheik,
Dragan Simic
In-Reply-To: <xmqqpm0xeyp9.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>> It is rather unfortunate that test_i18ngrep was deprecated without
>>> providing an alternative that offers the same debugging
>>> experience.
>> ...
>> We could rename test_i18ngrep to test_grep (and make test_i18ngrep
>> into a thin wrapper with warnings).
>>
>> test_grep -e must-exist file &&
>> test_grep ! -e must-not-exist file
>
> ... as the only remaining part in test_18ngrep has no hack to work
> around the tainted localization tests, so "was deprecated without"
> is a bit too strong. There is nothing we have lost yet.
Having said all that, when re-reading the test_i18ngrep with a fresh
pair of eyes, I somehow doubt there was much upside in "debugging
experience" with test_i18ngrep in the first place, and I doubt if
retaining it with a new name test_grep has much value.
Given that test_i18ngrep (hence test_grep) requires you to have the
haystack in a file, between
test_i18ngrep must-exist file &&
test_i18ngrep ! must-not-exist file
and
grep must-exist file &&
! grep must-not-exist file
I do not see any difference in "debugging experience" when you run
the test with "-i [-v] -d". The two cases you care about are
(1) the test expects the string "must-exist" in the file "file" but
the string is not there.
(2) the test expects the string "must-not-exist" missing from the
file "file", but the string is there.
The latter can clearly be seen in output from "-i -v -d" (the "grep"
outputs a line with "must-not-exist" on it). The former will show
silence but since you are debugging with "-d", and your haystack is
in a file, after such a step fails, the test stops, and without
removing the "file" even if the test piece had test_when_finished
to remove it (i.e. running tests in debugging mode "-d" and
immediately stopping upon failure "-i" behaves this way exactly to
help you debugging), so you can go there to the TRASH_DIRECTORY
yourself and inspect "file" to see what is going on anyway.
So, I dunno. Surely with a long &&-chain of steps, where a grep
that expects lack of something is in the middle, it is hard to see
if the lack of hit is because an earlier step failed (and the
control did not reach "grep must-exist file") or because the
haystack lacked the "must-exist" needle, so from that point of view,
it may be nicer that "did not find an expected match" is explicitly
stated.
^ permalink raw reply
* [PATCH] sequencer: remove use of comment character
From: Tony Tung via GitGitGadget @ 2023-10-30 3:08 UTC (permalink / raw)
To: git; +Cc: Tony Tung, Tony Tung
From: Tony Tung <tonytung@merly.org>
Instead of using the hardcoded `# `, use the
user-defined comment_line_char. Adds a test
to prevent regressions.
Signed-off-by: Tony Tung <tonytung@merly.org>
---
sequencer: remove use of hardcoded comment char
Instead of using the hardcoded # , use the user-defined
comment_line_char. Adds a test to prevent regressions.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1603%2Fttung%2Fttung%2Fcommentchar-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1603/ttung/ttung/commentchar-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1603
sequencer.c | 5 +++--
t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed9..8c6666d5e43 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6082,8 +6082,9 @@ static int add_decorations_to_list(const struct commit *commit,
/* If the branch is checked out, then leave a comment instead. */
if ((path = branch_checked_out(decoration->name))) {
item->command = TODO_COMMENT;
- strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
- decoration->name, path);
+ strbuf_commented_addf(ctx->buf, comment_line_char,
+ "Ref %s checked out at '%s'\n",
+ decoration->name, path);
} else {
struct string_list_item *sti;
item->command = TODO_UPDATE_REF;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8ea2bf13026..076dca87871 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1839,6 +1839,45 @@ test_expect_success '--update-refs adds label and update-ref commands' '
)
'
+test_expect_success '--update-refs works with core.commentChar' '
+ git checkout -b update-refs-with-commentchar no-conflict-branch &&
+ test_config core.commentChar : &&
+ git branch -f base HEAD~4 &&
+ git branch -f first HEAD~3 &&
+ git branch -f second HEAD~3 &&
+ git branch -f third HEAD~1 &&
+ git commit --allow-empty --fixup=third &&
+ git branch -f is-not-reordered &&
+ git commit --allow-empty --fixup=HEAD~4 &&
+ git branch -f shared-tip &&
+ git checkout update-refs &&
+ (
+ write_script fake-editor.sh <<-\EOF &&
+ grep "^[^:]" "$1"
+ exit 1
+ EOF
+ test_set_editor "$(pwd)/fake-editor.sh" &&
+
+ cat >expect <<-EOF &&
+ pick $(git log -1 --format=%h J) J
+ fixup $(git log -1 --format=%h update-refs) fixup! J : empty
+ update-ref refs/heads/second
+ update-ref refs/heads/first
+ pick $(git log -1 --format=%h K) K
+ pick $(git log -1 --format=%h L) L
+ fixup $(git log -1 --format=%h is-not-reordered) fixup! L : empty
+ update-ref refs/heads/third
+ pick $(git log -1 --format=%h M) M
+ update-ref refs/heads/no-conflict-branch
+ update-ref refs/heads/is-not-reordered
+ update-ref refs/heads/update-refs-with-commentchar
+ EOF
+
+ test_must_fail git rebase -i --autosquash --update-refs primary shared-tip >todo &&
+ test_cmp expect todo
+ )
+'
+
test_expect_success '--update-refs adds commands with --rebase-merges' '
git checkout -b update-refs-with-merge no-conflict-branch &&
git branch -f base HEAD~4 &&
base-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] sequencer: remove use of comment character
From: Junio C Hamano @ 2023-10-30 4:00 UTC (permalink / raw)
To: Tony Tung via GitGitGadget; +Cc: git, Tony Tung
In-Reply-To: <pull.1603.git.1698635292629.gitgitgadget@gmail.com>
"Tony Tung via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Tony Tung <tonytung@merly.org>
>
> Instead of using the hardcoded `# `, use the
> user-defined comment_line_char. Adds a test
> to prevent regressions.
Good spotting.
Two observations.
(1) There are a few more places that need similar treatment in the
same file; you may want to fix them all while at it.
(2) The second argument to strbuf_commented_addf() is always the
comment_line_char global variable, not just inside this file
but all callers across the codebase. We probably should drop
it and have the strbuf_commented_addf() helper itself refer to
the global. That way, if we ever want to change the global
variable reference to something else (e.g. function call), we
only have to touch a single place.
The latter is meant as #leftoverbits and will be a lot wider
clean-up that we may want to do long after this patch hits out
codebase. The "other places" I spotted for the former are the
following, but needs to be taken with a huge grain of salt, as it
has not even been compile tested.
Thanks.
sequencer.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git c/sequencer.c w/sequencer.c
index d584cac8ed..33208b1660 100644
--- c/sequencer.c
+++ w/sequencer.c
@@ -1893,8 +1893,8 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
size_t orig_msg_len;
int i = 1;
- strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
- strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
+ strbuf_addf(&buf1, comment_line_char, "%s\n", _(first_commit_msg_str));
+ strbuf_addf(&buf2, comment_line_char, "%s\n", _(skip_first_commit_msg_str));
s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
while (s) {
const char *next;
@@ -2269,8 +2269,8 @@ static int do_pick_commit(struct repository *r,
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
- strbuf_addstr(&msgbuf,
- "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ strbuf_commented_addf(&msgbuf, comment_line_char, "%s",
+ "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
/*
* We don't touch pre-existing repeated reverts, because
^ permalink raw reply related
* [PATCH 0/2] Avoid passing global comment_line_char repeatedly
From: Junio C Hamano @ 2023-10-30 5:10 UTC (permalink / raw)
To: git
Two strbuf functions used to produce commented lines take the
comment_line_char as their parameter, but in practice, all callers
feed the global variable comment_line_char from environment.[ch].
Dropping the parameter from the callchain will make the interface
less flexible, and less error prone. If we choose to change the
implementation of the customizable comment line character (e.g., we
may want to stop referencing the global variable and instead use a
getter function), we will have fewer places we need to modify.
Junio C Hamano (2):
strbuf_commented_addf(): drop the comment_line_char parameter
strbuf_add_commented_lines(): drop the comment_line_char parameter
add-patch.c | 8 ++++----
builtin/branch.c | 2 +-
builtin/merge.c | 8 ++++----
builtin/notes.c | 9 ++++-----
builtin/stripspace.c | 2 +-
builtin/tag.c | 4 ++--
fmt-merge-msg.c | 9 +++------
rebase-interactive.c | 8 ++++----
sequencer.c | 14 ++++++--------
strbuf.c | 9 +++++----
strbuf.h | 7 +++----
wt-status.c | 6 +++---
12 files changed, 40 insertions(+), 46 deletions(-)
--
2.42.0-526-g3130c155df
^ permalink raw reply
* [PATCH 1/2] strbuf_commented_addf(): drop the comment_line_char parameter
From: Junio C Hamano @ 2023-10-30 5:10 UTC (permalink / raw)
To: git
In-Reply-To: <20231030051034.2295242-1-gitster@pobox.com>
All the callers of this function supply the global variable
comment_line_char as an argument to its second parameter. Remove
the parameter to allow us in the future to change the reference to
the global variable with something else, like a function call.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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 | 3 ++-
strbuf.h | 4 ++--
wt-status.c | 2 +-
9 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index bfe19876cd..471a0037be 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1106,11 +1106,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,
_("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,
_("---\n"
"To remove '%c' lines, make them ' ' lines "
"(context).\n"
@@ -1119,13 +1119,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, "%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,
_("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 2ec190b14a..b2f171e10b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -668,7 +668,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,
_("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 d748d46e13..8f0e8be7c3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -857,15 +857,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, "\n");
}
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg,
_(merge_editor_comment));
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg,
_(scissors_editor_comment));
else
- strbuf_commented_addf(&msg, comment_line_char,
+ strbuf_commented_addf(&msg,
_(no_scissors_editor_comment), comment_line_char);
}
if (signoff)
diff --git a/builtin/tag.c b/builtin/tag.c
index 3918eacbb5..a85a0d8def 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -314,10 +314,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,
_(tag_template), tag, comment_line_char);
else
- strbuf_commented_addf(&buf, comment_line_char,
+ strbuf_commented_addf(&buf,
_(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 d9718409b3..3f33da7f03 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,
Q_("Rebase %s onto %s (%d command)",
"Rebase %s onto %s (%d commands)",
command_count),
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..5d348a3f12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -675,11 +675,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, "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,
"\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 7827178d8e..15550b2619 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,4 +1,5 @@
#include "git-compat-util.h"
+#include "environment.h"
#include "gettext.h"
#include "hex-ll.h"
#include "strbuf.h"
@@ -372,7 +373,7 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
add_lines(out, prefix1, prefix2, buf, size);
}
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
+void strbuf_commented_addf(struct strbuf *sb,
const char *fmt, ...)
{
va_list params;
diff --git a/strbuf.h b/strbuf.h
index e959caca87..981617dc77 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -378,8 +378,8 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
* Add a formatted string prepended by a comment character and a
* blank to the buffer.
*/
-__attribute__((format (printf, 3, 4)))
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char, const char *fmt, ...);
+__attribute__((format (printf, 2, 3)))
+void strbuf_commented_addf(struct strbuf *sb, 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 9f45bf6949..54b2775730 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1102,7 +1102,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, "%s", cut_line);
strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
}
--
2.42.0-526-g3130c155df
^ permalink raw reply related
* [PATCH 2/2] strbuf_add_commented_lines(): drop the comment_line_char parameter
From: Junio C Hamano @ 2023-10-30 5:10 UTC (permalink / raw)
To: git
In-Reply-To: <20231030051034.2295242-1-gitster@pobox.com>
All the callers of this function supply the global variable
comment_line_char as an argument to its last parameter. Remove the
parameter to allow us in the future to change the reference to the
global variable with something else, like a function call.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/notes.c | 9 ++++-----
builtin/stripspace.c | 2 +-
fmt-merge-msg.c | 9 +++------
rebase-interactive.c | 6 +++---
sequencer.c | 10 ++++------
strbuf.c | 6 +++---
strbuf.h | 3 +--
wt-status.c | 4 ++--
8 files changed, 21 insertions(+), 28 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 9f38863dd5..355ecce07a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -181,7 +181,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);
write_or_die(fd, cbuf.buf, cbuf.len);
strbuf_release(&cbuf);
@@ -209,10 +209,9 @@ 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, _(note_template), strlen(_(note_template)),
- comment_line_char);
- strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_char);
+ strbuf_add_commented_lines(&buf, "\n", strlen("\n"));
+ strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)));
+ strbuf_add_commented_lines(&buf, "\n", strlen("\n"));
write_or_die(fd, buf.buf, buf.len);
write_commented_object(fd, object);
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..11e475760c 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);
free(msg);
}
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 66e47449a0..adc85d2a72 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -509,8 +509,7 @@ static void fmt_tag_signature(struct strbuf *tagbuf,
strbuf_complete_line(tagbuf);
if (sig->len) {
strbuf_addch(tagbuf, '\n');
- strbuf_add_commented_lines(tagbuf, sig->buf, sig->len,
- comment_line_char);
+ strbuf_add_commented_lines(tagbuf, sig->buf, sig->len);
}
}
@@ -556,8 +555,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
strbuf_addch(&tagline, '\n');
strbuf_add_commented_lines(&tagline,
origins.items[first_tag].string,
- strlen(origins.items[first_tag].string),
- comment_line_char);
+ strlen(origins.items[first_tag].string));
strbuf_insert(&tagbuf, 0, tagline.buf,
tagline.len);
strbuf_release(&tagline);
@@ -565,8 +563,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
strbuf_addch(&tagbuf, '\n');
strbuf_add_commented_lines(&tagbuf,
origins.items[i].string,
- strlen(origins.items[i].string),
- comment_line_char);
+ strlen(origins.items[i].string));
fmt_tag_signature(&tagbuf, &sig, buf, len);
}
strbuf_release(&payload);
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 3f33da7f03..1138bd37ba 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));
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));
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));
}
int edit_todo_list(struct repository *r, struct todo_list *todo_list,
diff --git a/sequencer.c b/sequencer.c
index 5d348a3f12..29c8b5e32b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1859,7 +1859,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);
}
/* Does the current fixup chain contain a squash command? */
@@ -1958,7 +1958,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);
/* buf->buf may be reallocated so store an offset into the buffer */
fixup_off = buf->len;
strbuf_addstr(buf, body + commented_len);
@@ -2048,8 +2048,7 @@ static int update_squash_messages(struct repository *r,
_(first_commit_msg_str));
strbuf_addstr(&buf, "\n\n");
if (is_fixup_flag(command, flag))
- strbuf_add_commented_lines(&buf, body, strlen(body),
- comment_line_char);
+ strbuf_add_commented_lines(&buf, body, strlen(body));
else
strbuf_addstr(&buf, body);
@@ -2068,8 +2067,7 @@ static int update_squash_messages(struct repository *r,
strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),
++opts->current_fixup_count + 1);
strbuf_addstr(&buf, "\n\n");
- strbuf_add_commented_lines(&buf, body, strlen(body),
- comment_line_char);
+ strbuf_add_commented_lines(&buf, body, strlen(body));
} else
return error(_("unknown command: %d"), command);
repo_unuse_commit_buffer(r, commit, message);
diff --git a/strbuf.c b/strbuf.c
index 15550b2619..2088f7800a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -360,8 +360,8 @@ static void add_lines(struct strbuf *out,
strbuf_complete_line(out);
}
-void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
- size_t size, char comment_line_char)
+void strbuf_add_commented_lines(struct strbuf *out,
+ const char *buf, size_t size)
{
static char prefix1[3];
static char prefix2[2];
@@ -384,7 +384,7 @@ void strbuf_commented_addf(struct strbuf *sb,
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);
if (incomplete_line)
sb->buf[--sb->len] = '\0';
diff --git a/strbuf.h b/strbuf.h
index 981617dc77..4547efa62e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -287,8 +287,7 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
* by a comment character and a blank.
*/
void strbuf_add_commented_lines(struct strbuf *out,
- const char *buf, size_t size,
- char comment_line_char);
+ const char *buf, size_t size);
/**
diff --git a/wt-status.c b/wt-status.c
index 54b2775730..b390c77334 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1027,7 +1027,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);
free(summary_content);
}
@@ -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, "%s", cut_line);
- strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
+ strbuf_add_commented_lines(buf, explanation, strlen(explanation));
}
void wt_status_add_cut_line(FILE *fp)
--
2.42.0-526-g3130c155df
^ permalink raw reply related
* Re: [PATCH 0/2] Avoid passing global comment_line_char repeatedly
From: Dragan Simic @ 2023-10-30 5:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20231030051034.2295242-1-gitster@pobox.com>
On 2023-10-30 06:10, Junio C Hamano wrote:
> Two strbuf functions used to produce commented lines take the
> comment_line_char as their parameter, but in practice, all callers
> feed the global variable comment_line_char from environment.[ch].
>
> Dropping the parameter from the callchain will make the interface
> less flexible, and less error prone. If we choose to change the
> implementation of the customizable comment line character (e.g., we
> may want to stop referencing the global variable and instead use a
> getter function), we will have fewer places we need to modify.
>
> Junio C Hamano (2):
> strbuf_commented_addf(): drop the comment_line_char parameter
> strbuf_add_commented_lines(): drop the comment_line_char parameter
This series looks good to me. It removes unnecessary complexity that
provided pretty much no value.
> add-patch.c | 8 ++++----
> builtin/branch.c | 2 +-
> builtin/merge.c | 8 ++++----
> builtin/notes.c | 9 ++++-----
> builtin/stripspace.c | 2 +-
> builtin/tag.c | 4 ++--
> fmt-merge-msg.c | 9 +++------
> rebase-interactive.c | 8 ++++----
> sequencer.c | 14 ++++++--------
> strbuf.c | 9 +++++----
> strbuf.h | 7 +++----
> wt-status.c | 6 +++---
> 12 files changed, 40 insertions(+), 46 deletions(-)
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] status: add noob format from status.noob config
From: Jacob Stopak @ 2023-10-30 6:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzr4gaie.fsf@gitster.g>
On Mon, Oct 30, 2023 at 10:32:57AM +0900, Junio C Hamano wrote:
> Jacob Stopak <jacob@initialcommit.io> writes:
>
> > diff --git a/table.c b/table.c
> > new file mode 100644
> > index 0000000000..15600e117f
> > --- /dev/null
> > +++ b/table.c
>
> Yuck, do we need an entirely new file? What trait are the things
> that are thrown into this file together supposed to share [*]? It
> is not very clear to me what the focus of this file is.
>
> Side note: for example, stuff in wt-status.c are to compute
> per-path status of the working tree and in-index files.
I created a new file because:
* It will be used by more than just the status command (as patches 3/6
and 5/6 show it applied to the "add" and "restore" commands
respectively). And it will be applied to more commands as well.
* For these RFC versions I mainly wanted to get some kind of minimal
POC working, so I could get feedback and build on
* It was easier to work on it in it's own file.
That being said, I was probably careless with what I tossed in there.
I intended it to specifically be things related to populating, coloring,
and printing the new table format.
But after reading all of your comments here, I see that I most likely
made the error (which I've made before) of being too specific. If I
generalize this file to be a generic table format that can be used for
anything, then the command-specific details can likely be extracted into
their respective existing files.
Another thing I totally didn't consider until I read your mail is the
possibility of refactoring the existing "git column" functionality to
provide a configurable table output format. At this point I'm not sure
if that's feasible or makes sense, but it's at least worth considering.
> > @@ -0,0 +1,117 @@
> > +#define USE_THE_INDEX_VARIABLE
>
> I personally do not mind, but I suspect many people hate to see this
> compatibility set of macros used in a newly written source file.
Noted, I'll try to update this.
> > +static const char *color(int slot, struct wt_status *s)
> > +{
> > + const char *c = "";
> > + if (want_color(s->use_color))
> > + c = s->color_palette[slot];
> > + if (slot == WT_STATUS_ONBRANCH && color_is_nil(c))
> > + c = s->color_palette[WT_STATUS_HEADER];
> > + return c;
> > +}
>
> Do we need to duplicate this from other files? If this is about
> "git status", perhaps some parts of this patch, the truly new things
> (rather than what was copied, like this one) can be added to
> wt-status.c instead of adding a new file with unclear focus?
I just re-used the status coloring out of laziness, since I was thinking
about this in a narrow way that it's basically replicating the output
of Git status, so I figured I'd use the same colors.
Now that I'm thinking more generally I see why this needs to change,
to make table column output colors flexible.
> > +static void build_table_border(struct strbuf *buf, int cols)
> > +{
> > + strbuf_reset(buf);
> > + strbuf_addchars(buf, '-', cols);
> > +}
>
> This seems to be horizontal border; do we need a separate vertical
> border?
Yes it is a horizontal border, the vertical border pipe characters are
included as a part of each table line being drawn, so I didn't need a
standalone vertical border in order to "draw" the tables. This one could
be renamed to reflect it's horizontal-ness, for clarity.
> > +static void build_table_entry(struct strbuf *buf, char *entry, int cols)
> > +{
> > + strbuf_reset(buf);
> > + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
> > + strbuf_addstr(buf, entry);
> > +
> > + /* Bump right padding if entry length is odd */
> > + if (!(strlen(entry) % 2))
> > + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2 + 1);
> > + else
> > + strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
> > +}
>
> The code assumes that one byte in string "entry" occupies one and
> only one display columns, which is so 20th centry assumption that
> does not care about i18n. Often what takes 3 bytes in a UTF-8
> string occupies 2 display columns, for example. In addition, if you
> plan to color entries in the table, some substring would end up to
> be 0-width.
Thanks for this info. I'll look into these.
> Your pathname may be so long that 1/3 of a window
> width may not be sufficient to show it in its entirety, you might
> need to show it truncated in the middle.
This is exactly what patch 2/6 does (more details on that below). I
should have squashed that into 1/6. I'll do that in the next series.
> utf8.c has support for measuring the display width of UTF-8 string,
> which is used elsewhere in our code. You may want to study it if
> you want to do a "tabular" output. The code in diff.c that shows
> diffstat has many gems to help what this code wants to do, including
> measuring display columns of a string, chomping a long string to fit
> in a desired display columns, etc., by using helpers defined in
> utf8.c
Well that sounds like exactly what I need. I'll take a look, thanks!
> A potential excuse I can think of to have these outside wt-status.c
> and in a separate new file is to have a generic "table" layout
> machinery that is independent from "git status" or what each column
> of the table is showing (in other words, they may not be pathnames),
> and reusable by other subcommands that want to show things in the
> "table" layout.
Yes that is my excuse (altho it may not be a valid one) - this patch
series adds the table output format for the "git add" (see patch 3/6) and
"git restore" (see patch 5/6) commands. Dragan will be working on
implementing verbose config options for several commands, that I will then
use as stepping stones to try and add this table format as the "extended"
(not "noob") verbosity option.
> But even as a candidate for such a generic table
> mechanism, the above falls far short by hardcoding that it can only
> show 3-col table whose columns are evenly distributed and nothing
> else.
Yeah - I was planning to update the table format so that the number of
columns is flexible. For example, I can think of commands like "git
stash" that would likely require 4 columns to display the required info,
so parameterizing the number of columns is on my agenda.
But you're right maybe there are many other useful things that a generic
table format would include. Things like alignment, spacing, etc. I didn't
think about how this might be used outside of my specific use case - I
tend to get tunnel vision on what I'm working on. I'll try to think more
broadly about how a format like this might be applicable, and at least
try to implement it in a way that it can be re-used for other things if
and when the need arises.
> > +static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
> > +{
> > + printf(_("|"));
> > + color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", buf1->buf);
> > + printf(_("|"));
> > + color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%s", buf2->buf);
> > + printf(_("|"));
> > + color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%s", buf3->buf);
> > + printf(_("|\n"));
> > +}
>
> How does the code deal with unknown display width of translated
> version of "|" emitted here? Are you assuming that no matter how
> these are translated, they will always occupy one display column
> each?
I'm not familiar with how the translation process works yet, so _if_ the
version of the "|" that I'm emitting here is a canditate for translation,
it was not even my intention to do so, as I'm just using the vertical
pipe as a column separator and table border. I think what I need is to
make sure these are _not_ translatable, so that we can guarantee the
single character display width?
> > +void print_noob_status(struct wt_status *s)
> > +{
> > + struct winsize w;
> > + int cols;
> > + struct strbuf table_border = STRBUF_INIT;
> > + struct strbuf table_col_entry_1 = STRBUF_INIT;
> > + struct strbuf table_col_entry_2 = STRBUF_INIT;
> > + struct strbuf table_col_entry_3 = STRBUF_INIT;
> > + struct string_list_item *item;
> > +
> > + /* Get terminal width */
> > + ioctl(STDOUT_FILENO, TIOCGWINSZ, &w);
> > + cols = w.ws_col;
>
> Let's not reinvent an incomplete solution before studying and
> finding out what we already have in our codebase. Immediately after
> you got tempted to type TIOCGWINSZ, you can "git grep" the codebase
> for that particular constant to see if we already use it, as that is
> one very reasonable way to achieve what this piece of code wants to
> do (i.e. find out what the display width would be). You'd find
> pager.c:term_columns() and also learned that we want to prepare for
> the case where the ioctl() is not available.
Ah, ok let me look into that. I have utilized git grepping to understand
how various things are done in the codebase, but for some (no good) reason
I didn't think to try that here.
> > + build_table_entry(&table_col_entry_1, "Untracked files", cols);
> > + build_table_entry(&table_col_entry_2, "Unstaged changes", cols);
> > + build_table_entry(&table_col_entry_3, "Staging area", cols);
>
> Shouldn't these three strings be translatable?
Yes. I'll look into how translatable strings are done in the codebase.
> What should happen when these labels are wider than cols/3?
That's what patch 2/6 does. It splices out the center of strings that
are too long to fit in a column, keeping an equal number of characters
at the start and end of the string, and inserts a "..." in the middle.
Altho this makes sense for paths, this could be a bit confusing for
table headers, since it may be hard (or impossible) to read what the
column's purpose is, so it might be wiser to come up with even shorter
versions of the table headers to use when the console width is below a
certain size that makes the original strings unreadable.
> > diff --git a/table.h b/table.h
> > new file mode 100644
> > index 0000000000..c9e8c386de
> > --- /dev/null
> > +++ b/table.h
> > @@ -0,0 +1,6 @@
> > +#ifndef TABLE_H
> > +#define TABLE_H
> > +
> > +void print_noob_status(struct wt_status *s);
> > +
> > +#endif /* TABLE_H */
>
> I am guessing that your plan is to add other "distim_noob_add()" and
> other "noob" variant of operations for various Git subcommands here,
> but I really do not think you want to add table.[ch] that has logic
> for such random set of Git subcommands copied and tweaked from all
> over the place, as the only trait being shared among them will
> become "they are written by Jacob Stopak", that is not a very useful
> grouping of the functions. It is not even "this file collects all
> the code that produce tabular output from Git"---"git status -s"
> already gives tabular output, for example, without using any of the
> "I only want to draw a table with three columns of equal width"
> logic. Adding code that are necessary to add yet another output
> mode for "git status" directly to where various output modes of "git
> status" are implemented, i.e. wt-status.c, and do similar changes for
> each command would make more sense, I would think.
If you look at patches 3/6 and 5/6 I think you'll better understand my
"plan", not that I thought this would by any kind of final version, but
more of a starting point to get feedback and build on (hence the RFC).
I think the reason I clung so hard to "status" is that it contains a
lot of the info that I planned to display across various commands, so
much so that I baked it into the new table file itself. I see now that
needs to be separated out, as that data to be displayed can be calculated
in each command itself, and passed into the table functions to build and
print the thing.
But based on your input in this mail I'm going to:
* Look into the existing "column" formatter to see if it can be
repurposed for this table stuff.
* If not, generalize the table stuff so that it can be used for anything
and compartmentalize the command-specific stuff where it belongs.
> Thanks.
Thank you!
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Torsten Bögershausen @ 2023-10-30 6:36 UTC (permalink / raw)
To: Alexander Mills; +Cc: git
In-Reply-To: <CA+KyZp5mwGJ6YOvjKtfnDMDb9ci3vSq5KNUep6-8EfkHNaxREg@mail.gmail.com>
On Sun, Oct 29, 2023 at 06:15:35PM -0500, Alexander Mills wrote:
> When a feature branch is pushed, we get a link in the console to go to
> remote, however when I push directly to main/master, no such link, eg:
>
> ```
> alex.mills@alex node-be % git push
> Enumerating objects: 20, done.
> Counting objects: 100% (20/20), done.
> Delta compression using up to 12 threads
> Compressing objects: 100% (10/10), done.
> Writing objects: 100% (11/11), 1.56 KiB | 799.00 KiB/s, done.
> Total 11 (delta 7), reused 0 (delta 0), pack-reused 0
> remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
> remote: Bypassed rule violations for refs/heads/main:
> remote:
> remote: - Changes must be made through a pull request.
> remote:
> To github.com:elx-onlinx/beautychat-chatcards.git
> ffe1e05..bb7b0ef main -> main
> ```
>
> Having the link in the console saves me tremendous time and is
> extremely effective/efficient. Can we get links in the console plz?
>
If we look very carfully at the log, we see that all the messages prefixed
with "remote:" come from the remote (git server).
In your case github. Other "Git repo servers" like gitlab or bitbucket have
the same feature.
Git itself, running on your local computer, does not produce this links.
If you really want to push directly to the main branch and want to see the
a link, then you better talk to the gihub folks.
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Junio C Hamano @ 2023-10-30 6:55 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Alexander Mills, git
In-Reply-To: <20231030063633.GA7451@tb-raspi4>
Torsten Bögershausen <tboegi@web.de> writes:
> On Sun, Oct 29, 2023 at 06:15:35PM -0500, Alexander Mills wrote:
>> When a feature branch is pushed, we get a link in the console to go to
>> remote, however when I push directly to main/master, no such link, eg:
>>
>> ```
>> alex.mills@alex node-be % git push
>> Enumerating objects: 20, done.
>> Counting objects: 100% (20/20), done.
>> Delta compression using up to 12 threads
>> Compressing objects: 100% (10/10), done.
>> Writing objects: 100% (11/11), 1.56 KiB | 799.00 KiB/s, done.
>> Total 11 (delta 7), reused 0 (delta 0), pack-reused 0
>> remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
>> remote: Bypassed rule violations for refs/heads/main:
>> remote:
>> remote: - Changes must be made through a pull request.
>> remote:
>> To github.com:elx-onlinx/beautychat-chatcards.git
>> ffe1e05..bb7b0ef main -> main
>> ```
>>
>> Having the link in the console saves me tremendous time and is
>> extremely effective/efficient. Can we get links in the console plz?
>>
>
> If we look very carfully at the log, we see that all the messages prefixed
> with "remote:" come from the remote (git server).
> In your case github. Other "Git repo servers" like gitlab or bitbucket have
> the same feature.
>
> Git itself, running on your local computer, does not produce this links.
> If you really want to push directly to the main branch and want to see the
> a link, then you better talk to the gihub folks.
Thanks for a good suggestion.
Another piece of advice we may want to give is that a good bugreport
should illustrate both good and bad case. With only the above one,
I couldn't guess that the complaint was about lack of information
that comes over the wire from remote, *exactly* because the report
failed to show the good case that shows the information and made the
readers guess what thing that does not exist in the output is being
complained about. A good bugreport should not force readers guess.
If the report had both good and bad cases, it would have helped
readers help the reporter much easier.
Thanks.
^ permalink raw reply
* Re: [PATCH] credential/wincred: store oauth_refresh_token
From: M Hickford @ 2023-10-30 8:00 UTC (permalink / raw)
To: M Hickford
Cc: git-for-windows, Taylor Blau, Git Mailing List,
Matthew John Cheetham, Jakub Bereżański, patthoyts
In-Reply-To: <CAGJzqskekExeug45Zh-uQ1dpHpKP8HS3obWQyz8B_DzSZiuAwA@mail.gmail.com>
Hi. Is anyone with Windows development experience able to take a look
at this patch to credential-wincred?
https://lore.kernel.org/git/pull.1534.git.1684567247339.gitgitgadget@gmail.com/
https://github.com/gitgitgadget/git/pull/1534
On Thu, 10 Aug 2023 at 20:58, M Hickford <mirth.hickford@gmail.com> wrote:
>
> Hi. A small patch to git-credential-wincred needs review on the Git
> mailing list https://lore.kernel.org/git/pull.1534.git.1684567247339.gitgitgadget@gmail.com/
> . Does anyone here have the expertise to review?
>
> On Sat, 20 May 2023 at 08:20, M Hickford via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > a5c7656 (credential: new attribute oauth_refresh_token) introduced
> > a new confidential credential attribute for helpers to store.
> >
> > We encode the new attribute in the CredentialBlob, separated by
> > newline:
> >
> > hunter2
> > oauth_refresh_token=xyzzy
> >
> > This is extensible and backwards compatible. The credential protocol
> > already assumes that attribute values do not contain newlines.
> >
> > Alternatives considered: store oauth_refresh_token in a wincred
> > attribute. This would be insecure because wincred assumes attribute
> > values to be non-confidential.
> >
> > Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> > ---
> > credential/wincred: store oauth_refresh_token
> >
> > I'm not confident in C and found string manipulation difficult, so
> > please review carefully.
> >
> > I tested on Linux, cross compiling with Zig make CC="zig cc -target
> > x86_64-windows-gnu" and tested make GIT_TEST_CREDENTIAL_HELPER=wincred
> > t0303-credential-external.sh (with a shell script git-credential-wincred
> > that wraps wine64 git-credential-wincred.exe "$@")
> >
> > See also similar patch for credential-libsecret "[PATCH v4]
> > credential/libsecret: store new attributes"
> > https://lore.kernel.org/git/pull.1469.v4.git.git.1684306540947.gitgitgadget@gmail.com/T/#u
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1534%2Fhickford%2Fwincred-refresh-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1534/hickford/wincred-refresh-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1534
> >
> > .../wincred/git-credential-wincred.c | 40 ++++++++++++++++---
> > t/t0303-credential-external.sh | 1 +
> > 2 files changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> > index 96f10613aee..dc34283b85e 100644
> > --- a/contrib/credential/wincred/git-credential-wincred.c
> > +++ b/contrib/credential/wincred/git-credential-wincred.c
> > @@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
> > }
> >
> > static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
> > - *password_expiry_utc;
> > + *password_expiry_utc, *oauth_refresh_token;
> >
> > static void write_item(const char *what, LPCWSTR wbuf, int wlen)
> > {
> > @@ -128,6 +128,11 @@ static void get_credential(void)
> > DWORD num_creds;
> > int i;
> > CREDENTIAL_ATTRIBUTEW *attr;
> > + WCHAR *secret;
> > + WCHAR *line;
> > + WCHAR *remaining_lines;
> > + WCHAR *part;
> > + WCHAR *remaining_parts;
> >
> > if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
> > return;
> > @@ -137,9 +142,17 @@ static void get_credential(void)
> > if (match_cred(creds[i])) {
> > write_item("username", creds[i]->UserName,
> > creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
> > - write_item("password",
> > - (LPCWSTR)creds[i]->CredentialBlob,
> > - creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + secret = xmalloc(creds[i]->CredentialBlobSize);
> > + wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + line = wcstok_s(secret, L"\r\n", &remaining_lines);
> > + write_item("password", line, wcslen(line));
> > + while(line != NULL) {
> > + part = wcstok_s(line, L"=", &remaining_parts);
> > + if (!wcscmp(part, L"oauth_refresh_token")) {
> > + write_item("oauth_refresh_token", remaining_parts, wcslen(remaining_parts));
> > + }
> > + line = wcstok_s(NULL, L"\r\n", &remaining_lines);
> > + }
> > for (int j = 0; j < creds[i]->AttributeCount; j++) {
> > attr = creds[i]->Attributes + j;
> > if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
> > @@ -148,6 +161,7 @@ static void get_credential(void)
> > break;
> > }
> > }
> > + free(secret);
> > break;
> > }
> >
> > @@ -158,16 +172,26 @@ static void store_credential(void)
> > {
> > CREDENTIALW cred;
> > CREDENTIAL_ATTRIBUTEW expiry_attr;
> > + WCHAR *secret;
> > + int wlen;
> >
> > if (!wusername || !password)
> > return;
> >
> > + if (oauth_refresh_token) {
> > + wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> > + secret = xmalloc(sizeof(WCHAR) * wlen);
> > + _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> > + } else {
> > + secret = _wcsdup(password);
> > + }
> > +
> > cred.Flags = 0;
> > cred.Type = CRED_TYPE_GENERIC;
> > cred.TargetName = target;
> > cred.Comment = L"saved by git-credential-wincred";
> > - cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
> > - cred.CredentialBlob = (LPVOID)password;
> > + cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
> > + cred.CredentialBlob = (LPVOID)_wcsdup(secret);
> > cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> > cred.AttributeCount = 0;
> > cred.Attributes = NULL;
> > @@ -182,6 +206,8 @@ static void store_credential(void)
> > cred.TargetAlias = NULL;
> > cred.UserName = wusername;
> >
> > + free(secret);
> > +
> > if (!CredWriteW(&cred, 0))
> > die("CredWrite failed");
> > }
> > @@ -253,6 +279,8 @@ static void read_credential(void)
> > password = utf8_to_utf16_dup(v);
> > else if (!strcmp(buf, "password_expiry_utc"))
> > password_expiry_utc = utf8_to_utf16_dup(v);
> > + else if (!strcmp(buf, "oauth_refresh_token"))
> > + oauth_refresh_token = utf8_to_utf16_dup(v);
> > /*
> > * Ignore other lines; we don't know what they mean, but
> > * this future-proofs us when later versions of git do
> > diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> > index f028fd14182..51886b8e259 100755
> > --- a/t/t0303-credential-external.sh
> > +++ b/t/t0303-credential-external.sh
> > @@ -45,6 +45,7 @@ test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
> > helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> >
> > helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> > +helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
> >
> > if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
> > say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
> >
> > base-commit: 9e49351c3060e1fa6e0d2de64505b7becf157f28
> > --
> > gitgitgadget
^ permalink raw reply
* RE: [EXT] Re: ls-remote bug
From: Lior Zeltzer @ 2023-10-30 8:17 UTC (permalink / raw)
To: René Scharfe, Bagas Sanjaya, Git Mailing List
Cc: Andrzej Hunt, Ævar Arnfjörð Bjarmason,
Junio C Hamano
In-Reply-To: <cc829a27-2580-4e07-a6ff-2c4992131420@web.de>
But why when cancelling stderr redirect to stdout (2>&1) all works well ?
-----Original Message-----
From: René Scharfe <l.s.r@web.de>
Sent: Sunday, October 29, 2023 8:57 PM
To: Lior Zeltzer <liorz@marvell.com>; Bagas Sanjaya <bagasdotme@gmail.com>; Git Mailing List <git@vger.kernel.org>
Cc: Andrzej Hunt <ajrhunt@google.com>; Ævar Arnfjörð Bjarmason <avarab@gmail.com>; Junio C Hamano <gitster@pobox.com>
Subject: Re: [EXT] Re: ls-remote bug
Am 27.10.23 um 13:16 schrieb Lior Zeltzer:
> The reproduction ,as I wrote in the code, should be done with few
> threads in parallel Each working on a list of ~10 repos with each repo
> containing a lot of refs/tags (~1000) All this should be against
> gerrit (my gerrit is 3.8.0)
>
> Also read the notes below regarding the code that was moved between
> 2.31.8 and 2.32.0 in ls-remote.c file I can elaborate more, in a zoom meeting.
>
> 10x
> Lior.
>
>
> -----Original Message-----
> From: Bagas Sanjaya <bagasdotme@gmail.com>
> Sent: Friday, October 27, 2023 4:08 AM
> To: Lior Zeltzer <liorz@marvell.com>; Git Mailing List
> <git@vger.kernel.org>
> Cc: Andrzej Hunt <ajrhunt@google.com>; Ævar Arnfjörð Bjarmason
> <avarab@gmail.com>; Junio C Hamano <gitster@pobox.com>
> Subject: [EXT] Re: ls-remote bug
>
> External Email
>
> ----------------------------------------------------------------------
> On Tue, Oct 24, 2023 at 10:55:24AM +0000, Lior Zeltzer wrote:
>>
>>> uname -a
>> Linux dc3lp-veld0045 3.10.0-1160.21.1.el7.x86_64 #1 SMP Tue Mar 16
>> 18:28:22 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
>>
>> Gerrit version :
>> 3.8.0
>>
>> Bug description :
>> When running ls-remote : sometime data gets cut in the middle
>>
>> Reproducing :
>> You need a few files with a few repo names (I used 4 files with 10
>> repos each) Call then l1..l4 And the code below just cd into each of
>> them does ls-remote twice and compares the data Doing it in parallel
>> on all lists.
>> Data received in both ls-remotes should be the same , if not, it
>> prints *** Repos should contain a lot of tags and refs
>
> What repo did you find this regression? Did you mean linux.git (Linux kernel)?
>
>>
>> Note :
>> 1. without stderr redirection (2>&1) all works well 2. On local
>> repos (not through gerrit) all works well
>>
>> I compared various git vers and found the bug to be between 2.31.8
>> and
>> 2.32.0 Comparing ls-remote.c file between those vers gave me :
>>
>> Lines :
>> if (transport_disconnect(transport))
>> return 1;
>>
>> moved to end of sub
>>
>> copying ls-remote.c from 2.31.8 to 2.32.0 - fixed the bug
This partly undoes 68ffe095a2 (transport: also free remote_refs in transport_disconnect(), 2021-03-21). With that patch connections are kept open during ref sorting and printing.
Perhaps the other side gets tired of waiting and aborts? Maybe splitting transport_disconnect() into two functions -- one for disconnecting and and for cleaning up -- would make sense? And disconnecting as soon as possible? Just guessing -- didn't actually reproduce the bug. Still, demo patch below.
>>
>>
>>
>> Code reproducing bug :
>>
>> #!/proj/mislcad/areas/DAtools/tools/perl/5.10.1/bin/perl -w use
>> strict; use Cwd qw(cwd);
>>
>> my $count = 4;
>> for my $f (1..$count) {
>> my $child = fork();
>> if (!$child) {
>> my $curr = cwd();
>>
>> my @repos = `cat l$f`;
>> foreach my $repo (@repos) {
>> chomp $repo;
>> print "$repo\n";
>> chdir($repo);
>> my $remote_tags_str = `git ls-remote 2>&1`;
>> my $remote_tags_str2 = `git ls-remote 2>&1 `;
>> chdir($curr);
>> if ( $remote_tags_str ne $remote_tags_str2) {
>> print "***\n";
>> }
>> }
>>
>> exit(0);
>> }
>> }
>> while (wait != -1) {}
>> 1;
>>
>
> I tried reproducing this regression by:
>
> ```
> $ cd /path/to/git.git
> $ git ls-remote 2>&1 > /tmp/root.list
> $ cd builtin/
> $ git ls-remote 2>&1 > /tmp/builtin.list $ cd ../ $ git diff
> --no-index /tmp/root.list /tmp/builtin.list ```
>
> And indeed, the diff was empty (which meant that both listings are same).
>
> Confused...
>
> --
> An old man doll... just what I always wanted! - Clara
---
builtin/ls-remote.c | 8 +++++---
builtin/remote.c | 3 ++-
transport.c | 15 +++++++++++++--
transport.h | 2 ++
4 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index fc76575430..4c1daa0f92 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -128,6 +128,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
repo_set_hash_algo(the_repository, hash_algo);
}
+ if (transport_disconnect_raw(transport))
+ status = 1;
if (!dest && !quiet)
fprintf(stderr, "From %s\n", *remote->url); @@ -154,12 +156,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
if (show_symref_target && ref->symref)
printf("ref: %s\t%s\n", ref->symref, ref->refname);
printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
- status = 0; /* we found something */
+ if (status != 1)
+ status = 0; /* we found something */
}
ref_array_clear(&ref_array);
- if (transport_disconnect(transport))
- status = 1;
+ transport_clear(transport);
transport_ls_refs_options_release(&transport_options);
return status;
}
diff --git a/builtin/remote.c b/builtin/remote.c index d91bbe728d..055a221942 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1000,6 +1000,7 @@ static int get_remote_ref_states(const char *name,
transport = transport_get(states->remote, states->remote->url_nr > 0 ?
states->remote->url[0] : NULL);
remote_refs = transport_get_remote_refs(transport, NULL);
+ transport_disconnect_raw(transport);
states->queried = 1;
if (query & GET_REF_STATES)
@@ -1008,7 +1009,7 @@ static int get_remote_ref_states(const char *name,
get_head_names(remote_refs, states);
if (query & GET_PUSH_REF_STATES)
get_push_ref_states(remote_refs, states);
- transport_disconnect(transport);
+ transport_clear(transport);
} else {
for_each_ref(append_ref_to_tracked_list, states);
string_list_sort(&states->tracked);
diff --git a/transport.c b/transport.c
index 219af8fd50..c71dab75e9 100644
--- a/transport.c
+++ b/transport.c
@@ -1584,17 +1584,28 @@ int transport_connect(struct transport *transport, const char *name,
die(_("operation not supported by protocol")); }
-int transport_disconnect(struct transport *transport)
+int transport_disconnect_raw(struct transport *transport)
{
int ret = 0;
if (transport->vtable->disconnect)
ret = transport->vtable->disconnect(transport);
+ return ret;
+}
+
+void transport_clear(struct transport *transport) {
if (transport->got_remote_refs)
free_refs((void *)transport->remote_refs);
clear_bundle_list(transport->bundles);
free(transport->bundles);
free(transport);
- return ret;
+}
+
+int transport_disconnect(struct transport *transport) {
+ int ret = transport_disconnect_raw(transport);
+ transport_clear(transport);
+ return 0;
}
/*
diff --git a/transport.h b/transport.h
index 6393cd9823..fd75905568 100644
--- a/transport.h
+++ b/transport.h
@@ -320,6 +320,8 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs);
void transport_unlock_pack(struct transport *transport, unsigned int flags);
int transport_disconnect(struct transport *transport);
+void transport_clear(struct transport *transport);
+int transport_disconnect_raw(struct transport *transport);
char *transport_anonymize_url(const char *url);
void transport_take_over(struct transport *transport,
struct child_process *child);
--
2.42.0
^ permalink raw reply related
* Re: please add link / url to remote - when - git push
From: Michal Suchánek @ 2023-10-30 8:52 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Alexander Mills, git
In-Reply-To: <20231030063633.GA7451@tb-raspi4>
On Mon, Oct 30, 2023 at 07:36:33AM +0100, Torsten Bögershausen wrote:
> On Sun, Oct 29, 2023 at 06:15:35PM -0500, Alexander Mills wrote:
> > When a feature branch is pushed, we get a link in the console to go to
> > remote, however when I push directly to main/master, no such link, eg:
> >
> > ```
> > alex.mills@alex node-be % git push
> > Enumerating objects: 20, done.
> > Counting objects: 100% (20/20), done.
> > Delta compression using up to 12 threads
> > Compressing objects: 100% (10/10), done.
> > Writing objects: 100% (11/11), 1.56 KiB | 799.00 KiB/s, done.
> > Total 11 (delta 7), reused 0 (delta 0), pack-reused 0
> > remote: Resolving deltas: 100% (7/7), completed with 7 local objects.
> > remote: Bypassed rule violations for refs/heads/main:
> > remote:
> > remote: - Changes must be made through a pull request.
> > remote:
> > To github.com:elx-onlinx/beautychat-chatcards.git
> > ffe1e05..bb7b0ef main -> main
> > ```
> >
> > Having the link in the console saves me tremendous time and is
> > extremely effective/efficient. Can we get links in the console plz?
> >
>
> If we look very carfully at the log, we see that all the messages prefixed
> with "remote:" come from the remote (git server).
> In your case github. Other "Git repo servers" like gitlab or bitbucket have
> the same feature.
>
> Git itself, running on your local computer, does not produce this links.
> If you really want to push directly to the main branch and want to see the
> a link, then you better talk to the gihub folks.
On the other hand, option to NOT display those remote messages is also
missing. At least with git 2.35 they are displayed even when -q argument
is given.
Thanks
Michal
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Jeff King @ 2023-10-30 9:06 UTC (permalink / raw)
To: Michal Suchánek; +Cc: Torsten Bögershausen, Alexander Mills, git
In-Reply-To: <20231030085205.GF6241@kitsune.suse.cz>
On Mon, Oct 30, 2023 at 09:52:05AM +0100, Michal Suchánek wrote:
> > If we look very carfully at the log, we see that all the messages prefixed
> > with "remote:" come from the remote (git server).
> > In your case github. Other "Git repo servers" like gitlab or bitbucket have
> > the same feature.
> >
> > Git itself, running on your local computer, does not produce this links.
> > If you really want to push directly to the main branch and want to see the
> > a link, then you better talk to the gihub folks.
>
> On the other hand, option to NOT display those remote messages is also
> missing. At least with git 2.35 they are displayed even when -q argument
> is given.
That is also up to GitHub to fix on the server side:
https://lore.kernel.org/git/20230519090559.GA3515410@coredump.intra.peff.net/
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Jeff King @ 2023-10-30 9:10 UTC (permalink / raw)
To: Andy Koppe; +Cc: Liam Beguin, Junio C Hamano, Kousik Sanagavarapu, git
In-Reply-To: <9a1e3e90-3e94-41fa-897d-5c64c4a42871@gmail.com>
On Sat, Oct 28, 2023 at 07:58:31AM +0100, Andy Koppe wrote:
> > I chose the "a" for "address", but I'm not sold on %aa either.
> > I just couldn't find anything better that wasn't already taken.
> >
> > What about "a@"?
>
> Makes sense, and I suppose there's "%G?" as precedent for using a symbol
> rather than letter in these.
This is pretty subjective, but I somehow find "%a@" hard to parse
visually (despite the fact that yes, "%G?" already crossed that bridge).
But I think the real nail in the coffin is your later comment that we
cannot use capitalization to make the raw/mailmap distinction.
> If that's not suitable though, how about "m" for "mail domain"? It also
> immediately follows "l" for "local-part" in the alphabet.
FWIW, that makes sense to me over "a" (though admittedly it is not
really any less vague than "a", so it really might vary from person to
person).
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-30 9:13 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <ZTjedSluwyrVY+L9@ugly>
On Wed, Oct 25, 2023 at 11:23:01AM +0200, Oswald Buddenhagen wrote:
> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
> > The "//" operator was added in perl 5.10. I'm not sure what you found
> > that makes you think the ship has sailed. The only hits for "//" I see
> > look like the end of substitution regexes ("s/foo//" and similar).
> >
> grep with spaces around the operator, then you can see the instance in
> git-credential-netrc.perl easily.
Ah, yeah, there is one instance there. That script does not have a "use"
marker, though, and we do not necessarily need or want to be as strict
with contrib/ scripts, which are quite optional compared to core
functionality like send-email.
That said, I do suspect that requiring 5.10 or later would not be too
burdensome these days. If we want to do so, then the first step would be
updating the text in INSTALL, along with the "use" directives in most
files. Probably d48b284183 (perl: bump the required Perl version to 5.8
from 5.6.[21], 2010-09-24) could serve as a template.
-Peff
^ 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