* [PATCH 04/11] t: convert tests to not write references via the filesystem
From: Patrick Steinhardt @ 2023-10-18 5:35 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1697607222.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 9212 bytes --]
Some of our tests manually create, update or delete references by
writing the respective new values into the filesystem directly. While
this works with the current files reference backend, this will break
once we have a second reference backend implementation in our codebase.
Refactor these tests to instead use git-update-ref(1) or our `ref-store`
test tool. The latter is required in some cases where safety checks of
git-update-ref(1) would otherwise reject a reference update.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1400-update-ref.sh | 16 +++++++---------
t/t1450-fsck.sh | 28 +++++++++++++++-------------
t/t3404-rebase-interactive.sh | 2 +-
t/t5526-fetch-submodules.sh | 2 +-
4 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 4d66cd7f4a1..cd24018ce99 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -9,8 +9,6 @@ test_description='Test git update-ref and basic ref logging'
Z=$ZERO_OID
m=refs/heads/main
-n_dir=refs/heads/gu
-n=$n_dir/fixes
outside=refs/foo
bare=bare-repo
@@ -62,10 +60,10 @@ test_expect_success "delete $m without oldvalue verification" '
test_must_fail git show-ref --verify -q $m
'
-test_expect_success "fail to create $n" '
- test_when_finished "rm -f .git/$n_dir" &&
- touch .git/$n_dir &&
- test_must_fail git update-ref $n $A
+test_expect_success "fail to create $n due to file/directory conflict" '
+ test_when_finished "git update-ref -d refs/heads/gu" &&
+ git update-ref refs/heads/gu $A &&
+ test_must_fail git update-ref refs/heads/gu/fixes $A
'
test_expect_success "create $m (by HEAD)" '
@@ -222,7 +220,7 @@ test_expect_success 'delete symref without dereference when the referred ref is
test_expect_success 'update-ref -d is not confused by self-reference' '
git symbolic-ref refs/heads/self refs/heads/self &&
- test_when_finished "rm -f .git/refs/heads/self" &&
+ test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF refs/heads/self" &&
test_path_is_file .git/refs/heads/self &&
test_must_fail git update-ref -d refs/heads/self &&
test_path_is_file .git/refs/heads/self
@@ -230,7 +228,7 @@ test_expect_success 'update-ref -d is not confused by self-reference' '
test_expect_success 'update-ref --no-deref -d can delete self-reference' '
git symbolic-ref refs/heads/self refs/heads/self &&
- test_when_finished "rm -f .git/refs/heads/self" &&
+ test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF refs/heads/self" &&
test_path_is_file .git/refs/heads/self &&
git update-ref --no-deref -d refs/heads/self &&
test_must_fail git show-ref --verify -q refs/heads/self
@@ -434,7 +432,7 @@ test_expect_success 'Query "main@{2005-05-28}" (past end of history)' '
test_i18ngrep -F "warning: log for ref $m unexpectedly ended on $ld" e
'
-rm -f .git/$m .git/logs/$m expect
+git update-ref -d $m
test_expect_success 'creating initial files' '
test_when_finished rm -f M &&
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 10a539158c4..5cce24f1006 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -115,15 +115,16 @@ test_expect_success 'zlib corrupt loose object output ' '
'
test_expect_success 'branch pointing to non-commit' '
- git rev-parse HEAD^{tree} >.git/refs/heads/invalid &&
+ tree_oid=$(git rev-parse --verify HEAD^{tree}) &&
+ test-tool ref-store main update-ref msg refs/heads/invalid $tree_oid $ZERO_OID REF_SKIP_OID_VERIFICATION &&
test_when_finished "git update-ref -d refs/heads/invalid" &&
test_must_fail git fsck 2>out &&
test_i18ngrep "not a commit" out
'
test_expect_success 'HEAD link pointing at a funny object' '
- test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
- mv .git/HEAD .git/SAVED_HEAD &&
+ saved_head=$(git rev-parse --verify HEAD) &&
+ test_when_finished "git update-ref HEAD ${saved_head}" &&
echo $ZERO_OID >.git/HEAD &&
# avoid corrupt/broken HEAD from interfering with repo discovery
test_must_fail env GIT_DIR=.git git fsck 2>out &&
@@ -131,8 +132,8 @@ test_expect_success 'HEAD link pointing at a funny object' '
'
test_expect_success 'HEAD link pointing at a funny place' '
- test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
- mv .git/HEAD .git/SAVED_HEAD &&
+ saved_head=$(git rev-parse --verify HEAD) &&
+ test_when_finished "git update-ref --no-deref HEAD ${saved_head}" &&
echo "ref: refs/funny/place" >.git/HEAD &&
# avoid corrupt/broken HEAD from interfering with repo discovery
test_must_fail env GIT_DIR=.git git fsck 2>out &&
@@ -140,10 +141,10 @@ test_expect_success 'HEAD link pointing at a funny place' '
'
test_expect_success 'HEAD link pointing at a funny object (from different wt)' '
- test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
+ saved_head=$(git rev-parse --verify HEAD) &&
+ test_when_finished "git update-ref HEAD $saved_head" &&
test_when_finished "rm -rf .git/worktrees wt" &&
git worktree add wt &&
- mv .git/HEAD .git/SAVED_HEAD &&
echo $ZERO_OID >.git/HEAD &&
# avoid corrupt/broken HEAD from interfering with repo discovery
test_must_fail git -C wt fsck 2>out &&
@@ -161,7 +162,8 @@ test_expect_success 'other worktree HEAD link pointing at a funny object' '
test_expect_success 'other worktree HEAD link pointing at missing object' '
test_when_finished "rm -rf .git/worktrees other" &&
git worktree add other &&
- echo "Contents missing from repo" | git hash-object --stdin >.git/worktrees/other/HEAD &&
+ object_id=$(echo "Contents missing from repo" | git hash-object --stdin) &&
+ test-tool -C other ref-store main update-ref msg HEAD $object_id "" REF_NO_DEREF,REF_SKIP_OID_VERIFICATION &&
test_must_fail git fsck 2>out &&
test_i18ngrep "worktrees/other/HEAD: invalid sha1 pointer" out
'
@@ -391,7 +393,7 @@ test_expect_success 'tag pointing to nonexistent' '
tag=$(git hash-object -t tag -w --stdin <invalid-tag) &&
test_when_finished "remove_object $tag" &&
- echo $tag >.git/refs/tags/invalid &&
+ git update-ref refs/tags/invalid $tag &&
test_when_finished "git update-ref -d refs/tags/invalid" &&
test_must_fail git fsck --tags >out &&
test_i18ngrep "broken link" out
@@ -411,7 +413,7 @@ test_expect_success 'tag pointing to something else than its type' '
tag=$(git hash-object -t tag -w --stdin <wrong-tag) &&
test_when_finished "remove_object $tag" &&
- echo $tag >.git/refs/tags/wrong &&
+ git update-ref refs/tags/wrong $tag &&
test_when_finished "git update-ref -d refs/tags/wrong" &&
test_must_fail git fsck --tags
'
@@ -428,7 +430,7 @@ test_expect_success 'tag with incorrect tag name & missing tagger' '
tag=$(git hash-object --literally -t tag -w --stdin <wrong-tag) &&
test_when_finished "remove_object $tag" &&
- echo $tag >.git/refs/tags/wrong &&
+ git update-ref refs/tags/wrong $tag &&
test_when_finished "git update-ref -d refs/tags/wrong" &&
git fsck --tags 2>out &&
@@ -452,7 +454,7 @@ test_expect_success 'tag with bad tagger' '
tag=$(git hash-object --literally -t tag -w --stdin <wrong-tag) &&
test_when_finished "remove_object $tag" &&
- echo $tag >.git/refs/tags/wrong &&
+ git update-ref refs/tags/wrong $tag &&
test_when_finished "git update-ref -d refs/tags/wrong" &&
test_must_fail git fsck --tags 2>out &&
test_i18ngrep "error in tag .*: invalid author/committer" out
@@ -471,7 +473,7 @@ test_expect_success 'tag with NUL in header' '
tag=$(git hash-object --literally -t tag -w --stdin <tag-NUL-header) &&
test_when_finished "remove_object $tag" &&
- echo $tag >.git/refs/tags/wrong &&
+ git update-ref refs/tags/wrong $tag &&
test_when_finished "git update-ref -d refs/tags/wrong" &&
test_must_fail git fsck --tags 2>out &&
test_i18ngrep "error in tag $tag.*unterminated header: NUL at offset" out
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8ea2bf13026..d2a7a91f170 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2160,7 +2160,7 @@ test_expect_success '--update-refs: check failed ref update' '
# recorded in the update-refs file. We will force-update the
# "second" ref, but "git branch -f" will not work because of
# the lock in the update-refs file.
- git rev-parse third >.git/refs/heads/second &&
+ git update-ref refs/heads/second third &&
test_must_fail git rebase --continue 2>err &&
grep "update_ref failed for ref '\''refs/heads/second'\''" err &&
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 26e933f93ae..7ab220fa313 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
git -C dst fetch --recurse-submodules &&
# Break the receiving submodule
- rm -f dst/sub/.git/HEAD &&
+ test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
# NOTE: without the fix the following tests will recurse forever!
# They should terminate with an error.
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 03/11] t: convert tests to use helpers for reference existence
From: Patrick Steinhardt @ 2023-10-18 5:35 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1697607222.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 12736 bytes --]
Convert tests that use `test_path_is_file` and `test_path_is_missing` to
instead use our new helpers `test_ref_exists` and `test_ref_missing`.
These hook into the reference database directly and thus work indepently
of the actual reference backend that is being used.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1430-bad-ref-name.sh | 27 ++++++++++++++++++---------
t/t3200-branch.sh | 33 ++++++++++++++++++---------------
t/t5521-pull-options.sh | 4 ++--
t/t5605-clone-local.sh | 2 +-
4 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index ff1c967d550..7b7d6953c62 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -205,8 +205,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref to broken name'
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -216,8 +217,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git branch -d badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -225,8 +227,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_expect_success 'update-ref --no-deref -d can delete dangling symref to broken name' '
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -234,8 +237,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref to brok
test_expect_success 'branch -d can delete dangling symref to broken name' '
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git branch -d badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -245,8 +249,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/broken...ref &&
git update-ref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...ref &&
+ test_ref_missing refs/heads/broken...ref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -254,8 +259,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
test_expect_success 'update-ref --no-deref -d can delete symref with broken name' '
printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -263,8 +269,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref with broken name
test_expect_success 'branch -d can delete symref with broken name' '
printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git branch -d broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_i18ngrep "Deleted branch broken...symref (was refs/heads/main)" output &&
test_must_be_empty error
'
@@ -272,8 +279,9 @@ test_expect_success 'branch -d can delete symref with broken name' '
test_expect_success 'update-ref --no-deref -d can delete dangling symref with broken name' '
printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -281,8 +289,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref with br
test_expect_success 'branch -d can delete dangling symref with broken name' '
printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git branch -d broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_i18ngrep "Deleted branch broken...symref (was refs/heads/idonotexist)" output &&
test_must_be_empty error
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 080e4f24a6e..bde4f1485b7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -25,7 +25,7 @@ test_expect_success 'prepare a trivial repository' '
test_expect_success 'git branch --help should not have created a bogus branch' '
test_might_fail git branch --man --help </dev/null >/dev/null 2>&1 &&
- test_path_is_missing .git/refs/heads/--help
+ test_ref_missing refs/heads/--help
'
test_expect_success 'branch -h in broken repository' '
@@ -40,7 +40,8 @@ test_expect_success 'branch -h in broken repository' '
'
test_expect_success 'git branch abc should create a branch' '
- git branch abc && test_path_is_file .git/refs/heads/abc
+ git branch abc &&
+ test_ref_exists refs/heads/abc
'
test_expect_success 'git branch abc should fail when abc exists' '
@@ -61,11 +62,13 @@ test_expect_success 'git branch --force abc should succeed when abc exists' '
'
test_expect_success 'git branch a/b/c should create a branch' '
- git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c
+ git branch a/b/c &&
+ test_ref_exists refs/heads/a/b/c
'
test_expect_success 'git branch mb main... should create a branch' '
- git branch mb main... && test_path_is_file .git/refs/heads/mb
+ git branch mb main... &&
+ test_ref_exists refs/heads/mb
'
test_expect_success 'git branch HEAD should fail' '
@@ -78,14 +81,14 @@ EOF
test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
- test_path_is_file .git/refs/heads/d/e/f &&
+ test_ref_exists refs/heads/d/e/f &&
test_path_is_file .git/logs/refs/heads/d/e/f &&
test_cmp expect .git/logs/refs/heads/d/e/f
'
test_expect_success 'git branch -d d/e/f should delete a branch and a log' '
git branch -d d/e/f &&
- test_path_is_missing .git/refs/heads/d/e/f &&
+ test_ref_missing refs/heads/d/e/f &&
test_must_fail git reflog exists refs/heads/d/e/f
'
@@ -213,7 +216,7 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
test_commit initial &&
git checkout --orphan lonely &&
grep lonely .git/HEAD &&
- test_path_is_missing .git/refs/head/lonely &&
+ test_ref_missing refs/head/lonely &&
git branch -M main mistress &&
grep lonely .git/HEAD
)
@@ -799,8 +802,8 @@ test_expect_success 'deleting a symref' '
git symbolic-ref refs/heads/symref refs/heads/target &&
echo "Deleted branch symref (was refs/heads/target)." >expect &&
git branch -d symref >actual &&
- test_path_is_file .git/refs/heads/target &&
- test_path_is_missing .git/refs/heads/symref &&
+ test_ref_exists refs/heads/target &&
+ test_ref_missing refs/heads/symref &&
test_cmp expect actual
'
@@ -809,16 +812,16 @@ test_expect_success 'deleting a dangling symref' '
test_path_is_file .git/refs/heads/dangling-symref &&
echo "Deleted branch dangling-symref (was nowhere)." >expect &&
git branch -d dangling-symref >actual &&
- test_path_is_missing .git/refs/heads/dangling-symref &&
+ test_ref_missing refs/heads/dangling-symref &&
test_cmp expect actual
'
test_expect_success 'deleting a self-referential symref' '
git symbolic-ref refs/heads/self-reference refs/heads/self-reference &&
- test_path_is_file .git/refs/heads/self-reference &&
+ test_ref_exists refs/heads/self-reference &&
echo "Deleted branch self-reference (was refs/heads/self-reference)." >expect &&
git branch -d self-reference >actual &&
- test_path_is_missing .git/refs/heads/self-reference &&
+ test_ref_missing refs/heads/self-reference &&
test_cmp expect actual
'
@@ -826,8 +829,8 @@ test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/topic refs/heads/main &&
test_must_fail git branch -m topic new-topic &&
git symbolic-ref refs/heads/topic &&
- test_path_is_file .git/refs/heads/main &&
- test_path_is_missing .git/refs/heads/new-topic
+ test_ref_exists refs/heads/main &&
+ test_ref_missing refs/heads/new-topic
'
test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
@@ -1142,7 +1145,7 @@ EOF
test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git checkout -b g/h/i -l main &&
- test_path_is_file .git/refs/heads/g/h/i &&
+ test_ref_exists refs/heads/g/h/i &&
test_path_is_file .git/logs/refs/heads/g/h/i &&
test_cmp expect .git/logs/refs/heads/g/h/i
'
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 079b2f2536e..3681859f983 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -143,7 +143,7 @@ test_expect_success 'git pull --dry-run' '
cd clonedry &&
git pull --dry-run ../parent &&
test_path_is_missing .git/FETCH_HEAD &&
- test_path_is_missing .git/refs/heads/main &&
+ test_ref_missing refs/heads/main &&
test_path_is_missing .git/index &&
test_path_is_missing file
)
@@ -157,7 +157,7 @@ test_expect_success 'git pull --all --dry-run' '
git remote add origin ../parent &&
git pull --all --dry-run &&
test_path_is_missing .git/FETCH_HEAD &&
- test_path_is_missing .git/refs/remotes/origin/main &&
+ test_ref_missing refs/remotes/origin/main &&
test_path_is_missing .git/index &&
test_path_is_missing file
)
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index 1d7b1abda1a..946c5751885 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -69,7 +69,7 @@ test_expect_success 'local clone of repo with nonexistent ref in HEAD' '
git clone a d &&
(cd d &&
git fetch &&
- test ! -e .git/refs/remotes/origin/HEAD)
+ test_ref_missing refs/remotes/origin/HEAD)
'
test_expect_success 'bundle clone without .bundle suffix' '
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 02/11] t: allow skipping expected object ID in `ref-store update-ref`
From: Patrick Steinhardt @ 2023-10-18 5:35 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1697607222.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]
We require the caller to pass both the old and new expected object ID to
our `test-tool ref-store update-ref` helper. When trying to update a
symbolic reference though it's impossible to specify the expected object
ID, which means that the test would instead have to force-update the
reference. This is currently impossible though.
Update the helper to optionally skip verification of the old object ID
in case the test passes in an empty old object ID as input.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/helper/test-ref-store.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 7400f560ab6..7dc83137584 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -322,16 +322,19 @@ static int cmd_update_ref(struct ref_store *refs, const char **argv)
const char *new_sha1_buf = notnull(*argv++, "new-sha1");
const char *old_sha1_buf = notnull(*argv++, "old-sha1");
unsigned int flags = arg_flags(*argv++, "flags", transaction_flags);
- struct object_id old_oid;
+ struct object_id old_oid, *old_oid_ptr = NULL;
struct object_id new_oid;
- if (get_oid_hex(old_sha1_buf, &old_oid))
- die("cannot parse %s as %s", old_sha1_buf, the_hash_algo->name);
+ if (*old_sha1_buf) {
+ if (get_oid_hex(old_sha1_buf, &old_oid))
+ die("cannot parse %s as %s", old_sha1_buf, the_hash_algo->name);
+ old_oid_ptr = &old_oid;
+ }
if (get_oid_hex(new_sha1_buf, &new_oid))
die("cannot parse %s as %s", new_sha1_buf, the_hash_algo->name);
return refs_update_ref(refs, msg, refname,
- &new_oid, &old_oid,
+ &new_oid, old_oid_ptr,
flags, UPDATE_REFS_DIE_ON_ERR);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 01/11] t: add helpers to test for reference existence
From: Patrick Steinhardt @ 2023-10-18 5:35 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1697607222.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6798 bytes --]
There are two major ways to check for the existence of a reference in
our tests:
- `git rev-parse --verify` can be used to check for existence of a
reference. This only works in the case where the reference is well
formed though and resolves to an actual object ID. This does not
work with malformed reference names or invalid contents.
- `test_path_is_file` can be used to check for existence of a loose
reference if it is known to not resolve to an actual object ID. It
by necessity reaches into implementation details of the reference
backend though.
Similarly, there are two equivalent ways to check for the absence of a
reference:
- `test_must_fail git rev-parse` can be used to check for the
absence of a reference. It could fail due to a number of reasons
though, and all of these reasons will be thrown into the same bag
as an absent reference.
- `test_path_is_missing` can be used to check explicitly for the
absence of a loose reference, but again reaches into internal
implementation details of the reference backend.
So both our tooling to check for the presence and for the absence of
references in tests is lacking as either failure cases are thrown into
the same bag or we need to reach into internal implementation details of
the respective reference backend.
Introduce a new subcommand for our ref-store test helper that explicitly
checks only for the presence or absence of a reference. This addresses
these limitations:
- We can check for the presence of references with malformed names.
- We can check for the presence of references that don't resolve.
- We can explicitly handle the case where a reference is missing by
special-casing ENOENT errors.
- We don't need to reach into implementation details of the backend,
which would allow us to use this helper for the future reftable
backend.
Next to this subcommand we also provide two wrappers `test_ref_exists`
and `test_ref_missing` that make the helper easier to use.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/README | 9 ++++++
t/helper/test-ref-store.c | 27 +++++++++++++++-
t/test-lib-functions.sh | 66 +++++++++++++++++++++++++++++++++++++++
3 files changed, 101 insertions(+), 1 deletion(-)
diff --git a/t/README b/t/README
index 61080859899..779f7e7dd86 100644
--- a/t/README
+++ b/t/README
@@ -928,6 +928,15 @@ see test-lib-functions.sh for the full list and their options.
committer times to defined state. Subsequent calls will
advance the times by a fixed amount.
+ - test_ref_exists <ref>, test_ref_missing <ref>
+
+ Check whether a reference exists or is missing. In contrast to
+ git-rev-parse(1), these helpers also work with invalid reference
+ names and references whose contents are unresolvable. The latter
+ function also distinguishes generic errors from the case where a
+ reference explicitly doesn't exist and is thus safer to use than
+ `test_must_fail git rev-parse`.
+
- test_commit <message> [<filename> [<contents>]]
Creates a commit with the given message, committing the given
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 48552e6a9e0..7400f560ab6 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -1,6 +1,6 @@
#include "test-tool.h"
#include "hex.h"
-#include "refs.h"
+#include "refs/refs-internal.h"
#include "setup.h"
#include "worktree.h"
#include "object-store-ll.h"
@@ -221,6 +221,30 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
return ret;
}
+static int cmd_ref_exists(struct ref_store *refs, const char **argv)
+{
+ const char *refname = notnull(*argv++, "refname");
+ struct strbuf unused_referent = STRBUF_INIT;
+ struct object_id unused_oid;
+ unsigned int unused_type;
+ int failure_errno;
+
+ if (refs_read_raw_ref(refs, refname, &unused_oid, &unused_referent,
+ &unused_type, &failure_errno)) {
+ /*
+ * We handle ENOENT separately here such that it is possible to
+ * distinguish actually-missing references from any kind of
+ * generic error.
+ */
+ if (failure_errno == ENOENT)
+ return 17;
+ return -1;
+ }
+
+ strbuf_release(&unused_referent);
+ return 0;
+}
+
static int cmd_for_each_reflog(struct ref_store *refs,
const char **argv UNUSED)
{
@@ -325,6 +349,7 @@ static struct command commands[] = {
{ "for-each-ref--exclude", cmd_for_each_ref__exclude },
{ "resolve-ref", cmd_resolve_ref },
{ "verify-ref", cmd_verify_ref },
+ { "ref-exists", cmd_ref_exists },
{ "for-each-reflog", cmd_for_each_reflog },
{ "for-each-reflog-ent", cmd_for_each_reflog_ent },
{ "for-each-reflog-ent-reverse", cmd_for_each_reflog_ent_reverse },
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2f8868caa17..212fddffa96 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -251,6 +251,72 @@ debug () {
done
}
+# Usage: test_ref_exists [options] <ref>
+# -C <dir>:
+# Run all git commands in directory <dir>
+# --refdb <refdb>:
+# The reference database to run in. One of:
+# - "main", the main reference database (default).
+# - "submodule:<submodule>", the reference database of a
+# submodule.
+# - "worktree:<worktree>", the reference database for a
+# worktree's per-worktree references.
+#
+# This helper function checks whether a reference exists. Symrefs will not be
+# resolved. Can be used to check references with bad names.
+test_ref_exists () {
+ local indir=
+ local refdb=main
+
+ while test $# != 0
+ do
+ case "$1" in
+ -C)
+ indir="$2"
+ shift
+ ;;
+ --refdb)
+ refdb="$2"
+ shift
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done &&
+
+ indir=${indir:+"$indir"/} &&
+
+ if test "$#" != 1
+ then
+ BUG "expected exactly one reference"
+ fi &&
+
+ test-tool ${indir:+ -C "$indir"} ref-store "${refdb}" ref-exists "$1"
+}
+
+# Behaves the same as test_ref_exists, except that it checks for the absence of
+# a reference. This is preferable to `! test_ref_exists` as this function is
+# able to distinguish actually-missing references from other, generic errors.
+test_ref_missing () {
+ test_ref_exists "$@"
+ case "$?" in
+ 17)
+ # This is the good case.
+ return 0
+ ;;
+ 0)
+ echo >&4 "test_ref_missing: reference exists"
+ return 1
+ ;;
+ *)
+ echo >&4 "test_ref_missing: generic error"
+ return 1
+ ;;
+ esac
+}
+
# Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]
# -C <dir>:
# Run all git commands in directory <dir>
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 00/11] t: reduce direct disk access to data structures
From: Patrick Steinhardt @ 2023-10-18 5:35 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
[-- Attachment #1: Type: text/plain, Size: 2841 bytes --]
Hi,
this patch series refactors a bunch of our tests to perform less direct
disk access to on-disk data structures. Instead, the tests are converted
to use Git tools or our test-tool to access data to the best extent
possible. This serves two benefits:
- We increase test coverage of our own code base.
- We become less dependent on the actual on-disk format.
The main motivation for this patch series was the second bullet point as
it is preparatory work to get the reftable backend upstreamed. My intent
is to get rid of many or even most of the current blockers in the Git
project before trying to send the reftable implementation upstream.
While this will be a lot of up-front work that is going to span over a
long time period, I think this approach will make everyones live easier
by doing comparatively small and incremental improvements to the Git
project. Ultimately, the final patch series should in the best case only
contain the new backend as well as testing infrastructure, but not much
else.
Patrick
Patrick Steinhardt (11):
t: add helpers to test for reference existence
t: allow skipping expected object ID in `ref-store update-ref`
t: convert tests to use helpers for reference existence
t: convert tests to not write references via the filesystem
t: convert tests to not access symrefs via the filesystem
t: convert tests to not access reflog via the filesystem
t1450: convert tests to remove worktrees via git-worktree(1)
t4207: delete replace references via git-update-ref(1)
t7300: assert exact states of repo
t7900: assert the absence of refs via git-for-each-ref(1)
t: mark several tests that assume the files backend with REFFILES
t/README | 9 ++++
t/helper/test-ref-store.c | 38 +++++++++++++--
t/t1400-update-ref.sh | 49 ++++++++++----------
t/t1430-bad-ref-name.sh | 39 ++++++++++------
t/t1450-fsck.sh | 46 ++++++++++---------
t/t2011-checkout-invalid-head.sh | 16 +++----
t/t3200-branch.sh | 74 ++++++++++++++++--------------
t/t3400-rebase.sh | 2 +-
t/t3404-rebase-interactive.sh | 2 +-
t/t4013-diff-various.sh | 2 +-
t/t4202-log.sh | 2 +-
t/t4207-log-decoration-colors.sh | 4 +-
t/t5521-pull-options.sh | 4 +-
t/t5526-fetch-submodules.sh | 2 +-
t/t5605-clone-local.sh | 6 +--
t/t5702-protocol-v2.sh | 24 +++++++---
t/t7300-clean.sh | 23 ++++++----
t/t7900-maintenance.sh | 3 +-
t/t9133-git-svn-nested-git-repo.sh | 2 +-
t/test-lib-functions.sh | 66 ++++++++++++++++++++++++++
20 files changed, 277 insertions(+), 136 deletions(-)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [Outreachy][PATCH] branch.c: adjust error messages to coding guidelines
From: Isoken June Ibizugbe @ 2023-10-18 5:12 UTC (permalink / raw)
To: git; +Cc: rjusto, christian.couder, Isoken June Ibizugbe
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
1 file changed, 33 insertions(+), 33 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..938c40bfaa 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
(head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
if (merged)
warning(_("deleting branch '%s' that has been merged to\n"
- " '%s', but not yet merged to HEAD."),
+ " '%s', but not yet merged to HEAD"),
name, reference_name);
else
warning(_("not deleting branch '%s' that is not yet merged to\n"
- " '%s', even though it is merged to HEAD."),
+ " '%s', even though it is merged to HEAD"),
name, reference_name);
}
free(reference_name_to_free);
@@ -190,13 +190,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
{
struct commit *rev = lookup_commit_reference(the_repository, oid);
if (!force && !rev) {
- error(_("Couldn't look up commit object for '%s'"), refname);
+ error(_("couldn't look up commit object for '%s'"), refname);
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("The branch '%s' is not fully merged.\n"
+ error(_("the branch '%s' is not fully merged.\n"
"If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
+ "run 'git branch -D %s'"), branchname, branchname);
return -1;
}
return 0;
@@ -207,7 +207,7 @@ static void delete_branch_config(const char *branchname)
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "branch.%s", branchname);
if (git_config_rename_section(buf.buf, NULL) < 0)
- warning(_("Update of config-file failed"));
+ warning(_("update of config-file failed"));
strbuf_release(&buf);
}
@@ -260,7 +260,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const char *path;
if ((path = branch_checked_out(name))) {
- error(_("Cannot delete branch '%s' "
+ error(_("cannot delete branch '%s' "
"used by worktree at '%s'"),
bname.buf, path);
ret = 1;
@@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
&oid, &flags);
if (!target) {
if (remote_branch) {
- error(_("remote-tracking branch '%s' not found."), bname.buf);
+ error(_("remote-tracking branch '%s' not found"), bname.buf);
} else {
char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
char *virtual_target = resolve_refdup(virtual_name,
@@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
"Did you forget --remote?"),
bname.buf);
else
- error(_("branch '%s' not found."), bname.buf);
+ error(_("branch '%s' not found"), bname.buf);
FREE_AND_NULL(virtual_target);
}
ret = 1;
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch %s is being rebased at %s"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch %s is being bisected at %s"),
target, wt->path);
}
}
@@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (ref_exists(oldref.buf))
recovery = 1;
else
- die(_("Invalid branch name: '%s'"), oldname);
+ die(_("invalid branch name: '%s'"), oldname);
}
for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
if (oldref_usage & IS_HEAD)
- die(_("No commit on branch '%s' yet."), oldname);
+ die(_("no commit on branch '%s' yet"), oldname);
else
- die(_("No branch named '%s'."), oldname);
+ die(_("no branch named '%s'"), oldname);
}
/*
@@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && !(oldref_usage & IS_ORPHAN) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
if (recovery) {
if (copy)
- warning(_("Created a copy of a misnamed branch '%s'"),
+ warning(_("created a copy of a misnamed branch '%s'"),
interpreted_oldname);
else
- warning(_("Renamed a misnamed branch '%s' away"),
+ warning(_("renamed a misnamed branch '%s' away"),
interpreted_oldname);
}
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated"), newname);
strbuf_release(&logmsg);
strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldref);
strbuf_release(&newref);
strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
@@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf))
error((!argc || branch_checked_out(branch_ref.buf))
- ? _("No commit on branch '%s' yet.")
- : _("No branch named '%s'."),
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"),
branch_name);
else if (!edit_branch_description(branch_name))
ret = 0; /* happy */
@@ -892,8 +892,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
- die(copy? _("cannot copy the current branch while not on any.")
- : _("cannot rename the current branch while not on any."));
+ die(copy? _("cannot copy the current branch while not on any")
+ : _("cannot rename the current branch while not on any"));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
else if (argc == 2)
@@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
+ "it does not point to any branch"),
new_upstream);
die(_("no such branch '%s'"), argv[0]);
}
if (!ref_exists(branch->refname)) {
if (!argc || branch_checked_out(branch->refname))
- die(_("No commit on branch '%s' yet."), branch->name);
+ die(_("no commit on branch '%s' yet"), branch->name);
die(_("branch '%s' does not exist"), branch->name);
}
@@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
+ "it does not point to any branch"));
die(_("no such branch '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
if (recurse_submodules) {
create_branches_recursively(the_repository, branch_name,
--
2.42.0.346.g24618a8a3e.dirty
^ permalink raw reply related
* [PATCH] commit: do not use cryptic "new_index" in end-user facing messages
From: Junio C Hamano @ 2023-10-18 3:08 UTC (permalink / raw)
To: Jiang Xin; +Cc: git
In-Reply-To: <xmqqwmvkve83.fsf@gitster.g>
These error messages say "new_index" as if that spelling has some
significance to the end users (e.g. the file "$GIT_DIR/new_index"
has some issues), but that is not the case at all. The i18n folks
were made to include the word literally in the translated messages,
which was not a good idea at all. Spell it "new index", as we are
just telling the users that we failed to create a new index file.
The term is expected to be translated to the end-users' languages,
not left as if it were a literal file name.
This dates all the way back to the first re-implemenation of "git
commit" command in C (the scripted version did not have such wording
in its error messages), in f5bbc322 (Port git commit to C.,
2007-11-08).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano <gitster@pobox.com> writes:
> Jiang Xin <worldhello.net@gmail.com> writes:
> ...
> Wait. Is this supposed to be a good example of validator working
> well? We use this exact message three times in builtin/commit.c; is
> the validator insisting on the translated message to have verbatim
> string "new_index" in it so that the end-users will see it?
> ...
> I'd suggest doing s/new_index/new index/ to msgid string for these
> anyway.
Just so that we do not forget, in case my "using new_index in
the message is wrong" is not my misunderstanding, here is such a
patch. Note that "checkout", "clone", "read-tree", and "stash"
all use the "new index" spelling so this is not introducing any
new message. "git merge" and "git pull" that fast-forward also
use this same message when they cannot write a new index file.
builtin/commit.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 985a0445b7..7abd566bc7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -446,7 +446,7 @@ static const char *prepare_index(const char **argv, const char *prefix,
refresh_cache_or_die(refresh_flags);
cache_tree_update(&the_index, WRITE_TREE_SILENT);
if (write_locked_index(&the_index, &index_lock, 0))
- die(_("unable to write new_index file"));
+ die(_("unable to write new index file"));
commit_style = COMMIT_NORMAL;
ret = get_lock_file_path(&index_lock);
goto out;
@@ -470,7 +470,7 @@ static const char *prepare_index(const char **argv, const char *prefix,
cache_tree_update(&the_index, WRITE_TREE_SILENT);
if (write_locked_index(&the_index, &index_lock,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("unable to write new_index file"));
+ die(_("unable to write new index file"));
commit_style = COMMIT_AS_IS;
ret = get_index_file();
goto out;
@@ -518,7 +518,7 @@ static const char *prepare_index(const char **argv, const char *prefix,
refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
cache_tree_update(&the_index, WRITE_TREE_SILENT);
if (write_locked_index(&the_index, &index_lock, 0))
- die(_("unable to write new_index file"));
+ die(_("unable to write new index file"));
hold_lock_file_for_update(&false_lock,
git_path("next-index-%"PRIuMAX,
@@ -1852,7 +1852,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
if (commit_index_files())
die(_("repository has been updated, but unable to write\n"
- "new_index file. Check that disk is not full and quota is\n"
+ "new index file. Check that disk is not full and quota is\n"
"not exceeded, and then \"git restore --staged :/\" to recover."));
git_test_write_commit_graph_or_die();
--
2.42.0-398-ga9ecda2788
^ permalink raw reply related
* Re: Is there any interest in localizing term delimiters in git messages?
From: Junio C Hamano @ 2023-10-18 2:47 UTC (permalink / raw)
To: Jiang Xin
Cc: Alexander Shopov, Git List, jmas, alexhenrie24, ralf.thielow,
matthias.ruester, phillip.szelat, vyruss, christopher.diaz.riv,
jn.avila, flashcode, bagasdotme,
Ævar Arnfjörð Bjarmason, alessandro.menti,
elongbug, cwryu, uneedsihyeon, arek_koz, dacs.git,
insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, pan93412@gmail.com,
franklin@goodhorse.idv.tw
In-Reply-To: <CANYiYbHK90Ptq5v4EbquyRA7N9jo=xwkg=WuM=r60Wh9HMxdyA@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Starting with the release of git 2.34.0 two years ago, we had a new
> l10n pipeline and the git-po-helper tool as part of our l10n workflow.
> The first version of git-po-helper introduced a validator to protect
> git command parameters and variable names in megid.
Ahh, that is the piece I was missing. I didn't know you guys are
doing extra checks that could trigger false positives.
> E.g. In pull
> request 541 (https://github.com/git-l10n/git-po/pull/541), a
> mismatched variable name "new_index" was reported in bg.po as below:
>
> level=warning msg="mismatch variable names in msgstr: new_index"
> level=warning msg=">> msgid: unable to write new_index file"
> level=warning msg=">> msgstr: новият индекс не може да бъде записан"
>
> And po/bg.po changed as below:
>
> msgid "unable to write new_index file"
> msgstr "новият индекс (new_index) не може да бъде записан"
Wait. Is this supposed to be a good example of validator working
well? We use this exact message three times in builtin/commit.c; is
the validator insisting on the translated message to have verbatim
string "new_index" in it so that the end-users will see it?
I may still be confused, but if that is what is going on, I think it
is a wrong validation in this particular case. I can understand if
we were creating say .git/new_index file and it helps the end users
to diagnose a troubled repository by running "ls .git" to see if a
file called "new_index" exists and getting in the way, but I do not
think it is the case. A new file ".git/index.lock" is created via
repo_hold_locked_index() and I do not think it helps the end-user to
know that we may be calling it "new_index" internally among the
developers' circle. If the message were about "index.lock", it
might be a different story, but such an error would probably have
been issued long before write_locked_index() gets called.
I'd suggest doing s/new_index/new index/ to msgid string for these
anyway.
> Later, more validators were introduced into git-po-helper for checking
> git config name, place holders, etc. "git-po-helper" used a list of
> regular expressions to find git config names, placeholders, and there
> are some false positive cases need to be ignored.
OK, and "<file>" in msgid string, for example, will automatically
insist on the translated msgstr string to have a string that is
enclosed by a pair of such angle brackets, regardless of the target
language convention? If so, I can now understand where Alexander
comes from (assuming that the common convention in Bulgarian language
is not to use a pair of angle brackets to highlight such a placeholder
word).
I can see that you have a lot better handle on the matter than I do,
so I trust you and Alexander can resolve what the best "validation"
(and possibly override per language) should be in the git-po-helper
tool.
Thanks for explaining.
^ permalink raw reply
* Re: [PATCH v2 5/7] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Junio C Hamano @ 2023-10-18 2:18 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <239bf39bfb21ef621a15839bade34446dcbc3103.1697560266.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> bulk-checkin.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
> bulk-checkin.h | 4 ++
> 2 files changed, 122 insertions(+)
Unlike the previous four, which were very clear refactoring to
create reusable helper functions, this step leaves a bad aftertaste
after reading twice, and I think what is disturbing is that many new
lines are pretty much literally copied from stream_blob_to_pack().
I wonder if we can introduce an "input" source abstraction, that
replaces "fd" and "size" (and "path" for error reporting) parameters
to the stream_blob_to_pack(), so that the bulk of the implementation
of stream_blob_to_pack() can call its .read() method to read bytes
up to "size" from such an abstracted interface? That would be a
good sized first half of this change. Then in the second half, you
can add another "input" source that works with in-core "buf" and
"size", whose .read() method will merely be a memcpy().
That way, we will have two functions, one for stream_obj_to_pack()
that reads from an open file descriptor, and the other for
stream_obj_to_pack_incore() that reads from an in-core buffer,
sharing the bulk of the implementation that is extracted from the
current code, which hopefully be easier to audit.
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index f4914fb6d1..25cd1ffa25 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -140,6 +140,69 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
> return 0;
> }
>
> +static int stream_obj_to_pack_incore(struct bulk_checkin_packfile *state,
> + git_hash_ctx *ctx,
> + off_t *already_hashed_to,
> + const void *buf, size_t size,
> + enum object_type type,
> + const char *path, unsigned flags)
> +{
> + git_zstream s;
> + unsigned char obuf[16384];
> + unsigned hdrlen;
> + int status = Z_OK;
> + int write_object = (flags & HASH_WRITE_OBJECT);
> +
> + git_deflate_init(&s, pack_compression_level);
> +
> + hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
> + s.next_out = obuf + hdrlen;
> + s.avail_out = sizeof(obuf) - hdrlen;
> +
> + if (*already_hashed_to < size) {
> + size_t hsize = size - *already_hashed_to;
> + if (hsize) {
> + the_hash_algo->update_fn(ctx, buf, hsize);
> + }
> + *already_hashed_to = size;
> + }
> + s.next_in = (void *)buf;
> + s.avail_in = size;
> +
> + while (status != Z_STREAM_END) {
> + status = git_deflate(&s, Z_FINISH);
> + if (!s.avail_out || status == Z_STREAM_END) {
> + if (write_object) {
> + size_t written = s.next_out - obuf;
> +
> + /* would we bust the size limit? */
> + if (state->nr_written &&
> + pack_size_limit_cfg &&
> + pack_size_limit_cfg < state->offset + written) {
> + git_deflate_abort(&s);
> + return -1;
> + }
> +
> + hashwrite(state->f, obuf, written);
> + state->offset += written;
> + }
> + s.next_out = obuf;
> + s.avail_out = sizeof(obuf);
> + }
> +
> + switch (status) {
> + case Z_OK:
> + case Z_BUF_ERROR:
> + case Z_STREAM_END:
> + continue;
> + default:
> + die("unexpected deflate failure: %d", status);
> + }
> + }
> + git_deflate_end(&s);
> + return 0;
> +}
> +
> /*
> * Read the contents from fd for size bytes, streaming it to the
> * packfile in state while updating the hash in ctx. Signal a failure
> @@ -316,6 +379,50 @@ static void finalize_checkpoint(struct bulk_checkin_packfile *state,
> }
> }
>
> +static int deflate_obj_contents_to_pack_incore(struct bulk_checkin_packfile *state,
> + git_hash_ctx *ctx,
> + struct hashfile_checkpoint *checkpoint,
> + struct object_id *result_oid,
> + const void *buf, size_t size,
> + enum object_type type,
> + const char *path, unsigned flags)
> +{
> + struct pack_idx_entry *idx = NULL;
> + off_t already_hashed_to = 0;
> +
> + /* Note: idx is non-NULL when we are writing */
> + if (flags & HASH_WRITE_OBJECT)
> + CALLOC_ARRAY(idx, 1);
> +
> + while (1) {
> + prepare_checkpoint(state, checkpoint, idx, flags);
> + if (!stream_obj_to_pack_incore(state, ctx, &already_hashed_to,
> + buf, size, type, path, flags))
> + break;
> + truncate_checkpoint(state, checkpoint, idx);
> + }
> +
> + finalize_checkpoint(state, ctx, checkpoint, idx, result_oid);
> +
> + return 0;
> +}
> +
> +static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
> + struct object_id *result_oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags)
> +{
> + git_hash_ctx ctx;
> + struct hashfile_checkpoint checkpoint = {0};
> +
> + format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,
> + size);
> +
> + return deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,
> + result_oid, buf, size,
> + OBJ_BLOB, path, flags);
> +}
> +
> static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
> struct object_id *result_oid,
> int fd, size_t size,
> @@ -396,6 +503,17 @@ int index_blob_bulk_checkin(struct object_id *oid,
> return status;
> }
>
> +int index_blob_bulk_checkin_incore(struct object_id *oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags)
> +{
> + int status = deflate_blob_to_pack_incore(&bulk_checkin_packfile, oid,
> + buf, size, path, flags);
> + if (!odb_transaction_nesting)
> + flush_bulk_checkin_packfile(&bulk_checkin_packfile);
> + return status;
> +}
> +
> void begin_odb_transaction(void)
> {
> odb_transaction_nesting += 1;
> diff --git a/bulk-checkin.h b/bulk-checkin.h
> index aa7286a7b3..1b91daeaee 100644
> --- a/bulk-checkin.h
> +++ b/bulk-checkin.h
> @@ -13,6 +13,10 @@ int index_blob_bulk_checkin(struct object_id *oid,
> int fd, size_t size,
> const char *path, unsigned flags);
>
> +int index_blob_bulk_checkin_incore(struct object_id *oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags);
> +
> /*
> * Tell the object database to optimize for adding
> * multiple objects. end_odb_transaction must be called
^ permalink raw reply
* Re: Is there any interest in localizing term delimiters in git messages?
From: Jiang Xin @ 2023-10-18 2:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Alexander Shopov, Git List, jmas, alexhenrie24, ralf.thielow,
matthias.ruester, phillip.szelat, vyruss, christopher.diaz.riv,
jn.avila, flashcode, bagasdotme,
Ævar Arnfjörð Bjarmason, alessandro.menti,
elongbug, cwryu, uneedsihyeon, arek_koz, dacs.git,
insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, pan93412@gmail.com,
franklin@goodhorse.idv.tw
In-Reply-To: <xmqqzg0gx6k9.fsf@gitster.g>
On Wed, Oct 18, 2023 at 5:50 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alexander Shopov <ash@kambanaria.org> writes:
>
> > Typical example:
> > ORIGINAL
> > msgid " (use \"git rm --cached <file>...\" to unstage)"
> >
> > TRANSLATION
> > msgstr ""
> > " (използвайте „git rm --cached %s ФАЙЛ…“, за да извадите ФАЙЛа от индекса)"
> >
> > The important part are the `<' and `>' delimiters of the term "file"
> >
> > Instead of using them - I omit them and capitalize the term. As if `<'
> > and `>' are declared as localizable and then I translate them as `',
> > `'
>
> Is it because it is more common in your target language to omit <>
> around the placeholder word, or is it just your personal preference?
>
> Whichever is the case, I am not sure how it affects ...
>
> > So I am asking - is there any interest from other localizers to have
> > such a feature? Would the additional maintenance be OK for the
> > developers?
>
> ... the maintenance burden for developers. Perhaps I am not getting
> what you are proposing, but we are not going to change the message
> in "C" locale (the original you see in msgid). In untranslated Git,
> we will keep the convention to highlight the placeholder word by
> having <> around it, so the "(use \"git rm --cached <file>...\" to
> unstage)" message will be spelled with "<file>". You can translate
> that to a msgstr without <> markings without asking anybody's
> permission, and I do not think of a reason why it would burden
> developers to do so.
Starting with the release of git 2.34.0 two years ago, we had a new
l10n pipeline and the git-po-helper tool as part of our l10n workflow.
The first version of git-po-helper introduced a validator to protect
git command parameters and variable names in megid. E.g. In pull
request 541 (https://github.com/git-l10n/git-po/pull/541), a
mismatched variable name "new_index" was reported in bg.po as below:
level=warning msg="mismatch variable names in msgstr: new_index"
level=warning msg=">> msgid: unable to write new_index file"
level=warning msg=">> msgstr: новият индекс не може да бъде записан"
And po/bg.po changed as below:
msgid "unable to write new_index file"
msgstr "новият индекс (new_index) не може да бъде записан"
Later, more validators were introduced into git-po-helper for checking
git config name, place holders, etc. "git-po-helper" used a list of
regular expressions to find git config names, placeholders, and there
are some false positive cases need to be ignored. So I added tweaks in
smarge tables in "dict/*.go" of git-po-helper. E.g. For German
translation, there are two exceptions that need to be ignored:
"e.g." was translated to "z.B.",
"you@example.com" was translated to "ihre@emailadresse.de"
In pull request 593 (https://github.com/git-l10n/git-po/pull/593), it
was the first time I know that in Bulgarian translations, markers
around <placeholder> were not suitable for Bulgarian. So I decided to
add more tweaks for Bulgarian by adding more exception rules in
"dict/smudge-bg.go".
I wonder if Bulgarian can use some unique characters to wrap the
placeholders (e.g. Chinese can use wrappers around placeholders
like「placeholder」，【placeholder】，etc). It will be much simpler to
define exception rules for Bulgarian. Otherwize, maybe I can add
filters for validators in "po-helper", and Bulgarian can bypass some
validators to suppress warnings in pull requests.
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files
From: brian m. carlson @ 2023-10-18 0:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Taylor Blau, Jason Hatton
In-Reply-To: <20231017000019.GB551672@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]
On 2023-10-17 at 00:00:19, Jeff King wrote:
> On Thu, Oct 12, 2023 at 04:09:30PM +0000, brian m. carlson wrote:
>
> > +static unsigned int munge_st_size(off_t st_size) {
> > + unsigned int sd_size = st_size;
> > +
> > + /*
> > + * If the file is an exact multiple of 4 GiB, modify the value so it
> > + * doesn't get marked as racily clean (zero).
> > + */
> > + if (!sd_size && st_size)
> > + return 0x80000000;
> > + else
> > + return sd_size;
> > +}
>
> Coverity complained that the "true" side of this conditional is
> unreachable, since sd_size is assigned from st_size, so the two values
> cannot be both true and false. But obviously we are depending here on
> the truncation of off_t to "unsigned int". I'm not sure if Coverity is
> just dumb, or if it somehow has a different size for off_t.
Technically, on 32-bit machines, you can have a 32-bit off_t if you
don't compile with -D_FILE_OFFSET_BITS=64. However, such a program is
not very useful on modern systems, since it wouldn't be able to handle
files that are 2 GiB or larger, and so everyone considers that a silly
and buggy way to compile programs.
> I don't _think_ this would ever cause confusion in a real compiler, as
> assignment from a larger type to a smaller has well-defined truncation,
> as far as I know.
>
> But I do wonder if an explicit "& 0xFFFFFFFF" would make it more obvious
> what is happening (which would also do the right thing if in some
> hypothetical platform "unsigned int" ended up larger than 32 bits).
Such a hypothetical platform is of course allowed by the C standard, but
it's also going to run near zero real Unix programs or kernels. I am at
this point reflecting on the prudent decision Rust made to make
almost everything an explicit bit size (e.g., u32, i64).
Nonetheless, I'll reroll this with the other things you mentioned, and
I'll switch that "unsigned int" above to "uint32_t", which I think makes
this more obvious.
I don't, however, intend to change the constant from 0x8000000 to 1,
because I think that's a poorer choice, but I'll try to explain it
better in the commit message. (I had originally aimed to avoid editing
it as much as possible since it's not my name on the commit to avoid
Jason getting blamed unnecessarily for any mistakes I might make.)
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Junio C Hamano @ 2023-10-17 23:02 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, Joanna Wang
In-Reply-To: <CAMmZTi8swsSMcLUcW+YwUDg8GcrY_ks2+i35-nsHE3o9MNpsUQ@mail.gmail.com>
Joanna Wang <jojwang@google.com> writes:
> I will dive into the code more to confirm, but that's my current
> high-level plan, in case you immediately see something very wrong.
Nothing I immediately see glaringly wrong in there ;-)
Thanks for digging!
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Joanna Wang @ 2023-10-17 22:48 UTC (permalink / raw)
To: gitster; +Cc: git, Joanna Wang, Joanna Wang
> (2) does collect_some_attrs() or git_check_attr() leave enough
> information in check[] to tell between [a] the attr stack had
> no opinion on "bits" attribute and [b] the attr stack
> explicitly said "bits" attribute is unspecified?
I will double check this but IIUC, I could change this part
(https://github.com/git/git/blob/master/attr.c#L1100-L1105),
such that:
```
if (n == ATTR__UNKNOWN && v = NULL):
// keep n equal to ATTR_UNKNOWN
```
And here: https://github.com/git/git/blob/master/attr.c#L1238,
if value == 'ATTR__UNKNOWN', then keep it at ATTR__UNKNOWN
> (1) where in that callchain would we intercept and insert our own
> "bits" value based on the filesystem property?
I was planning to do that somewhere around here:
https://github.com/git/git/blob/master/pathspec.c#L764-L767
and possibly combine it with adding a new match_mode (e.g.
MATCH_BITS) which would be set by parse_pathspec().
Something like:
```
else if (ATTR_UNKNOWN(value))
matched = (match_mode == MATCH_BITS &&
!strcmp(item->attr_match[i].value, get_mode(path)));
```
I will dive into the code more to confirm, but that's my current
high-level plan, in case you immediately see something very wrong.
^ permalink raw reply
* Re: [PATCH v2 1/1] [OUTREACHY] builtin/add.c: clean up die() messages
From: Junio C Hamano @ 2023-10-17 22:16 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <20231017221323.352-1-naomi.ibeh69@gmail.com>
Naomi Ibe <naomi.ibeh69@gmail.com> writes:
> As described in the CodingGuidelines document, a single line
> message given to die() and its friends should not capitalize its
> first word, and should not add full-stop at the end.
>
> Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> ---
> builtin/add.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Looks good. Will queue. Thanks.
^ permalink raw reply
* [PATCH v2 1/1] [OUTREACHY] builtin/add.c: clean up die() messages
From: Naomi Ibe @ 2023-10-17 22:13 UTC (permalink / raw)
To: git; +Cc: Naomi Ibe
As described in the CodingGuidelines document, a single line
message given to die() and its friends should not capitalize its
first word, and should not add full-stop at the end.
Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
builtin/add.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
if (repo_read_index(the_repository) < 0)
- die(_("Could not read the index"));
+ die(_("could not read the index"));
repo_init_revisions(the_repository, &rev, prefix);
rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
die(_("editing patch failed"));
if (stat(file, &st))
- die_errno(_("Could not stat '%s'"), file);
+ die_errno(_("could not stat '%s'"), file);
if (!st.st_size)
- die(_("Empty patch. Aborted."));
+ die(_("empty patch. aborted"));
child.git_cmd = 1;
strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
NULL);
if (run_command(&child))
- die(_("Could not apply '%s'"), file);
+ die(_("could not apply '%s'"), file);
unlink(file);
free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
finish:
if (write_locked_index(&the_index, &lock_file,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("Unable to write new index file"));
+ die(_("unable to write new index file"));
dir_clear(&dir);
clear_pathspec(&pathspec);
--
2.36.1.windows.1
^ permalink raw reply related
* [PATCH v2 1/1] [OUTREACHY] add: standardize die() messages output.
From: Naomi Ibe @ 2023-10-17 22:06 UTC (permalink / raw)
To: git; +Cc: Naomi Ibe
builtin/add.c: clean up die() messages
As described in the CodingGuidelines document, a single line
message given to die() and its friends should not capitalize its
first word, and should not add full-stop at the end.
Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
builtin/add.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
if (repo_read_index(the_repository) < 0)
- die(_("Could not read the index"));
+ die(_("could not read the index"));
repo_init_revisions(the_repository, &rev, prefix);
rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
die(_("editing patch failed"));
if (stat(file, &st))
- die_errno(_("Could not stat '%s'"), file);
+ die_errno(_("could not stat '%s'"), file);
if (!st.st_size)
- die(_("Empty patch. Aborted."));
+ die(_("empty patch. aborted"));
child.git_cmd = 1;
strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
NULL);
if (run_command(&child))
- die(_("Could not apply '%s'"), file);
+ die(_("could not apply '%s'"), file);
unlink(file);
free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
finish:
if (write_locked_index(&the_index, &lock_file,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("Unable to write new index file"));
+ die(_("unable to write new index file"));
dir_clear(&dir);
clear_pathspec(&pathspec);
--
2.36.1.windows.1
^ permalink raw reply related
* Re: Is there any interest in localizing term delimiters in git messages?
From: Junio C Hamano @ 2023-10-17 21:49 UTC (permalink / raw)
To: Alexander Shopov
Cc: Git List, jmas, alexhenrie24, ralf.thielow, matthias.ruester,
phillip.szelat, vyruss, christopher.diaz.riv, jn.avila, flashcode,
bagasdotme, Ævar Arnfjörð Bjarmason,
alessandro.menti, elongbug, cwryu, uneedsihyeon, arek_koz,
dacs.git, insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, Xin Jiang,
pan93412@gmail.com, franklin@goodhorse.idv.tw
In-Reply-To: <CAP6f5Mmi=f4DPcFwfvEiJMdKMa0BUyZ019mc8uFXyOufgD4NjA@mail.gmail.com>
Alexander Shopov <ash@kambanaria.org> writes:
> Typical example:
> ORIGINAL
> msgid " (use \"git rm --cached <file>...\" to unstage)"
>
> TRANSLATION
> msgstr ""
> " (използвайте „git rm --cached %s ФАЙЛ…“, за да извадите ФАЙЛа от индекса)"
>
> The important part are the `<' and `>' delimiters of the term "file"
>
> Instead of using them - I omit them and capitalize the term. As if `<'
> and `>' are declared as localizable and then I translate them as `',
> `'
Is it because it is more common in your target language to omit <>
around the placeholder word, or is it just your personal preference?
Whichever is the case, I am not sure how it affects ...
> So I am asking - is there any interest from other localizers to have
> such a feature? Would the additional maintenance be OK for the
> developers?
... the maintenance burden for developers. Perhaps I am not getting
what you are proposing, but we are not going to change the message
in "C" locale (the original you see in msgid). In untranslated Git,
we will keep the convention to highlight the placeholder word by
having <> around it, so the "(use \"git rm --cached <file>...\" to
unstage)" message will be spelled with "<file>". You can translate
that to a msgstr without <> markings without asking anybody's
permission, and I do not think of a reason why it would burden
developers to do so.
As long as the target audience of your translation wants to see
<file> to be translated to ФАЙЛ without <> around the word, I do
not think there is any problem doing so. I of course am assuming
that using capitalized placeholder is the norm for all users who use
Bulgarian translated Git---if it is not some users want to see <>
around the placeholder word just like "C" locale, then you'd need to
answer your users wish first, or course, but that would not need to
concern the developers who write the "C" locale messages.
Thanks for helping Git easier to use for users with your language.
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Junio C Hamano @ 2023-10-17 21:36 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, Joanna Wang
In-Reply-To: <CAMmZTi-xOWxpzL1SvErgri0_gwvED5Vu2NfeGVcW7jCN8gyEDQ@mail.gmail.com>
Joanna Wang <jojwang@google.com> writes:
>> choose among attr:submodule, attr:type=<what>, attr:bits=<permbits>, decide what keyword to use
> Whatever we choose, do we want to block these keywords from being used
> by folks in their .gitattributes files?
> That would break any existing usage of the keywords. Is this a concern?
> Option A: To keep things working, we could add this support for attr,
> but then always prioritize whatever is set in .gitattributes. So this
> new behavior would only be triggered if the keywords (e.g.
> "submodule", "type", "bits") aren't set in .gitattributes (or w/e list
> of attributes are being read).
Without thinking things through, I think this sounds easier to
explain to the users. I have to wonder how one would implement such
override, though. Suppose we are doing attr:bits=160000, so we ask
git_check_attr() about "bits" attribute. In collect_some_attrs(), we
will have "bits" in the check[] array. We prepare the attr_stack
and fill(), which would go grab whatever is defined in the
attributes system. We'll lstat() or do the equivalent conversion
from the tree_entry.mode to permission bits only if the attributes
system has nothing to say for that "bits" attribute. I think a few
key things that needs to be thought out are:
(1) where in that callchain would we intercept and insert our own
"bits" value based on the filesystem property?
(2) does collect_some_attrs() or git_check_attr() leave enough
information in check[] to tell between [a] the attr stack had
no opinion on "bits" attribute and [b] the attr stack
explicitly said "bits" attribute is unspecified?
^ permalink raw reply
* [PATCH v2] upload-pack: add tracing for fetches
From: Robert Coup via GitGitGadget @ 2023-10-17 21:12 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Jeff King, Robert Coup, Robert Coup
In-Reply-To: <pull.1598.git.1697040242703.gitgitgadget@gmail.com>
From: Robert Coup <robert@coup.net.nz>
Information on how users are accessing hosted repositories can be
helpful to server operators. For example, being able to broadly
differentiate between fetches and initial clones; the use of shallow
repository features; or partial clone filters.
a29263c (fetch-pack: add tracing for negotiation rounds, 2022-08-02)
added some information on have counts to fetch-pack itself to help
diagnose negotiation; but from a git-upload-pack (server) perspective,
there's no means of accessing such information without using
GIT_TRACE_PACKET to examine the protocol packets.
Improve this by emitting a Trace2 JSON event from upload-pack with
summary information on the contents of a fetch request.
* haves, wants, and want-ref counts can help determine (broadly) between
fetches and clones, and the use of single-branch, etc.
* shallow clone depth, tip counts, and deepening options.
* any partial clone filter type.
Signed-off-by: Robert Coup <robert@coup.net.nz>
---
upload-pack: add tracing for fetches
Changes since V1
================
* Don't generate the JSON event unless Trace2 is active.
* Code style fix.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1598%2Frcoup%2Frc-upload-pack-trace-info-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1598/rcoup/rc-upload-pack-trace-info-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1598
Range-diff vs v1:
1: f9157979848 ! 1: 9290d586b08 upload-pack: add tracing for fetches
@@ upload-pack.c: static int parse_have(const char *line, struct oid_array *haves)
+ struct json_writer jw = JSON_WRITER_INIT;
+
+ jw_object_begin(&jw, 0);
-+ {
-+ jw_object_intmax(&jw, "haves", data->haves.nr);
-+ jw_object_intmax(&jw, "wants", data->want_obj.nr);
-+ jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
-+ jw_object_intmax(&jw, "depth", data->depth);
-+ jw_object_intmax(&jw, "shallows", data->shallows.nr);
-+ jw_object_bool(&jw, "deepen-since", data->deepen_since);
-+ jw_object_intmax(&jw, "deepen-not", data->deepen_not.nr);
-+ jw_object_bool(&jw, "deepen-relative", data->deepen_relative);
-+ if (data->filter_options.choice)
-+ jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));
-+ else
-+ jw_object_null(&jw, "filter");
-+ }
++ jw_object_intmax(&jw, "haves", data->haves.nr);
++ jw_object_intmax(&jw, "wants", data->want_obj.nr);
++ jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
++ jw_object_intmax(&jw, "depth", data->depth);
++ jw_object_intmax(&jw, "shallows", data->shallows.nr);
++ jw_object_bool(&jw, "deepen-since", data->deepen_since);
++ jw_object_intmax(&jw, "deepen-not", data->deepen_not.nr);
++ jw_object_bool(&jw, "deepen-relative", data->deepen_relative);
++ if (data->filter_options.choice)
++ jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));
++ else
++ jw_object_null(&jw, "filter");
+ jw_end(&jw);
+
+ trace2_data_json("upload-pack", the_repository, "fetch-info", &jw);
@@ upload-pack.c: static void process_args(struct packet_reader *request,
if (request->status != PACKET_READ_FLUSH)
die(_("expected flush after fetch arguments"));
+
-+ trace2_fetch_info(data);
++ if (trace2_is_enabled())
++ trace2_fetch_info(data);
}
static int process_haves(struct upload_pack_data *data, struct oid_array *common)
t/t5500-fetch-pack.sh | 38 +++++++++++++++++++++++++++-----------
upload-pack.c | 28 ++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 11 deletions(-)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index d18f2823d86..bb15ac34f77 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -132,13 +132,18 @@ test_expect_success 'single branch object count' '
'
test_expect_success 'single given branch clone' '
- git clone --single-branch --branch A "file://$(pwd)/." branch-a &&
- test_must_fail git --git-dir=branch-a/.git rev-parse origin/B
+ GIT_TRACE2_EVENT="$(pwd)/branch-a/trace2_event" \
+ git clone --single-branch --branch A "file://$(pwd)/." branch-a &&
+ test_must_fail git --git-dir=branch-a/.git rev-parse origin/B &&
+ grep \"fetch-info\".*\"haves\":0 branch-a/trace2_event &&
+ grep \"fetch-info\".*\"wants\":1 branch-a/trace2_event
'
test_expect_success 'clone shallow depth 1' '
- git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0 &&
- test "$(git --git-dir=shallow0/.git rev-list --count HEAD)" = 1
+ GIT_TRACE2_EVENT="$(pwd)/shallow0/trace2_event" \
+ git clone --no-single-branch --depth 1 "file://$(pwd)/." shallow0 &&
+ test "$(git --git-dir=shallow0/.git rev-list --count HEAD)" = 1 &&
+ grep \"fetch-info\".*\"depth\":1 shallow0/trace2_event
'
test_expect_success 'clone shallow depth 1 with fsck' '
@@ -235,7 +240,10 @@ test_expect_success 'add two more (part 2)' '
test_expect_success 'deepening pull in shallow repo' '
(
cd shallow &&
- git pull --depth 4 .. B
+ GIT_TRACE2_EVENT="$(pwd)/trace2_event" \
+ git pull --depth 4 .. B &&
+ grep \"fetch-info\".*\"depth\":4 trace2_event &&
+ grep \"fetch-info\".*\"shallows\":2 trace2_event
)
'
@@ -306,9 +314,12 @@ test_expect_success 'fetch --depth --no-shallow' '
test_expect_success 'turn shallow to complete repository' '
(
cd shallow &&
- git fetch --unshallow &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2_event" \
+ git fetch --unshallow &&
! test -f .git/shallow &&
- git fsck --full
+ git fsck --full &&
+ grep \"fetch-info\".*\"shallows\":2 trace2_event &&
+ grep \"fetch-info\".*\"depth\":2147483647 trace2_event
)
'
@@ -826,13 +837,15 @@ test_expect_success 'clone shallow since ...' '
'
test_expect_success 'fetch shallow since ...' '
- git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
+ GIT_TRACE2_EVENT=$(pwd)/shallow11/trace2_event \
+ git -C shallow11 fetch --shallow-since "200000000 +0700" origin &&
git -C shallow11 log --pretty=tformat:%s origin/main >actual &&
cat >expected <<-\EOF &&
three
two
EOF
- test_cmp expected actual
+ test_cmp expected actual &&
+ grep \"fetch-info\".*\"deepen-since\":true shallow11/trace2_event
'
test_expect_success 'clone shallow since selects no commits' '
@@ -987,13 +1000,16 @@ test_expect_success 'filtering by size' '
test_config -C server uploadpack.allowfilter 1 &&
test_create_repo client &&
- git -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&
+ GIT_TRACE2_EVENT=$(pwd)/client/trace2_event \
+ git -C client fetch-pack --filter=blob:limit=0 ../server HEAD &&
# Ensure that object is not inadvertently fetched
commit=$(git -C server rev-parse HEAD) &&
blob=$(git hash-object server/one.t) &&
git -C client rev-list --objects --missing=allow-any "$commit" >oids &&
- ! grep "$blob" oids
+ ! grep "$blob" oids &&
+
+ grep \"fetch-info\".*\"filter\":\"blob:limit\" client/trace2_event
'
test_expect_success 'filtering by size has no effect if support for it is not advertised' '
diff --git a/upload-pack.c b/upload-pack.c
index 83f3d2651ab..ea234ab6a45 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -33,6 +33,7 @@
#include "commit-reach.h"
#include "shallow.h"
#include "write-or-die.h"
+#include "json-writer.h"
/* Remember to update object flag allocation in object.h */
#define THEY_HAVE (1u << 11)
@@ -1552,6 +1553,30 @@ static int parse_have(const char *line, struct oid_array *haves)
return 0;
}
+static void trace2_fetch_info(struct upload_pack_data *data)
+{
+ struct json_writer jw = JSON_WRITER_INIT;
+
+ jw_object_begin(&jw, 0);
+ jw_object_intmax(&jw, "haves", data->haves.nr);
+ jw_object_intmax(&jw, "wants", data->want_obj.nr);
+ jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
+ jw_object_intmax(&jw, "depth", data->depth);
+ jw_object_intmax(&jw, "shallows", data->shallows.nr);
+ jw_object_bool(&jw, "deepen-since", data->deepen_since);
+ jw_object_intmax(&jw, "deepen-not", data->deepen_not.nr);
+ jw_object_bool(&jw, "deepen-relative", data->deepen_relative);
+ if (data->filter_options.choice)
+ jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));
+ else
+ jw_object_null(&jw, "filter");
+ jw_end(&jw);
+
+ trace2_data_json("upload-pack", the_repository, "fetch-info", &jw);
+
+ jw_release(&jw);
+}
+
static void process_args(struct packet_reader *request,
struct upload_pack_data *data)
{
@@ -1640,6 +1665,9 @@ static void process_args(struct packet_reader *request,
if (request->status != PACKET_READ_FLUSH)
die(_("expected flush after fetch arguments"));
+
+ if (trace2_is_enabled())
+ trace2_fetch_info(data);
}
static int process_haves(struct upload_pack_data *data, struct oid_array *common)
base-commit: aab89be2eb6ca51eefeb8c8066f673f447058856
--
gitgitgadget
^ permalink raw reply related
* Is there any interest in localizing term delimiters in git messages?
From: Alexander Shopov @ 2023-10-17 21:09 UTC (permalink / raw)
To: Git List
Cc: jmas, alexhenrie24, ralf.thielow, matthias.ruester,
phillip.szelat, vyruss, christopher.diaz.riv, jn.avila, flashcode,
bagasdotme, Ævar Arnfjörð Bjarmason,
alessandro.menti, elongbug, cwryu, uneedsihyeon, arek_koz,
dacs.git, insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, Xin Jiang,
pan93412@gmail.com, franklin@goodhorse.idv.tw, Junio C Hamano
Hello all,
Is there any interest in being able to change the delimiters of the
changeable terms in git messages?
Typical example:
ORIGINAL
msgid " (use \"git rm --cached <file>...\" to unstage)"
TRANSLATION
msgstr ""
" (използвайте „git rm --cached %s ФАЙЛ…“, за да извадите ФАЙЛа от индекса)"
The important part are the `<' and `>' delimiters of the term "file"
Instead of using them - I omit them and capitalize the term. As if `<'
and `>' are declared as localizable and then I translate them as `',
`'
This has the following benefits:
1. The translation gets shorter
2. We skip potentially dangerous shell characters (<> redirect IN/OUT)
3. Readability improves for some strings, ex:
- git pack-objects [<options>] <base-name> [< <ref-list> | < <object-list>]
- git mailinfo [<options>] <msg> <patch> < mail >info
On the other hand - this can increase the maintenance burden of
messages and tests and the shortening benefit is applicable to
languages using capitalization or some other form of letter changing
that preserves readability (I understand there are many languages with
lots of speakers that are not like that). They might decide to convey
`<' and `>' as `«', `»' to get benefits 2 and 3.
So I am asking - is there any interest from other localizers to have
such a feature? Would the additional maintenance be OK for the
developers?
It is possible that no one besides me is interested in this - in which
case I will rework the Bulgarian translation as:
- More and more messages containing only the term automatically add
the `<' and `>'
- I need to keep adding smudge rules to the git-po-helper tool
(https://github.com/git-l10n/git-po-helper).
Kind regards:
al_shopov
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Joanna Wang @ 2023-10-17 20:59 UTC (permalink / raw)
To: gitster; +Cc: git, Joanna Wang
> choose among attr:submodule, attr:type=<what>, attr:bits=<permbits>, decide what keyword to use
Whatever we choose, do we want to block these keywords from being used
by folks in their .gitattributes files?
That would break any existing usage of the keywords. Is this a concern?
Option A: To keep things working, we could add this support for attr,
but then always prioritize whatever is set in .gitattributes. So this
new behavior would only be triggered if the keywords (e.g.
"submodule", "type", "bits") aren't set in .gitattributes (or w/e list
of attributes are being read).
Option B: Or we could add this new behavior for `attr-bits:<permbits>`
or just `bits:<permbits>`.
I personally don't see big UX differences between A and B. Any opinions on this?
^ permalink raw reply
* Re: [PATCH 0/8] t7900: untangle test dependencies
From: Junio C Hamano @ 2023-10-17 20:49 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, stolee
In-Reply-To: <8cd788dc-7d16-4cfd-9f70-7889dcaa7199@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Tue, Oct 17, 2023, at 21:59, Junio C Hamano wrote:
>> It is kind-of surprising that with only 8 patches you can reach such
>> a state, but ...
>>
>>> # The tests that used to depend on each other should still pass
>>> # when run together
>>> ./t7900-maintenance.sh --quiet --run=setup,30,31 &&
>>
>> ... this puzzles me. What does it mean for tests to "depend on each
>> other"? Does this mean running #31 with or without running #30 runs
>> under different condition and potentially run different things?
>
> What I mean is that some preceding test has a side-effect that a test
> depends on.
I see. And 31 used to depend on the side effect of having ran 30,
but in the updated test, the precondition 31 depends on is created
by itself without relying on what 30 did (and in fact, perhaps in
the updated test, 30 may rewind what it did as part of the clean-up
process using test_when_finished). That makes sense.
> I don't know what the policy is. :) My motivation was that I was working
> on something else which seemed to break the suite, then I tried to reduce
> the tests that were run to get rid of the noise (`--verbose`), but then it
> got confusing because I didn't know if I had really broken some tests
> myself or if more tests would start failing by only running a subset of
> them.
Yeah, it is a laudable goal, but I am not sure how practical it is
to expect developers to maintain that propertly. Unless there is
some automated test to enforce the independence of the tests, that
is.
Thanks.
^ permalink raw reply
* Re: [PATCH] grep: die gracefully when outside repository
From: Junio C Hamano @ 2023-10-17 20:39 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, ks1322
In-Reply-To: <087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 39d6d713ecb..b976f81a166 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
> )
> '
>
> +test_expect_success 'outside of git repository with pathspec outside the directory tree' '
> + test_when_finished rm -fr non &&
> + rm -fr non &&
> + mkdir -p non/git/sub &&
> + (
> + GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non/git &&
> + test_expect_code 128 git grep --no-index search .. 2>error &&
> + grep "is outside the directory tree" error
> + )
> +'
> +
So you create non/git/sub, go to non/git (so there is sub/ directory),
and try running "..".
If you had a directory non/tig next to non/git and used ../tig
instead of .. as the path given to "git grep", it would also
correctly fail. Searching in a non-existing path, ../non, dies in a
different error, with an error message that is not technically
wrong, but it probably can be improved. It has been a while since I
looked at the pathspec matching code, but if we are lucky, it might
be just the matter of swapping the order of checking (in other
words, check "is it outside" first and then "does it exist" next, or
something like that)?
t/t7810-grep.sh | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git c/t/t7810-grep.sh w/t/t7810-grep.sh
index b976f81a16..84838c0fe1 100755
--- c/t/t7810-grep.sh
+++ w/t/t7810-grep.sh
@@ -1234,16 +1234,30 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
)
'
-test_expect_success 'outside of git repository with pathspec outside the directory tree' '
+test_expect_success 'no repository with path outside $cwd' '
test_when_finished rm -fr non &&
rm -fr non &&
- mkdir -p non/git/sub &&
+ mkdir -p non/git/sub non/tig &&
(
GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
export GIT_CEILING_DIRECTORIES &&
cd non/git &&
test_expect_code 128 git grep --no-index search .. 2>error &&
grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../tig 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../non 2>error &&
+ grep "no such path in the working tree" error
)
'
^ permalink raw reply related
* Re: [PATCH] grep: die gracefully when outside repository
From: Junio C Hamano @ 2023-10-17 20:25 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, ks1322, peff
In-Reply-To: <f8a2abc0f610912af3eb56536ed217b8f90db2f9.1697571664.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> On Tue, Oct 17, 2023, at 18:42, Junio C Hamano wrote:
>> It is curious that the original has two sources of hint_path (i.e.,
>> get_git_dir() is used as a fallback for get_git_work_tree()). Are
>> we certain that the check is at the right place? If we do not have
>> a repository, then both would fail by returning NULL, so it should
>> not matter if we add the new check before we check either or both,
>> or even after we checked both before dying.
>>
>> I wonder if
>>
>> const char *hint_path = get_git_work_tree();
>>
>> if (!hint_path)
>> hint_path = get_git_dir();
>> if (hint_path)
>> die(_("%s: '%s' is outside repository at '%s'"),
>> elt, copyfrom, absolute_path(hint_path));
>> else
>> die(_("%s: '%s' is outside the directory tree"),
>> elt, copyfrom);
>>
>> makes the intent of the code clearer.
>
> That doesn't work since `get_git_dir()` triggers `BUG` instead of
> returning `NULL`.
Ah, interesting.
> The `hint_path` declaration has to be at the start because of style
> rules. But we can initialize it after.
Yes, what you have below (but please leave a blank line between the
last line of decl and the first line of statement for readablility)
looks very readable and sensible.
> I can also have a second look at the test since I am using `grep` to
> test the failure output and not the translation string variant.
That is not necessary, as we no longer run under phoney i18n that
required us to use test_i18ngrep. It is OK to assume that the tests
are run under "C" locale.
Thanks.
> -- >8 --
> Subject: [PATCH] fixup! grep: die gracefully when outside repository
>
> ---
> pathspec.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index e115832f17a..0c1061fad11 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -467,10 +467,11 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
> match = prefix_path_gently(prefix, prefixlen,
> &prefixlen, copyfrom);
> if (!match) {
> - const char *hint_path = get_git_work_tree();
> + const char *hint_path;
> if (!have_git_dir())
> die(_("'%s' is outside the directory tree"),
> copyfrom);
> + hint_path = get_git_work_tree();
> if (!hint_path)
> hint_path = get_git_dir();
> die(_("%s: '%s' is outside repository at '%s'"), elt,
^ permalink raw reply
* Re: none
From: Junio C Hamano @ 2023-10-17 20:21 UTC (permalink / raw)
To: Dorcas Litunya; +Cc: christian.couder, git
In-Reply-To: <ZS2ESFGP2H3CTJSK@dorcaslitunya-virtual-machine>
Dorcas Litunya <anonolitunya@gmail.com> writes:
> Bcc:
> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
> Reply-To:
> In-Reply-To: <xmqq1qdumrto.fsf@gitster.g>
What are these lines doing here?
> So should I replace this in the next version or leave this as is?
Perhaps I was not clear enough, but I found the commit title and
description need to be updated to clearly record the intent of the
change with a handful of points, so I will not be accepting the
patch as-is.
These two sections may be of help.
Documentation/MyFirstContribution.txt::now-what
Documentation/MyFirstContribution.txt::reviewing
Thanks.
^ 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