* Re: [PATCH 04/22] t/annotate-tests.sh: avoid redundant use of cat
From: Junio C Hamano @ 2024-03-06 0:26 UTC (permalink / raw)
To: Rubén Justo; +Cc: git, Beat Bolli
In-Reply-To: <bdae6d2d-af56-4bc0-a000-5cf2ef44cd44@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> On Tue, Mar 05, 2024 at 02:28:15PM -0800, Junio C Hamano wrote:
>> "Beat Bolli" <bb@drbeat.li> writes:
>>
>> > Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>> > ---
>> > t/annotate-tests.sh | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
>> > index 5e21e84f3884..87572459e4b8 100644
>> > --- a/t/annotate-tests.sh
>> > +++ b/t/annotate-tests.sh
>> > @@ -532,7 +532,7 @@ test_expect_success 'blame -L :funcname with userdiff driver' '
>> > "$(cat file.template)" &&
>> > test_commit --author "B <B@test.git>" \
>> > "change" "$fortran_file" \
>> > - "$(cat file.template | sed -e s/ChangeMe/IWasChanged/)" &&
>> > + "$(sed -e s/ChangeMe/IWasChanged/ file.template)" &&
>>
>> Obviously correct, but
>>
>> "$(sed -e s/ChangeMe/IWasChanged/ <file.template)" &&
>>
>> might be a more faithful conversion (when "sed" looks at its ARGV[],
>> it did not find anything before, and it would not find anything
>> after this patch).
>
> Good point. Thank you for being careful.
Heh, I actually consider it the most irrelevant one among my
comments. I actally do not think there is a way tell if your "sed"
invocation is reading from one of the files listed on the command
line, or reading from the standard input, from your sed script,
unlike say Perl that has access to @ARGV. Certainly a simple s/A/B/
would not care.
Compared to that, rewriting $(cat file | wc -l) to $(wc -l <file)
does matter, which was done in [05/22].
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: Jeff King @ 2024-03-06 0:33 UTC (permalink / raw)
To: phillip.wood; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <a3de2b7b-4603-4604-a4d2-938a598e312e@gmail.com>
On Tue, Mar 05, 2024 at 03:30:35PM +0000, Phillip Wood wrote:
> Hi John
>
> On 04/03/2024 22:51, John Cai via GitGitGadget wrote:
> > From: John Cai <johncai86@gmail.com>
> >
> > For reftable development, it would be handy to have a tool to provide
> > the direct value of any ref whether it be a symbolic ref or not.
> > Currently there is git-symbolic-ref, which only works for symbolic refs,
> > and git-rev-parse, which will resolve the ref. Let's add a --unresolved
> > option that will only take one ref and return whatever it points to
> > without dereferencing it.
>
> "--unresolved" makes me think of merge conflicts. I wonder if
> "--no-dereference" would be clearer.
We have "--no-deref" in "git update-ref" already. It is probably better
to stay consistent.
-Peff
^ permalink raw reply
* Re: [PATCH 04/22] t/annotate-tests.sh: avoid redundant use of cat
From: Rubén Justo @ 2024-03-06 0:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Beat Bolli
In-Reply-To: <xmqq7cigxl1o.fsf@gitster.g>
On Tue, Mar 05, 2024 at 04:26:27PM -0800, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> > On Tue, Mar 05, 2024 at 02:28:15PM -0800, Junio C Hamano wrote:
> >> "Beat Bolli" <bb@drbeat.li> writes:
> >>
> >> > Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> >> > ---
> >> > t/annotate-tests.sh | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
> >> > index 5e21e84f3884..87572459e4b8 100644
> >> > --- a/t/annotate-tests.sh
> >> > +++ b/t/annotate-tests.sh
> >> > @@ -532,7 +532,7 @@ test_expect_success 'blame -L :funcname with userdiff driver' '
> >> > "$(cat file.template)" &&
> >> > test_commit --author "B <B@test.git>" \
> >> > "change" "$fortran_file" \
> >> > - "$(cat file.template | sed -e s/ChangeMe/IWasChanged/)" &&
> >> > + "$(sed -e s/ChangeMe/IWasChanged/ file.template)" &&
> >>
> >> Obviously correct, but
> >>
> >> "$(sed -e s/ChangeMe/IWasChanged/ <file.template)" &&
> >>
> >> might be a more faithful conversion (when "sed" looks at its ARGV[],
> >> it did not find anything before, and it would not find anything
> >> after this patch).
> >
> > Good point. Thank you for being careful.
>
> Heh, I actually consider it the most irrelevant one among my
> comments. I actally do not think there is a way tell if your "sed"
> invocation is reading from one of the files listed on the command
> line, or reading from the standard input, from your sed script,
> unlike say Perl that has access to @ARGV. Certainly a simple s/A/B/
> would not care.
>
> Compared to that, rewriting $(cat file | wc -l) to $(wc -l <file)
> does matter, which was done in [05/22].
Yeah, that is needed; faithfulness is appreciated.
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: Jeff King @ 2024-03-06 0:41 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <pull.1684.git.git.1709592718743.gitgitgadget@gmail.com>
On Mon, Mar 04, 2024 at 10:51:58PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> For reftable development, it would be handy to have a tool to provide
> the direct value of any ref whether it be a symbolic ref or not.
> Currently there is git-symbolic-ref, which only works for symbolic refs,
> and git-rev-parse, which will resolve the ref. Let's add a --unresolved
> option that will only take one ref and return whatever it points to
> without dereferencing it.
What about "git rev-parse --symbolic-full-name"? I don't think that
behaves quite the same as your patch here:
- it is actually not a true no-deref; it resolves to the final name
and then prints it (so the behavior is the same for a single-level
symref, but I believe a multi-level symref chain like
one->two->three will print "three" when resolving "one").
- it always prints the resolved name, whereas your patch prints an oid
for non-symrefs
I'm not sure if those are important or not, as I don't quite understand
what you're trying to accomplish. I'd probably have just run:
git symbolic-ref -q $name || git rev-parse --verify $name
I'm not opposed to making that more ergonomic, but I think we should
avoid redundant plumbing options if we can (I'm not sure yet if this is
redundant or not, but in general I find "show-ref" to be a weird mix of
"rev-parse" and "for-each-ref" that I'd be just as happy if it did not
exist).
-Peff
^ permalink raw reply
* Re: [PATCH 14/22] t/t9*: avoid redundant uses of cat
From: Junio C Hamano @ 2024-03-06 0:43 UTC (permalink / raw)
To: Rubén Justo; +Cc: Beat Bolli, git, Beat Bolli
In-Reply-To: <8b9667e5-0d2f-4624-8f7c-f8400250a21e@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
>> test_when_finished "git update-ref -d refs/heads/L2" &&
>> git fast-import <input &&
>> git ls-tree L2 g/b/ >tmp &&
>> - cat tmp | cut -f 2 >actual &&
>> + cut -f 2 <tmp >actual &&
>> test_cmp expect actual &&
>
> Nit: Maybe we can avoid tmp.
Piping "git ls-tree" output to "cut" would hide the exit status of
"git ls-tree" if it fails, which is not a good idea, so I do not
think of a way to avoid tmp so easily.
>
>> git fsck $(git rev-parse L2)
>> '
>> @@ -2012,7 +2012,7 @@ test_expect_success 'Q: verify first notes tree' '
>> 100644 blob $commit2
>> 100644 blob $commit3
>> EOF
>> - cat expect.unsorted | sort >expect &&
>> + sort expect.unsorted >expect &&
>
> Nit: I wonder if we can also avoid the cat that just precedes this hunk.
The whole thing reads like this:
test_expect_success 'Q: verify first notes tree' '
cat >expect.unsorted <<-EOF &&
100644 blob $commit1
100644 blob $commit2
100644 blob $commit3
EOF
cat expect.unsorted | sort >expect &&
git cat-file -p refs/notes/foobar~2^{tree} | sed "s/ [0-9a-f]* / /" >actual &&
test_cmp expect actual
'
As we are not in the business of debugging system-provided "sort",
I agree that
sort >expect <<-EOF &&
100644 blob $commit1
100644 blob $commit2
100644 blob $commit3
EOF
without having to use expect.unsorted would probably make sense.
Well spotted.
This is outside the topic, but this test has different clean-up
opportunities that are not related to the "do not run cat a single
file and send its output into a pipe" pattern. The expected output
we see here implicitly depends on the fact that the notes tree is so
small that it hasn't been reorganized using fan-out levels. If the
algorithm to decide when to start using fan-out directories changes,
this test can break. To avoid that, we may need to do something
like
git ls-tree -r refs/notes/foobar~2 >ls-tree &&
sed -e "s/ [0-9a-f]* / /" -e "s|/||g" >actual &&
so that we will get the commit object name without the slashes that
show the fan-out directories. Also, the current "actual" generation
hides the exit status from "git cat-file".
^ permalink raw reply
* Re: [PATCH 09/22] t/t4*: avoid redundant uses of cat
From: Junio C Hamano @ 2024-03-06 0:49 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-10-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> @@ -786,7 +786,7 @@ test_expect_success 'am takes patches from a Pine mailbox' '
> rm -fr .git/rebase-apply &&
> git reset --hard &&
> git checkout first &&
> - cat pine patch1 | git am &&
> + git am pine patch1 &&
> test_path_is_missing .git/rebase-apply &&
> git diff --exit-code main^..HEAD
> '
I am not so certain about this one.
We can say "sed can read from the file listed on the command line,
or it can read from its standard input, so we can use whichever is
convenient for us", as we are not in the business of testing "sed"
that is supplied by the system.
On the other hand, the ability of "git am" to read either from the
files listed on the command line or from the standard input is not a
given. It is one of the many aspects of how "git am" behaves that
we are testing. By changing a test that feeds the contents of the
mailboxes from the standard input to instead have the command read
these mailbox files listed on the command line, this changes what
gets tested.
All other changes in the file look good to me.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] ci: also define CXX environment variable
From: Jeff King @ 2024-03-06 0:50 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git
In-Reply-To: <75f98cbf98005b0a069977096ec5501f2f7830fe.1709673020.git.steadmon@google.com>
On Tue, Mar 05, 2024 at 01:11:59PM -0800, Josh Steadmon wrote:
> In a future commit, we will build the fuzzer executables as part of the
> default 'make all' target, which requires a C++ compiler. If we do not
> explicitly set CXX, it defaults to g++ on GitHub CI. However, this can
> lead to incorrect feature detection when CC=clang, since the
> 'detect-compiler' script only looks at CC. Fix the issue by always
> setting CXX to match CC in our CI config.
>
> We only plan on building fuzzers on Linux, so none of the other CI
> configs need a similar adjustment.
Does this mean that after your patch 2, running:
make CC=clang
may have problems on Linux, because it will now try to link fuzzers
using g++, even though everything else is built with clang (and ditto
the detect-compiler used it)?
-Peff
^ permalink raw reply
* Re: [PATCH 16/22] t/t3*: merge a "grep | awk" pipeline
From: Junio C Hamano @ 2024-03-06 0:55 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-17-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> t/t3920-crlf-messages.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh
> index 5eed640a6825..50ae222f0842 100755
> --- a/t/t3920-crlf-messages.sh
> +++ b/t/t3920-crlf-messages.sh
> @@ -97,7 +97,7 @@ test_expect_success 'branch: --verbose works with messages using CRLF' '
> git branch -v >tmp &&
> # Remove first two columns, and the line for the currently checked out branch
> current=$(git branch --show-current) &&
> - grep -v $current <tmp | awk "{\$1=\$2=\"\"}1" >actual &&
> + awk "/$current/ { next } { \$1 = \$2 = \"\" } 1" <tmp >actual &&
> test_cmp expect actual
> '
OK. The original excludes any line that has $current (a branch name
without any funny letter in it) on it and sends the rest to awk.
The updated does the skipping inside the awk script. They should be
equivalents.
Looks good.
^ permalink raw reply
* Re: [PATCH 18/22] t/t5*: merge a "grep | sed" pipeline
From: Junio C Hamano @ 2024-03-06 0:57 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-19-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> t/t5401-update-hooks.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
> index 8b8bc47dc0b9..d8cadeec7331 100755
> --- a/t/t5401-update-hooks.sh
> +++ b/t/t5401-update-hooks.sh
> @@ -123,7 +123,7 @@ remote: STDOUT post-update
> remote: STDERR post-update
> EOF
> test_expect_success 'send-pack stderr contains hook messages' '
> - grep ^remote: send.err | sed "s/ *\$//" >actual &&
> + sed -n "/^remote:/s/ *\$//p" send.err >actual &&
> test_cmp expect actual
> '
Both 17 & 18 look good.
^ permalink raw reply
* Re: [PATCH 19/22] t/t8*: merge "grep | sed" pipelines
From: Junio C Hamano @ 2024-03-06 0:59 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-20-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> t/t8013-blame-ignore-revs.sh | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
> index 9a03b0f361ff..05213d13f30f 100755
> --- a/t/t8013-blame-ignore-revs.sh
> +++ b/t/t8013-blame-ignore-revs.sh
> @@ -25,11 +25,11 @@ test_expect_success setup '
>
> git blame --line-porcelain file >blame_raw &&
>
> - grep -E "^[0-9a-f]+ [0-9]+ 1" blame_raw | sed -e "s/ .*//" >actual &&
> + sed -Ene "/^[0-9a-f]+ [0-9]+ 1/s/ .*//p" blame_raw >actual &&
Isn't -E a GNUism?
At least,
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
does not seem to have it (we may need to fix t6030 to rid its only
existing use).
^ permalink raw reply
* Re: [PATCH 1/2] ci: also define CXX environment variable
From: Jeff King @ 2024-03-06 1:00 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git
In-Reply-To: <20240306005057.GC3797463@coredump.intra.peff.net>
On Tue, Mar 05, 2024 at 07:50:58PM -0500, Jeff King wrote:
> On Tue, Mar 05, 2024 at 01:11:59PM -0800, Josh Steadmon wrote:
>
> > In a future commit, we will build the fuzzer executables as part of the
> > default 'make all' target, which requires a C++ compiler. If we do not
> > explicitly set CXX, it defaults to g++ on GitHub CI. However, this can
> > lead to incorrect feature detection when CC=clang, since the
> > 'detect-compiler' script only looks at CC. Fix the issue by always
> > setting CXX to match CC in our CI config.
> >
> > We only plan on building fuzzers on Linux, so none of the other CI
> > configs need a similar adjustment.
>
> Does this mean that after your patch 2, running:
>
> make CC=clang
>
> may have problems on Linux, because it will now try to link fuzzers
> using g++, even though everything else is built with clang (and ditto
> the detect-compiler used it)?
Also, if the answer is "yes": do we really need a c++ linker here? My
understanding from reading "git log -SCXX Makefile" is that when using
oss-fuzz, you'd sometimes want to pass c++ specific things in
FUZZ_CXXFLAGS. But we're not using that here, and are just making sure
that things can be linked. Can we just use $(CC) by default here, then?
Something like:
diff --git a/Makefile b/Makefile
index f74e96d7c2..3f09d75f46 100644
--- a/Makefile
+++ b/Makefile
@@ -3861,17 +3861,18 @@ cover_db_html: cover_db
#
# An example command to build against libFuzzer from LLVM 11.0.0:
#
-# make CC=clang CXX=clang++ \
+# make CC=clang FUZZ_CXX=clang++ \
# CFLAGS="-fsanitize=fuzzer-no-link,address" \
# LIB_FUZZING_ENGINE="-fsanitize=fuzzer,address" \
# fuzz-all
#
+FUZZ_CXX ?= $(CC)
FUZZ_CXXFLAGS ?= $(ALL_CFLAGS)
.PHONY: fuzz-all
$(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
- $(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
+ $(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
-Wl,--allow-multiple-definition \
$(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
-Peff
^ permalink raw reply related
* Re: [PATCH 20/22] t/t9*: merge "grep | sed" pipelines
From: Junio C Hamano @ 2024-03-06 1:00 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-21-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> t/t9118-git-svn-funky-branch-names.sh | 2 +-
> t/t9350-fast-export.sh | 2 +-
> t/t9824-git-p4-git-lfs.sh | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
Looking good. Thanks.
^ permalink raw reply
* Re: [PATCH 09/22] t/t4*: avoid redundant uses of cat
From: Eric Sunshine @ 2024-03-06 1:08 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Junio C Hamano, Beat Bolli
In-Reply-To: <20240305212533.12947-10-dev+git@drbeat.li>
On Tue, Mar 5, 2024 at 4:31 PM Beat Bolli <bb@drbeat.li> wrote:
> diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
> @@ -232,7 +232,7 @@ keep_only_cr () {
> test_expect_success 'external diff with autocrlf = true' '
> test_config core.autocrlf true &&
> GIT_EXTERNAL_DIFF=./fake-diff.sh git diff &&
> - test $(wc -l < crlfed.txt) = $(cat crlfed.txt | keep_only_cr | wc -c)
> + test $(wc -l < crlfed.txt) = $(keep_only_cr <crlfed.txt | wc -c)
> '
Could also fix the style problem (drop whitespace after existing `<`
operator) while here, but not at all worth a reroll.
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> @@ -786,7 +786,7 @@ test_expect_success 'am takes patches from a Pine mailbox' '
> git checkout first &&
> - cat pine patch1 | git am &&
> + git am pine patch1 &&
As with Junio, the semantic change made here concerned me.
^ permalink raw reply
* Re: [PATCH 14/22] t/t9*: avoid redundant uses of cat
From: Rubén Justo @ 2024-03-06 1:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Beat Bolli, git, Beat Bolli
In-Reply-To: <xmqqwmqgw5oq.fsf@gitster.g>
On Tue, Mar 05, 2024 at 04:43:33PM -0800, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> >> git ls-tree L2 g/b/ >tmp &&
> >> - cat tmp | cut -f 2 >actual &&
> >> + cut -f 2 <tmp >actual &&
> >> test_cmp expect actual &&
> >
> > Nit: Maybe we can avoid tmp.
>
> Piping "git ls-tree" output to "cut" would hide the exit status of
> "git ls-tree" if it fails, which is not a good idea, so I do not
> think of a way to avoid tmp so easily.
Right. Thanks for pointing that out.
^ permalink raw reply
* Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe
From: Jeff King @ 2024-03-06 1:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Atneya Nair, git, jeffhost, me, nasamuffin, Tanay Abhra,
Glen Choo
In-Reply-To: <xmqqsf141pf5.fsf@gitster.g>
On Tue, Mar 05, 2024 at 10:53:02AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > The use of strintern() comes originally from 3df8fd62 ...
> > ..., so they may
> > know how safe the change on the config side would be (I still do
> > not understand why you'd want to do this in the first place, though,
> > especially if you are protecting the callsites with mutex).
>
> The risks of turning code that uses strintern() to use strdup() are
>
> * you will leak the allocated string unless you explicitly free the
> string you now own.
>
> * you may consume too much memory if you are creating too many
> copies of the same string (e.g. if you need filename for each
> line in a file in an application, the memory consumption can
> become 1000-fold).
>
> * the code may be taking advantage of the fact that two such
> strings can be compared for (in)equality simply by comparing
> their addresses, which you would need to adjust to use !strcmp()
> and the like.
There is one more, I think: if you _do_ free the allocated string to
avoid the leak you mention, then some other code which was relying on
the lifetime of that string to be effectively infinite will now have a
user-after-free.
And I think that may be the case here. The "kvi" struct itself is local
to do_config_from(). But when we load the caching configset, we do so in
configset_add_value() which makes a shallow copy of the kvi struct. And
then that shallow copy may live on and be accessed with further calls to
git_config().
So doing just this:
diff --git a/config.c b/config.c
index 3cfeb3d8bd..2f6c83ffe7 100644
--- a/config.c
+++ b/config.c
@@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
enum config_scope scope,
struct key_value_info *out)
{
- out->filename = strintern(cs->name);
+ out->filename = xstrdup(cs->name);
out->origin_type = cs->origin_type;
out->linenr = cs->linenr;
out->scope = scope;
@@ -1855,6 +1855,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
ret = git_parse_source(top, fn, &kvi, data, opts);
+ free((char *)kvi.filename);
strbuf_release(&top->value);
strbuf_release(&top->var);
will cause t4013.199 (among others) to fail when built with
SANITIZE=address, as it detects the user-after-free. I think you'd need
this on top:
diff --git a/config.c b/config.c
index 2f6c83ffe7..9854ca002d 100644
--- a/config.c
+++ b/config.c
@@ -2262,6 +2262,7 @@ static int configset_add_value(const struct key_value_info *kvi_p,
l_item->value_index = e->value_list.nr - 1;
*kv_info = *kvi_p;
+ kv_info->filename = xstrdup_or_null(kvi_p->filename); /* deep copy! */
si->util = kv_info;
return 0;
though probably an actual kvi_copy() function would be less horrible.
A few other things to note, looking at this code:
- isn't kvi->path in the same boat? We do not duplicate it at all, so
it seems like the shallow copy made in the configset could cause a
user-after-free.
- the "fix" I showed above hits your point 2: now we are making a lot
more copies of that string. I will note that we're already making a
lot of copies of the kvi struct in the first place, so unless you
have really long pathnames, it probably isn't a big difference.
But it possibly could make sense to have the configset own a single
duplicate string, and then let the kvi structs it holds point to
that string. But IMHO all of this should be details of the configset
code, and the main config-iteration code should not have to worry
about this at all. I.e., I think kvi_from_source() should not be
duplicating anything in the first place.
-Peff
^ permalink raw reply related
* Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe
From: Junio C Hamano @ 2024-03-06 1:58 UTC (permalink / raw)
To: Jeff King
Cc: Atneya Nair, git, jeffhost, me, nasamuffin, Tanay Abhra,
Glen Choo
In-Reply-To: <20240306012323.GA3817803@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> There is one more, I think: if you _do_ free the allocated string to
> avoid the leak you mention, then some other code which was relying on
> the lifetime of that string to be effectively infinite will now have a
> user-after-free.
Ah, yes, you're right. I completely forgot about that shallow copy.
> A few other things to note, looking at this code:
>
> - isn't kvi->path in the same boat? We do not duplicate it at all, so
> it seems like the shallow copy made in the configset could cause a
> user-after-free.
>
> - the "fix" I showed above hits your point 2: now we are making a lot
> more copies of that string. I will note that we're already making a
> lot of copies of the kvi struct in the first place, so unless you
> have really long pathnames, it probably isn't a big difference.
>
> But it possibly could make sense to have the configset own a single
> duplicate string, and then let the kvi structs it holds point to
> that string. But IMHO all of this should be details of the configset
> code, and the main config-iteration code should not have to worry
> about this at all. I.e., I think kvi_from_source() should not be
> duplicating anything in the first place.
Thanks for a detailed write-up.
^ permalink raw reply
* Re: [PATCH 19/22] t/t8*: merge "grep | sed" pipelines
From: Todd Zullinger @ 2024-03-06 2:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Beat Bolli, git, Beat Bolli
In-Reply-To: <xmqq8r2ww4xg.fsf@gitster.g>
Junio C Hamano wrote:
> Isn't -E a GNUism?
>
> At least,
>
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
>
> does not seem to have it (we may need to fix t6030 to rid its only
> existing use).
I _thought_ that -r was the GNUism. The GNU sed-4.8 manpage
says:
-E, -r, --regexp-extended
use extended regular expressions in the script
(for portability use POSIX -E).
That doesn't mean the man page is right, of course. :)
https://www.austingroupbugs.net/view.php?id=528 suggests
that -E has been adopted and, importanly, is more widely
supported than -r (if we were considering using that rather
than rewriting this to not use ERE syntax). MacOS in
particular supports -E but not -r, according to that link.
It seems like the documentation hasn't quite caught up to
reality yet, perhaps?
--
Todd
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: Junio C Hamano @ 2024-03-06 2:19 UTC (permalink / raw)
To: Jeff King; +Cc: phillip.wood, John Cai via GitGitGadget, git, John Cai
In-Reply-To: <20240306003343.GA3797463@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Mar 05, 2024 at 03:30:35PM +0000, Phillip Wood wrote:
>
>> Hi John
>>
>> On 04/03/2024 22:51, John Cai via GitGitGadget wrote:
>> > From: John Cai <johncai86@gmail.com>
>> >
>> > For reftable development, it would be handy to have a tool to provide
>> > the direct value of any ref whether it be a symbolic ref or not.
>> > Currently there is git-symbolic-ref, which only works for symbolic refs,
>> > and git-rev-parse, which will resolve the ref. Let's add a --unresolved
>> > option that will only take one ref and return whatever it points to
>> > without dereferencing it.
>>
>> "--unresolved" makes me think of merge conflicts. I wonder if
>> "--no-dereference" would be clearer.
>
> We have "--no-deref" in "git update-ref" already. It is probably better
> to stay consistent.
That's an excellent precedent. Thanks.
^ permalink raw reply
* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Elijah Newren @ 2024-03-06 2:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Haller, git, Derrick Stolee, Phillip Wood,
Christian Couder
In-Reply-To: <xmqqsf144pi7.fsf@gitster.g>
[Restoring part of Stefan's earlier message so I can respond to both
that piece, as well as add to the ideas Junio presents.]
Hi,
On Tue, Mar 5, 2024 at 8:22 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Haller <lists@haller-berlin.de> writes:
>
> >> And I now see that "git replay --contained --onto" has the same problem,
> >> which I find very unfortunate. In my opinion, "contained" should only
> >> include refs that form a stack, but not copies of the current branch.
I wouldn't want to change the default. Even if we were to add an
option, I'm not entirely sure what it should even implement. In
addition to Phillip's previous response in the thread, and part of
Junio's response below (which I'll add to):
1) What if there is a branch that is "just a copy" of one of the
branches earlier in the "stack"? Since it's "just a copy", shouldn't
it be excluded for similar reasons to what you are arguing? And, if
so, which branch is the copy?
2) Further, a "stack", to me at least, suggests a linear history
without branching (i.e. each commit has at most one parent _and_ at
most one child among the commits in the stack). I designed `git
replay` to handle diverging histories (i.e. rebasing multiple branches
that _might_ share a subset of common history but none necessarily
need to fully contain the others, though perhaps the branches do share
some other contained branches), and I want it to handle replaying
merges as well. While `git rebase --update-refs` is absolutely
limited to "stacks", and thus your argument might make sense in the
context of `git rebase`, since you are bringing `git replay` into the
mix, it needs to apply beyond a stack of commits. It's not clear to
me how to genericize your suggestions to handle cases other than a
simple stack of commits, though.
3) This is mostly covered in (1) and (2), but to be explicit: `git
replay` is completely against the HEAD-is-special assumptions that are
pervasive within `git rebase`, and your problem is entirely phrased as
HEAD-is-special due to your call out of "the current branch". Is your
argument limited to such special cases? (If so, it might still be
valid for `git rebase`, of course.)
4) Aren't there easier ways to handle this -- for both rebase and
replay? I'll suggest some alternatives below...
> >> Both of these cases could be fixed by --update-refs not touching any
> >> refs that point to the current HEAD. I'm having a hard time coming up
> >> with cases where you would ever want those to be updated, in fact.
>
> The point of "update-refs", as I understand it, is that in addition
> to the end point of the history (E in "git rebase --onto N O E"),
> any branch tips that are between O..E can be migrated to point at
> their rewritten counterparts. So I am not sure how it fundamentally
> solves much by protecting only refs that point at a single commit
> ("the current HEAD" in your statement).
>
> When I want to see how the rebased history would look like without
> touching the original, I often rebase a detached HEAD (i.e. instead
> of the earlier one, use "git rebase --onto N O E^0", or when
> rebasing the current branch, "git rebase [--onto N] O HEAD^0") and
> that would protect the current branch well, but --update-refs of
> course would not work well. There is no handy place like detached
> HEAD that can be used to save rewritten version of these extra
> branch tips.
>
> If branch tips A, B, and C are involved in the range of commits
> being rewritten, one way to help us in such a situation may be to
> teach "git rebase" to (1) somehow create a new set of proposed-A,
> proposed-B, and proposed-C refs (they do not have to be branches),
> while keeping the original A, B, and C intact, (2) allow us to
> inspect the resulting refs, compare the corresponding ones from
> these two sets, and (3) allow us to promote (possibly a subset of)
> proposed- ones to their counterpart real branches after we inspect
> them. The latter two do not have to be subcommands of "git rebase"
> but can be separate and new commands.
Here, Junio is suggesting one alternative, and it's already
implemented in `git replay`. Let me extend upon it and add two other
alternatives as well:
4a) `git replay` does what Junio suggests naturally, since it doesn't
update the refs but instead gives commands which can be fed to `git
update-ref --stdin`. Thus, users can inspect the output of `git
replay` and only perform the updates they want (by feeding a subset of
the lines to update-ref --stdin).
4b) For `git replay`, --contained is just syntactic sugar -- it isn't
necessary. git replay will allow you to list multiple branches that
you want replayed, so you can specify which branches are relevant to
you. (This doesn't help with `git rebase`, because `--update-refs` is
the only way to get additional branches replayed.)
4c) For `git rebase --update-refs`, you can add `--interactive` and
then delete the `update-ref` line(s) corresponding to the refs you
don't want updated.
^ permalink raw reply
* [PATCH v4] build: support z/OS (OS/390).
From: Haritha via GitGitGadget @ 2024-03-06 5:44 UTC (permalink / raw)
To: git; +Cc: Haritha D, Ghanshyam Thakkar, rsbecker, Haritha, Haritha D
In-Reply-To: <pull.1663.v3.git.git.1708841439516.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
Introduced z/OS (OS/390) as a platform in config.mak.uname
Signed-off-by: Haritha D <harithamma.d@ibm.com>
---
This PR enables a successful git build on z/OS.
Introduced z/OS (OS/390) as a platform in config.mak.uname
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1663%2FHarithaIBM%2Fzos-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1663/HarithaIBM/zos-v4
Pull-Request: https://github.com/git/git/pull/1663
Range-diff vs v3:
1: 2f1ad41bc14 ! 1: cbc38a801e9 build: support z/OS (OS/390).
@@ Metadata
## Commit message ##
build: support z/OS (OS/390).
- Since the z/OS linker does not support searching dynamic libraries,
- and the current setting of CC_LD_DYNPATH results in a directory
- to be supplied to the link step with no option as the suffix,
- it causes a linker error because the z/OS LD linker
- does not accept directories as input.
- Therefore, -L option is added.
- Also introduced z/OS (OS/390) as a platform in config.mak.uname
+ Introduced z/OS (OS/390) as a platform in config.mak.uname
Signed-off-by: Haritha D <harithamma.d@ibm.com>
@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
SHELL_PATH = /usr/coreutils/bin/bash
endif
+ifeq ($(uname_S),OS/390)
-+ NO_SYS_POLL_H = YesPlease
-+ NO_STRCASESTR = YesPlease
-+ NO_REGEX = YesPlease
-+ NO_MMAP = YesPlease
-+ NO_NSEC = YesPlease
-+ NO_STRLCPY = YesPlease
-+ NO_MEMMEM = YesPlease
-+ NO_GECOS_IN_PWENT = YesPlease
-+ HAVE_STRINGS_H = YesPlease
-+ NEEDS_MODE_TRANSLATION = YesPlease
++ NO_SYS_POLL_H = YesPlease
++ NO_STRCASESTR = YesPlease
++ NO_REGEX = YesPlease
++ NO_MMAP = YesPlease
++ NO_NSEC = YesPlease
++ NO_STRLCPY = YesPlease
++ NO_MEMMEM = YesPlease
++ NO_GECOS_IN_PWENT = YesPlease
++ HAVE_STRINGS_H = YesPlease
++ NEEDS_MODE_TRANSLATION = YesPlease
+endif
ifeq ($(uname_S),MINGW)
ifeq ($(shell expr "$(uname_R)" : '1\.'),2)
config.mak.uname | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/config.mak.uname b/config.mak.uname
index dacc95172dc..d0dcca2ec55 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -638,6 +638,18 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
SHELL_PATH = /usr/coreutils/bin/bash
endif
+ifeq ($(uname_S),OS/390)
+ NO_SYS_POLL_H = YesPlease
+ NO_STRCASESTR = YesPlease
+ NO_REGEX = YesPlease
+ NO_MMAP = YesPlease
+ NO_NSEC = YesPlease
+ NO_STRLCPY = YesPlease
+ NO_MEMMEM = YesPlease
+ NO_GECOS_IN_PWENT = YesPlease
+ HAVE_STRINGS_H = YesPlease
+ NEEDS_MODE_TRANSLATION = YesPlease
+endif
ifeq ($(uname_S),MINGW)
ifeq ($(shell expr "$(uname_R)" : '1\.'),2)
$(error "Building with MSys is no longer supported")
base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] show-ref: add --unresolved option
From: Patrick Steinhardt @ 2024-03-06 7:31 UTC (permalink / raw)
To: Jeff King; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <20240306004139.GB3797463@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2400 bytes --]
On Tue, Mar 05, 2024 at 07:41:39PM -0500, Jeff King wrote:
> On Mon, Mar 04, 2024 at 10:51:58PM +0000, John Cai via GitGitGadget wrote:
>
> > From: John Cai <johncai86@gmail.com>
> >
> > For reftable development, it would be handy to have a tool to provide
> > the direct value of any ref whether it be a symbolic ref or not.
> > Currently there is git-symbolic-ref, which only works for symbolic refs,
> > and git-rev-parse, which will resolve the ref. Let's add a --unresolved
> > option that will only take one ref and return whatever it points to
> > without dereferencing it.
>
> What about "git rev-parse --symbolic-full-name"? I don't think that
> behaves quite the same as your patch here:
>
> - it is actually not a true no-deref; it resolves to the final name
> and then prints it (so the behavior is the same for a single-level
> symref, but I believe a multi-level symref chain like
> one->two->three will print "three" when resolving "one").
>
> - it always prints the resolved name, whereas your patch prints an oid
> for non-symrefs
>
> I'm not sure if those are important or not, as I don't quite understand
> what you're trying to accomplish. I'd probably have just run:
>
> git symbolic-ref -q $name || git rev-parse --verify $name
>
> I'm not opposed to making that more ergonomic, but I think we should
> avoid redundant plumbing options if we can (I'm not sure yet if this is
> redundant or not, but in general I find "show-ref" to be a weird mix of
> "rev-parse" and "for-each-ref" that I'd be just as happy if it did not
> exist).
Yeah, the proposed patch basically aims to do the above chained command
in an easier way. I think that this would be a nice addition to make
this easier to use and better discoverable. But in the long run what I
think would be really useful is to extend git-for-each-ref(1) and/or
git-show-ref(1) so that they can print all existing refs with their
direct values. Right now, it's impossible to get a globally consistent
view of all refs in the refdb with their unresolved values.
That will end up a bit more involved though. The ref iterators we have
right now do not provide any way to return symref targets to the caller,
so we would have to first extend the interfaces and adapt both backends
to support this. But this is kind of where I want to end up.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: Jeff King @ 2024-03-06 7:51 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <Zegbw2i-PGfvb5q_@tanuki>
On Wed, Mar 06, 2024 at 08:31:15AM +0100, Patrick Steinhardt wrote:
> Yeah, the proposed patch basically aims to do the above chained command
> in an easier way. I think that this would be a nice addition to make
> this easier to use and better discoverable. But in the long run what I
> think would be really useful is to extend git-for-each-ref(1) and/or
> git-show-ref(1) so that they can print all existing refs with their
> direct values. Right now, it's impossible to get a globally consistent
> view of all refs in the refdb with their unresolved values.
Yeah, it seems like if this were a format-specifier for for-each-ref it
would be a lot more flexible.
You can do:
git for-each-ref --format='%(refname) %(objectname) %(symref)'
to get the resolved values next to the symrefs (if any). I think that
does a full resolution, though (so again, if you had one->two->three,
you can never learn about the intermediate "two").
> That will end up a bit more involved though. The ref iterators we have
> right now do not provide any way to return symref targets to the caller,
> so we would have to first extend the interfaces and adapt both backends
> to support this. But this is kind of where I want to end up.
I think for-each-ref in the above command works by calling
resolve_refdup() itself, and then recording the result. It would be nice
to get it from the iterator, though (more efficient, and avoids any
races).
-Peff
^ permalink raw reply
* [messy PATCH] multi-byte core.commentChar
From: Jeff King @ 2024-03-06 8:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <xmqqmsrc4osm.fsf@gitster.g>
On Tue, Mar 05, 2024 at 08:38:17AM -0800, Junio C Hamano wrote:
> It is not my personal itch, so I haven't done anything to make the
> commentChar take more than one byte. But if it is somebody else's
> itch, I do not see a reason why we should forbid them from
> scratching. If the setting seeps through across repository
> boundaries, that may create a compatibility issue and that by itself
> might be such a reason. If it greatly makes the code more complex,
> that may be another reason you can use to argue against adding such
> a "feature". If it makes the semantics of what "a comment string"
> is and how they are added and stripped at various stages of
> processing commit log messages fuzzy and harder to document and
> understand, that might be another reason. I however do not think
> any of these to be true. Maybe I am overly optimistic. I haven't
> looked deeply into the code around commentChar for quite some time.
Here's a messy version of what it would look like, in case anybody is
interested. It passes the tests (using the string "#" for the most
part), but there may be corner cases lurking. The %c/%s conversions are
noisy but obvious. The trickier parts are matching, which goes from
single-character to a string match. I used starts_with() but had to
introduce a "_mem" variant for buffers that aren't NUL-terminated.
There's a bit of mild refactoring/cleanup to avoid awkwardness in a few
spots, as well.
I also did a few manual tests with "foo>" and "•" as comment chars,
which seemed to work.
I can't imagine using this myself (I don't even set core.commentChar at
all), so it was mostly that I nerd-sniped myself by thinking "how hard
could it be?". Not too bad, but not trivial. But maybe it spurs somebody
interested in working on it. I am on the fence whether supporting UTF-8
like the bullet-point above is maybe something we should just do on
principle.
For a more readable series, I'd guess it would make sense to introduce
comment_line_str as a separate variable (but continue to enforce the
single-char rule), convert the easy cases en masse, the tricky cases one
by one, and then finally drop comment_line_char entirely. At which point
the config rules can be lifted to allow multi-byte strings.
---
add-patch.c | 4 ++--
builtin/branch.c | 2 +-
builtin/commit.c | 19 ++++++++++------
builtin/merge.c | 2 +-
builtin/tag.c | 4 ++--
commit.c | 3 ++-
config.c | 7 +++---
environment.c | 2 +-
environment.h | 2 +-
fmt-merge-msg.c | 2 +-
sequencer.c | 28 ++++++++++++-----------
strbuf.c | 43 +++++++++++++++++++----------------
strbuf.h | 7 +++---
t/t7508-status.sh | 4 +++-
trailer.c | 6 ++---
wt-status.c | 23 +++++++------------
16 files changed, 82 insertions(+), 76 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..4b4db0f253 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1114,7 +1114,7 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
"To remove '%c' lines, make them ' ' lines "
"(context).\n"
"To remove '%c' lines, delete them.\n"
- "Lines starting with %c will be removed.\n"),
+ "Lines starting with %s will be removed.\n"),
s->mode->is_reverse ? '+' : '-',
s->mode->is_reverse ? '-' : '+',
comment_line_char);
@@ -1139,7 +1139,7 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
for (i = 0; i < s->buf.len; ) {
size_t next = find_next_line(&s->buf, i);
- if (s->buf.buf[i] != comment_line_char)
+ if (!starts_with(s->buf.buf + i, comment_line_char))
strbuf_add(&s->plain, s->buf.buf + i, next - i);
i = next;
}
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5f..0b6e1d1adb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -670,7 +670,7 @@ static int edit_branch_description(const char *branch_name)
strbuf_commented_addf(&buf, comment_line_char,
_("Please edit the description for the branch\n"
" %s\n"
- "Lines starting with '%c' will be stripped.\n"),
+ "Lines starting with '%s' will be stripped.\n"),
branch_name, comment_line_char);
write_file_buf(edit_description(), buf.buf, buf.len);
strbuf_reset(&buf);
diff --git a/builtin/commit.c b/builtin/commit.c
index 6d1fa71676..898c8aadc7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -678,15 +678,20 @@ static int author_date_is_interesting(void)
return author_message || force_date;
}
+/*
+ * This only supports single-byte comment chars, but that's OK;
+ * our candidate list is fixed.
+ */
static void adjust_comment_line_char(const struct strbuf *sb)
{
char candidates[] = "#;@!$%^&|:";
char *candidate;
const char *p;
- comment_line_char = candidates[0];
- if (!memchr(sb->buf, comment_line_char, sb->len))
+ if (!memchr(sb->buf, candidates[0], sb->len)) {
+ comment_line_char = xstrfmt("%c", candidates[0]);
return;
+ }
p = sb->buf;
candidate = strchr(candidates, *p);
@@ -705,7 +710,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
if (!*p)
die(_("unable to select a comment character that is not used\n"
"in the current commit message"));
- comment_line_char = *p;
+ comment_line_char = xstrfmt("%c", *p);
}
static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
@@ -909,18 +914,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
struct ident_split ci, ai;
const char *hint_cleanup_all = allow_empty_message ?
_("Please enter the commit message for your changes."
- " Lines starting\nwith '%c' will be ignored.\n") :
+ " Lines starting\nwith '%s' will be ignored.\n") :
_("Please enter the commit message for your changes."
- " Lines starting\nwith '%c' will be ignored, and an empty"
+ " Lines starting\nwith '%s' will be ignored, and an empty"
" message aborts the commit.\n");
const char *hint_cleanup_space = allow_empty_message ?
_("Please enter the commit message for your changes."
" Lines starting\n"
- "with '%c' will be kept; you may remove them"
+ "with '%s' will be kept; you may remove them"
" yourself if you want to.\n") :
_("Please enter the commit message for your changes."
" Lines starting\n"
- "with '%c' will be kept; you may remove them"
+ "with '%s' will be kept; you may remove them"
" yourself if you want to.\n"
"An empty message aborts the commit.\n");
if (whence != FROM_COMMIT) {
diff --git a/builtin/merge.c b/builtin/merge.c
index 935c8a57dd..81b1cf5b90 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -821,7 +821,7 @@ static const char scissors_editor_comment[] =
N_("An empty message aborts the commit.\n");
static const char no_scissors_editor_comment[] =
-N_("Lines starting with '%c' will be ignored, and an empty message aborts\n"
+N_("Lines starting with '%s' will be ignored, and an empty message aborts\n"
"the commit.\n");
static void write_merge_heads(struct commit_list *);
diff --git a/builtin/tag.c b/builtin/tag.c
index 19a7e06bf4..8b17705cf6 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -158,11 +158,11 @@ static int do_sign(struct strbuf *buffer)
static const char tag_template[] =
N_("\nWrite a message for tag:\n %s\n"
- "Lines starting with '%c' will be ignored.\n");
+ "Lines starting with '%s' will be ignored.\n");
static const char tag_template_nocleanup[] =
N_("\nWrite a message for tag:\n %s\n"
- "Lines starting with '%c' will be kept; you may remove them"
+ "Lines starting with '%s' will be kept; you may remove them"
" yourself if you want to.\n");
static int git_tag_config(const char *var, const char *value,
diff --git a/commit.c b/commit.c
index ef679a0b93..ff9d49a141 100644
--- a/commit.c
+++ b/commit.c
@@ -1796,7 +1796,8 @@ size_t ignored_log_message_bytes(const char *buf, size_t len)
else
next_line++;
- if (buf[bol] == comment_line_char || buf[bol] == '\n') {
+ if (starts_with_mem(buf + bol, cutoff - bol, comment_line_char) ||
+ buf[bol] == '\n') {
/* is this the first of the run of comments? */
if (!boc)
boc = bol;
diff --git a/config.c b/config.c
index 3cfeb3d8bd..9280dc9844 100644
--- a/config.c
+++ b/config.c
@@ -1565,11 +1565,10 @@ static int git_default_core_config(const char *var, const char *value,
return config_error_nonbool(var);
else if (!strcasecmp(value, "auto"))
auto_comment_line_char = 1;
- else if (value[0] && !value[1]) {
- comment_line_char = value[0];
+ else {
+ comment_line_char = xstrdup(value);
auto_comment_line_char = 0;
- } else
- return error(_("core.commentChar should only be one ASCII character"));
+ }
return 0;
}
diff --git a/environment.c b/environment.c
index 90632a39bc..4435866d4e 100644
--- a/environment.c
+++ b/environment.c
@@ -110,7 +110,7 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT;
* The character that begins a commented line in user-editable file
* that is subject to stripspace.
*/
-char comment_line_char = '#';
+char *comment_line_char = "#";
int auto_comment_line_char;
/* Parallel index stat data preload? */
diff --git a/environment.h b/environment.h
index e5351c9dd9..821c6079af 100644
--- a/environment.h
+++ b/environment.h
@@ -8,7 +8,7 @@ struct strvec;
* The character that begins a commented line in user-editable file
* that is subject to stripspace.
*/
-extern char comment_line_char;
+extern char *comment_line_char;
extern int auto_comment_line_char;
/*
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 66e47449a0..daf57917fa 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -321,7 +321,7 @@ static void credit_people(struct strbuf *out,
skip_prefix(me, them->items->string, &me) &&
starts_with(me, " <")))
return;
- strbuf_addf(out, "\n%c %s ", comment_line_char, label);
+ strbuf_addf(out, "\n%s %s ", comment_line_char, label);
add_people_count(out, them);
}
diff --git a/sequencer.c b/sequencer.c
index f49a871ac0..2370abc379 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -663,7 +663,7 @@ void append_conflicts_hint(struct index_state *istate,
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
strbuf_addch(msgbuf, '\n');
wt_status_append_cut_line(msgbuf);
- strbuf_addch(msgbuf, comment_line_char);
+ strbuf_addstr(msgbuf, comment_line_char);
}
strbuf_addch(msgbuf, '\n');
@@ -1779,14 +1779,16 @@ static const char *command_to_string(const enum todo_command command)
{
if (command < TODO_COMMENT)
return todo_command_info[command].str;
+ if (command == TODO_COMMENT)
+ return comment_line_char;
die(_("unknown command: %d"), command);
}
static char command_to_char(const enum todo_command command)
{
if (command < TODO_COMMENT)
return todo_command_info[command].c;
- return comment_line_char;
+ return 0;
}
static int is_noop(const enum todo_command command)
@@ -1840,7 +1842,7 @@ static int is_fixup_flag(enum todo_command command, unsigned flag)
static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
{
const char *s = str;
- while (len > 0 && s[0] == comment_line_char) {
+ while (len > 0 && starts_with_mem(s, len, comment_line_char)) {
size_t count;
const char *n = memchr(s, '\n', len);
if (!n)
@@ -1946,7 +1948,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
(starts_with(body, "squash!") || starts_with(body, "fixup!"))))
commented_len = commit_subject_length(body);
- strbuf_addf(buf, "\n%c ", comment_line_char);
+ strbuf_addf(buf, "\n%s ", comment_line_char);
strbuf_addf(buf, _(nth_commit_msg_fmt),
++opts->current_fixup_count + 1);
strbuf_addstr(buf, "\n\n");
@@ -2003,10 +2005,10 @@ static int update_squash_messages(struct repository *r,
return error(_("could not read '%s'"),
rebase_path_squash_msg());
- eol = buf.buf[0] != comment_line_char ?
+ eol = !starts_with(buf.buf, comment_line_char) ?
buf.buf : strchrnul(buf.buf, '\n');
- strbuf_addf(&header, "%c ", comment_line_char);
+ strbuf_addf(&header, "%s ", comment_line_char);
strbuf_addf(&header, _(combined_commit_msg_fmt),
opts->current_fixup_count + 2);
strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
@@ -2032,9 +2034,9 @@ static int update_squash_messages(struct repository *r,
repo_unuse_commit_buffer(r, head_commit, head_message);
return error(_("cannot write '%s'"), rebase_path_fixup_msg());
}
- strbuf_addf(&buf, "%c ", comment_line_char);
+ strbuf_addf(&buf, "%s ", comment_line_char);
strbuf_addf(&buf, _(combined_commit_msg_fmt), 2);
- strbuf_addf(&buf, "\n%c ", comment_line_char);
+ strbuf_addf(&buf, "\n%s ", comment_line_char);
strbuf_addstr(&buf, is_fixup_flag(command, flag) ?
_(skip_first_commit_msg_str) :
_(first_commit_msg_str));
@@ -2056,7 +2058,7 @@ static int update_squash_messages(struct repository *r,
if (command == TODO_SQUASH || is_fixup_flag(command, flag)) {
res = append_squash_message(&buf, body, command, opts, flag);
} else if (command == TODO_FIXUP) {
- strbuf_addf(&buf, "\n%c ", comment_line_char);
+ strbuf_addf(&buf, "\n%s ", comment_line_char);
strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),
++opts->current_fixup_count + 1);
strbuf_addstr(&buf, "\n\n");
@@ -2562,7 +2564,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
/* left-trim */
bol += strspn(bol, " \t");
- if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
+ if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_char)) {
item->command = TODO_COMMENT;
item->commit = NULL;
item->arg_offset = bol - buf;
@@ -5659,7 +5661,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
oid_to_hex(&commit->object.oid),
oneline.buf);
if (is_empty)
- strbuf_addf(&buf, " %c empty",
+ strbuf_addf(&buf, " %s empty",
comment_line_char);
FLEX_ALLOC_STR(entry, string, buf.buf);
@@ -5750,7 +5752,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
entry = oidmap_get(&state.commit2label, &commit->object.oid);
if (entry)
- strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
+ strbuf_addf(out, "\n%s Branch %s\n", comment_line_char, entry->string);
else
strbuf_addch(out, '\n');
@@ -5887,7 +5889,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
oid_to_hex(&commit->object.oid));
pretty_print_commit(&pp, commit, out);
if (is_empty)
- strbuf_addf(out, " %c empty", comment_line_char);
+ strbuf_addf(out, " %s empty", comment_line_char);
strbuf_addch(out, '\n');
}
if (skipped_commit)
diff --git a/strbuf.c b/strbuf.c
index 7827178d8e..5d2a32d8f0 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -24,6 +24,17 @@ int istarts_with(const char *str, const char *prefix)
return 0;
}
+int starts_with_mem(const char *str, size_t len, const char *prefix)
+{
+ const char *end = str + len;
+ for (; ; str++, prefix++) {
+ if (!*prefix)
+ return 1;
+ else if (str == end || *str != *prefix)
+ return 0;
+ }
+}
+
int skip_to_optional_arg_default(const char *str, const char *prefix,
const char **arg, const char *def)
{
@@ -340,18 +351,17 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
}
static void add_lines(struct strbuf *out,
- const char *prefix1,
- const char *prefix2,
- const char *buf, size_t size)
+ const char *prefix,
+ const char *buf, size_t size,
+ int space_after_prefix)
{
while (size) {
- const char *prefix;
const char *next = memchr(buf, '\n', size);
next = next ? (next + 1) : (buf + size);
- prefix = ((prefix2 && (buf[0] == '\n' || buf[0] == '\t'))
- ? prefix2 : prefix1);
strbuf_addstr(out, prefix);
+ if (space_after_prefix && buf[0] != '\n' && buf[0] != '\t')
+ strbuf_addch(out, ' ');
strbuf_add(out, buf, next - buf);
size -= next - buf;
buf = next;
@@ -360,19 +370,12 @@ static void add_lines(struct strbuf *out,
}
void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
- size_t size, char comment_line_char)
+ size_t size, const char *comment_line_char)
{
- static char prefix1[3];
- static char prefix2[2];
-
- if (prefix1[0] != comment_line_char) {
- xsnprintf(prefix1, sizeof(prefix1), "%c ", comment_line_char);
- xsnprintf(prefix2, sizeof(prefix2), "%c", comment_line_char);
- }
- add_lines(out, prefix1, prefix2, buf, size);
+ add_lines(out, comment_line_char, buf, size, 1);
}
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
+void strbuf_commented_addf(struct strbuf *sb, const char *comment_line_char,
const char *fmt, ...)
{
va_list params;
@@ -750,7 +753,7 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
void strbuf_add_lines(struct strbuf *out, const char *prefix,
const char *buf, size_t size)
{
- add_lines(out, prefix, NULL, buf, size);
+ add_lines(out, prefix, buf, size, 0);
}
void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s)
@@ -1005,10 +1008,10 @@ static size_t cleanup(char *line, size_t len)
*
* If last line does not have a newline at the end, one is added.
*
- * Pass a non-NUL comment_line_char to skip every line starting
+ * Pass a non-NULL comment_line_char to skip every line starting
* with it.
*/
-void strbuf_stripspace(struct strbuf *sb, char comment_line_char)
+void strbuf_stripspace(struct strbuf *sb, const char *comment_line_char)
{
size_t empties = 0;
size_t i, j, len, newlen;
@@ -1022,7 +1025,7 @@ void strbuf_stripspace(struct strbuf *sb, char comment_line_char)
len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
if (comment_line_char && len &&
- sb->buf[i] == comment_line_char) {
+ starts_with(sb->buf + i, comment_line_char)) {
newlen = 0;
continue;
}
diff --git a/strbuf.h b/strbuf.h
index e959caca87..b310a55095 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -288,7 +288,7 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
*/
void strbuf_add_commented_lines(struct strbuf *out,
const char *buf, size_t size,
- char comment_line_char);
+ const char *comment_line_char);
/**
@@ -379,7 +379,7 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
* blank to the buffer.
*/
__attribute__((format (printf, 3, 4)))
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char, const char *fmt, ...);
+void strbuf_commented_addf(struct strbuf *sb, const char *comment_line_char, const char *fmt, ...);
__attribute__((format (printf,2,0)))
void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
@@ -517,7 +517,7 @@ int strbuf_normalize_path(struct strbuf *sb);
* then lines beginning with that character are considered comments,
* thus removed.
*/
-void strbuf_stripspace(struct strbuf *buf, char comment_line_char);
+void strbuf_stripspace(struct strbuf *buf, const char *comment_line_char);
static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
{
@@ -673,6 +673,7 @@ char *xstrfmt(const char *fmt, ...);
int starts_with(const char *str, const char *prefix);
int istarts_with(const char *str, const char *prefix);
+int starts_with_mem(const char *str, size_t len, const char *prefix);
/*
* If the string "str" is the same as the string in "prefix", then the "arg"
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index a3c18a4fc2..10ed8b32bc 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1403,7 +1403,9 @@ test_expect_success "status (core.commentchar with submodule summary)" '
test_expect_success "status (core.commentchar with two chars with submodule summary)" '
test_config core.commentchar ";;" &&
- test_must_fail git -c status.displayCommentPrefix=true status
+ sed "s/^/;/" <expect >expect.double &&
+ git -c status.displayCommentPrefix=true status >output &&
+ test_cmp expect.double output
'
test_expect_success "--ignore-submodules=all suppresses submodule summary" '
diff --git a/trailer.c b/trailer.c
index ef9df4af55..b3edcfe695 100644
--- a/trailer.c
+++ b/trailer.c
@@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
/* The first paragraph is the title and cannot be trailers */
for (s = buf; s < buf + len; s = next_line(s)) {
- if (s[0] == comment_line_char)
+ if (starts_with_mem(s, buf + len - s, comment_line_char))
continue;
if (is_blank_line(s))
break;
@@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
const char **p;
ssize_t separator_pos;
- if (bol[0] == comment_line_char) {
+ if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_char)) {
non_trailer_lines += possible_continuation_lines;
possible_continuation_lines = 0;
continue;
@@ -1013,7 +1013,7 @@ static void parse_trailers(struct trailer_info *info,
for (i = 0; i < info->trailer_nr; i++) {
int separator_pos;
char *trailer = info->trailers[i];
- if (trailer[0] == comment_line_char)
+ if (starts_with(trailer, comment_line_char))
continue;
separator_pos = find_separator(trailer, separators);
if (separator_pos >= 1) {
diff --git a/wt-status.c b/wt-status.c
index b5a29083df..dfe120a559 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -70,7 +70,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
strbuf_vaddf(&sb, fmt, ap);
if (!sb.len) {
if (s->display_comment_prefix) {
- strbuf_addch(&sb, comment_line_char);
+ strbuf_addstr(&sb, comment_line_char);
if (!trail)
strbuf_addch(&sb, ' ');
}
@@ -85,7 +85,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
strbuf_reset(&linebuf);
if (at_bol && s->display_comment_prefix) {
- strbuf_addch(&linebuf, comment_line_char);
+ strbuf_addstr(&linebuf, comment_line_char);
if (*line != '\n' && *line != '\t')
strbuf_addch(&linebuf, ' ');
}
@@ -1090,7 +1090,7 @@ size_t wt_status_locate_end(const char *s, size_t len)
const char *p;
struct strbuf pattern = STRBUF_INIT;
- strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
+ strbuf_addf(&pattern, "\n%s %s", comment_line_char, cut_line);
if (starts_with(s, pattern.buf + 1))
len = 0;
else if ((p = strstr(s, pattern.buf)))
@@ -1176,8 +1176,6 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
struct strbuf sb = STRBUF_INIT;
const char *cp, *ep, *branch_name;
struct branch *branch;
- char comment_line_string[3];
- int i;
uint64_t t_begin = 0;
assert(s->branch && !s->is_initial);
@@ -1202,19 +1200,14 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
}
}
- i = 0;
- if (s->display_comment_prefix) {
- comment_line_string[i++] = comment_line_char;
- comment_line_string[i++] = ' ';
- }
- comment_line_string[i] = '\0';
-
for (cp = sb.buf; (ep = strchr(cp, '\n')) != NULL; cp = ep + 1)
color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s),
- "%s%.*s", comment_line_string,
+ "%s%s%.*s",
+ s->display_comment_prefix ? comment_line_char : "",
+ s->display_comment_prefix ? " " : "",
(int)(ep - cp), cp);
if (s->display_comment_prefix)
- color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%c",
+ color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%s",
comment_line_char);
else
fputs("\n", s->fp);
@@ -1382,7 +1375,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)
git_path("%s", fname));
}
while (!strbuf_getline_lf(&line, f)) {
- if (line.len && line.buf[0] == comment_line_char)
+ if (starts_with(line.buf, comment_line_char))
continue;
strbuf_trim(&line);
if (!line.len)
^ permalink raw reply related
* Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe
From: Jean-Noël Avila @ 2024-03-06 8:13 UTC (permalink / raw)
To: Atneya Nair, git; +Cc: gitster, jeffhost, me, nasamuffin
In-Reply-To: <20240305012112.1598053-3-atneya@google.com>
Le 05/03/2024 à 02:21, Atneya Nair a écrit :
<snip>
> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>
> /*
> * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found. The return value comes from
> + * return path to git directory if found. If passed a valid strbuf, the return
> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
> * a shared buffer.
> *
> * On failure, if return_error_code is not NULL, return_error_code
> @@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
> * return_error_code is NULL the function will die instead (for most
> * cases).
> */
> -const char *read_gitfile_gently(const char *path, int *return_error_code)
> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
> {
> const int max_file_size = 1 << 20; /* 1MB */
> int error_code = 0;
> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> struct stat st;
> int fd;
> ssize_t len;
> - static struct strbuf realpath = STRBUF_INIT;
> + static struct strbuf shared = STRBUF_INIT;
> + if (!result_buf) {
> + result_buf = &shared;
> + }
>
Question about general style: is it accepted practice to override a
parameter of a function?
I would have written:
@@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const
char *path, const char *dir)
* return_error_code is NULL the function will die instead (for most
* cases).
*/
-const char *read_gitfile_gently(const char *path, int *return_error_code)
+const char *read_gitfile_gently(const char *path, int
*return_error_code, struct strbuf* input_buf)
{
const int max_file_size = 1 << 20; /* 1MB */
int error_code = 0;
@@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path,
int *return_error_code)
struct stat st;
int fd;
ssize_t len;
- static struct strbuf realpath = STRBUF_INIT;
+ static struct strbuf shared = STRBUF_INIT;
+ struct strbuf* result_buf;
+ if (!input_buf) {
+ result_buf = &shared;
+ } else {
+ result_buf = input_buf;
+ }
> if (stat(path, &st)) {
> /* NEEDSWORK: discern between ENOENT vs other errors */
> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> goto cleanup_return;
> }
>
> - strbuf_realpath(&realpath, dir, 1);
> - path = realpath.buf;
> + strbuf_realpath(result_buf, dir, 1);
> + path = result_buf->buf;
> + // TODO is this valid?
> + if (!path) die(_("Unexpected null from realpath '%s'"), dir);
In fact, this is not a null path, but an empty path (null is not part of
the string).
By the way, shouldn't this be an internal bug instead of a message to
the user?
Thanks
^ permalink raw reply
* Tc
From: Rifani mei @ 2024-03-06 8:34 UTC (permalink / raw)
To: git, Ardi Syah Asen
Dikirim dari iPhone saya
^ 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