* Re: [PATCH v3 03/10] trailer: unify trailer formatting machinery
From: Junio C Hamano @ 2024-02-01 17:48 UTC (permalink / raw)
To: Linus Arver
Cc: Josh Steadmon, Linus Arver via GitGitGadget, git,
Christian Couder, Emily Shaffer, Randall S. Becker
In-Reply-To: <owly1q9x2io6.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Josh Steadmon <steadmon@google.com> writes:
>
>> On 2024.01.31 01:22, Linus Arver via GitGitGadget wrote:
>>> This unification will allow us to delete the format_trailer_info() and
>>> print_tok_val() functions in the next patch. They are not deleted here
>>> in order to keep the diff small.
>>
>> Needs to be removed after squashing v2 patch 4 :)
>
> Oops. Will update in next reroll, thanks.
FWIW, by the way, having them in the same patch made it a lot easier
to compare what the original did (with these removed functions) and
what the updated code would do. When a change is supposed to be a
clean-up of an existing code without changing the behaviour, it helps
to make the before and after versions visible in the patch.
Thanks.
^ permalink raw reply
* Re: Migrate away from vger to GitHub or (on-premise) GitLab?
From: Dragan Simic @ 2024-02-01 17:49 UTC (permalink / raw)
To: Hans Meiser; +Cc: Konstantin Ryabitsev, git
In-Reply-To: <DB9P195MB21301E5E271567256303443CE2432@DB9P195MB2130.EURP195.PROD.OUTLOOK.COM>
On 2024-02-01 18:28, Hans Meiser wrote:
> Thank you for enlightening me and elaborating on all of these very
> important facts!
>
> Just to make sure: So "git" is considered part of the kernel? And the
> "git documentation" is considered part of the kernel, too?
Of course it isn't.
> Shouldn't these topics be separated then into separate repositories,
> particularly the git documentation?
>
> For people like me, who are contributing to dozens of documentations
> on GitHub (and GitLab) … We don't focus on the kernel alone. We
> receive dozens of important technical, business and financially
> important e-mails from different sources day by day. So, people like
> me need some modern, common channels/tools for contributing. (If
> contribution is considered helpful and valuable by the kernel team at
> all.)
Could you, please, clarify what kind of git documentation are you
referring to? Are you having git man pages in mind?
> With todays platforms, issues can be created by e-mail and e-mails
> will be received with each issue update. It's even possible to upload
> patches via REST services. No web browser required. So this would keep
> mailing list users acquainted to their habit.
>
> Setting up a local (on-premise) GitLab or Azure DevOps server for
> long-term use should not be impossible. I'm running each of these
> myself. Once installed on-premise, the installation wouldn't be bound
> to any continuous support. All it needs is a provider for keeping the
> server machine running.
Quite frankly, I think you've missed some important points from the
Konstantin's message. To sum it up a bit, not having continuous support
is simply unacceptable for any kind of a long-term project.
^ permalink raw reply
* Re: [PATCH v3 04/10] sequencer: use the trailer iterator
From: Junio C Hamano @ 2024-02-01 18:06 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <bf2b8e1a3c4bc77022fab1ebaa0fc89a7813b4c4.1706664145.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> This patch allows for the removal of "trailer_info_get()" from the
> trailer.h API, which will be in the next patch.
Hmph, do you mean "shortlog" and the sequencer were the only two
external callers and with this we can make it file-scope static to
trailer.c? Or do you mean the next step will be more than a removal
of a declaration from trailer.h plus adding "static" in front of its
definition in trailer.c, because there need other adjustments before
that happens?
> Instead of calling "trailer_info_get()", which is a low-level function
> in the trailers implementation (trailer.c), call
> trailer_iterator_advance(), which was specifically designed for public
> consumption in f0939a0eb1 (trailer: add interface for iterating over
> commit trailers, 2020-09-27).
;-).
> Also, teach the iterator about non-trailer lines, by adding a new field
> called "raw" to hold both trailer and non-trailer lines. This is
> necessary because a "trailer block" is a list of trailer lines of at
> least 25% trailers (see 146245063e (trailer: allow non-trailers in
> trailer block, 2016-10-21)), such that it may hold non-trailer lines.
That sounds like a task larger than something we would want in a
patch that focuses on another task (e.g. update sequencer not to
call trailer_info_get()) while at it. It seems from a casual glance
that the change to shortlog.c is to accomodate this change in the
semantics of what the iterator could return? It smells that this
patch does two more or less unrelated things at the same time?
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> builtin/shortlog.c | 7 +++++--
> sequencer.c | 35 +++++++++++++++--------------------
> trailer.c | 17 +++++++++--------
> trailer.h | 13 +++++++++++++
> 4 files changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 1307ed2b88a..dc8fd5a5532 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -172,7 +172,7 @@ static void insert_records_from_trailers(struct shortlog *log,
> const char *oneline)
> {
> struct trailer_iterator iter;
> - const char *commit_buffer, *body;
> + const char *commit_buffer, *body, *value;
> struct strbuf ident = STRBUF_INIT;
>
> if (!log->trailers.nr)
> @@ -190,7 +190,10 @@ static void insert_records_from_trailers(struct shortlog *log,
>
> trailer_iterator_init(&iter, body);
> while (trailer_iterator_advance(&iter)) {
> - const char *value = iter.val.buf;
> + if (!iter.is_trailer)
> + continue;
> +
> + value = iter.val.buf;
>
> if (!string_list_has_string(&log->trailers, iter.key.buf))
> continue;
> diff --git a/sequencer.c b/sequencer.c
> index 3cc88d8a800..bc7c82c5271 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -319,37 +319,32 @@ static const char *get_todo_path(const struct replay_opts *opts)
> static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
> size_t ignore_footer)
> {
> - struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> - struct trailer_info info;
> - size_t i;
> - int found_sob = 0, found_sob_last = 0;
> - char saved_char;
> -
> - opts.no_divider = 1;
> + struct trailer_iterator iter;
> + size_t i = 0, found_sob = 0;
> + char saved_char = sb->buf[sb->len - ignore_footer];
>
> if (ignore_footer) {
> - saved_char = sb->buf[sb->len - ignore_footer];
> sb->buf[sb->len - ignore_footer] = '\0';
> }
>
> - trailer_info_get(&info, sb->buf, &opts);
> + trailer_iterator_init(&iter, sb->buf);
> + while (trailer_iterator_advance(&iter)) {
> + i++;
> + if (sob &&
> + iter.is_trailer &&
> + !strncmp(iter.raw, sob->buf, sob->len)) {
> + found_sob = i;
> + }
> + }
> + trailer_iterator_release(&iter);
>
> if (ignore_footer)
> sb->buf[sb->len - ignore_footer] = saved_char;
>
> - if (info.trailer_block_start == info.trailer_block_end)
> + if (!i)
> return 0;
>
> - for (i = 0; i < info.trailer_nr; i++)
> - if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
> - found_sob = 1;
> - if (i == info.trailer_nr - 1)
> - found_sob_last = 1;
> - }
> -
> - trailer_info_release(&info);
> -
> - if (found_sob_last)
> + if (found_sob == i)
> return 3;
> if (found_sob)
> return 2;
> diff --git a/trailer.c b/trailer.c
> index 71ea2bb67f8..5bcc9b0006c 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1158,17 +1158,18 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
>
> int trailer_iterator_advance(struct trailer_iterator *iter)
> {
> - while (iter->internal.cur < iter->internal.info.trailer_nr) {
> - char *trailer = iter->internal.info.trailers[iter->internal.cur++];
> - int separator_pos = find_separator(trailer, separators);
> -
> - if (separator_pos < 1)
> - continue; /* not a real trailer */
> -
> + char *line;
> + int separator_pos;
> + if (iter->internal.cur < iter->internal.info.trailer_nr) {
> + line = iter->internal.info.trailers[iter->internal.cur++];
> + separator_pos = find_separator(line, separators);
> + iter->is_trailer = (separator_pos > 0);
> +
> + iter->raw = line;
> strbuf_reset(&iter->key);
> strbuf_reset(&iter->val);
> parse_trailer(&iter->key, &iter->val, NULL,
> - trailer, separator_pos);
> + line, separator_pos);
> unfold_value(&iter->val);
> return 1;
> }
> diff --git a/trailer.h b/trailer.h
> index 244f29fc91f..a7599067acc 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -127,6 +127,19 @@ struct trailer_iterator {
> struct strbuf key;
> struct strbuf val;
>
> + /*
> + * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
> + * key/val pair as part of a trailer block. A trailer block can be
> + * either 100% trailer lines, or mixed in with non-trailer lines (in
> + * which case at least 25% must be trailer lines).
> + */
> + const char *raw;
> +
> + /*
> + * 1 if the raw line was parsed as a trailer line (key/val pair).
> + */
> + int is_trailer;
> +
> /* private */
> struct {
> struct trailer_info info;
^ permalink raw reply
* Re: [PATCH v3 03/10] trailer: unify trailer formatting machinery
From: Linus Arver @ 2024-02-01 18:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Josh Steadmon, Linus Arver via GitGitGadget, git,
Christian Couder, Emily Shaffer, Randall S. Becker
In-Reply-To: <xmqqjzno13ev.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Josh Steadmon <steadmon@google.com> writes:
>>
>>> On 2024.01.31 01:22, Linus Arver via GitGitGadget wrote:
>>>> This unification will allow us to delete the format_trailer_info() and
>>>> print_tok_val() functions in the next patch. They are not deleted here
>>>> in order to keep the diff small.
>>>
>>> Needs to be removed after squashing v2 patch 4 :)
>>
>> Oops. Will update in next reroll, thanks.
>
> FWIW, by the way, having them in the same patch made it a lot easier
> to compare what the original did (with these removed functions) and
> what the updated code would do. When a change is supposed to be a
> clean-up of an existing code without changing the behaviour, it helps
> to make the before and after versions visible in the patch.
>
> Thanks.
Ack, will try to keep that in mind.
^ permalink raw reply
* Re: [PATCH v3 03/10] trailer: unify trailer formatting machinery
From: Linus Arver @ 2024-02-01 18:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <xmqqfryd2drm.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> I could just grow this series with another ~22 patches to include those
>> additional refactors, but I am hesitant about doing so, simply due to
>> the sheer number of them.
>
> I actually do not mind at all if you started with a preliminary
> clean-up series, and stopped the first batch somewhere in the middle
> of the 20+ patches before even reaching any of these 10 patches we
> see here, if that gives us a readable set of bite-sized changes that
> prepare a solid foundation to rebuild things on top. I am having a
> feeling that not even a single person has reviewed them on list even
> though we are already at the third iteration, which is quite
> frustrating (and I would imagine that it would be frustrating for
> you, too), and I suspect that the step like [v3 03/10] that makes
> too large a change with too little explanation (and perhaps a bit of
> "trust me, this does not change the behaviour") is one contributing
> factor why people are afraid of touching it.
I am planning to spend today trying to break up this patch into smaller
preparatory chunks that still end up at the same state as this patch.
Will post another update on how this goes by EOD. Thanks.
^ permalink raw reply
* Re: Migrate away from vger to GitHub or (on-premise) GitLab?
From: Hans Meiser @ 2024-02-01 18:36 UTC (permalink / raw)
To: Dragan Simic; +Cc: Konstantin Ryabitsev, git@vger.kernel.org
In-Reply-To: <c3b6de0c2ccf71f0dfa5aff06fa63d8f@manjaro.org>
> Could you, please, clarify what kind of git documentation are you
> referring to? Are you having git man pages in mind?
Yes, these in particular.
From my point of view, many of these are quite unorganized, hard to read and – as I believe – need a fix-up. Markdown could replace the currently used language, so editing them would be more easy, proving support for preview and lint the documentation.
>Quite frankly, I think you've missed some important points from the
> Konstantin's message. To sum it up a bit, not having continuous support
> is simply unacceptable for any kind of a long-term project.
As I wrote, once installed on-premise, no-one will shut down an on-premise git server except for yourself. It can run for eternity. You just need someone to administer it properly and publish the website.
In the end, it's all just about git. You may create your own git webserver (https://git-scm.com/book/en/v2/Git-on-the-Server-GitWeb), or just use an existing one, like the GitLab server: https://about.gitlab.com/install/
In these servers, everything is configurable. Moreover, many plug-ins exist for plumbing extensions to these providers. It's possible to establish your own workflow, rights management and automatic handling. You just need someone who is an expert with the tool of your choice.
Many other great repositories already are using one of those providers; Meta, Google, Microsoft for example share their code there – just to name a few. I wouldn't consider these users as being known for being exceptional risk-takers.
^ permalink raw reply
* Re: [PATCH v3 05/10] trailer: make trailer_info struct private
From: Junio C Hamano @ 2024-02-01 18:49 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <c19c1dcc592186d8da2857692f4ebbfe35adfac0.1706664145.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Make this struct private by putting its definition inside trailer.c.
> This has two benefits:
>
> (1) it makes the surface area of the public facing
> interface (trailer.h) smaller, and
>
> (2) external API users are unable to peer inside this struct (because
> it is only ever exposed as an opaque pointer).
At the cost of an extra pointer dereference every time the member of
the struct is accessed, plus the cost of allocation/deallocation.
Which may not be a huge deal, but I wonder if the approach to name
the member of the outer struct with "private" that seems to be used
in other parts of the code (e.g. the .private_size member in the
hashmap structure, the .refs_private member in the repository
structure) or even a documented convention (e.g. raw_object_store),
might be more appropriate here. If Coccinelle works well (which we
may be having some trouble with --- cf. <xmqq1q9ybsnl.fsf@gitster.g>),
we should be able to catch external accesses without having to hide
the implementation details via an extra pointer dereference.
> @@ -176,11 +176,12 @@ static void interpret_trailers(const struct process_trailer_options *opts,
> strbuf_release(&trailer_block);
>
> free_trailers(&head);
> - trailer_info_release(&info);
>
> /* Print the lines after the trailers as is */
> if (!opts->only_trailers)
> - fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
> + fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
> +
> + trailer_info_release(info);
Interesting. Is this an indenendent bugfix even if we decided not
to take this patch? No, I have not fully decided to be negative on
the move this entire patch makes (even though I am leaning towards
saying so). Just hypothetically, even if we wanted to keep "info"
here as a structure and not a pointer to an opaque structure,
doesn't this hunk fix a real bug?
Well, technically, not quite, because the members referenced in that
if (.only_trailers) block are still live in the info struct. But it
still smells wrong to access info.* after calling _release() on it,
and this fix should come before "info" is turned from an instance to
a pointer, I would say.
> diff --git a/trailer.h b/trailer.h
> index a7599067acc..e19ddf84e64 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -4,6 +4,8 @@
> #include "list.h"
> #include "strbuf.h"
>
> +struct trailer_info;
> +
> enum trailer_where {
> WHERE_DEFAULT,
> WHERE_END,
> @@ -29,27 +31,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
> int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
> int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
>
> +size_t trailer_block_start(struct trailer_info *info);
> +size_t trailer_block_end(struct trailer_info *info);
> +int blank_line_before_trailer_block(struct trailer_info *info);
And we need new accessors, which is a good change regardless of the
answer to the "do we really want an extra pointer dereference?
Shouldn't the existing 'private' and 'internal' label we see below
sufficient?" question.
> @@ -142,7 +123,7 @@ struct trailer_iterator {
>
> /* private */
> struct {
> - struct trailer_info info;
> + struct trailer_info *info;
> size_t cur;
> } internal;
> };
^ permalink raw reply
* Re: [PATCH v3 06/10] trailer: spread usage of "trailer_block" language
From: Junio C Hamano @ 2024-02-01 18:57 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <0a9a7438c3ff39f1434087bf3ed6a9865758c803.1706664145.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Deprecate the "trailer_info" struct name and replace it with
> "trailer_block". The main reason is to help readability, because
> "trailer_info" on the surface sounds like it's about a single trailer
> when in reality it is a collection of contiguous lines, at least 25% of
> which are trailers.
Yup, "info" is usually a fairly meaningless word. At least "block"
may imply it is a collection of trailers.
The naming would not matter as much to the API users, if the thing
is now opaque, though.
^ permalink raw reply
* Re: Migrate away from vger to GitHub or (on-premise) GitLab?
From: Dragan Simic @ 2024-02-01 19:00 UTC (permalink / raw)
To: Hans Meiser; +Cc: Konstantin Ryabitsev, git
In-Reply-To: <DB9P195MB21303B5546A764A18FE78C97E2432@DB9P195MB2130.EURP195.PROD.OUTLOOK.COM>
On 2024-02-01 19:36, Hans Meiser wrote:
>> Could you, please, clarify what kind of git documentation are you
>> referring to? Are you having git man pages in mind?
>
> Yes, these in particular.
>
> From my point of view, many of these are quite unorganized, hard to
> read and – as I believe – need a fix-up. Markdown could replace the
> currently used language, so editing them would be more easy, proving
> support for preview and lint the documentation.
Please keep in mind that editing the git man pages requires very
intimate knowledge of the related git source code. Many times even
small changes to the language style can change the meaning and diverge
the man pages from the source code, making the man pages useless.
>> Quite frankly, I think you've missed some important points from the
>> Konstantin's message. To sum it up a bit, not having continuous
>> support
>> is simply unacceptable for any kind of a long-term project.
>
> As I wrote, once installed on-premise, no-one will shut down an
> on-premise git server except for yourself. It can run for eternity.
> You just need someone to administer it properly and publish the
> website.
A git server? I was under impression that you proposed running an
own instance of GitLab or something similar.
> In the end, it's all just about git. You may create your own git
> webserver (https://git-scm.com/book/en/v2/Git-on-the-Server-GitWeb),
> or just use an existing one, like the GitLab server:
> https://about.gitlab.com/install/
>
> In these servers, everything is configurable. Moreover, many plug-ins
> exist for plumbing extensions to these providers. It's possible to
> establish your own workflow, rights management and automatic handling.
> You just need someone who is an expert with the tool of your choice.
>
> Many other great repositories already are using one of those
> providers; Meta, Google, Microsoft for example share their code there
> – just to name a few. I wouldn't consider these users as being known
> for being exceptional risk-takers.
^ permalink raw reply
* Re: [PATCH v3 07/10] trailer: prepare to move parse_trailers_from_command_line_args() to builtin
From: Junio C Hamano @ 2024-02-01 19:06 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <97e5d86ddf0ea25df1b512dfbf620910298c1b02.1706664145.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Rename add_arg_item() to trailer_add_arg_item() because it will have to
> be exposed as an API function in the next patch. Rename
> new_trailers_clear() to free_new_trailers() because it will be promoted
> into an API function; the API already has free_trailers(), so using the
> "free_*" naming style will keep it consistent. Also rename "conf_info"
> to "trailer_conf" for readability, dropping the low-value "_info" suffix
> as we did earlier in this series for "trailer_info" to "trailer_block".
Makes sense.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 04/10] sequencer: use the trailer iterator
From: Linus Arver @ 2024-02-01 19:14 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker
In-Reply-To: <xmqqa5ok12lt.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Linus Arver <linusa@google.com>
>>
>> This patch allows for the removal of "trailer_info_get()" from the
>> trailer.h API, which will be in the next patch.
>
> Hmph, do you mean "shortlog" and the sequencer were the only two
> external callers and with this we can make it file-scope static to
> trailer.c?
This was what I meant (originally ...
> Or do you mean the next step will be more than a removal
> of a declaration from trailer.h plus adding "static" in front of its
> definition in trailer.c, because there need other adjustments before
> that happens?
... but now I realize that the operation adds a few small tweaks, such
as tweaking the parameters it expects and also what it returns). In the
spirit of breaking up patch 3, I will also break this up into
preparatory patches as well.
>> Also, teach the iterator about non-trailer lines, by adding a new field
>> called "raw" to hold both trailer and non-trailer lines. This is
>> necessary because a "trailer block" is a list of trailer lines of at
>> least 25% trailers (see 146245063e (trailer: allow non-trailers in
>> trailer block, 2016-10-21)), such that it may hold non-trailer lines.
>
> That sounds like a task larger than something we would want in a
> patch that focuses on another task (e.g. update sequencer not to
> call trailer_info_get()) while at it. It seems from a casual glance
> that the change to shortlog.c is to accomodate this change in the
> semantics of what the iterator could return? It smells that this
> patch does two more or less unrelated things at the same time?
I think you're correct. Hopefully breaking this up will make things
easier to review.
I am learning very quickly from your review comments in patch 3 and in
here that, in the absence of area experts, the existing tests/CI tests
cannot be trusted alone (after all some tests could be missing), which
makes it more important to do so-called "micro-commits".
But overall, breaking things up is a good thing anyway as a general
practice, so, I think this is a good lesson. TBH I have a (bad) habit of
saying "is the diff ~100 lines?" and if so I don't spend any time
thinking of breaking these up. X-<
Thanks.
^ permalink raw reply
* Re: [PATCH v3 03/10] trailer: unify trailer formatting machinery
From: Junio C Hamano @ 2024-02-01 19:21 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <owlyo7d011no.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> I am planning to spend today trying to break up this patch into smaller
> preparatory chunks that still end up at the same state as this patch.
>
> Will post another update on how this goes by EOD. Thanks.
I stopped reading the function after noticing the double unfolding,
so there may be similar "why do we do this unexplained new thing in
the function that the original didn't?" issues in the "same state",
In any case, if I understood your plan I heard from you in the
discussion yesterday correctly, the unfolding should not be added to
format (to make it double), but would be moved from parse to format
in a single step. It would avoid making it double, and would make
the parse step about purely parsing without modification, which is a
very worthwhile thing to do. So I am not sure if we want to end up
with the same state in the first place, though.
THanks.
^ permalink raw reply
* Re: [RFC PATCH 1/4] t0080: turn t-basic unit test into a helper
From: Josh Steadmon @ 2024-02-01 19:40 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, johannes.schindelin, phillip.wood
In-Reply-To: <20240123004306.GA835964@coredump.intra.peff.net>
On 2024.01.22 19:43, Jeff King wrote:
> On Tue, Jan 16, 2024 at 02:54:21PM -0800, Junio C Hamano wrote:
>
> > > ... so let's move it to t/t0080/t-basic.c and
> > > adjust Makefiles and .gitignores as necessary.
> >
> > ... this conclusion. I somehow thought that t-basic part would be a
> > good test-tool subcommand, as it is run from the suite of shell
> > scripts for end-to-end testing CLI interaction.
>
> Heh, I was about to ask the same thing.
>
> In particular...
>
> > Do we have any precedent to place programs placed under t/tXXXX/ and
> > get them compiled?
>
> ...no, I don't think we do. And quite often I exclude those directories
> when grepping around the code base, because there is often code there
> that is purely used as a data fixture. E.g., t4256 contains a copy of
> mailinfo.c which it uses as input for some of the tests. That code also
> happens to have out-of-bounds memory reads which we have since fixed in
> the real mailinfo.c, but of course "grep" finds them both. :)
>
> So I would prefer a rule like "no buildable code in t/t[0-9]*". Barring
> that, maybe we could avoid using things that look too much like real Git
> code in our tests (though we sometimes do need fake code for things like
> funclines, and even that might end up creating false positives).
>
> -Peff
Ack. I've moved this to a test-tool subcommand for V2, which I hope to
send out soon.
^ permalink raw reply
* Re: Migrate away from vger to GitHub or (on-premise) GitLab?
From: Dragan Simic @ 2024-02-01 20:09 UTC (permalink / raw)
To: rsbecker; +Cc: 'Hans Meiser', 'Konstantin Ryabitsev', git
In-Reply-To: <060d01da5549$6e93e250$4bbba6f0$@nexbridge.com>
On 2024-02-01 21:01, rsbecker@nexbridge.com wrote:
> On Thursday, February 1, 2024 2:00 PM, Dragan Simic wrote:
>> On 2024-02-01 19:36, Hans Meiser wrote:
>>>> Quite frankly, I think you've missed some important points from the
>>>> Konstantin's message. To sum it up a bit, not having continuous
>>>> support is simply unacceptable for any kind of a long-term project.
>>>
>>> As I wrote, once installed on-premise, no-one will shut down an
>>> on-premise git server except for yourself. It can run for eternity.
>>> You just need someone to administer it properly and publish the
>>> website.
>>
>> A git server? I was under impression that you proposed running an own
>> instance of
>> GitLab or something similar.
>
> Git is unique, as a project, given that everything (! Not everything
> but a whole lot) is managed using git, including the enterprise git
> server platforms.
>
> A huge advantage of using a git server is being able to mirror the
> repository. If we went with a GitLab host, we could potentially mirror
> over to GitHub. The drawback is that the pull request history (and
> related discussions) id not (currently) preserved. I think this is a
> situation no matter what, even if we go GitLab/GitLab or
> GitHub/GitHub. The value of the discussion threads is the most
> important part of what needs to be preserved. I have high confidence
> that the team could move to either Pull Request/Merge Request
> structure reasonably easily, but if we had to move again in future
> (count on it), there must be a way to preserve the community assets of
> the discussions that went into making decisions. Without that, I am
> concerned that a migration to a GitLab (or any other) instance would
> increase velocity but put long term decisions at risk.
Good point, I agree that the value of the discussions on the mailing
list is extremely high. We should also keep in mind that the risk of
a vendor lock-in is even higher when it comes to the discussions.
Frankly, the resilience of email as a service and the openness of its
format can hardly be beaten.
>>> In the end, it's all just about git. You may create your own git
>>> webserver (https://git-scm.com/book/en/v2/Git-on-the-Server-GitWeb),
>>> or just use an existing one, like the GitLab server:
>>> https://about.gitlab.com/install/
>>>
>>> In these servers, everything is configurable. Moreover, many plug-ins
>>> exist for plumbing extensions to these providers. It's possible to
>>> establish your own workflow, rights management and automatic
>>> handling.
>>> You just need someone who is an expert with the tool of your choice.
>>>
>>> Many other great repositories already are using one of those
>>> providers; Meta, Google, Microsoft for example share their code there
>>> – just to name a few. I wouldn't consider these users as being known
>>> for being exceptional risk-takers.
^ permalink raw reply
* RE: Migrate away from vger to GitHub or (on-premise) GitLab?
From: rsbecker @ 2024-02-01 20:01 UTC (permalink / raw)
To: 'Dragan Simic', 'Hans Meiser'
Cc: 'Konstantin Ryabitsev', git
In-Reply-To: <691395bc13ea6c3013adcb98cfcbd102@manjaro.org>
On Thursday, February 1, 2024 2:00 PM, Dragan Simic wrote:
>On 2024-02-01 19:36, Hans Meiser wrote:
>>> Could you, please, clarify what kind of git documentation are you
>>> referring to? Are you having git man pages in mind?
>>
>> Yes, these in particular.
>>
>> From my point of view, many of these are quite unorganized, hard to
>> read and – as I believe – need a fix-up. Markdown could replace the
>> currently used language, so editing them would be more easy, proving
>> support for preview and lint the documentation.
>
>Please keep in mind that editing the git man pages requires very intimate knowledge
>of the related git source code. Many times even small changes to the language style
>can change the meaning and diverge the man pages from the source code, making
>the man pages useless.
>
>>> Quite frankly, I think you've missed some important points from the
>>> Konstantin's message. To sum it up a bit, not having continuous
>>> support is simply unacceptable for any kind of a long-term project.
>>
>> As I wrote, once installed on-premise, no-one will shut down an
>> on-premise git server except for yourself. It can run for eternity.
>> You just need someone to administer it properly and publish the
>> website.
>
>A git server? I was under impression that you proposed running an own instance of
>GitLab or something similar.
Git is unique, as a project, given that everything (! Not everything but a whole lot) is managed using git, including the enterprise git server platforms.
A huge advantage of using a git server is being able to mirror the repository. If we went with a GitLab host, we could potentially mirror over to GitHub. The drawback is that the pull request history (and related discussions) id not (currently) preserved. I think this is a situation no matter what, even if we go GitLab/GitLab or GitHub/GitHub. The value of the discussion threads is the most important part of what needs to be preserved. I have high confidence that the team could move to either Pull Request/Merge Request structure reasonably easily, but if we had to move again in future (count on it), there must be a way to preserve the community assets of the discussions that went into making decisions. Without that, I am concerned that a migration to a GitLab (or any other) instance would increase velocity but put long term decisions at risk.
>> In the end, it's all just about git. You may create your own git
>> webserver (https://git-scm.com/book/en/v2/Git-on-the-Server-GitWeb),
>> or just use an existing one, like the GitLab server:
>> https://about.gitlab.com/install/
>>
>> In these servers, everything is configurable. Moreover, many plug-ins
>> exist for plumbing extensions to these providers. It's possible to
>> establish your own workflow, rights management and automatic handling.
>> You just need someone who is an expert with the tool of your choice.
>>
>> Many other great repositories already are using one of those
>> providers; Meta, Google, Microsoft for example share their code there
>> – just to name a few. I wouldn't consider these users as being known
>> for being exceptional risk-takers.
--Randall
^ permalink raw reply
* Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...
From: Junio C Hamano @ 2024-02-01 20:20 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Patrick Steinhardt, John Cai, Christian Couder
In-Reply-To: <20240201115809.1177064-4-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> In 9830926c7d (rev-list: add commit object support in `--missing`
> option, 2023-10-27) we fixed the `--missing` option in `git rev-list`
> so that it now works with commits too.
>
> Unfortunately, such a command would still fail with a "fatal: bad
> object <oid>" if it is passed a missing commit, blob or tree as an
> argument.
>
> When such a command is used to find the dependencies of some objects,
> for example the dependencies of quarantined objects, it would be
> better if the command would instead consider such missing objects,
> especially commits, in the same way as other missing objects.
>
> If, for example `--missing=print` is used, it would be nice for some
> use cases if the missing tips passed as arguments were reported in
> the same way as other missing objects instead of the command just
> failing.
>
> Let's introduce a new `--allow-missing-tips` option to make it work
> like this.
OK. Unlike a missing object referenced by a tree, a commit, or a
tag, where the expected type of the missing object is known to Git,
I would expect that nobody knowsn what type these missing objects at
the tip have. So do we now require "object X missing" instead of
"commit X missing" in the output? If we are not giving any type
information even when we know what the type we expect is, then we do
not have to worry about this change introducing a new output logic,
I guess.
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index b3f4783858..ae7bb15478 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -562,6 +562,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> break;
> }
> }
> + for (i = 1; i < argc; i++) {
> + const char *arg = argv[i];
> + if (!strcmp(arg, "--allow-missing-tips")) {
> + if (arg_missing_action == MA_ERROR)
> + die(_("option '%s' only makes sense with '%s' set to '%s' or '%s'"),
> + "--allow-missing-tips", "--missing=", "allow-*", "print");
> + revs.do_not_die_on_missing_tips = 1;
> + break;
> + }
> + }
It is unfortunate that we need to add yet another dumb loop that
does not even try to understand there may be an option whose value
happens to be the string strcmp() looks for (we already have two
such loops above this hunk). I have to wonder if we can do better.
An idle piece of idea. Perhaps we can instruct setup_revisions()
not to die immediately upon seeing a problematic argument, marking
the revs as "broken" instead, and keep going and interpreting as
much as it could, so that it may record the presence of "--missing",
"--exclude-promisor-objects", and "--allow-missing-tips". Then upon
seeing setup_revisions() return with such an error, we can redo the
call with these bits already on.
Anyway, I digress.
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> index 527aa94f07..283e8fc2c2 100755
> --- a/t/t6022-rev-list-missing.sh
> +++ b/t/t6022-rev-list-missing.sh
> @@ -77,4 +77,55 @@ do
> done
> done
>
> +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + for tip in "" "HEAD"
> + do
> + for action in "allow-any" "print"
> + do
> + test_expect_success "--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'" '
> + oid="$(git rev-parse $obj)" &&
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> + # Before the object is made missing, we use rev-list to
> + # get the expected oids.
> + if [ "$tip" = "HEAD" ]; then
Style:
if test "$tip" = "HEAD"
then
> + git rev-list --objects --no-object-names \
> + HEAD ^$obj >expect.raw
> + else
> + >expect.raw
> + fi &&
> +
> + # Blobs are shared by all commits, so even though a commit/tree
> + # might be skipped, its blob must be accounted for.
> + if [ "$tip" = "HEAD" ] && [ $obj != "HEAD:1.t" ]; then
Ditto.
> + echo $(git rev-parse HEAD:1.t) >>expect.raw &&
> + echo $(git rev-parse HEAD:2.t) >>expect.raw
> + fi &&
> +
> + mv "$path" "$path.hidden" &&
> + test_when_finished "mv $path.hidden $path" &&
> +
> + git rev-list --missing=$action --allow-missing-tips \
> + --objects --no-object-names $oid $tip >actual.raw &&
> +
> + # When the action is to print, we should also add the missing
> + # oid to the expect list.
> + case $action in
> + allow-any)
> + ;;
> + print)
> + grep ?$oid actual.raw &&
> + echo ?$oid >>expect.raw
> + ;;
OK. We do not say anything more than the object name (and the fact
that it is missing with a single byte '?'), so my earlier worry was
unfounded. Good.
> + esac &&
> +
> + sort actual.raw >actual &&
> + sort expect.raw >expect &&
> + test_cmp expect actual
> + '
> + done
> + done
> +done
> +
> test_done
THanks.
^ permalink raw reply
* [ANNOUNCE] Git Rev News edition 107
From: Christian Couder @ 2024-02-01 20:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narebski, Markus Jansen,
Štěpán Němec, Kaartic Sivaraam, Taylor Blau,
Johannes Schindelin, Ævar Arnfjörð Bjarmason, lwn,
Bruno Brito, Elijah Newren, Brandon Pugh, Jeremy Pridmore,
Philip Oakley, John Cai
Hi everyone,
The 107th edition of Git Rev News is now published:
https://git.github.io/rev_news/2024/01/31/edition-107/
Thanks a lot to Elijah Newren, Bruno Brito, Brandon Pugh and Štěpán
Němec who helped this month!
Enjoy,
Christian, Jakub, Markus and Kaartic.
PS: An issue for the next edition is already opened and contributions
are welcome:
https://github.com/git/git.github.io/issues/686
^ permalink raw reply
* Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...
From: Junio C Hamano @ 2024-02-01 21:27 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Patrick Steinhardt, John Cai, Christian Couder
In-Reply-To: <20240201115809.1177064-4-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> When such a command is used to find the dependencies of some objects,
> for example the dependencies of quarantined objects, it would be
> better if the command would instead consider such missing objects,
> especially commits, in the same way as other missing objects.
>
> If, for example `--missing=print` is used, it would be nice for some
> use cases if the missing tips passed as arguments were reported in
> the same way as other missing objects instead of the command just
> failing.
>
> Let's introduce a new `--allow-missing-tips` option to make it work
> like this.
An obvious question is if this even needs to be a new option. What
are expected use cases where --missing=print without this option is
beneficial? If there is no plausible use case, perhaps we can treat
it as a mere bugfix to the existing --missing mechanism, especially
given that support of commits in "--missing" itself is relatively
a new thing.
If we can do this as a bugfix that is always on when --missing is
used, then we do not have to worry about adding another tasteless
loop outside the main command line parser, which is a huge upside
;-).
^ permalink raw reply
* Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests
From: Josh Steadmon @ 2024-02-01 22:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <xmqqv87sx3y2.fsf@gitster.g>
On 2024.01.16 15:18, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> > With these changes, you can now use test-tool to run the unit tests:
> > $ make
> > $ cd t/unit-tests/bin
> > $ ../../helper/test-tool run-command testsuite --no-run-in-shell \
> > --no-require-shell-test-pattern
>
> This makes me wonder why we want to do the readdir() loop ourselves.
> Instead of saying --no-require-shell-test-pattern there, wouldn't it
> be simpler to say "*" right there, and have testsuite() run the test
> programs named from the command line?
It's speculation on my part, but I wonder if it has something to do with
the number of shell tests? Google tells me that on Windows, the maximum
command line length is 8191 characters. Which is actually a fair bit
smaller than expanding the shell test list:
$ echo t????-*.sh | wc -c
25714
^ permalink raw reply
* Re: [PATCH v3 08/10] trailer: move arg handling to interpret-trailers.c
From: Junio C Hamano @ 2024-02-01 22:23 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <465dc51cdcba28d235241021bc52369f6082d329.1706664145.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> We don't move the "arg_item" struct to interpret-trailers.c, because it
> is now a struct that contains information about trailers that could be
> added into the input text's own trailers. This is a generic concept that
> extends beyond trailers defined as CLI arguments (it applies to trailers
> defined in configuration as well). We will rename "arg_item" to
> "trailer_template" in a follow-up patch to keep the diff here small.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> builtin/interpret-trailers.c | 88 ++++++++++++++++++++++--------------
> trailer.c | 62 ++++++++++++++++++-------
> trailer.h | 12 +++++
> 3 files changed, 112 insertions(+), 50 deletions(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 9f0ba39b317..9a902012912 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -45,23 +45,17 @@ static int option_parse_if_missing(const struct option *opt,
> return trailer_set_if_missing(opt->value, arg);
> }
>
> -static void free_new_trailers(struct list_head *trailers)
> -{
> - struct list_head *pos, *tmp;
> - struct new_trailer_item *item;
> -
> - list_for_each_safe(pos, tmp, trailers) {
> - item = list_entry(pos, struct new_trailer_item, list);
> - list_del(pos);
> - free(item);
> - }
> -}
> +static char *cl_separators;
It somehow feels ugly and goes backward to depend on a new global,
especially when you are aiming to libify more things. If we are
doing something like ...
> static int option_parse_trailer(const struct option *opt,
> const char *arg, int unset)
> {
> struct list_head *trailers = opt->value;
> - struct new_trailer_item *item;
> + struct strbuf tok = STRBUF_INIT;
> + struct strbuf val = STRBUF_INIT;
> + const struct trailer_conf *conf;
> + struct trailer_conf *conf_current = new_trailer_conf();
> + ssize_t separator_pos;
>
> if (unset) {
> free_new_trailers(trailers);
> @@ -71,12 +65,31 @@ static int option_parse_trailer(const struct option *opt,
> if (!arg)
> return -1;
>
> - item = xmalloc(sizeof(*item));
> - item->text = arg;
> - item->where = where;
> - item->if_exists = if_exists;
> - item->if_missing = if_missing;
> - list_add_tail(&item->list, trailers);
> + separator_pos = find_separator(arg, cl_separators);
> + if (separator_pos) {
> + parse_trailer(arg, separator_pos, &tok, &val, &conf);
> + duplicate_trailer_conf(conf_current, conf);
> +
> + /*
> + * Override conf_current with settings specified via CLI flags.
> + */
... this, which apparently needs to know that we are dealing with
input from CLI, shouldn't we be able to do the "CLI allows '=' as
well as whatever is the default" here, instead of mucking with the
global variable in the caller?
Even if you plan to later make this function callable from outside
the context of parse_options() callback, and if you do not want "CLI
allows '=' as well" for such new callers, we should be able to have
a shared helper function that is the bulk of this function but takes
additional parameter to tweak its behaviour slightly depending on
the needs of the callers?
> + trailer_conf_set(where, if_exists, if_missing, conf_current);
I am getting an impression that the use of and the introduction of
the new helper, mixed with movement of the responsibility, makes
reviewing the change unnecessarily more cumbersome. An equivalent
series with a few more steps, each of them focusing on a small
change (like, "updating the conf_current members is done with
assignments that appear as a pattern very often---introduce a helper
to reduce boilerplate") would have been more enticing to reviewers.
> + trailer_add_arg_item(strbuf_detach(&tok, NULL),
> + strbuf_detach(&val, NULL),
> + conf_current,
> + trailers);
> + } else {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_addstr(&sb, arg);
> + strbuf_trim(&sb);
> + error(_("empty trailer token in trailer '%.*s'"),
> + (int) sb.len, sb.buf);
> + strbuf_release(&sb);
> + }
> +
> + free(conf_current);
> +
> return 0;
> }
>
> @@ -135,7 +148,7 @@ static void read_input_file(struct strbuf *sb, const char *file)
> }
>
> static void interpret_trailers(const struct process_trailer_options *opts,
> - struct list_head *new_trailer_head,
> + struct list_head *arg_trailers,
> const char *file)
> {
> LIST_HEAD(head);
> @@ -144,8 +157,6 @@ static void interpret_trailers(const struct process_trailer_options *opts,
> struct trailer_block *trailer_block;
> FILE *outfile = stdout;
>
> - trailer_config_init();
> -
> read_input_file(&sb, file);
>
> if (opts->in_place)
> @@ -162,12 +173,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>
>
> if (!opts->only_input) {
> - LIST_HEAD(config_head);
> - LIST_HEAD(arg_head);
> - parse_trailers_from_config(&config_head);
> - parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
> - list_splice(&config_head, &arg_head);
> - process_trailers_lists(&head, &arg_head);
> + process_trailers_lists(&head, arg_trailers);
> }
So, the bulk of the parsing is no longer responsibility of this
function. Instead, the caller (e.g., cmd_interpret_trailers()) is
expected to do that before they call us.
> ...
> git_config(git_default_config, NULL);
> + trailer_config_init();
> +
> + if (!opts.only_input) {
> + parse_trailers_from_config(&configured_trailers);
> + }
By the way, until we add more statements in this block, lose the
unnecessary {} around it.
> + /*
> + * In command-line arguments, '=' is accepted (in addition to the
> + * separators that are defined).
> + */
> + cl_separators = xstrfmt("=%s", trailer_default_separators());
>
> argc = parse_options(argc, argv, prefix, options,
> git_interpret_trailers_usage, 0);
>
> - if (opts.only_input && !list_empty(&trailers))
> + free(cl_separators);
> +
> + if (opts.only_input && !list_empty(&arg_trailers))
> usage_msg_opt(
> _("--trailer with --only-input does not make sense"),
> git_interpret_trailers_usage,
> options);
>
> + list_splice(&configured_trailers, &arg_trailers);
This corresponds to what the old code did in interpret_trailers(),
OK.
The move of the responsibility may make sense.
> if (argc) {
> int i;
> for (i = 0; i < argc; i++)
> - interpret_trailers(&opts, &trailers, argv[i]);
> + interpret_trailers(&opts, &arg_trailers, argv[i]);
> } else {
> if (opts.in_place)
> die(_("no input file given for in-place editing"));
> - interpret_trailers(&opts, &trailers, NULL);
> + interpret_trailers(&opts, &arg_trailers, NULL);
> }
>
> - free_new_trailers(&trailers);
> + free_new_trailers(&arg_trailers);
>
> return 0;
> }
> diff --git a/trailer.c b/trailer.c
> index c16f552b463..19637ff295d 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
>
> static char *separators = ":";
>
> +const char *trailer_default_separators(void)
> +{
> + return separators;
> +}
> +
> static int configured;
>
> #define TRAILER_ARG_STRING "$ARG"
> @@ -424,6 +429,25 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
> return 0;
> }
>
> +void trailer_conf_set(enum trailer_where where,
> + enum trailer_if_exists if_exists,
> + enum trailer_if_missing if_missing,
> + struct trailer_conf *conf)
> +{
> + if (where != WHERE_DEFAULT)
> + conf->where = where;
> + if (if_exists != EXISTS_DEFAULT)
> + conf->if_exists = if_exists;
> + if (if_missing != MISSING_DEFAULT)
> + conf->if_missing = if_missing;
> +}
So, this is such a helper function. It is curious that it
deliberately lacks the ability to reset what has already been
configured back to the default.
For example, we earlier saw this original code ...
> - item = xmalloc(sizeof(*item));
> - item->text = arg;
> - item->where = where;
> - item->if_exists = if_exists;
> - item->if_missing = if_missing;
... gets replaced with this call.
> + struct trailer_conf *conf_current = new_trailer_conf();
> ...
> + trailer_conf_set(where, if_exists, if_missing, conf_current);
For this conversion to be correct, we require that the value of the
*_DEFAULT macro to be 0 and/or these three values getting assigned
are not *_DEFAULT values. Maybe that may happen to be the case in
today's code, but such an unwritten assumption makes me feel nervous.
I am not sure if it is worth the confusion factor to make a function
whose name is $anything_set() to be making a conditional assignment.
If such a conditional assignment *also* happens often and deserves
to have its own helper, it is fine to have such a helper for
conditional assignment, but calling it $anything_set() is probably a
mistake.
Other than that, the main thrust of this step seems sensible.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 09/10] trailer: delete obsolete argument handling code from API
From: Junio C Hamano @ 2024-02-01 22:25 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <885ac87a5447e54139171fb3eda62055ffd517cd.1706664145.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> This commit was not squashed with its parent in order to keep the diff
> separate (to make the additive changes in the parent easier to read).
Hopefully we won't see such an artificial separation in the new
iteration. As we saw, being able to compare before and after images
in a single patch is useful while reviewing a change that is supposed
to be making things cleaner without altering what they do.
^ permalink raw reply
* Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests
From: Junio C Hamano @ 2024-02-01 22:26 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <ZbwV9iGsTWZ3ddfn@google.com>
Josh Steadmon <steadmon@google.com> writes:
> On 2024.01.16 15:18, Junio C Hamano wrote:
>> Josh Steadmon <steadmon@google.com> writes:
>> > With these changes, you can now use test-tool to run the unit tests:
>> > $ make
>> > $ cd t/unit-tests/bin
>> > $ ../../helper/test-tool run-command testsuite --no-run-in-shell \
>> > --no-require-shell-test-pattern
>>
>> This makes me wonder why we want to do the readdir() loop ourselves.
>> Instead of saying --no-require-shell-test-pattern there, wouldn't it
>> be simpler to say "*" right there, and have testsuite() run the test
>> programs named from the command line?
>
> It's speculation on my part, but I wonder if it has something to do with
> the number of shell tests? Google tells me that on Windows, the maximum
> command line length is 8191 characters. Which is actually a fair bit
> smaller than expanding the shell test list:
>
> $ echo t????-*.sh | wc -c
> 25714
OK.
^ permalink raw reply
* Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests
From: Josh Steadmon @ 2024-02-01 23:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <xmqqv87sx3y2.fsf@gitster.g>
On 2024.01.16 15:18, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> > Second "--(no-)require-shell-test-pattern" bypasses the check that the
> > test filenames match the expected t####-*.sh pattern.
>
> This one I am not so sure. Do we still have situations where
> erroring out when fed a non t[0-9][0-9][0-9][0-9]-*.sh script is
> problematic? IOW, do we need to keep it as conditional?
>
> ... goes and looks ...
>
> Ah, this variable/option is misnamed and that is what invited the
> above nonsense question out of me. The logic this option disables
> does not "require" (and error out if the requirement is not met); it
> is used in a loop over "ls *" and "filtering" files out that are not
> the numbered scripts.
>
> But that confusion makes me wonder if non-script side of tests would
> also want some filtering in the longer run, even if the directory we
> feed to "test-tool run" happens to contain nothing but what we want
> to run right now. I wonder if we instead want a variable that holds
> a pattern used to match programs readdir() discovers and skip those
> that do not match the pattern? Its default value may be something
> like "t[0-9][0-9][0-9][0-9]-*.sh" but alternatively you can give,
> say, "*" to pass everything, or something like that.
The original implementation and this series both still allow passing
additional patterns on the command line. Only tests matching the
hardcoded t[0-9][0-9][0-9][0-9]-*.sh pattern plus all the command-line
patterns will be run. The new flag just bypasses the hardcoded pattern.
Given that V2 will already be changing in non-backwards-compatible ways,
I think it makes the most sense to just delete the hardcoded pattern and
require callers to provide it if they want it.
^ permalink raw reply
* Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests
From: Josh Steadmon @ 2024-02-01 23:14 UTC (permalink / raw)
To: Junio C Hamano, git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <Zbwki_Dk2hGRce6Y@google.com>
On 2024.02.01 15:08, Josh Steadmon wrote:
> On 2024.01.16 15:18, Junio C Hamano wrote:
> > Josh Steadmon <steadmon@google.com> writes:
> > > Second "--(no-)require-shell-test-pattern" bypasses the check that the
> > > test filenames match the expected t####-*.sh pattern.
> >
> > This one I am not so sure. Do we still have situations where
> > erroring out when fed a non t[0-9][0-9][0-9][0-9]-*.sh script is
> > problematic? IOW, do we need to keep it as conditional?
> >
> > ... goes and looks ...
> >
> > Ah, this variable/option is misnamed and that is what invited the
> > above nonsense question out of me. The logic this option disables
> > does not "require" (and error out if the requirement is not met); it
> > is used in a loop over "ls *" and "filtering" files out that are not
> > the numbered scripts.
> >
> > But that confusion makes me wonder if non-script side of tests would
> > also want some filtering in the longer run, even if the directory we
> > feed to "test-tool run" happens to contain nothing but what we want
> > to run right now. I wonder if we instead want a variable that holds
> > a pattern used to match programs readdir() discovers and skip those
> > that do not match the pattern? Its default value may be something
> > like "t[0-9][0-9][0-9][0-9]-*.sh" but alternatively you can give,
> > say, "*" to pass everything, or something like that.
>
> The original implementation and this series both still allow passing
> additional patterns on the command line. Only tests matching the
> hardcoded t[0-9][0-9][0-9][0-9]-*.sh pattern plus all the command-line
> patterns will be run. The new flag just bypasses the hardcoded pattern.
Actually, I misspoke here. The test must match the hardcoded pattern
plus at least one of the command-line patterns (if any were provided).
> Given that V2 will already be changing in non-backwards-compatible ways,
> I think it makes the most sense to just delete the hardcoded pattern and
> require callers to provide it if they want it.
Given that it sounds like the testsuite functionality is not really used
at the moment, I still feel OK with changing the pattern matching logic
here for V2.
^ permalink raw reply
* [PATCH] imap-send: add missing "strbuf.h" include under NO_CURL
From: Philippe Blain via GitGitGadget @ 2024-02-02 0:18 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Christian Hesse, Taylor Blau, Philippe Blain,
Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
Building with NO_CURL is currently broken since imap-send.c uses things
defined in "strbuf.h" wihtout including it.
The inclusion of that header was removed in eea0e59ffb (treewide: remove
unnecessary includes in source files, 2023-12-23), which failed to
notice that "strbuf.h" was transitively included in imap-send.c via
"http.h", but only if USE_CURL_FOR_IMAP_SEND is defined. Add back the
missing include. Note that it was explicitely added in 3307f7dde2
(imap-send: include strbuf.h, 2023-05-17) after a similar breakage in
ba3d1c73da (treewide: remove unnecessary cache.h includes, 2023-02-24) -
see the thread starting at [1].
It can be verified by inspection that this is the only case where a
header we include is dependent on a Makefile knob in the files modified
in eea0e59ffb.
[1] https://lore.kernel.org/git/20230517070632.71884-1-list@eworm.de/
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
imap-send: add missing "strbuf.h" include under NO_CURL
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1664%2Fphil-blain%2Fimap-send-include-no-openssl-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1664/phil-blain/imap-send-include-no-openssl-v1
Pull-Request: https://github.com/git/git/pull/1664
imap-send.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/imap-send.c b/imap-send.c
index d662811ee83..f2e1947e638 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -28,6 +28,7 @@
#include "run-command.h"
#include "parse-options.h"
#include "setup.h"
+#include "strbuf.h"
#if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)
typedef void *SSL;
#endif
base-commit: bc7ee2e5e16f0d1e710ef8fab3db59ab11f2bbe7
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox