* [PATCH v4 05/14] t7610: use test_when_finished for cleanup tasks
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 9927 bytes --]
This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
t/t7610-mergetool.sh | 71 +++++++++++++++++++++++++++-------------------------
1 file changed, 37 insertions(+), 34 deletions(-)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 550838a1c..f62ceffdc 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -145,6 +145,11 @@ test_expect_success 'custom mergetool' '
'
test_expect_success 'mergetool crlf' '
+ test_when_finished "git reset --hard" &&
+ # This test_config line must go after the above reset line so that
+ # core.autocrlf is unconfigured before reset runs. (The
+ # test_config command uses test_when_finished internally and
+ # test_when_finished is LIFO.)
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -161,9 +166,7 @@ test_expect_success 'mergetool crlf' '
test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
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
+ git commit -m "branch1 resolved with mergetool - autocrlf"
'
test_expect_success 'mergetool in subdir' '
@@ -194,6 +197,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 &&
@@ -202,8 +206,7 @@ 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' '
@@ -223,6 +226,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 &&
@@ -232,8 +236,7 @@ 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' '
@@ -264,6 +267,7 @@ test_expect_success 'conflicted stash sets up rerere' '
'
test_expect_success 'mergetool takes partial path' '
+ test_when_finished "git reset --hard" &&
git reset --hard &&
test_config rerere.enabled false &&
git checkout -b test$test_count branch1 &&
@@ -272,11 +276,11 @@ 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' '
+ test_when_finished "git reset --hard HEAD" &&
git checkout move-to-c &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
@@ -288,29 +292,30 @@ 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 HEAD" &&
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 HEAD" &&
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 HEAD" &&
+ test_when_finished "git clean -fdx" &&
test_config mergetool.keepTemporaries true &&
test_must_fail git merge move-to-b &&
! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -321,12 +326,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 HEAD" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
@@ -391,8 +395,7 @@ 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' '
@@ -479,6 +482,7 @@ test_expect_success 'submodule in subdirectory' '
git commit -m "add initial versions"
)
) &&
+ test_when_finished "rm -rf subdir/subdir_module" &&
git submodule add git://example.com/subsubmodule subdir/subdir_module &&
git add subdir/subdir_module &&
git commit -m "add submodule in subdirectory" &&
@@ -523,8 +527,7 @@ test_expect_success 'submodule in subdirectory' '
test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
git submodule update -N &&
test "$(cat subdir/subdir_module/file15)" = "test$test_count.a" &&
- git commit -m "branch1 resolved with mergetool" &&
- rm -rf subdir/subdir_module
+ git commit -m "branch1 resolved with mergetool"
'
test_expect_success 'directory vs modified submodule' '
@@ -578,34 +581,34 @@ test_expect_success 'directory vs modified submodule' '
'
test_expect_success 'file with no base' '
+ test_when_finished "git reset --hard master >/dev/null 2>&1" &&
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 master >/dev/null 2>&1" &&
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 master >/dev/null 2>&1" &&
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 '
@@ -614,6 +617,7 @@ test_lazy_prereq MKTEMP '
'
test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
+ test_when_finished "git reset --hard master >/dev/null 2>&1" &&
git checkout -b test$test_count branch1 &&
test_config mergetool.writeToTemp true &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -621,11 +625,11 @@ 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 >/dev/null" &&
git checkout order-file-side2 &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -640,10 +644,10 @@ 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 >/dev/null 2>&1" &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
@@ -667,8 +671,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
* [PATCH v4 04/14] t7610: Move setup code to the 'setup' test case.
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 3682 bytes --]
Multiple test cases depend on these hunks, so move them to the 'setup'
test case. This is a step toward making the tests more independent so
that if one test fails it doesn't cause subsequent tests to fail.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
t/t7610-mergetool.sh | 65 ++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 30 deletions(-)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 14090739f..550838a1c 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 &&
@@ -244,21 +277,7 @@ test_expect_success 'mergetool takes partial path' '
'
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" &&
+ git checkout 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 &&
@@ -607,26 +626,12 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
'
test_expect_success 'diff.orderFile configuration is honored' '
+ git checkout 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:
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH v4 02/14] rev-parse doc: use "--" in the --prefix example
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 807 bytes --]
The "--" argument avoids "ambiguous argument: unknown revision or
path not in the working tree" errors when a pathname argument refers
to a non-existent file.
Signed-off-by: Richard Hansen <hansenr@google.com>
---
Documentation/git-rev-parse.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index b6c6326cd..f1190765f 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -91,7 +91,8 @@ repository. For example:
----
prefix=$(git rev-parse --show-prefix)
cd "$(git rev-parse --show-toplevel)"
-eval "set -- $(git rev-parse --sq --prefix "$prefix" "$@")"
+eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
+shift
----
--verify::
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH v4 01/14] .mailmap: Use my personal email address as my canonical
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]
When I changed employers my work address changed from rhansen@bbn.com
to hansenr@google.com. Rather than map my old work address to my new,
map them both to my permanent personal email address. (I will still
use my work address in commits I submit so that my employer gets some
credit.)
Signed-off-by: Richard Hansen <hansenr@google.com>
---
.mailmap | 2 ++
1 file changed, 2 insertions(+)
diff --git a/.mailmap b/.mailmap
index 9cc33e925..9c87a3840 100644
--- a/.mailmap
+++ b/.mailmap
@@ -192,6 +192,8 @@ Philippe Bruhat <book@cpan.org>
Ralf Thielow <ralf.thielow@gmail.com> <ralf.thielow@googlemail.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> <ramsay@ramsay1.demon.co.uk>
René Scharfe <l.s.r@web.de> <rene.scharfe@lsrfire.ath.cx>
+Richard Hansen <rhansen@rhansen.org> <hansenr@google.com>
+Richard Hansen <rhansen@rhansen.org> <rhansen@bbn.com>
Robert Fitzsimons <robfitz@273k.net>
Robert Shearman <robertshearman@gmail.com> <rob@codeweavers.com>
Robert Zeh <robert.a.zeh@gmail.com>
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH v4 00/14] fix mergetool+rerere+subdir regression
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109054238.42599-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 1532 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 v3:
* Update rev-parse docs to use "--" in the --prefix example.
* Unconditionally run cd_to_toplevel at the beginning to simplify
the control flow. Thanks Junio!
* Test -O<pathname> when there is also a branch named <pathname>.
Richard Hansen (14):
.mailmap: Use my personal email address as my canonical
rev-parse doc: use "--" in the --prefix example
t7610: update branch names to match test number
t7610: Move setup code to the 'setup' test case.
t7610: use test_when_finished for cleanup tasks
t7610: don't rely on state from previous test
t7610: run 'git reset --hard' after each test to clean up
t7610: delete some now-unnecessary 'git reset --hard' lines
t7610: always work on a test-specific branch
t7610: don't assume the checked-out commit
t7610: spell 'git reset --hard' consistently
t7610: add test case for rerere+mergetool+subdir bug
mergetool: take the "-O" out of $orderfile
mergetool: fix running in subdir when rerere enabled
.mailmap | 2 +
Documentation/git-rev-parse.txt | 3 +-
git-mergetool.sh | 20 ++-
t/t7610-mergetool.sh | 281 ++++++++++++++++++++++++----------------
4 files changed, 186 insertions(+), 120 deletions(-)
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply
* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Junio C Hamano @ 2017-01-09 23:29 UTC (permalink / raw)
To: Richard Hansen; +Cc: git, davvid, j6t, sbeller, simon
In-Reply-To: <xmqqvatot5ob.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I wonder if it makes more sense to always move to toplevel upfront
>> and consistently use path from the toplevel, perhaps like the patch
>
> s/the patch/the attached patch/ I meant.
>
>> does. The first hunk is what you wrote but only inside MERGE_RR
>> block, and the second hunk deals with converting end-user supplied
>> paths that are relative to the original relative to the top-level.
>>
>> The tweaking of $orderfile you have in the first hunk may have to be
>> tightened mimicking the way how "eval ... --sq ... ; shift" is used
>> in the second hunk to avoid confusion in case orderfile specified by
>> the end user happens to be the same as a valid revname
>> (e.g. "master").
>
> And here is a squash-able patch to illustrate what I mean.
By the way, I didn't think this through, but how is the orderfile
that comes from the configuration file handled when it is not an
absolute path? I think it is _wrong_ to take it as relative to
where the user started the program. The -O<file> parameter from the
command line, when <file> is not absolute, should be taken as
relative to where the user _thinks_ s/he is, but when it comes from
the diff.orderfile configuration and it is not absolute, it should
be taken as relative to the top of the working tree. As we always
cd_to_top with the suggested SQUASH, it means that the orderfile
that came from the configuration does not have to be touched, while
the orderfile given via -O<file> on the command line needs
prefixing.
^ permalink raw reply
* [PATCHv8] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-09 23:16 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, peff, Stefan Beller
In-Reply-To: <CAGZ79kZVKFvijfwEfmiaahz2VYgaa=m2m-ty2tbOXh2toO_muQ@mail.gmail.com>
Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.
This patch accomplishes two things:
1. Switch assert() to die("BUG") to give a more readable message.
2. Take one of the cases where we hit a BUG and turn it into a normal
"there was something wrong with the input" message.
This assertion triggered for cases where there wasn't a programming
bug, but just bogus input. In particular, if the user asks for a
pathspec that is inside a submodule, we shouldn't assert() or
die("BUG"); we should tell the user their request is bogus.
The only reason we did not check for it, is the expensive nature
of such a check, so callers avoid setting the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
to bogus input, the expense of CPU cycles spent outweighs the user
wondering what went wrong, so run that check unconditionally before
dying with a more generic error message.
Note: There is a case (e.g. "git -C submodule add .") in which we call
strip_submodule_slash_expensive, as git-add requests it via the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to
trigger nevertheless, because the flag PATHSPEC_LITERAL was not set,
such that we executed
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
and prefixlen was not adapted (e.g. it was computed from "submodule/")
So in the die_inside_submodule_path function we also need handle paths,
that were stripped before, i.e. are the exact submodule path. This
is why the conditions in die_inside_submodule_path are slightly
different than in strip_submodule_slash_expensive.
[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html
Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This comes as a single patch again, replacing sb/pathspec-errors.
It goes directly on top of bw/pathspec-cleanup.
v8:
cleaned white spaces in commit message
removed TEST_CREATE_SUBMODULE=yes
: > instead of touch.
v7:
do not rely on "test_commit -C" being there, nor the infrastructure
to request a "good" submodule upstream. Just create a submodule outselves
to test in.
Thanks,
Stefan
pathspec.c | 35 +++++++++++++++++++++++++++++++++--
t/t6134-pathspec-in-submodule.sh | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 2 deletions(-)
create mode 100755 t/t6134-pathspec-in-submodule.sh
diff --git a/pathspec.c b/pathspec.c
index ff2509ddd1..7ababb3159 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
}
}
+static void die_inside_submodule_path(struct pathspec_item *item)
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int ce_len = ce_namelen(ce);
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+ if (item->len < ce_len ||
+ !(item->match[ce_len] == '/' || item->match[ce_len] == '\0') ||
+ memcmp(ce->name, item->match, ce_len))
+ continue;
+
+ die(_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_len, ce->name);
+ }
+}
+
/*
* Perform the initialization of a pathspec_item based on a pathspec element.
*/
@@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
}
/* sanity checks, pathspec matchers assume these are sane */
- assert(item->nowildcard_len <= item->len &&
- item->prefix <= item->len);
+ if (item->nowildcard_len > item->len ||
+ item->prefix > item->len) {
+ /*
+ * This case can be triggered by the user pointing us to a
+ * pathspec inside a submodule, which is an input error.
+ * Detect that here and complain, but fallback in the
+ * non-submodule case to a BUG, as we have no idea what
+ * would trigger that.
+ */
+ die_inside_submodule_path(item);
+ die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+ }
}
static int pathspec_item_cmp(const void *a_, const void *b_)
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..fd401ca605
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+ test_create_repo pretzel &&
+ : >pretzel/a &&
+ git -C pretzel add a &&
+ git -C pretzel commit -m "add a file" -- a &&
+ git submodule add ./pretzel sub &&
+ git commit -a -m "add submodule" &&
+ git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+ echo a >sub/a &&
+ test_must_fail git add sub/a 2>actual &&
+ test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+ test_must_fail git -C sub add . 2>actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* Re: [PATCHv7] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-09 23:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Jeff King
In-Reply-To: <xmqqeg0bu9oq.fsf@gitster.mtv.corp.google.com>
On Mon, Jan 9, 2017 at 2:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Is it only me who sees funny black rectangles in front of these four
> lines instead of blanks, by the way?
I copied this part literally from Jeff. My mail display-client (gmail
at the time)
did mangle it apparently, (it inserted some non-breaking white space characters
as well as a character that I can not really identify, as it is no
valid UTF8 and bogus
ASCII)
>> +TEST_CREATE_SUBMODULE=yes
>
> Did you mean to keep this?
No.
> because use of touch, when you do not care about the file timestamp,
> is misleading.
makes sense.
I'll reroll again.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Junio C Hamano @ 2017-01-09 23:02 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqqinpnuaxl.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I ran out of time for the day and haven't looked at individual
> patches yet. I may find other issues, but from the interdiff, that
> was the only thing that I found even worse than the previous round,
> and everything else was either the same or has improved.
... I forgot to say a few things:
* In the meantime, I think this is overall better than what we had
for the past few weeks; I'll try to replace the copy in my tree
with this round, even though this may not be the final one.
* There were a few style issues [*1*] pointed out by the
checkpatch.pl script borrowed from the kernel land.
* And finally, you wrote "This marks the count down to '3': two
more patch series after this (really tiny ones) and we have a
faster rebase -i." This is seriously an exciting news. Thanks.
I probably am still forgetting to say a few more things, but I'd
send this message out in this shape for now. Thanks.
[Footnote]
*1*
Subject: [PATCH v3 10/38] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
ERROR: do not use assignment in if condition
#118: FILE: sequencer.c:725:
+ if (buf.buf[0] != comment_line_char ||
ERROR: do not use assignment in if condition
#142: FILE: sequencer.c:749:
+ if (!(head_commit = lookup_commit_reference(head)))
ERROR: do not use assignment in if condition
#144: FILE: sequencer.c:751:
+ if (!(head_message = get_commit_buffer(head_commit, NULL)))
ERROR: do not use assignment in if condition
#167: FILE: sequencer.c:774:
+ if (!(message = get_commit_buffer(commit, NULL)))
total: 4 errors, 1 warnings, 320 lines checked
------------------------------------------------
Subject: [PATCH v3 12/38] sequencer (rebase -i): write an author-script file
ERROR: do not use assignment in if condition
#35: FILE: sequencer.c:500:
+ else if ((eol = strchr(message, '\n')))
total: 1 errors, 0 warnings, 62 lines checked
------------------------------------------------
Subject: [PATCH v3 23/38] sequencer (rebase -i): copy commit notes at end
ERROR: open brace '{' following function declarations go on the next line
#36: FILE: sequencer.c:862:
+static void flush_rewritten_pending(void) {
ERROR: do not use assignment in if condition
#41: FILE: sequencer.c:867:
+ if (strbuf_read_file(&buf, rebase_path_rewritten_pending(), 82) > 0 &&
total: 2 errors, 0 warnings, 112 lines checked
^ permalink raw reply
* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-09 22:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, davvid, j6t, sbeller, simon
In-Reply-To: <xmqqvatot5ob.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 5501 bytes --]
On 2017-01-09 14:05, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I wonder if it makes more sense to always move to toplevel upfront
>> and consistently use path from the toplevel, perhaps like the patch
>
> s/the patch/the attached patch/ I meant.
>
>> does. The first hunk is what you wrote but only inside MERGE_RR
>> block, and the second hunk deals with converting end-user supplied
>> paths that are relative to the original relative to the top-level.
>>
>> The tweaking of $orderfile you have in the first hunk may have to be
>> tightened mimicking the way how "eval ... --sq ... ; shift" is used
>> in the second hunk to avoid confusion in case orderfile specified by
>> the end user happens to be the same as a valid revname
>> (e.g. "master").
>
> And here is a squash-able patch to illustrate what I mean.
Thanks for this; I'll cook up a reroll.
I tried this approach before I emailed the v3 reroll, except I left out
the "--" argument to rev-parse. This caused the tests to fail due to
"ambiguous argument: unknown revision or path not in the working tree".
I didn't think to add the "--" because I got it in my head that "--"
shouldn't be used with --prefix because it shows up in the output, plus
the example in the rev-parse documentation doesn't use it. I'll patch
rev-parse's man page to use "--" in the example.
Thanks,
Richard
>
> I removed both of the comment blocks as the code always works with
> the worktree-relative pathname after this patch while adjusting
> end-user supplied paths from relative to original cwd. As that is
> how the core parts of the system (including the parts written in C)
> work, even though an explanation you did in the log message is
> needed to explain why the change was needed and what the change
> intended to do to readers of "git log", it is not necessary to
> explain it to the readers of the latest code, which is what the
> in-code comment is about.
>
> The single-liner addition to the test creates a branch whose name is
> the same as the specified orderfile to deliberately create a
> confusing situation. I haven't tried, but I am fairly sure that the
> test will demonstrate how broken the orderfile=$(...) in the
> original is, if you apply the test part of the attached patch,
> without the changes to git-mergetool.sh, to your version.
>
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 22f56c25a2..21f82d5b58 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -454,53 +454,34 @@ main () {
> merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
> merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
>
> - if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> + prefix=$(git rev-parse --show-prefix) || exit 1
> + cd_to_toplevel
> +
> + if test -n "$orderfile"
> then
> - # The pathnames output by the 'git rerere remaining'
> - # command below are relative to the top-level
> - # directory but the 'git diff --name-only' command
> - # further below expects the pathnames to be relative
> - # to the current working directory. Thus, we cd to
> - # the top-level directory before running 'git diff
> - # --name-only'. We change directories even earlier
> - # (before running 'git rerere remaining') in case 'git
> - # rerere remaining' is ever changed to output
> - # pathnames relative to the current working directory.
> - #
> - # Changing directories breaks a relative $orderfile
> - # pathname argument, so fix it up to be relative to
> - # the top-level directory.
> -
> - prefix=$(git rev-parse --show-prefix) || exit 1
> - cd_to_toplevel
> - if test -n "$orderfile"
> - then
> - orderfile=$(git rev-parse --prefix "$prefix" "$orderfile") || exit 1
> - fi
> + orderfile=$(
> + git rev-parse --prefix "$prefix" -- "$orderfile" |
> + sed -e 1d
> + )
> + fi
>
> + if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> + then
> set -- $(git rerere remaining)
> if test $# -eq 0
> then
> print_noop_and_exit
> fi
> + elif test $# -ge 0
> + then
> + eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> + shift
> fi
>
> - # Note: The pathnames output by 'git diff --name-only' are
> - # relative to the top-level directory, but it expects input
> - # pathnames to be relative to the current working directory.
> - # Thus:
> - # * Either cd_to_toplevel must not be run before this or all
> - # relative input pathnames must be converted to be
> - # relative to the top-level directory (or absolute).
> - # * Either cd_to_toplevel must be run after this or all
> - # relative output pathnames must be converted to be
> - # relative to the current working directory (or absolute).
> files=$(git -c core.quotePath=false \
> diff --name-only --diff-filter=U \
> ${orderfile:+"-O$orderfile"} -- "$@")
>
> - cd_to_toplevel
> -
> if test -z "$files"
> then
> print_noop_and_exit
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index dfd641d34b..180dd7057a 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -678,6 +678,11 @@ test_expect_success 'diff.orderFile configuration is honored' '
> b
> a
> EOF
> +
> + # make sure "order-file" that is ambiguous between
> + # rev and path is understood correctly.
> + git branch order-file HEAD &&
> +
> git mergetool --no-prompt --tool myecho >output &&
> git grep --no-index -h -A2 Merging: output >actual &&
> test_cmp expect actual
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply
* Re: [PATCHv7] pathspec: give better message for submodule related pathspec error
From: Junio C Hamano @ 2017-01-09 22:53 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, peff
In-Reply-To: <20170109224330.28405-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1]. The usual response from the mailing
> list is link to old discussions[2], and acknowledging the problem
> stating it is known.
>
> This patch accomplishes two things:
>
> 1. Switch assert() to die("BUG") to give a more readable message.
>
> 2. Take one of the cases where we hit a BUG and turn it into a normal
> "there was something wrong with the input" message.
>
> This assertion triggered for cases where there wasn't a programming
> bug, but just bogus input. In particular, if the user asks for a
> pathspec that is inside a submodule, we shouldn't assert() or
> die("BUG"); we should tell the user their request is bogus.
>
Is it only me who sees funny black rectangles in front of these four
lines instead of blanks, by the way?
> This comes as a single patch again, replacing sb/pathspec-errors.
> It goes directly on top of bw/pathspec-cleanup.
>
> v7:
> do not rely on "test_commit -C" being there, nor the infrastructure
> to request a "good" submodule upstream. Just create a submodule outselves
> to test in.
>
Thanks.
> diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
> new file mode 100755
> index 0000000000..d952ae2cae
> --- /dev/null
> +++ b/t/t6134-pathspec-in-submodule.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +
> +test_description='test case exclude pathspec'
> +
> +TEST_CREATE_SUBMODULE=yes
Did you mean to keep this?
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a submodule' '
> + test_create_repo pretzel &&
> + (
> + cd pretzel &&
> + touch a &&
This is better spelled as
: >a &&
because use of touch, when you do not care about the file timestamp,
is misleading.
Thanks.
^ permalink raw reply
* [PATCHv7] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-09 22:43 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, peff, Stefan Beller
Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.
This patch accomplishes two things:
1. Switch assert() to die("BUG") to give a more readable message.
2. Take one of the cases where we hit a BUG and turn it into a normal
"there was something wrong with the input" message.
This assertion triggered for cases where there wasn't a programming
bug, but just bogus input. In particular, if the user asks for a
pathspec that is inside a submodule, we shouldn't assert() or
die("BUG"); we should tell the user their request is bogus.
The only reason we did not check for it, is the expensive nature
of such a check, so callers avoid setting the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
to bogus input, the expense of CPU cycles spent outweighs the user
wondering what went wrong, so run that check unconditionally before
dying with a more generic error message.
Note: There is a case (e.g. "git -C submodule add .") in which we call
strip_submodule_slash_expensive, as git-add requests it via the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to
trigger nevertheless, because the flag PATHSPEC_LITERAL was not set,
such that we executed
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
and prefixlen was not adapted (e.g. it was computed from "submodule/")
So in the die_inside_submodule_path function we also need handle paths,
that were stripped before, i.e. are the exact submodule path. This
is why the conditions in die_inside_submodule_path are slightly
different than in strip_submodule_slash_expensive.
[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html
Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This comes as a single patch again, replacing sb/pathspec-errors.
It goes directly on top of bw/pathspec-cleanup.
v7:
do not rely on "test_commit -C" being there, nor the infrastructure
to request a "good" submodule upstream. Just create a submodule outselves
to test in.
Thanks,
Stefan
pathspec.c | 35 +++++++++++++++++++++++++++++++++--
t/t6134-pathspec-in-submodule.sh | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 73 insertions(+), 2 deletions(-)
create mode 100755 t/t6134-pathspec-in-submodule.sh
diff --git a/pathspec.c b/pathspec.c
index ff2509ddd1..7ababb3159 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
}
}
+static void die_inside_submodule_path(struct pathspec_item *item)
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int ce_len = ce_namelen(ce);
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+ if (item->len < ce_len ||
+ !(item->match[ce_len] == '/' || item->match[ce_len] == '\0') ||
+ memcmp(ce->name, item->match, ce_len))
+ continue;
+
+ die(_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_len, ce->name);
+ }
+}
+
/*
* Perform the initialization of a pathspec_item based on a pathspec element.
*/
@@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
}
/* sanity checks, pathspec matchers assume these are sane */
- assert(item->nowildcard_len <= item->len &&
- item->prefix <= item->len);
+ if (item->nowildcard_len > item->len ||
+ item->prefix > item->len) {
+ /*
+ * This case can be triggered by the user pointing us to a
+ * pathspec inside a submodule, which is an input error.
+ * Detect that here and complain, but fallback in the
+ * non-submodule case to a BUG, as we have no idea what
+ * would trigger that.
+ */
+ die_inside_submodule_path(item);
+ die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+ }
}
static int pathspec_item_cmp(const void *a_, const void *b_)
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..d952ae2cae
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+TEST_CREATE_SUBMODULE=yes
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+ test_create_repo pretzel &&
+ (
+ cd pretzel &&
+ touch a &&
+ git add a &&
+ git commit -m "add a file" -- a
+ ) &&
+ git submodule add ./pretzel sub &&
+ git commit -a -m "add submodule" &&
+ git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+ echo a >sub/a &&
+ test_must_fail git add sub/a 2>actual &&
+ test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+ test_must_fail git -C sub add . 2>actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Junio C Hamano @ 2017-01-09 22:26 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <cover.1483370556.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Changes since v2:
>
> - fixed a TRANSLATORS: comment
> ...
> - replaced a spawned `diff-tree` command by code using the diff functions
> directly
I just finished skimming the interdiff (the difference between the
result of merging the v2 into 'master' and the result of applying
this series on 'master'). I didn't cross reference it with all the
review comments that I saw on the list for the previous round, I
wouldn't have noticed even if there are issues that still haven't
been addressed. But what I saw in the interdiff were mostly good
changes.
I however think that the renaming of read_author_script() is totally
backwards from maintainability's point of view.
Both "am" and "rebase" use an external file whose name is the same
"author-script" (because they are meant to serve the same purpose),
and introduction of new code that is different from what "am" uses
to read it in v2 was bad enough. If the rename of the function in
this round really means "author-script of 'am' can hold only author
information, but that of 'rebase' can hold anything the end user
desires", it makes things even worse. If 'rebase' side extends the
file to hold other environment variables (in its own code), users
would wonder why the same extension will not be available in 'am'
side (and vice versa).
I ran out of time for the day and haven't looked at individual
patches yet. I may find other issues, but from the interdiff, that
was the only thing that I found even worse than the previous round,
and everything else was either the same or has improved.
Thanks.
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Marc Branchaud @ 2017-01-09 22:03 UTC (permalink / raw)
To: Michael Haggerty, Paul Mackerras; +Cc: git
In-Reply-To: <24a89c72-af53-0c87-d148-708e014ebe75@alum.mit.edu>
On 2016-12-22 03:15 AM, Michael Haggerty wrote:
> On 12/21/2016 08:07 PM, Marc Branchaud wrote:
>> On 2016-12-20 07:05 PM, Michael Haggerty wrote:
>>> On 12/20/2016 04:01 PM, Marc Branchaud wrote:
>>>> [...]
>>>> Please don't change the remotebgcolor default.
>>>>
>>>> Also, perhaps the default remoterefbgcolor should be
>>>> set remoterefbgcolor $headbgcolor
>>>> ?
>>>>
>>>> I say this because when I applied the series, without the last patch, I
>>>> was miffed that the remote/ref colour had changed.
>>>
>>> This is a one-time inconvenience that gitk developers will experience. I
>>> doubt that users jump backwards and forwards in gitk versions very often.
>>
>> In what way do you mean it's restricted to gitk developers?
>
> Maybe I misunderstood your objection.
>
> While developing this, I realized that the very first time your run gitk
> (i.e., when you don't already have a configuration file), it writes the
> then-current default colors into your configuration file. If you later
> switch gitk versions to a version with different default colors, the
> colors from the first-run version are preserved (unless the names of the
> variables used to hold the colors are changed).
>
> So if you would run the tip version of my branch first, then the parent
> of that version, you would continue to see the colors as modified by the
> final commit. If you then switch to the master version, the remote
> branch names go back to green (because it goes back to using
> `headbgcolor` again), but the remote prefix stays light brown. I thought
> you were unhappy about some form of this unexpected persistence problem.
> But this problem will mostly be experienced by gitk developers who jump
> back and forth between versions.
>
> A normal user probably mostly moves forward through version numbers, and
> only occasionally. Such a user, jumping from master to the tip of my
> branch (assuming they haven't customized anything), would see
>
> * local branches stay lime
> * remote branch prefixes stay pale orange (they don't change to light
> brown because the pale orange color from master is stored in their
> configuration file)
> * remote branch names change from lime to brown (because the
> `remoterefbgcolor` configuration setting didn't exist before)
>
>> Patch 12 introduces remoterefbgcolor, with a specific default value.
>> Prior to that, the "ref part" of remote refs was rendered with
>> headbgcolor. Users who changed their headbgcolor are used to seeing the
>> "ref part" of remote refs in the same color as their local heads.
>> Applying patch 12 changes the "ref part" color of remote refs, for such
>> users.
>>
>> All I'm saying is that make the remoterefbgcolor default be $headbgcolor
>> avoids this.
>
> For somebody who thinks that most people will want local and
> remote-tracking branch names to be rendered in the same color, your
> suggestion would be an improvement.
>
> But for somebody like me who thinks it is a good idea to render
> remote-tracking branch names in a different color than local branch
> names, this is a feature, not a bug. Even a user who explicitly
> configured `headbgcolor` to, say, purple, wasn't necessarily expressing
> a wish to have remote-tracking branch names rendered in purple. Until
> now they had no choice to set these colors separately!
>
> So even for somebody who configured this setting before, I think that
> having the remote-tracking branch names change color when the user
> upgrades to this version is a good thing, because it lets the user know
> that these two things can now be colored independently. If they don't
> like the new default brown color, such a user knows where to change it
> (even to make it agree with `headbgcolor` if they want that).
>
> But I understand that this is a matter of personal preference. I have
> but one "vote" :-)
I think we understand each other, and that we disagree. I don't feel
strongly about it though, so if you want to go this way that's fine by me.
Your case would be helped if the various ref colours were exposed in the
preferences dialog. Someone who upgrades to your gitk and doesn't like
the default remoterefbgcolor has to track down the right setting, close
all running gitk instances, and hand-edit ~/.gitk.
But I think improving gitk's colour-preferences dialog is a bit beyond
the scope of your topic...
M.
^ permalink raw reply
* Re: [PATCH 3/3] blame: output porcelain "previous" header for each file
From: Junio C Hamano @ 2017-01-09 21:57 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> /*
> + * 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');
> }
Yes, "previous" is not per commit but per "origin", and "origin" is
(commit, path) pair, so allowing this helper to examine the entire
suspect instead of just suspect->path makes sense.
Thanks for a fix.
^ permalink raw reply
* Re: Test failures when Git is built with libpcre and grep is built without it
From: Jeff King @ 2017-01-09 21:33 UTC (permalink / raw)
To: Andreas Schwab; +Cc: A. Wilcox, git
In-Reply-To: <871swcjsd3.fsf@linux-m68k.org>
On Mon, Jan 09, 2017 at 02:05:44PM +0100, Andreas Schwab wrote:
> On Jan 09 2017, "A. Wilcox" <awilfox@adelielinux.org> wrote:
>
> > Interestingly enough, you seem to be right. The failure is very
> > bizarre and has nothing to do with system /bin/grep:
> >
> > test_must_fail: command succeeded: git grep -G -F -P -E a\x{2b}b\x{2a}c ab
> > not ok 142 - grep -G -F -P -E pattern
> > #
> > # >empty &&
> > # test_must_fail git grep -G -F -P -E "a\x{2b}b\x{2a}c"
> > ab >actual &&
> > # test_cmp empty actual
> > #
> >
> > However:
> >
> > elaine trash directory.t7810-grep # git grep -G -F -P -E
> > a\x{2b}b\x{2a}c ab >actual
>
> You need to quote the regexp argument, see the line starting with
> "test_must_fail" above.
Oh, duh. I checked that the line in the test was quoted, and didn't
notice that only the unquoted version he ran later complained. Thanks
for noticing, as that explains the mystery, I think.
The problem is that we are expecting the regex "\x{2b}" to complain in
regcomp() (as an ERE), but it doesn't. And that probably _is_ related to
musl, which is providing the libc regex (I know this looks like a pcre
test, but it's checking that "-P -E" overrides the pcre option with
"-E").
I'm not sure if musl is wrong for failing to complain about a bogus
regex. Generally making something that would break into something that
works is an OK way to extend the standard. So our test is at fault for
assuming that the regex will fail. I guess we'd need to find some more
exotic syntax that pcre supports, but that ERE doesn't. Maybe "(?:)" or
something.
-Peff
^ permalink raw reply
* Re: Rebasing a branch with merges
From: Philip Oakley @ 2017-01-09 21:03 UTC (permalink / raw)
To: Robert Dailey; +Cc: Git, Johannes Schindelin
In-Reply-To: <CAHd499CuZBXF0AddP7AfqtzvA8rBArwqtktRmNdD-kwGUCruLg@mail.gmail.com>
From: "Robert Dailey" <rcdailey.lists@gmail.com>
> On Fri, Jan 6, 2017 at 3:28 PM, Philip Oakley <philipoakley@iee.org>
> wrote:
>> From: "Robert Dailey" <rcdailey.lists@gmail.com>
>>>
>>> Here's the scenario:
>>>
>>> I create a topic branch so one other developer and myself can work on
>>> a feature that takes 2 weeks to complete. During that 2 week period,
>>> changes are occurring on master that I need in my topic branch. Since
>>> I have a collaborator on the branch, I opt for merges instead of
>>> rebase.
>>>
>>> Each day I merge from master to the topic branch, which changes code
>>> I'm actively working in and requires semantic changes (functions
>>> renamed, moved, etc).
>>>
>>> Once I'm ready to merge the topic branch back into master, I have two
>>> options (bearing in mind the goal is to keep history as clean as
>>> possible. Furthermore this implies that the constant merging into
>>> topic from master has made the topic branch look unwieldy and
>>> difficult to audit):
>>
>>
>> a broader question zero;
>> 0. Is the merge always clean? Do you always do a preparatory fixup! to
>> ensure that the merge will be clean?
>>
>> Ensuring that the merge will be clean should greatly simplify your
>> decision
>> about process.
>
> I don't understand what you're asking. How would I do a fixup with
> merges? Can you explain a bit? Normally the only time I use fixup! or
> squash! is for local changes prior to pushing.
>
I was using 'fixup!' figuratively, rather than literally. If I understand
the general setup correctly, what you want is that if one does a trial
merge, then it must merge cleanly, compile and pass all tests, or if it
doesn't, you would need to add that extra commit to ensure that you get that
clean merge.
The alternative is that there is extra work to be done after the 'merge'
(which may be a management choice) to then get your nice code ready for
wider release - it may even be someone elses job to do 'integration'.
Either way, at least if you rationalise the clean end point it becomes
easier to discuss the start point. If both ends are 'dirty' it is an open
argument...
>>> 1. Do a squash merge, which keeps history clean but we lose context
>>> for the important bits (the commits representing units of work that
>>> contribute to the topic itself).
>>>
>>> 2. Do a final rebase prior to merging.
>>>
>>> #2 doesn't seem to be possible due to patch ordering. For example, if
>>> I have real commits after merge commits that depend on those changes
>>> from master being present as a base at that point in time, the rebase
>>> will cause the patch before it to no longer include those changes from
>>> master.
>>
>>
>> How much of the historic fixups to cover changes on master do you want to
>> keep visible? i.e. how many fork-points are truly needed (a. by you, b.
>> by
>> the project - personal knowledge vs corporate knowledge).?
>
>
> Again, I do not understand. Maybe the first question you asked needs
> to be understood before I can answer this one. Sorry for the trouble.
>
In some scenarios it is important to keep the record of the exact start
point (date and place) to formally justify the work performed (time eleapsed
etc). Now given the other part of your scenario of a moving api / function
naming etc, you now have two reference points - the original start point,
and the release point of the new api/names.
So at that point you probably want to merge in those changes (one way or
another), which gives you a choice of a true merge (option 1), or adding a
patch to cherry pick the bulk changes on master (option2), or just the api
changes (if you can isolate them) (option3). No option is inherently
better - it will depend on local needs.
From a clean code perspective, it's nicest if you can simply rebase your
code, but that isn't alway possible. The hard bit is to be clear about the
local issues, and which ones are immutable. These will decide your choice.
Philip
^ permalink raw reply
* Re: RFC: Enable delayed responses to Git clean/smudge filter requests
From: Stefan Beller @ 2017-01-09 20:44 UTC (permalink / raw)
To: Lars Schneider; +Cc: Git Mailing List
In-Reply-To: <D10F7C47-14E8-465B-8B7A-A09A1B28A39F@gmail.com>
On Mon, Nov 14, 2016 at 1:09 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> Hi,
>
> Git always performs a clean/smudge filter on files in sequential order.
> Sometimes a filter operation can take a noticeable amount of time.
> This blocks the entire Git process.
>
> I would like to give a filter process the possibility to answer Git with
> "I got your request, I am processing it, ask me for the result later!".
>
> I see the following way to realize this:
>
> In unpack-trees.c:check_updates() [1] we loop through the cache
> entries and "ask me later" could be an acceptable return value of the
> checkout_entry() call. The loop could run until all entries returned
> success or error.
Late to this thread, but here is an answer nevertheless.
I am currently working on getting submodules working
for working tree modifying commands (prominently checkout, but
also read-tree -u and any other caller that uses the code in
unpack-trees.)
Once the submodules are supported and used, I anticipate that
putting the files in the working tree on disk will become a bottle neck,
i.e. the checkout taking way too long for an oversized project.
So in the future we have to do something to make checkout fast
again, which IMHO is threading. My current vision is to have checkout
automatically choose a number of threads based on expected workload,
c.f. preload-index.c, line 18-25.
> The filter machinery is triggered in various other places in Git and
> all places that want to support "ask me later" would need to be patched
> accordingly.
I think this makes sense, even in a threaded git-checkout.
I assume this idea is implemented before threading hits checkout,
so a question on the design:
Who determines the workload that is acceptable?
From reading this email, it seems to be solely the filter that uses
as many threads/processes as it thinks is ok.
Would it be possible to enhance the protocol further to have
Git also mingle with the workload, i.e. tell the filter it is
allowed to use up (N-M) threads, as it itself already uses
M out of N configured threads?
(I do not want to discuss the details here, but only if such a thing
is viable with this approach as well)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv6 1/2] submodule tests: don't use itself as a submodule
From: Stefan Beller @ 2017-01-09 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <xmqqh959ynb4.fsf@gitster.mtv.corp.google.com>
On Sun, Jan 8, 2017 at 6:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>
> Yuck.
>
> I find it doubtful that it is a good idea to create two submodule
> repositories by merely dot-including the test-lib.sh; I find it
> doubly doubtful that it is a good idea to make test_create_repo
> pay attention to the special variable to implement that.
>
> I am OK with a solution where callers that set TEST_CREATE_SUBMODULE
> variable in this patch to instead have an explicit call
>
> test_create_repo --submodule pretzel
>
> That would be a lot more obvious.
agreed.
>
> The primary reason why I hate the implementation in this patch is
> that it is very easy for a test that says TEST_CREATE_SUBMODULE
> upfront, only to get the initial test repository (which everybody
> else gets) with two test submodules, to later gain a test that wants
> to use a separate repository and call "test_create_repo". It will
> always get the pretzel submodules, which may or may not match what
> the test writer who adds a new test needs.
I agree. At the time of writing the patch series, I was anticipating writing
way more submodule tests, but now I have these future tests integrated into
lib-submodule-update.sh.
>
>> 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.
>
> But isn't the point of this change that use of ./. cannot be
> mimicking any real-world use, hence pointless for the purpose of
> really testing the components of the system? If "we craft
> deliberately for both inside and outside use" indeed _IS_ a good
> thing, then perhaps use of ./. has practical real-world use---if
> not, wouldn't we want to fix the scripts that include the
> lib-submodule-repo helper not to test such an unrealistic layout?
>
Makes sense; I tried to fix it up to avoid the ./. clone in
create_lib_submodule_repo, but the issue is there are too many
implicit assumption of these two repos, such that a faithful conversion
would just duplicate code for the submodule, (e.g. create the same
amount of commits, containing the same diffs, etc.), which then
can be argued to just slow down the test suite as the clone from ./.
is actually reducing the needed work by a factor of 2.
^ permalink raw reply
* Re: [PATCHv6 2/2] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-09 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <xmqqbmvhyn1m.fsf@gitster.mtv.corp.google.com>
On Sun, Jan 8, 2017 at 6:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> For future reference, do not bury a useful fix behind unproven new
> things. The main purpose of this two-patch series is this change,
> and it does not have to wait for the change to allow test_commit to
> notice "-C" you have in another series.
noted.
The bug fixed here doesn't need to be fast-tracked IMO, because
of its history. (It's well documented on the web already)
I think this series can wait until the
"test_commit -C" is available.
In a reroll I'll drop the dependency then.
^ permalink raw reply
* Re: [PATCH] Makefile: POSIX windres
From: Johannes Sixt @ 2017-01-09 19:54 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano; +Cc: Steven Penny, git
In-Reply-To: <alpine.DEB.2.20.1701090903500.3469@virtualbox>
Am 09.01.2017 um 09:09 schrieb Johannes Schindelin:
> Hi Junio,
>
> On Sun, 8 Jan 2017, Junio C Hamano wrote:
>
>> Steven Penny <svnpenn@gmail.com> writes:
>>
>>> When environment variable POSIXLY_CORRECT is set, the "input -o
>>> output" syntax is not supported.
>>>
>>> http://cygwin.com/ml/cygwin/2017-01/msg00036.html
>>>
>>> Signed-off-by: Steven Penny <svnpenn@gmail.com>
>>> ---
>>
>> Who other than cygwin build uses this target? Git for Windows?
>
> Yes, Git for Windows uses this target, as did msysGit (and I suspect
> Hannes' setup).
The patch works for me, too.
-- Hannes
^ permalink raw reply
* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Johannes Sixt @ 2017-01-09 19:53 UTC (permalink / raw)
To: Junio C Hamano, Richard Hansen; +Cc: git, davvid, sbeller, simon
In-Reply-To: <xmqqvatot5ob.fsf@gitster.mtv.corp.google.com>
Am 09.01.2017 um 20:05 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I wonder if it makes more sense to always move to toplevel upfront
>> and consistently use path from the toplevel, perhaps like the patch
>
> s/the patch/the attached patch/ I meant.
>
>> does. The first hunk is what you wrote but only inside MERGE_RR
>> block, and the second hunk deals with converting end-user supplied
>> paths that are relative to the original relative to the top-level.
>>
>> The tweaking of $orderfile you have in the first hunk may have to be
>> tightened mimicking the way how "eval ... --sq ... ; shift" is used
>> in the second hunk to avoid confusion in case orderfile specified by
>> the end user happens to be the same as a valid revname
>> (e.g. "master").
>
> And here is a squash-able patch to illustrate what I mean.
>
> I removed both of the comment blocks as the code always works with
> the worktree-relative pathname after this patch while adjusting
> end-user supplied paths from relative to original cwd. As that is
> how the core parts of the system (including the parts written in C)
> work, even though an explanation you did in the log message is
> needed to explain why the change was needed and what the change
> intended to do to readers of "git log", it is not necessary to
> explain it to the readers of the latest code, which is what the
> in-code comment is about.
>
> The single-liner addition to the test creates a branch whose name is
> the same as the specified orderfile to deliberately create a
> confusing situation. I haven't tried, but I am fairly sure that the
> test will demonstrate how broken the orderfile=$(...) in the
> original is, if you apply the test part of the attached patch,
> without the changes to git-mergetool.sh, to your version.
>
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 22f56c25a2..21f82d5b58 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -454,53 +454,34 @@ main () {
> merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
> merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
>
> - if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> + prefix=$(git rev-parse --show-prefix) || exit 1
> + cd_to_toplevel
> +
> + if test -n "$orderfile"
> then
> - # The pathnames output by the 'git rerere remaining'
> - # command below are relative to the top-level
> - # directory but the 'git diff --name-only' command
> - # further below expects the pathnames to be relative
> - # to the current working directory. Thus, we cd to
> - # the top-level directory before running 'git diff
> - # --name-only'. We change directories even earlier
> - # (before running 'git rerere remaining') in case 'git
> - # rerere remaining' is ever changed to output
> - # pathnames relative to the current working directory.
> - #
> - # Changing directories breaks a relative $orderfile
> - # pathname argument, so fix it up to be relative to
> - # the top-level directory.
> -
> - prefix=$(git rev-parse --show-prefix) || exit 1
> - cd_to_toplevel
> - if test -n "$orderfile"
> - then
> - orderfile=$(git rev-parse --prefix "$prefix" "$orderfile") || exit 1
> - fi
> + orderfile=$(
> + git rev-parse --prefix "$prefix" -- "$orderfile" |
> + sed -e 1d
> + )
> + fi
>
> + if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> + then
> set -- $(git rerere remaining)
> if test $# -eq 0
> then
> print_noop_and_exit
> fi
> + elif test $# -ge 0
> + then
> + eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> + shift
> fi
>
> - # Note: The pathnames output by 'git diff --name-only' are
> - # relative to the top-level directory, but it expects input
> - # pathnames to be relative to the current working directory.
> - # Thus:
> - # * Either cd_to_toplevel must not be run before this or all
> - # relative input pathnames must be converted to be
> - # relative to the top-level directory (or absolute).
> - # * Either cd_to_toplevel must be run after this or all
> - # relative output pathnames must be converted to be
> - # relative to the current working directory (or absolute).
> files=$(git -c core.quotePath=false \
> diff --name-only --diff-filter=U \
> ${orderfile:+"-O$orderfile"} -- "$@")
>
> - cd_to_toplevel
> -
> if test -z "$files"
> then
> print_noop_and_exit
The control flow after this patch looks much more like what I had in
mind. Thanks.
-- Hannes
^ permalink raw reply
* [PATCHv2 2/5] unpack-trees: remove unneeded continue
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170109194621.17013-1-sbeller@google.com>
The continue is the last statement in the loop, so not needed.
This situation arose in 700e66d66 (2010-07-30, unpack-trees: let
read-tree -u remove index entries outside sparse area) when statements
after the continue were removed.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 8e6768f283..d443decb23 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -253,7 +253,6 @@ static int check_updates(struct unpack_trees_options *o)
display_progress(progress, ++cnt);
if (o->update && !o->dry_run)
unlink_entry(ce);
- continue;
}
}
remove_marked_cache_entries(index);
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* [PATCHv2 1/5] unpack-trees: move checkout state into check_updates
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170109194621.17013-1-sbeller@google.com>
The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.
The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
unpack-trees.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index ea6bdd20e0..8e6768f283 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,14 +218,19 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o,
- const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ int i, errs = 0;
+
struct progress *progress = NULL;
struct index_state *index = &o->result;
- int i;
- int errs = 0;
+ struct checkout state = CHECKOUT_INIT;
+
+ state.force = 1;
+ state.quiet = 1;
+ state.refresh_cache = 1;
+ state.istate = index;
if (o->update && o->verbose_update) {
for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -240,7 +245,7 @@ static int check_updates(struct unpack_trees_options *o,
}
if (o->update)
- git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
+ git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
@@ -251,7 +256,7 @@ static int check_updates(struct unpack_trees_options *o,
continue;
}
}
- remove_marked_cache_entries(&o->result);
+ remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
@@ -264,7 +269,7 @@ static int check_updates(struct unpack_trees_options *o,
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
- errs |= checkout_entry(ce, state, NULL);
+ errs |= checkout_entry(ce, &state, NULL);
}
}
}
@@ -1094,14 +1099,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
int i, ret;
static struct cache_entry *dfc;
struct exclude_list el;
- struct checkout state = CHECKOUT_INIT;
if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
- state.force = 1;
- state.quiet = 1;
- state.refresh_cache = 1;
- state.istate = &o->result;
memset(&el, 0, sizeof(el));
if (!core_apply_sparse_checkout || !o->update)
@@ -1238,7 +1238,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
}
o->src_index = NULL;
- ret = check_updates(o, &state) ? (-2) : 0;
+ ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
if (!o->result.cache_tree)
--
2.11.0.rc2.30.g7c4be45.dirty
^ permalink raw reply related
* [PATCHv2 3/5] unpack-trees: factor progress setup out of check_updates
From: Stefan Beller @ 2017-01-09 19:46 UTC (permalink / raw)
To: gitster, peff, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170109194621.17013-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index d443decb23..b564024472 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,9 +218,27 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o)
+static struct progress *get_progress(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ struct index_state *index = &o->result;
+
+ if (!o->update || !o->verbose_update)
+ return NULL;
+
+ for (; cnt < index->cache_nr; cnt++) {
+ const struct cache_entry *ce = index->cache[cnt];
+ if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
+ total++;
+ }
+
+ return start_progress_delay(_("Checking out files"),
+ total, 50, 1);
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+ unsigned cnt = 0;
int i, errs = 0;
struct progress *progress = NULL;
@@ -232,17 +250,7 @@ static int check_updates(struct unpack_trees_options *o)
state.refresh_cache = 1;
state.istate = index;
- if (o->update && o->verbose_update) {
- for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
- const struct cache_entry *ce = index->cache[cnt];
- if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
- total++;
- }
-
- progress = start_progress_delay(_("Checking out files"),
- total, 50, 1);
- cnt = 0;
- }
+ progress = get_progress(o);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
--
2.11.0.rc2.30.g7c4be45.dirty
^ 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