* [PATCH 5/6] t1510 (worktree): fix '&&' chaining
From: Ramkumar Ramachandra @ 2011-12-09 11:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323430158-14885-1-git-send-email-artagnon@gmail.com>
Breaks in a test assertion's && chain can potentially hide failures
from earlier commands in the chain. Fix these breaks.
'unset' returns non-zero status when the variable passed was already
unset on some shells; now that its status is tested, change these
instances to 'sane_unset'.
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
t/t1501-worktree.sh | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 6384983..e661147 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -48,7 +48,7 @@ test_expect_success 'setup: helper for testing rev-parse' '
'
test_expect_success 'setup: core.worktree = relative path' '
- unset GIT_WORK_TREE;
+ sane_unset GIT_WORK_TREE &&
GIT_DIR=repo.git &&
GIT_CONFIG="$(pwd)"/$GIT_DIR/config &&
export GIT_DIR GIT_CONFIG &&
@@ -89,7 +89,7 @@ test_expect_success 'subdir of work tree' '
'
test_expect_success 'setup: core.worktree = absolute path' '
- unset GIT_WORK_TREE;
+ sane_unset GIT_WORK_TREE &&
GIT_DIR=$(pwd)/repo.git &&
GIT_CONFIG=$GIT_DIR/config &&
export GIT_DIR GIT_CONFIG &&
@@ -334,7 +334,7 @@ test_expect_success 'absolute pathspec should fail gracefully' '
'
test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
- >dummy_file
+ >dummy_file &&
echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file &&
git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
'
--
1.7.7.3
^ permalink raw reply related
* [PATCH 4/6] t3200 (branch): fix '&&' chaining
From: Ramkumar Ramachandra @ 2011-12-09 11:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323430158-14885-1-git-send-email-artagnon@gmail.com>
Breaks in a test assertion's && chain can potentially hide failures
from earlier commands in the chain. Fix these breaks.
'git branch --help' will fail when git manpages aren't already
installed; now that its status is tested, guard these instances with
'test_might_fail'.
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
t/t3200-branch.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index bc73c20..95066d0 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -22,7 +22,7 @@ test_expect_success \
test_expect_success \
'git branch --help should not have created a bogus branch' '
- git branch --help </dev/null >/dev/null 2>/dev/null;
+ test_might_fail git branch --help </dev/null >/dev/null 2>/dev/null &&
test_path_is_missing .git/refs/heads/--help
'
@@ -88,7 +88,7 @@ test_expect_success \
test_expect_success \
'git branch -m n/n n should work' \
'git branch -l n/n &&
- git branch -m n/n n
+ git branch -m n/n n &&
test_path_is_file .git/logs/refs/heads/n'
test_expect_success 'git branch -m o/o o should fail when o/p exists' '
--
1.7.7.3
^ permalink raw reply related
* [PATCH 3/6] t1006 (cat-file): use test_cmp
From: Ramkumar Ramachandra @ 2011-12-09 11:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323430158-14885-1-git-send-email-artagnon@gmail.com>
In testing, a common paradigm involves checking the expected output
with the actual output: test-lib provides a test_cmp to show the diff
between the two outputs. So, use this function in preference to
calling a human over to compare command outputs by eye.
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
t/t1006-cat-file.sh | 119 ++++++++++++++++++++++++---------------------------
1 files changed, 56 insertions(+), 63 deletions(-)
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index d8b7f2f..5be3ce9 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -36,66 +36,41 @@ $content"
'
test_expect_success "Type of $type is correct" '
- test $type = "$(git cat-file -t $sha1)"
+ echo $type >expect &&
+ git cat-file -t $sha1 >actual &&
+ test_cmp expect actual
'
test_expect_success "Size of $type is correct" '
- test $size = "$(git cat-file -s $sha1)"
+ echo $size >expect &&
+ git cat-file -s $sha1 >actual &&
+ test_cmp expect actual
'
test -z "$content" ||
test_expect_success "Content of $type is correct" '
- expect="$(maybe_remove_timestamp "$content" $no_ts)"
- actual="$(maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts)"
-
- if test "z$expect" = "z$actual"
- then
- : happy
- else
- echo "Oops: expected $expect"
- echo "but got $actual"
- false
- fi
+ maybe_remove_timestamp "$content" $no_ts >expect &&
+ maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts >actual &&
+ test_cmp expect actual
'
test_expect_success "Pretty content of $type is correct" '
- expect="$(maybe_remove_timestamp "$pretty_content" $no_ts)"
- actual="$(maybe_remove_timestamp "$(git cat-file -p $sha1)" $no_ts)"
- if test "z$expect" = "z$actual"
- then
- : happy
- else
- echo "Oops: expected $expect"
- echo "but got $actual"
- false
- fi
+ maybe_remove_timestamp "$pretty_content" $no_ts >expect &&
+ maybe_remove_timestamp "$(git cat-file -p $sha1)" $no_ts >actual &&
+ test_cmp expect actual
'
test -z "$content" ||
test_expect_success "--batch output of $type is correct" '
- expect="$(maybe_remove_timestamp "$batch_output" $no_ts)"
- actual="$(maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)" $no_ts)"
- if test "z$expect" = "z$actual"
- then
- : happy
- else
- echo "Oops: expected $expect"
- echo "but got $actual"
- false
- fi
+ maybe_remove_timestamp "$batch_output" $no_ts >expect &&
+ maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)" $no_ts >actual &&
+ test_cmp expect actual
'
test_expect_success "--batch-check output of $type is correct" '
- expect="$sha1 $type $size"
- actual="$(echo_without_newline $sha1 | git cat-file --batch-check)"
- if test "z$expect" = "z$actual"
- then
- : happy
- else
- echo "Oops: expected $expect"
- echo "but got $actual"
- false
- fi
+ echo "$sha1 $type $size" >expect &&
+ echo_without_newline $sha1 | git cat-file --batch-check >actual &&
+ test_cmp expect actual
'
}
@@ -144,10 +119,13 @@ tag_size=$(strlen "$tag_content")
run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_pretty_content" 1
-test_expect_success \
- "Reach a blob from a tag pointing to it" \
- "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
+test_expect_success "Reach a blob from a tag pointing to it" '
+ echo_without_newline "$hello_content" >expect &&
+ git cat-file blob "$tag_sha1" >actual &&
+ test_cmp expect actual
+'
+test_done
for batch in batch batch-check
do
for opt in t s e p
@@ -175,30 +153,41 @@ do
done
test_expect_success "--batch-check for a non-existent named object" '
- test "foobar42 missing
-foobar84 missing" = \
- "$( ( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)"
+ cat >expect <<\-EOF &&
+foobar42 missing
+foobar84 missing
+EOF
+ echo foobar42; echo_without_newline foobar84 | \
+ git cat-file --batch-check >actual &&
+ test_cmp expect actual
'
test_expect_success "--batch-check for a non-existent hash" '
- test "0000000000000000000000000000000000000042 missing
-0000000000000000000000000000000000000084 missing" = \
- "$( ( echo 0000000000000000000000000000000000000042;
- echo_without_newline 0000000000000000000000000000000000000084; ) \
- | git cat-file --batch-check)"
+ cat >expect <<\-EOF &&
+0000000000000000000000000000000000000042 missing
+0000000000000000000000000000000000000084 missing
+EOF
+ echo 0000000000000000000000000000000000000042;
+ echo_without_newline 0000000000000000000000000000000000000084 | \
+ git cat-file --batch-check >actual &&
+ test_cmp expect actual
'
test_expect_success "--batch for an existent and a non-existent hash" '
- test "$tag_sha1 tag $tag_size
+ cat >expect <<\-EOF &&
+tag_sha1 tag $tag_size
$tag_content
-0000000000000000000000000000000000000000 missing" = \
- "$( ( echo $tag_sha1;
- echo_without_newline 0000000000000000000000000000000000000000; ) \
- | git cat-file --batch)"
+0000000000000000000000000000000000000000 missing
+EOF
+ echo $tag_sha1; echo_without_newline 0000000000000000000000000000000000000000 | \
+ git cat-file --batch >actual &&
+ test_cmp expect_actual
'
test_expect_success "--batch-check for an emtpy line" '
- test " missing" = "$(echo | git cat-file --batch-check)"
+ echo " missing" >expect &&
+ echo | git cat-file --batch-check >actual &&
+ test_cmp expect actual
'
batch_input="$hello_sha1
@@ -218,7 +207,10 @@ deadbeef missing
missing"
test_expect_success '--batch with multiple sha1s gives correct format' '
- test "$(maybe_remove_timestamp "$batch_output" 1)" = "$(maybe_remove_timestamp "$(echo_without_newline "$batch_input" | git cat-file --batch)" 1)"
+ maybe_remove_timestamp "$batch_output" 1 >expect &&
+ maybe_remove_timestamp "echo_without_newline "$batch_input" | \
+ git cat-file --batch" 1 >actual &&
+ test_cmp expect actual
'
batch_check_input="$hello_sha1
@@ -237,8 +229,9 @@ deadbeef missing
missing"
test_expect_success "--batch-check with multiple sha1s gives correct format" '
- test "$batch_check_output" = \
- "$(echo_without_newline "$batch_check_input" | git cat-file --batch-check)"
+ echo "$batch_check_output" >expect &&
+ echo_without_newline "$batch_check_input" | git cat-file --batch-check >actual &&
+ test_cmp expect actual
'
test_done
--
1.7.7.3
^ permalink raw reply related
* [PATCH 2/6] t3030 (merge-recursive): use test_expect_code
From: Ramkumar Ramachandra @ 2011-12-09 11:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323430158-14885-1-git-send-email-artagnon@gmail.com>
Use test_expect_code in preference to repeatedly checking exit codes
by hand.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
t/t3030-merge-recursive.sh | 72 ++++----------------------------------------
1 files changed, 6 insertions(+), 66 deletions(-)
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 55ef189..a5e3da7 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -285,17 +285,7 @@ test_expect_success 'merge-recursive simple' '
rm -fr [abcd] &&
git checkout -f "$c2" &&
- git merge-recursive "$c0" -- "$c2" "$c1"
- status=$?
- case "$status" in
- 1)
- : happy
- ;;
- *)
- echo >&2 "why status $status!!!"
- false
- ;;
- esac
+ test_expect_code 1 git merge-recursive "$c0" -- "$c2" "$c1"
'
test_expect_success 'merge-recursive result' '
@@ -334,17 +324,7 @@ test_expect_success 'merge-recursive remove conflict' '
rm -fr [abcd] &&
git checkout -f "$c1" &&
- git merge-recursive "$c0" -- "$c1" "$c5"
- status=$?
- case "$status" in
- 1)
- : happy
- ;;
- *)
- echo >&2 "why status $status!!!"
- false
- ;;
- esac
+ test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c5"
'
test_expect_success 'merge-recursive remove conflict' '
@@ -388,17 +368,7 @@ test_expect_success 'merge-recursive d/f conflict' '
git reset --hard &&
git checkout -f "$c1" &&
- git merge-recursive "$c0" -- "$c1" "$c4"
- status=$?
- case "$status" in
- 1)
- : happy
- ;;
- *)
- echo >&2 "why status $status!!!"
- false
- ;;
- esac
+ test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c4"
'
test_expect_success 'merge-recursive d/f conflict result' '
@@ -422,17 +392,7 @@ test_expect_success 'merge-recursive d/f conflict the other way' '
git reset --hard &&
git checkout -f "$c4" &&
- git merge-recursive "$c0" -- "$c4" "$c1"
- status=$?
- case "$status" in
- 1)
- : happy
- ;;
- *)
- echo >&2 "why status $status!!!"
- false
- ;;
- esac
+ test_expect_code 1 git merge-recursive "$c0" -- "$c4" "$c1"
'
test_expect_success 'merge-recursive d/f conflict result the other way' '
@@ -456,17 +416,7 @@ test_expect_success 'merge-recursive d/f conflict' '
git reset --hard &&
git checkout -f "$c1" &&
- git merge-recursive "$c0" -- "$c1" "$c6"
- status=$?
- case "$status" in
- 1)
- : happy
- ;;
- *)
- echo >&2 "why status $status!!!"
- false
- ;;
- esac
+ test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c6"
'
test_expect_success 'merge-recursive d/f conflict result' '
@@ -490,17 +440,7 @@ test_expect_success 'merge-recursive d/f conflict' '
git reset --hard &&
git checkout -f "$c6" &&
- git merge-recursive "$c0" -- "$c6" "$c1"
- status=$?
- case "$status" in
- 1)
- : happy
- ;;
- *)
- echo >&2 "why status $status!!!"
- false
- ;;
- esac
+ test_expect_code 1 git merge-recursive "$c0" -- "$c6" "$c1"
'
test_expect_success 'merge-recursive d/f conflict result' '
--
1.7.7.3
^ permalink raw reply related
* [PATCH 1/6] t3040 (subprojects-basic): fix '&&' chaining, modernize style
From: Ramkumar Ramachandra @ 2011-12-09 11:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323430158-14885-1-git-send-email-artagnon@gmail.com>
Breaks in a test assertion's && chain can potentially hide failures
from earlier commands in the chain. Fix instances of this. While at
it, clean up the style to fit the prevailing style. This means:
- Put the opening quote starting each test on the same line as the
test_expect_* invocation.
- Indent the file with tabs, not spaces.
- Use test_expect_code() in preference to checking the exit status of
various statements by hand.
- Guard commands that prepare test input for individual tests in the
same test_expect_success, so that their scope is clearer and errors
at that stage can be caught.
- Use <<-\EOF in preference to <<EOF to save readers the trouble of
looking for variable interpolations.
- Include "setup" in the titles of test assertions that prepare for
later ones to make it more obvious which tests can be skipped.
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
t/t3040-subprojects-basic.sh | 144 +++++++++++++++++++++---------------------
1 files changed, 72 insertions(+), 72 deletions(-)
diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh
index f6973e9..0a4ff6d 100755
--- a/t/t3040-subprojects-basic.sh
+++ b/t/t3040-subprojects-basic.sh
@@ -3,81 +3,81 @@
test_description='Basic subproject functionality'
. ./test-lib.sh
-test_expect_success 'Super project creation' \
- ': >Makefile &&
- git add Makefile &&
- git commit -m "Superproject created"'
-
-
-cat >expected <<EOF
-:000000 160000 00000... A sub1
-:000000 160000 00000... A sub2
-EOF
-test_expect_success 'create subprojects' \
- 'mkdir sub1 &&
- ( cd sub1 && git init && : >Makefile && git add * &&
- git commit -q -m "subproject 1" ) &&
- mkdir sub2 &&
- ( cd sub2 && git init && : >Makefile && git add * &&
- git commit -q -m "subproject 2" ) &&
- git update-index --add sub1 &&
- git add sub2 &&
- git commit -q -m "subprojects added" &&
- git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
- test_cmp expected current'
-
-git branch save HEAD
-
-test_expect_success 'check if fsck ignores the subprojects' \
- 'git fsck --full'
-
-test_expect_success 'check if commit in a subproject detected' \
- '( cd sub1 &&
- echo "all:" >>Makefile &&
- echo " true" >>Makefile &&
- git commit -q -a -m "make all" ) && {
- git diff-files --exit-code
- test $? = 1
- }'
-
-test_expect_success 'check if a changed subproject HEAD can be committed' \
- 'git commit -q -a -m "sub1 changed" && {
- git diff-tree --exit-code HEAD^ HEAD
- test $? = 1
- }'
-
-test_expect_success 'check if diff-index works for subproject elements' \
- 'git diff-index --exit-code --cached save -- sub1
- test $? = 1'
-
-test_expect_success 'check if diff-tree works for subproject elements' \
- 'git diff-tree --exit-code HEAD^ HEAD -- sub1
- test $? = 1'
-
-test_expect_success 'check if git diff works for subproject elements' \
- 'git diff --exit-code HEAD^ HEAD
- test $? = 1'
-
-test_expect_success 'check if clone works' \
- 'git ls-files -s >expected &&
- git clone -l -s . cloned &&
- ( cd cloned && git ls-files -s ) >current &&
- test_cmp expected current'
-
-test_expect_success 'removing and adding subproject' \
- 'git update-index --force-remove -- sub2 &&
- mv sub2 sub3 &&
- git add sub3 &&
- git commit -q -m "renaming a subproject" && {
- git diff -M --name-status --exit-code HEAD^ HEAD
- test $? = 1
- }'
+test_expect_success 'setup: create superproject' '
+ : >Makefile &&
+ git add Makefile &&
+ git commit -m "Superproject created"
+'
+
+test_expect_success 'setup: create subprojects' '
+ mkdir sub1 &&
+ ( cd sub1 && git init && : >Makefile && git add * &&
+ git commit -q -m "subproject 1" ) &&
+ mkdir sub2 &&
+ ( cd sub2 && git init && : >Makefile && git add * &&
+ git commit -q -m "subproject 2" ) &&
+ git update-index --add sub1 &&
+ git add sub2 &&
+ git commit -q -m "subprojects added" &&
+ git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
+ git branch save HEAD &&
+ cat >expected <<-\EOF &&
+ :000000 160000 00000... A sub1
+ :000000 160000 00000... A sub2
+ EOF
+ test_cmp expected current
+'
+
+test_expect_success 'check if fsck ignores the subprojects' '
+ git fsck --full
+'
+
+test_expect_success 'check if commit in a subproject detected' '
+ ( cd sub1 &&
+ echo "all:" >>Makefile &&
+ echo " true" >>Makefile &&
+ git commit -q -a -m "make all" ) &&
+ test_expect_code 1 git diff-files --exit-code
+'
+
+test_expect_success 'check if a changed subproject HEAD can be committed' '
+ git commit -q -a -m "sub1 changed" &&
+ test_expect_code 1 git diff-tree --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'check if diff-index works for subproject elements' '
+ test_expect_code 1 git diff-index --exit-code --cached save -- sub1
+'
+
+test_expect_success 'check if diff-tree works for subproject elements' '
+ test_expect_code 1 git diff-tree --exit-code HEAD^ HEAD -- sub1
+'
+
+test_expect_success 'check if git diff works for subproject elements' '
+ test_expect_code 1 git diff --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'check if clone works' '
+ git ls-files -s >expected &&
+ git clone -l -s . cloned &&
+ ( cd cloned && git ls-files -s ) >current &&
+ test_cmp expected current
+'
+
+test_expect_success 'removing and adding subproject' '
+ git update-index --force-remove -- sub2 &&
+ mv sub2 sub3 &&
+ git add sub3 &&
+ git commit -q -m "renaming a subproject" &&
+ test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD
+'
# the index must contain the object name the HEAD of the
# subproject sub1 was at the point "save"
-test_expect_success 'checkout in superproject' \
- 'git checkout save &&
- git diff-index --exit-code --raw --cached save -- sub1'
+test_expect_success 'checkout in superproject' '
+ git checkout save &&
+ git diff-index --exit-code --raw --cached save -- sub1
+'
# just interesting what happened...
# git diff --name-status -M save master
--
1.7.7.3
^ permalink raw reply related
* [PATCH v3 0/6] Fix '&&' chaining in tests
From: Ramkumar Ramachandra @ 2011-12-09 11:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-1-git-send-email-artagnon@gmail.com>
This replaces the previous iteration of the series at $gmane/186553.
Thanks to Jonathan for reviewing.
-- Ram
Ramkumar Ramachandra (6):
t3040 (subprojects-basic): fix '&&' chaining, modernize style
t3030 (merge-recursive): use test_expect_code
t1006 (cat-file): use test_cmp
t3200 (branch): fix '&&' chaining
t1510 (worktree): fix '&&' chaining
tests: fix '&&' chaining
t/t1006-cat-file.sh | 119 +++++++++++++--------------
t/t1007-hash-object.sh | 2 +-
t/t1013-loose-object-format.sh | 2 +-
t/t1300-repo-config.sh | 2 +-
t/t1412-reflog-loop.sh | 2 +-
t/t1501-worktree.sh | 6 +-
t/t1510-repo-setup.sh | 4 +-
t/t1511-rev-parse-caret.sh | 2 +-
t/t3030-merge-recursive.sh | 72 ++---------------
t/t3040-subprojects-basic.sh | 144 ++++++++++++++++----------------
t/t3200-branch.sh | 4 +-
t/t3310-notes-merge-manual-resolve.sh | 10 +-
t/t3400-rebase.sh | 4 +-
t/t3418-rebase-continue.sh | 4 +-
t/t3419-rebase-patch-id.sh | 2 +-
15 files changed, 156 insertions(+), 223 deletions(-)
--
1.7.7.3
^ permalink raw reply
* [PATCH] tag deletions not rejected with receive.denyDeletes= true
From: Jerome DE VIVIE @ 2011-12-09 8:51 UTC (permalink / raw)
To: git
In-Reply-To: <18285669.571323420520289.JavaMail.root@promailix.prometil.com>
Hello,
I have try to deny tag deletion over push using denyDeletes parameter:
git config --system receive.denyDeletes true
git daemon --reuseaddr --base-path=.. --export-all --verbose --enable=receive-pack
I can push tag deletions despite what the internet says (http://progit.org/book/ch7-1.html#receivedenydeletes). I don't know if it is a bug. Could you have a look, pls ? Thank you
BR
Jérôme
Signed-off-by: Jérôme de Vivie <j.edevivie@prometil.com>
---
builtin/receive-pack.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..bf91042 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -432,7 +432,7 @@ static const char *update(struct command *cmd)
}
if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
- if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
+ if (deny_deletes && (!prefixcmp(name, "refs/heads/") || !prefixcmp(name, "refs/tags/"))) {
rp_error("denying ref deletion for %s", name);
return "deletion prohibited";
}
--
1.7.6.msysgit.0
^ permalink raw reply related
* Re: [POC PATCH 0/5] Threaded loose object and pack access
From: Thomas Rast @ 2011-12-09 8:45 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman, Jeff King
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
Thomas Rast wrote:
> Well, just to make sure we're all left in a confused mess of partly
> conflicting patches, here's another angle on the same thing:
Bleh, obviously that was intended to be a reply to
http://thread.gmane.org/gmane.comp.version-control.git/185932/focus=186231
and CC'd to Peff.
Sorry for the mess. I'll go sulking with a few cups of coffee.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* [POC PATCH 2/5] grep: push locking into read_sha1_*
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
Move the locking away from grep (the user) and into read_sha1_* and
read_object_* (the subsystem). This will allow future work on the
locking granularity there.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
builtin/grep.c | 35 ++++-------------------------------
sha1_file.c | 12 ++++++++++--
thread-utils.c | 11 +++++++++++
thread-utils.h | 6 ++++++
4 files changed, 31 insertions(+), 33 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 76f2c4f..6c5bdfa 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -82,19 +82,6 @@ static inline void grep_unlock(void)
unlock_if_threaded(&grep_mutex);
}
-/* Used to serialize calls to read_sha1_file. */
-static pthread_mutex_t read_sha1_mutex;
-
-static inline void read_sha1_lock(void)
-{
- lock_if_threaded(&read_sha1_mutex);
-}
-
-static inline void read_sha1_unlock(void)
-{
- unlock_if_threaded(&read_sha1_mutex);
-}
-
/* Signalled when a new work_item is added to todo. */
static pthread_cond_t cond_add;
@@ -248,8 +235,8 @@ static void start_threads(struct grep_opt *opt)
{
int i;
+ init_subsystem_locks();
pthread_mutex_init(&grep_mutex, NULL);
- pthread_mutex_init(&read_sha1_mutex, NULL);
pthread_cond_init(&cond_add, NULL);
pthread_cond_init(&cond_write, NULL);
pthread_cond_init(&cond_result, NULL);
@@ -296,16 +283,14 @@ static int wait_all(void)
}
pthread_mutex_destroy(&grep_mutex);
- pthread_mutex_destroy(&read_sha1_mutex);
pthread_cond_destroy(&cond_add);
pthread_cond_destroy(&cond_write);
pthread_cond_destroy(&cond_result);
+ destroy_subsystem_locks();
return hit;
}
#else /* !NO_PTHREADS */
-#define read_sha1_lock()
-#define read_sha1_unlock()
static int wait_all(void)
{
@@ -363,21 +348,11 @@ static int grep_config(const char *var, const char *value, void *cb)
return 0;
}
-static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
-{
- void *data;
-
- read_sha1_lock();
- data = read_sha1_file(sha1, type, size);
- read_sha1_unlock();
- return data;
-}
-
static void *load_sha1(const unsigned char *sha1, unsigned long *size,
const char *name)
{
enum object_type type;
- void *data = lock_and_read_sha1_file(sha1, &type, size);
+ void *data = read_sha1_file(sha1, &type, size);
if (!data)
error(_("'%s': unable to read %s"), name, sha1_to_hex(sha1));
@@ -578,7 +553,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
void *data;
unsigned long size;
- data = lock_and_read_sha1_file(entry.sha1, &type, &size);
+ data = read_sha1_file(entry.sha1, &type, &size);
if (!data)
die(_("unable to read tree (%s)"),
sha1_to_hex(entry.sha1));
@@ -608,10 +583,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
struct strbuf base;
int hit, len;
- read_sha1_lock();
data = read_object_with_reference(obj->sha1, tree_type,
&size, NULL);
- read_sha1_unlock();
if (!data)
die(_("unable to read tree (%s)"), sha1_to_hex(obj->sha1));
diff --git a/sha1_file.c b/sha1_file.c
index 956422b..c3595b3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -18,6 +18,7 @@
#include "refs.h"
#include "pack-revindex.h"
#include "sha1-lookup.h"
+#include "thread-utils.h"
#ifndef O_NOATIME
#if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -2237,13 +2238,19 @@ void *read_sha1_file_extended(const unsigned char *sha1,
void *data;
char *path;
const struct packed_git *p;
- const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)
+ const unsigned char *repl;
+
+ lock_if_threaded(&read_sha1_mutex);
+
+ repl = (flag & READ_SHA1_FILE_REPLACE)
? lookup_replace_object(sha1) : sha1;
errno = 0;
data = read_object(repl, type, size);
- if (data)
+ if (data) {
+ unlock_if_threaded(&read_sha1_mutex);
return data;
+ }
if (errno && errno != ENOENT)
die_errno("failed to read object %s", sha1_to_hex(sha1));
@@ -2263,6 +2270,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,
die("packed object %s (stored in %s) is corrupt",
sha1_to_hex(repl), p->pack_name);
+ unlock_if_threaded(&read_sha1_mutex);
return NULL;
}
diff --git a/thread-utils.c b/thread-utils.c
index fb75a29..70af3f9 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -62,6 +62,17 @@ int init_recursive_mutex(pthread_mutex_t *m)
return ret;
}
+pthread_mutex_t read_sha1_mutex;
+void init_subsystem_locks(void)
+{
+ pthread_mutex_init(&read_sha1_mutex, NULL);
+}
+
+void destroy_subsystem_locks(void)
+{
+ pthread_mutex_destroy(&read_sha1_mutex);
+}
+
#ifndef NO_PTHREADS
void lock_if_threaded(pthread_mutex_t *m)
{
diff --git a/thread-utils.h b/thread-utils.h
index 9a780a2..3906753 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -17,10 +17,16 @@
extern void lock_if_threaded(pthread_mutex_t*);
extern void unlock_if_threaded(pthread_mutex_t*);
+extern pthread_mutex_t read_sha1_mutex;
+extern void init_subsystem_locks(void);
+extern void destroy_subsystem_locks(void);
+
#else
#define lock_if_threaded(lock)
#define unlock_if_threaded(lock)
+#define init_subsystem_locks()
+#define destroy_subsystem_locks()
#endif
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [POC PATCH 4/5] sha1_file: stuff various pack reading variables into a struct
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
In preparation for making these variables thread-local, put various
delta-cache related bits of pack reading state into a struct. For now
the accessor function is a dummy that always returns a static instance
of this struct.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
sha1_file.c | 99 ++++++++++++++++++++++++++++++++++++++++-------------------
1 files changed, 67 insertions(+), 32 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 18648c3..7c367f9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1655,21 +1655,50 @@ static void *unpack_compressed_entry(struct packed_git *p,
#define MAX_DELTA_CACHE (256)
-static size_t delta_base_cached;
-
-static struct delta_base_cache_lru_list {
+struct delta_base_cache_lru_list {
struct delta_base_cache_lru_list *prev;
struct delta_base_cache_lru_list *next;
-} delta_base_cache_lru = { &delta_base_cache_lru, &delta_base_cache_lru };
+};
+
+static struct delta_base_cache_lru_list main_delta_base_cache_lru = {
+ &main_delta_base_cache_lru, &main_delta_base_cache_lru
+};
-static struct delta_base_cache_entry {
+struct delta_base_cache_entry {
struct delta_base_cache_lru_list lru;
void *data;
struct packed_git *p;
off_t base_offset;
unsigned long size;
enum object_type type;
-} delta_base_cache[MAX_DELTA_CACHE];
+};
+
+static struct delta_base_cache_entry main_delta_base_cache[MAX_DELTA_CACHE];
+
+struct pack_context {
+ size_t delta_base_cached;
+ struct delta_base_cache_entry *delta_base_cache;
+ struct delta_base_cache_lru_list *delta_base_cache_lru;
+ struct packed_git *last_found;
+};
+
+static struct pack_context main_pack_context = {
+ 0, main_delta_base_cache, &main_delta_base_cache_lru, (void*)1
+};
+
+static struct pack_context *pack_context_alloc(void)
+{
+ struct pack_context *ctx = xmalloc(sizeof(struct pack_context));
+ ctx->delta_base_cached = 0;
+ ctx->delta_base_cache_lru = xmalloc(sizeof(struct delta_base_cache_lru_list));
+ ctx->delta_base_cache_lru->prev = ctx->delta_base_cache_lru;
+ ctx->delta_base_cache_lru->next = ctx->delta_base_cache_lru;
+ ctx->delta_base_cache = xcalloc(MAX_DELTA_CACHE, sizeof(struct delta_base_cache_entry));
+ ctx->last_found = (void*)1;
+ return ctx;
+}
+
+#define get_thread_pack_context() (&main_pack_context)
static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)
{
@@ -1683,7 +1712,8 @@ static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)
static int in_delta_base_cache(struct packed_git *p, off_t base_offset)
{
unsigned long hash = pack_entry_hash(p, base_offset);
- struct delta_base_cache_entry *ent = delta_base_cache + hash;
+ struct delta_base_cache_entry *ent
+ = get_thread_pack_context()->delta_base_cache + hash;
return (ent->data && ent->p == p && ent->base_offset == base_offset);
}
@@ -1692,7 +1722,8 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
{
void *ret;
unsigned long hash = pack_entry_hash(p, base_offset);
- struct delta_base_cache_entry *ent = delta_base_cache + hash;
+ struct pack_context *ctx = get_thread_pack_context();
+ struct delta_base_cache_entry *ent = ctx->delta_base_cache + hash;
ret = ent->data;
if (!ret || ent->p != p || ent->base_offset != base_offset)
@@ -1702,7 +1733,7 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
ent->data = NULL;
ent->lru.next->prev = ent->lru.prev;
ent->lru.prev->next = ent->lru.next;
- delta_base_cached -= ent->size;
+ ctx->delta_base_cached -= ent->size;
} else {
ret = xmemdupz(ent->data, ent->size);
}
@@ -1711,48 +1742,52 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
return ret;
}
-static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
+static inline void release_delta_base_cache(struct pack_context *ctx,
+ struct delta_base_cache_entry *ent)
{
if (ent->data) {
free(ent->data);
ent->data = NULL;
ent->lru.next->prev = ent->lru.prev;
ent->lru.prev->next = ent->lru.next;
- delta_base_cached -= ent->size;
+ ctx->delta_base_cached -= ent->size;
}
}
void clear_delta_base_cache(void)
{
unsigned long p;
+ struct pack_context *ctx = get_thread_pack_context();
+ struct delta_base_cache_entry *delta_base_cache = ctx->delta_base_cache;
for (p = 0; p < MAX_DELTA_CACHE; p++)
- release_delta_base_cache(&delta_base_cache[p]);
+ release_delta_base_cache(ctx, &delta_base_cache[p]);
}
static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
void *base, unsigned long base_size, enum object_type type)
{
unsigned long hash = pack_entry_hash(p, base_offset);
- struct delta_base_cache_entry *ent = delta_base_cache + hash;
+ struct pack_context *ctx = get_thread_pack_context();
+ struct delta_base_cache_entry *ent = ctx->delta_base_cache + hash;
struct delta_base_cache_lru_list *lru;
- release_delta_base_cache(ent);
- delta_base_cached += base_size;
+ release_delta_base_cache(ctx, ent);
+ ctx->delta_base_cached += base_size;
- for (lru = delta_base_cache_lru.next;
- delta_base_cached > delta_base_cache_limit
- && lru != &delta_base_cache_lru;
+ for (lru = ctx->delta_base_cache_lru->next;
+ ctx->delta_base_cached > delta_base_cache_limit
+ && lru != ctx->delta_base_cache_lru;
lru = lru->next) {
struct delta_base_cache_entry *f = (void *)lru;
if (f->type == OBJ_BLOB)
- release_delta_base_cache(f);
+ release_delta_base_cache(ctx, f);
}
- for (lru = delta_base_cache_lru.next;
- delta_base_cached > delta_base_cache_limit
- && lru != &delta_base_cache_lru;
+ for (lru = ctx->delta_base_cache_lru->next;
+ ctx->delta_base_cached > delta_base_cache_limit
+ && lru != ctx->delta_base_cache_lru;
lru = lru->next) {
struct delta_base_cache_entry *f = (void *)lru;
- release_delta_base_cache(f);
+ release_delta_base_cache(ctx, f);
}
ent->p = p;
@@ -1760,10 +1795,10 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
ent->type = type;
ent->data = base;
ent->size = base_size;
- ent->lru.next = &delta_base_cache_lru;
- ent->lru.prev = delta_base_cache_lru.prev;
- delta_base_cache_lru.prev->next = &ent->lru;
- delta_base_cache_lru.prev = &ent->lru;
+ ent->lru.next = ctx->delta_base_cache_lru;
+ ent->lru.prev = ctx->delta_base_cache_lru->prev;
+ ctx->delta_base_cache_lru->prev->next = &ent->lru;
+ ctx->delta_base_cache_lru->prev = &ent->lru;
}
static void *read_object(const unsigned char *sha1, enum object_type *type,
@@ -2021,14 +2056,14 @@ int is_pack_valid(struct packed_git *p)
static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
{
- static struct packed_git *last_found = (void *)1;
struct packed_git *p;
off_t offset;
+ struct pack_context *ctx = get_thread_pack_context();
prepare_packed_git();
if (!packed_git)
return 0;
- p = (last_found == (void *)1) ? packed_git : last_found;
+ p = (ctx->last_found == (void *)1) ? packed_git : ctx->last_found;
do {
if (p->num_bad_objects) {
@@ -2055,16 +2090,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
e->offset = offset;
e->p = p;
hashcpy(e->sha1, sha1);
- last_found = p;
+ ctx->last_found = p;
return 1;
}
next:
- if (p == last_found)
+ if (p == ctx->last_found)
p = packed_git;
else
p = p->next;
- if (p == last_found)
+ if (p == ctx->last_found)
p = p->next;
} while (p);
return 0;
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [POC PATCH 5/5] sha1_file: make the pack machinery thread-safe
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
More precisely speaking, this pushes the locking down from
read_object() into bits of the pack machinery that cannot (yet) run in
parallel.
There are several hacks here:
a) prepare_packed_git() must be called before any parallel accesses
happen. It now unconditionally opens and maps all index files.
b) similarly, prepare_replace_object() must be called before any
parallel read_sha1_file() happens
This simplification lets us avoid locking outright to guard the index
accesses; locking is then mainly required for open_packed_git(),
[un]use_pack(), and such.
The ultimate goal would of course be to let at least _some_ pack
accesses happen without any locking whatsoever. But grep already
benefits from it with a nice speed boost on non-worktree greps.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
builtin/grep.c | 9 ++++++
cache.h | 1 +
replace_object.c | 5 ++-
sha1_file.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++------
thread-utils.c | 9 ++++--
thread-utils.h | 3 +-
6 files changed, 93 insertions(+), 15 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 6c5bdfa..212497d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -980,6 +980,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (opt.pre_context || opt.post_context || opt.file_break ||
opt.funcbody)
skip_first_line = 1;
+ /*
+ * This does the non-threadsafe work early. FIXME:
+ * grep shouldn't have to know about this mess.
+ */
+ use_threads = 0;
+ prepare_replace_object();
+ prepare_packed_git();
+ use_threads = 1;
+
start_threads(&opt);
}
#endif
diff --git a/cache.h b/cache.h
index 8c98d05..379dd44 100644
--- a/cache.h
+++ b/cache.h
@@ -764,6 +764,7 @@ static inline const unsigned char *lookup_replace_object(const unsigned char *sh
return sha1;
return do_lookup_replace_object(sha1);
}
+extern void prepare_replace_object(void);
/* Read and unpack a sha1 file into memory, write memory to a sha1 file */
extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/replace_object.c b/replace_object.c
index d0b1548..b303392 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -2,6 +2,7 @@
#include "sha1-lookup.h"
#include "refs.h"
#include "commit.h"
+#include "thread-utils.h"
static struct replace_object {
unsigned char sha1[2][20];
@@ -76,13 +77,15 @@ static int register_replace_ref(const char *refname,
return 0;
}
-static void prepare_replace_object(void)
+void prepare_replace_object(void)
{
static int replace_object_prepared;
if (replace_object_prepared)
return;
+ assert(!use_threads);
+
for_each_replace_ref(register_replace_ref, NULL);
replace_object_prepared = 1;
if (!replace_object_nr)
diff --git a/sha1_file.c b/sha1_file.c
index 7c367f9..b61692e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -429,7 +429,8 @@ void prepare_alt_odb(void)
static int has_loose_object_local(const unsigned char *sha1)
{
- char *name = sha1_file_name(sha1);
+ char name[PATH_MAX];
+ sha1_file_name_buf(name, sha1);
return !access(name, F_OK);
}
@@ -650,9 +651,12 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)
void release_pack_memory(size_t need, int fd)
{
- size_t cur = pack_mapped;
+ size_t cur;
+ lock_if_threaded(&pack_access_mutex);
+ cur = pack_mapped;
while (need >= (cur - pack_mapped) && unuse_one_window(NULL, fd))
; /* nothing */
+ unlock_if_threaded(&pack_access_mutex);
}
void *xmmap(void *start, size_t length,
@@ -689,9 +693,12 @@ void close_pack_windows(struct packed_git *p)
void unuse_pack(struct pack_window **w_cursor)
{
struct pack_window *w = *w_cursor;
+
if (w) {
+ lock_if_threaded(&pack_access_mutex);
w->inuse_cnt--;
*w_cursor = NULL;
+ unlock_if_threaded(&pack_access_mutex);
}
}
@@ -712,10 +719,13 @@ void close_pack_index(struct packed_git *p)
* must subsist at this point. If ever objects from this pack are requested
* again, the new version of the pack will be reinitialized through
* reprepare_packed_git().
+ *
+ * NOT THREAD-SAFE
*/
void free_pack_by_name(const char *pack_name)
{
struct packed_git *p, **pp = &packed_git;
+ assert(!use_threads);
while (*pp) {
p = *pp;
@@ -821,13 +831,33 @@ static int open_packed_git_1(struct packed_git *p)
static int open_packed_git(struct packed_git *p)
{
- if (!open_packed_git_1(p))
+ lock_if_threaded(&pack_access_mutex);
+ /*
+ * is_pack_valid() took the easy route and did not
+ * lock. This is probably okay; if the pack was
+ * *ever* open, it was valid unless another process is
+ * actively trying to corrupt it, in which case:
+ * meh.
+ *
+ * However, a concurrent open_packed_git() may already have
+ * opened it before we get here. So we test again in a locked
+ * section. If it beat us to it, then we have no work left to
+ * do.
+ */
+ if (p->pack_fd != -1) {
+ unlock_if_threaded(&pack_access_mutex);
return 0;
+ }
+ if (!open_packed_git_1(p)) {
+ unlock_if_threaded(&pack_access_mutex);
+ return 0;
+ }
if (p->pack_fd != -1) {
close(p->pack_fd);
pack_open_fds--;
p->pack_fd = -1;
}
+ unlock_if_threaded(&pack_access_mutex);
return -1;
}
@@ -858,6 +888,9 @@ unsigned char *use_pack(struct packed_git *p,
*/
if (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))
die("packfile %s cannot be accessed", p->pack_name);
+
+ lock_if_threaded(&pack_access_mutex);
+
if (offset > (p->pack_size - 20))
die("offset beyond end of packfile (truncated pack?)");
@@ -916,6 +949,9 @@ unsigned char *use_pack(struct packed_git *p,
offset -= win->offset;
if (left)
*left = win->len - xsize_t(offset);
+
+ unlock_if_threaded(&pack_access_mutex);
+
return win->base + offset;
}
@@ -1044,6 +1080,7 @@ static void prepare_packed_git_one(char *objdir, int local)
if (!p)
continue;
install_packed_git(p);
+ open_pack_index(p);
}
closedir(dir);
}
@@ -1102,6 +1139,12 @@ static void rearrange_packed_git(void)
free(ary);
}
+/*
+ * NOT THREAD-SAFE
+ *
+ * However, it's ok if you run this early, before starting threads,
+ * and then use the pack machinery from threads.
+ */
static int prepare_packed_git_run_once = 0;
void prepare_packed_git(void)
{
@@ -1109,6 +1152,7 @@ void prepare_packed_git(void)
if (prepare_packed_git_run_once)
return;
+ assert (!use_threads);
prepare_packed_git_one(get_object_directory(), 1);
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
@@ -1180,8 +1224,10 @@ static int git_open_noatime(const char *name)
static int open_sha1_file(const unsigned char *sha1)
{
int fd;
- char *name = sha1_file_name(sha1);
+ char namebuf[PATH_MAX];
+ char *name = namebuf;
struct alternate_object_database *alt;
+ sha1_file_name_buf(name, sha1);
fd = git_open_noatime(name);
if (fd >= 0)
@@ -1698,7 +1744,22 @@ static struct pack_context *pack_context_alloc(void)
return ctx;
}
+#ifdef NO_PTHREADS
#define get_thread_pack_context() (&main_pack_context)
+#else
+static struct pack_context *get_thread_pack_context(void)
+{
+ struct pack_context *ctx;
+ if (!use_threads)
+ return &main_pack_context;
+ ctx = pthread_getspecific(pack_context_key);
+ if (ctx)
+ return ctx;
+ ctx = pack_context_alloc();
+ pthread_setspecific(pack_context_key, ctx);
+ return ctx;
+}
+#endif
static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)
{
@@ -2219,6 +2280,10 @@ static void *read_packed_sha1(const unsigned char *sha1,
return data;
}
+/*
+ * WARNING: must never be called concurrently with read_sha1_file and
+ * friends! They do lookups in the cached_objects without locking.
+ */
int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
unsigned char *sha1)
{
@@ -2280,19 +2345,15 @@ void *read_sha1_file_extended(const unsigned char *sha1,
unsigned flag)
{
void *data;
- char *path;
const struct packed_git *p;
const unsigned char *repl;
- lock_if_threaded(&read_sha1_mutex);
-
repl = (flag & READ_SHA1_FILE_REPLACE)
? lookup_replace_object(sha1) : sha1;
errno = 0;
data = read_object(repl, type, size);
if (data) {
- unlock_if_threaded(&read_sha1_mutex);
return data;
}
@@ -2305,7 +2366,8 @@ void *read_sha1_file_extended(const unsigned char *sha1,
sha1_to_hex(repl), sha1_to_hex(sha1));
if (has_loose_object(repl)) {
- path = sha1_file_name(sha1);
+ char path[PATH_MAX];
+ sha1_file_name_buf(path, sha1);
die("loose object %s (stored in %s) is corrupt",
sha1_to_hex(repl), path);
}
@@ -2314,7 +2376,6 @@ void *read_sha1_file_extended(const unsigned char *sha1,
die("packed object %s (stored in %s) is corrupt",
sha1_to_hex(repl), p->pack_name);
- unlock_if_threaded(&read_sha1_mutex);
return NULL;
}
diff --git a/thread-utils.c b/thread-utils.c
index 70af3f9..0da2b65 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -62,15 +62,18 @@ int init_recursive_mutex(pthread_mutex_t *m)
return ret;
}
-pthread_mutex_t read_sha1_mutex;
+pthread_mutex_t pack_access_mutex;
+pthread_key_t pack_context_key;
void init_subsystem_locks(void)
{
- pthread_mutex_init(&read_sha1_mutex, NULL);
+ init_recursive_mutex(&pack_access_mutex);
+ pthread_key_create(&pack_context_key, NULL);
}
void destroy_subsystem_locks(void)
{
- pthread_mutex_destroy(&read_sha1_mutex);
+ pthread_mutex_destroy(&pack_access_mutex);
+ pthread_key_delete(pack_context_key);
}
#ifndef NO_PTHREADS
diff --git a/thread-utils.h b/thread-utils.h
index 3906753..7d3cc0a 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -17,7 +17,8 @@
extern void lock_if_threaded(pthread_mutex_t*);
extern void unlock_if_threaded(pthread_mutex_t*);
-extern pthread_mutex_t read_sha1_mutex;
+extern pthread_mutex_t pack_access_mutex;
+extern pthread_key_t pack_context_key;
extern void init_subsystem_locks(void);
extern void destroy_subsystem_locks(void);
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [POC PATCH 3/5] sha1_file_name_buf(): sha1_file_name in caller's buffer
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
sha1_file_name is non-reentrant because of its use of a static buffer.
Split it into just the buffer writing (which can be called even from
threads as long as the buffer is stack'd) and a small wrapper that
uses the static buffer as before.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
sha1_file.c | 29 +++++++++++++++++++----------
1 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index c3595b3..18648c3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -153,18 +153,11 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
}
/*
- * NOTE! This returns a statically allocated buffer, so you have to be
- * careful about using it. Do an "xstrdup()" if you need to save the
- * filename.
- *
- * Also note that this returns the location for creating. Reading
- * SHA1 file can happen from any alternate directory listed in the
- * DB_ENVIRONMENT environment variable if it is not found in
- * the primary object database.
+ * Similar to sha1_file_name but you provide a buffer of size at least
+ * PATH_MAX.
*/
-char *sha1_file_name(const unsigned char *sha1)
+void sha1_file_name_buf(char *buf, const unsigned char *sha1)
{
- static char buf[PATH_MAX];
const char *objdir;
int len;
@@ -179,6 +172,22 @@ char *sha1_file_name(const unsigned char *sha1)
buf[len+3] = '/';
buf[len+42] = '\0';
fill_sha1_path(buf + len + 1, sha1);
+}
+
+/*
+ * NOTE! This returns a statically allocated buffer, so you have to be
+ * careful about using it. Do an "xstrdup()" if you need to save the
+ * filename.
+ *
+ * Also note that this returns the location for creating. Reading
+ * SHA1 file can happen from any alternate directory listed in the
+ * DB_ENVIRONMENT environment variable if it is not found in
+ * the primary object database.
+ */
+char *sha1_file_name(const unsigned char *sha1)
+{
+ static char buf[PATH_MAX];
+ sha1_file_name_buf(buf, sha1);
return buf;
}
--
1.7.8.431.g2abf2
^ permalink raw reply related
* [POC PATCH 0/5] Threaded loose object and pack access
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
Well, just to make sure we're all left in a confused mess of partly
conflicting patches, here's another angle on the same thing:
Jeff King wrote:
> Wow, that's horrible. Leaving aside the parallelism, it's just terrible
> that reading from the cache is 20 times slower than the worktree. I get
> similar results on my quad-core machine.
By poking around in sha1_file.c I got that down to about 10. It's not
great yet, but it seems a start.
The goal would be to improve it to the point where a patch lookup that
already has all relevant packs open and windows mapped can proceed
without locking. I'm not sure that's doable short of duplicating the
whole pack state (including fds and windows) across threads, but I'll
give it some more thought before going that route.
Thomas Rast (5):
Turn grep's use_threads into a global flag
grep: push locking into read_sha1_*
sha1_file_name_buf(): sha1_file_name in caller's buffer
sha1_file: stuff various pack reading variables into a struct
sha1_file: make the pack machinery thread-safe
builtin/grep.c | 60 +++++-----------
cache.h | 1 +
replace_object.c | 5 +-
sha1_file.c | 213 +++++++++++++++++++++++++++++++++++++++++-------------
thread-utils.c | 30 ++++++++
thread-utils.h | 23 ++++++
6 files changed, 240 insertions(+), 92 deletions(-)
--
1.7.8.431.g2abf2
^ permalink raw reply
* [POC PATCH 1/5] Turn grep's use_threads into a global flag
From: Thomas Rast @ 2011-12-09 8:39 UTC (permalink / raw)
To: git; +Cc: René Scharfe, Junio C Hamano, Eric Herman
In-Reply-To: <cover.1323419666.git.trast@student.ethz.ch>
In preparation for further work on this, turn use_threads into a flag
shared across the whole code base. The supporting (un)lock_if_threaded()
functions are to be used for locking; they return immediately when not
threading.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
builtin/grep.c | 20 ++++++++------------
thread-utils.c | 16 ++++++++++++++++
thread-utils.h | 16 ++++++++++++++++
3 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 988ea1d..76f2c4f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -24,8 +24,6 @@
NULL
};
-static int use_threads = 1;
-
#ifndef NO_PTHREADS
#define THREADS 8
static pthread_t threads[THREADS];
@@ -76,14 +74,12 @@ struct work_item {
static inline void grep_lock(void)
{
- if (use_threads)
- pthread_mutex_lock(&grep_mutex);
+ lock_if_threaded(&grep_mutex);
}
static inline void grep_unlock(void)
{
- if (use_threads)
- pthread_mutex_unlock(&grep_mutex);
+ unlock_if_threaded(&grep_mutex);
}
/* Used to serialize calls to read_sha1_file. */
@@ -91,14 +87,12 @@ static inline void grep_unlock(void)
static inline void read_sha1_lock(void)
{
- if (use_threads)
- pthread_mutex_lock(&read_sha1_mutex);
+ lock_if_threaded(&read_sha1_mutex);
}
static inline void read_sha1_unlock(void)
{
- if (use_threads)
- pthread_mutex_unlock(&read_sha1_mutex);
+ unlock_if_threaded(&read_sha1_mutex);
}
/* Signalled when a new work_item is added to todo. */
@@ -984,6 +978,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
argc--;
}
+#ifndef NO_PTHREADS
+ use_threads = 1;
+#endif
+
if (show_in_pager == default_pager)
show_in_pager = git_pager(1);
if (show_in_pager) {
@@ -1011,8 +1009,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
skip_first_line = 1;
start_threads(&opt);
}
-#else
- use_threads = 0;
#endif
compile_grep_patterns(&opt);
diff --git a/thread-utils.c b/thread-utils.c
index 7f4b76a..fb75a29 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -1,6 +1,8 @@
#include "cache.h"
#include "thread-utils.h"
+int use_threads;
+
#if defined(hpux) || defined(__hpux) || defined(_hpux)
# include <sys/pstat.h>
#endif
@@ -59,3 +61,17 @@ int init_recursive_mutex(pthread_mutex_t *m)
}
return ret;
}
+
+#ifndef NO_PTHREADS
+void lock_if_threaded(pthread_mutex_t *m)
+{
+ if (use_threads)
+ pthread_mutex_lock(m);
+}
+
+void unlock_if_threaded(pthread_mutex_t *m)
+{
+ if (use_threads)
+ pthread_mutex_unlock(m);
+}
+#endif
diff --git a/thread-utils.h b/thread-utils.h
index 6fb98c3..9a780a2 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -1,11 +1,27 @@
#ifndef THREAD_COMPAT_H
#define THREAD_COMPAT_H
+/*
+ * This variable is used by commands to globally tell affected
+ * subsystems that they must use thread-safe mechanisms.
+ */
+extern int use_threads;
+
#ifndef NO_PTHREADS
#include <pthread.h>
extern int online_cpus(void);
extern int init_recursive_mutex(pthread_mutex_t*);
+/* These functions do nothing if use_threads==0 or NO_PTHREADS */
+extern void lock_if_threaded(pthread_mutex_t*);
+extern void unlock_if_threaded(pthread_mutex_t*);
+
+#else
+
+#define lock_if_threaded(lock)
+#define unlock_if_threaded(lock)
+
#endif
+
#endif /* THREAD_COMPAT_H */
--
1.7.8.431.g2abf2
^ permalink raw reply related
* Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style
From: Ramkumar Ramachandra @ 2011-12-09 7:56 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <20111208163448.GA2394@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Put the opening quote starting each test on the same line as the
>> test_expect_* invocation. While at it:
>
> I suspect the above description, while it does describe your patch,
> does not describe the _reason_ that the patch exists or that someone
> would want to apply it. Isn't it something more like:
> [...]
Right, fixed.
> I didn't read over the patch again. Has it changed since v1?
No. I refrained from making other style changes and/ or combining tests.
-- Ram
^ permalink raw reply
* Re: Question about commit message wrapping
From: Frans Klaver @ 2011-12-09 7:51 UTC (permalink / raw)
To: git, Sidney San Martín
In-Reply-To: <op.v57na7120aolir@keputer>
On Fri, Dec 9, 2011 at 8:05 AM, Frans Klaver <fransklaver@gmail.com> wrote:
>> *Nothing else* in my everyday life works this way anymore. Line wrapping
>> gets done on the display end in my email client, my web browser, my ebook
>> reader entirely automatically, and it adapts to the size of the window.
It appears none of the books I have available here have more than 80
characters on one line. It's a typography thing. And typographers for
some reason rarely get it wrong. Why not make use of that knowledge?
^ permalink raw reply
* [PATCH] am: don't persist keepcr flag
From: Martin von Zweigbergk @ 2011-12-09 7:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
The keepcr flag is only used in the split_patches function, which is
only called before a patch application has to stopped for user input,
not after resuming. It is therefore unnecessary to persist the
flag. This seems to have been the case since it was introduced in
ad2c928 (git-am: Add command line parameter `--keep-cr` passing it to
git-mailsplit, 2010-02-27).
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-am.sh | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index 9042432..1c13b13 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -530,7 +530,6 @@ else
echo "$sign" >"$dotest/sign"
echo "$utf8" >"$dotest/utf8"
echo "$keep" >"$dotest/keep"
- echo "$keepcr" >"$dotest/keepcr"
echo "$scissors" >"$dotest/scissors"
echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
echo "$GIT_QUIET" >"$dotest/quiet"
@@ -576,12 +575,6 @@ if test "$(cat "$dotest/keep")" = t
then
keep=-k
fi
-case "$(cat "$dotest/keepcr")" in
-t)
- keepcr=--keep-cr ;;
-f)
- keepcr=--no-keep-cr ;;
-esac
case "$(cat "$dotest/scissors")" in
t)
scissors=--scissors ;;
--
1.7.8.237.gcc4e3
^ permalink raw reply related
* Re: Question about commit message wrapping
From: Frans Klaver @ 2011-12-09 7:05 UTC (permalink / raw)
To: git, Sidney San Martín
In-Reply-To: <35A5A513-91FD-4EF9-B890-AB3D1550D63F@sidneysm.com>
On Fri, 09 Dec 2011 02:59:06 +0100, Sidney San Martín <s@sidneysm.com>
wrote:
> Hey, I want to ask about the practice of wrapping commit messages to
> 70-something charaters.
>
> The webpage most cited about it, which I otherwise really like, is
>
> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>
> *Nothing else* in my everyday life works this way anymore. Line wrapping
> gets done on the display end in my email client, my web browser, my
> ebook reader entirely automatically, and it adapts to the size of the
> window.
Actually, opera-mail autowraps at 72 characters but sets the text format
to flowed. It also wraps the quoted text when you reply. But there's a
reasonable chance that you don't use opera in your daily life. On the
other hand I would not be surprised if most decent e-mail clients worked
that way.
Nobody's forcing you to use the same practice in your own projects anyway.
>
> That article gives two reasons why commits should be wrapped to 72
> columns. First:
>
>> git log doesn’t do any special special wrapping of the commit messages.
>> With the default pager of less -S, this means your paragraphs flow far
>> off the edge of the screen, making them difficult to read. On an 80
>> column terminal, if we subtract 4 columns for the indent on the left
>> and 4 more for symmetry on the right, we’re left with 72 columns.
>
> Here, I put a patch at the bottom of this email that wraps commit
> messages to, right now, 80 columns when they're displayed. (It’s a quick
> one, probably needs configurability. Also, beware, I don’t program in C
> too much.)
Hm. Saying "that's how the tool works" is not a good reason in my opinion.
There might be tons of other reasons for wrapping at 80 characters.
Readability is one that comes to mind for me.
>
> Second:
>
>> git format-patch --stdout converts a series of commits to a series of
>> emails, using the messages for the message body. Good email netiquette
>> dictates we wrap our plain text emails such that there’s room for a few
>> levels of nested reply indicators without overflow in an 80 column
>> terminal. (The current rails.git workflow doesn’t include email, but
>> who knows what the future will bring.)
>
> There's been a standard for flowed plain text emails (which don't have
> to wrap at 80 columns) for well over ten years, RFC-2646 and is widely
> supported. Besides, code in diffs is often longer than 7x characters,
> and wrapping, like `git log`, could be done inside git. FWIW, there are
> a bunch of merge commits with lines longer than 80 characters in the git
> repo itself.
Yes, that standard allows e-mail clients to display the text more fluidly,
even if the source text is word-wrapped. While git uses e-mail format, it
isn't an e-mail client. I always interpreted this whole thing as git
basically creating plain-text e-mails. You're actually writing the source
of the e-mail in your commit message. If you care about actual use in
e-mail (like we do here on the list) you might want to add the relevant
header to the mails. That said, Apple Mail (the client you used to send
your mail) doesn't even use the RFC you quote in the sent message. That
mail is going to be a pain in the butt to read in mutt from work ;).
>
> Finally, people read commits these days in many other places than `git
> log` (and make commits in many other places than a text editor
> configured to wrap). Most every GUI and already word wraps commit
> messages just fine. As a result, there are commits in popular repos much
> longer than the 72-column standard and no one notices. Instead,
> properly-formatted commit messages end up looking cramped when you see
> them in anywhere wider than 80 columns.
Cramped? I think it's compact and actually I prefer it over long lines.
> Am I crazy?
Probably not. Don't take my word for it. I'm not a psychiatrist.
> If this makes sense to anyone else, I'd be happy to help massage this
> into something git-worthy, with some help (never worked on Git before).
>
> - - -
>
> From a93b390d1506652d4ad41d1cbd987ba98a8deca0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
> Date: Thu, 8 Dec 2011 20:26:23 -0500
> Subject: [PATCH] Wrap commit messages on display
>
> - Wrap to 80 characters minus the indent
> - Use a hanging indent for lines which begin with "- "
> - Do not wrap lines which begin with whitespace
> ---
> pretty.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 230fe1c..15804ce 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1243,8 +1243,14 @@ void pp_remainder(const struct
> pretty_print_context *pp,
> memset(sb->buf + sb->len, ' ', indent);
> strbuf_setlen(sb, sb->len + indent);
> }
> - strbuf_add(sb, line, linelen);
> - strbuf_addch(sb, '\n');
> + if (line[0] == ' ' || line[0] == '\t') {
> + strbuf_add(sb, line, linelen);
> + } else {
> + struct strbuf wrapped = STRBUF_INIT;
> + strbuf_add(&wrapped, line, linelen);
> + strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + (line[0] == '-'
> && line[1] == ' ' ? 2 : 0), 80 - indent);
While on the subject, In my mail view, the new line started with the [1]
from line[1], in the quote the line looks entirely different. Now this is
code we're talking about, so it makes slightly more sense to have a proper
wrapping hard-coded. Compare the above with the following:
+ int hanging_indent = ((line[0] == '-' && line[1] == ' ') ? 2 : 0);
[...]
+ strbuf_add_wrapped_text(sb, wrapped.buf, 0,
+ indent + hanging_indent,
+ 80 - indent);
Much clearer, no? I personally usually have two or three terminals tucked
next to each other, so I can look at two or three things at the same time.
80 characters limit is a nice feature then.
> + strbuf_addch(sb, '\n');
> + }
> }
> }
>
Cheers,
Frans
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-09 6:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <CALkWK0nkPB5WptJ9nSkSOif5_0R_f39Dg_HR3Rmg02hG_4Q1Tg@mail.gmail.com>
Hi,
Ramkumar Ramachandra wrote:
> 1. Implement the strscpn() using two strchrnul() calls.
> 2. Drop this patch and use strbuf to replace the fixed-size buffer.
>
> I think I'll go with the second option. What do you think?
> [...]
Er, I have an 'eol' which I'm not using. Making sure that 'eol' is
valid is good enough to avoid unexpected failures: if strchrnul() is
able to find a '\n', strcspn() should have no trouble finding either a
' ' or '\n' (whichever comes first).
Sorry for the nonsense.
-- Ram
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-09 5:57 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207070223.GC11737@elie.hsd1.il.comcast.net>
Hi again,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> [...]
>> While at it, also fix a bug: currently, we use a commit-id-shaped
>> buffer to store the word after "pick" in '.git/sequencer/todo'. This
>> is both wasteful and wrong because it places an artificial limit on
>> the line length. Eliminate the need for the buffer altogether, and
>> add a test demonstrating this.
>
> Reading the above does not make it at all obvious that I should want
> to apply this patch because otherwise my prankster friend can cause my
> copy of git to crash or run arbitrary code by putting a long commit
Working backwards:
get_sha1() is what will finally misbehave: how? It uses strlen() and
let's assume that the number returned by it is too big to fit in a
size_t. Surely, this means that we should only use get_sha1() on
something whose length is bounded. So, do we ever try to get to the
end of the line? Yes! Let's assume that the problem starts when
end_of_object_name calls strcspn which returns something too big to
fit in a size_t. From the manpage it has no standard way of reporting
failure. I'm not sure what to do; I think I have two choices:
1. Implement the strscpn() using two strchrnul() calls.
2. Drop this patch and use strbuf to replace the fixed-size buffer.
I think I'll go with the second option. What do you think?
-- Ram
^ permalink raw reply
* Re: [PATCH 4/5] revert: allow mixed pick and revert instructions
From: Ramkumar Ramachandra @ 2011-12-09 5:34 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207074501.GE11737@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> This patch lays the foundation for extending the parser to support
>> more actions so 'git rebase -i' can reuse this machinery in the
>> future. While at it, also improve the error messages reported by the
>> insn sheet parser.
>
> I don't know what this part is about...
I'll separate out these changes in the re-roll. Thanks.
>> @@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
> [...]
>> status = get_sha1(bol, commit_sha1);
>> *end_of_object_name = saved;
> [...]
>> if (status < 0)
>> - return NULL;
>> + return error(_("Line %d: Malformed object name: %s"), lineno, bol);
>
> (Not about this patch, but an earlier one) What is this idiom about?
> Why not
>
> if (get_sha1(bol, commit_sha1))
> return error(_(...));
>
> ?
I'm first detecting the end of the object name and terminating it with
a '\0', before using using get_sha1() to read out the object name.
Then, I restore the instruction sheet to the original state by
restoring the character from 'saved'. If I use the idiom you
suggested, I'll leave midway after editing the instruction sheet, and
this is undesirable.
-- Ram
^ permalink raw reply
* Re: [PATCH v2 0/6] Fix '&&' chaining in tests
From: Ramkumar Ramachandra @ 2011-12-09 5:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Matthieu Moy, Git List
In-Reply-To: <7vzkf2hm94.fsf@alter.siamese.dyndns.org>
Hi,
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> This follows-up $gmane/186481.
>
> I take that you meant "replaces". It was confusing especially because you
> seem to have included a few unrelated patches in the thread.
Yeah. Sorry- stray files were lying around from the previous 'git
format-patch' invocation. Which brings me to: I wonder if it makes
sense to (optionally) check that the directory is empty when executing
'git format-patch -o <dir>'. I sometimes forget to do it by hand.
-- Ram
^ permalink raw reply
* Re: Query on git commit amend
From: Viresh Kumar @ 2011-12-09 4:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Björn Steinbrink, Vijay Lakshminarayanan,
git@vger.kernel.org, Shiraz HASHIM, Vipin KUMAR
In-Reply-To: <7vvcprar3v.fsf@alter.siamese.dyndns.org>
On 12/8/2011 11:22 PM, Junio C Hamano wrote:
> So by saying "--amend -C HEAD" you are saying "I want to reuse the log
> message of the commit I am amending,... eh, scratch that, I instead want
> to use the log message of the HEAD commit", as if the commit you are
> amending and HEAD are two different things. That is idiotic.
>
> Of course, if "git commit --amend" honoured "--no-edit", that is even more
> direct, straightforward and intuitive way to say so ;-)
Got your point. That's correct.
--
viresh
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-09 2:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmxb2hhne.fsf@alter.siamese.dyndns.org>
On Thu, Dec 08, 2011 at 01:34:29PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Because the pattern takes 0 or more lines and no terminator, we can't
> > distinguish between empty or truncated input and the empty pattern.
>
> I agree that such a positive "Ok here is the end of specification" marker
> is a good idea, even if we do not worry about "an empty set".
>
> When the requestor wants to specify the credentials with host and user,
> but the wire is cut after host is communicated but before user is, we do
> want to notice the communication error, instead of silently erasing all
> the credentials on the host regardless of the user.
OK, I've tweaked the series to require an end-of-credential marker (a
blank line) both in input and output.
In addition, I've changed the code that runs helpers to make reading
from the helpers an all-or-nothing thing (instead of incrementally
ovewriting our credential as we read from it). Before, if a helper
exited with error, we would happily use its partial result. Instead, we
now read its response into a holding area, and only copy it into our
credential when we get a successful exit code. This lets us detect
truncation when reading from the helper, too.
It works, and it detects truncated output both ways properly (I know
because I had to update every test, since the old output was missing the
end-of-credential marker).
It makes me a little sad, because the original format (relying on EOF)
was so Unix-y. You could make a helper like this:
echo password=`gpg -qd ~/.secret.gpg`
and now you must remember to tack an extra "echo" at the end. Not a big
deal, but it somehow just feels less elegant to my gut. OTOH, classic
Unix constructs have always been a nightmare for robustness and error
checking[1], so this is certainly nothing new.
The diff from this tip to the old tip is below to give you a sense of
the magnitude of the change (the individual changes are squashed into
their respective patches for the next re-roll, of course). I'll hold off
on posting the whole series to see if we get any more comments.
-Peff
[1] I mean things like:
grep foo bar | sed 's/some/transformation/'
where we totally ignore errors from grep, and where a truncated
output on the pipe would just subtly generate wrong answers.
---
Documentation/technical/api-credentials.txt | 2 +-
credential-cache--daemon.c | 1 +
credential-cache.c | 2 +
credential-store.c | 1 +
credential.c | 39 +++++++++++++++++++++++---
t/lib-credential.sh | 1 +
t/t0300-credentials.sh | 3 ++
t/t5550-http-fetch.sh | 1 +
8 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index 21ca6a2..0aac899 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -199,7 +199,7 @@ followed by a newline. The key may contain any bytes except `=`,
newline, or NUL. The value may contain any bytes except newline or NUL.
In both cases, all bytes are treated as-is (i.e., there is no quoting,
and one cannot transmit a value with newline or NUL in it). The list of
-attributes is terminated by a blank line or end-of-file.
+attributes is terminated by a blank line.
Git will send the following attributes (but may not send all of
them for a given credential; for example, a `host` attribute makes no
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..38403645 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -138,6 +138,7 @@ static void serve_one_client(FILE *in, FILE *out)
if (e) {
fprintf(out, "username=%s\n", e->item.username);
fprintf(out, "password=%s\n", e->item.password);
+ fprintf(out, "\n");
}
}
else if (!strcmp(action.buf, "exit"))
diff --git a/credential-cache.c b/credential-cache.c
index dc98372..5b8d8c9 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -70,6 +70,8 @@ static void do_cache(const char *socket, const char *action, int timeout,
if (strbuf_read(&buf, 0, 0) < 0)
die_errno("unable to relay credential");
}
+ else
+ strbuf_addch(&buf, '\n');
if (!send_request(socket, &buf))
return;
diff --git a/credential-store.c b/credential-store.c
index ed58768..00e38f0 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -43,6 +43,7 @@ static void print_entry(struct credential *c)
{
printf("username=%s\n", c->username);
printf("password=%s\n", c->password);
+ printf("\n");
}
static void print_line(struct strbuf *buf)
diff --git a/credential.c b/credential.c
index a17eafe..6d2a37d 100644
--- a/credential.c
+++ b/credential.c
@@ -147,8 +147,10 @@ int credential_read(struct credential *c, FILE *fp)
char *key = line.buf;
char *value = strchr(key, '=');
- if (!line.len)
- break;
+ if (!line.len) {
+ strbuf_release(&line);
+ return 0;
+ }
if (!value) {
warning("invalid credential line: %s", key);
@@ -181,7 +183,7 @@ int credential_read(struct credential *c, FILE *fp)
}
strbuf_release(&line);
- return 0;
+ return -1;
}
static void credential_write_item(FILE *fp, const char *key, const char *value)
@@ -198,6 +200,26 @@ static void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path);
credential_write_item(fp, "username", c->username);
credential_write_item(fp, "password", c->password);
+ putc('\n', fp);
+}
+
+static void credential_merge_one(char **dst, char **src)
+{
+ if (!*src)
+ return;
+ free(*dst);
+ *dst = *src;
+ *src = NULL;
+}
+
+static void credential_merge(struct credential *dst,
+ struct credential *src)
+{
+ credential_merge_one(&dst->protocol, &src->protocol);
+ credential_merge_one(&dst->host, &src->host);
+ credential_merge_one(&dst->path, &src->path);
+ credential_merge_one(&dst->username, &src->username);
+ credential_merge_one(&dst->password, &src->password);
}
static int run_credential_helper(struct credential *c,
@@ -206,6 +228,7 @@ static int run_credential_helper(struct credential *c,
{
struct child_process helper;
const char *argv[] = { NULL, NULL };
+ struct credential response = CREDENTIAL_INIT;
FILE *fp;
memset(&helper, 0, sizeof(helper));
@@ -227,17 +250,23 @@ static int run_credential_helper(struct credential *c,
if (want_output) {
int r;
+
fp = xfdopen(helper.out, "r");
- r = credential_read(c, fp);
+ r = credential_read(&response, fp);
fclose(fp);
if (r < 0) {
+ credential_clear(&response);
finish_command(&helper);
return -1;
}
}
- if (finish_command(&helper))
+ if (finish_command(&helper)) {
+ credential_clear(&response);
return -1;
+ }
+
+ credential_merge(c, &response);
return 0;
}
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index fc34447..c0de4e9 100755
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -5,6 +5,7 @@
# separated by "--".
check() {
read_chunk >stdin &&
+ echo >>stdin &&
read_chunk >expect-stdout &&
read_chunk >expect-stderr &&
test-credential "$@" <stdin >stdout 2>stderr &&
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 885af8f..f0e77dc 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -9,6 +9,7 @@ test_expect_success 'setup helper scripts' '
whoami=`echo $0 | sed s/.*git-credential-//`
echo >&2 "$whoami: $*"
while IFS== read key value; do
+ test -z "$key" && break
echo >&2 "$whoami: $key=$value"
eval "$key=$value"
done
@@ -28,6 +29,7 @@ test_expect_success 'setup helper scripts' '
. ./dump
test -z "$user" || echo username=$user
test -z "$pass" || echo password=$pass
+ echo
EOF
chmod +x git-credential-verbatim &&
@@ -196,6 +198,7 @@ HELPER="!f() {
cat >/dev/null
echo username=foo
echo password=bar
+ echo
}; f"
test_expect_success 'respect configured credentials' '
test_config credential.helper "$HELPER" &&
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 95a133d..b817c69 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -106,6 +106,7 @@ test_expect_success 'http auth respects credential helper config' '
cat >/dev/null
echo username=user@host
echo password=user@host
+ echo
}; f" &&
>askpass-query &&
echo wrong >askpass-response &&
^ permalink raw reply related
* Question about commit message wrapping
From: Sidney San Martín @ 2011-12-09 1:59 UTC (permalink / raw)
To: git
Hey, I want to ask about the practice of wrapping commit messages to 70-something charaters.
The webpage most cited about it, which I otherwise really like, is
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
*Nothing else* in my everyday life works this way anymore. Line wrapping gets done on the display end in my email client, my web browser, my ebook reader entirely automatically, and it adapts to the size of the window.
That article gives two reasons why commits should be wrapped to 72 columns. First:
> git log doesn’t do any special special wrapping of the commit messages. With the default pager of less -S, this means your paragraphs flow far off the edge of the screen, making them difficult to read. On an 80 column terminal, if we subtract 4 columns for the indent on the left and 4 more for symmetry on the right, we’re left with 72 columns.
Here, I put a patch at the bottom of this email that wraps commit messages to, right now, 80 columns when they're displayed. (It’s a quick one, probably needs configurability. Also, beware, I don’t program in C too much.)
Second:
> git format-patch --stdout converts a series of commits to a series of emails, using the messages for the message body. Good email netiquette dictates we wrap our plain text emails such that there’s room for a few levels of nested reply indicators without overflow in an 80 column terminal. (The current rails.git workflow doesn’t include email, but who knows what the future will bring.)
There's been a standard for flowed plain text emails (which don't have to wrap at 80 columns) for well over ten years, RFC-2646 and is widely supported. Besides, code in diffs is often longer than 7x characters, and wrapping, like `git log`, could be done inside git. FWIW, there are a bunch of merge commits with lines longer than 80 characters in the git repo itself.
Finally, people read commits these days in many other places than `git log` (and make commits in many other places than a text editor configured to wrap). Most every GUI and already word wraps commit messages just fine. As a result, there are commits in popular repos much longer than the 72-column standard and no one notices. Instead, properly-formatted commit messages end up looking cramped when you see them in anywhere wider than 80 columns.
Am I crazy? If this makes sense to anyone else, I'd be happy to help massage this into something git-worthy, with some help (never worked on Git before).
- - -
From a93b390d1506652d4ad41d1cbd987ba98a8deca0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
Date: Thu, 8 Dec 2011 20:26:23 -0500
Subject: [PATCH] Wrap commit messages on display
- Wrap to 80 characters minus the indent
- Use a hanging indent for lines which begin with "- "
- Do not wrap lines which begin with whitespace
---
pretty.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/pretty.c b/pretty.c
index 230fe1c..15804ce 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1243,8 +1243,14 @@ void pp_remainder(const struct pretty_print_context *pp,
memset(sb->buf + sb->len, ' ', indent);
strbuf_setlen(sb, sb->len + indent);
}
- strbuf_add(sb, line, linelen);
- strbuf_addch(sb, '\n');
+ if (line[0] == ' ' || line[0] == '\t') {
+ strbuf_add(sb, line, linelen);
+ } else {
+ struct strbuf wrapped = STRBUF_INIT;
+ strbuf_add(&wrapped, line, linelen);
+ strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + (line[0] == '-' && line[1] == ' ' ? 2 : 0), 80 - indent);
+ strbuf_addch(sb, '\n');
+ }
}
}
--
1.7.8
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox