* [PATCH 00/22] avoid redundant pipelines
From: Beat Bolli @ 2024-03-05 21:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Beat Bolli, René Scharfe, Elijah Newren,
Philippe Blain, Johannes Schindelin
As follow-up to [1], here's a series that eliminates redundant
pipelines, mostly under t/.
Spawning a process is notoriously slow on Windows, so this will improve
test performance.
1/22 to 14/22 remove redundant uses of "cat" where the shell handles
input redirection from a file or the command can take a file argument.
15/22 to 20/22 merge redundant uses of "grep" into the following "sed"
or "awk" command. Both of these are capable of filtering themselves. I'm
a bit on the fence about this part because readability suffers in some
cases. It was a fun exercise, though :-)
21/22 merges multiple "sed" calls into one and replaces a whole "cat |
grep | awk" construct with a single "awk" call and uses "sort -u"
instead of "sort | uniq".
22/22 finally is a bit of an outlier in that it replaces a subshell with
a shell compound command.
In the tests, I have completely arbitrarily separated the commits into
thousands blocks to keep the commits and their reviews manageable.
All tests that I was able to run still pass. I don't have p4 or cvs
installed.
[1] https://lore.kernel.org/git/20240216171046.927552-1-dev+git@drbeat.li/
Beat Bolli (22):
doc: avoid redundant use of cat
contrib/subtree/t: avoid redundant use of cat
t/lib-cvs.sh: avoid redundant use of cat
t/annotate-tests.sh: avoid redundant use of cat
t/perf: avoid redundant use of cat
t/t0*: avoid redundant uses of cat
t/t1*: avoid redundant uses of cat
t/t3*: avoid redundant uses of cat
t/t4*: avoid redundant uses of cat
t/t5*: avoid redundant uses of cat
t/t6*: avoid redundant uses of cat
t/t7*: avoid redundant use of cat
t/t8*: avoid redundant use of cat
t/t9*: avoid redundant uses of cat
t/t1*: merge a "grep | sed" pipeline
t/t3*: merge a "grep | awk" pipeline
t/t4*: merge a "grep | sed" pipeline
t/t5*: merge a "grep | sed" pipeline
t/t8*: merge "grep | sed" pipelines
t/t9*: merge "grep | sed" pipelines
contrib/coverage-diff: avoid redundant pipelines
git-quiltimport: avoid an unnecessary subshell
Documentation/howto/update-hook-example.txt | 4 +--
contrib/coverage-diff.sh | 9 ++----
contrib/subtree/t/t7900-subtree.sh | 2 +-
git-quiltimport.sh | 2 +-
t/annotate-tests.sh | 2 +-
t/lib-cvs.sh | 4 +--
t/perf/repos/inflate-repo.sh | 2 +-
t/t0002-gitfile.sh | 2 +-
t/t0011-hashmap.sh | 2 +-
t/t0028-working-tree-encoding.sh | 4 +--
t/t0204-gettext-reencode-sanity.sh | 2 +-
t/t1007-hash-object.sh | 6 ++--
t/t1091-sparse-checkout-builtin.sh | 2 +-
t/t1509/prepare-chroot.sh | 2 +-
t/t3200-branch.sh | 2 +-
t/t3321-notes-stripspace.sh | 8 ++---
t/t3920-crlf-messages.sh | 4 +--
t/t4002-diff-basic.sh | 2 +-
t/t4020-diff-external.sh | 2 +-
t/t4150-am.sh | 2 +-
t/t4205-log-pretty-formats.sh | 2 +-
t/t4301-merge-tree-write-tree.sh | 8 ++---
t/t5100-mailinfo.sh | 2 +-
t/t5317-pack-objects-filter-objects.sh | 2 +-
t/t5401-update-hooks.sh | 2 +-
t/t5534-push-signed.sh | 2 +-
t/t6112-rev-list-filters-objects.sh | 2 +-
t/t6413-merge-crlf.sh | 4 +--
t/t7704-repack-cruft.sh | 2 +-
t/t8010-cat-file-filters.sh | 2 +-
t/t8013-blame-ignore-revs.sh | 28 ++++++++---------
t/t9118-git-svn-funky-branch-names.sh | 2 +-
t/t9300-fast-import.sh | 10 +++---
t/t9350-fast-export.sh | 4 +--
t/t9400-git-cvsserver-server.sh | 35 ++++++++++-----------
t/t9802-git-p4-filetype.sh | 2 +-
t/t9807-git-p4-submit.sh | 2 +-
t/t9824-git-p4-git-lfs.sh | 4 +--
38 files changed, 86 insertions(+), 94 deletions(-)
--
2.44.0
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: Junio C Hamano @ 2024-03-05 21:29 UTC (permalink / raw)
To: John Cai; +Cc: John Cai via GitGitGadget, git, Patrick Steinhardt
In-Reply-To: <44609CA3-1215-4EAD-8456-DDD66A3B93A8@gmail.com>
John Cai <johncai86@gmail.com> writes:
> I think something like --no-dereference that was suggested in [1] could work
> since the concept of dereferencing should be familiar to the user. However, this
> maybe confusing because of the existing --dereference flag that is specific to
> tags...
True.
As a symbolic reference is like a symbolic link [*], another possibility
is to use "follow", which often is used to refer to the action of
reading the contents of a symbolic link and using it as the target
pathname (e.g., O_NOFOLLOW flag forbids open(2) from following
symbolic links). Unfortunately the checkbox feature "--follow" the
"git log" has refers to an entirely different kind of following, so
it is just as confusing as --dereference.
Perhaps "--readlink", if you are showing only "ref: refs/heads/main"
instead of the usual object name in the output? If we are showing
both, we may want to use a name that signals the optional nature of
the symref part of the output, e.g., "--with-symref-target" [*].
[Footnote]
* In fact that is how the symbolic reference started out as. We
literally used a symbolic link .git/HEAD that points at the
current branch, which can be seen in v0.99~418 (git-init-db: set
up the full default environment, 2005-05-30).
* Yes, I know, I am terrible at naming things---my names may be
descriptive but end up being unusably long and verbose.
^ permalink raw reply
* Re: [PATCH 1/2] ci: also define CXX environment variable
From: Junio C Hamano @ 2024-03-05 21:37 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git
In-Reply-To: <75f98cbf98005b0a069977096ec5501f2f7830fe.1709673020.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> 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.
Sounds fair. It's not like we as the project decides to never build
fuzzers on macOS and will forbid others from doing so. Those who
are not part of "we" are welcome to add support to build fuzzers on
other platforms. So perhaps
We only plan on building fuzzers on Linux with the next patch,
so for now, only adjust configuration for the Linux CI jobs.
may convey our intention better to our future selves.
Thanks.
^ permalink raw reply
* Perorg
From: Rifani mei @ 2024-03-05 21:40 UTC (permalink / raw)
To: git
Dikirim dari iPhone saya
^ permalink raw reply
* git
From: Rifani mei @ 2024-03-05 21:40 UTC (permalink / raw)
To: git
Dikirim dari iPhone saya
^ permalink raw reply
* Re: [PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux
From: Junio C Hamano @ 2024-03-05 21:44 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git
In-Reply-To: <eef15e3d3da3ca6953fa8bf3ade190da8e68bf46.1709673020.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we
> have compiled object files for the fuzz tests as part of the default
> 'make all' target. This helps prevent bit-rot in lesser-used parts of
> the codebase, by making sure that incompatible changes are caught at
> build time.
>
> However, since we never linked the fuzzer executables, this did not
> protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test
> build rules, 2024-01-19), it's now possible to link the fuzzer
> executables without using a fuzzing engine and a variety of
> compiler-specific (and compiler-version-specific) flags, at least on
> Linux. So let's add a platform-specific option in config.mak.uname to
> link the executables as part of the default `make all` target.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> Makefile | 14 +++++++++++---
> config.mak.uname | 1 +
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 4e255c81f2..f74e96d7c2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -409,6 +409,9 @@ include shared.mak
> # to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
> # that implements the `fsm_os_settings__*()` routines.
> #
> +# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test
> +# programs in oss-fuzz/.
> +#
> # === Optional library: libintl ===
> #
> # Define NO_GETTEXT if you don't want Git output to be translated.
> @@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
> .PHONY: fuzz-objs
> fuzz-objs: $(FUZZ_OBJS)
>
> -# Always build fuzz objects even if not testing, to prevent bit-rot.
> -all:: $(FUZZ_OBJS)
> -
> FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))
>
> # Empty...
> @@ -2368,6 +2368,14 @@ ifndef NO_TCLTK
> endif
> $(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
>
> +# Build fuzz programs if possible, or at least compile the object files; even
> +# without the necessary fuzzing support, this prevents bit-rot.
> +ifdef LINK_FUZZ_PROGRAMS
> +all:: $(FUZZ_PROGRAMS)
> +else
> +all:: $(FUZZ_OBJS)
> +endif
It would have been easier on the eyes if we had the fuzz things
together, perhaps like this simplified version? We build FUZZ_OBJS
either way, and when the LINK_FUZZ_PROGRAMS is requested, we follow
the fuzz-all recipe, too.
diff --git c/Makefile w/Makefile
index 4e255c81f2..46e457a7a8 100644
--- c/Makefile
+++ w/Makefile
@@ -409,6 +409,9 @@ include shared.mak
# to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
# that implements the `fsm_os_settings__*()` routines.
#
+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test
+# programs in oss-fuzz/.
+#
# === Optional library: libintl ===
#
# Define NO_GETTEXT if you don't want Git output to be translated.
@@ -766,6 +769,12 @@ fuzz-objs: $(FUZZ_OBJS)
# Always build fuzz objects even if not testing, to prevent bit-rot.
all:: $(FUZZ_OBJS)
+# Build fuzz programs, even without the necessary fuzzing support,
+# this prevents bit-rot.
+ifdef LINK_FUZZ_PROGRAMS
+all:: fuzz-all
+endif
+
FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))
# Empty...
^ permalink raw reply related
* Re: [PATCH v2 1/1] completion: don't complete revs when --no-format-patch
From: Junio C Hamano @ 2024-03-05 21:58 UTC (permalink / raw)
To: Britton Kerin; +Cc: Patrick Steinhardt, git
In-Reply-To: <CAC4O8c88Z3ZqxH2VVaNPpEGB3moL5dJcg3cOWuLWwQ_hLrJMtA@mail.gmail.com>
Britton Kerin <britton.kerin@gmail.com> writes:
> On Wed, Feb 7, 2024 at 10:57 PM Patrick Steinhardt <ps@pks.im> wrote:
>> ...
>> Also, adding a test for git-send-email that exercises this new behaviour
>> would be very much welcome, too.
>
> I'll look this one over again and add some tests eventually.
Just pinging the thread to keep it visible in the list of recent
topics I need to keep an eye on. No rush.
Thanks.
^ permalink raw reply
* Re: [Improvements on messages 0/5] Disambuiguate between options and commands
From: Junio C Hamano @ 2024-03-05 22:05 UTC (permalink / raw)
To: git; +Cc: Alexander Shopov, worldhello.net, Alex Henrie
In-Reply-To: <20240216101647.28837-1-ash@kambanaria.org>
Alexander Shopov <ash@kambanaria.org> writes:
> These are nice to be merged but this is not urgent.
>
> Alexander Shopov (5):
> rebase: trivial fix of error message
> transport-helper.c: trivial fix of error message
> builtin/remote.c: trivial fix of error message
> builtin/clone.c: trivial fix of message
> revision.c: trivial fix to message
The first one got commented on with a suggestion to drop it; any
comments on other patches, anybody?
If not, I'll drop the first patch and merge the remainder down to
'next'.
Thanks.
^ permalink raw reply
* [PATCH v3] tests: modernize the test script t0010-racy-git.sh
From: Aryan Gupta via GitGitGadget @ 2024-03-05 22:09 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ]
In-Reply-To: <pull.1675.v2.git.1709243831190.gitgitgadget@gmail.com>
From: Aryan Gupta <garyan447@gmail.com>
Modernize the formatting of the test script to align with current
standards and improve its overall readability.
Signed-off-by: Aryan Gupta <garyan447@gmail.com>
---
[GSOC][PATCH] Modernize a test script
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1675
Range-diff vs v2:
1: fa381d0b57a ! 1: 05ee9e8a458 tests: modernize the test script t0010-racy-git.sh
@@ Commit message
Signed-off-by: Aryan Gupta <garyan447@gmail.com>
## t/t0010-racy-git.sh ##
-@@
- #!/bin/sh
+@@ t/t0010-racy-git.sh: TEST_PASSES_SANITIZE_LEAK=true
--test_description='racy GIT'
-+test_description='racy git'
-
- TEST_PASSES_SANITIZE_LEAK=true
- . ./test-lib.sh
-@@ t/t0010-racy-git.sh: do
- echo xyzzy >infocom
-
- files=$(git diff-files -p)
+ for trial in 0 1 2 3 4
+ do
+- rm -f .git/index
+- echo frotz >infocom
+- git update-index --add infocom
+- echo xyzzy >infocom
+-
+- files=$(git diff-files -p)
- test_expect_success \
- "Racy GIT trial #$trial part A" \
- 'test "" != "$files"'
+-
+ test_expect_success "Racy git trial #$trial part A" '
-+ test "" != "$files"
++ rm -f .git/index &&
++ echo frotz >infocom &&
++ git update-index --add infocom &&
++ echo xyzzy >infocom &&
++
++ git diff-files -p >out &&
++ test_file_not_empty out
+ '
-
sleep 1
- echo xyzzy >cornerstone
- git update-index --add cornerstone
+- echo xyzzy >cornerstone
+- git update-index --add cornerstone
- files=$(git diff-files -p)
+- files=$(git diff-files -p)
- test_expect_success \
- "Racy GIT trial #$trial part B" \
- 'test "" != "$files"'
--
+ test_expect_success "Racy git trial #$trial part B" '
-+ test "" != "$files"
++ echo xyzzy >cornerstone &&
++ git update-index --add cornerstone &&
+
++ git diff-files -p >out &&
++ test_file_not_empty out
+ '
done
t/t0010-racy-git.sh | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh
index 837c8b7228b..84172a37390 100755
--- a/t/t0010-racy-git.sh
+++ b/t/t0010-racy-git.sh
@@ -10,25 +10,24 @@ TEST_PASSES_SANITIZE_LEAK=true
for trial in 0 1 2 3 4
do
- rm -f .git/index
- echo frotz >infocom
- git update-index --add infocom
- echo xyzzy >infocom
-
- files=$(git diff-files -p)
- test_expect_success \
- "Racy GIT trial #$trial part A" \
- 'test "" != "$files"'
-
+ test_expect_success "Racy git trial #$trial part A" '
+ rm -f .git/index &&
+ echo frotz >infocom &&
+ git update-index --add infocom &&
+ echo xyzzy >infocom &&
+
+ git diff-files -p >out &&
+ test_file_not_empty out
+ '
sleep 1
- echo xyzzy >cornerstone
- git update-index --add cornerstone
- files=$(git diff-files -p)
- test_expect_success \
- "Racy GIT trial #$trial part B" \
- 'test "" != "$files"'
+ test_expect_success "Racy git trial #$trial part B" '
+ echo xyzzy >cornerstone &&
+ git update-index --add cornerstone &&
+ git diff-files -p >out &&
+ test_file_not_empty out
+ '
done
test_done
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/4] lockfile: report when rollback fails
From: Justin Tobler @ 2024-03-05 22:09 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <1acaa9ca1a53e3d8e875fed7ce961d0b5426688f.1709549619.git.ps@pks.im>
On 24/03/04 12:10PM, Patrick Steinhardt wrote:
> We do not report to the caller when rolling back a lockfile fails, which
> will be needed by the reftable compaction logic in a subsequent commit.
> It also cannot really report on all errors because the function calls
> `delete_tempfile()`, which doesn't return an error either.
>
> Refactor the code so that both `delete_tempfile()` and
> `rollback_lock_file()` return an error code.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> lockfile.h | 4 ++--
> tempfile.c | 21 +++++++++++++--------
> tempfile.h | 2 +-
> 3 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/lockfile.h b/lockfile.h
> index 90af4e66b2..4ed570d3f7 100644
> --- a/lockfile.h
> +++ b/lockfile.h
> @@ -323,9 +323,9 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)
> * for a `lock_file` object that has already been committed or rolled
> * back.
> */
> -static inline void rollback_lock_file(struct lock_file *lk)
> +static inline int rollback_lock_file(struct lock_file *lk)
> {
> - delete_tempfile(&lk->tempfile);
> + return delete_tempfile(&lk->tempfile);
> }
question: For a lockfile that is already committed or rolled back, is
the underlying tempfile still active? I'm trying to figure out if it
possible for an error to be returned in this scenerio. If not, it might
be nice to clarify in the comment above that, in addition to being a
NOOP, no error is returned.
-Justin
^ permalink raw reply
* Re: [PATCH 01/22] doc: avoid redundant use of cat
From: Junio C Hamano @ 2024-03-05 22:24 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-2-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> The update-hook-example.txt script uses this anti-pattern twice. Call grep
> with the input file name directy. While at it, merge the two consecutive
> grep calls.
OK.
While at it, we could also place $allowed_users_file inside a pair
of double quotes, as the "test -f" before the part we are touching
is prepared to have $IFS whitespace in it (perhaps inside $GIT_DIR),
but I am OK to leave it as a #leftoverbit outside this topic.
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> Documentation/howto/update-hook-example.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/howto/update-hook-example.txt b/Documentation/howto/update-hook-example.txt
> index 151ee84cebce..4e727deedd21 100644
> --- a/Documentation/howto/update-hook-example.txt
> +++ b/Documentation/howto/update-hook-example.txt
> @@ -100,7 +100,7 @@ info "The user is: '$username'"
>
> if test -f "$allowed_users_file"
> then
> - rc=$(cat $allowed_users_file | grep -v '^#' | grep -v '^$' |
> + rc=$(grep -Ev '^(#|$)' $allowed_users_file |
> while read heads user_patterns
> do
> # does this rule apply to us?
> @@ -138,7 +138,7 @@ info "'$groups'"
>
> if test -f "$allowed_groups_file"
> then
> - rc=$(cat $allowed_groups_file | grep -v '^#' | grep -v '^$' |
> + rc=$(grep -Ev '^(#|$)' $allowed_groups_file |
> while read heads group_patterns
> do
> # does this rule apply to us?
^ permalink raw reply
* Re: [PATCH 02/22] contrib/subtree/t: avoid redundant use of cat
From: Junio C Hamano @ 2024-03-05 22:25 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-3-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> contrib/subtree/t/t7900-subtree.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index ca4df5be8324..c3bd2a58b941 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -63,7 +63,7 @@ test_create_pre2_32_repo () {
> git -C "$1" log -1 --format=%B HEAD^2 >msg &&
> test_commit -C "$1-sub" --annotate sub2 &&
> git clone --no-local "$1" "$1-clone" &&
> - new_commit=$(cat msg | sed -e "s/$commit/$tag/" | git -C "$1-clone" commit-tree HEAD^2^{tree}) &&
> + new_commit=$(sed -e "s/$commit/$tag/" msg | git -C "$1-clone" commit-tree HEAD^2^{tree}) &&
> git -C "$1-clone" replace HEAD^2 $new_commit
> }
Obviously correct.
^ permalink raw reply
* Re: [PATCH 03/22] t/lib-cvs.sh: avoid redundant use of cat
From: Junio C Hamano @ 2024-03-05 22:26 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-4-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> t/lib-cvs.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
> index 32b347337908..57b9b2db9b3f 100644
> --- a/t/lib-cvs.sh
> +++ b/t/lib-cvs.sh
> @@ -71,8 +71,8 @@ test_cmp_branch_tree () {
> find . -type d -name .git -prune -o -type f -print
> ) | sort >module-git-"$1".list &&
> test_cmp module-cvs-"$1".list module-git-"$1".list &&
> - cat module-cvs-"$1".list | while read f
> + while read f
> do
> test_cmp_branch_file "$1" "$f" || return 1
> - done
> + done <module-cvs-"$1".list
> }
Again, trivially correct.
^ permalink raw reply
* Re: [PATCH 04/22] t/annotate-tests.sh: avoid redundant use of cat
From: Junio C Hamano @ 2024-03-05 22:28 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-5-dev+git@drbeat.li>
"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).
Not worth a reroll, of course, though.
^ permalink raw reply
* Re: [PATCH 05/22] t/perf: avoid redundant use of cat
From: Junio C Hamano @ 2024-03-05 22:29 UTC (permalink / raw)
To: Beat Bolli; +Cc: git, Beat Bolli
In-Reply-To: <20240305212533.12947-6-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> Take care to redirect stdin, otherwise the output of wc would also contain
> the file name.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> t/perf/repos/inflate-repo.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/perf/repos/inflate-repo.sh b/t/perf/repos/inflate-repo.sh
> index fcfc992b5b02..412e4b450b16 100755
> --- a/t/perf/repos/inflate-repo.sh
> +++ b/t/perf/repos/inflate-repo.sh
> @@ -33,7 +33,7 @@ do
> done
>
> git ls-tree -r HEAD >GEN_src_list
> -nr_src_files=$(cat GEN_src_list | wc -l)
> +nr_src_files=$(wc -l <GEN_src_list)
Good thinking to explicitly redirect into the command.
>
> src_branch=$(git symbolic-ref --short HEAD)
^ permalink raw reply
* Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles
From: Justin Tobler @ 2024-03-05 22:30 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <02bf41d419efd00e510a89a405e1b046b166ba20.1709549619.git.ps@pks.im>
On 24/03/04 12:10PM, Patrick Steinhardt wrote:
> We do not register new tables which we're about to add to the stack with
> the tempfile API. Those tables will thus not be deleted in case Git gets
> killed.
>
> Refactor the code to register tables as tempfiles.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> reftable/stack.c | 29 ++++++++++++-----------------
> 1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index b64e55648a..81544fbfa0 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,
> struct strbuf tab_file_name = STRBUF_INIT;
> struct strbuf next_name = STRBUF_INIT;
> struct reftable_writer *wr = NULL;
> + struct tempfile *tab_file = NULL;
> int err = 0;
> - int tab_fd = 0;
> + int tab_fd;
>
> strbuf_reset(&next_name);
> format_name(&next_name, add->next_update_index, add->next_update_index);
> @@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,
> stack_filename(&temp_tab_file_name, add->stack, next_name.buf);
> strbuf_addstr(&temp_tab_file_name, ".temp.XXXXXX");
>
> - tab_fd = mkstemp(temp_tab_file_name.buf);
> - if (tab_fd < 0) {
> + tab_file = mks_tempfile(temp_tab_file_name.buf);
> + if (!tab_file) {
> err = REFTABLE_IO_ERROR;
> goto done;
> }
> if (add->stack->config.default_permissions) {
> - if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {
> + if (chmod(get_tempfile_path(tab_file),
> + add->stack->config.default_permissions)) {
> err = REFTABLE_IO_ERROR;
> goto done;
> }
> }
Since the tempfile is now being created through the tempfile API, I
think the file mode can be set directly through `mks_tempfile_m()`
instead of creating the tempfile and then using chmod. Just something I
thought to mention.
> + tab_fd = get_tempfile_fd(tab_file);
-Justin
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: brian m. carlson @ 2024-03-05 22:48 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Manlio Perillo, git
In-Reply-To: <5e10f1e5-b87f-43cd-ac1e-d7c01b7dad21@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]
On 2024-03-05 at 09:00:06, Kristoffer Haugsbakk wrote:
>
> On Tue, Mar 5, 2024, at 09:43, Manlio Perillo wrote:
> > I sent this email after reading the documentation of "git diff
> > --color-moved=blocks, where the text says:
> >> Blocks of moved text of at least 20 alphanumeric characters are detected greedily.
> >
> > In this case it is not clear if the number of characters are counted
> > as UTF-8 or normal 8bit bytes.
>
> Alphanumeric characters (a-z and A-Z and 0-9) are ASCII. And one ASCII
> char is represented using one byte in UTF-8. This already looks precise
> to me.
I don't believe that's an appropriate definition. é is an alphanumeric
character, as is ç. ½ is numeric. I would argue an alphanumeric
character comprises at least Unicode classes Ll, Lm, Lo, Lt, Lu, and Nd.
Unicode TR#18 agrees with my assessment.
If we wanted to restrict it ASCII, we need to state that explicitly.
Alternately, if the constraint is 20 UTF-8 octets or something else, we
should state that instead.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: [PATCH 14/22] t/t9*: avoid redundant uses of cat
From: Rubén Justo @ 2024-03-05 22:52 UTC (permalink / raw)
To: Beat Bolli, git; +Cc: Junio C Hamano, Beat Bolli
In-Reply-To: <20240305212533.12947-15-dev+git@drbeat.li>
On Tue, Mar 05, 2024 at 10:25:13PM +0100, Beat Bolli wrote:
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> t/t9300-fast-import.sh | 10 +++++-----
> t/t9350-fast-export.sh | 2 +-
> t/t9400-git-cvsserver-server.sh | 35 +++++++++++++++------------------
> t/t9802-git-p4-filetype.sh | 2 +-
> t/t9807-git-p4-submit.sh | 2 +-
> t/t9824-git-p4-git-lfs.sh | 4 ++--
> 6 files changed, 26 insertions(+), 29 deletions(-)
>
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index dbb5042b0b8f..c03adbdd145f 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -986,7 +986,7 @@ test_expect_success 'L: nested tree copy does not corrupt deltas' '
> 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.
> 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.
> '
> @@ -2053,7 +2053,7 @@ test_expect_success 'Q: verify second notes tree' '
> 100644 blob $commit2
> 100644 blob $commit3
> EOF
> - cat expect.unsorted | sort >expect &&
> + sort expect.unsorted >expect &&
Ditto.
> git cat-file -p refs/notes/foobar^^{tree} | sed "s/ [0-9a-f]* / /" >actual &&
> test_cmp expect actual
> '
> @@ -2091,7 +2091,7 @@ test_expect_success 'Q: verify third notes tree' '
> cat >expect.unsorted <<-EOF &&
> 100644 blob $commit1
> EOF
> - cat expect.unsorted | sort >expect &&
> + sort expect.unsorted >expect &&
Ditto.
> git cat-file -p refs/notes/foobar2^{tree} | sed "s/ [0-9a-f]* / /" >actual &&
> test_cmp expect actual
> '
> @@ -2118,7 +2118,7 @@ test_expect_success 'Q: verify fourth notes tree' '
> cat >expect.unsorted <<-EOF &&
> 100644 blob $commit2
> EOF
> - cat expect.unsorted | sort >expect &&
> + sort expect.unsorted >expect &&
Ditto.
^ permalink raw reply
* Re: [PATCH 22/22] git-quiltimport: avoid an unnecessary subshell
From: Rubén Justo @ 2024-03-05 22:55 UTC (permalink / raw)
To: Beat Bolli, git; +Cc: Junio C Hamano, Beat Bolli
In-Reply-To: <20240305212533.12947-23-dev+git@drbeat.li>
On Tue, Mar 05, 2024 at 10:25:20PM +0100, Beat Bolli wrote:
> Merge multiple sed and "grep | awk" invocations, finally use "sort -u"
> instead of "sort | uniq".
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> contrib/coverage-diff.sh | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/contrib/coverage-diff.sh b/contrib/coverage-diff.sh
> index 4ec419f90048..6ce9603568ef 100755
> --- a/contrib/coverage-diff.sh
> +++ b/contrib/coverage-diff.sh
> @@ -74,8 +74,7 @@ do
> sort >uncovered_lines.txt
>
> comm -12 uncovered_lines.txt new_lines.txt |
> - sed -e 's/$/\)/' |
> - sed -e 's/^/ /' >uncovered_new_lines.txt
> + sed -e 's/$/\)/' -e 's/^/ /' >uncovered_new_lines.txt
>
> grep -q '[^[:space:]]' <uncovered_new_lines.txt &&
> echo $file >>coverage-data.txt &&
> @@ -91,11 +90,7 @@ cat coverage-data.txt
>
> echo "Commits introducing uncovered code:"
>
> -commit_list=$(cat coverage-data.txt |
> - grep -E '^[0-9a-f]{7,} ' |
> - awk '{print $1;}' |
> - sort |
> - uniq)
> +commit_list=$(awk '/^[0-9a-f]{7,}/ { print $1 }' coverage-data.txt | sort -u)
OK. Not worth a re-roll of course, but perhaps some wrapping would be
welcomed here.
>
> (
> for commit in $commit_list
> --
> 2.44.0
>
^ permalink raw reply
* Re: [PATCH 00/22] avoid redundant pipelines
From: Rubén Justo @ 2024-03-05 22:58 UTC (permalink / raw)
To: Beat Bolli, git
Cc: Junio C Hamano, Beat Bolli, René Scharfe, Elijah Newren,
Philippe Blain, Johannes Schindelin
In-Reply-To: <20240305212533.12947-1-dev+git@drbeat.li>
On Tue, Mar 05, 2024 at 10:24:59PM +0100, Beat Bolli wrote:
> As follow-up to [1], here's a series that eliminates redundant
> pipelines, mostly under t/.
>
> Spawning a process is notoriously slow on Windows, so this will improve
> test performance.
>
> 1/22 to 14/22 remove redundant uses of "cat" where the shell handles
> input redirection from a file or the command can take a file argument.
>
> 15/22 to 20/22 merge redundant uses of "grep" into the following "sed"
> or "awk" command. Both of these are capable of filtering themselves. I'm
> a bit on the fence about this part because readability suffers in some
> cases. It was a fun exercise, though :-)
>
> 21/22 merges multiple "sed" calls into one and replaces a whole "cat |
> grep | awk" construct with a single "awk" call and uses "sort -u"
> instead of "sort | uniq".
>
> 22/22 finally is a bit of an outlier in that it replaces a subshell with
> a shell compound command.
I have given it a cursory read and it looks a very good improvement.
I've left a couple of nits, none of them deserve a re-roll.
Thank you!
^ permalink raw reply
* Re: [PATCH v3] tests: modernize the test script t0010-racy-git.sh
From: Junio C Hamano @ 2024-03-05 23:02 UTC (permalink / raw)
To: Aryan Gupta via GitGitGadget
Cc: git, Patrick Steinhardt, Michal Suchánek,
Jean-Noël AVILA, Kristoffer Haugsbakk, Eric Sunshine,
Aryan Gupta, Johannes Schindelin
In-Reply-To: <pull.1675.v3.git.1709676557639.gitgitgadget@gmail.com>
"Aryan Gupta via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Aryan Gupta <garyan447@gmail.com>
>
> Modernize the formatting of the test script to align with current
> standards and improve its overall readability.
>
> Signed-off-by: Aryan Gupta <garyan447@gmail.com>
> ---
Both the above proposed commit log message and the proposed changes
to the code look good.
This is a tangent but as can be seen at
https://lore.kernel.org/git/pull.1675.v3.git.1709676557639.gitgitgadget@gmail.com/raw
your message has these headers that are not syntactically correct
and may even cause responses to bounce unless editted.
Cc: "Patrick Steinhardt [ ]" <ps@pks.im>,
"Michal =?UTF-8?Q?Such=C3=A1nek?= [ ]" <msuchanek@suse.de>,
"=?UTF-8?Q?Jean-No=C3=ABl?= AVILA [ ]" <jn.avila@free.fr>,
Kristoffer Haugsbakk <[code@khaugsbakk.name]>,
Eric Sunshine <sunshine@sunshineco.com>,
Aryan Gupta <garyan447@gmail.com>,
Aryan Gupta <garyan447@gmail.com>
I cannot tell if this is a bug in GGG, or a fault of a corrupt pull
request thrown at GGG, but it is annoying that recipients have to
edit the addresses in e-mail header. Can somebody take a look into
it, please (or is it a known issue?).
Thanks.
^ permalink raw reply
* Re: [PATCH 3/4] reftable/stack: register lockfiles during compaction
From: Justin Tobler @ 2024-03-05 23:30 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <45b5c3167fbfd64d8d1e14ed55bae94cb9cba28b.1709549619.git.ps@pks.im>
On 24/03/04 12:10PM, Patrick Steinhardt wrote:
> We do not register any of the locks we acquire when compacting the
> reftable stack via our lockfiles interfaces. These locks will thus not
> be released when Git gets killed.
>
> Refactor the code to register locks as lockfiles.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ...
> + /*
> + * Write the new "tables.list" contents with the compacted table we
> + * have just written. In case the compacted table became empty we
> + * simply skip writing it.
> + */
> + for (i = 0; i < first; i++)
> + strbuf_addf(&tables_list_buf, "%s\n", st->readers[i]->name);
> + if (!is_empty_table)
> + strbuf_addf(&tables_list_buf, "%s\n", new_table_name.buf);
Something not really related to this patch, but I noticed and had a
question about.
If I'm understanding this correctly, when a newly compacted table is
empty, it becomes possible for a range of indexes to no longer exist
within the stack. If this occurs in the middle of the stack, future
compaction will likely combine the tables on either side and restore the
missing index range. If the empty table was at the end of the stack,
would this effectly reset the max index to something lower for future
tables written to the stack? If so, could this lead to issues with
separate concurrent table writes?
> ...
> diff --git a/reftable/system.h b/reftable/system.h
> index 6b74a81514..5d8b6dede5 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -12,7 +12,9 @@ license that can be found in the LICENSE file or at
> /* This header glues the reftable library to the rest of Git */
>
> #include "git-compat-util.h"
> +#include "lockfile.h"
> #include "strbuf.h"
> +#include "tempfile.h"
> #include "hash-ll.h" /* hash ID, sizes.*/
> #include "dir.h" /* remove_dir_recursively, for tests.*/
Naive question, why do we include the headers in `system.h`? I assume
this is because they are common? Are there other benefits to this
indirection?
-Justin
^ permalink raw reply
* Re: [PATCH 00/22] avoid redundant pipelines
From: Junio C Hamano @ 2024-03-05 23:51 UTC (permalink / raw)
To: Beat Bolli
Cc: git, Beat Bolli, René Scharfe, Elijah Newren, Philippe Blain,
Johannes Schindelin
In-Reply-To: <20240305212533.12947-1-dev+git@drbeat.li>
"Beat Bolli" <bb@drbeat.li> writes:
> As follow-up to [1], here's a series that eliminates redundant
> pipelines, mostly under t/.
>
> Spawning a process is notoriously slow on Windows, so this will improve
> test performance.
Yay!
> 1/22 to 14/22 remove redundant uses of "cat" where the shell handles
> input redirection from a file or the command can take a file argument.
>
> 15/22 to 20/22 merge redundant uses of "grep" into the following "sed"
> or "awk" command. Both of these are capable of filtering themselves. I'm
> a bit on the fence about this part because readability suffers in some
> cases. It was a fun exercise, though :-)
>
> 21/22 merges multiple "sed" calls into one and replaces a whole "cat |
> grep | awk" construct with a single "awk" call and uses "sort -u"
> instead of "sort | uniq".
>
> 22/22 finally is a bit of an outlier in that it replaces a subshell with
> a shell compound command.
>
> In the tests, I have completely arbitrarily separated the commits into
> thousands blocks to keep the commits and their reviews manageable.
>
> All tests that I was able to run still pass. I don't have p4 or cvs
> installed.
>
> [1] https://lore.kernel.org/git/20240216171046.927552-1-dev+git@drbeat.li/
Thanks.
^ permalink raw reply
* Re: [PATCH 04/22] t/annotate-tests.sh: avoid redundant use of cat
From: Rubén Justo @ 2024-03-06 0:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Beat Bolli
In-Reply-To: <xmqqplw8xqio.fsf@gitster.g>
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.
>
> Not worth a reroll, of course, though.
^ permalink raw reply
* 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
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