* [PATCHv6 1/2] submodule tests: don't use itself as a submodule
From: Stefan Beller @ 2017-01-05 19:29 UTC (permalink / raw)
To: bmwill, peff, gitster; +Cc: git, Stefan Beller
In-Reply-To: <20170105192904.1107-1-sbeller@google.com>
In reality nobody would run "git submodule add ./. <submodule-path>"
to add the repository to itself as a submodule as this comes with some
nasty surprises, such as infinite recursion when cloning that repository.
However we do this all the time in the test suite, because most of the
time this was the most convenient way to test a very specific thing
for submodule behavior.
This provides an easier way to have submodules in tests, by just setting
TEST_CREATE_SUBMODULE to a non empty string, similar to
TEST_NO_CREATE_REPO.
Make use of it in those tests that add a submodule from ./. except for
the occurrence in create_lib_submodule_repo as there it seems we craft
a repository deliberately for both inside as well as outside use.
The name "pretzel.[non]bare" was chosen deliberate to not introduce
more strings to the test suite containing "sub[module]" as searching for
"sub" already yields a lot of hits from different contexts. "pretzel"
doesn't occur in the test suite yet, so it is a good candidate for
a potential remote for a submodule.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/lib-submodule-update.sh | 2 ++
t/t7001-mv.sh | 5 +++--
t/t7507-commit-verbose.sh | 4 +++-
t/t7800-difftool.sh | 4 +++-
t/test-lib-functions.sh | 16 ++++++++++++++++
5 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 79cdd34a54..58d76d9df8 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -44,6 +44,8 @@ create_lib_submodule_repo () {
git branch "no_submodule" &&
git checkout -b "add_sub1" &&
+ # Adding the repo itself as a submodule is a hack.
+ # Do not imitate this.
git submodule add ./. sub1 &&
git config -f .gitmodules submodule.sub1.ignore all &&
git config submodule.sub1.ignore all &&
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index e365d1ff77..6cb32f3a3a 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -1,6 +1,7 @@
#!/bin/sh
test_description='git mv in subdirs'
+TEST_CREATE_SUBMODULE=yes
. ./test-lib.sh
test_expect_success \
@@ -288,12 +289,12 @@ rm -f moved symlink
test_expect_success 'setup submodule' '
git commit -m initial &&
git reset --hard &&
- git submodule add ./. sub &&
+ git submodule add ./pretzel.bare sub &&
echo content >file &&
git add file &&
git commit -m "added sub and file" &&
mkdir -p deep/directory/hierarchy &&
- git submodule add ./. deep/directory/hierarchy/sub &&
+ git submodule add ./pretzel.bare deep/directory/hierarchy/sub &&
git commit -m "added another submodule" &&
git branch submodule
'
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index ed2653d46f..d269900afa 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -1,6 +1,7 @@
#!/bin/sh
test_description='verbose commit template'
+TEST_CREATE_SUBMODULE=yes
. ./test-lib.sh
write_script "check-for-diff" <<\EOF &&
@@ -74,11 +75,12 @@ test_expect_success 'diff in message is retained with -v' '
test_expect_success 'submodule log is stripped out too with -v' '
git config diff.submodule log &&
- git submodule add ./. sub &&
+ git submodule add ./pretzel.bare sub &&
git commit -m "sub added" &&
(
cd sub &&
echo "more" >>file &&
+ git add file &&
git commit -a -m "submodule commit"
) &&
(
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461a..d13a5d0453 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -7,6 +7,7 @@ test_description='git-difftool
Testing basic diff tool invocation
'
+TEST_CREATE_SUBMODULE=Yes
. ./test-lib.sh
@@ -534,7 +535,8 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
'
test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
- git submodule add ./. submod/ule &&
+ git submodule add ./pretzel.bare submod/ule &&
+ test_commit -C submod/ule second_commit &&
test_config -C submod/ule diff.tool checktrees &&
test_config -C submod/ule difftool.checktrees.cmd '\''
test -d "$LOCAL" && test -d "$REMOTE" && echo good
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 579e812506..aa327a7dff 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -800,6 +800,22 @@ test_create_repo () {
error "cannot run git init -- have you built things yet?"
mv .git/hooks .git/hooks-disabled
) || exit
+ if test -n "$TEST_CREATE_SUBMODULE"
+ then
+ (
+ cd "$repo"
+ TEST_CREATE_SUBMODULE=
+ export TEST_CREATE_SUBMODULE
+ test_create_repo "pretzel.nonbare"
+ test_commit -C "pretzel.nonbare" \
+ "create submodule" "submodule-file" \
+ "submodule-content" "submodule-tag" >&3 2>&4 ||
+ error "cannot run test_commit"
+ git clone --bare "pretzel.nonbare" \
+ "pretzel.bare" >&3 2>&4 ||
+ error "cannot clone into bare"
+ )
+ fi
}
# This function helps on symlink challenged file systems when it is not
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCHv6 0/2] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-05 19:29 UTC (permalink / raw)
To: bmwill, peff, gitster; +Cc: git, Stefan Beller
v6:
* rebased on top of origin/bw/pathspec-cleanup, resolving conflicts.
(Additionally needs merging with origin/sb/submodule-embed-gitdir to have
6f94351b0, test-lib-functions.sh: teach test_commit -C <dir>)
* reworded comments and commit message
* do not reuse the strip_submodule_slash_expensive function, but have
a dedicated die_inside_submodule_path function.
v5:
* was just resending the latest patch, which turns out to be in conflict with
origin/bw/pathspec-cleanup
v4:
> It MIGHT be a handy hack when writing a test, but let's stop doing
> that insanity. No sane project does that in real life, doesn't it?
> Create a subdirectory, make it a repository, have a commit there and
> bind that as our own submodule. That would be a more normal way to
> start your own superproject and its submodule pair if they originate
> together at the same place.
This comes as an extra patch before the actual fix.
The actual fixing patch was reworded borrowing some words from Jeff.
As this makes use of "test_commit -C", it goes on top of sb/submodule-embed-gitdir
v3:
more defensive and with tests.
Stefan Beller (2):
submodule tests: don't use itself as a submodule
pathspec: give better message for submodule related pathspec error
pathspec.c | 31 ++++++++++++++++++++++---------
t/lib-submodule-update.sh | 2 ++
t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
t/t7001-mv.sh | 5 +++--
t/t7507-commit-verbose.sh | 4 +++-
t/t7800-difftool.sh | 4 +++-
t/test-lib-functions.sh | 16 ++++++++++++++++
7 files changed, 82 insertions(+), 13 deletions(-)
create mode 100755 t/t6134-pathspec-in-submodule.sh
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply
* [PATCH v2 0/4] fix mergetool+rerere+subdir regression
From: Richard Hansen @ 2017-01-06 1:09 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170104005042.51530-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 886 bytes --]
If rerere is enabled, no pathnames are given, and mergetool is run
from a subdirectory, mergetool always prints "No files need merging".
Fix the bug.
This regression was introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
Changes since v1:
* Patch 2/4 was reworked to improve the commit message, improve test
case independence even further, and use 'test_when_finished "git
reset --hard"' instead of a plain 'git reset --hard'.
Richard Hansen (4):
t7610: update branch names to match test number
t7610: make tests more independent and debuggable
t7610: add test case for rerere+mergetool+subdir bug
mergetool: fix running in subdir when rerere enabled
git-mergetool.sh | 1 +
t/t7610-mergetool.sh | 251 ++++++++++++++++++++++++++++++---------------------
2 files changed, 147 insertions(+), 105 deletions(-)
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply
* [PATCH v2 4/4] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-06 1:09 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170106010945.79382-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]
If rerere is enabled and no pathnames are given, run cd_to_toplevel
before running 'git diff --name-only' so that 'git diff --name-only'
sees the pathnames emitted by 'git rerere remaining'.
Also run cd_to_toplevel before running 'git rerere remaining' in case
'git rerere remaining' is ever changed to print pathnames relative to
the current directory rather than to $GIT_WORK_TREE.
This fixes a regression introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
Signed-off-by: Richard Hansen <hansenr@google.com>
---
git-mergetool.sh | 1 +
t/t7610-mergetool.sh | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..67ea0d6db 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -456,6 +456,7 @@ main () {
if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
then
+ cd_to_toplevel
set -- $(git rerere remaining)
if test $# -eq 0
then
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 1cef1ec2e..5d76772cf 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -231,7 +231,7 @@ test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
)
'
-test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+test_expect_success 'mergetool merges all from subdir (rerere enabled)' '
test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled true &&
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH v2 3/4] t7610: add test case for rerere+mergetool+subdir bug
From: Richard Hansen @ 2017-01-06 1:09 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170106010945.79382-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]
If rerere is enabled and mergetool is run from a subdirectory,
mergetool always prints "No files need merging". Add an expected
failure test case for this situation.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
t/t7610-mergetool.sh | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 2c06cf73f..1cef1ec2e 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -213,7 +213,7 @@ test_expect_success 'mergetool skips autoresolved' '
test "$output" = "No files need merging"
'
-test_expect_success 'mergetool merges all from subdir' '
+test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
@@ -231,6 +231,25 @@ test_expect_success 'mergetool merges all from subdir' '
)
'
+test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+ test_when_finished "git reset --hard" &&
+ git checkout -b test$test_count branch1 &&
+ test_config rerere.enabled true &&
+ rm -rf .git/rr-cache &&
+ (
+ cd subdir &&
+ test_must_fail git merge master &&
+ ( yes "r" | git mergetool ../submod ) &&
+ ( yes "d" "d" | git mergetool --no-prompt ) &&
+ test "$(cat ../file1)" = "master updated" &&
+ test "$(cat ../file2)" = "master new" &&
+ test "$(cat file3)" = "master new sub" &&
+ ( cd .. && git submodule update -N ) &&
+ test "$(cat ../submod/bar)" = "master submodule" &&
+ git commit -m "branch2 resolved by mergetool from subdir"
+ )
+'
+
test_expect_success 'mergetool skips resolved paths when rerere is active' '
test_when_finished "git reset --hard" &&
test_config rerere.enabled true &&
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH v2 1/4] t7610: update branch names to match test number
From: Richard Hansen @ 2017-01-06 1:09 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170106010945.79382-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 11801 bytes --]
Rename the testNN branches so that NN matches the test number. This
should make it easier to troubleshoot test issues. Use $test_count to
keep this future-proof.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
t/t7610-mergetool.sh | 82 ++++++++++++++++++++++++++--------------------------
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 6d9f21511..14090739f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -94,7 +94,7 @@ test_expect_success 'setup' '
'
test_expect_success 'custom mergetool' '
- git checkout -b test1 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -113,7 +113,7 @@ test_expect_success 'custom mergetool' '
test_expect_success 'mergetool crlf' '
test_config core.autocrlf true &&
- git checkout -b test2 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
@@ -134,7 +134,7 @@ test_expect_success 'mergetool crlf' '
'
test_expect_success 'mergetool in subdir' '
- git checkout -b test3 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
cd subdir &&
@@ -161,7 +161,7 @@ test_expect_success 'mergetool on file in parent dir' '
'
test_expect_success 'mergetool skips autoresolved' '
- git checkout -b test4 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -192,7 +192,7 @@ test_expect_success 'mergetool merges all from subdir' '
test_expect_success 'mergetool skips resolved paths when rerere is active' '
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
- git checkout -b test5 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
@@ -233,7 +233,7 @@ test_expect_success 'conflicted stash sets up rerere' '
test_expect_success 'mergetool takes partial path' '
git reset --hard &&
test_config rerere.enabled false &&
- git checkout -b test12 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -308,12 +308,12 @@ test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
'
test_expect_success 'deleted vs modified submodule' '
- git checkout -b test6 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
git rm --cached submod &&
git commit -m "Submodule deleted from branch" &&
- git checkout -b test6.a test6 &&
+ git checkout -b test$test_count.a test$test_count &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -329,7 +329,7 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by keeping module" &&
mv submod submod-movedaside &&
- git checkout -b test6.b test6 &&
+ git checkout -b test$test_count.b test$test_count &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -343,9 +343,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
mv submod-movedaside submod &&
- git checkout -b test6.c master &&
+ git checkout -b test$test_count.c master &&
git submodule update -N &&
- test_must_fail git merge test6 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -359,9 +359,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
mv submod.orig submod &&
- git checkout -b test6.d master &&
+ git checkout -b test$test_count.d master &&
git submodule update -N &&
- test_must_fail git merge test6 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -377,14 +377,14 @@ test_expect_success 'deleted vs modified submodule' '
'
test_expect_success 'file vs modified submodule' '
- git checkout -b test7 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
git rm --cached submod &&
echo not a submodule >submod &&
git add submod &&
git commit -m "Submodule path becomes file" &&
- git checkout -b test7.a branch1 &&
+ git checkout -b test$test_count.a branch1 &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -400,7 +400,7 @@ test_expect_success 'file vs modified submodule' '
git commit -m "Merge resolved by keeping module" &&
mv submod submod-movedaside &&
- git checkout -b test7.b test7 &&
+ git checkout -b test$test_count.b test$test_count &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -413,11 +413,11 @@ test_expect_success 'file vs modified submodule' '
test "$output" = "No files need merging" &&
git commit -m "Merge resolved by keeping file" &&
- git checkout -b test7.c master &&
+ git checkout -b test$test_count.c master &&
rmdir submod && mv submod-movedaside submod &&
test ! -e submod.orig &&
git submodule update -N &&
- test_must_fail git merge test7 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -430,10 +430,10 @@ test_expect_success 'file vs modified submodule' '
test "$output" = "No files need merging" &&
git commit -m "Merge resolved by keeping file" &&
- git checkout -b test7.d master &&
+ git checkout -b test$test_count.d master &&
rmdir submod && mv submod.orig submod &&
git submodule update -N &&
- test_must_fail git merge test7 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool both>/dev/null 2>&1 ) &&
@@ -448,7 +448,7 @@ test_expect_success 'file vs modified submodule' '
'
test_expect_success 'submodule in subdirectory' '
- git checkout -b test10 branch1 &&
+ git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
cd subdir &&
@@ -464,52 +464,52 @@ test_expect_success 'submodule in subdirectory' '
git add subdir/subdir_module &&
git commit -m "add submodule in subdirectory" &&
- git checkout -b test10.a test10 &&
+ git checkout -b test$test_count.a test$test_count &&
git submodule update -N &&
(
cd subdir/subdir_module &&
git checkout -b super10.a &&
- echo test10.a >file15 &&
+ echo test$test_count.a >file15 &&
git add file15 &&
git commit -m "on branch 10.a"
) &&
git add subdir/subdir_module &&
- git commit -m "change submodule in subdirectory on test10.a" &&
+ git commit -m "change submodule in subdirectory on test$test_count.a" &&
- git checkout -b test10.b test10 &&
+ git checkout -b test$test_count.b test$test_count &&
git submodule update -N &&
(
cd subdir/subdir_module &&
git checkout -b super10.b &&
- echo test10.b >file15 &&
+ echo test$test_count.b >file15 &&
git add file15 &&
git commit -m "on branch 10.b"
) &&
git add subdir/subdir_module &&
- git commit -m "change submodule in subdirectory on test10.b" &&
+ git commit -m "change submodule in subdirectory on test$test_count.b" &&
- test_must_fail git merge test10.a >/dev/null 2>&1 &&
+ test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
(
cd subdir &&
( yes "l" | git mergetool subdir_module )
) &&
- test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+ test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
git submodule update -N &&
- test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+ test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
git reset --hard &&
git submodule update -N &&
- test_must_fail git merge test10.a >/dev/null 2>&1 &&
+ test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
( yes "r" | git mergetool subdir/subdir_module ) &&
- test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+ test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
git submodule update -N &&
- test "$(cat subdir/subdir_module/file15)" = "test10.a" &&
+ test "$(cat subdir/subdir_module/file15)" = "test$test_count.a" &&
git commit -m "branch1 resolved with mergetool" &&
rm -rf subdir/subdir_module
'
test_expect_success 'directory vs modified submodule' '
- git checkout -b test11 branch1 &&
+ git checkout -b test$test_count branch1 &&
mv submod submod-movedaside &&
git rm --cached submod &&
mkdir submod &&
@@ -537,9 +537,9 @@ test_expect_success 'directory vs modified submodule' '
test "$(cat submod/bar)" = "master submodule" &&
git reset --hard >/dev/null 2>&1 && rm -rf submod-movedaside &&
- git checkout -b test11.c master &&
+ git checkout -b test$test_count.c master &&
git submodule update -N &&
- test_must_fail git merge test11 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "l" | git mergetool submod ) &&
git submodule update -N &&
@@ -547,7 +547,7 @@ test_expect_success 'directory vs modified submodule' '
git reset --hard >/dev/null 2>&1 &&
git submodule update -N &&
- test_must_fail git merge test11 &&
+ test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
test ! -e submod.orig &&
( yes "r" | git mergetool submod ) &&
@@ -559,7 +559,7 @@ test_expect_success 'directory vs modified submodule' '
'
test_expect_success 'file with no base' '
- git checkout -b test13 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool mybase -- both &&
>expected &&
@@ -568,7 +568,7 @@ test_expect_success 'file with no base' '
'
test_expect_success 'custom commands override built-ins' '
- git checkout -b test14 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
test_config mergetool.defaults.trustExitCode true &&
test_must_fail git merge master &&
@@ -579,7 +579,7 @@ test_expect_success 'custom commands override built-ins' '
'
test_expect_success 'filenames seen by tools start with ./' '
- git checkout -b test15 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp false &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
@@ -595,7 +595,7 @@ test_lazy_prereq MKTEMP '
'
test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
- git checkout -b test16 branch1 &&
+ git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp true &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH v2 2/4] t7610: make tests more independent and debuggable
From: Richard Hansen @ 2017-01-06 1:09 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170106010945.79382-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 14301 bytes --]
Reduce how much a test can interfere other tests:
* Move setup code that multiple tests depend on to the 'setup' test
case.
* Run 'git reset --hard' after every test (pass or fail) to clean up
in case the test fails and leaves the repository in a strange
state.
* If the repository must be in a particular state (beyond what is
already done by the 'setup' test case) before the test can run,
make the necessary repository changes in the test script even if
it means duplicating some lines of code from the previous test
case.
* Never assume that a particular commit is checked out.
* Always work on a test-specific branch when the test might create a
commit. This is not always necessary for correctness, but it
improves debuggability by ensuring a commit created by test #N
shows up on the testN branch, not the branch for test #N-1.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
t/t7610-mergetool.sh | 146 +++++++++++++++++++++++++++++----------------------
1 file changed, 84 insertions(+), 62 deletions(-)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 14090739f..2c06cf73f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -55,6 +55,22 @@ test_expect_success 'setup' '
git rm file12 &&
git commit -m "branch1 changes" &&
+ git checkout -b delete-base branch1 &&
+ mkdir -p a/a &&
+ (echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
+ git add a/a/file.txt &&
+ git commit -m"base file" &&
+ git checkout -b move-to-b delete-base &&
+ mkdir -p b/b &&
+ git mv a/a/file.txt b/b/file.txt &&
+ (echo one; echo two; echo 4) >b/b/file.txt &&
+ git commit -a -m"move to b" &&
+ git checkout -b move-to-c delete-base &&
+ mkdir -p c/c &&
+ git mv a/a/file.txt c/c/file.txt &&
+ (echo one; echo two; echo 3) >c/c/file.txt &&
+ git commit -a -m"move to c" &&
+
git checkout -b stash1 master &&
echo stash1 change file11 >file11 &&
git add file11 &&
@@ -86,6 +102,23 @@ test_expect_success 'setup' '
git rm file11 &&
git commit -m "master updates" &&
+ git clean -fdx &&
+ git checkout -b order-file-start master &&
+ echo start >a &&
+ echo start >b &&
+ git add a b &&
+ git commit -m start &&
+ git checkout -b order-file-side1 order-file-start &&
+ echo side1 >a &&
+ echo side1 >b &&
+ git add a b &&
+ git commit -m side1 &&
+ git checkout -b order-file-side2 order-file-start &&
+ echo side2 >a &&
+ echo side2 >b &&
+ git add a b &&
+ git commit -m side2 &&
+
git config merge.tool mytool &&
git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
git config mergetool.mytool.trustExitCode true &&
@@ -94,6 +127,7 @@ test_expect_success 'setup' '
'
test_expect_success 'custom mergetool' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -112,6 +146,7 @@ test_expect_success 'custom mergetool' '
'
test_expect_success 'mergetool crlf' '
+ test_when_finished "git reset --hard" &&
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -129,11 +164,11 @@ test_expect_success 'mergetool crlf' '
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
git commit -m "branch1 resolved with mergetool - autocrlf" &&
- test_config core.autocrlf false &&
- git reset --hard
+ test_config core.autocrlf false
'
test_expect_success 'mergetool in subdir' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -145,8 +180,13 @@ test_expect_success 'mergetool in subdir' '
'
test_expect_success 'mergetool on file in parent dir' '
+ test_when_finished "git reset --hard" &&
+ git checkout -b test$test_count branch1 &&
+ git submodule update -N &&
(
cd subdir &&
+ test_must_fail git merge master >/dev/null 2>&1 &&
+ ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
@@ -161,6 +201,7 @@ test_expect_success 'mergetool on file in parent dir' '
'
test_expect_success 'mergetool skips autoresolved' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -169,11 +210,12 @@ test_expect_success 'mergetool skips autoresolved' '
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
output="$(git mergetool --no-prompt)" &&
- test "$output" = "No files need merging" &&
- git reset --hard
+ test "$output" = "No files need merging"
'
test_expect_success 'mergetool merges all from subdir' '
+ test_when_finished "git reset --hard" &&
+ git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
(
cd subdir &&
@@ -190,6 +232,7 @@ test_expect_success 'mergetool merges all from subdir' '
'
test_expect_success 'mergetool skips resolved paths when rerere is active' '
+ test_when_finished "git reset --hard" &&
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
git checkout -b test$test_count branch1 &&
@@ -199,11 +242,11 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
git submodule update -N &&
output="$(yes "n" | git mergetool --no-prompt)" &&
- test "$output" = "No files need merging" &&
- git reset --hard
+ test "$output" = "No files need merging"
'
test_expect_success 'conflicted stash sets up rerere' '
+ test_when_finished "git reset --hard" &&
test_config rerere.enabled true &&
git checkout stash1 &&
echo "Conflicting stash content" >file11 &&
@@ -231,7 +274,7 @@ test_expect_success 'conflicted stash sets up rerere' '
'
test_expect_success 'mergetool takes partial path' '
- git reset --hard &&
+ test_when_finished "git reset --hard" &&
test_config rerere.enabled false &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
@@ -239,26 +282,12 @@ test_expect_success 'mergetool takes partial path' '
( yes "" | git mergetool subdir ) &&
- test "$(cat subdir/file3)" = "master new sub" &&
- git reset --hard
+ test "$(cat subdir/file3)" = "master new sub"
'
test_expect_success 'mergetool delete/delete conflict' '
- git checkout -b delete-base branch1 &&
- mkdir -p a/a &&
- (echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
- git add a/a/file.txt &&
- git commit -m"base file" &&
- git checkout -b move-to-b delete-base &&
- mkdir -p b/b &&
- git mv a/a/file.txt b/b/file.txt &&
- (echo one; echo two; echo 4) >b/b/file.txt &&
- git commit -a -m"move to b" &&
- git checkout -b move-to-c delete-base &&
- mkdir -p c/c &&
- git mv a/a/file.txt c/c/file.txt &&
- (echo one; echo two; echo 3) >c/c/file.txt &&
- git commit -a -m"move to c" &&
+ test_when_finished "git reset --hard" &&
+ git checkout -b test$test_count move-to-c &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
! test -f a/a/file.txt &&
@@ -269,29 +298,32 @@ test_expect_success 'mergetool delete/delete conflict' '
git reset --hard HEAD &&
test_must_fail git merge move-to-b &&
! echo a | git mergetool a/a/file.txt &&
- ! test -f a/a/file.txt &&
- git reset --hard HEAD
+ ! test -f a/a/file.txt
'
test_expect_success 'mergetool produces no errors when keepBackup is used' '
+ test_when_finished "git reset --hard" &&
+ git checkout -b test$test_count move-to-c &&
test_config mergetool.keepBackup true &&
test_must_fail git merge move-to-b &&
: >expect &&
echo d | git mergetool a/a/file.txt 2>actual &&
test_cmp expect actual &&
- ! test -d a &&
- git reset --hard HEAD
+ ! test -d a
'
test_expect_success 'mergetool honors tempfile config for deleted files' '
+ test_when_finished "git reset --hard" &&
+ git checkout -b test$test_count move-to-c &&
test_config mergetool.keepTemporaries false &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
- ! test -d a &&
- git reset --hard HEAD
+ ! test -d a
'
test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
+ test_when_finished "git reset --hard; git clean -fdx" &&
+ git checkout -b test$test_count move-to-c &&
test_config mergetool.keepTemporaries true &&
test_must_fail git merge move-to-b &&
! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -302,12 +334,11 @@ test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
file_REMOTE_.txt
EOF
ls -1 a/a | sed -e "s/[0-9]*//g" >actual &&
- test_cmp expect actual &&
- git clean -fdx &&
- git reset --hard HEAD
+ test_cmp expect actual
'
test_expect_success 'deleted vs modified submodule' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
@@ -372,11 +403,11 @@ test_expect_success 'deleted vs modified submodule' '
test "$(cat submod/bar)" = "master submodule" &&
output="$(git mergetool --no-prompt)" &&
test "$output" = "No files need merging" &&
- git commit -m "Merge resolved by keeping module" &&
- git reset --hard HEAD
+ git commit -m "Merge resolved by keeping module"
'
test_expect_success 'file vs modified submodule' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
@@ -448,6 +479,7 @@ test_expect_success 'file vs modified submodule' '
'
test_expect_success 'submodule in subdirectory' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -509,6 +541,7 @@ test_expect_success 'submodule in subdirectory' '
'
test_expect_success 'directory vs modified submodule' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
mv submod submod-movedaside &&
git rm --cached submod &&
@@ -559,34 +592,34 @@ test_expect_success 'directory vs modified submodule' '
'
test_expect_success 'file with no base' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool mybase -- both &&
>expected &&
- test_cmp both expected &&
- git reset --hard master >/dev/null 2>&1
+ test_cmp both expected
'
test_expect_success 'custom commands override built-ins' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
test_config mergetool.defaults.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool defaults -- both &&
echo master both added >expected &&
- test_cmp both expected &&
- git reset --hard master >/dev/null 2>&1
+ test_cmp both expected
'
test_expect_success 'filenames seen by tools start with ./' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp false &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool myecho -- both >actual &&
- grep ^\./both_LOCAL_ actual >/dev/null &&
- git reset --hard master >/dev/null 2>&1
+ grep ^\./both_LOCAL_ actual >/dev/null
'
test_lazy_prereq MKTEMP '
@@ -595,6 +628,7 @@ test_lazy_prereq MKTEMP '
'
test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
+ test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp true &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -602,31 +636,17 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
test_must_fail git merge master &&
git mergetool --no-prompt --tool myecho -- both >actual &&
test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
- grep /both_LOCAL_ actual >/dev/null &&
- git reset --hard master >/dev/null 2>&1
+ grep /both_LOCAL_ actual >/dev/null
'
test_expect_success 'diff.orderFile configuration is honored' '
+ test_when_finished "git reset --hard" &&
+ git checkout -b test$test_count order-file-side2 &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
echo b >order-file &&
echo a >>order-file &&
- git checkout -b order-file-start master &&
- echo start >a &&
- echo start >b &&
- git add a b &&
- git commit -m start &&
- git checkout -b order-file-side1 order-file-start &&
- echo side1 >a &&
- echo side1 >b &&
- git add a b &&
- git commit -m side1 &&
- git checkout -b order-file-side2 order-file-start &&
- echo side2 >a &&
- echo side2 >b &&
- git add a b &&
- git commit -m side2 &&
test_must_fail git merge order-file-side1 &&
cat >expect <<-\EOF &&
Merging:
@@ -635,13 +655,16 @@ test_expect_success 'diff.orderFile configuration is honored' '
EOF
git mergetool --no-prompt --tool myecho >output &&
git grep --no-index -h -A2 Merging: output >actual &&
- test_cmp expect actual &&
- git reset --hard >/dev/null
+ test_cmp expect actual
'
test_expect_success 'mergetool -Oorder-file is honored' '
+ test_when_finished "git reset --hard" &&
+ git checkout -b test$test_count order-file-side2 &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
+ echo b >order-file &&
+ echo a >>order-file &&
test_must_fail git merge order-file-side1 &&
cat >expect <<-\EOF &&
Merging:
@@ -662,8 +685,7 @@ test_expect_success 'mergetool -Oorder-file is honored' '
EOF
git mergetool -Oorder-file --no-prompt --tool myecho >output &&
git grep --no-index -h -A2 Merging: output >actual &&
- test_cmp expect actual &&
- git reset --hard >/dev/null 2>&1
+ test_cmp expect actual
'
test_done
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* Re: [PATCH v2 2/4] t7610: make tests more independent and debuggable
From: Stefan Beller @ 2017-01-06 1:31 UTC (permalink / raw)
To: Richard Hansen
Cc: git@vger.kernel.org, David Aguilar, Johannes Sixt, Simon Ruderich
In-Reply-To: <20170106010945.79382-3-hansenr@google.com>
On Thu, Jan 5, 2017 at 5:09 PM, Richard Hansen <hansenr@google.com> wrote:
> Reduce how much a test can interfere other tests:
A bullet point list as an unordered list often indicates that you're
doing multiple
things at once, possibly unrelated, so they could go into different patches. ;)
While telling you to make even more commits: you may also want to make
a patch with an entry to the .mailmap file (assuming you're the same
Richard Hansen that contributed from rhansen@bbn.com);
Welcome to Google!
>
> * Move setup code that multiple tests depend on to the 'setup' test
> case.
> * Run 'git reset --hard' after every test (pass or fail) to clean up
> in case the test fails and leaves the repository in a strange
> state.
> * If the repository must be in a particular state (beyond what is
> already done by the 'setup' test case) before the test can run,
> make the necessary repository changes in the test script even if
> it means duplicating some lines of code from the previous test
> case.
> * Never assume that a particular commit is checked out.
> * Always work on a test-specific branch when the test might create a
> commit. This is not always necessary for correctness, but it
> improves debuggability by ensuring a commit created by test #N
> shows up on the testN branch, not the branch for test #N-1.
> @@ -112,6 +146,7 @@ test_expect_success 'custom mergetool' '
> '
>
> test_expect_success 'mergetool crlf' '
> + test_when_finished "git reset --hard" &&
> test_config core.autocrlf true &&
> git checkout -b test$test_count branch1 &&
> test_must_fail git merge master >/dev/null 2>&1 &&
> @@ -129,11 +164,11 @@ test_expect_success 'mergetool crlf' '
> git submodule update -N &&
> test "$(cat submod/bar)" = "master submodule" &&
> git commit -m "branch1 resolved with mergetool - autocrlf" &&
> - test_config core.autocrlf false &&
> - git reset --hard
> + test_config core.autocrlf false
> '
This is the nit that led me to writing this email in the first place:
test_config is a function that sets a configuration for a single test only,
so it makes no sense as the last statement of a test. (In its implementation
it un-configures with test_when_finished)
So I think we do not want to add it back, but rather remove this
test_config statement.
But to do this we need to actually be careful with the order of the newly
added test_when_finished "git reset --hard" and test_config core.autocrlf true,
which uses test_when_finished internally.
The order seems correct to me, as the reset would be executed after the
"test_config core.autocrlf true" is un-configured.
^ permalink raw reply
* [PATCH 0/3] fixing some random blame corner cases
From: Jeff King @ 2017-01-06 4:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Here are three fixes for some fairly obscure corner cases. I haven't
actually seen these in the wild. I came up with the final one while
discussing a hypothetical with somebody, then ran across the middle one
while trying to write a test for the third, which made me scratch my
head enough to yield the first one. Classic yak-shaving.
One other thing that surprised me while writing blame tests is that
"--root" is not the default for git-blame (though it has been for many
years in git-log). I'm not sure if it would be a good idea to change it,
or if blame is too plumbing-ish to allow that.
[1/3]: blame: fix alignment with --abbrev=40
[2/3]: blame: handle --no-abbrev
[3/3]: blame: output porcelain "previous" header for each file
builtin/blame.c | 27 ++++++----
t/t8002-blame.sh | 32 ++++++++++++
t/t8011-blame-split-file.sh | 117 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 166 insertions(+), 10 deletions(-)
create mode 100755 t/t8011-blame-split-file.sh
-Peff
^ permalink raw reply
* [PATCH 2/3] blame: handle --no-abbrev
From: Jeff King @ 2017-01-06 4:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net>
You can already ask blame for full sha1s with "-l" or with
"--abbrev=40". But for consistency with other parts of Git,
we should support "--no-abbrev".
Worse, blame already accepts --no-abbrev, but it's totally
broken. When we see --no-abbrev, the abbrev variable is set
to 0, which is then used as a printf precision. For regular
sha1s, that means we print nothing at all (which is very
wrong). For boundary commits we decrement it to "-1", which
printf interprets as "no limit" (which is almost correct,
except it misses the 39-length magic explained in the
previous commit).
Let's detect --no-abbrev and behave as if --abbrev=40 was
given.
Signed-off-by: Jeff King <peff@peff.net>
---
I also wondered if we needed to clamp this within MINIMUM_ABBREV, but
that is done for us already by the parseopt handler.
builtin/blame.c | 2 ++
t/t8002-blame.sh | 4 ++++
2 files changed, 6 insertions(+)
diff --git a/builtin/blame.c b/builtin/blame.c
index 1d312542de..c6170fed81 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2659,6 +2659,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
if (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)
/* one more abbrev length is needed for the boundary commit */
abbrev++;
+ else if (!abbrev)
+ abbrev = GIT_SHA1_HEXSZ;
if (revs_file && read_ancestry(revs_file))
die_errno("reading graft file '%s' failed", revs_file);
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index c6347ad8fd..380e1c1054 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -114,4 +114,8 @@ test_expect_success 'blame --abbrev=40 behaves like -l' '
check_abbrev 39 --abbrev=40 ^HEAD
'
+test_expect_success '--no-abbrev works like --abbrev=40' '
+ check_abbrev 40 --no-abbrev
+'
+
test_done
--
2.11.0.519.g31435224cf
^ permalink raw reply related
* [PATCH 1/3] blame: fix alignment with --abbrev=40
From: Jeff King @ 2017-01-06 4:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net>
The blame command internally adds 1 to any requested sha1
abbreviation length, and then subtracts it when outputting a
boundary commit. This lets regular and boundary sha1s line
up visually, but it misses one corner case.
When the requested length is 40, we bump the value to 41.
But since we only have 40 characters, that's all we can show
(fortunately the truncation is done by a printf precision
field, so it never tries to read past the end of the
buffer). So a normal sha1 shows 40 hex characters, and a
boundary sha1 shows "^" plus 40 hex characters. The result
is misaligned.
The "-l" option to show long sha1s gets around this by
skipping the "abbrev" variable entirely and just always
using GIT_SHA1_HEXSZ. This avoids the "+1" issue, but it
does mean that boundary commits only have 39 characters
printed. This is somewhat odd, but it does look good
visually: the results are aligned and left-justified. The
alternative would be to allocate an extra column that would
contain either an extra space or the "^" boundary marker.
As this is by definition the human-readable view, it's
probably not that big a deal either way (and of course
--porcelain, etc, correctly produce correct 40-hex sha1s).
But for consistency, this patch teaches --abbrev=40 to
produce the same output as "-l" (always left-aligned, with
40-hex for normal sha1s, and "^" plus 39-hex for
boundaries).
Signed-off-by: Jeff King <peff@peff.net>
---
I mostly didn't explore the extra-column solution out of a sense of
inertia. The "-l" option has behaved this way for years. But it was also
out of laziness, as I doubt anybody cares too much, and it would require
a fair bit of special-casing in the printing routines.
builtin/blame.c | 2 +-
t/t8002-blame.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..1d312542de 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2656,7 +2656,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
} else if (show_progress < 0)
show_progress = isatty(2);
- if (0 < abbrev)
+ if (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)
/* one more abbrev length is needed for the boundary commit */
abbrev++;
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index ab79de9544..c6347ad8fd 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -86,4 +86,32 @@ test_expect_success 'blame with showEmail config true' '
test_cmp expected_n result
'
+test_expect_success 'set up abbrev tests' '
+ test_commit abbrev &&
+ sha1=$(git rev-parse --verify HEAD) &&
+ check_abbrev () {
+ expect=$1; shift
+ echo $sha1 | cut -c 1-$expect >expect &&
+ git blame "$@" abbrev.t >actual &&
+ perl -lne "/[0-9a-f]+/ and print \$&" <actual >actual.sha &&
+ test_cmp expect actual.sha
+ }
+'
+
+test_expect_success 'blame --abbrev=<n> works' '
+ # non-boundary commits get +1 for alignment
+ check_abbrev 31 --abbrev=30 HEAD &&
+ check_abbrev 30 --abbrev=30 ^HEAD
+'
+
+test_expect_success 'blame -l aligns regular and boundary commits' '
+ check_abbrev 40 -l HEAD &&
+ check_abbrev 39 -l ^HEAD
+'
+
+test_expect_success 'blame --abbrev=40 behaves like -l' '
+ check_abbrev 40 --abbrev=40 HEAD &&
+ check_abbrev 39 --abbrev=40 ^HEAD
+'
+
test_done
--
2.11.0.519.g31435224cf
^ permalink raw reply related
* [PATCH 3/3] blame: output porcelain "previous" header for each file
From: Jeff King @ 2017-01-06 4:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net>
It's possible for content currently found in one file to
have originated in two separate files, each of which may
have been modified in some single older commit. The
--porcelain output generates an incorrect "previous" header
in this case, whereas --line-porcelain gets it right. The
problem is that the porcelain output tries to omit repeated
details of commits, and treats "previous" as a property of
the commit, when it is really a property of the blamed block
of lines.
Let's look at an example. In a case like this, you might see
this output from --line-porcelain:
SOME_SHA1 1 1 1
author ...
committer ...
previous SOME_SHA1^ file_one
filename file_one
...some line content...
SOME_SHA1 2 1 1
author ...
committer ...
previous SOME_SHA1^ file_two
filename file_two
...some different content....
The "filename" fields tell us that the two lines are from
two different files. But notice that the filename also
appears in the "previous" field, which tells us where to
start a re-blame. The second content line never appeared in
file_one at all, so we would obviously need to re-blame from
file_two (or possibly even some other file, if had just been
renamed to file_two in SOME_SHA1).
So far so good. Now here's what --porcelain looks like:
SOME_SHA1 1 1 1
author ...
committer ...
previous SOME_SHA1^ file_one
filename file_one
...some line content...
SOME_SHA1 2 1 1
filename file_two
...some different content....
We've dropped the author and committer fields from the
second line, as they would just be repeats. But we can't
omit "filename", because it depends on the actual block of
blamed lines, not just the commit. This is handled by
emit_porcelain_details(), which will show the filename
either if it is the first mention of the commit _or_ if the
commit has multiple paths in it.
But we don't give "previous" the same handling. It's written
inside emit_one_suspect_detail(), which bails early if we've
already seen that commit. And so the output above is wrong;
a reader would assume that the correct place to re-blame
line two is from file_one, but that's obviously nonsense.
Let's treat "previous" the same as "filename", and show it
fresh whenever we know we are in a confusing case like this.
Signed-off-by: Jeff King <peff@peff.net>
---
I'm assuming that the parent sha1 for a "previous" field will always be
the same for a given commit. So we really only need to care about
reprinting when we know there are multiple paths, as this patch does
(i.e., treat it exactly the same as "filename"). If I'm wrong, then
there's probably another corner case that this doesn't handle. I
couldn't think of a way to trigger such a setup, though.
builtin/blame.c | 23 +++++----
t/t8011-blame-split-file.sh | 117 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 131 insertions(+), 9 deletions(-)
create mode 100755 t/t8011-blame-split-file.sh
diff --git a/builtin/blame.c b/builtin/blame.c
index c6170fed81..3aae19a0f9 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1700,13 +1700,23 @@ static void get_commit_info(struct commit *commit,
}
/*
+ * Write out any suspect information which depends on the path. This must be
+ * handled separately from emit_one_suspect_detail(), because a given commit
+ * may have changes in multiple paths. So this needs to appear each time
+ * we mention a new group.
+ *
* To allow LF and other nonportable characters in pathnames,
* they are c-style quoted as needed.
*/
-static void write_filename_info(const char *path)
+static void write_filename_info(struct origin *suspect)
{
+ if (suspect->previous) {
+ struct origin *prev = suspect->previous;
+ printf("previous %s ", oid_to_hex(&prev->commit->object.oid));
+ write_name_quoted(prev->path, stdout, '\n');
+ }
printf("filename ");
- write_name_quoted(path, stdout, '\n');
+ write_name_quoted(suspect->path, stdout, '\n');
}
/*
@@ -1735,11 +1745,6 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)
printf("summary %s\n", ci.summary.buf);
if (suspect->commit->object.flags & UNINTERESTING)
printf("boundary\n");
- if (suspect->previous) {
- struct origin *prev = suspect->previous;
- printf("previous %s ", oid_to_hex(&prev->commit->object.oid));
- write_name_quoted(prev->path, stdout, '\n');
- }
commit_info_destroy(&ci);
@@ -1760,7 +1765,7 @@ static void found_guilty_entry(struct blame_entry *ent,
oid_to_hex(&suspect->commit->object.oid),
ent->s_lno + 1, ent->lno + 1, ent->num_lines);
emit_one_suspect_detail(suspect, 0);
- write_filename_info(suspect->path);
+ write_filename_info(suspect);
maybe_flush_or_die(stdout, "stdout");
}
pi->blamed_lines += ent->num_lines;
@@ -1884,7 +1889,7 @@ static void emit_porcelain_details(struct origin *suspect, int repeat)
{
if (emit_one_suspect_detail(suspect, repeat) ||
(suspect->commit->object.flags & MORE_THAN_ONE_PATH))
- write_filename_info(suspect->path);
+ write_filename_info(suspect);
}
static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
diff --git a/t/t8011-blame-split-file.sh b/t/t8011-blame-split-file.sh
new file mode 100755
index 0000000000..831125047b
--- /dev/null
+++ b/t/t8011-blame-split-file.sh
@@ -0,0 +1,117 @@
+#!/bin/sh
+
+test_description='
+The general idea is that we have a single file whose lines come from
+multiple other files, and those individual files were modified in the same
+commits. That means that we will see the same commit in multiple contexts,
+and each one should be attributed to the correct file.
+
+Note that we need to use "blame -C" to find the commit for all lines. We will
+not bother testing that the non-C case fails to find it. That is how blame
+behaves now, but it is not a property we want to make sure is retained.
+'
+. ./test-lib.sh
+
+# help avoid typing and reading long strings of similar lines
+# in the tests below
+generate_expect () {
+ while read nr data
+ do
+ i=0
+ while test $i -lt $nr
+ do
+ echo $data
+ i=$((i + 1))
+ done
+ done
+}
+
+test_expect_success 'setup split file case' '
+ # use lines long enough to trigger content detection
+ test_seq 1000 1010 >one &&
+ test_seq 2000 2010 >two &&
+ git add one two &&
+ test_commit base &&
+
+ sed "6s/^/modified /" <one >one.tmp &&
+ mv one.tmp one &&
+ sed "6s/^/modified /" <two >two.tmp &&
+ mv two.tmp two &&
+ git add -u &&
+ test_commit modified &&
+
+ cat one two >combined &&
+ git add combined &&
+ git rm one two &&
+ test_commit combined
+'
+
+test_expect_success 'setup simulated porcelain' '
+ # This just reads porcelain-ish output and tries
+ # to output the value of a given field for each line (either by
+ # reading the field that accompanies this line, or referencing
+ # the information found last time the commit was mentioned).
+ cat >read-porcelain.pl <<-\EOF
+ my $field = shift;
+ while (<>) {
+ if (/^[0-9a-f]{40} /) {
+ flush();
+ $hash = $&;
+ } elsif (/^$field (.*)/) {
+ $cache{$hash} = $1;
+ }
+ }
+ flush();
+
+ sub flush {
+ return unless defined $hash;
+ if (defined $cache{$hash}) {
+ print "$cache{$hash}\n";
+ } else {
+ print "NONE\n";
+ }
+ }
+ EOF
+'
+
+for output in porcelain line-porcelain
+do
+ test_expect_success "generate --$output output" '
+ git blame --root -C --$output combined >output
+ '
+
+ test_expect_success "$output output finds correct commits" '
+ generate_expect >expect <<-\EOF &&
+ 5 base
+ 1 modified
+ 10 base
+ 1 modified
+ 5 base
+ EOF
+ perl read-porcelain.pl summary <output >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$output output shows correct filenames" '
+ generate_expect >expect <<-\EOF &&
+ 11 one
+ 11 two
+ EOF
+ perl read-porcelain.pl filename <output >actual &&
+ test_cmp expect actual
+ '
+
+ test_expect_success "$output output shows correct previous pointer" '
+ generate_expect >expect <<-EOF &&
+ 5 NONE
+ 1 $(git rev-parse modified^) one
+ 10 NONE
+ 1 $(git rev-parse modified^) two
+ 5 NONE
+ EOF
+ perl read-porcelain.pl previous <output >actual &&
+ test_cmp expect actual
+ '
+done
+
+test_done
--
2.11.0.519.g31435224cf
^ permalink raw reply related
* [PATCH] branch_get_push: do not segfault when HEAD is detached
From: Kyle Meyer @ 2017-01-06 4:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Kyle Meyer
Move the detached HEAD check from branch_get_push_1() to
branch_get_push() to avoid setting branch->push_tracking_ref when
branch is NULL.
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
remote.c | 6 +++---
t/t1514-rev-parse-push.sh | 6 ++++++
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index ad6c5424e..d5eaec737 100644
--- a/remote.c
+++ b/remote.c
@@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
{
struct remote *remote;
- if (!branch)
- return error_buf(err, _("HEAD does not point to a branch"));
-
remote = remote_get(pushremote_for_branch(branch, NULL));
if (!remote)
return error_buf(err,
@@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
const char *branch_get_push(struct branch *branch, struct strbuf *err)
{
+ if (!branch)
+ return error_buf(err, _("HEAD does not point to a branch"));
+
if (!branch->push_tracking_ref)
branch->push_tracking_ref = branch_get_push_1(branch, err);
return branch->push_tracking_ref;
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 7214f5b33..90c639ae1 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '
resolve topic@{push} refs/remotes/origin/magic/topic
'
+test_expect_success 'resolving @{push} fails with a detached HEAD' '
+ git checkout HEAD^{} &&
+ test_when_finished "git checkout -" &&
+ test_must_fail git rev-parse @{push}
+'
+
test_done
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] branch_get_push: do not segfault when HEAD is detached
From: Jeff King @ 2017-01-06 6:18 UTC (permalink / raw)
To: Kyle Meyer; +Cc: git
In-Reply-To: <20170106045623.21118-1-kyle@kyleam.com>
On Thu, Jan 05, 2017 at 11:56:23PM -0500, Kyle Meyer wrote:
> Move the detached HEAD check from branch_get_push_1() to
> branch_get_push() to avoid setting branch->push_tracking_ref when
> branch is NULL.
Yep, I think this is the right fix.
> diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
> index 7214f5b33..90c639ae1 100755
> --- a/t/t1514-rev-parse-push.sh
> +++ b/t/t1514-rev-parse-push.sh
> @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '
> resolve topic@{push} refs/remotes/origin/magic/topic
> '
>
> +test_expect_success 'resolving @{push} fails with a detached HEAD' '
> + git checkout HEAD^{} &&
> + test_when_finished "git checkout -" &&
> + test_must_fail git rev-parse @{push}
> +'
Looks good. Thanks.
-Peff
PS Looks like this is your first patch. Welcome. :)
^ permalink raw reply
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Jeff King @ 2017-01-06 6:40 UTC (permalink / raw)
To: Trygve Aaberge; +Cc: git
In-Reply-To: <20170105142529.GA15009@aaberge.net>
On Thu, Jan 05, 2017 at 03:25:29PM +0100, Trygve Aaberge wrote:
> I'm experiencing an issue when using aliases for commands that open the pager.
> When I press Ctrl-c from the pager, it exits. This does not happen when I
> don't use an alias and did not happen before. It causes problems because
> Ctrl-c is also used for other things, such as canceling a search that hasn't
> completed.
>
> To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
> that the pager exits.
That's weird. I can't reproduce at all here. But I also can't think of a
thing that Git could do that would impact the behavior. For example:
1. Git should never kill() the pager; in fact it waits for it to
finish.
2. Git can impact the pager environment and command line options, but
those haven't changed anytime recently (and besides, I'm not sure
there even is an option to convince less to die).
3. Git can impact the set of blocked signals that the pager sees, but
I think less would set up its own handler for SIGINT anyway.
I suppose it's possible that your shell or another program (tmux,
maybe?) catches the SIGINT and does a process-group kill. But then I
don't know why it would matter that you're using an alias. The process
tree without an alias:
|-xterm,21861,peff
| `-bash,21863
| `-git,22376 log
| `-less,22377
and with:
|-xterm,21861,peff
| `-bash,21863
| `-git,22391 l
| `-git-log,22393
| `-less,22394
are pretty similar.
Puzzling.
-Peff
^ permalink raw reply
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Jeff King @ 2017-01-06 6:47 UTC (permalink / raw)
To: Trygve Aaberge; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170106064032.eqxxer5mx5hsh2md@sigill.intra.peff.net>
On Fri, Jan 06, 2017 at 01:40:32AM -0500, Jeff King wrote:
> On Thu, Jan 05, 2017 at 03:25:29PM +0100, Trygve Aaberge wrote:
>
> > I'm experiencing an issue when using aliases for commands that open the pager.
> > When I press Ctrl-c from the pager, it exits. This does not happen when I
> > don't use an alias and did not happen before. It causes problems because
> > Ctrl-c is also used for other things, such as canceling a search that hasn't
> > completed.
> >
> > To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> > Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
> > that the pager exits.
>
> That's weird. I can't reproduce at all here. But I also can't think of a
> thing that Git could do that would impact the behavior. For example:
I take it back. I could not reproduce under my normal config, which sets
the pager to "diff-highlight | less". But if I drop that config, I can
reproduce easily. And bisect it to that same commit, 86d26f240f
(setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..,
2015-12-20).
-Peff
^ permalink raw reply
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Jeff King @ 2017-01-06 7:26 UTC (permalink / raw)
To: Trygve Aaberge; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170106064752.iccrk656c6k2wrfy@sigill.intra.peff.net>
On Fri, Jan 06, 2017 at 01:47:52AM -0500, Jeff King wrote:
> > > To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> > > Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
> > > that the pager exits.
> >
> > That's weird. I can't reproduce at all here. But I also can't think of a
> > thing that Git could do that would impact the behavior. For example:
>
> I take it back. I could not reproduce under my normal config, which sets
> the pager to "diff-highlight | less". But if I drop that config, I can
> reproduce easily. And bisect it to that same commit, 86d26f240f
> (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..,
> 2015-12-20).
I made a little progress with strace. Prior to 86d26f240f, we would run
the "log" builtin directly inside the same executable. After it, we exec
a separate process, and the process tree looks like:
| `-bash,10123
| `-git,10125 l
| `-git-log,10127
| `-less,10128
Now if I strace all of those, I see (I've reordered and snipped a bit
for clarity):
$ strace -p 10125 -p 10127 -p 10128
strace: Process 10125 attached
strace: Process 10127 attached
strace: Process 10128 attached
[pid 10127] write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481 <unfinished ...>
[pid 10128] read(5, <unfinished ...>
[pid 10125] wait4(10127, <unfinished ...>
The main git process is waiting for the child to finish, the child is
blocked writing to the pager, and the pager is waiting for input from
the terminal (fd 5).
So I hit ^C:
[pid 10128] <... read resumed> 0x7ffd39153b57, 1) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
[pid 10127] <... write resumed> ) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
[pid 10125] <... wait4 resumed> 0x7ffe88d0a560, 0, NULL) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
[pid 10128] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
[pid 10127] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
[pid 10125] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
Everybody gets the signal...
[pid 10127] write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481 <unfinished ...>
The blocked writer will resume its write() until it returns. This is the
same as it would get under the older code, too (after write() returns it
will handle the signal and die).
[pid 10125] kill(10127, SIGINT <unfinished ...>
[pid 10125] <... kill resumed> ) = 0
The parent git process tries to propagate the signal to the child.
Unnecessary in this instance, but helpful when the signal is only
delivered to the parent. This is due to
[pid 10128] rt_sigaction(SIGINT, {sa_handler=0x558dd1af0300, sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f03fc8c2040}, <unfinished ...>
[pid 10128] <... rt_sigaction resumed> {sa_handler=0x558dd1af0300, sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f03fc8c2040}, 8) = 0
[pid 10128] rt_sigprocmask(SIG_SETMASK, [], <unfinished ...>
[pid 10128] <... rt_sigprocmask resumed> NULL, 8) = 0
[pid 10128] write(1, "\7\r\33[K:\33[K", 9 <unfinished ...>
[pid 10128] <... write resumed> ) = 9
[pid 10128] read(5, <unfinished ...>
And here's the pager handling the signal, and going back to waiting for
terminal input.
[pid 10125] rt_sigaction(SIGINT, {sa_handler=SIG_DFL, sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fbe2b4c1040}, <unfinished ...>
[pid 10125] <... rt_sigaction resumed> {sa_handler=0x55aec373a6a0, sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fbe2b4c1040}, 8) = 0
[pid 10125] rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [INT], 8) = 0
[pid 10125] getpid( <unfinished ...>
[pid 10125] <... getpid resumed> ) = 10125
[pid 10125] gettid() = 10125
[pid 10125] tgkill(10125, 10125, SIGINT) = 0
[pid 10125] rt_sigprocmask(SIG_SETMASK, [INT], NULL, 8) = 0
[pid 10125] rt_sigreturn({mask=[]}) = 61
[pid 10125] --- SIGINT {si_signo=SIGINT, si_code=SI_TKILL, si_pid=10125, si_uid=1000} ---
[pid 10125] +++ killed by SIGINT +++
The parent process pops the signal handler and propagates to itself,
dying.
At this point things pause, and nothing happens. But as soon as I hit a
key, the pager dies:
[pid 10128] <... read resumed> "\r", 1) = 1
[pid 10128] write(1, "\r\33[K", 4) = 4
[pid 10128] write(1, " \"Another round of MIPS fixe"..., 50) = 50
[pid 10128] read(5, 0x7ffd39153b57, 1) = -1 EIO (Input/output error)
[pid 10128] write(1, "\r\33[K\33[?1l\33>", 11) = 11
[pid 10128] fsync(5) = -1 EINVAL (Invalid argument)
[pid 10128] ioctl(5, TCGETS, {B38400 opost isig -icanon -echo ...}) = 0
[pid 10128] ioctl(5, SNDCTL_TMR_STOP or TCSETSW, {B38400 opost isig icanon echo ...}) = -1 EIO (Input/output error)
[pid 10128] exit_group(1) = ?
[pid 10128] +++ exited with 1 +++
The key thing is the EIO we get reading from the terminal. I think
because the head of the process group died, we lost our controlling
terminal.
And then naturally the git-log process gets SIGPIPE and dies:
<... write resumed> ) = -1 EPIPE (Broken pipe)
--- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=10127, si_uid=1000} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=10128, si_uid=1000, si_status=1, si_utime=0, si_stime=0} ---
write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481) = -1 EPIPE (Broken pipe)
close(1) = 0
close(2) = 0
wait4(10128, [{WIFEXITED(s) && WEXITSTATUS(s) == 1}], 0, NULL) = 10128
rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fa2d51e2040}, {sa_handler=0x564583689d30, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fa2d51e2040}, 8) = 0
rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [INT PIPE], 8) = 0
getpid() = 10127
gettid() = 10127
tgkill(10127, 10127, SIGPIPE) = 0
rt_sigprocmask(SIG_SETMASK, [INT PIPE], NULL, 8) = 0
rt_sigreturn({mask=[INT]}) = -1 EPIPE (Broken pipe)
--- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=10127, si_uid=1000} ---
+++ killed by SIGPIPE +++
You'll notice that it actually calls wait() on the pager. That's due to
a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
IIRC was addressing a very similar problem. We want to stop feeding the
pager when we get a signal, but we don't want the main process to
actually exit, or the pager loses the controlling terminal.
In our new scenario we have an extra process, though. The git-log child
will wait on the pager, but the parent process can't. It doesn't know
about it. I think that it in turn needs to wait on the child when it
dies, and then the whole chain will stand still until the pager exits.
-Peff
^ permalink raw reply
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Jeff King @ 2017-01-06 7:32 UTC (permalink / raw)
To: Trygve Aaberge; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170106072602.wkbzho5z3osz5hee@sigill.intra.peff.net>
On Fri, Jan 06, 2017 at 02:26:02AM -0500, Jeff King wrote:
> You'll notice that it actually calls wait() on the pager. That's due to
> a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
> IIRC was addressing a very similar problem. We want to stop feeding the
> pager when we get a signal, but we don't want the main process to
> actually exit, or the pager loses the controlling terminal.
>
> In our new scenario we have an extra process, though. The git-log child
> will wait on the pager, but the parent process can't. It doesn't know
> about it. I think that it in turn needs to wait on the child when it
> dies, and then the whole chain will stand still until the pager exits.
And here's a patch to do that. It seems to work.
I'll sleep on it and then write up a commit message tomorrow if it still
makes sense.
diff --git a/run-command.c b/run-command.c
index ca905a9e80..db47c429b7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
static void cleanup_children(int sig, int in_signal)
{
+ struct child_to_clean *children_to_wait_for = NULL;
+
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
@@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
}
kill(p->pid, sig);
+ p->next = children_to_wait_for;
+ children_to_wait_for = p;
+ }
+
+ while (children_to_wait_for) {
+ struct child_to_clean *p = children_to_wait_for;
+ children_to_wait_for = p->next;
+
+ while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
+ ; /* spin waiting for process exit or error */
+
if (!in_signal)
free(p);
}
^ permalink raw reply related
* Squashing commits with git rebase -i may miscount commits in commit message
From: Brandon Tolsch @ 2017-01-06 9:04 UTC (permalink / raw)
To: git
git --version: 2.11.0
When using git rebase -i to squash a series of commits that includes
more than 10 commits, the generated commit message you are given to
edit counts the old messages incorrectly. It will say the total
number of commits is (actual % 10) (if they were 0-based) and it will
also count the commits as (actual % 10).
For example, 15, 25, 35, etc. commits:
# This is a combination of 5 commits.
# This is the 1st commit message:
msg
# This is the commit message #2:
...
...
# This is commit message #10:
msg
# This is commit message #1:
...
# This is commit message #5:
msg
While not a big issue, it did make me double check what I was doing
when I saw "a combination of 10 commits" instead of 20 in the commit
message.
-Brandon Tolsch
^ permalink raw reply
* Encoding issue in git gui
From: Anton Tsyganenko @ 2017-01-06 8:59 UTC (permalink / raw)
To: git
[-- Attachment #1.1.1: Type: text/plain, Size: 301 bytes --]
When I run a tool from tools menu in the standard git gui, the output
seems to have wrong encoding. For example, add a tool called "encoding
test" that runs command "echo Проверка", then run it. In the output
window "Проверка" in printed instead of "Проверка".
[-- Attachment #1.1.2: 2017-01-06-115157_416x253_scrot.png --]
[-- Type: image/png, Size: 19154 bytes --]
[-- Attachment #1.1.3: 2017-01-06-115239_831x305_scrot.png --]
[-- Type: image/png, Size: 10701 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 870 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/4] mergetool: fix running in subdir when rerere enabled
From: Johannes Sixt @ 2017-01-06 9:42 UTC (permalink / raw)
To: Richard Hansen; +Cc: git, davvid, sbeller, simon
In-Reply-To: <20170106010945.79382-5-hansenr@google.com>
Am 06.01.2017 um 02:09 schrieb Richard Hansen:
> If rerere is enabled and no pathnames are given, run cd_to_toplevel
> before running 'git diff --name-only' so that 'git diff --name-only'
> sees the pathnames emitted by 'git rerere remaining'.
>
> Also run cd_to_toplevel before running 'git rerere remaining' in case
> 'git rerere remaining' is ever changed to print pathnames relative to
> the current directory rather than to $GIT_WORK_TREE.
>
> This fixes a regression introduced in
> 57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
>
> Signed-off-by: Richard Hansen <hansenr@google.com>
> ---
> git-mergetool.sh | 1 +
> t/t7610-mergetool.sh | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index e52b4e4f2..67ea0d6db 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -456,6 +456,7 @@ main () {
>
> if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> then
> + cd_to_toplevel
> set -- $(git rerere remaining)
> if test $# -eq 0
> then
This cannot be a complete solution. Why do we have another
cd_to_toplevel later, after `git diff --name-only -- "$@"`?
Maybe it is necessary to revert back to the flow control that we had
before 57937f70a09c ("mergetool: honor diff.orderFile", 2016-10-07)? It
did not have `test $# -eq 0` and `test -e "$GIT_DIR/MERGE_RR"` in a
single condition.
-- Hannes
^ permalink raw reply
* Re: git branch -D doesn't work with deleted worktree
From: Duy Nguyen @ 2017-01-06 10:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: Roland Illig, git@vger.kernel.org
In-Reply-To: <CAGZ79kaLpf1nzSAgRJQamMGk-327LO+qQYihYVVcU+86n92ivg@mail.gmail.com>
On Thu, Jan 5, 2017 at 9:02 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Jan 5, 2017 at 2:06 AM, Roland Illig <rillig@novomind.com> wrote:
>> Git 2.11.0 gives a wrong error message after the following commands:
>>
>> $ git init
>> $ echo hello >file
>> $ git add file
>> $ git commit -m "message"
>> $ git worktree add ../worktree
>> $ rm -rf ../worktree
>> $ git br -D worktree
>> error: Cannot delete branch 'worktree' checked out at '../worktree'
>>
>> Since ../worktree has been deleted, there cannot be anything checked out at that location.
>>
>> In my opinion, deleting the branch should just work. Especially since I used the -D option and the "git worktree" documentation says "When you are done with a linked working tree you can simply delete it."
Since -D means "I know what I'm doing, get out of my way", maybe we
should continue if any worktree has the branch checked out by
detaching it?
(Yes I'm carefully tip toeing around the deleted worktree issue since
"git worktree remove" is coming. After that point, running "worktree
prune" before "branch -D" does not sound so bad)
--
Duy
^ permalink raw reply
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Trygve Aaberge @ 2017-01-06 13:19 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170106073224.5hsrib77tx5tgx7d@sigill.intra.peff.net>
On Fri, Jan 06, 2017 at 02:32:25 -0500, Jeff King wrote:
> And here's a patch to do that. It seems to work.
Nice, thanks! This works very well for me too.
--
Trygve Aaberge
^ permalink raw reply
* Re: [PATCH] branch_get_push: do not segfault when HEAD is detached
From: Johannes Schindelin @ 2017-01-06 13:41 UTC (permalink / raw)
To: Kyle Meyer; +Cc: git, Jeff King
In-Reply-To: <20170106045623.21118-1-kyle@kyleam.com>
Hi Kyle,
On Thu, 5 Jan 2017, Kyle Meyer wrote:
> Move the detached HEAD check from branch_get_push_1() to
> branch_get_push() to avoid setting branch->push_tracking_ref when
> branch is NULL.
>
> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
Good point.
> diff --git a/remote.c b/remote.c
> index ad6c5424e..d5eaec737 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
> {
> struct remote *remote;
>
> - if (!branch)
> - return error_buf(err, _("HEAD does not point to a branch"));
> -
> remote = remote_get(pushremote_for_branch(branch, NULL));
> if (!remote)
> return error_buf(err,
> @@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
>
> const char *branch_get_push(struct branch *branch, struct strbuf *err)
> {
> + if (!branch)
> + return error_buf(err, _("HEAD does not point to a branch"));
> +
> if (!branch->push_tracking_ref)
> branch->push_tracking_ref = branch_get_push_1(branch, err);
This is the only caller of branch_get_push_1(), so all is good.
> diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
> index 7214f5b33..90c639ae1 100755
> --- a/t/t1514-rev-parse-push.sh
> +++ b/t/t1514-rev-parse-push.sh
> @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '
> resolve topic@{push} refs/remotes/origin/magic/topic
> '
>
> +test_expect_success 'resolving @{push} fails with a detached HEAD' '
> + git checkout HEAD^{} &&
I seem to recall that we prefer HEAD^0 over HEAD^{} and the existing
scripts seem to agree with me:
$ git grep -c HEAD^0 junio/pu -- t/
junio/pu:t/t1450-fsck.sh:1
junio/pu:t/t1507-rev-parse-upstream.sh:2
junio/pu:t/t2020-checkout-detach.sh:5
junio/pu:t/t3200-branch.sh:1
junio/pu:t/t3203-branch-output.sh:3
junio/pu:t/t3400-rebase.sh:1
junio/pu:t/t3404-rebase-interactive.sh:1
junio/pu:t/t5407-post-rewrite-hook.sh:2
junio/pu:t/t5505-remote.sh:1
junio/pu:t/t5510-fetch.sh:1
junio/pu:t/t5533-push-cas.sh:3
junio/pu:t/t6035-merge-dir-to-symlink.sh:3
junio/pu:t/t7201-co.sh:2
junio/pu:t/t7402-submodule-rebase.sh:1
junio/pu:t/t9105-git-svn-commit-diff.sh:1
junio/pu:t/t9107-git-svn-migrate.sh:1
$ git grep -c HEAD^{} junio/pu -- t/
junio/pu:t/t3200-branch.sh:3
Maybe use HEAD^0 just for consistency?
Ciao,
Johannes
^ permalink raw reply
* Re: Squashing commits with git rebase -i may miscount commits in commit message
From: Johannes Schindelin @ 2017-01-06 13:46 UTC (permalink / raw)
To: Brandon Tolsch; +Cc: git
In-Reply-To: <CAMWRQeRaQQQcJ-R8eHc7f0KqZF2eEkYJOyTb9n7ds78pTqV-AA@mail.gmail.com>
Hi,
On Fri, 6 Jan 2017, Brandon Tolsch wrote:
> git --version: 2.11.0
>
> When using git rebase -i to squash a series of commits that includes
> more than 10 commits, the generated commit message you are given to
> edit counts the old messages incorrectly. It will say the total
> number of commits is (actual % 10) (if they were 0-based) and it will
> also count the commits as (actual % 10).
>
> For example, 15, 25, 35, etc. commits:
> # This is a combination of 5 commits.
> # This is the 1st commit message:
> msg
>
> # This is the commit message #2:
> ...
> ...
> # This is commit message #10:
> msg
>
> # This is commit message #1:
> ...
>
> # This is commit message #5:
> msg
>
> While not a big issue, it did make me double check what I was doing
> when I saw "a combination of 10 commits" instead of 20 in the commit
> message.
Just for the record: I verified that the rebase--helper based interactive
rebase (which is already in Git for Windows since v2.10.0) does *not* have
this bug.
Maybe a point in favor rebase--helper...
Ciao,
Johannes
^ 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