* Re: [PATCH v4 19/28] trailer: make trailer_info struct private
From: Linus Arver @ 2024-02-13 17:41 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD1twELGKvvesxgCrZrypKZpgSt04ira3mvurG1UbpDfxQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
>> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
>> index 11f4ce9e4a2..6bf8cec005a 100644
>> --- a/builtin/interpret-trailers.c
>> +++ b/builtin/interpret-trailers.c
>> @@ -141,7 +141,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>> LIST_HEAD(head);
>> struct strbuf sb = STRBUF_INIT;
>> struct strbuf trailer_block = STRBUF_INIT;
>> - struct trailer_info info;
>> + struct trailer_info *info;
>> FILE *outfile = stdout;
>>
>> trailer_config_init();
>> @@ -151,13 +151,13 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>> if (opts->in_place)
>> outfile = create_in_place_tempfile(file);
>>
>> - parse_trailers(opts, &info, sb.buf, &head);
>> + info = parse_trailers(opts, sb.buf, &head);
>
> I think this patch might be doing too much at once and could have been
> split into 3 or more patches to make reviews easier.
Ack, will do.
> For example the first patch could introduce trailer_info_new() and
> make interpret_trailers() use it. Then the second patch could modify
> parse_trailers() so that it returns a 'struct trailer_info *'. Then
> the third patch could make the trailer_info struct private.
Thanks, I will try to use this breakdown in the next reroll.
^ permalink raw reply
* Re: [PATCH v4 21/28] trailer: spread usage of "trailer_block" language
From: Linus Arver @ 2024-02-13 17:47 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD3ZRfrG4s5jox55dYMRF7UT3uYMkyMraEGWRJ2HqBEYZA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> 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.
>>
>> Signed-off-by: Linus Arver <linusa@google.com>
>> ---
>> builtin/interpret-trailers.c | 25 +++++-----
>> trailer.c | 97 ++++++++++++++++++------------------
>> trailer.h | 18 +++----
>> 3 files changed, 71 insertions(+), 69 deletions(-)
>>
>> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
>> index 6bf8cec005a..f76841c5280 100644
>> --- a/builtin/interpret-trailers.c
>> +++ b/builtin/interpret-trailers.c
>> @@ -140,8 +140,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>> {
>> LIST_HEAD(head);
>> struct strbuf sb = STRBUF_INIT;
>> - struct strbuf trailer_block = STRBUF_INIT;
>> - struct trailer_info *info;
>> + struct strbuf tb = STRBUF_INIT;
>> + struct trailer_block *trailer_block;
>
> I understand that using 'trailer_block' for a 'struct trailer_block *'
> makes sense and I like the idea behind this patch, but it's
> unfortunate that 'struct strbuf trailer_block' becomes 'struct strbuf
> tb'.
I confess I did not like the name "tb" either. It does have "symmetry in
naming with 'sb' above" going for it, but that symmetry is only on the
surface because the "b" in "sb" stands for "buf" (buffer),
not "block" as is the case for "tb".
Let me try to clean up the naming scheme a bit around here.
> Also the name change for 'struct strbuf trailer_block' could be
> in a separate patch.
Will do, thanks.
^ permalink raw reply
* Re: [GSOC][RFC] microproject: use test_path_is_* functions in test scripts
From: Junio C Hamano @ 2024-02-13 17:49 UTC (permalink / raw)
To: Vincenzo MEZZELA; +Cc: git
In-Reply-To: <cbb450b3-b333-4391-ac83-66c2daf0ae4d@gmail.com>
Vincenzo MEZZELA <vincenzo.mezzela@gmail.com> writes:
> Approach:
>
> As far as I understood, The work consists in replacing the shell
> 'test' command in the test
> script under 't/' directory with the ones present in the
> t/test_lib_functions.sh as follows:
>
> - test -f --> test_path_is_file
>
> - test -d --> test_path_is_dir
>
> - test -e --> test_path_exists
One thing to note is that you'd need to make sure if "test -e" for
example originally written really means "we want to see something
there and it does not matter if it is a file or a directory" while
turning it into test_path_exists. The operations that such a test
follows may be expected to create a file and never a directory, in
which case the condition the original code is testing may need to
be corrected first to expect a more specific type (e.g. "test -f").
The same comment applies for the other two.
Some tests check with "! test -f <path>", which often would want to
be turned into "test_path_is_missing", but you'd need to make sure
that is what the original test really meant to do.
A microproject is not about "doing the real work to help the
project". It is a practice session to come up with a set of small
changes and explain them well, to send the result to the list to get
reviewed, to respond to reviews and possibly send updates, and to
repeat the cycle until completion. So most likely you'd pick a
single file or two and do the above conversion, while leaving the
others as practice material for other GSoC participants.
Enjoy.
^ permalink raw reply
* Re: [PATCH v4 22/28] trailer: prepare to delete "parse_trailers_from_command_line_args()"
From: Linus Arver @ 2024-02-13 17:53 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD3pcyRYaJLBmiDdN3sdvRzo==dx-CQKoX1w1n6Hsg5+-g@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> Expose more functions in the trailer.h API, in preparation for deleting
>>
>> parse_trailers_from_command_line_args()
>>
>> from the trailers implementation, because it should not be concerned
>> with command line arguments (as they have nothing to do with trailers
>> themselves). Indeed, the interpret-trailers builtin is the only caller
>> of this function inside Git.
>>
>> Rename add_arg_item() to trailer_add_arg_item() to expose it as an API
>> function. 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".
>
> That's a lot done in a single patch. I think splitting it into 3 or
> more patches would be nice for reviewers.
Thank you for pointing this out (and, for everyone else reading, please
don't hesitate to ask for this from me); will do in the next reroll.
^ permalink raw reply
* Re: [PATCH v4 23/28] trailer: add new helper functions to API
From: Linus Arver @ 2024-02-13 17:57 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD0XjHtwB55XDajJia994TmUwub4L73QxhCQ8yTXc8++Ww@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> This is a preparatory refactor for deprecating "new_trailer_item" from
>> the API (which will let us deprecate
>> parse_trailers_from_command_line_args()).
>>
>> Expose new helper functions from the API, because we'll be calling them
>> from interpret-trailers.c soon when we move
>> parse_trailers_from_command_line_args() there.
>>
>> Move free_new_trailers() from the builtin to trailer.c because later on
>> we will adjust it to free arg_item structs, which are private to
>> trailer.c.
>
> This patch seems to be also doing too much.
I assume you mean that you'd like for the movement of
free_new_trailers() to be separated into its own patch, separate from
the introduction of new helper functions. I agree.
Will update.
^ permalink raw reply
* Re: [PATCH v4 26/28] trailer: unify "--trailer ..." arg handling
From: Linus Arver @ 2024-02-13 18:12 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD3u8qNpxObdOJDfBq+zVfxNwAG56bBcPeSC4i2=ZuhWjw@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> Move the logic of parse_trailer_from_command_line_arg() into
>> option_parse_trailer(), because that is the only caller and there's no
>> benefit in keeping these two separate.
>
> Well one benefit could be that 2 small functions might be easier to
> understand than a big one.
True.
> So perhaps
> parse_trailer_from_command_line_arg() could just have been made
> static?
In this case I don't think keeping these two functions separate would
make sense because parse_trailer_from_command_line_arg() is much more
heavyweight than option_parse_trailer() (one is just a thin wrapper
around the other). And I didn't like the thought of having 2 function
names that look very different:
parse_trailer_from_command_line_arg()
option_parse_trailer()
be so closely related in behavior.
And I already have some more patches (not in this series) that refactors
this area a bit also, so I wanted to wait until the dust settled down a
bit before deciding (esp. when unit tests are added) whether keep this
function separate. It's not clear to me yet whether we do want to add
unit tests for parse_trailer_from_command_line_arg() (if we do end up
"resurrecting it" in this patch or later), so IDK. The main reason is
because I think the first set of unit tests should be for the exposed
functions in <trailer.h>, not so much the helper functions that only the
builtin uses.
^ permalink raw reply
* Re: [PATCH v4 27/28] trailer_set_*(): put out parameter at the end
From: Linus Arver @ 2024-02-13 18:14 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD2-M20uuo4G8HykRL=1g9634wKUT9R6DBG6nmLBqGpxzw@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> The new trailer_config_set_*() functions which were introduced a few
>> patches ago put the out parameter (the variable being mutated) at the
>> end of the parameter list.
>>
>> Put the out parameter at the end for these functions for these existing
>
> s/for these functions for these/for the/
That wording is much better; will update.
>> trailer_set_*() functions for consistency. This also avoids confusion
>> for API users because otherwise these two sets of functions look rather
>> similar in <trailer.h> even though they have completely different out
>> parameters.
^ permalink raw reply
* Re: [PATCH v4 28/28] trailer: introduce "template" term for readability
From: Linus Arver @ 2024-02-13 18:20 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD01SPF8bqAb2Kn-q7SfX9cYiU_tpDSZz1CnEFbUqQpXYA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Linus Arver <linusa@google.com>
>>
>> The term "arg_item" is ambiguous because we use it to hold data for
>>
>> (1) trailers specified as command line arguments (in
>> builtin/interpret-trailers.c), and
>>
>> (2) trailers specified in configuration,
>>
>> and these are both used to ultimately insert new trailers (based
>> on the contents of arg_item, acting as a kind of template) into some
>> other set of existing trailers (such as those found in a trailer block
>> inside a log message) that have already been parsed.
>>
>> Rename "arg_item" to "trailer_template". This necessitates further
>> renames to make the functions that act on these templates match the data
>> structures (parameters) they act on:
>>
>> - [*] add_arg_to_input_list() to apply_template_to_trailers()
>> - [*] apply_arg_if_exists() to maybe_add_if_exists()
>> - [*] apply_arg_if_missing() to maybe_add_if_missing()
>> - apply_command() to run_command_from_template()
>> - [*] apply_item_command() to populate_template_value()
>> - free_arg_item() to free_template() (non-API function)
>> - free_new_trailers() to free_trailer_templates() (API function)
>> - get_conf_item() to get_or_add_template_by()
>> - option_parse_trailer() to option_parse_trailer_template()
>> - parse_trailers_from_config() to parse_trailer_templates_from_config()
>> - [*] process_trailers_lists() to apply_trailer_templates()
>> - token_from_item() to token_from_template()
>> - token_matches_item to token_matches_template
>> - [*] trailer_add_arg_item() to add_trailer_template()
>> - trailer_from_arg() to trailer_from()
>> - [*] check_if_different() (reorder parameters only)
>> - [*] find_same_and_apply_arg() (reorder parameters only)
>>
>> Reorder parameters to prefer input parameters toward the beginning and
>> out parameters at the end; these functions have been marked with an
>> asterisk ([*]).
>
> That's a lot of changes in a single patch.
I confess I was not happy with the volume of the change either. I
suppose I could break things down into renames vs parameter reorderings.
Will update.
>> This removes the "arg" terminology (standing for "CLI arguments") from
>> the trailer implementation, which makes sense because trailers
>> themselves have nothing to do with CLI argument handling.
>>
>> Also note that these renames expose the previously liberal use of
>> "trailer" to mean both trailers we read from the input text (trailer
>> block) and trailer templates that are defined as CLI args or
>> configurations. Some functions implied a single action when they could
>> do two different things, so introduce words like "maybe" and "or" to
>> make their behavior more explicit.
>>
>> In summary this patch renames and reorders parameters for readability,
>> without any behavioral change. We don't rename
>> find_same_and_apply_arg(), because it will be refactored soon.
>>
>> For parse_trailers_from_config() (renamed to
>> parse_trailer_templates_from_config()), add a NEEDSWORK discussion about
>> how the deprecated trailer.*.command configuration option is oddly more
>> featureful than trailer.*.cmd (if we were to remove support for
>> trailer.*.command, users would not be able to replicate the behavior
>> with trailer.*.cmd and would lose out on functionality).
>
> This change could be in a separate patch. Also there were discussions
> when trailer.*.command was deprecated and trailer.*.cmd introduced. I
> think it might be useful to talk about them in the commit message of
> the separate patch introducing the NEEDSWORK.
Will do (will reference the commit that introduced trailer.*.cmd also),
thanks.
^ permalink raw reply
* Re: [PATCH 00/12] The merge-base logic vs missing commit objects
From: Junio C Hamano @ 2024-02-13 18:37 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> I am deeply sorry both about the length of this patch series as well as
> having to send it in the -rc phase.
You do not have to ;-), but thanks.
Also outside the scope of -rc, there were some untied loose ends in
the missing tree thing before we can move to a new topic, if I
recall correctly.
Speaking of -rc, the project would be helped by your expertise a lot
if you gave a quick Ack on the make_relative() thing [*].
[Reference]
* f6628636 (unit-tests: do show relative file paths on non-Windows,
too, 2024-02-11)
https://lore.kernel.org/git/xmqqle7r9enn.fsf_-_@gitster.g/
^ permalink raw reply
* Re: [PATCH v3 2/2] column: guard against negative padding
From: Rubén Justo @ 2024-02-13 18:39 UTC (permalink / raw)
To: Junio C Hamano, Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <xmqqcyt08fa1.fsf@gitster.g>
On 13-feb-2024 09:06:46, Junio C Hamano wrote:
> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>
> > Make sure that client code can’t pass in a negative padding by accident.
> >
> > Suggested-by: Rubén Justo <rjusto@gmail.com>
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
> >
> > Notes (series):
> > Apparently these are the only publicly-visible functions that use this
> > struct according to `column.h`.
> >
> > column.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/column.c b/column.c
> > index ff2f0abf399..50bbccc92ee 100644
> > --- a/column.c
> > +++ b/column.c
> > @@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts,
> > {
> > struct column_options nopts;
> >
> > + if (opts && (0 > opts->padding))
> > + BUG("padding must be non-negative");
>
> The only two current callers may happen to be "git branch" that
> passes NULL as opts, and "git clean" that passes 2 in opts->padding,
> so this BUG() will not trigger. Once we add new callers to this
> function, or update the current callers, this safety start to matter.
>
> The actual breakage from a negative padding happens in layout(),
> so another option would be to have this guard there, which will
> protect us from having new callers of that function as well, or
> its caller display_table(), but these have only one caller each,
> so having the guard print_columns() here, that is the closest to
> the callers would be fine.
>
> > if (!list->nr)
> > return;
> > assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
> > @@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts)
> > {
> > struct strvec *argv;
> >
> > + if (opts && (0 > opts->padding))
> > + BUG("padding must be non-negative");
>
> This one happens to be safe currently because "git tag" passes 2 in
> opts->padding, but I do not think this is needed.
At first glance, I also thought this was not necessary.
However, callers of run_column_filter() might forget to check the return
value, and the BUG() triggered by the underlying process could be buried
and ignored. Having the BUG() here, in the same process, makes it more
noticeable.
Based on this, I'm not opposed to this change.
^ permalink raw reply
* Re: [PATCH v3 0/2] column: disallow negative padding
From: Rubén Justo @ 2024-02-13 19:27 UTC (permalink / raw)
To: Kristoffer Haugsbakk, git; +Cc: Tiago Pascoal, Chris Torek, Junio C Hamano
In-Reply-To: <cover.1707839454.git.code@khaugsbakk.name>
On 13-feb-2024 17:01:19, Kristoffer Haugsbakk wrote:
> The series gets split into two patches.
Very good.
>
> Cc: Tiago Pascoal <tiago@pascoal.net>
> Cc: Chris Torek <chris.torek@gmail.com>
> Cc: Junio C Hamano <gitster@pobox.com>
> Cc: Rubén Justo <rjusto@gmail.com>
>
> Kristoffer Haugsbakk (2):
> column: disallow negative padding
> column: guard against negative padding
>
> builtin/column.c | 2 ++
> column.c | 4 ++++
> t/t9002-column.sh | 11 +++++++++++
> 3 files changed, 17 insertions(+)
>
> Range-diff against v2:
> 1: 1c959378cf4 ! 1: 4cac42ca6f8 column: disallow negative padding
> @@ Commit message
> A negative padding does not make sense and can cause errors in the
> memory allocator since it’s interpreted as an unsigned integer.
>
> - Disallow negative padding. Also guard against negative padding in
> - `column.c` where it is conditionally used.
> -
> Reported-by: Tiago Pascoal <tiago@pascoal.net>
> - Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> -
> - ## Notes (series) ##
> - v2:
> - • Incorporate Junio’s changes (guard against negative padding in
> - `column.c`)
> - • Tweak commit message based on Junio’s analysis
> - • Use gettext for error message
> - • However I noticed that the “translation string” from `fast-import`
> - isn’t a translation string. So let’s invent a new one and use a
> - parameter so that it can be used elsewhere.
> - • Make a test
> -
> ## builtin/column.c ##
> @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix)
> memset(&copts, 0, sizeof(copts));
> @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix
> usage_with_options(builtin_column_usage, options);
> if (real_command || command) {
>
> - ## column.c ##
> -@@ column.c: void print_columns(const struct string_list *list, unsigned int colopts,
> - memset(&nopts, 0, sizeof(nopts));
> - nopts.indent = opts && opts->indent ? opts->indent : "";
> - nopts.nl = opts && opts->nl ? opts->nl : "\n";
> -- nopts.padding = opts ? opts->padding : 1;
> -+ nopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;
> - nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
> - if (!column_active(colopts)) {
> - display_plain(list, "", "\n");
> -@@ column.c: int run_column_filter(int colopts, const struct column_options *opts)
> - strvec_pushf(argv, "--width=%d", opts->width);
> - if (opts && opts->indent)
> - strvec_pushf(argv, "--indent=%s", opts->indent);
> -- if (opts && opts->padding)
> -+ if (opts && 0 <= opts->padding)
> - strvec_pushf(argv, "--padding=%d", opts->padding);
> -
> - fflush(stdout);
> -
> ## t/t9002-column.sh ##
> @@ t/t9002-column.sh: EOF
> test_cmp expected actual
> -: ----------- > 2: 9355fc98e3d column: guard against negative padding
> --
> 2.43.0
>
The BUG() in run_column_filter() may be questionable, but overall this
v3 LGTM.
Thanks Kristoffer for your work. And also thanks to Tiago for
reporting.
* P.D. *
Thinking about this in a more general way, I've found that this kind
of error has hit us several times:
- 953aa54e1a (pack-objects: clamp negative window size to 0, 2021-05-01)
- 6d52b6a5df (pack-objects: clamp negative depth to 0, 2021-05-01)
Maybe the source of this error is how easy is to forget that
OPT_INTEGER can accept negative values (after all, that's what an
integer is).
There are not many users of OPT_INTEGER, and a quick check gives me
the impression (maybe wrong...) that many of them do not expect
negative values.
Maybe we should consider having an OPT_INTEGER that fails if the
value supplied is negative. Ideally, some kind of opt-in machinery
could be desirable, I think, for example to include/exclude:
- negative values
- "0" ( may not be a desired value )
- "-1" ( may have some special meaning )
- ...
I'll leave the idea here, just in case it inspires someone. Thank
you.
^ permalink raw reply
* Re: [PATCH v3 2/2] column: guard against negative padding
From: Junio C Hamano @ 2024-02-13 19:39 UTC (permalink / raw)
To: Rubén Justo; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <69f60c3a-ff47-4cb9-a229-6c5a36e7d9fa@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
>> This one happens to be safe currently because "git tag" passes 2 in
>> opts->padding, but I do not think this is needed.
>
> At first glance, I also thought this was not necessary.
>
> However, callers of run_column_filter() might forget to check the return
> value, and the BUG() triggered by the underlying process could be buried
> and ignored. Having the BUG() here, in the same process, makes it more
> noticeable.
The point of BUG() is to help developers catch the silly breakage
before it excapes from the lab, and we can expect these careless
developers to ignore the return value. But "column --padding=-1"
invoked as a subprocess will show a human-readable error message
to such a developer, so it is less important than the BUG() in the
other place.
There is no black or white decision, but this one is much less
darker gray than the other one is.
^ permalink raw reply
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Linus Arver @ 2024-02-13 19:39 UTC (permalink / raw)
To: Christian Couder, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD3qLGua5PTA+i29N+yJH8iKVBPwUn=S606B2E+s959JFQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> This patch series is the first 10 patches of a larger cleanup/bugfix series
>> (henceforth "larger series") I've been working on.
>
> There are now 28 patches in this series.
>
> I took a look at all of them, and I think that this series should be
> split into 4 or more series.
This sounds fine to me. IIRC this means for the 2nd, 3rd+ series I have
to remember to say "this series builds on top of the other topic
branches '...'" in the cover letter. Now that I've written this out I
will hopefully not forget to do this...
Or, I suppose I could just introduce the 1st sub-series, wait for that
to get queued to next, then (re)introduce the 2nd sub-series, etc, in
order. Hmm. I think this will be simpler.
> For example perhaps one series until the "trailer: move
> interpret_trailers() to interpret-trailers.c" patch, then another one
> until "trailer: finish formatting unification". etc.
>
> Also I think it might be possible to avoid some test failures
> introduced by some patches. If it's not possible, I agree with Junio
> that it would be nice if the failing tests were changed to use
> 'test_expect_failure'.
As the breakages are deliberate, I will have to go with using
"test_expect_failure".
> Also it seems to me that many patches towards the end of this series
> should be split.
In hindsight, I fully agree.
Aside: I am delighted with the quality of reviews on this project. It's
not something I am used to, so please bear with me while I try to break
old habits. Thanks.
>> The main goal of this
>> series is to begin the process of "libifying" the trailer API. By "API" I
>> mean the interface exposed in trailer.h. The larger series brings a number
>> of additional cleanups (exposing and fixing some bugs along the way), and
>> builds on top of this series.
>
> [...]
>
>> With the libification-focused goals out of the way, let's turn to this patch
>> series in more detail.
>
> I like the goal of libifying Git the trailer API, and the way you want
> to do it seems reasonable to me.
>
> [...]
>
>> In summary this series breaks up "process_trailers()" into smaller pieces,
>> exposing many of the parts relevant to trailer-related processing in
>> trailer.h. This will force us to eventually introduce unit tests for these
>> API functions, but that is a good thing for API stability.
>
> I am a bit sad that this series doesn't introduce unit tests using the
> new test framework in C yet. I understand that this series is mostly a
> big refactoring and maybe it's better to introduce unit tests only
> when the refactoring is finished though.
This was my original goal as well, and still is.
> Anyway I hope the next series will introduce such tests.
I will see which API functions are stable enough, and add tests
accordingly (in a patch series sooner than later).
Probably the "biggest" (?) thing that is coming from the larger series
is the introduction of a complete separation between parsing (without
any modification of the input) and formatting. The parser/formatter is
a large chunk of the trailer implementation, so I would expect unit
tests for those bits to have to wait until those improvements are merged
into "next".
> [...]
>
>> Thanks to the aggressive refactoring in this series, I've been able to
>> identify and fix several bugs in our existing implementation. Those fixes
>> build on top of this series but were not included here, in order to keep
>> this series small. Below is a "shortlog" of those fixes I have locally:
>>
>> * "trailer: trailer replacement should not change its position" (If we
>> found a trailer we'd like to replace, preserve its position relative to
>> the other trailers found in the trailer block, instead of always moving
>> it to the beginning or end of the entire trailer block.)
>
> I believe there was a reason why it was done this way. I don't
> remember what it was though.
Noted. I'll see what I can find in our commit history.
>> * "interpret-trailers: preserve trailers coming from the input" (Sometimes,
>> the parsed trailers from the input will be formatted differently
>> depending on whether we provide --only-trailers or not. Make the trailers
>> that were not modified and which are coming directly from the input get
>> formatted the same way, regardless of this flag.)
>
> It could be a feature to be able to normalize trailers in a certain way.
True. But doing such normalization silently is undocumented behavior,
and we should provide explicit flags for this sort of thing. Adding such
flags might be the right thing to do (let's discuss more when this patch
gets proposed). FWIW the behavior I observed is that this normalization
only happens for *some* trailers that have configuration options, not
all trailers in the input. So it's a special kind of (limited)
normalization.
>> * "interpret-trailers: do not modify the input if NOP" (Refrain from
>> subtracting or adding a newline around the patch divider "---" if we are
>> not adding new trailers.)
>
> It could be a feature to be able to normalize this too.
OK, but it would again be undocumented behavior.
>> * "trailer formatter: split up format_trailer() monolith" (Fix a bug in
>> git-log where we still printed a blank newline even if we didn't want to
>> format anything.)
>
> I am not sure this is a bug fix either. It could perhaps be a normalization too.
See my comment above.
>> * "interpret-trailers: fail if given unrecognized arguments" (E.g., for
>> "--where", only accept recognized WHERE_* enum values. If we get
>> something unrecognized, fail with an error instead of silently doing
>> nothing. Ditto for "--if-exists" and "--if-missing".)
>
> It's possible that there was a reason why it was done this way.
>
> I think you might want to take a look at the discussions on the
> mailing list when "interpret-trailers" was developed. There were a lot
> of discussions over a long time, and there were a lot of requests and
> suggestions about what it should do.
I confess I haven't looked too deeply into the original threads
surrounding the introduction of "interpret-trailers". But all of the
changes which I categorize as bugfixes above have a theme of
undocumented modifications.
While working on this (and the larger) series around trailers, I only
looked into some (not all) of the discussions on the mailing list in
this area. Instead, I deferred to
Documentation/git-interpret-trailers.txt as the official (authoritative)
source of truth. This is partly why I first started out on this project
last year by making improvements to that doc. And, this is why seeing so
many edge cases where we perform such undocumented modifications smelled
more like bugs to me than features.
That being said, I cannot disagree with your position that the so-called
bugfixes I've reported above could be misguided (and should rather be
just updates to missing documentation). Let's not try to decide that
here, but instead later when I get to introduce those changes on the
list, one at a time. Thanks.
^ permalink raw reply
* Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`
From: Junio C Hamano @ 2024-02-13 19:42 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git, phillip.wood123, Jeff King
In-Reply-To: <ZcoTbRxIaGmTd4fJ@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> I wonder whether we can maybe consolidate the interface into one or
> maybe even two functions where the behaviour can be tweaked with a flag
> field. Something like `refname_is_valid()` with a bunch of flags:
>
> - REFNAME_ACCEPT_HEAD to accept "HEAD"
> - REFNAME_ACCEPT_PSEUDOREF to accept all of the refs ending with
> "_HEAD" or being one of the irregular pseudorefs.
> - REFNAME_ACCEPT_INVALID_BUT_SAFE to accept refnames which aren't
> valid, but which would pass `refname_is_safe()`.
I am certain we _can_, but it will take an actual patch to see if
such a refactoring makes the callers easier to follow, which is the
real test. FWIW, I am much less skeptical than hopeful in this
particular case.
^ permalink raw reply
* Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)
From: Junio C Hamano @ 2024-02-13 19:48 UTC (permalink / raw)
To: Phillip Wood
Cc: Xiaoguang WANG, Taylor Blau, Torsten Bögershausen,
Chandra Pratap, git
In-Reply-To: <xmqqle7o6zs8.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Unfortunately the minimum fix is already in 'next', so let me turn
> what you wrote into an update relative to that. I'll assume your
> patch in the discussion is signed-off already?
Nah, my mistake. The topic still is outside 'next', so I'll replace
it with the attached while queuing.
Thanks.
------- >8 ------------- >8 ------------- >8 -------
Subject: [PATCH] write-or-die: fix the polarity of GIT_FLUSH environment variable
When GIT_FLUSH is set to 1, true, on, yes, then we should disable
skip_stdout_flush, but the conversion somehow did the opposite.
With the understanding of the original motivation behind "skip" in
06f59e9f (Don't fflush(stdout) when it's not helpful, 2007-06-29),
we can sympathize with the current naming (we wanted to avoid
useless flushing of stdout by default, with an escape hatch to
always flush), but it is still not a good excuse.
Retire the "skip_stdout_flush" variable and replace it with "flush_stdout"
that tells if we do or do not want to run fflush().
Reported-by: Xiaoguang WANG <wxiaoguang@gmail.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
write-or-die.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/write-or-die.c b/write-or-die.c
index 3942152865..01a9a51fa2 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -18,20 +18,20 @@
*/
void maybe_flush_or_die(FILE *f, const char *desc)
{
- static int skip_stdout_flush = -1;
-
if (f == stdout) {
- if (skip_stdout_flush < 0) {
- skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
- if (skip_stdout_flush < 0) {
+ static int force_flush_stdout = -1;
+
+ if (force_flush_stdout < 0) {
+ force_flush_stdout = git_env_bool("GIT_FLUSH", -1);
+ if (force_flush_stdout < 0) {
struct stat st;
if (fstat(fileno(stdout), &st))
- skip_stdout_flush = 0;
+ force_flush_stdout = 1;
else
- skip_stdout_flush = S_ISREG(st.st_mode);
+ force_flush_stdout = !S_ISREG(st.st_mode);
}
}
- if (skip_stdout_flush && !ferror(f))
+ if (!force_flush_stdout && !ferror(f))
return;
}
if (fflush(f)) {
--
2.44.0-rc0-46-g2996f11c1d
^ permalink raw reply related
* Re: [PATCH v4 15/28] format_trailer_info(): avoid double-printing the separator
From: Linus Arver @ 2024-02-13 19:52 UTC (permalink / raw)
To: Christian Couder
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Junio C Hamano, Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <CAP8UFD0nmK4ZigW9LcWOr_POEX5LX7m+T=Jq9rK34YL5C6xatw@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Feb 13, 2024 at 6:21 PM Linus Arver <linusa@google.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> > On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
>> > <gitgitgadget@gmail.com> wrote:
>
>> > Also I wonder why it was not possible to modify format_trailer_info()
>> > like it is done in this patch before using it to replace
>> > format_trailers().
>>
>> The artificial organization apparent in this patch was deliberate, in
>> order to make it painfully obvious exactly what was being replaced and
>> how. See https://lore.kernel.org/git/xmqqjzno13ev.fsf@gitster.g/
>
> As for the previous patch, I would have thought that it would be
> better not to break the tests.
I could just squash these patches together to avoid breaking tests (and
also avoid doing the flipping of expect_success to expect_fail and back
again). I don't mind at all which way we go, but now that we have these
patches broken out I wonder if it's better to just keep them that way.
Junio, do you mind if I squash the relevant changes together into just
one patch? I'd like your input because you requested the current style
(modulo test breakages which was my error). Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/2] column: guard against negative padding
From: Rubén Justo @ 2024-02-13 19:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <xmqqle7o5f34.fsf@gitster.g>
On 13-feb-2024 11:39:11, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> >> This one happens to be safe currently because "git tag" passes 2 in
> >> opts->padding, but I do not think this is needed.
> >
> > At first glance, I also thought this was not necessary.
> >
> > However, callers of run_column_filter() might forget to check the return
> > value, and the BUG() triggered by the underlying process could be buried
> > and ignored. Having the BUG() here, in the same process, makes it more
> > noticeable.
>
> The point of BUG() is to help developers catch the silly breakage
> before it excapes from the lab, and we can expect these careless
> developers to ignore the return value. But "column --padding=-1"
> invoked as a subprocess will show a human-readable error message
> to such a developer, so it is less important than the BUG() in the
> other place.
>
> There is no black or white decision, but this one is much less
> darker gray than the other one is.
I've checked this, without that BUG(), and the result has not been
pretty:
diff --git a/builtin/tag.c b/builtin/tag.c
index 37473ac21f..e15dfa73d2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (column_active(colopts)) {
struct column_options copts;
memset(&copts, 0, sizeof(copts));
- copts.padding = 2;
+ copts.padding = -1;
run_column_filter(colopts, &copts);
}
filter.name_patterns = argv;
I can imagine a future change that opens that current "2" to the user.
And the possible report from a user who tries "-1" would not be easy.
But I agree with you, that BUG() does not leave a good taste in the
mouth.
Maybe we should refactor run_column_filter(), I don't know, but I think
that is outside of the scope of this series.
^ permalink raw reply related
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Junio C Hamano @ 2024-02-13 19:57 UTC (permalink / raw)
To: Linus Arver
Cc: Christian Couder, Linus Arver via GitGitGadget, git,
Christian Couder, Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <owlywmr89ms9.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> This sounds fine to me. IIRC this means for the 2nd, 3rd+ series I have
> to remember to say "this series builds on top of the other topic
> branches '...'" in the cover letter. Now that I've written this out I
> will hopefully not forget to do this...
>
> Or, I suppose I could just introduce the 1st sub-series, wait for that
> to get queued to next, then (re)introduce the 2nd sub-series, etc, in
> order. Hmm. I think this will be simpler.
FWIW, which was how the recent flurry of topics related to the
reftable backend were done by Patrick, which was quite nice. People
certainly need the feel of larger picture to get motivated to review
an earlier part of the series, but now that they saw the projected
end-game, splitting these into 4 series, each with materials in 7 or
so patches in v4, and presenting in not-so-quick succession one by
one, would make it less distracting and less daunting.
>> I am a bit sad that this series doesn't introduce unit tests using the
>> new test framework in C yet. I understand that this series is mostly a
>> big refactoring and maybe it's better to introduce unit tests only
>> when the refactoring is finished though.
>
> This was my original goal as well, and still is.
That's OK. If v4 were 40-patch series instead of 28, I am sure you
would have had the unit-test part near the end. So a bite sized
series of serieses may not show the unit-tests while the earlier
batches are still cooking, but as long as we all are aiming for that
same goal, we are fine. Let's help ourselves get there soon by
reviewing each other's patches ;-).
Thanks, both.
^ permalink raw reply
* Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too
From: Johannes Schindelin @ 2024-02-13 19:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git, Randall S. Becker
In-Reply-To: <xmqqle7r9enn.fsf_-_@gitster.g>
Hi Junio,
On Sun, 11 Feb 2024, Junio C Hamano wrote:
> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
> index 7bf9dfdb95..66d6980ffb 100644
> --- a/t/unit-tests/test-lib.c
> +++ b/t/unit-tests/test-lib.c
> @@ -21,12 +21,11 @@ static struct {
> .result = RESULT_NONE,
> };
>
> -#ifndef _MSC_VER
> -#define make_relative(location) location
> -#else
> /*
> * Visual C interpolates the absolute Windows path for `__FILE__`,
> * but we want to see relative paths, as verified by t0080.
> + * There are other compilers that do the same, and are not for
> + * Windows.
> */
> #include "dir.h"
>
> @@ -34,32 +33,66 @@ static const char *make_relative(const char *location)
> {
> static char prefix[] = __FILE__, buf[PATH_MAX], *p;
> static size_t prefix_len;
> + static int need_bs_to_fs = -1;
>
> - if (!prefix_len) {
> + /* one-time preparation */
> + if (need_bs_to_fs < 0) {
> size_t len = strlen(prefix);
> - const char *needle = "\\t\\unit-tests\\test-lib.c";
> + char needle[] = "t\\unit-tests\\test-lib.c";
> size_t needle_len = strlen(needle);
>
> - if (len < needle_len || strcmp(needle, prefix + len - needle_len))
> - die("unexpected suffix of '%s'", prefix);
> + if (len < needle_len)
> + die("unexpected prefix '%s'", prefix);
> +
> + /*
> + * The path could be relative (t/unit-tests/test-lib.c)
> + * or full (/home/user/git/t/unit-tests/test-lib.c).
> + * Check the slash between "t" and "unit-tests".
> + */
> + prefix_len = len - needle_len;
> + if (prefix[prefix_len + 1] == '/') {
> + /* Oh, we're not Windows */
> + for (size_t i = 0; i < needle_len; i++)
> + if (needle[i] == '\\')
> + needle[i] = '/';
This looks very similar to the `convert_slashes()` function that is
defined in `compat/mingw.h`. It might be a good opportunity to rename that
function and move it to `git-compat-util.h`, then use it here and once
more below at the end of `make_relative()`.
That's just a nit, though. The patch looks good to me.
Thanks,
Johannes
> + need_bs_to_fs = 0;
> + } else {
> + need_bs_to_fs = 1;
> + }
>
> - /* let it end in a directory separator */
> - prefix_len = len - needle_len + 1;
> + /*
> + * prefix_len == 0 if the compiler gives paths relative
> + * to the root of the working tree. Otherwise, we want
> + * to see that we did find the needle[] at a directory
> + * boundary. Again we rely on that needle[] begins with
> + * "t" followed by the directory separator.
> + */
> + if (fspathcmp(needle, prefix + prefix_len) ||
> + (prefix_len && prefix[prefix_len - 1] != needle[1]))
> + die("unexpected suffix of '%s'", prefix);
> }
>
> - /* Does it not start with the expected prefix? */
> - if (fspathncmp(location, prefix, prefix_len))
> + /*
> + * Does it not start with the expected prefix?
> + * Return it as-is without making it worse.
> + */
> + if (prefix_len && fspathncmp(location, prefix, prefix_len))
> return location;
>
> - strlcpy(buf, location + prefix_len, sizeof(buf));
> + /*
> + * If we do not need to munge directory separator, we can return
> + * the substring at the tail of the location.
> + */
> + if (!need_bs_to_fs)
> + return location + prefix_len;
> +
> /* convert backslashes to forward slashes */
> + strlcpy(buf, location + prefix_len, sizeof(buf));
> for (p = buf; *p; p++)
> if (*p == '\\')
> *p = '/';
> -
> return buf;
> }
> -#endif
>
> static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
> {
> --
> 2.44.0-rc0
>
>
>
^ permalink raw reply
* Re: [PATCH] git: --no-lazy-fetch option
From: Linus Arver @ 2024-02-13 20:23 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Christian Couder
In-Reply-To: <xmqq1q9mmtpw.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sometimes, especially during tests of low level machinery, it is
> handy to have a way to disable lazy fetching of objects. This
> allows us to say, for example, "git cat-file -e <object-name>", to
> see if the object is locally available.
Nit: perhaps s/locally/already locally/ is better?
^ permalink raw reply
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Christian Couder @ 2024-02-13 20:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <xmqqa5o46zla.fsf@gitster.g>
(Sorry for sending this previously to Junio only)
On Tue, Feb 13, 2024 at 6:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
> >> I presume that [01-09/28] would be the first part, nothing
> >> controversial and consisting of obvious clean-ups? I do not mind
> >> merging that part down to remove the future review load if everybody
> >> agrees.
> >
> > Yeah, patches [01-09/28] look good to me.
>
> I was hoping that you'll give us more details of what the other 3 or
> more you would envision the series to be, actually.
I think the next one could be [10-16/28], so until "trailer: finish
formatting unification".
Then I am not sure about the next one, perhaps [17-20/28] or [17-21/28].
The rest would depend on the splitting of the big patches towards the
end of the series.
^ permalink raw reply
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Kristoffer Haugsbakk @ 2024-02-13 20:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, Josh Soref, git, Christian Couder,
Emily Shaffer, Josh Steadmon, rsbecker, Linus Arver
In-Reply-To: <xmqq5xys5e98.fsf@gitster.g>
On Tue, Feb 13, 2024, at 20:57, Junio C Hamano wrote:
> Linus Arver <linusa@google.com> writes:
>
>> This sounds fine to me. IIRC this means for the 2nd, 3rd+ series I have
>> to remember to say "this series builds on top of the other topic
>> branches '...'" in the cover letter. Now that I've written this out I
>> will hopefully not forget to do this...
>>
>> Or, I suppose I could just introduce the 1st sub-series, wait for that
>> to get queued to next, then (re)introduce the 2nd sub-series, etc, in
>> order. Hmm. I think this will be simpler.
>
> FWIW, which was how the recent flurry of topics related to the
> reftable backend were done by Patrick, which was quite nice. People
> certainly need the feel of larger picture to get motivated to review
> an earlier part of the series, but now that they saw the projected
> end-game, splitting these into 4 series, each with materials in 7 or
> so patches in v4, and presenting in not-so-quick succession one by
> one, would make it less distracting and less daunting.
I feel like I’m learning some things about how version control programs
(and accompanying review software (here email)) can be vital—not just
helpful, not just a nice-to-have—for structuring reviews of large
changes from observing these conversations. Thanks to both/all.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3 0/2] column: disallow negative padding
From: Kristoffer Haugsbakk @ 2024-02-13 20:32 UTC (permalink / raw)
To: Rubén Justo; +Cc: Tiago Pascoal, Chris Torek, Junio C Hamano, git
In-Reply-To: <48b96426-9231-4e80-b55d-628dd8847337@gmail.com>
On Tue, Feb 13, 2024, at 20:27, Rubén Justo wrote:
> * P.D. *
>
> Thinking about this in a more general way, I've found that this kind
> of error has hit us several times:
>
> - 953aa54e1a (pack-objects: clamp negative window size to 0, 2021-05-01)
> - 6d52b6a5df (pack-objects: clamp negative depth to 0, 2021-05-01)
>
> Maybe the source of this error is how easy is to forget that
> OPT_INTEGER can accept negative values (after all, that's what an
> integer is).
>
> There are not many users of OPT_INTEGER, and a quick check gives me
> the impression (maybe wrong...) that many of them do not expect
> negative values.
>
> Maybe we should consider having an OPT_INTEGER that fails if the
> value supplied is negative. Ideally, some kind of opt-in machinery
> could be desirable, I think, for example to include/exclude:
>
> - negative values
> - "0" ( may not be a desired value )
> - "-1" ( may have some special meaning )
> - ...
>
> I'll leave the idea here, just in case it inspires someone. Thank
> you.
Thanks to both for providing a wider perspective on guarding against
such bugs.
And this is an excellent point. I don’t know anything about the opt-args
implementation but it would be great to guard against user-supplied
values through the option parsing library.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3 2/2] column: guard against negative padding
From: Kristoffer Haugsbakk @ 2024-02-13 20:35 UTC (permalink / raw)
To: Rubén Justo; +Cc: git, Junio C Hamano
In-Reply-To: <b9ca1ab7-f8f6-4fe0-885a-51728d9ec708@gmail.com>
On Tue, Feb 13, 2024, at 20:56, Rubén Justo wrote:
> On 13-feb-2024 11:39:11, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>> The point of BUG() is to help developers catch the silly breakage
>> before it excapes from the lab, and we can expect these careless
>> developers to ignore the return value. But "column --padding=-1"
>> invoked as a subprocess will show a human-readable error message
>> to such a developer, so it is less important than the BUG() in the
>> other place.
>>
>> There is no black or white decision, but this one is much less
>> darker gray than the other one is.
>
> I've checked this, without that BUG(), and the result has not been
> pretty:
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 37473ac21f..e15dfa73d2 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> if (column_active(colopts)) {
> struct column_options copts;
> memset(&copts, 0, sizeof(copts));
> - copts.padding = 2;
> + copts.padding = -1;
> run_column_filter(colopts, &copts);
> }
> filter.name_patterns = argv;
>
> I can imagine a future change that opens that current "2" to the user.
> And the possible report from a user who tries "-1" would not be easy.
>
> But I agree with you, that BUG() does not leave a good taste in the
> mouth.
>
> Maybe we should refactor run_column_filter(), I don't know, but I think
> that is outside of the scope of this series.
Thanks for trying that out—some very topical testing!
I will take the night to think about v4. But I will defer to the
reviewers’ judgement on the scope of this series/change.
(All I know is that it can be tricky balancing such defensive checks
with readability and maintanability.)
Thanks
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] git: --no-lazy-fetch option
From: Linus Arver @ 2024-02-13 20:37 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Christian Couder
In-Reply-To: <owlyr0hg9kr3.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sometimes, especially during tests of low level machinery, it is
>> handy to have a way to disable lazy fetching of objects. This
>> allows us to say, for example, "git cat-file -e <object-name>", to
>> see if the object is locally available.
>
> Nit: perhaps s/locally/already locally/ is better?
I forgot to mention that the new flag is missing a documentation entry
in Documentation/git.txt. Perhaps something like
--no-lazy-fetch::
Do not fetch missing objects. Useful together with `git cat-file -e
<object-name>`, for example, to see if the object is already
locally available.
?
^ 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