* RE: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: rsbecker @ 2024-02-13 14:36 UTC (permalink / raw)
To: 'Jeff King', 'Junio C Hamano'
Cc: 'Josh Steadmon', git, johannes.schindelin, phillip.wood
In-Reply-To: <20240213074118.GA2225494@coredump.intra.peff.net>
On Tuesday, February 13, 2024 2:41 AM, Peff wrote:
>On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
>
>> Josh Steadmon <steadmon@google.com> writes:
>>
>> > I see this line in the docs [1]: "As with wildcard expansion in
>> > rules, the results of the wildcard function are sorted". GNU Make
>> > has restored the sorted behavior of $(wildcard) since 2018 [2]. I'll
>> > leave the sort off for now, but if folks feel like we need to
>> > support older versions of `make`, I'll add it back.
>> >
>> > [1]
>> > https://www.gnu.org/software/make/manual/html_node/Wildcard-Function
>> > .html [2] https://savannah.gnu.org/bugs/index.php?52076
>>
>> Thanks for digging. I thought I was certain that woldcard is sorted
>> and stable and was quite perplexed when I could not find the mention
>> in a version of doc I had handy ("""This is Edition 0.75, last updated
>> 19 January 2020, of 'The GNU Make Manual', for GNU 'make'
>> version 4.3.""").
>
>Likewise (mine is the latest version in Debian unstable). The change to sort comes
>from their[1] eedea52a, which was in GNU make 4.2.90. But the matching
>documentation change didn't happen until 5b993ae, which was
>4.3.90 in late 2021. So that explains the mystery.
>
>Those dates imply to me that we should keep the $(sort), though. Six years is not so
>long in distro timescales, especially given that Debian unstable is on a 4-year-old
>version. (And if we did want to get rid of it, certainly we should do so consistently
>across the Makefile in a separate patch).
I am stuck on 4.2.1 and cannot get to 4.3.90 any time soon. Can you want on this? It will take us out unless we can suppress the $(sort)
Sincerely,
Randall
^ permalink raw reply
* [GSOC][RFC] microproject: use test_path_is_* functions in test scripts
From: Vincenzo MEZZELA @ 2024-02-13 15:44 UTC (permalink / raw)
To: git
Hello everyone,
I'm Vincenzo, a Master's degree student in Computer Engineering and
Cybersecurity.
As I'm approaching the end of my academic journey, I'm eager to
contribute to the Git project
and the GSoC represents a good opportunity to do so. Upon exploring the
online documentation
of git for the application to the GSoC I'm keen to begin the required
microproject.
Among the microproject proposed here
https://git.github.io/SoC-2024-Microprojects/ , I would like to
work on replacing 'test -(e|f|g|...)' with test_path_is* .
Can you confirm if this task has already been taken by someone else?
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
To approximately measure the number of required replacement, I run the
following commands from the
't/' directory (branch master):
> # Files that requires a replacement
> git grep -r 'test -[efdx]' 2>/dev/null| awk '{print $1}' | uniq -c |
sort -n -r
>> 190 t7301-clean-interactive.sh:
>> 147 t7300-clean.sh:
>> 21 t2004-checkout-cache-temp.sh:
>> 17 t2401-worktree-prune.sh:
>> 16 t2003-checkout-cache-mkdir.sh:
>> 14 t0601-reffiles-pack-refs.sh:
>> 13 t4200-rerere.sh:
>> 12 t9146-git-svn-empty-dirs.sh:
>> 12 t7603-merge-reduce-heads.sh:
>> 12 lib-submodule-update.sh:
>> ...
>>
> # Number of replacements
> git grep -r 'test -[efdx]' 2>/dev/null| awk '{print $1}' | uniq -c |
sort -n -r | awk '{sum += $1} END {print sum}'
>> 853
> # Number of files that requires a patch
> git grep -r 'test -[efdx]' 2>/dev/null| awk '{print $1}' | uniq -c |
wc -l
>> 169
Although the replacement work might not be difficult, it spans over many
different test files.
Do you want me to submit a patch for each of them as part of the
microproject?
If not, How many patches do you expect me to submit?
Thanks,
Vincenzo
^ permalink raw reply
* Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`
From: Karthik Nayak @ 2024-02-13 15:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster, phillip.wood123, Jeff King
In-Reply-To: <ZcoTbRxIaGmTd4fJ@tanuki>
[-- Attachment #1: Type: text/plain, Size: 3851 bytes --]
Hello,
Patrick Steinhardt <ps@pks.im> writes:
>> +
>> +int is_headref(struct ref_store *refs, const char *refname)
>> +{
>> + if (!strcmp(refname, "HEAD"))
>> + return refs_ref_exists(refs, refname);
>> +
>> + return 0;
>> +}
>
> The same comment applies here, as well.
>
> I also worry a bit about the API we have. It becomes really hard to
> figure out which function to call now as the API surface seems to
> explode. We have:
>
> - is_pseudoref_syntax
> - is_pseudoref
> - is_headref
> - check_refname_format
> - refname_is_safe
>
I also found `is_head()` in 'reflog.c'.
> 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.
>
You do bring up an interesting point about the APIs present and I agree
that it would be best to consolidate them to something much simpler and
nicer.
> 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()`.
>
> Another alternative could be something like `classify_refname()` that
> accepts a refname and returns an enum saying what kind of ref something
> is.
>
> Given that this topic won't be included in Git v2.44 anymore, I think
> that opening this can of worms would be sensible now.
>
Over the two I do prefer something like the former method of the callee
using flags to state the requirements, this way the function only does
what is necessary between the listed operations.
Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> I think it's quite confusing that `is_pseudoref()` not only checks
>> whether the refname may be a pseudoref, but also whether it actually
>> exists. Furthermore, why is a pseudoref only considered to exist in case
>> it's not a symbolic ref? That sounds overly restrictive to me.
>
> I am torn on this, but in favor of the proposed naming, primarily
> because is_pseudoref_syntax() was about "does this string look like
> the fullref a pseudoref would have?", and the reason why we wanted
> to have this new function was we wanted to ask "does this string
> name a valid pseudoref?"
>
> Q: Is CHERRY_PICK_HEAD a pseudoref?
> A: It would have been if it existed, but I see only
> $GIT_DIR/CHERRY_PICK_HEAD that is a symbolic link, and it cannot
> be a pseudoref.
>
> I can certainly see a broken out set of helper functions to check
>
> - Does this string make a good fullref for a pseudoref?
> - Does a pseudoref with his string as its fullref exist?
>
> independently. The first one would answer Yes and the second one
> would answer No in such a context.
>
This does work into the flags based mechanism that Patrick mentioned
too. The users of this new function can then only request information as
necessary.
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> + if (ends_with(refname, "_HEAD")) {
>> + refs_resolve_ref_unsafe(refs, refname,
>> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>> + &oid, NULL);
>> + return !is_null_oid(&oid);
>> + }
>
> FYI. I see
>
> .git/rebase-apply/patch:31: space before tab in indent.
> RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> .git/rebase-apply/patch:32: space before tab in indent.
> &oid, NULL);
> .git/rebase-apply/patch:33: space before tab in indent.
> return !is_null_oid(&oid);
>
> around here.
Thanks for notifying, Jeff mentioned the same and I thought I fixed it.
I'll have a look at why my editor did this and add some steps to avoid
this in the following patches.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [PATCH v3 0/2] column: disallow negative padding
From: Kristoffer Haugsbakk @ 2024-02-13 16:01 UTC (permalink / raw)
To: git
Cc: Kristoffer Haugsbakk, Tiago Pascoal, Chris Torek, Junio C Hamano,
Rubén Justo
In-Reply-To: <1c959378cf495d7a3d70d0c7bdf08cc501ed6e5d.1707679627.git.code@khaugsbakk.name>
Fix bug in git-column(1): a user can pass a negative `padding` which
causes issues inside the memory allocator.
§ Changes in v3
Incorporate Ruben’s suggestion about guarding against negative padding
with `BUG` in `column.c` (not `builtin/column.c`). This then supersedes
Junio’s extra conditional checks since they are no longer needed. The
series gets split into two patches.
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
^ permalink raw reply
* [PATCH v3 1/2] column: disallow negative padding
From: Kristoffer Haugsbakk @ 2024-02-13 16:01 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Tiago Pascoal
In-Reply-To: <cover.1707839454.git.code@khaugsbakk.name>
A negative padding does not make sense and can cause errors in the
memory allocator since it’s interpreted as an unsigned integer.
Reported-by: Tiago Pascoal <tiago@pascoal.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
builtin/column.c | 2 ++
t/t9002-column.sh | 11 +++++++++++
2 files changed, 13 insertions(+)
diff --git a/builtin/column.c b/builtin/column.c
index e80218f81f9..10ff7e01668 100644
--- a/builtin/column.c
+++ b/builtin/column.c
@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)
memset(&copts, 0, sizeof(copts));
copts.padding = 1;
argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);
+ if (copts.padding < 0)
+ die(_("%s must be non-negative"), "--padding");
if (argc)
usage_with_options(builtin_column_usage, options);
if (real_command || command) {
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 348cc406582..d5b98e615bc 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -196,4 +196,15 @@ EOF
test_cmp expected actual
'
+test_expect_success 'padding must be non-negative' '
+ cat >input <<\EOF &&
+1 2 3 4 5 6
+EOF
+ cat >expected <<\EOF &&
+fatal: --padding must be non-negative
+EOF
+ test_must_fail git column --mode=column --padding=-1 <input >actual 2>&1 &&
+ test_cmp expected actual
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* [PATCH v3 2/2] column: guard against negative padding
From: Kristoffer Haugsbakk @ 2024-02-13 16:01 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Rubén Justo
In-Reply-To: <cover.1707839454.git.code@khaugsbakk.name>
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");
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");
if (fd_out != -1)
return -1;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4 10/28] format_trailer_info(): use trailer_item objects
From: Linus Arver @ 2024-02-13 16:35 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: <xmqq34u1e22y.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 is another preparatory refactor to unify the trailer formatters.
>>
>> Make format_trailer_info() operate on trailer_item objects, not the raw
>> string array.
>>
>> This breaks t4205 and t6300. We will continue to make improvements until
>> the test suite passes again, ultimately renaming format_trailer_info()
>> to format_trailers(), at which point the unification of these formatters
>> will be complete.
>>
>> Signed-off-by: Linus Arver <linusa@google.com>
>> ---
>> trailer.c | 21 ++++++++++-----------
>> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> I would have expected a (tentative) flip from test_expect_success to
> test_expect_failure in the affected tests, to illustrate what behaviour
> change this step introduces and why.
Somehow, such a simple idea did not cross my mind (even though I admit I
didn't like how I was breaking tests, albeit temporarily). Will update
on next reroll.
> But the huge single step in the previous round broken out into
> smaller steps like this round makes them much easier to follow.
Great, I'm doing something right for once! ;)
^ permalink raw reply
* Re: [PATCH v4 12/28] format_trailer_info(): append newline for non-trailer lines
From: Linus Arver @ 2024-02-13 16:49 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: <CAP8UFD1YaEfdM=apQ7An+GcKV+Wc26StsMqJcd9e5tHJg9U_hQ@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:
>
> [...]
>
>> The test suite passes again, so format_trailer_info() is in better shape
>> supersede format_trailers(), which we'll do in the next patch.
>
> s/supersede/to supersede/
Nice catch! Will update for next reroll, thanks.
^ permalink raw reply
* Re: libification: how to avoid symbol collisions?
From: Junio C Hamano @ 2024-02-13 16:50 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git
In-Reply-To: <CAO_smVizKLL2NHFBpszJn+ieJhCEZyvvOT-BWv6Oz5pGiafPVg@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
> struct/enum/typedef/union names. That's going to be quite annoying to
> maintain; even if we don't end up having to do all 3,500 symbols, for
> the files that are part of some public library, we'd add maintenance
> burden because we'd need to remember to either make every new function
> be static, or add it to this list. I assume we could create a test
> that would enforce this ("static, named with <prefix>, or added to
> <list>"), so the issue is catchable, but it will be exceedingly
> annoying every time one encounters this.
No matter how we do this, we'd need to maintain that list, so the
choices are between "#define" and "objcopy --redefine-sym" if we
limit ourselves to static linking, I think. The former may be more
portable but makes me feel dirty. The debuggers will not see the
names we want to use, for one thing. "rename selected symbols in *.o
files" approach, if it can be done on platforms we want the lib thing
on, would be more preferable.
We also should be able to create a single linkable object (e.g., Z.o
out of X.o and Y.o from the previous example---it could be Z.so that
is dynamically linked at runtime) to resolve the symbols that need
to be shared only among the object files (like "foo" that is defined
in X.o whose address is needed in Y.o) and after such a linking is
done, these "internal" symbols can be stripped away from the
resulting object file. For that, we'd also need to maintain that
list of internal symbols that are needed in order to link our
objects together.
^ permalink raw reply
* Re: [PATCH v4 14/28] format_trailer_info(): teach it about opts->trim_empty
From: Linus Arver @ 2024-02-13 17:05 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: <CAP8UFD3wMD5SmfpP20jST4YndBdPX9U9qAbKh=4TnUzLkpULRA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
>> diff --git a/trailer.c b/trailer.c
>> index f4defad3dae..c28b6c11cc5 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -162,20 +162,6 @@ static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
>> strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
>> }
>>
>> -static void format_trailers(const struct process_trailer_options *opts,
>> - struct list_head *trailers,
>> - struct strbuf *out)
>> -{
>> - struct list_head *pos;
>> - struct trailer_item *item;
>> - list_for_each(pos, trailers) {
>> - item = list_entry(pos, struct trailer_item, list);
>> - if ((!opts->trim_empty || strlen(item->value) > 0) &&
>> - (!opts->only_trailers || item->token))
>> - print_tok_val(out, item->token, item->value);
>> - }
>> -}
>
> It seems to me that this function could and should have been removed
> in the previous patch. If there is a reason why it is better to do it
> in this patch, I think it should be explained more clearly in the
> commit message.
Ah yes, the decision to delay the deletion like this was deliberate.
Will update the commit message to add something like:
Although we could have deleted format_trailers() in the previous patch,
we perform the deletion here like this in order to isolate
(and highlight) in this patch the salvaging of the logic in the deleted
code
if ((!opts->trim_empty || strlen(item->value) > 0) && ...)
print_tok_val(...)
as
if (opts->trim_empty && !strlen(item->value))
continue;
in the new code, which has the same effect (because we are skipping the
formatting in the new code).
>> static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
>> {
>> struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
>> @@ -1101,6 +1087,15 @@ void format_trailer_info(const struct process_trailer_options *opts,
>> strbuf_addstr(&tok, item->token);
>> strbuf_addstr(&val, item->value);
>>
>> + /*
>> + * Skip key/value pairs where the value was empty. This
>> + * can happen from trailers specified without a
>> + * separator, like `--trailer "Reviewed-by"` (no
>> + * corresponding value).
>> + */
>> + if (opts->trim_empty && !strlen(item->value))
>> + continue;
>> +
>
> Wasn't it possible to make this change in format_trailer_info() before
> using format_trailer_info() to replace format_trailers()?
It was certainly possible, but the choice to purposely time the
addition/deletion of code like this was deliberate (see my comment
above).
>> if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
>> if (opts->separator && out->len != origlen)
>> strbuf_addbuf(out, opts->separator);
^ permalink raw reply
* Re: [PATCH v3 2/2] column: guard against negative padding
From: Junio C Hamano @ 2024-02-13 17:06 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Rubén Justo
In-Reply-To: <9355fc98e3dac5768ecaf9e179be2f7a0e74d633.1707839454.git.code@khaugsbakk.name>
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.
We will pass these through to "git column" and the negative padding
will be caught as an error there anyway, no? So whether "git tag"
is updated or a new caller of run_column_filter() is added, the
developer will already notice it (and they will have to protect
themselves just like the [1/2] of your series did for "git column"
itself).
> if (fd_out != -1)
> return -1;
^ permalink raw reply
* Re: [PATCH v3 0/4] completion: remove hardcoded config variable names
From: Junio C Hamano @ 2024-02-13 17:09 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Philippe Blain via GitGitGadget, git, Philippe Blain
In-Reply-To: <Zcs34kGTqTbIana6@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> On Sat, Feb 10, 2024 at 06:32:19PM +0000, Philippe Blain via GitGitGadget wrote:
>> Changes since v2:
>>
>> * Moved the addition of the tests to 2/4, and tweaked 3/4 and 4/4 so they
>> simply adjust the test names
>> * Added a test for user-defined submodule names, as suggested by Patrick
>> * Added more details in the commit message of 3/4 around the use of global
>> variables as caches
>> * Slightly improved commit message wording and fixed typos
>> * Added 'local' where suggested
>> * Dropped 4/5 which modified 'git help', since it's not needed (thanks
>> Patrick!)
>>
>> Changes since v1:
>>
>> * Corrected my email in PATCH 2/5 (sorry for the noise)
>>
>> v1: This series removes hardcoded config variable names in the
>> __git_complete_config_variable_name function, partly by adding a new mode to
>> 'git help'. It also adds completion for 'submodule.*' config variables,
>> which were previously missing.
>>
>> I think it makes sense to do that in the same series since it's closely
>> related, and splitting it would result in textual conflicts between both
>> series if one does not build on top of the other, but I'm open to other
>> suggestions.
>>
>> Thanks,
>>
>> Philippe.
>
> I ain't got anything else to add to this patch series. Thanks!
Thanks, both. Let's mark it for 'next' so that it can be one of the
topics to graduate first after the current cycle.
^ permalink raw reply
* Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too
From: Junio C Hamano @ 2024-02-13 17:10 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Johannes Schindelin, Randall S. Becker
In-Reply-To: <fd8874bb-8e45-4b39-986c-5a96ccf0747f@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I think it was only the MSVC that needed the paths munging which we
> don't test by default.
Ah, my sillies.
> I have tweaked our github actions to run those
> tests and they pass
> https://github.com/phillipwood/git/actions/runs/7885144920/job/21515922057#step:5:146
Thanks.
^ permalink raw reply
* Re: [PATCH v4 15/28] format_trailer_info(): avoid double-printing the separator
From: Linus Arver @ 2024-02-13 17:21 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: <CAP8UFD3u+QDx2LqpO2ZpeHQszwjMAsQ90qqbE7Om=t1vPRQ==w@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>
>>
>> Do not hardcode the printing of ": " as the separator and space (which
>> can result in double-printing these characters); instead only
>> print the separator and space if we cannot find any recognized separator
>> somewhere in the key (yes, keys may have a trailing separator in it ---
>> we will eventually fix this design but not now). Do so by copying the
>> code out of print_tok_val(), and deleting the same function.
>>
>> The test suite passes again with this change.
>
> I think it should be clearer above that this fixes a bug that was
> introduced earlier in the series.
Ack, will add something like
This double printing is from a bug introduced earlier when we
started using format_trailer_info() everywhere.
to this patch's description, but also add explicit language in
"trailer: begin formatting unification" to say that the change is
introducing temporary bugs (and that this is why the tests break).
> 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/
^ permalink raw reply
* Re: [PATCH v4 14/28] format_trailer_info(): teach it about opts->trim_empty
From: Christian Couder @ 2024-02-13 17:21 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Junio C Hamano, Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <owlyttmcb8ge.fsf@fine.c.googlers.com>
On Tue, Feb 13, 2024 at 6:05 PM Linus Arver <linusa@google.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
> >> static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
> >> {
> >> struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
> >> @@ -1101,6 +1087,15 @@ void format_trailer_info(const struct process_trailer_options *opts,
> >> strbuf_addstr(&tok, item->token);
> >> strbuf_addstr(&val, item->value);
> >>
> >> + /*
> >> + * Skip key/value pairs where the value was empty. This
> >> + * can happen from trailers specified without a
> >> + * separator, like `--trailer "Reviewed-by"` (no
> >> + * corresponding value).
> >> + */
> >> + if (opts->trim_empty && !strlen(item->value))
> >> + continue;
> >> +
> >
> > Wasn't it possible to make this change in format_trailer_info() before
> > using format_trailer_info() to replace format_trailers()?
>
> It was certainly possible, but the choice to purposely time the
> addition/deletion of code like this was deliberate (see my comment
> above).
I would have thought that it would be better to make this change
earlier to avoid breaking tests.
> >> if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
> >> if (opts->separator && out->len != origlen)
> >> strbuf_addbuf(out, opts->separator);
^ permalink raw reply
* Re: [PATCH v4 15/28] format_trailer_info(): avoid double-printing the separator
From: Christian Couder @ 2024-02-13 17:25 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Junio C Hamano, Emily Shaffer, Josh Steadmon, Randall S. Becker
In-Reply-To: <owlyr0hgb7qg.fsf@fine.c.googlers.com>
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.
^ permalink raw reply
* Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)
From: Junio C Hamano @ 2024-02-13 17:26 UTC (permalink / raw)
To: Phillip Wood
Cc: Xiaoguang WANG, Taylor Blau, Torsten Bögershausen,
Chandra Pratap, git
In-Reply-To: <c243c260-b346-4b53-b8a2-685389ad344e@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Given we're in a rc-period a minimal fix like this looks appropriate
> (though it is missing some braces according to our coding
> guidelines). The interaction of "skip_stdout_flush" and git_env_bool()
> is unfortunate, It might be clearer if we changed to having
> "force_stdout_flush" instead but that would be a more invasive change.
I admit that I did find the polarity of the existing variable
annoying, and it does make sense to flip it like you did here.
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?
------- >8 ------------- >8 ------------- >8 -------
From: Phillip Wood <phillip.wood123@gmail.com>
Subject: maybe_flush_or_die(): flip the polarity of an internal variable
We take GIT_FLUSH that tells us if we want to flush (or not) from
the outside, but internally use a variable that tells us to skip (or
not) the flushing operation, which makes the code flow unnecessarily
confusing to read.
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().
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
write-or-die.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git c/write-or-die.c w/write-or-die.c
index 3ecb9e2af5..01a9a51fa2 100644
--- c/write-or-die.c
+++ w/write-or-die.c
@@ -18,23 +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) {
- int flush_setting = git_env_bool("GIT_FLUSH", -1);
+ static int force_flush_stdout = -1;
- if (0 <= flush_setting)
- skip_stdout_flush = !flush_setting;
- else {
+ 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)) {
^ permalink raw reply related
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Junio C Hamano @ 2024-02-13 17:28 UTC (permalink / raw)
To: Jeff King; +Cc: Josh Steadmon, git, johannes.schindelin, phillip.wood
In-Reply-To: <20240213074118.GA2225494@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Likewise (mine is the latest version in Debian unstable). The change to
> sort comes from their[1] eedea52a, which was in GNU make 4.2.90. But the
> matching documentation change didn't happen until 5b993ae, which was
> 4.3.90 in late 2021. So that explains the mystery.
>
> Those dates imply to me that we should keep the $(sort), though. Six
> years is not so long in distro timescales, especially given that Debian
> unstable is on a 4-year-old version. (And if we did want to get rid of
> it, certainly we should do so consistently across the Makefile in a
> separate patch).
Sounds sensible. The topic is expecting a reroll so I'll make sure
I won't touch it until I see an update.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 16/28] trailer: finish formatting unification
From: Linus Arver @ 2024-02-13 17:30 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: <CAP8UFD283wk9b+D+s3azcg9DHjaonaq6-eY1KDHkP8UWZYVXQg@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:
>
>> /*
>> - * Format the trailers from the commit msg "msg" into the strbuf "out".
>> - * Note two caveats about "opts":
>> - *
>> - * - this is primarily a helper for pretty.c, and not
>> - * all of the flags are supported.
>> - *
>> - * - this differs from process_trailers slightly in that we always format
>> - * only the trailer block itself, even if the "only_trailers" option is not
>> - * set.
>
> This makes me wonder if there was actually a good reason why
> format_trailers() and format_trailer_info() were 2 different
> functions. Is there info about this in the commit message of the
> commit which introduced this comment?
Good question. I see a388b10fc1 (pretty: move trailer formatting to
trailer.c, 2017-08-15) and there it says:
pretty: move trailer formatting to trailer.c
The next commit will add many features to the %(trailer)
placeholder in pretty.c. We'll need to access some internal
functions of trailer.c for that, so our options are either:
1. expose those functions publicly
or
2. make an entry point into trailer.c to do the formatting
Doing (2) ends up exposing less surface area, though do note
that caveats in the docstring of the new function.
so it looks like this function started out from pretty.c and did not
have access to all of the trailer implementation internals, and was
never intended to replace (unify) the formatting machinery in trailer.c
both before and after that commit. This seems like good information to
include in the commit message, so I will do so in the next reroll.
Aside: it is interesting that the current patch series is taking the
direction of (1) instead of (2) as was done in the past (we had a choice
to "libify" at that time but did not do so, in order to "expos[e] less
surface area [in <trailer.h>]").
^ permalink raw reply
* Re: [PATCH v4 00/28] Enrich Trailer API
From: Junio C Hamano @ 2024-02-13 17:30 UTC (permalink / raw)
To: Christian Couder
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Josh Steadmon, Randall S. Becker, Linus Arver
In-Reply-To: <CAP8UFD0C37qdTUvCpRFe6_zAeAcssoySY6tobw+AO8hpA8iAfg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
>> > I took a look at all of them, and I think that this series should be
>> > split into 4 or more series.
>>
>> 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.
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Junio C Hamano @ 2024-02-13 17:35 UTC (permalink / raw)
To: Jeff King; +Cc: Maarten Bosmans, git, Teng Long, Maarten Bosmans
In-Reply-To: <20240213080014.GB2225494@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Feb 06, 2024 at 09:52:38AM -0800, Junio C Hamano wrote:
>
>> > That is also a cool idea. That would probably use the functionality of
>> > the cat-file batch mode, right?
>>
>> Not really. I was hoping that "git show" that can take multiple
>> objects from its command line would directly be used, or with a new
>> option that gives a separator between these objects.
>
> How about:
>
> cat some_commit_ids |
> git show --stdin -s -z --format='%H%n%N'
Yeah, that is more in line with what I was hoping to see, instead of
invoking "git show %s" for a note object one by one.
Thanks.
Remind me if you can if we (1) had plans to allow non-blob objects
as notes, or (2) actively support to have non-text blob objects as
notes. I _think_ we do assume people may try to add non-text blob
as notes (while they accept that they cannot merge two such notes on
the same object), but I do not recall if we planned to allow them to
store trees and commits.
^ permalink raw reply
* Re: [GSOC][RFC] microproject: use test_path_is_* functions in test scripts
From: Christian Couder @ 2024-02-13 17:35 UTC (permalink / raw)
To: Vincenzo MEZZELA; +Cc: git
In-Reply-To: <cbb450b3-b333-4391-ac83-66c2daf0ae4d@gmail.com>
Hi,
On Tue, Feb 13, 2024 at 5:10 PM Vincenzo MEZZELA
<vincenzo.mezzela@gmail.com> wrote:
>
> Hello everyone,
>
> I'm Vincenzo, a Master's degree student in Computer Engineering and
> Cybersecurity.
>
> As I'm approaching the end of my academic journey, I'm eager to
> contribute to the Git project
> and the GSoC represents a good opportunity to do so. Upon exploring the
> online documentation
> of git for the application to the GSoC I'm keen to begin the required
> microproject.
Thanks for your interest in working on Git!
> Among the microproject proposed here
> https://git.github.io/SoC-2024-Microprojects/ , I would like to
> work on replacing 'test -(e|f|g|...)' with test_path_is* .
>
> Can you confirm if this task has already been taken by someone else?
This is a generic microproject so even if someone else is working on
this microproject, you can also work on it as long as you don't work
on the same file as that person.
> 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
Note that you only need to do that replacement in one test script
under the 't/' directory.
> To approximately measure the number of required replacement, I run the
> following commands from the
> 't/' directory (branch master):
>
> > # Files that requires a replacement
> > git grep -r 'test -[efdx]' 2>/dev/null| awk '{print $1}' | uniq -c |
> sort -n -r
> >> 190 t7301-clean-interactive.sh:
> >> 147 t7300-clean.sh:
> >> 21 t2004-checkout-cache-temp.sh:
> >> 17 t2401-worktree-prune.sh:
> >> 16 t2003-checkout-cache-mkdir.sh:
> >> 14 t0601-reffiles-pack-refs.sh:
> >> 13 t4200-rerere.sh:
> >> 12 t9146-git-svn-empty-dirs.sh:
> >> 12 t7603-merge-reduce-heads.sh:
> >> 12 lib-submodule-update.sh:
> >> ...
> >>
> > # Number of replacements
> > git grep -r 'test -[efdx]' 2>/dev/null| awk '{print $1}' | uniq -c |
> sort -n -r | awk '{sum += $1} END {print sum}'
> >> 853
> > # Number of files that requires a patch
> > git grep -r 'test -[efdx]' 2>/dev/null| awk '{print $1}' | uniq -c |
> wc -l
> >> 169
>
> Although the replacement work might not be difficult, it spans over many
> different test files.
> Do you want me to submit a patch for each of them as part of the
> microproject?
> If not, How many patches do you expect me to submit?
We would like a single patch that performs the necessary replacements
in a single file.
For reference in https://git.github.io/General-Microproject-Information/ we say:
"Change only a few files"
and
"This means that for a microproject that consists in refactoring or
rewriting a small amount of code, your patch should change only ONE
file, or perhaps 2 files if they are closely related, like “foo.c” and
“foo.h”."
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v4 19/28] trailer: make trailer_info struct private
From: Linus Arver @ 2024-02-13 17:36 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: <xmqqjznde235.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This patch believes that the benefits outweight the advantages for
>> designing APIs, as explained below.
>
> "outweigh the disadvantages"?
Will update, thanks.
^ permalink raw reply
* Re: [GSOC][RFC] microproject: use test_path_is_* functions in test scripts
From: Eric Sunshine @ 2024-02-13 17:39 UTC (permalink / raw)
To: Christian Couder; +Cc: Vincenzo MEZZELA, git
In-Reply-To: <CAP8UFD2ypikjKEVMmHMg7=jwv8J0y5xhx6PxsiLsGdhz2+S3PQ@mail.gmail.com>
On Tue, Feb 13, 2024 at 12:36 PM Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Feb 13, 2024 at 5:10 PM Vincenzo MEZZELA
> <vincenzo.mezzela@gmail.com> wrote:
> > Can you confirm if this task has already been taken by someone else?
>
> This is a generic microproject so even if someone else is working on
> this microproject, you can also work on it as long as you don't work
> on the same file as that person.
>
> > >> 12 t9146-git-svn-empty-dirs.sh:
This particular test script is being tackled by someone else on the
list presently, so probably best to choose one of the others.
^ permalink raw reply
* 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
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