* Re: [PATCH v2 00/26] pack-objects: multi-pack verbatim reuse
From: Taylor Blau @ 2023-12-15 1:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Patrick Steinhardt
In-Reply-To: <xmqq7clgnvqv.fsf@gitster.g>
On Thu, Dec 14, 2023 at 04:40:40PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I haven't looked into the details yet, but it seems that
> > t5309-pack-delta-cycles.sh fails under
> >
> > $ SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true make -j16 test
>
> Hmph, this seems to be elusive. I tried it again and then it did
> not fail this time.
Indeed, but I was able to reproduce the failure both on my branch and on
'master' under --stress, yielding the following failure in t5309.6:
+ git index-pack --fix-thin --stdin
fatal: REF_DELTA at offset 46 already resolved (duplicate base 01d7713666f4de822776c7622c10f1b07de280dc?)
=================================================================
==3904583==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
#1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180
#2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150
#3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598
#4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51
#5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440
#6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444
#7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).
Aborted
The duplicate base thing is a red-herring, and is an expected result of
the test. But the leak is definitely not, and I'm not sure what's going
on here since the frames listed above are in the LSan runtime, not Git.
I'll try to dig into this a bit more, but I'm not quite sure what's
going on yet.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] tests: prefer host Git to verify chainlint self-checks
From: Patrick Steinhardt @ 2023-12-15 5:33 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Junio C Hamano, git
In-Reply-To: <CAPig+cQvcSeSKVE=0kDyNiSztNAgVwhfAzoL5K7uYHEKe=0f_A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]
On Thu, Dec 14, 2023 at 01:10:48PM -0500, Eric Sunshine wrote:
> On Thu, Dec 14, 2023 at 11:16 AM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > I sent a reply[1] in the other thread explaining why I'm still leaning
> > > toward `sed` to smooth over these minor differences rather than
> > > churning the "expect" files, especially since the minor differences
> > > are not significant to what is actually being tested.
> >
> > If it is just one time bulk conversion under t/chainlint/ to match
> > what the chainlint.pl script produces, with the possibility of
> > similar bulk updates in the future when the script gets updated, I
> > tend to agree with Patrick that getting rid of the fuzzy comparison
> > will be the best way forward.
>
> Okay, that's fine. If we take this approach, though, then it would
> make sense to eliminate _all_ gratuitous postprocessing of the
> "expect" files[1] so that we really are comparing the direct output of
> chainlint.pl with the "expect" files, rather than merely munging the
> inline whitespace of the "expect" files slightly as Patrick's proposed
> patch does[2].
>
> (The only postprocessing of "expect" files which needs to stay is the
> bit which removes the "# LINT:" comments which litter the "expect"
> files explaining to human readers why the linter should insert a
> "???FOO???" annotation at that particular point.)
Okay. I'll send a v3 to also drop the other post-processing steps then.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] tests: drop dependency on `git diff` in check-chainlint
From: Patrick Steinhardt @ 2023-12-15 5:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, git, Eric Sunshine, Eric Sunshine
In-Reply-To: <xmqqo7esohjm.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]
On Thu, Dec 14, 2023 at 08:49:49AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > This strongly reminds me of the thread at [1], where a similar issue was
> > discussed for git-grep(1). Quoting Junio:
> >
> >> I actually do not think these "we are allowing Git tools to be used
> >> on random garbage" is a good idea to begin with X-<. If we invented
> >> something nice for our variant in "git grep" and wish we can use it
> >> outside the repository, contributing the feature to implementations
> >> of "grep" would have been the right way to move forward, instead of
> >> contaminating the codebase with things that are not related to Git.
> >
> > So this might not be the best way to go.
>
> That is not a conclusion I want people to draw.
>
> Like it or not, "git diff --no-index" will be with us to stay, and
> "--no-index" being "we have abused the rest of Git code to implement
> 'diff' that works _outside_ a Git repository---now go and do your
> thing", we would eventually want to correct it, if it is misbehaving
> when a repository it finds is in a shape it does not like, no?
>
> We should have what you quoted in mind as a general principle, and
> think twice when we are tempted to hoard useful features for another
> tool we initially wrote for Git and allow them to be used with the
> "--no-index" option, instead of contributing them to the tool that
> does not know or care "git" repositories (like "diff" and "grep").
Okay, thanks for clarifying!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3] tests: adjust whitespace in chainlint expectations
From: Patrick Steinhardt @ 2023-12-15 6:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <4112adbe467c14a8f22a87ea41aa4705f8760cf6.1702380646.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 19823 bytes --]
The "check-chainlint" target runs automatically when running tests and
performs self-checks to verify that the chainlinter itself produces the
expected output. Originally, the chainlinter was implemented via sed,
but the infrastructure has been rewritten in fb41727b7e (t: retire
unused chainlint.sed, 2022-09-01) to use a Perl script instead.
The rewrite caused some slight whitespace changes in the output that are
ultimately not of much importance. In order to be able to assert that
the actual chainlinter errors match our expectations we thus have to
ignore whitespace characters when diffing them. As the `-w` flag is not
in POSIX we try to use `git diff -w --no-index` before we fall back to
`diff -w -u`.
To accomodate for cases where the host system has no Git installation we
use the locally-compiled version of Git. This can result in problems
though when the Git project's repository is using extensions that the
locally-compiled version of Git doesn't understand. It will refuse to
run and thus cause the checks to fail.
Instead of improving the detection logic, fix our ".expect" files so
that we do not need any post-processing at all anymore. This allows us
to drop the `-w` flag when diffing so that we can always use diff(1)
now.
Note that we leave the post-processing of `chainlint.pl` output intact.
All we do here is to strip leading line numbers that it would otherwise
generate. Having these would cause a rippling effect whenever we add a
new test that sorts into the middle of existing tests and would require
us to renumerate all subsequent lines, which seems rather pointless.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/Makefile | 12 ++-------
t/chainlint/blank-line-before-esac.expect | 12 +++------
t/chainlint/block.expect | 6 ++---
t/chainlint/chain-break-background.expect | 4 +--
t/chainlint/chain-break-continue.expect | 1 -
t/chainlint/chain-break-return-exit.expect | 15 +++++------
t/chainlint/chain-break-status.expect | 5 ++--
t/chainlint/chained-block.expect | 1 -
t/chainlint/chained-subshell.expect | 5 ++--
.../command-substitution-subsubshell.expect | 2 +-
t/chainlint/cuddled.expect | 4 ---
t/chainlint/dqstring-line-splice.expect | 4 +--
t/chainlint/dqstring-no-interpolate.expect | 7 ++---
t/chainlint/empty-here-doc.expect | 4 +--
t/chainlint/exclamation.expect | 2 +-
t/chainlint/for-loop-abbreviated.expect | 2 +-
t/chainlint/function.expect | 6 ++---
t/chainlint/here-doc-indent-operator.expect | 2 --
t/chainlint/here-doc.expect | 7 ++---
| 1 -
t/chainlint/loop-detect-failure.expect | 1 -
t/chainlint/loop-detect-status.expect | 20 +++++++-------
t/chainlint/negated-one-liner.expect | 1 -
t/chainlint/nested-here-doc.expect | 3 ---
t/chainlint/nested-loop-detect-failure.expect | 27 +++++++++----------
t/chainlint/not-heredoc.expect | 1 -
t/chainlint/one-liner.expect | 2 --
t/chainlint/subshell-here-doc.expect | 6 ++---
t/chainlint/token-pasting.expect | 18 +++++--------
29 files changed, 65 insertions(+), 116 deletions(-)
diff --git a/t/Makefile b/t/Makefile
index 225aaf78ed..ab620cf732 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -103,20 +103,12 @@ check-chainlint:
echo "# chainlint: $(CHAINLINTTMP_SQ)/tests" && \
for i in $(CHAINLINTTESTS); do \
echo "# chainlint: $$i" && \
- sed -e '/^[ ]*$$/d' chainlint/$$i.expect; \
+ cat chainlint/$$i.expect; \
done \
} >'$(CHAINLINTTMP_SQ)'/expect && \
$(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
sed -e 's/^[1-9][0-9]* //;/^[ ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
- if test -f ../GIT-BUILD-OPTIONS; then \
- . ../GIT-BUILD-OPTIONS; \
- fi && \
- if test -x ../git$$X; then \
- DIFFW="../git$$X --no-pager diff -w --no-index"; \
- else \
- DIFFW="diff -w -u"; \
- fi && \
- $$DIFFW '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
+ diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
test-lint-filenames
diff --git a/t/chainlint/blank-line-before-esac.expect b/t/chainlint/blank-line-before-esac.expect
index 48ed4eb124..7400a1df7e 100644
--- a/t/chainlint/blank-line-before-esac.expect
+++ b/t/chainlint/blank-line-before-esac.expect
@@ -1,18 +1,14 @@
-test_done ( ) {
+test_done () {
case "$test_failure" in
- 0 )
+ 0)
test_at_end_hook_
-
exit 0 ;;
-
- * )
+ *)
if test $test_external_has_tap -eq 0
then
say_color error "# failed $test_failure among $msg"
say "1..$test_count"
fi
-
exit 1 ;;
-
- esac
+ esac
}
diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect
index a3bcea492a..569211de08 100644
--- a/t/chainlint/block.expect
+++ b/t/chainlint/block.expect
@@ -10,12 +10,10 @@
} ?!AMP?!
baz
) &&
-
{
- echo a ; ?!AMP?! echo b
+ echo a; ?!AMP?! echo b
} &&
-{ echo a ; ?!AMP?! echo b ; } &&
-
+{ echo a; ?!AMP?! echo b; } &&
{
echo "${var}9" &&
echo "done"
diff --git a/t/chainlint/chain-break-background.expect b/t/chainlint/chain-break-background.expect
index 28f9114f42..20d0bb5333 100644
--- a/t/chainlint/chain-break-background.expect
+++ b/t/chainlint/chain-break-background.expect
@@ -1,9 +1,9 @@
JGIT_DAEMON_PID= &&
git init --bare empty.git &&
-> empty.git/git-daemon-export-ok &&
+>empty.git/git-daemon-export-ok &&
mkfifo jgit_daemon_output &&
{
- jgit daemon --port="$JGIT_DAEMON_PORT" . > jgit_daemon_output &
+ jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
JGIT_DAEMON_PID=$!
} &&
test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
diff --git a/t/chainlint/chain-break-continue.expect b/t/chainlint/chain-break-continue.expect
index 47a3457710..fa00c9fdda 100644
--- a/t/chainlint/chain-break-continue.expect
+++ b/t/chainlint/chain-break-continue.expect
@@ -4,7 +4,6 @@ do
test "$path" = "foobar/non-note.txt" && continue
test "$path" = "deadbeef" && continue
test "$path" = "de/adbeef" && continue
-
if test $(expr length "$path") -ne $hexsz
then
return 1
diff --git a/t/chainlint/chain-break-return-exit.expect b/t/chainlint/chain-break-return-exit.expect
index 1732d221c3..bedc0711e4 100644
--- a/t/chainlint/chain-break-return-exit.expect
+++ b/t/chainlint/chain-break-return-exit.expect
@@ -1,18 +1,17 @@
case "$(git ls-files)" in
-one ) echo pass one ;;
-* ) echo bad one ; return 1 ;;
+one) echo pass one ;;
+*) echo bad one; return 1 ;;
esac &&
(
case "$(git ls-files)" in
- two ) echo pass two ;;
- * ) echo bad two ; exit 1 ;;
-esac
+ two) echo pass two ;;
+ *) echo bad two; exit 1 ;;
+ esac
) &&
case "$(git ls-files)" in
-dir/two"$LF"one ) echo pass both ;;
-* ) echo bad ; return 1 ;;
+dir/two"$LF"one) echo pass both ;;
+*) echo bad; return 1 ;;
esac &&
-
for i in 1 2 3 4 ; do
git checkout main -b $i || return $?
test_commit $i $i $i tag$i || return $?
diff --git a/t/chainlint/chain-break-status.expect b/t/chainlint/chain-break-status.expect
index f4bada9463..53b3f42e7a 100644
--- a/t/chainlint/chain-break-status.expect
+++ b/t/chainlint/chain-break-status.expect
@@ -1,7 +1,6 @@
-OUT=$(( ( large_git ; echo $? 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
test_match_signal 13 "$OUT" &&
-
-{ test-tool sigchain > actual ; ret=$? ; } &&
+{ test-tool sigchain >actual; ret=$?; } &&
{
test_match_signal 15 "$ret" ||
test "$ret" = 3
diff --git a/t/chainlint/chained-block.expect b/t/chainlint/chained-block.expect
index 574cdceb07..2416e0fc80 100644
--- a/t/chainlint/chained-block.expect
+++ b/t/chainlint/chained-block.expect
@@ -2,7 +2,6 @@ echo nobody home && {
test the doohicky ?!AMP?!
right now
} &&
-
GIT_EXTERNAL_DIFF=echo git diff | {
read path oldfile oldhex oldmode newfile newhex newmode &&
test "z$oh" = "z$oldhex"
diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect
index af0369d328..2a44845759 100644
--- a/t/chainlint/chained-subshell.expect
+++ b/t/chainlint/chained-subshell.expect
@@ -3,8 +3,7 @@ mkdir sub && (
foo the bar ?!AMP?!
nuff said
) &&
-
-cut "-d " -f actual | ( read s1 s2 s3 &&
+cut "-d " -f actual | (read s1 s2 s3 &&
test -f $s1 ?!AMP?!
test $(cat $s2) = tree2path1 &&
-test $(cat $s3) = tree3path1 )
+test $(cat $s3) = tree3path1)
diff --git a/t/chainlint/command-substitution-subsubshell.expect b/t/chainlint/command-substitution-subsubshell.expect
index ab2f79e845..ec42f2c30c 100644
--- a/t/chainlint/command-substitution-subsubshell.expect
+++ b/t/chainlint/command-substitution-subsubshell.expect
@@ -1,2 +1,2 @@
-OUT=$(( ( large_git 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git 1>&3) | :) 3>&1 ) &&
test_match_signal 13 "$OUT"
diff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect
index c3e0be4047..9558362917 100644
--- a/t/chainlint/cuddled.expect
+++ b/t/chainlint/cuddled.expect
@@ -1,17 +1,13 @@
(cd foo &&
bar
) &&
-
(cd foo ?!AMP?!
bar
) &&
-
(
cd foo &&
bar) &&
-
(cd foo &&
bar) &&
-
(cd foo ?!AMP?!
bar)
diff --git a/t/chainlint/dqstring-line-splice.expect b/t/chainlint/dqstring-line-splice.expect
index bf9ced60d4..f8fa3bc969 100644
--- a/t/chainlint/dqstring-line-splice.expect
+++ b/t/chainlint/dqstring-line-splice.expect
@@ -1,3 +1,3 @@
-echo 'fatal: reword option of --fixup is mutually exclusive with' '--patch/--interactive/--all/--include/--only' > expect &&
-test_must_fail git commit --fixup=reword:HEAD~ $1 2 > actual &&
+echo 'fatal: reword option of --fixup is mutually exclusive with' '--patch/--interactive/--all/--include/--only' >expect &&
+test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
test_cmp expect actual
diff --git a/t/chainlint/dqstring-no-interpolate.expect b/t/chainlint/dqstring-no-interpolate.expect
index 10724987a5..bd60121b47 100644
--- a/t/chainlint/dqstring-no-interpolate.expect
+++ b/t/chainlint/dqstring-no-interpolate.expect
@@ -1,11 +1,8 @@
grep "^ ! [rejected][ ]*$BRANCH -> $BRANCH (non-fast-forward)$" out &&
-
grep "^\.git$" output.txt &&
-
-
(
cd client$version &&
GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. $(cat ../input)
-) > output &&
- cut -d ' ' -f 2 < output | sort > actual &&
+) >output &&
+ cut -d ' ' -f 2 <output | sort >actual &&
test_cmp expect actual
diff --git a/t/chainlint/empty-here-doc.expect b/t/chainlint/empty-here-doc.expect
index e8733c97c6..8507721192 100644
--- a/t/chainlint/empty-here-doc.expect
+++ b/t/chainlint/empty-here-doc.expect
@@ -1,4 +1,4 @@
-git ls-tree $tree path > current &&
-cat > expected <<\EOF &&
+git ls-tree $tree path >current &&
+cat >expected <<\EOF &&
EOF
test_output
diff --git a/t/chainlint/exclamation.expect b/t/chainlint/exclamation.expect
index 2d961a58c6..765a35bb4c 100644
--- a/t/chainlint/exclamation.expect
+++ b/t/chainlint/exclamation.expect
@@ -1,4 +1,4 @@
-if ! condition ; then echo nope ; else yep ; fi &&
+if ! condition; then echo nope; else yep; fi &&
test_prerequisite !MINGW &&
mail uucp!address &&
echo !whatever!
diff --git a/t/chainlint/for-loop-abbreviated.expect b/t/chainlint/for-loop-abbreviated.expect
index a21007a63f..02c0d15cca 100644
--- a/t/chainlint/for-loop-abbreviated.expect
+++ b/t/chainlint/for-loop-abbreviated.expect
@@ -1,5 +1,5 @@
for it
do
- path=$(expr "$it" : ( [^:]*) ) &&
+ path=$(expr "$it" : ([^:]*)) &&
git update-index --add "$path" || exit
done
diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect
index a14388e6b9..826b2ccc95 100644
--- a/t/chainlint/function.expect
+++ b/t/chainlint/function.expect
@@ -1,11 +1,9 @@
-sha1_file ( ) {
+sha1_file() {
echo "$*" | sed "s#..#.git/objects/&/#"
} &&
-
-remove_object ( ) {
+remove_object() {
file=$(sha1_file "$*") &&
test -e "$file" ?!AMP?!
rm -f "$file"
} ?!AMP?!
-
sha1_file arg && remove_object arg
diff --git a/t/chainlint/here-doc-indent-operator.expect b/t/chainlint/here-doc-indent-operator.expect
index f92a7ce999..bbaad024d2 100644
--- a/t/chainlint/here-doc-indent-operator.expect
+++ b/t/chainlint/here-doc-indent-operator.expect
@@ -3,9 +3,7 @@ header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
num_commits: $1
chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data
EOF
-
cat >expect << -EOF ?!AMP?!
this is not indented
-EOF
-
cleanup
diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
index 1df3f78282..9e612ac8b1 100644
--- a/t/chainlint/here-doc.expect
+++ b/t/chainlint/here-doc.expect
@@ -1,22 +1,19 @@
boodle wobba \
- gorgo snoot \
- wafta snurb <<EOF &&
+ gorgo snoot \
+ wafta snurb <<EOF &&
quoth the raven,
nevermore...
EOF
-
cat <<-Arbitrary_Tag_42 >foo &&
snoz
boz
woz
Arbitrary_Tag_42
-
cat <<"zump" >boo &&
snoz
boz
woz
zump
-
horticulture <<\EOF
gomez
morticia
--git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect
index 6bad218530..c7fc411f91 100644
--- a/t/chainlint/inline-comment.expect
+++ b/t/chainlint/inline-comment.expect
@@ -3,6 +3,5 @@
barfoo ?!AMP?! # wrong position for &&
flibble "not a # comment"
) &&
-
(cd foo &&
flibble "not a # comment")
diff --git a/t/chainlint/loop-detect-failure.expect b/t/chainlint/loop-detect-failure.expect
index a66025c39d..7a1a1ff0bf 100644
--- a/t/chainlint/loop-detect-failure.expect
+++ b/t/chainlint/loop-detect-failure.expect
@@ -5,7 +5,6 @@ do
git -C r1 add file.$n &&
git -C r1 commit -m "$n" || return 1
done &&
-
git init r2 &&
for n in 1000 10000
do
diff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect
index 24da9e86d5..7ce3a34806 100644
--- a/t/chainlint/loop-detect-status.expect
+++ b/t/chainlint/loop-detect-status.expect
@@ -1,18 +1,18 @@
-( while test $i -le $blobcount
-do
- printf "Generating blob $i/$blobcount\r" >& 2 &&
+(while test $i -le $blobcount
+ do
+ printf "Generating blob $i/$blobcount\r" >&2 &&
printf "blob\nmark :$i\ndata $blobsize\n" &&
#test-tool genrandom $i $blobsize &&
printf "%-${blobsize}s" $i &&
echo "M 100644 :$i $i" >> commit &&
i=$(($i+1)) ||
echo $? > exit-status
-done &&
-echo "commit refs/heads/main" &&
-echo "author A U Thor <author@email.com> 123456789 +0000" &&
-echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
-echo "data 5" &&
-echo ">2gb" &&
-cat commit ) |
+ done &&
+ echo "commit refs/heads/main" &&
+ echo "author A U Thor <author@email.com> 123456789 +0000" &&
+ echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
+ echo "data 5" &&
+ echo ">2gb" &&
+ cat commit) |
git fast-import --big-file-threshold=2 &&
test ! -f exit-status
diff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect
index ad4c2d949e..7ae60eb770 100644
--- a/t/chainlint/negated-one-liner.expect
+++ b/t/chainlint/negated-one-liner.expect
@@ -1,5 +1,4 @@
! (foo && bar) &&
! (foo && bar) >baz &&
-
! (foo; ?!AMP?! bar) &&
! (foo; ?!AMP?! bar) >baz
diff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect
index 29b3832a98..93eeded9f4 100644
--- a/t/chainlint/nested-here-doc.expect
+++ b/t/chainlint/nested-here-doc.expect
@@ -6,7 +6,6 @@ fub <<EOF
EOF
formp
ARBITRARY
-
(
cat <<-\INPUT_END &&
fish are mice
@@ -17,7 +16,6 @@ ARBITRARY
EOF
toink
INPUT_END
-
cat <<-\EOT ?!AMP?!
text goes here
data <<EOF
@@ -25,6 +23,5 @@ ARBITRARY
EOF
more test here
EOT
-
foobar
)
diff --git a/t/chainlint/nested-loop-detect-failure.expect b/t/chainlint/nested-loop-detect-failure.expect
index 4793a0e8e1..211fdb7479 100644
--- a/t/chainlint/nested-loop-detect-failure.expect
+++ b/t/chainlint/nested-loop-detect-failure.expect
@@ -1,31 +1,28 @@
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
do
- for j in 0 1 2 3 4 5 6 7 8 9 ;
+ for j in 0 1 2 3 4 5 6 7 8 9;
do
- echo "$i$j" > "path$i$j" ?!LOOP?!
+ echo "$i$j" >"path$i$j" ?!LOOP?!
done ?!LOOP?!
done &&
-
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
do
- for j in 0 1 2 3 4 5 6 7 8 9 ;
+ for j in 0 1 2 3 4 5 6 7 8 9;
do
- echo "$i$j" > "path$i$j" || return 1
+ echo "$i$j" >"path$i$j" || return 1
done
done &&
-
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
do
- for j in 0 1 2 3 4 5 6 7 8 9 ;
+ for j in 0 1 2 3 4 5 6 7 8 9;
do
- echo "$i$j" > "path$i$j" ?!LOOP?!
+ echo "$i$j" >"path$i$j" ?!LOOP?!
done || return 1
done &&
-
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
do
- for j in 0 1 2 3 4 5 6 7 8 9 ;
+ for j in 0 1 2 3 4 5 6 7 8 9;
do
- echo "$i$j" > "path$i$j" || return 1
+ echo "$i$j" >"path$i$j" || return 1
done || return 1
done
diff --git a/t/chainlint/not-heredoc.expect b/t/chainlint/not-heredoc.expect
index 2e9bb135fe..47311f4c2d 100644
--- a/t/chainlint/not-heredoc.expect
+++ b/t/chainlint/not-heredoc.expect
@@ -3,7 +3,6 @@ echo ourside &&
echo "=======" &&
echo theirside &&
echo ">>>>>>> theirs" &&
-
(
echo "<<<<<<< ours" &&
echo ourside &&
diff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect
index 57a7a444c1..cdb45231f1 100644
--- a/t/chainlint/one-liner.expect
+++ b/t/chainlint/one-liner.expect
@@ -1,9 +1,7 @@
(foo && bar) &&
(foo && bar) |
(foo && bar) >baz &&
-
(foo; ?!AMP?! bar) &&
(foo; ?!AMP?! bar) |
(foo; ?!AMP?! bar) >baz &&
-
(foo "bar; baz")
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 52789278d1..fa7cb0288a 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -1,15 +1,13 @@
(
echo wobba \
- gorgo snoot \
- wafta snurb <<-EOF &&
+ gorgo snoot \
+ wafta snurb <<-EOF &&
quoth the raven,
nevermore...
EOF
-
cat <<EOF >bip ?!AMP?!
fish fly high
EOF
-
echo <<-\EOF >bop
gomez
morticia
diff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect
index 342360bcd0..cda7d54037 100644
--- a/t/chainlint/token-pasting.expect
+++ b/t/chainlint/token-pasting.expect
@@ -1,26 +1,22 @@
git config filter.rot13.smudge ./rot13.sh &&
git config filter.rot13.clean ./rot13.sh &&
-
{
echo "*.t filter=rot13" ?!AMP?!
echo "*.i ident"
-} > .gitattributes &&
-
+} >.gitattributes &&
{
echo a b c d e f g h i j k l m ?!AMP?!
echo n o p q r s t u v w x y z ?!AMP?!
echo '$Id$'
-} > test &&
-cat test > test.t &&
-cat test > test.o &&
-cat test > test.i &&
+} >test &&
+cat test >test.t &&
+cat test >test.o &&
+cat test >test.i &&
git add test test.t test.i &&
rm -f test test.t test.i &&
git checkout -- test test.t test.i &&
-
-echo "content-test2" > test2.o &&
-echo "content-test3 - filename with special characters" > "test3 'sq',$x=.o" ?!AMP?!
-
+echo "content-test2" >test2.o &&
+echo "content-test3 - filename with special characters" >"test3 'sq',$x=.o" ?!AMP?!
downstream_url_for_sed=$(
printf "%sn" "$downstream_url" |
sed -e 's/\/\\/g' -e 's/[[/.*^$]/\&/g'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v3] tests: adjust whitespace in chainlint expectations
From: Eric Sunshine @ 2023-12-15 6:24 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <8c3e1cb5eae13210070cc14f5f2a3f8c0dfc39c3.1702620230.git.ps@pks.im>
On Fri, Dec 15, 2023 at 1:04 AM Patrick Steinhardt <ps@pks.im> wrote:
> [...]
> Instead of improving the detection logic, fix our ".expect" files so
> that we do not need any post-processing at all anymore. This allows us
> to drop the `-w` flag when diffing so that we can always use diff(1)
> now.
>
> Note that we leave the post-processing of `chainlint.pl` output intact.
> All we do here is to strip leading line numbers that it would otherwise
> generate.
Hmm, okay, but... (see below)
> Having these would cause a rippling effect whenever we add a
> new test that sorts into the middle of existing tests and would require
> us to renumerate all subsequent lines, which seems rather pointless.
Just an aside, not strictly relevant at this time: Ævar has proposed
that check-chainlint should not be creating conglomerate "test",
"expect", and "actual" files, but should instead let `make` run
chainlint.pl separately on each chainlint self-test file, thus
benefiting from `make`'s innate parallelism rather than baking
parallelism into chainlint.pl itself. More importantly, `make`'s
dependency tracking would ensure that a chainlint self-test file only
gets rechecked if its timestamp changes. That differs from the current
situation in which _all_ of the chainlint self-test files are checked
on _every_ `make test` which is wasteful if none of them have changed.
Anyhow, with his proposed approach, there wouldn't be cascading line
number changes just because a new self-test file was added.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/Makefile b/t/Makefile
> @@ -103,20 +103,12 @@ check-chainlint:
> $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
> sed -e 's/^[1-9][0-9]* //;/^[ ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
The commit message claims that this is only stripping the line numbers
which prefix each emitted line, but the `/^[ ]*$$/d` bit is also
deleting blank lines from the output of chainlint.pl. Thus, this ought
to be:
sed -e 's/^[1-9][0-9]* //' >'$(CHAINLINTTMP_SQ)'/actual && \
^ permalink raw reply
* Re: [PATCH v3] tests: adjust whitespace in chainlint expectations
From: Patrick Steinhardt @ 2023-12-15 6:29 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano
In-Reply-To: <CAPig+cQozi+aiTc5Bve4OHrfuSRGUCSkKmhoYtkGTmn64Ps-rw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]
On Fri, Dec 15, 2023 at 01:24:20AM -0500, Eric Sunshine wrote:
> On Fri, Dec 15, 2023 at 1:04 AM Patrick Steinhardt <ps@pks.im> wrote:
> > [...]
> > Instead of improving the detection logic, fix our ".expect" files so
> > that we do not need any post-processing at all anymore. This allows us
> > to drop the `-w` flag when diffing so that we can always use diff(1)
> > now.
> >
> > Note that we leave the post-processing of `chainlint.pl` output intact.
> > All we do here is to strip leading line numbers that it would otherwise
> > generate.
>
> Hmm, okay, but... (see below)
>
> > Having these would cause a rippling effect whenever we add a
> > new test that sorts into the middle of existing tests and would require
> > us to renumerate all subsequent lines, which seems rather pointless.
>
> Just an aside, not strictly relevant at this time: Ævar has proposed
> that check-chainlint should not be creating conglomerate "test",
> "expect", and "actual" files, but should instead let `make` run
> chainlint.pl separately on each chainlint self-test file, thus
> benefiting from `make`'s innate parallelism rather than baking
> parallelism into chainlint.pl itself. More importantly, `make`'s
> dependency tracking would ensure that a chainlint self-test file only
> gets rechecked if its timestamp changes. That differs from the current
> situation in which _all_ of the chainlint self-test files are checked
> on _every_ `make test` which is wasteful if none of them have changed.
> Anyhow, with his proposed approach, there wouldn't be cascading line
> number changes just because a new self-test file was added.
I was indeed also thinking along this way and would tend to agree. I
punted on it as I honestly only really care for fixing the immediate
issue that the post-processing causes for me.
Are you fine with deferring this bigger refactoring?
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/Makefile b/t/Makefile
> > @@ -103,20 +103,12 @@ check-chainlint:
> > $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
> > sed -e 's/^[1-9][0-9]* //;/^[ ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
>
> The commit message claims that this is only stripping the line numbers
> which prefix each emitted line, but the `/^[ ]*$$/d` bit is also
> deleting blank lines from the output of chainlint.pl. Thus, this ought
> to be:
>
> sed -e 's/^[1-9][0-9]* //' >'$(CHAINLINTTMP_SQ)'/actual && \
Gah, you're right, I missed the second part. Will fix in another round.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3] tests: adjust whitespace in chainlint expectations
From: Eric Sunshine @ 2023-12-15 6:40 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <ZXvyL2wtoTIt6OVc@tanuki>
On Fri, Dec 15, 2023 at 1:29 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Fri, Dec 15, 2023 at 01:24:20AM -0500, Eric Sunshine wrote:
> > Just an aside, not strictly relevant at this time: Ævar has proposed
> > that check-chainlint should not be creating conglomerate "test",
> > "expect", and "actual" files, but should instead let `make` run
> > chainlint.pl separately on each chainlint self-test file, thus
> > benefiting from `make`'s innate parallelism rather than baking
> > parallelism into chainlint.pl itself. More importantly, `make`'s
> > dependency tracking would ensure that a chainlint self-test file only
> > gets rechecked if its timestamp changes. That differs from the current
> > situation in which _all_ of the chainlint self-test files are checked
> > on _every_ `make test` which is wasteful if none of them have changed.
> > Anyhow, with his proposed approach, there wouldn't be cascading line
> > number changes just because a new self-test file was added.
>
> I was indeed also thinking along this way and would tend to agree. I
> punted on it as I honestly only really care for fixing the immediate
> issue that the post-processing causes for me.
>
> Are you fine with deferring this bigger refactoring?
Oh, yes, of course. Please don't read my aside-comment as a suggestion
that you should tackle such a change or that there is any urgent need
to change how this all works. The currently proposed simple
solution(s) to allow you to get past this issue are perfectly
acceptable.
(As a further aside, just for completeness since I already mentioned
part of his plan, Ævar also really intended that the test scripts
themselves, "t/t????-*.sh", should be run individually through
chainlint.pl by the Makefile rather than sending all of them through
it in one go, thus once again taking advantage of `make`'s innate
parallelism, and only checking for &&-chain breakage if a test
script's timestamp has actually changed rather than checking all test
scripts unconditionally on every `make test`[1,2,3].)
[1]: https://lore.kernel.org/git/220901.86bkrzjm6e.gmgdl@evledraar.gmail.com/
[2]: https://lore.kernel.org/git/221122.86cz9fbyln.gmgdl@evledraar.gmail.com/
[3]: https://github.com/avar/git/commit/ff8b4a12b30ac5ca521a64e74b0fd968ab2d4585
^ permalink raw reply
* [PATCH v4] tests: adjust whitespace in chainlint expectations
From: Patrick Steinhardt @ 2023-12-15 6:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <4112adbe467c14a8f22a87ea41aa4705f8760cf6.1702380646.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 17894 bytes --]
The "check-chainlint" target runs automatically when running tests and
performs self-checks to verify that the chainlinter itself produces the
expected output. Originally, the chainlinter was implemented via sed,
but the infrastructure has been rewritten in fb41727b7e (t: retire
unused chainlint.sed, 2022-09-01) to use a Perl script instead.
The rewrite caused some slight whitespace changes in the output that are
ultimately not of much importance. In order to be able to assert that
the actual chainlinter errors match our expectations we thus have to
ignore whitespace characters when diffing them. As the `-w` flag is not
in POSIX we try to use `git diff -w --no-index` before we fall back to
`diff -w -u`.
To accomodate for cases where the host system has no Git installation we
use the locally-compiled version of Git. This can result in problems
though when the Git project's repository is using extensions that the
locally-compiled version of Git doesn't understand. It will refuse to
run and thus cause the checks to fail.
Instead of improving the detection logic, fix our ".expect" files so
that we do not need any post-processing at all anymore. This allows us
to drop the `-w` flag when diffing so that we can always use diff(1)
now.
Note that we keep some of the post-processing of `chainlint.pl` output
intact to strip leading line numbers generated by the script. Having
these would cause a rippling effect whenever we add a new test that
sorts into the middle of existing tests and would require us to
renumerate all subsequent lines, which seems rather pointless.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/Makefile | 14 +++--------
t/chainlint/blank-line-before-esac.expect | 8 +++----
t/chainlint/blank-line.expect | 4 ++++
t/chainlint/block.expect | 4 ++--
t/chainlint/chain-break-background.expect | 4 ++--
t/chainlint/chain-break-return-exit.expect | 14 +++++------
t/chainlint/chain-break-status.expect | 4 ++--
t/chainlint/chained-subshell.expect | 4 ++--
.../command-substitution-subsubshell.expect | 2 +-
t/chainlint/dqstring-line-splice.expect | 6 +++--
t/chainlint/dqstring-no-interpolate.expect | 5 ++--
t/chainlint/empty-here-doc.expect | 4 ++--
t/chainlint/exclamation.expect | 2 +-
t/chainlint/for-loop-abbreviated.expect | 2 +-
t/chainlint/for-loop.expect | 1 +
t/chainlint/function.expect | 4 ++--
t/chainlint/here-doc.expect | 4 ++--
t/chainlint/loop-detect-status.expect | 20 ++++++++--------
t/chainlint/nested-cuddled-subshell.expect | 6 +++++
t/chainlint/nested-loop-detect-failure.expect | 24 +++++++++----------
t/chainlint/nested-subshell.expect | 1 +
t/chainlint/pipe.expect | 2 ++
t/chainlint/subshell-here-doc.expect | 4 ++--
t/chainlint/subshell-one-liner.expect | 5 ++++
t/chainlint/t7900-subtree.expect | 1 +
t/chainlint/token-pasting.expect | 14 +++++------
t/chainlint/while-loop.expect | 1 +
27 files changed, 90 insertions(+), 74 deletions(-)
diff --git a/t/Makefile b/t/Makefile
index 225aaf78ed..b7a6fefe28 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -103,20 +103,12 @@ check-chainlint:
echo "# chainlint: $(CHAINLINTTMP_SQ)/tests" && \
for i in $(CHAINLINTTESTS); do \
echo "# chainlint: $$i" && \
- sed -e '/^[ ]*$$/d' chainlint/$$i.expect; \
+ cat chainlint/$$i.expect; \
done \
} >'$(CHAINLINTTMP_SQ)'/expect && \
$(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
- sed -e 's/^[1-9][0-9]* //;/^[ ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
- if test -f ../GIT-BUILD-OPTIONS; then \
- . ../GIT-BUILD-OPTIONS; \
- fi && \
- if test -x ../git$$X; then \
- DIFFW="../git$$X --no-pager diff -w --no-index"; \
- else \
- DIFFW="diff -w -u"; \
- fi && \
- $$DIFFW '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
+ sed -e 's/^[1-9][0-9]* //' >'$(CHAINLINTTMP_SQ)'/actual && \
+ diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
test-lint-filenames
diff --git a/t/chainlint/blank-line-before-esac.expect b/t/chainlint/blank-line-before-esac.expect
index 48ed4eb124..056e03003d 100644
--- a/t/chainlint/blank-line-before-esac.expect
+++ b/t/chainlint/blank-line-before-esac.expect
@@ -1,11 +1,11 @@
-test_done ( ) {
+test_done () {
case "$test_failure" in
- 0 )
+ 0)
test_at_end_hook_
exit 0 ;;
- * )
+ *)
if test $test_external_has_tap -eq 0
then
say_color error "# failed $test_failure among $msg"
@@ -14,5 +14,5 @@ test_done ( ) {
exit 1 ;;
- esac
+ esac
}
diff --git a/t/chainlint/blank-line.expect b/t/chainlint/blank-line.expect
index f76fde1ffb..b47827d749 100644
--- a/t/chainlint/blank-line.expect
+++ b/t/chainlint/blank-line.expect
@@ -1,4 +1,8 @@
(
+
nothing &&
+
something
+
+
)
diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect
index a3bcea492a..1c87326364 100644
--- a/t/chainlint/block.expect
+++ b/t/chainlint/block.expect
@@ -12,9 +12,9 @@
) &&
{
- echo a ; ?!AMP?! echo b
+ echo a; ?!AMP?! echo b
} &&
-{ echo a ; ?!AMP?! echo b ; } &&
+{ echo a; ?!AMP?! echo b; } &&
{
echo "${var}9" &&
diff --git a/t/chainlint/chain-break-background.expect b/t/chainlint/chain-break-background.expect
index 28f9114f42..20d0bb5333 100644
--- a/t/chainlint/chain-break-background.expect
+++ b/t/chainlint/chain-break-background.expect
@@ -1,9 +1,9 @@
JGIT_DAEMON_PID= &&
git init --bare empty.git &&
-> empty.git/git-daemon-export-ok &&
+>empty.git/git-daemon-export-ok &&
mkfifo jgit_daemon_output &&
{
- jgit daemon --port="$JGIT_DAEMON_PORT" . > jgit_daemon_output &
+ jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
JGIT_DAEMON_PID=$!
} &&
test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
diff --git a/t/chainlint/chain-break-return-exit.expect b/t/chainlint/chain-break-return-exit.expect
index 1732d221c3..4cd18e2edf 100644
--- a/t/chainlint/chain-break-return-exit.expect
+++ b/t/chainlint/chain-break-return-exit.expect
@@ -1,16 +1,16 @@
case "$(git ls-files)" in
-one ) echo pass one ;;
-* ) echo bad one ; return 1 ;;
+one) echo pass one ;;
+*) echo bad one; return 1 ;;
esac &&
(
case "$(git ls-files)" in
- two ) echo pass two ;;
- * ) echo bad two ; exit 1 ;;
-esac
+ two) echo pass two ;;
+ *) echo bad two; exit 1 ;;
+ esac
) &&
case "$(git ls-files)" in
-dir/two"$LF"one ) echo pass both ;;
-* ) echo bad ; return 1 ;;
+dir/two"$LF"one) echo pass both ;;
+*) echo bad; return 1 ;;
esac &&
for i in 1 2 3 4 ; do
diff --git a/t/chainlint/chain-break-status.expect b/t/chainlint/chain-break-status.expect
index f4bada9463..e6b3b2193e 100644
--- a/t/chainlint/chain-break-status.expect
+++ b/t/chainlint/chain-break-status.expect
@@ -1,7 +1,7 @@
-OUT=$(( ( large_git ; echo $? 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
test_match_signal 13 "$OUT" &&
-{ test-tool sigchain > actual ; ret=$? ; } &&
+{ test-tool sigchain >actual; ret=$?; } &&
{
test_match_signal 15 "$ret" ||
test "$ret" = 3
diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect
index af0369d328..83810ea7ec 100644
--- a/t/chainlint/chained-subshell.expect
+++ b/t/chainlint/chained-subshell.expect
@@ -4,7 +4,7 @@ mkdir sub && (
nuff said
) &&
-cut "-d " -f actual | ( read s1 s2 s3 &&
+cut "-d " -f actual | (read s1 s2 s3 &&
test -f $s1 ?!AMP?!
test $(cat $s2) = tree2path1 &&
-test $(cat $s3) = tree3path1 )
+test $(cat $s3) = tree3path1)
diff --git a/t/chainlint/command-substitution-subsubshell.expect b/t/chainlint/command-substitution-subsubshell.expect
index ab2f79e845..ec42f2c30c 100644
--- a/t/chainlint/command-substitution-subsubshell.expect
+++ b/t/chainlint/command-substitution-subsubshell.expect
@@ -1,2 +1,2 @@
-OUT=$(( ( large_git 1 >& 3 ) | : ) 3 >& 1) &&
+OUT=$( ((large_git 1>&3) | :) 3>&1 ) &&
test_match_signal 13 "$OUT"
diff --git a/t/chainlint/dqstring-line-splice.expect b/t/chainlint/dqstring-line-splice.expect
index bf9ced60d4..37eab80738 100644
--- a/t/chainlint/dqstring-line-splice.expect
+++ b/t/chainlint/dqstring-line-splice.expect
@@ -1,3 +1,5 @@
-echo 'fatal: reword option of --fixup is mutually exclusive with' '--patch/--interactive/--all/--include/--only' > expect &&
-test_must_fail git commit --fixup=reword:HEAD~ $1 2 > actual &&
+
+echo 'fatal: reword option of --fixup is mutually exclusive with' '--patch/--interactive/--all/--include/--only' >expect &&
+test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&
test_cmp expect actual
+
diff --git a/t/chainlint/dqstring-no-interpolate.expect b/t/chainlint/dqstring-no-interpolate.expect
index 10724987a5..087eda15e4 100644
--- a/t/chainlint/dqstring-no-interpolate.expect
+++ b/t/chainlint/dqstring-no-interpolate.expect
@@ -6,6 +6,7 @@ grep "^\.git$" output.txt &&
(
cd client$version &&
GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. $(cat ../input)
-) > output &&
- cut -d ' ' -f 2 < output | sort > actual &&
+) >output &&
+ cut -d ' ' -f 2 <output | sort >actual &&
test_cmp expect actual
+
diff --git a/t/chainlint/empty-here-doc.expect b/t/chainlint/empty-here-doc.expect
index e8733c97c6..8507721192 100644
--- a/t/chainlint/empty-here-doc.expect
+++ b/t/chainlint/empty-here-doc.expect
@@ -1,4 +1,4 @@
-git ls-tree $tree path > current &&
-cat > expected <<\EOF &&
+git ls-tree $tree path >current &&
+cat >expected <<\EOF &&
EOF
test_output
diff --git a/t/chainlint/exclamation.expect b/t/chainlint/exclamation.expect
index 2d961a58c6..765a35bb4c 100644
--- a/t/chainlint/exclamation.expect
+++ b/t/chainlint/exclamation.expect
@@ -1,4 +1,4 @@
-if ! condition ; then echo nope ; else yep ; fi &&
+if ! condition; then echo nope; else yep; fi &&
test_prerequisite !MINGW &&
mail uucp!address &&
echo !whatever!
diff --git a/t/chainlint/for-loop-abbreviated.expect b/t/chainlint/for-loop-abbreviated.expect
index a21007a63f..02c0d15cca 100644
--- a/t/chainlint/for-loop-abbreviated.expect
+++ b/t/chainlint/for-loop-abbreviated.expect
@@ -1,5 +1,5 @@
for it
do
- path=$(expr "$it" : ( [^:]*) ) &&
+ path=$(expr "$it" : ([^:]*)) &&
git update-index --add "$path" || exit
done
diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect
index d65c82129a..d2237f1e38 100644
--- a/t/chainlint/for-loop.expect
+++ b/t/chainlint/for-loop.expect
@@ -6,6 +6,7 @@
bar
EOF
done ?!AMP?!
+
for i in a b c; do
echo $i &&
cat $i ?!LOOP?!
diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect
index a14388e6b9..dd7c997a3c 100644
--- a/t/chainlint/function.expect
+++ b/t/chainlint/function.expect
@@ -1,8 +1,8 @@
-sha1_file ( ) {
+sha1_file() {
echo "$*" | sed "s#..#.git/objects/&/#"
} &&
-remove_object ( ) {
+remove_object() {
file=$(sha1_file "$*") &&
test -e "$file" ?!AMP?!
rm -f "$file"
diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
index 1df3f78282..91b961242a 100644
--- a/t/chainlint/here-doc.expect
+++ b/t/chainlint/here-doc.expect
@@ -1,6 +1,6 @@
boodle wobba \
- gorgo snoot \
- wafta snurb <<EOF &&
+ gorgo snoot \
+ wafta snurb <<EOF &&
quoth the raven,
nevermore...
EOF
diff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect
index 24da9e86d5..7ce3a34806 100644
--- a/t/chainlint/loop-detect-status.expect
+++ b/t/chainlint/loop-detect-status.expect
@@ -1,18 +1,18 @@
-( while test $i -le $blobcount
-do
- printf "Generating blob $i/$blobcount\r" >& 2 &&
+(while test $i -le $blobcount
+ do
+ printf "Generating blob $i/$blobcount\r" >&2 &&
printf "blob\nmark :$i\ndata $blobsize\n" &&
#test-tool genrandom $i $blobsize &&
printf "%-${blobsize}s" $i &&
echo "M 100644 :$i $i" >> commit &&
i=$(($i+1)) ||
echo $? > exit-status
-done &&
-echo "commit refs/heads/main" &&
-echo "author A U Thor <author@email.com> 123456789 +0000" &&
-echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
-echo "data 5" &&
-echo ">2gb" &&
-cat commit ) |
+ done &&
+ echo "commit refs/heads/main" &&
+ echo "author A U Thor <author@email.com> 123456789 +0000" &&
+ echo "committer C O Mitter <committer@email.com> 123456789 +0000" &&
+ echo "data 5" &&
+ echo ">2gb" &&
+ cat commit) |
git fast-import --big-file-threshold=2 &&
test ! -f exit-status
diff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect
index 2a86885ee6..3836049cc4 100644
--- a/t/chainlint/nested-cuddled-subshell.expect
+++ b/t/chainlint/nested-cuddled-subshell.expect
@@ -2,18 +2,24 @@
(cd foo &&
bar
) &&
+
(cd foo &&
bar
) ?!AMP?!
+
(
cd foo &&
bar) &&
+
(
cd foo &&
bar) ?!AMP?!
+
(cd foo &&
bar) &&
+
(cd foo &&
bar) ?!AMP?!
+
foobar
)
diff --git a/t/chainlint/nested-loop-detect-failure.expect b/t/chainlint/nested-loop-detect-failure.expect
index 4793a0e8e1..3461df40e5 100644
--- a/t/chainlint/nested-loop-detect-failure.expect
+++ b/t/chainlint/nested-loop-detect-failure.expect
@@ -1,31 +1,31 @@
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
do
- for j in 0 1 2 3 4 5 6 7 8 9 ;
+ for j in 0 1 2 3 4 5 6 7 8 9;
do
- echo "$i$j" > "path$i$j" ?!LOOP?!
+ echo "$i$j" >"path$i$j" ?!LOOP?!
done ?!LOOP?!
done &&
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
do
- for j in 0 1 2 3 4 5 6 7 8 9 ;
+ for j in 0 1 2 3 4 5 6 7 8 9;
do
- echo "$i$j" > "path$i$j" || return 1
+ echo "$i$j" >"path$i$j" || return 1
done
done &&
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
do
- for j in 0 1 2 3 4 5 6 7 8 9 ;
+ for j in 0 1 2 3 4 5 6 7 8 9;
do
- echo "$i$j" > "path$i$j" ?!LOOP?!
+ echo "$i$j" >"path$i$j" ?!LOOP?!
done || return 1
done &&
-for i in 0 1 2 3 4 5 6 7 8 9 ;
+for i in 0 1 2 3 4 5 6 7 8 9;
do
- for j in 0 1 2 3 4 5 6 7 8 9 ;
+ for j in 0 1 2 3 4 5 6 7 8 9;
do
- echo "$i$j" > "path$i$j" || return 1
+ echo "$i$j" >"path$i$j" || return 1
done || return 1
done
diff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect
index 02e0a9f1bb..73ff28546a 100644
--- a/t/chainlint/nested-subshell.expect
+++ b/t/chainlint/nested-subshell.expect
@@ -4,6 +4,7 @@
echo a &&
echo b
) >file &&
+
cd foo &&
(
echo a ?!AMP?!
diff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect
index 2cfc028297..811971b1a3 100644
--- a/t/chainlint/pipe.expect
+++ b/t/chainlint/pipe.expect
@@ -2,7 +2,9 @@
foo |
bar |
baz &&
+
fish |
cow ?!AMP?!
+
sunder
)
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 52789278d1..75d6f607e2 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -1,7 +1,7 @@
(
echo wobba \
- gorgo snoot \
- wafta snurb <<-EOF &&
+ gorgo snoot \
+ wafta snurb <<-EOF &&
quoth the raven,
nevermore...
EOF
diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect
index b7015361bf..8f694990e8 100644
--- a/t/chainlint/subshell-one-liner.expect
+++ b/t/chainlint/subshell-one-liner.expect
@@ -2,13 +2,18 @@
(foo && bar) &&
(foo && bar) |
(foo && bar) >baz &&
+
(foo; ?!AMP?! bar) &&
(foo; ?!AMP?! bar) |
(foo; ?!AMP?! bar) >baz &&
+
(foo || exit 1) &&
(foo || exit 1) |
(foo || exit 1) >baz &&
+
(foo && bar) ?!AMP?!
+
(foo && bar; ?!AMP?! baz) ?!AMP?!
+
foobar
)
diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect
index 71b3b3bc20..02f3129232 100644
--- a/t/chainlint/t7900-subtree.expect
+++ b/t/chainlint/t7900-subtree.expect
@@ -15,6 +15,7 @@ main-sub4" &&
$chkms
TXT
) &&
+
subfiles=$(git ls-files) &&
check_equal "$subfiles" "$chkms
$chks"
diff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect
index 342360bcd0..6a387917a7 100644
--- a/t/chainlint/token-pasting.expect
+++ b/t/chainlint/token-pasting.expect
@@ -4,22 +4,22 @@ git config filter.rot13.clean ./rot13.sh &&
{
echo "*.t filter=rot13" ?!AMP?!
echo "*.i ident"
-} > .gitattributes &&
+} >.gitattributes &&
{
echo a b c d e f g h i j k l m ?!AMP?!
echo n o p q r s t u v w x y z ?!AMP?!
echo '$Id$'
-} > test &&
-cat test > test.t &&
-cat test > test.o &&
-cat test > test.i &&
+} >test &&
+cat test >test.t &&
+cat test >test.o &&
+cat test >test.i &&
git add test test.t test.i &&
rm -f test test.t test.i &&
git checkout -- test test.t test.i &&
-echo "content-test2" > test2.o &&
-echo "content-test3 - filename with special characters" > "test3 'sq',$x=.o" ?!AMP?!
+echo "content-test2" >test2.o &&
+echo "content-test3 - filename with special characters" >"test3 'sq',$x=.o" ?!AMP?!
downstream_url_for_sed=$(
printf "%sn" "$downstream_url" |
diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect
index 1f5eaea0fd..06c1567f48 100644
--- a/t/chainlint/while-loop.expect
+++ b/t/chainlint/while-loop.expect
@@ -6,6 +6,7 @@
bar
EOF
done ?!AMP?!
+
while true; do
echo foo &&
cat bar ?!LOOP?!
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v4] tests: adjust whitespace in chainlint expectations
From: Eric Sunshine @ 2023-12-15 7:17 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <fb312f559de7b99244e4c86a995250599cd9be06.1702622508.git.ps@pks.im>
On Fri, Dec 15, 2023 at 1:42 AM Patrick Steinhardt <ps@pks.im> wrote:
> [...]
> Instead of improving the detection logic, fix our ".expect" files so
> that we do not need any post-processing at all anymore. This allows us
> to drop the `-w` flag when diffing so that we can always use diff(1)
> now.
>
> Note that we keep some of the post-processing of `chainlint.pl` output
> intact to strip leading line numbers generated by the script. Having
> these would cause a rippling effect whenever we add a new test that
> sorts into the middle of existing tests and would require us to
> renumerate all subsequent lines, which seems rather pointless.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/Makefile b/t/Makefile
> @@ -103,20 +103,12 @@ check-chainlint:
> for i in $(CHAINLINTTESTS); do \
> echo "# chainlint: $$i" && \
> - sed -e '/^[ ]*$$/d' chainlint/$$i.expect; \
> + cat chainlint/$$i.expect; \
> done \
> $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
> - sed -e 's/^[1-9][0-9]* //;/^[ ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \
> + sed -e 's/^[1-9][0-9]* //' >'$(CHAINLINTTMP_SQ)'/actual && \
> + diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
Thanks, this version looks fine. FWIW, you may consider this:
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Aside: I was rather surprised to see this output from git-am when applying v4:
Applying: tests: adjust whitespace in chainlint expectations
.git/rebase-apply/patch:205: new blank line at EOF.
+
.git/rebase-apply/patch:219: new blank line at EOF.
+
warning: 2 lines add whitespace errors.
But upon investigating the two "test" files in question,
dqstring-line-splice.test and dqstring-no-interpolate.test, I recalled
that I had to play tricks to escape the single-quote context created
by the Makefile when generating t/chainlinttmp/tests in order to allow
chainlint.pl to see a double-quoted string. So, the abovementioned
blank lines are indeed expected output from chainlint.pl given the
tricks played.
^ permalink raw reply
* Re: [PATCH 0/2] avoiding recursion in mailinfo
From: Patrick Steinhardt @ 2023-12-15 7:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, Taylor Blau, Carlos Andrés Ramírez Cataño
In-Reply-To: <20231214214444.GB2297853@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]
On Thu, Dec 14, 2023 at 04:44:44PM -0500, Jeff King wrote:
> On Thu, Dec 14, 2023 at 08:41:20AM +0100, Patrick Steinhardt wrote:
>
> > > @@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)
> > > take_next_literally = 1;
> > > continue;
> > > case '(':
> > > - in = unquote_comment(outbuf, in);
> > > + strbuf_addch(outbuf, '(');
> > > + depth++;
> > > continue;
> > > case ')':
> > > strbuf_addch(outbuf, ')');
> > > - return in;
> > > + if (!--depth)
> > > + return in;
> > > + continue;
> > > }
> > > }
> > >
> > > I doubt it's a big deal either way, but if it's that easy to do it might
> > > be worth it.
> >
> > Isn't this only protecting against unbalanced braces? That might be a
> > sensible check to do regardless, but does it protect against recursion
> > blowing the stack if you just happen to have many opening braces?
> >
> > Might be I'm missing something.
>
> It protects against recrusion blowing the stack because it removes the
> recursive call to unquote_comment(). ;)
Doh. Of course it does, I completely missed the removals.
> The "depth" stuff is there because without recursion, we have to know
> when we've hit the correct closing paren (as opposed to an embedded
> one).
Yes, makes sense now. The patches look good to me, thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] fetch: no redundant error message for atomic fetch
From: Patrick Steinhardt @ 2023-12-15 9:56 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <6fb83a00000563a79f3948f9087c634ae507b9f5.1702556642.git.zhiyou.jx@alibaba-inc.com>
[-- Attachment #1: Type: text/plain, Size: 4346 bytes --]
On Thu, Dec 14, 2023 at 08:33:12PM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> If an error occurs during an atomic fetch, a redundant error message
> will appear at the end of do_fetch(). It was introduced in b3a804663c
> (fetch: make `--atomic` flag cover backfilling of tags, 2022-02-17).
>
> In function do_fetch(), a failure message is already shown before the
> retcode is set, so we should not call additional error() at the end of
> this function.
>
> We can remove the redundant error() function, because we know that
> the function ref_transaction_abort() never fails.
Okay, so this still suffers from the same issue as discussed in the
thread at <ZTYue-3gAS1aGXNa@tanuki>, but now it's documented in the
commit message. I'm still not convinced that is a good argument to say
that the function never fails, and if it ever would it would populate
the error message. Especially now where there's churn to introduce the
new reftable backend this could change any time.
For the record, I'm proposing to do something like the following:
diff --git a/builtin/fetch.c b/builtin/fetch.c
index fd134ba74d..80b8bc549d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1651,7 +1651,7 @@ static int do_fetch(struct transport *transport,
if (atomic_fetch) {
transaction = ref_transaction_begin(&err);
if (!transaction) {
- retcode = error("%s", err.buf);
+ retcode = -1;
goto cleanup;
}
}
@@ -1711,7 +1711,6 @@ static int do_fetch(struct transport *transport,
retcode = ref_transaction_commit(transaction, &err);
if (retcode) {
- error("%s", err.buf);
ref_transaction_free(transaction);
transaction = NULL;
goto cleanup;
@@ -1775,9 +1774,13 @@ static int do_fetch(struct transport *transport,
}
cleanup:
+ if (retcode && err.len)
+ error("%s", err.buf);
if (retcode && transaction) {
+ strbuf_reset(&err);
ref_transaction_abort(transaction, &err);
- error("%s", err.buf);
+ if (err.len)
+ error("%s", err.buf);
}
display_state_release(&display_state);
This would both fix the issue you observed, but also fixes issues in
case the ref backend failed without writing an error message to the
buffer. It also fixes issues if there were multiple failures, where we'd
print the initial error printed to the buffer twice.
I know this is mostly solidifying us against potential future changes,
but if it's comparatively easy like this I don't see much of a reason
against it.
Patrick
> While we can find a
> common pattern for calling ref_transaction_abort() by running command
> "git grep -A1 ref_transaction_abort", e.g.:
>
> if (ref_transaction_abort(transaction, &error))
> error("abort: %s", error.buf);
>
> We can fix this issue follow this pattern, and the test case "fetch
> porcelain output (atomic)" in t5574 will also be fixed. If in the future
> we decide that we don't need to check the return value of the function
> ref_transaction_abort(), this change can be fixed along with it.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> builtin/fetch.c | 4 +---
> t/t5574-fetch-output.sh | 2 +-
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index fd134ba74d..01a573cf8d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,
> }
>
> cleanup:
> - if (retcode && transaction) {
> - ref_transaction_abort(transaction, &err);
> + if (retcode && transaction && ref_transaction_abort(transaction, &err))
> error("%s", err.buf);
> - }
>
> display_state_release(&display_state);
> close_fetch_head(&fetch_head);
> diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
> index bc747efefc..8d01e36b3d 100755
> --- a/t/t5574-fetch-output.sh
> +++ b/t/t5574-fetch-output.sh
> @@ -98,7 +98,7 @@ do
> opt=
> ;;
> esac
> - test_expect_failure "fetch porcelain output ${opt:+(atomic)}" '
> + test_expect_success "fetch porcelain output ${opt:+(atomic)}" '
> test_when_finished "rm -rf porcelain" &&
>
> # Clone and pre-seed the repositories. We fetch references into two
> --
> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch
From: Patrick Steinhardt @ 2023-12-15 9:56 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <210191917bcfa9293622908c291652059576f3e5.1702556642.git.zhiyou.jx@alibaba-inc.com>
[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]
On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
[snip]
> @@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '
> git checkout force-updated &&
> git reset --hard HEAD~ &&
> test_commit --no-tag force-update-new &&
> - FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
> -
> - cat >expect <<-EOF &&
> - - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
> - - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
> - $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
> - ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
> - * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
> - $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
> - + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
> - * $ZERO_OID $MAIN_OLD refs/forced/new-branch
> - $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
> - + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
> - * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
> - EOF
> -
> - # Execute a dry-run fetch first. We do this to assert that the dry-run
> - # and non-dry-run fetches produces the same output. Execution of the
> - # fetch is expected to fail as we have a rejected reference update.
> - test_must_fail git -C porcelain fetch \
> - --porcelain --dry-run --prune origin $refspecs >actual &&
> - test_cmp expect actual &&
> -
> - # And now we perform a non-dry-run fetch.
> - test_must_fail git -C porcelain fetch \
> - --porcelain --prune origin $refspecs >actual 2>stderr &&
> - test_cmp expect actual &&
> - test_must_be_empty stderr
> + FORCE_UPDATED_NEW=$(git rev-parse HEAD)
> '
>
> +for opt in off on
> +do
> + case $opt in
> + on)
> + opt=--atomic
> + ;;
> + off)
> + opt=
> + ;;
> + esac
Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
this case statement. Not sure whether this is worth a reroll though,
probably not.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch
From: Jiang Xin @ 2023-12-15 11:16 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <ZXwi2MA-KUxszfGj@tanuki>
On Fri, Dec 15, 2023 at 5:56 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Dec 14, 2023 at 08:33:11PM +0800, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> [snip]
> > @@ -91,36 +85,61 @@ test_expect_success 'fetch porcelain output' '
> > git checkout force-updated &&
> > git reset --hard HEAD~ &&
> > test_commit --no-tag force-update-new &&
> > - FORCE_UPDATED_NEW=$(git rev-parse HEAD) &&
> > -
> > - cat >expect <<-EOF &&
> > - - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
> > - - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
> > - $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
> > - ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
> > - * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
> > - $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
> > - + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
> > - * $ZERO_OID $MAIN_OLD refs/forced/new-branch
> > - $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
> > - + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
> > - * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
> > - EOF
> > -
> > - # Execute a dry-run fetch first. We do this to assert that the dry-run
> > - # and non-dry-run fetches produces the same output. Execution of the
> > - # fetch is expected to fail as we have a rejected reference update.
> > - test_must_fail git -C porcelain fetch \
> > - --porcelain --dry-run --prune origin $refspecs >actual &&
> > - test_cmp expect actual &&
> > -
> > - # And now we perform a non-dry-run fetch.
> > - test_must_fail git -C porcelain fetch \
> > - --porcelain --prune origin $refspecs >actual 2>stderr &&
> > - test_cmp expect actual &&
> > - test_must_be_empty stderr
> > + FORCE_UPDATED_NEW=$(git rev-parse HEAD)
> > '
> >
> > +for opt in off on
> > +do
> > + case $opt in
> > + on)
> > + opt=--atomic
> > + ;;
> > + off)
> > + opt=
> > + ;;
> > + esac
>
> Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
> this case statement. Not sure whether this is worth a reroll though,
> probably not.
Yes, your code is much simpler.
^ permalink raw reply
* Re: What's the recommendation for forgetting all rerere's records?
From: Sean Allred @ 2023-12-15 12:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sean Allred, git
In-Reply-To: <xmqqcyvgz3ih.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sean Allred <allred.sean@gmail.com> writes:
>> When outside the context of a conflict (no rebase/merge in progress),
>> what's the intended workflow to clear out the contents of
>> $GIT_DIR/rr-cache?
>
> You could "rm -fr .git/rr-cache/??*" if you want.
Here's my reasoning for not wanting that:
>> I'm wary of recommending `rm -rf "$(git rev-parse --git-dir)/rr-cache"`
>> -- it's hard to describe this as anything but hacky when the rest of the
>> experience is handled in porcelain.
> The "intended" workflow is there will no need to "clear out" at all;
> you may notice mistaken resolution, and you will remove it when you
> notice one, but the bulk of the remaining database entries ought to
> be still correct.
When we noticed mistaken resolutions, rerere had already applied the
recorded resolution and there was no apparent way to return to the
conflicted state. If clearing out the rerere database was never an
intended workflow here, maybe _that's_ my actual question?
It seems likely this 'recovery' workflow should just be documented in
git-rerere.txt (which I'm happy to take on once I learn what that
workflow should be).
--
Sean Allred
^ permalink raw reply
* Bug | Documentation | git add -all | Synopsis has minor mistake
From: Benjamin Lehmann @ 2023-12-15 12:38 UTC (permalink / raw)
To: git
Hey.
The mistake can be found in the synopsis here:
https://git-scm.com/docs/git-add#Documentation/git-add.txt--A
In the synopsys, the options -all currently reads:
[--[no-]all | --[no-]ignore-removal |
You can see that there is no mention of -A, which is the main way that
people would use -all perhaps, so it really ought to be included
correctly in the synopsis. In addition, the closing square-bracket is
missing.
Hope this was the right place to report this - seemed to be the only option.
Ben
^ permalink raw reply
* Re: [PATCH] Use ^=1 to toggle between 0 and 1
From: Phillip Wood @ 2023-12-15 14:46 UTC (permalink / raw)
To: Jeff King, René Scharfe
Cc: AtariDreams via GitGitGadget, git, Seija Kijin
In-Reply-To: <20231214220503.GA3320432@coredump.intra.peff.net>
On 14/12/2023 22:05, Jeff King wrote:
> On Thu, Dec 14, 2023 at 02:08:31PM +0100, René Scharfe wrote:
>
>>> I don't even know that we'd need much of a weather-balloon patch. I
>>> think it would be valid to do:
>>>
>>> #ifndef bool
>>> #define bool int
>>>
>>> to handle pre-C99 compilers (if there even are any these days). Of
>>> course we probably need some conditional magic to try to "#include
>>> <stdbool.h>" for the actual C99. I guess we could assume C99 by default
>>> and then add NO_STDBOOL as an escape hatch if anybody complains.
>>
>> The semantics are slightly different in edge cases, so that fallback
>> would not be fully watertight. E.g. consider:
>>
>> bool b(bool cond) {return cond == true;}
>> bool b2(void) {return b(2);}
Thanks for bring this up René, I had similar concerns when I saw the
suggestion of using "int" as a fallback.
> Yeah. b2() is wrong for passing "2" to a bool.
I think it depends what you mean by "wrong" §6.3.1.2 of standard is
quite clear that when any non-zero scalar value is converted to _Bool
the result is "1"
> I assumed that the
> compiler would warn of that (at least for people on modern C99
> compilers, not the fallback code), but it doesn't seem to. It's been a
> long time since I've worked on a code base that made us of "bool", but I
> guess that idea is that silently coercing a non-zero int to a bool is
> reasonable in many cases (e.g., "bool found_foo = count_foos()").
I guess it is also consistent with the way "if" and "while" consider a
non-zero scalar value to be "true".
> I guess one could argue that b() is also sub-optimal, as it should just
> say "return cond" or "return !cond" rather than explicitly comparing to
> true/false. But I won't be surprised if it happens from time to time.
Even if it unlikely that we would directly compare a boolean variable to
"true" or "false" it is certainly conceivable that we'd compare two
boolean variables directly. For the integer fallback to be safe we'd
need to write
if (!cond_a == !cond_b)
rather than
if (cond_a == cond_b)
>> A coding rule to not compare bools could mitigate that. Or a rule to
>> only use the values true and false in bool context and to only use
>> logical operators on them.
>
> That seems more complex than we want if our goal is just supporting
> legacy systems that may or may not even exist. Given your example, I'd
> be more inclined to just do a weather-balloon adding <stdbool.h> to
> git-compat-util.h, and using "bool" in a single spot in the code. If
> nobody screams after a few releases, we can consider it OK. If they do,
> it's a trivial patch to convert back.
A weather-balloon seems like the safest route forward. We have been
requiring C99 for two years now [1], hopefully there aren't any
compilers out that claim to support C99 but don't provide "<stdbool.h>"
(I did check online and the compiler on NonStop does support _Bool).
Best Wishes
Phillip
[1] 7bc341e21b (git-compat-util: add a test balloon for C99 support,
2021-12-01)
^ permalink raw reply
* Re: [PATCH 00/24] pack-objects: multi-pack verbatim reuse
From: Taylor Blau @ 2023-12-15 15:37 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <20231212081238.GD1117953@coredump.intra.peff.net>
On Tue, Dec 12, 2023 at 03:12:38AM -0500, Jeff King wrote:
> So my question is: how much of what you're seeing is from (1) and (2),
> and how much is from (3)? Because there are other ways to trigger (3),
> such as lowering the window size. For example, if you try your same
> packing example with --window=0, how do the CPU and output size compare
> to the results of your series? (I'd also check peak memory usage).
Interesting question! Here are some preliminary numbers on my machine
(which runs Debian unstable with a Intel Xenon W-2255 CPU @ 3.70GHz and
64GB of RAM).
I ran the following hyperfine command on my testing repository, which
has the Git repository broken up into ~75 packs or so:
$ hyperfine -L v single,multi -L window 0,10 \
--show-output \
-n '{v}-pack reuse, pack.window={window}' \
'git.compile \
-c pack.allowPackReuse={v} \
-c pack.window={window} \
pack-objects --revs --stdout --use-bitmap-index --delta-base-offset <in 2>/dev/null | wc -c'
Which gave the following results for runtime:
Benchmark 1: single-pack reuse, pack.window=0
[...]
Time (mean ± σ): 1.248 s ± 0.004 s [User: 1.160 s, System: 0.188 s]
Range (min … max): 1.244 s … 1.259 s 10 runs
Benchmark 2: multi-pack reuse, pack.window=0
[...]
Time (mean ± σ): 1.075 s ± 0.005 s [User: 0.990 s, System: 0.188 s]
Range (min … max): 1.071 s … 1.088 s 10 runs
Benchmark 3: single-pack reuse, pack.window=10
[...]
Time (mean ± σ): 6.281 s ± 0.024 s [User: 43.727 s, System: 0.492 s]
Range (min … max): 6.252 s … 6.326 s 10 runs
Benchmark 4: multi-pack reuse, pack.window=10
[...]
Time (mean ± σ): 1.028 s ± 0.002 s [User: 1.150 s, System: 0.184 s]
Range (min … max): 1.026 s … 1.032 s 10 runs
Here are the average numbers for the resulting "clone" size in each of
the above configurations:
Benchmark 1: single-pack reuse, pack.window=0
264.443 MB
Benchmark 2: multi-pack reuse, pack.window=0
268.670 MB
Benchmark 3: single-pack reuse, pack.window=10
194.355 MB
Benchmark 4: multi-pack reuse, pack.window=10
266.473 MB
So it looks like setting pack.window=0 (with both single and multi-pack
reuse) yields a similarly sized pack output as multi-pack reuse with any
window setting.
Running the same benchmark as above again, but this time sending the
pack output to /dev/null and instead capturing the maximum RSS value
from `/usr/bin/time -v` gives us the following (averages, in MB):
Benchmark 1: single-pack reuse, pack.window=0
354.224 MB (max RSS)
Benchmark 2: multi-pack reuse, pack.window=0
315.730 MB (max RSS)
Benchmark 3: single-pack reuse, pack.window=10
470.651 MB (max RSS)
Benchmark 4: multi-pack reuse, pack.window=10
328.786 MB (max RSS)
So memory usage is similar between runs except for the single-pack reuse
case with a window size of 10.
It looks like the sweet spot is probably multi-pack reuse with a
small-ish window size, which achieves the best of both worlds (small
pack size, relative to other configurations that reuse large portions of
the pack, and low memory usage).
It's pretty close between multi-pack reuse with a window size of 0 and
a window size of 10. If you want to optimize for pack size, you could
trade a ~4% reduction in pack size for a ~1% increase in peak memory
usage.
Of course, YMMV depending on the repository, packing strategy, and
pack.window configuration (among others) while packing. But this should
give you a general idea of what to expect.
Thanks,
Taylor
^ permalink raw reply
* Re: What's the recommendation for forgetting all rerere's records?
From: Junio C Hamano @ 2023-12-15 16:30 UTC (permalink / raw)
To: Sean Allred; +Cc: git
In-Reply-To: <m0sf43abw7.fsf@epic96565.epic.com>
Sean Allred <allred.sean@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>> Sean Allred <allred.sean@gmail.com> writes:
>>> When outside the context of a conflict (no rebase/merge in progress),
>>> what's the intended workflow to clear out the contents of
>>> $GIT_DIR/rr-cache?
>>
>> You could "rm -fr .git/rr-cache/??*" if you want.
>
> Here's my reasoning for not wanting that:
>
>>> I'm wary of recommending `rm -rf "$(git rev-parse --git-dir)/rr-cache"`
>>> -- it's hard to describe this as anything but hacky when the rest of the
>>> experience is handled in porcelain.
It is meant to be ugly ;-); the reason why the Porcelain does not
offer bulk erasure is because we want to strongly discourage it.
>> The "intended" workflow is there will no need to "clear out" at all;
>> you may notice mistaken resolution, and you will remove it when you
>> notice one, but the bulk of the remaining database entries ought to
>> be still correct.
>
> When we noticed mistaken resolutions, rerere had already applied the
> recorded resolution and there was no apparent way to return to the
> conflicted state.
The simplest is to go back to the original state before the merge
and then redo the operation without rerere enabled.
$ git reset --hard
$ git -c rerere.enabled=no merge <whatever arguments you used>
And you can redo the merge manually.
But more generally, after you incorrectly resolved a merge conflict,
whether you fumbled with your editor yourself or you let rerere kick
in to reuse your recorded resolution that you made in the past that
was faulty, before or after you run "git add" to tell the resolution
to the index, you should be able to do
$ git checkout --merge <pathspec>
to tell Git to "unresolve" such paths. Here is the relevant
paragraph from the "git checkout --help":
When checking out paths from the index, this option lets you recreate the
conflicted merge in the specified paths. This option cannot be used when
checking out paths from a tree-ish.
The below is what I just did to demonstrate. You can try the same
if you have our source code. The first attempt will likely to
conflict because you do not have the rerere record, but you can
resolve the conflict in builtin/mv.c the way I did (shown by first
"git diff" in the sample transcript), and run "git rerere" (or just
"git commit -a" should also work) to record the resolution, and then
"git reset --hard" to obtain a sample rerere record you can use to
practice.
# Just a sample merge I know will have a conflict
$ SAMPLE=a59dbae0b3bd; # v2.43.0-rc0~126
# Go to its parent and retry the merge with its second parent
$ git checkout --detach $SAMPLE^1
$ git merge $SAMPLE^2
Auto-merging builtin/mv.c
CONFLICT (content): Merge conflict in builtin/mv.c
Resolved 'builtin/mv.c' using previous resolution.
Automatic merge failed; fix conflicts and then commit the result.
# We have conflict, and rerere kicked in. Because I do not have
# rerere.autoupdate configuration set, I can as "ls-files -u"
# which paths are conflicting, but that is unnecessary (we
# can see the path with conflict with the CONFLICT label above).
$ git ls-files -u
100644 665bd274485f6c76403e9230539e2650073a47f3 1 builtin/mv.c
100644 05e7156034e04d637990cabf105f7fa967b0f2aa 2 builtin/mv.c
100644 80fc7a3c7029603a0fcaf9d15d8432ed799b4909 3 builtin/mv.c
# This is the resolution "rerere" gave me, which is what I did
# back in August this year. If you are following this example,
# you'll first have to edit builtin/mv.c to hand resolve,
# register the resolution to "rerere" database, and then run
# "git reset --hard" to go back to the state before you did the
# "git merge $SAMPLE^2" step to repeat.
$ git diff
diff --cc builtin/mv.c
index 05e7156034,80fc7a3c70..0000000000
--- i/builtin/mv.c
+++ w/builtin/mv.c
@@@ -304,8 -303,8 +304,8 @@@ int cmd_mv(int argc, const char **argv
goto act_on_entry;
}
if (S_ISDIR(st.st_mode)
- && lstat(dst, &st) == 0) {
+ && lstat(dst, &dest_st) == 0) {
- bad = _("cannot move directory over file");
+ bad = _("destination already exists");
goto act_on_entry;
}
# Now the fun command you seem to have missed. You MUST give
# "git checkout --merge" a pathspec. I do not encourage it but
# using "." to say "unresolve everything under the sun" should
# also work.
$ git checkout --merge builtin/mv.c
Recreated 1 merge conflict
# Let's check the result. We have recreated the conflicted
# state in the working tree.
$ git diff
diff --cc builtin/mv.c
index 05e7156034,80fc7a3c70..0000000000
--- i/builtin/mv.c
+++ w/builtin/mv.c
@@@ -304,8 -303,8 +304,16 @@@ int cmd_mv(int argc, const char **argv
goto act_on_entry;
}
if (S_ISDIR(st.st_mode)
++<<<<<<< ours
+ && lstat(dst, &dest_st) == 0) {
+ bad = _("cannot move directory over file");
++||||||| base
++ && lstat(dst, &st) == 0) {
++ bad = _("cannot move directory over file");
++=======
+ && lstat(dst, &st) == 0) {
+ bad = _("destination already exists");
++>>>>>>> theirs
goto act_on_entry;
}
# You should then be able to correct the resolution with your
# editor.
$ edit builtin/mv.c
# If this is one-time fix (you are happy with the original
# resolution and wanted to deviate from it only once this time),
# there is nothing else need to be done. If you want to record
# this as a new resolution, you'd get rid of the old one and
# record this one.
$ git rerere forget builtin/mv.c
$ git rerere
^ permalink raw reply
* Re: [PATCH v2 1/2] t5574: test porcelain output of atomic fetch
From: Junio C Hamano @ 2023-12-15 16:47 UTC (permalink / raw)
To: Jiang Xin; +Cc: Patrick Steinhardt, Git List, Jiang Xin
In-Reply-To: <CANYiYbGaJjnuVx7wJshgqiwvpGTmdq2JiOe4S_ph1bgiZ7XTJg@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
>> Nit: you could also do `for opt in "--atomic" ""` directly to get rid of
>> this case statement. Not sure whether this is worth a reroll though,
>> probably not.
>
> Yes, your code is much simpler.
Yup, thanks for careful and helpful reviews, Patrick, on both
patches. And of course, thanks, Jiang, for working on them.
^ permalink raw reply
* Re: [PATCH v4] tests: adjust whitespace in chainlint expectations
From: Junio C Hamano @ 2023-12-15 16:44 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Patrick Steinhardt, git
In-Reply-To: <CAPig+cSNZ75WSjxfJFoVPBTkv7yQpxGz1c4ugCyQRLytpNLjig@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> Aside: I was rather surprised to see this output from git-am when applying v4:
>
> Applying: tests: adjust whitespace in chainlint expectations
> .git/rebase-apply/patch:205: new blank line at EOF.
> +
> .git/rebase-apply/patch:219: new blank line at EOF.
> +
> warning: 2 lines add whitespace errors.
>
> But upon investigating the two "test" files in question,
> dqstring-line-splice.test and dqstring-no-interpolate.test, I recalled
> that I had to play tricks to escape the single-quote context created
> by the Makefile when generating t/chainlinttmp/tests in order to allow
> chainlint.pl to see a double-quoted string. So, the abovementioned
> blank lines are indeed expected output from chainlint.pl given the
> tricks played.
Thanks for being extra careful while reviewing.
^ permalink raw reply
* Re: [PATCH] Use ^=1 to toggle between 0 and 1
From: Junio C Hamano @ 2023-12-15 17:09 UTC (permalink / raw)
To: Phillip Wood
Cc: Jeff King, René Scharfe, AtariDreams via GitGitGadget, git,
Seija Kijin
In-Reply-To: <99b3a727-36fd-4fa5-a6be-60ae6fc5911e@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Even if it unlikely that we would directly compare a boolean variable
> to "true" or "false" it is certainly conceivable that we'd compare two
> boolean variables directly. For the integer fallback to be safe we'd
> need to write
>
> if (!cond_a == !cond_b)
>
> rather than
>
> if (cond_a == cond_b)
Eek, it defeats the benefit of using true Boolean type if we had to
train ourselves to write the former, doesn't it?
> A weather-balloon seems like the safest route forward. We have been
> requiring C99 for two years now [1], hopefully there aren't any
> compilers out that claim to support C99 but don't provide
> "<stdbool.h>" (I did check online and the compiler on NonStop does
> support _Bool).
>
> Best Wishes
>
> Phillip
>
> [1] 7bc341e21b (git-compat-util: add a test balloon for C99 support,
> 2021-12-01)
Nice to be reminded of this one.
The cited commit does not start to use any specific feature from
C99, other than that we now require that the compiler claims C99
conformance by __STDC_VERSION__ set appropriately. The commit log
message says C99 "provides a variety of useful features, including
..., many of which we already use.", which implies that our wish was
to officially allow any and all features in C99 to be used in our
codebase after a successful flight of this test balloon.
Now, I think we saw a successful flight of this test balloon by now.
Is allowing all the C99 the next step we really want to take?
I still personally have an aversion against decl-after-statement and
//-comments, not due to portability reasons at all, but because I
find that the code is easier to read without it. But in principle,
it is powerful to be able to say "OK, as long as the feature is in
C99 you can use it", instead of having to decide on individual
features, and I am not fundamentally against going that route if it
is where people want to go.
Thanks.
^ permalink raw reply
* [PATCH 2/5] git-bisect.txt: BISECT_HEAD is not that special
From: Junio C Hamano @ 2023-12-15 20:32 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20231215203245.3622299-1-gitster@pobox.com>
The description of "git bisect --no-checkout" called BISECT_HEAD a
"special ref", but there is nothing special about it. It merely is
yet another pseudoref.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-bisect.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 191b4a42b6..aa02e46224 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -362,7 +362,7 @@ OPTIONS
--no-checkout::
+
Do not checkout the new working tree at each iteration of the bisection
-process. Instead just update a special reference named `BISECT_HEAD` to make
+process. Instead just update the reference named `BISECT_HEAD` to make
it point to the commit that should be tested.
+
This option may be useful when the test you would perform in each step
--
2.43.0-76-g1a87c842ec
^ permalink raw reply related
* [PATCH 0/5] make room for "special ref"
From: Junio C Hamano @ 2023-12-15 20:32 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt
Patrick's reftable work is progressing nicely and wants to establish
"special ref" as a phrase with some defined meaning that is somewhat
different from a mere "pseudo ref".
A pseudo ref is merely a normal ref with a funny naming convention,
i.e., being outside the refs/ hierarchy and has names with all
uppercase letters (or an underscore). But there truly are refs that
are more than that. For example, FETCH_HEAD currently stores not
just a single object name, but can and is used to store multiple
object names, each with annotations to record where they came from.
There indeed may be a need to introduce a new term to refer to such
"special refs".
Existing documentation, however, uses "special ref" to refer to
pseudo refs without any "special" property, like FETCH_HEAD does.
This series merely corrects such existing uses of the word, to make
room for Patrick's series to introduce (and formally define in the
glossary) "special refs".
Junio C Hamano (5):
git.txt: HEAD is not that special
git-bisect.txt: BISECT_HEAD is not that special
refs.h: HEAD is not that special
docs: AUTO_MERGE is not that special
docs: MERGE_AUTOSTASH is not that special
Documentation/git-bisect.txt | 2 +-
Documentation/git-diff.txt | 2 +-
Documentation/git-merge.txt | 2 +-
Documentation/git.txt | 7 ++++---
Documentation/merge-options.txt | 2 +-
Documentation/user-manual.txt | 2 +-
refs.h | 2 +-
7 files changed, 10 insertions(+), 9 deletions(-)
--
2.43.0-76-g1a87c842ec
^ permalink raw reply
* [PATCH 1/5] git.txt: HEAD is not that special
From: Junio C Hamano @ 2023-12-15 20:32 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20231215203245.3622299-1-gitster@pobox.com>
The introductory text in "git help git" that describes HEAD called
it "a special ref". It is special compared to the more regular refs
like refs/heads/master and refs/tags/v1.0.0, but not that special,
unlike truly special ones like FETCH_HEAD.
Rewrite a few sentences to also introduce the distinction between a
regular ref that contain the object name and a symbolic ref that
contain the name of another ref. Update the description of HEAD
that point at the current branch to use the more correct term, a
"symbolic ref".
This was found as part of auditing the documentation and in-code
comments for uses of "special ref" that refer merely a "pseudo ref".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git.txt | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2535a30194..880cdc5d7f 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1025,10 +1025,11 @@ When first created, objects are stored in individual files, but for
efficiency may later be compressed together into "pack files".
Named pointers called refs mark interesting points in history. A ref
-may contain the SHA-1 name of an object or the name of another ref. Refs
-with names beginning `ref/head/` contain the SHA-1 name of the most
+may contain the SHA-1 name of an object or the name of another ref (the
+latter is called a "symbolic ref").
+Refs with names beginning `ref/head/` contain the SHA-1 name of the most
recent commit (or "head") of a branch under development. SHA-1 names of
-tags of interest are stored under `ref/tags/`. A special ref named
+tags of interest are stored under `ref/tags/`. A symbolic ref named
`HEAD` contains the name of the currently checked-out branch.
The index file is initialized with a list of all paths and, for each
--
2.43.0-76-g1a87c842ec
^ permalink raw reply related
* [PATCH 3/5] refs.h: HEAD is not that special
From: Junio C Hamano @ 2023-12-15 20:32 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20231215203245.3622299-1-gitster@pobox.com>
In-code comment explains pseudorefs but used a wrong nomenclature
"special ref".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
refs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.h b/refs.h
index 23211a5ea1..ff113bb12a 100644
--- a/refs.h
+++ b/refs.h
@@ -56,7 +56,7 @@ struct worktree;
* Even with RESOLVE_REF_ALLOW_BAD_NAME, names that escape the refs/
* directory and do not consist of all caps and underscores cannot be
* resolved. The function returns NULL for such ref names.
- * Caps and underscores refers to the special refs, such as HEAD,
+ * Caps and underscores refers to the pseudorefs, such as HEAD,
* FETCH_HEAD and friends, that all live outside of the refs/ directory.
*/
#define RESOLVE_REF_READING 0x01
--
2.43.0-76-g1a87c842ec
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox