[PATCH 01/29] t7508: use test_when_finished() instead of managing exit code manually

[Eric Sunshine]

This test manages its own exit code in order to perform a cleanup action
unconditionally, whether the test succeeds or fails overall. In doing
so, it intentionally breaks the &&-chain. Such manual exit code
management to ensure cleanup predates the invention of
test_when_finished().

An upcoming change will teach --chain-lint to detect &&-chain breakage
inside subshells, so this manual exit code management with intentional
&&-chain breakage will run afoul of --chain-lint. Therefore, replace
the manual exit code handling with test_when_finished() and a normal
&&-chain. While at it, drop the now-unnecessary subshell.

Signed-off-by: Eric Sunshine 
---

Notes:
This series is built atop 'master'. If the series is queued there,
this patch is needed to avoid test-suite breakage. However, the
issue fixed by this patch is already also fixed by
'jc/clean-after-sanity-tests' in 'next' (although, that patch
doesn't bother dropping the now-unnecessary subshell, like this
patch does.)

 t/t7508-status.sh | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 18a40257fb..67bf4393bb 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1099,18 +1099,14 @@ EOF
 '
 
 test_expect_success POSIXPERM,SANITY 'status succeeds in a read-only 
repository' '
-   (
-   chmod a-w .git &&
-   # make dir1/tracked stat-dirty
-   >dir1/tracked1 && mv -f dir1/tracked1 dir1/tracked &&
-   git status -s >output &&
-   ! grep dir1/tracked output &&
-   # make sure "status" succeeded without writing index out
-   git diff-files | grep dir1/tracked
-   )
-   status=$?
-   chmod 775 .git
-   (exit $status)
+   chmod a-w .git &&
+   test_when_finished "chmod 775 .git" &&
+   # make dir1/tracked stat-dirty
+   >dir1/tracked1 && mv -f dir1/tracked1 dir1/tracked &&
+   git status -s >output &&
+   ! grep dir1/tracked output &&
+   # make sure "status" succeeded without writing index out
+   git diff-files | grep dir1/tracked
 '
 
 (cd sm && echo > bar && git add bar && git commit -q -m 'Add bar') && git add 
sm
-- 
2.18.0.419.gfe4b301394

git@vger.kernel.org

[Eric Sunshine]

This test intentionally breaks the &&-chain after "unset HOME" since it
doesn't know if 'unset' will succeed or fail. However, an upcoming
change will teach --chain-lint to detect &&-chain breakage inside
subshells, and it will catch this broken &&-chain. Instead, use
sane_unset() which can be safely linked into the &&-chain.

Signed-off-by: Eric Sunshine 
---
 t/t1300-config.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 03c223708e..24706ba412 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -888,7 +888,7 @@ EOF
 
 test_expect_success !MINGW 'get --path copes with unset $HOME' '
(
-   unset HOME;
+   sane_unset HOME &&
test_must_fail git config --get --path path.home \
>result 2>msg &&
git config --get --path path.normal >>result &&
-- 
2.18.0.419.gfe4b301394

[PATCH 02/29] t0001: use "{...}" block around "||" expression rather than subshell

[Eric Sunshine]

This test uses (... || true) as a shorthand for an if-then-else
statement. The subshell prevents it from breaking the top-level
&&-chain.

However, an upcoming change will teach --chain-lint to check the
&&-chain inside subshells. Although it specially recognizes and allows
(... || git ...) and (... || test*), it intentionally avoids treating
(... || true) specially since such a construct typically can be
expressed more naturally with test_might_fail() and a normal &&-chain.

This case is special, though, since the invoked command, 'setfacl',
might not even exist (indeed, MacOS has no such command) which is not a
valid use-case for test_might_fail(). Sidestep the issue by wrapping the
"||" expression in a "{...}" block instead of a subshell since "{...}"
blocks are not checked for &&-chain breakage.

Signed-off-by: Eric Sunshine 
---
 t/t0001-init.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index c413bff9cf..fa44a55a5b 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -260,7 +260,7 @@ test_expect_success POSIXPERM 'init creates a new deep 
directory (umask vs. shar
# the repository itself should follow "shared"
mkdir newdir &&
# Remove a default ACL if possible.
-   (setfacl -k newdir 2>/dev/null || true) &&
+   { setfacl -k newdir 2>/dev/null || true; } &&
umask 002 &&
git init --bare --shared=0660 newdir/a/b/c &&
test_path_is_dir newdir/a/b/c/refs &&
-- 
2.18.0.419.gfe4b301394

[PATCH 00/29] t: detect and fix broken &&-chains in subshells

[Eric Sunshine]

The --chain-lint[1] option detects breakage in the top-level &&-chain of
tests. This series undertakes the more complex task of teaching it to
also detect &&-chain breakage within subshells. See patch 29/29 for the
gory details of how that's done.

The series is built atop 'master', however, it has a conflict with an
in-flight topic. In particular, patch 1/29 rewrites a test in t7508 in
'master' to avoid &&-chain breakage. 'jc/clean-after-sanity-tests' in
'next' performs pretty much the same rewrite. If this series is queued
atop 'master', then it needs patch 1/29; if it is queued somewhere above
'jc/clean-after-sanity-tests', then 1/29 can be dropped.

Aside from identifying a rather significant number of &&-chain breaks,
repairing those broken chains uncovered genuine bugs in several tests
which were hidden by missing &&-chain links. Those bugs are also fixed
by this series. I would appreciate if the following people would
double-check my fixes:

Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update"
Jonathan Tan - 10/29 "t9001"
Elijah Newren - 6/29 "t6036"

In order to keep the noise level down and ease review burden, please
take into account that I largely avoided modernizations and cleanups,
and limited changes to merely adding "&&" even when some other
transformation would have made the fix nicer overall. (For example, I
resisted the urge to replace a series of 'echo' statements in a subshell
with a simple here-doc.)

Finally, although all simple &&-chain fixes originally inhabited a
single patch, they were split out into several patches to avoid hitting
vger.kernel.org's message size limit. However, when queuing, all patches
titled "t: fix broken &&-chains in subshells" could be squashed into
a single patch titled "t: fix broken &&-chains in subshells".

[1]: https://public-inbox.org/git/20150320100429.ga17...@peff.net/

Eric Sunshine (29):
  t7508: use test_when_finished() instead of managing exit code manually
  t0001: use "{...}" block around "||" expression rather than subshell
  t1300: use sane_unset() to avoid breaking &&-chain
  t3303: use standard here-doc tag "EOF" to avoid fooling --chain-lint
  t5505: modernize and simplify hard-to-digest test
  t6036: fix broken "merge fails but has appropriate contents" tests
  t7201: drop pointless "exit 0" at end of subshell
  t7400: fix broken "submodule add/reconfigure --force" test
  t7810: use test_expect_code() instead of hand-rolled comparison
  t9001: fix broken "invoke hook" test
  t9104: use "{...}" block around "||" expression rather than subshell
  t9401: drop unnecessary nested subshell
  t/lib-submodule-update: fix broken "replace submodule must-fail" test
  t: drop subshell with missing &&-chain in favor of simpler construct
  t: drop unnecessary terminating semicolons in subshell
  t: use test_might_fail() instead of manipulating exit code manually
  t: use test_must_fail() instead of checking exit code manually
  t-t0999: fix broken &&-chains in subshells
  t1000-t1999: fix broken &&-chains in subshells
  t2000-t2999: fix broken &&-chains in subshells
  t3000-t3999: fix broken &&-chains in subshells
  t3030: fix broken &&-chains in subshells
  t4000-t4999: fix broken &&-chains in subshells
  t5000-t5999: fix broken &&-chains in subshells
  t6000-t6999: fix broken &&-chains in subshells
  t7000-t7999: fix broken &&-chains in subshells
  t9000-t: fix broken &&-chains in subshells
  t9119: fix broken &&-chains in subshells
  t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

 t/lib-submodule-update.sh |  10 +-
 t/t-basic.sh  |   2 +-
 t/t0001-init.sh   |   2 +-
 t/t0003-attributes.sh |  24 +-
 t/t0021-conversion.sh |   4 +-
 t/t0090-cache-tree.sh |   2 +-
 t/t1004-read-tree-m-u-wf.sh   |   8 +-
 t/t1005-read-tree-reset.sh|  10 +-
 t/t1008-read-tree-overlay.sh  |   2 +-
 t/t1020-subdirectory.sh   |   2 +-
 t/t1050-large.sh  |   6 +-
 t/t1300-config.sh |   2 +-
 t/t1411-reflog-show.sh|   6 +-
 t/t1507-rev-parse-upstream.sh |   6 +-
 t/t1512-rev-parse-disambiguation.sh   |   6 +-
 t/t1700-split-index.sh|   2 +-
 t/t2016-checkout-patch.sh |  24 +-
 t/t2103-update-index-ignore-missing.sh|   2 +-
 t/t2202-add-addremove.sh  |  14 +-
 t/t3000-ls-files-others.sh|   2 +-
 t/t3006-ls-files-long.sh  |   2 +-
 t/t3008-ls-files-lazy-init-name-hash.sh   |   8 +-
 t/t3030-merge-recursive.sh| 340 +-
 t/t3050-subprojects-fetch.sh  |   8 +-
 t/t3102-ls-tree-wildcards.sh  |   2 +-
 t/t3301-notes.sh  

[PATCH 04/29] t3303: use standard here-doc tag "EOF" to avoid fooling --chain-lint

[Eric Sunshine]

An upcoming change will teach --chain-lint to detect &&-chain breakage
inside subshells. The check works by performing textual transformations
on the test to link the subshell body directly into the parent's
&&-chain. Special care is taken with the final statement in a
subshell. For instance:

statement1 &&
(
statement2
) &&
statement3

is transformed to:

statement1 &&
statement2 &&
statement3

Notice that "statement2" gains a "&&".

Special care is is taken with here-docs since "&&" needs to be added to
the "<
---
 t/t3303-notes-subtrees.sh | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3303-notes-subtrees.sh b/t/t3303-notes-subtrees.sh
index 704aee81ef..e353faa50b 100755
--- a/t/t3303-notes-subtrees.sh
+++ b/t/t3303-notes-subtrees.sh
@@ -39,7 +39,7 @@ test_expect_success "setup: create $number_of_commits 
commits" '
while [ $nr -lt $number_of_commits ]; do
nr=$(($nr+1)) &&
test_tick &&
-   cat < $GIT_COMMITTER_DATE
 data < $GIT_COMMITTER_DATE
 data <

[PATCH 15/29] t: drop unnecessary terminating semicolons in subshell

[Eric Sunshine]

An upcoming change will teach --chain-lint to check the &&-chain inside
subshells. The semicolons after the final commands in these subshells
will trip up --chain-lint since they break the &&-chain. Since those
semicolons are unnecessary, just drop them.

Signed-off-by: Eric Sunshine 
---
 t/t3102-ls-tree-wildcards.sh | 2 +-
 t/t4010-diff-pathspec.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3102-ls-tree-wildcards.sh b/t/t3102-ls-tree-wildcards.sh
index e804377f1c..1e16c6b8ea 100755
--- a/t/t3102-ls-tree-wildcards.sh
+++ b/t/t3102-ls-tree-wildcards.sh
@@ -23,7 +23,7 @@ test_expect_success 'ls-tree outside prefix' '
cat >expect <<-EOF &&
100644 blob $EMPTY_BLOB ../a[a]/three
EOF
-   ( cd aa && git ls-tree -r HEAD "../a[a]"; ) >actual &&
+   ( cd aa && git ls-tree -r HEAD "../a[a]" ) >actual &&
test_cmp expect actual
 '
 
diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index 35b35a81c8..b7f25071cf 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -111,10 +111,10 @@ test_expect_success 'diff-tree -r with wildcard' '
 test_expect_success 'setup submodules' '
test_tick &&
git init submod &&
-   ( cd submod && test_commit first; ) &&
+   ( cd submod && test_commit first ) &&
git add submod &&
git commit -m first &&
-   ( cd submod && test_commit second; ) &&
+   ( cd submod && test_commit second ) &&
git add submod &&
git commit -m second
 '
-- 
2.18.0.419.gfe4b301394

[PATCH 20/29] t2000-t2999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t2103-update-index-ignore-missing.sh |  2 +-
 t/t2202-add-addremove.sh   | 14 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t2103-update-index-ignore-missing.sh 
b/t/t2103-update-index-ignore-missing.sh
index 332694e7d3..0114f05228 100755
--- a/t/t2103-update-index-ignore-missing.sh
+++ b/t/t2103-update-index-ignore-missing.sh
@@ -32,7 +32,7 @@ test_expect_success basics '
test_create_repo xyzzy &&
cd xyzzy &&
>file &&
-   git add file
+   git add file &&
git commit -m "sub initial"
) &&
git add xyzzy &&
diff --git a/t/t2202-add-addremove.sh b/t/t2202-add-addremove.sh
index 6a5a3166b1..17744e8c57 100755
--- a/t/t2202-add-addremove.sh
+++ b/t/t2202-add-addremove.sh
@@ -6,12 +6,12 @@ test_description='git add --all'
 
 test_expect_success setup '
(
-   echo .gitignore
+   echo .gitignore &&
echo will-remove
) >expect &&
(
-   echo actual
-   echo expect
+   echo actual &&
+   echo expect &&
echo ignored
) >.gitignore &&
git --literal-pathspecs add --all &&
@@ -25,10 +25,10 @@ test_expect_success setup '
 
 test_expect_success 'git add --all' '
(
-   echo .gitignore
-   echo not-ignored
-   echo "M .gitignore"
-   echo "A not-ignored"
+   echo .gitignore &&
+   echo not-ignored &&
+   echo "M .gitignore" &&
+   echo "A not-ignored" &&
echo "D will-remove"
) >expect &&
>ignored &&
-- 
2.18.0.419.gfe4b301394

[PATCH 14/29] t: drop subshell with missing &&-chain in favor of simpler construct

[Eric Sunshine]

These tests employ a noisy subshell (with missing &&-chain) to feed
input into Git commands:

(echo a; echo b; echo c) | git some-command ...

Drop the subshell in favor of a simple 'printf':

printf "%s\n" a b c | git some-command ...

Signed-off-by: Eric Sunshine 
---
 t/t0090-cache-tree.sh |  2 +-
 t/t2016-checkout-patch.sh | 24 ++--
 t/t3404-rebase-interactive.sh |  6 ++---
 t/t3701-add-interactive.sh| 16 +++---
 t/t3904-stash-patch.sh|  8 +++
 t/t7105-reset-patch.sh| 12 +-
 t/t7301-clean-interactive.sh  | 41 +--
 t/t7501-commit.sh |  4 ++--
 t/t7610-mergetool.sh  |  8 +++
 9 files changed, 60 insertions(+), 61 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 0c61268fd2..f86cc76c9d 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -156,7 +156,7 @@ test_expect_success PERL 'commit --interactive gives 
cache-tree on partial commi
return 44;
}
EOT
-   (echo p; echo 1; echo; echo s; echo n; echo y; echo q) |
+   printf "%s\n" p 1 "" s n y q |
git commit --interactive -m foo &&
test_cache_tree
 '
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 9cd0ac4ba3..4dcad0f4a2 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -20,33 +20,33 @@ test_expect_success PERL 'setup' '
 
 test_expect_success PERL 'saying "n" does nothing' '
set_and_save_state dir/foo work head &&
-   (echo n; echo n) | git checkout -p &&
+   printf "%s\n" n n | git checkout -p &&
verify_saved_state bar &&
verify_saved_state dir/foo
 '
 
 test_expect_success PERL 'git checkout -p' '
-   (echo n; echo y) | git checkout -p &&
+   printf "%s\n" n y | git checkout -p &&
verify_saved_state bar &&
verify_state dir/foo head head
 '
 
 test_expect_success PERL 'git checkout -p with staged changes' '
set_state dir/foo work index &&
-   (echo n; echo y) | git checkout -p &&
+   printf "%s\n" n y | git checkout -p &&
verify_saved_state bar &&
verify_state dir/foo index index
 '
 
 test_expect_success PERL 'git checkout -p HEAD with NO staged changes: abort' '
set_and_save_state dir/foo work head &&
-   (echo n; echo y; echo n) | git checkout -p HEAD &&
+   printf "%s\n" n y n | git checkout -p HEAD &&
verify_saved_state bar &&
verify_saved_state dir/foo
 '
 
 test_expect_success PERL 'git checkout -p HEAD with NO staged changes: apply' '
-   (echo n; echo y; echo y) | git checkout -p HEAD &&
+   printf "%s\n" n y y | git checkout -p HEAD &&
verify_saved_state bar &&
verify_state dir/foo head head
 '
@@ -54,14 +54,14 @@ test_expect_success PERL 'git checkout -p HEAD with NO 
staged changes: apply' '
 test_expect_success PERL 'git checkout -p HEAD with change already staged' '
set_state dir/foo index index &&
# the third n is to get out in case it mistakenly does not apply
-   (echo n; echo y; echo n) | git checkout -p HEAD &&
+   printf "%s\n" n y n | git checkout -p HEAD &&
verify_saved_state bar &&
verify_state dir/foo head head
 '
 
 test_expect_success PERL 'git checkout -p HEAD^' '
# the third n is to get out in case it mistakenly does not apply
-   (echo n; echo y; echo n) | git checkout -p HEAD^ &&
+   printf "%s\n" n y n | git checkout -p HEAD^ &&
verify_saved_state bar &&
verify_state dir/foo parent parent
 '
@@ -69,7 +69,7 @@ test_expect_success PERL 'git checkout -p HEAD^' '
 test_expect_success PERL 'git checkout -p handles deletion' '
set_state dir/foo work index &&
rm dir/foo &&
-   (echo n; echo y) | git checkout -p &&
+   printf "%s\n" n y | git checkout -p &&
verify_saved_state bar &&
verify_state dir/foo index index
 '
@@ -81,21 +81,21 @@ test_expect_success PERL 'git checkout -p handles deletion' 
'
 
 test_expect_success PERL 'path limiting works: dir' '
set_state dir/foo work head &&
-   (echo y; echo n) | git checkout -p dir &&
+   printf "%s\n" y n | git checkout -p dir &&
verify_saved_state bar &&
verify_state dir/foo head head
 '
 
 test_expect_success PERL 'path limiting works: -- dir' '
set_state dir/foo work head &&
-   (echo y; echo n) | git checkout -p -- dir &&
+   printf "%s\n" y n | git checkout -p -- dir &&
verify_saved_state bar &&
verify_state dir/foo head head
 '
 
 test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
# the third n is to get out in case it mistakenly does not apply
-   (echo y; echo n; echo n) | git checkout -p HEAD^ -- dir &&
+   printf "%s\n" y n n | git checkout -p HEAD^ -- dir &&
verify_saved_state bar &&
verify_state dir/foo parent parent
 '
@@ -103,7 +103,7 @@ test

[PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

[Eric Sunshine]

The --chain-lint option detects broken &&-chains by forcing the test to
exit early (as the very first step) with a sentinel value. If that
sentinel is the test's overall exit code, then the &&-chain is intact;
if not, then the chain is broken. Unfortunately, this detection does not
extend to &&-chains within subshells even when the subshell itself is
properly linked into the outer &&-chain.

Address this shortcoming by eliminating the subshell during the
"linting" phase and incorporating its body directly into the surrounding
&&-chain. To keep this transformation cheap, no attempt is made at
properly parsing shell code. Instead, the manipulations are purely
textual. For example:

statement1 &&
(
statement2 &&
statement3
) &&
statement4

is transformed to:

statement1 &&
statement2 &&
statement3 &&
statement4

Notice how "statement3" gains the "&&" which dwelt originally on the
closing ") &&" line. Since this manipulation is purely textual (in fact,
line-by-line), special care is taken to ensure that the "&&" is moved to
the final _statement_ before the closing ")", not necessarily the final
_line_ of text within the subshell. For example, with a here-doc, the
"&&" needs to be added to the opening "<
---
 t/test-lib.sh | 245 +-
 1 file changed, 244 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 28315706be..ade5066fff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -664,6 +664,248 @@ test_eval_ () {
return $test_eval_ret_
 }
 
+test_subshell_sed_='
+# incomplete line -- slurp up next line
+/[^\\]\\$/ {
+  N
+  s/\\\n//
+}
+
+# here-doc -- swallow it to avoid false hits within its body (but keep the
+# command to which it was attached)
+/<<[   ]*[-\\]*EOF[]*&&[   ]*$/ {
+   s/<<[   ]*[-\\]*EOF//
+   h
+   :hereslurp
+   N
+   s/.*\n//
+   /^[ ]*EOF[  ]*$/!bhereslurp
+   x
+   }
+
+# one-liner "(... || git ...)" or "(... || test ...)" -- short-hand for
+# "if ... then : else ...", so leave untouched; contrast with "(... || true)"
+# which ought to be replaced with "test_might_fail ..." to avoid breaking
+# &&-chain
+/^[]*(..*||[   ]*git[  ]..*)[  ]*&&[   ]*$/b
+/^[]*(..*||[   ]*git[  ]..*)[  ]*$/b
+/^[]*(..*||[   ]*test..*)[ ]*&&[   ]*$/b
+/^[]*(..*||[   ]*test..*)[ ]*$/b
+
+# one-liner "(... &) &&" backgrounder -- needs to remain in subshell to avoid
+# becoming syntactically invalid "... & &&"
+/^[]*(..*&[]*)[]*&&[   ]*$/b
+
+# one-liner "(...) &&" -- strip "(" and ")"
+/^[]*(..*)[]*&&[   ]*$/ {
+   s/(//
+   s/)[]*&&[   ]*$/ \&\&/
+   b
+}
+
+# same as above but without trailing "&&"
+/^[]*(..*)[]*$/ {
+   s/(//
+   s/)[]*$//
+   b
+}
+
+# one-liner "(...) >x" (or "2>x" or "|&]/ {
+   s/(//
+   s/)[]*\([0-9]*[<>|&]\)/\1/
+   b
+}
+
+# multi-line "(..." -- strip "(" and pass-thru enclosed lines until ")"
+/^[]*(/ {
+   # strip "(" and stash for later printing
+   s/(//
+   h
+
+   :discard
+   N
+   s/.*\n//
+
+   # loop: slurp enclosed lines
+   :slurp
+   # end-of-file
+   $beof
+   # incomplete line
+   /[^\\]\\$/bincomplete
+   # here-doc
+   /<<[]*[-\\]*EOF/bheredoc
+   # comment or empty line -- discard since closing ") &&" will need to
+   # add "&&" to final non-comment, non-empty subshell line
+   /^[ ]*#/bdiscard
+   /^[ ]*$/bdiscard
+   # one-liner "case ... esac"
+   /^[ ]*case[ ]*..*esac/bpassthru
+   # multi-line "case ... esac"
+   /^[ ]*case[ ]..*[   ]in/bcase
+   # nested one-liner "(...) &&"
+   /^[ ]*(.*)[ ]*&&[   ]*$/boneline
+   # nested one-liner "(...)"
+   /^[ ]*(.*)[ ]*$/boneline
+   # nested one-liner "(...) >x" (or "2>x" or "|]/bonelineredir
+   # nested multi-line "(...\n...)"
+   /^[ ]*(/bnest
+   # closing ") &&" on own line
+   /^[ ]*)[]*&&[   ]*$/bcloseamp
+   # closing ")" on own line
+   /^[ ]*)[]*$/bclose
+   # closing ") >x" (or "2>x" or "|]/bcloseredir
+   # "$((...))" -- not closing ")"
+   /\$(([^)][^)]*))[^)]*$/bpassthru
+   # "$(...)" -- not closing ")"
+   /\$([^)][^)]*)[^)]*$/bpassthru
+   # "=(...)" -- Bash array assignment; not closing ")"
+   /=(/bpassthru
+   # closing "...) &&"
+   /)[ ]*&&[   ]*$/bcloseampx
+   # closing "...)"
+   /)[ ]*$/bclosex
+   # closing "...) >x" (or "2>x" or "|]/bcloseredirx
+   :passthru
+   # retrieve and print previous line
+   x
+   n
+   bslurp
+
+   # end-of-file -- must be closing "...)" line or empty line; if empty,
+   # strip ")" from previous line, else strip ")" from this line
+   :eof
+   /^[ ]*$/bem

[PATCH 24/29] t5000-t5999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t5300-pack-object.sh |  2 +-
 t/t5302-pack-index.sh  |  2 +-
 t/t5401-update-hooks.sh|  4 ++--
 t/t5406-remote-rejects.sh  |  2 +-
 t/t5500-fetch-pack.sh  |  2 +-
 t/t5505-remote.sh  |  2 +-
 t/t5512-ls-remote.sh   |  4 ++--
 t/t5516-fetch-push.sh  | 10 +-
 t/t5517-push-mirror.sh | 10 +-
 t/t5526-fetch-submodules.sh|  2 +-
 t/t5531-deep-submodule-push.sh |  2 +-
 t/t5543-atomic-push.sh |  2 +-
 t/t5601-clone.sh   |  2 +-
 t/t5605-clone-local.sh |  2 +-
 t/t5801-remote-helpers.sh  |  2 +-
 15 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 2336d09dcc..6c620cd540 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -191,7 +191,7 @@ test_expect_success 'survive missing objects/pack 
directory' '
mkdir missing-pack &&
cd missing-pack &&
git init &&
-   GOP=.git/objects/pack
+   GOP=.git/objects/pack &&
rm -fr $GOP &&
git index-pack --stdin --keep=test 
<../test-3-${packname_3}.pack &&
test -f $GOP/pack-${packname_3}.pack &&
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index bb9b8bb309..91d51b35f9 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -237,7 +237,7 @@ test_expect_success 'running index-pack in the object 
store' '
 rm -f .git/objects/pack/* &&
 cp test-1-${pack1}.pack .git/objects/pack/pack-${pack1}.pack &&
 (
-   cd .git/objects/pack
+   cd .git/objects/pack &&
git index-pack pack-${pack1}.pack
 ) &&
 test -f .git/objects/pack/pack-${pack1}.idx
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 7f278d8ce9..b5f886a0e2 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -82,13 +82,13 @@ test_expect_success 'hooks ran' '
 '
 
 test_expect_success 'pre-receive hook input' '
-   (echo $commit0 $commit1 refs/heads/master;
+   (echo $commit0 $commit1 refs/heads/master &&
 echo $commit1 $commit0 refs/heads/tofail
) | test_cmp - victim.git/pre-receive.stdin
 '
 
 test_expect_success 'update hook arguments' '
-   (echo refs/heads/master $commit0 $commit1;
+   (echo refs/heads/master $commit0 $commit1 &&
 echo refs/heads/tofail $commit1 $commit0
) | test_cmp - victim.git/update.args
 '
diff --git a/t/t5406-remote-rejects.sh b/t/t5406-remote-rejects.sh
index 59e80a5ea2..350d2e6ea5 100755
--- a/t/t5406-remote-rejects.sh
+++ b/t/t5406-remote-rejects.sh
@@ -6,7 +6,7 @@ test_description='remote push rejects are reported by client'
 
 test_expect_success 'setup' '
mkdir .git/hooks &&
-   (echo "#!/bin/sh" ; echo "exit 1") >.git/hooks/update &&
+   (echo "#!/bin/sh" && echo "exit 1") >.git/hooks/update &&
chmod +x .git/hooks/update &&
echo 1 >file &&
git add file &&
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 8390c0a2d2..c394779f65 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -259,7 +259,7 @@ test_expect_success 'clone shallow object count' '
 test_expect_success 'pull in shallow repo with missing merge base' '
(
cd shallow &&
-   git fetch --depth 4 .. A
+   git fetch --depth 4 .. A &&
test_must_fail git merge --allow-unrelated-histories FETCH_HEAD
)
 '
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 3552b51b4c..11e14a1e0f 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -844,7 +844,7 @@ test_expect_success 'migrate a remote from named file in 
$GIT_DIR/branches (2)'
git remote rename origin origin &&
test_path_is_missing .git/branches/origin &&
test "$(git config remote.origin.url)" = "quux" &&
-   test "$(git config remote.origin.fetch)" = 
"refs/heads/foom:refs/heads/origin"
+   test "$(git config remote.origin.fetch)" = 
"refs/heads/foom:refs/heads/origin" &&
test "$(git config remote.origin.push)" = "HEAD:refs/heads/foom"
)
 '
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 6a949484d0..ea020040e8 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -15,7 +15,7 @@ test_expect_success setup '
git tag mark1.10 &&
git show-ref --tags -d | sed -e "s/ /   /" >expected.tag &&
(
-   echo "$(git rev-parse HEAD) HEAD"
+   echo "$(git rev-parse HEAD) HEAD" &&
git show-ref -d | sed -e "s/ /  /"
) >expected.all &&
 
@@ -105,7 +105,7 @@ test_expect_success 'use branch..remote if possible' '
git clone . other.git &&
(
cd other.git &&
-   echo "$(git rev-parse HEAD) HEAD"
+   echo "$(git rev-par

[PATCH 18/29] t0000-t0999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t-basic.sh  |  2 +-
 t/t0003-attributes.sh | 24 
 t/t0021-conversion.sh |  4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/t/t-basic.sh b/t/t-basic.sh
index af61d083b4..34859fe4a5 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -1081,7 +1081,7 @@ test_expect_success 'very long name in the index handled 
sanely' '
(
git ls-files -s path4 |
sed -e "s/  .*/ /" |
-   tr -d "\012"
+   tr -d "\012" &&
echo "$a"
) | git update-index --index-info &&
len=$(git ls-files "a*" | wc -c) &&
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f19ae4f8cc..5c37c2e1f8 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -34,15 +34,15 @@ test_expect_success 'open-quoted pathname' '
 test_expect_success 'setup' '
mkdir -p a/b/d a/c b &&
(
-   echo "[attr]notest !test"
-   echo "\" d \"   test=d"
-   echo " etest=e"
-   echo " e\"  test=e"
-   echo "f test=f"
-   echo "a/i test=a/i"
-   echo "onoff test -test"
-   echo "offon -test test"
-   echo "no notest"
+   echo "[attr]notest !test" &&
+   echo "\" d \"   test=d" &&
+   echo " etest=e" &&
+   echo " e\"  test=e" &&
+   echo "f test=f" &&
+   echo "a/i test=a/i" &&
+   echo "onoff test -test" &&
+   echo "offon -test test" &&
+   echo "no notest" &&
echo "A/e/F test=A/e/F"
) >.gitattributes &&
(
@@ -51,7 +51,7 @@ test_expect_success 'setup' '
) >a/.gitattributes &&
(
echo "h test=a/b/h" &&
-   echo "d/* test=a/b/d/*"
+   echo "d/* test=a/b/d/*" &&
echo "d/yes notest"
) >a/b/.gitattributes &&
(
@@ -287,7 +287,7 @@ test_expect_success 'bare repository: check that 
.gitattribute is ignored' '
(
cd bare.git &&
(
-   echo "f test=f"
+   echo "f test=f" &&
echo "a/i test=a/i"
) >.gitattributes &&
attr_check f unspecified &&
@@ -312,7 +312,7 @@ test_expect_success 'bare repository: test info/attributes' 
'
(
cd bare.git &&
(
-   echo "f test=f"
+   echo "f test=f" &&
echo "a/i test=a/i"
) >info/attributes &&
attr_check f f &&
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 9479a4aaab..6a213608cc 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -785,7 +785,7 @@ test_expect_success PERL 'missing file in delayed checkout' 
'
cd repo &&
git init &&
echo "*.a filter=bug" >.gitattributes &&
-   cp "$TEST_ROOT/test.o" missing-delay.a
+   cp "$TEST_ROOT/test.o" missing-delay.a &&
git add . &&
git commit -m "test commit"
) &&
@@ -807,7 +807,7 @@ test_expect_success PERL 'invalid file in delayed checkout' 
'
git init &&
echo "*.a filter=bug" >.gitattributes &&
cp "$TEST_ROOT/test.o" invalid-delay.a &&
-   cp "$TEST_ROOT/test.o" unfiltered
+   cp "$TEST_ROOT/test.o" unfiltered &&
git add . &&
git commit -m "test commit"
) &&
-- 
2.18.0.419.gfe4b301394

[PATCH 28/29] t9119: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t9119-git-svn-info.sh | 120 
 1 file changed, 60 insertions(+), 60 deletions(-)

diff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh
index 88241baee3..8201c3e808 100755
--- a/t/t9119-git-svn-info.sh
+++ b/t/t9119-git-svn-info.sh
@@ -22,8 +22,8 @@ esac
 # same value as "svn info" (i.e. the commit timestamp that touched the
 # path most recently); do not expect that field to match.
 test_cmp_info () {
-   sed -e '/^Text Last Updated:/d' "$1" >tmp.expect
-   sed -e '/^Text Last Updated:/d' "$2" >tmp.actual
+   sed -e '/^Text Last Updated:/d' "$1" >tmp.expect &&
+   sed -e '/^Text Last Updated:/d' "$2" >tmp.actual &&
test_cmp tmp.expect tmp.actual &&
rm -f tmp.expect tmp.actual
 }
@@ -59,24 +59,24 @@ test_expect_success 'setup repository and import' '
'
 
 test_expect_success 'info' "
-   (cd svnwc; svn info) > expected.info &&
-   (cd gitwc; git svn info) > actual.info &&
+   (cd svnwc && svn info) > expected.info &&
+   (cd gitwc && git svn info) > actual.info &&
test_cmp_info expected.info actual.info
"
 
 test_expect_success 'info --url' '
-   test "$(cd gitwc; git svn info --url)" = "$quoted_svnrepo"
+   test "$(cd gitwc && git svn info --url)" = "$quoted_svnrepo"
'
 
 test_expect_success 'info .' "
-   (cd svnwc; svn info .) > expected.info-dot &&
-   (cd gitwc; git svn info .) > actual.info-dot &&
+   (cd svnwc && svn info .) > expected.info-dot &&
+   (cd gitwc && git svn info .) > actual.info-dot &&
test_cmp_info expected.info-dot actual.info-dot
"
 
 test_expect_success 'info $(pwd)' '
-   (cd svnwc; svn info "$(pwd)") >expected.info-pwd &&
-   (cd gitwc; git svn info "$(pwd)") >actual.info-pwd &&
+   (cd svnwc && svn info "$(pwd)") >expected.info-pwd &&
+   (cd gitwc && git svn info "$(pwd)") >actual.info-pwd &&
grep -v ^Path: expected.info-np &&
grep -v ^Path: actual.info-np &&
test_cmp_info expected.info-np actual.info-np &&
@@ -85,8 +85,8 @@ test_expect_success 'info $(pwd)' '
'
 
 test_expect_success 'info $(pwd)/../___wc' '
-   (cd svnwc; svn info "$(pwd)/../svnwc") >expected.info-pwd &&
-   (cd gitwc; git svn info "$(pwd)/../gitwc") >actual.info-pwd &&
+   (cd svnwc && svn info "$(pwd)/../svnwc") >expected.info-pwd &&
+   (cd gitwc && git svn info "$(pwd)/../gitwc") >actual.info-pwd &&
grep -v ^Path: expected.info-np &&
grep -v ^Path: actual.info-np &&
test_cmp_info expected.info-np actual.info-np &&
@@ -95,8 +95,8 @@ test_expect_success 'info $(pwd)/../___wc' '
'
 
 test_expect_success 'info $(pwd)/../___wc//file' '
-   (cd svnwc; svn info "$(pwd)/../svnwc//file") >expected.info-pwd &&
-   (cd gitwc; git svn info "$(pwd)/../gitwc//file") >actual.info-pwd &&
+   (cd svnwc && svn info "$(pwd)/../svnwc//file") >expected.info-pwd &&
+   (cd gitwc && git svn info "$(pwd)/../gitwc//file") >actual.info-pwd &&
grep -v ^Path: expected.info-np &&
grep -v ^Path: actual.info-np &&
test_cmp_info expected.info-np actual.info-np &&
@@ -105,56 +105,56 @@ test_expect_success 'info $(pwd)/../___wc//file' '
'
 
 test_expect_success 'info --url .' '
-   test "$(cd gitwc; git svn info --url .)" = "$quoted_svnrepo"
+   test "$(cd gitwc && git svn info --url .)" = "$quoted_svnrepo"
'
 
 test_expect_success 'info file' "
-   (cd svnwc; svn info file) > expected.info-file &&
-   (cd gitwc; git svn info file) > actual.info-file &&
+   (cd svnwc && svn info file) > expected.info-file &&
+   (cd gitwc && git svn info file) > actual.info-file &&
test_cmp_info expected.info-file actual.info-file
"
 
 test_expect_success 'info --url file' '
-   test "$(cd gitwc; git svn info --url file)" = "$quoted_svnrepo/file"
+   test "$(cd gitwc && git svn info --url file)" = "$quoted_svnrepo/file"
'
 
 test_expect_success 'info directory' "
-   (cd svnwc; svn info directory) > expected.info-directory &&
-   (cd gitwc; git svn info directory) > actual.info-directory &&
+   (cd svnwc && svn info directory) > expected.info-directory &&
+   (cd gitwc && git svn info directory) > actual.info-directory &&
test_cmp_info expected.info-directory actual.info-directory
"
 
 test_expect_success 'info inside directory' "
-   (cd svnwc/directory; svn info) > expected.info-inside-directory &&
-   (cd gitwc/directory; git svn info) > actual.info-inside-directory &&
+   (cd svnwc/directory && svn info) > expected.info-inside-directory &&
+   (cd gitwc/directory && git svn info) > actual.info-inside-directory &&
test_cmp_info expected.info-inside-directory 
actual.info-inside-directory
"
 
 test_expect_success 'info --url directory' '
-   test "$(cd gitwc; git svn info --url di

[PATCH 09/29] t7810: use test_expect_code() instead of hand-rolled comparison

[Eric Sunshine]

This test manually checks the exit code of git-grep for a particular
value. In doing so, it breaks the &&-chain. An upcoming change will
teach --chain-lint to check the &&-chain inside subshells, so this
manual exit code handling will run afoul of the &&-chain check.
Therefore, replace the manual handling with test_expect_code() and a
normal &&-chain.

Signed-off-by: Eric Sunshine 
---
 t/t7810-grep.sh | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..fecee602c1 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -845,10 +845,9 @@ test_expect_success 'grep from a subdirectory to search 
wider area (1)' '
 test_expect_success 'grep from a subdirectory to search wider area (2)' '
mkdir -p s &&
(
-   cd s || exit 1
-   ( git grep xxyyzz .. >out ; echo $? >status )
-   ! test -s out &&
-   test 1 = $(cat status)
+   cd s &&
+   test_expect_code 1 git grep xxyyzz .. >out &&
+   ! test -s out
)
 '
 
-- 
2.18.0.419.gfe4b301394

[PATCH 12/29] t9401: drop unnecessary nested subshell

[Eric Sunshine]

This test employs an unnecessary nested subshell:

(cd foo &&
 statement1 &&
 (cd bar &&
  statement2))

An upcoming change will teach --chain-lint to check the &&-chain in
subshells. The check works by performing textual transformations on the
test to link the subshell body directly into the parent's &&-chain. It
employs heuristics to identify the extent of a subshell, however,
closing two subshells on a single line like this will fool it.

Rather than extending the heuristics even further for this one-off case,
just drop the pointless nested subshell.

Signed-off-by: Eric Sunshine 
---
 t/t9401-git-cvsserver-crlf.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index 84787eee9a..8b8d7ac34a 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh
@@ -288,9 +288,9 @@ test_expect_success 'add bin (guess)' '
 test_expect_success 'remove files (guess)' '
 (cd cvswork &&
 GIT_CONFIG="$git_config" cvs -Q rm -f subdir/file.h &&
-(cd subdir &&
+cd subdir &&
 GIT_CONFIG="$git_config" cvs -Q rm -f withCr.bin
-)) &&
+) &&
 marked_as cvswork/subdir withCr.bin -kb &&
 marked_as cvswork/subdir file.h ""
 '
-- 
2.18.0.419.gfe4b301394

[PATCH 27/29] t9000-t9999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t9001-send-email.sh   |  6 +++---
 t/t9100-git-svn-basic.sh|  2 +-
 t/t9101-git-svn-props.sh|  2 +-
 t/t9122-git-svn-author.sh   |  6 +++---
 t/t9129-git-svn-i18n-commitencoding.sh  |  2 +-
 t/t9130-git-svn-authors-file.sh |  4 ++--
 t/t9134-git-svn-ignore-paths.sh |  6 +++---
 t/t9137-git-svn-dcommit-clobber-series.sh   |  2 +-
 t/t9138-git-svn-authors-prog.sh |  6 +++---
 t/t9146-git-svn-empty-dirs.sh   | 12 ++--
 t/t9147-git-svn-include-paths.sh|  6 +++---
 t/t9164-git-svn-dcommit-concurrent.sh   |  2 +-
 t/t9165-git-svn-fetch-merge-branch-of-branch.sh |  2 +-
 t/t9200-git-cvsexportcommit.sh  |  6 +++---
 t/t9400-git-cvsserver-server.sh |  6 +++---
 t/t9600-cvsimport.sh|  2 +-
 t/t9806-git-p4-options.sh   |  2 +-
 t/t9810-git-p4-rcs.sh   |  2 +-
 t/t9811-git-p4-label-import.sh  |  2 +-
 t/t9815-git-p4-submit-fail.sh   |  2 +-
 t/t9830-git-p4-symlink-dir.sh   |  2 +-
 t/t9831-git-p4-triggers.sh  |  2 +-
 t/t9902-completion.sh   |  4 ++--
 t/t9903-bash-prompt.sh  |  2 +-
 24 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 776769fe0d..53314ff54e 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -330,7 +330,7 @@ test_expect_success $PREREQ 'Show all headers' '
 
 test_expect_success $PREREQ 'Prompting works' '
clean_fake_sendmail &&
-   (echo "t...@example.com"
+   (echo "t...@example.com" &&
 echo ""
) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
--smtp-server="$(pwd)/fake.sendmail" \
@@ -470,8 +470,8 @@ test_expect_success $PREREQ 'Invalid In-Reply-To' '
 
 test_expect_success $PREREQ 'Valid In-Reply-To when prompting' '
clean_fake_sendmail &&
-   (echo "From Example "
-echo "To Example "
+   (echo "From Example " &&
+echo "To Example " &&
 echo ""
) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
--smtp-server="$(pwd)/fake.sendmail" \
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index c937330a5f..9af6078844 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -31,7 +31,7 @@ test_expect_success \
(
cd import &&
echo foo >foo &&
-   ln -s foo foo.link
+   ln -s foo foo.link &&
mkdir -p dir/a/b/c/d/e &&
echo "deep dir" >dir/a/b/c/d/e/file &&
mkdir bar &&
diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh
index 07bfb63777..8a5c8dc1aa 100755
--- a/t/t9101-git-svn-props.sh
+++ b/t/t9101-git-svn-props.sh
@@ -149,7 +149,7 @@ test_expect_success 'test show-ignore' "
svn_cmd up &&
svn_cmd propset -R svn:ignore '
 no-such-file*
-' .
+' . &&
svn_cmd commit -m 'propset svn:ignore'
) &&
git svn show-ignore > show-ignore.got &&
diff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh
index 30013b7bb9..9e8fe38e7e 100755
--- a/t/t9122-git-svn-author.sh
+++ b/t/t9122-git-svn-author.sh
@@ -7,8 +7,8 @@ test_expect_success 'setup svn repository' '
svn_cmd checkout "$svnrepo" work.svn &&
(
cd work.svn &&
-   echo >file
-   svn_cmd add file
+   echo >file &&
+   svn_cmd add file &&
svn_cmd commit -m "first commit" file
)
 '
@@ -17,7 +17,7 @@ test_expect_success 'interact with it via git svn' '
mkdir work.git &&
(
cd work.git &&
-   git svn init "$svnrepo"
+   git svn init "$svnrepo" &&
git svn fetch &&
 
echo modification >file &&
diff --git a/t/t9129-git-svn-i18n-commitencoding.sh 
b/t/t9129-git-svn-i18n-commitencoding.sh
index 8dbd6476fa..2c213ae654 100755
--- a/t/t9129-git-svn-i18n-commitencoding.sh
+++ b/t/t9129-git-svn-i18n-commitencoding.sh
@@ -51,7 +51,7 @@ do
git add F &&
git commit -a -F "$TEST_DIRECTORY"/t3900/$H.txt &&
E=$(git cat-file commit HEAD | sed -ne "s/^encoding //p") &&
-   test "z$E" = "z$H"
+   test "z$E" = "z$H" &&
compare_git_head_with "$TEST_DIRECTORY"/t3900/$H.txt
)
'
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index d8262854bb..cb764bcadc 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -25,7 +25,7 @@ test_expect_success 'start import with incomplete authors 
file' '
 
 test

[PATCH 22/29] t3030: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t3030-merge-recursive.sh | 340 ++---
 1 file changed, 170 insertions(+), 170 deletions(-)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 3563e77b37..ff641b348a 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -36,15 +36,15 @@ test_expect_success 'setup 1' '
test_tick &&
git commit -m "master modifies a and d/e" &&
c1=$(git rev-parse --verify HEAD) &&
-   ( git ls-tree -r HEAD ; git ls-files -s ) >actual &&
+   ( git ls-tree -r HEAD && git ls-files -s ) >actual &&
(
-   echo "100644 blob $o1   a"
-   echo "100644 blob $o0   b"
-   echo "100644 blob $o0   c"
-   echo "100644 blob $o1   d/e"
-   echo "100644 $o1 0  a"
-   echo "100644 $o0 0  b"
-   echo "100644 $o0 0  c"
+   echo "100644 blob $o1   a" &&
+   echo "100644 blob $o0   b" &&
+   echo "100644 blob $o0   c" &&
+   echo "100644 blob $o1   d/e" &&
+   echo "100644 $o1 0  a" &&
+   echo "100644 $o0 0  b" &&
+   echo "100644 $o0 0  c" &&
echo "100644 $o1 0  d/e"
) >expected &&
test_cmp expected actual
@@ -54,15 +54,15 @@ test_expect_success 'setup 2' '
 
rm -rf [abcd] &&
git checkout side &&
-   ( git ls-tree -r HEAD ; git ls-files -s ) >actual &&
+   ( git ls-tree -r HEAD && git ls-files -s ) >actual &&
(
-   echo "100644 blob $o0   a"
-   echo "100644 blob $o0   b"
-   echo "100644 blob $o0   c"
-   echo "100644 blob $o0   d/e"
-   echo "100644 $o0 0  a"
-   echo "100644 $o0 0  b"
-   echo "100644 $o0 0  c"
+   echo "100644 blob $o0   a" &&
+   echo "100644 blob $o0   b" &&
+   echo "100644 blob $o0   c" &&
+   echo "100644 blob $o0   d/e" &&
+   echo "100644 $o0 0  a" &&
+   echo "100644 $o0 0  b" &&
+   echo "100644 $o0 0  c" &&
echo "100644 $o0 0  d/e"
) >expected &&
test_cmp expected actual &&
@@ -75,15 +75,15 @@ test_expect_success 'setup 2' '
test_tick &&
git commit -m "side modifies a" &&
c2=$(git rev-parse --verify HEAD) &&
-   ( git ls-tree -r HEAD ; git ls-files -s ) >actual &&
+   ( git ls-tree -r HEAD && git ls-files -s ) >actual &&
(
-   echo "100644 blob $o2   a"
-   echo "100644 blob $o0   b"
-   echo "100644 blob $o0   c"
-   echo "100644 blob $o0   d/e"
-   echo "100644 $o2 0  a"
-   echo "100644 $o0 0  b"
-   echo "100644 $o0 0  c"
+   echo "100644 blob $o2   a" &&
+   echo "100644 blob $o0   b" &&
+   echo "100644 blob $o0   c" &&
+   echo "100644 blob $o0   d/e" &&
+   echo "100644 $o2 0  a" &&
+   echo "100644 $o0 0  b" &&
+   echo "100644 $o0 0  c" &&
echo "100644 $o0 0  d/e"
) >expected &&
test_cmp expected actual
@@ -93,15 +93,15 @@ test_expect_success 'setup 3' '
 
rm -rf [abcd] &&
git checkout df-1 &&
-   ( git ls-tree -r HEAD ; git ls-files -s ) >actual &&
+   ( git ls-tree -r HEAD && git ls-files -s ) >actual &&
(
-   echo "100644 blob $o0   a"
-   echo "100644 blob $o0   b"
-   echo "100644 blob $o0   c"
-   echo "100644 blob $o0   d/e"
-   echo "100644 $o0 0  a"
-   echo "100644 $o0 0  b"
-   echo "100644 $o0 0  c"
+   echo "100644 blob $o0   a" &&
+   echo "100644 blob $o0   b" &&
+   echo "100644 blob $o0   c" &&
+   echo "100644 blob $o0   d/e" &&
+   echo "100644 $o0 0  a" &&
+   echo "100644 $o0 0  b" &&
+   echo "100644 $o0 0  c" &&
echo "100644 $o0 0  d/e"
) >expected &&
test_cmp expected actual &&
@@ -112,15 +112,15 @@ test_expect_success 'setup 3' '
test_tick &&
git commit -m "df-1 makes b/c" &&
c3=$(git rev-parse --verify HEAD) &&
-   ( git ls-tree -r HEAD ; git ls-files -s ) >actual &&
+   ( git ls-tree -r HEAD && git ls-files -s ) >actual &&
(
-   echo "100644 blob $o0   a"
-   echo "100644 blob $o3   b/c"
-   echo "100644 blob $o0   c"
-   echo "100644 blob $o0   d/e"
-   echo "100644 $o0 0  a"
-   echo "100644 $o3 0  b/c"
-   echo "100644 $o0 0  c"
+   echo "100644 blob $o0   a" &&
+   echo "

[PATCH 21/29] t3000-t3999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t3000-ls-files-others.sh  | 2 +-
 t/t3006-ls-files-long.sh| 2 +-
 t/t3008-ls-files-lazy-init-name-hash.sh | 8 
 t/t3050-subprojects-fetch.sh| 8 
 t/t3301-notes.sh| 8 
 t/t3400-rebase.sh   | 8 
 t/t3402-rebase-merge.sh | 4 ++--
 t/t3418-rebase-continue.sh  | 4 ++--
 t/t3700-add.sh  | 8 
 9 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index c525656b2c..afd4756134 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -84,7 +84,7 @@ test_expect_success SYMLINKS 'ls-files --others with 
symlinked submodule' '
) &&
(
cd super &&
-   "$SHELL_PATH" 
"$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" ../sub sub
+   "$SHELL_PATH" 
"$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" ../sub sub &&
git ls-files --others --exclude-standard >../actual
) &&
echo sub/ >expect &&
diff --git a/t/t3006-ls-files-long.sh b/t/t3006-ls-files-long.sh
index 202ad658b8..e109c3fbfb 100755
--- a/t/t3006-ls-files-long.sh
+++ b/t/t3006-ls-files-long.sh
@@ -29,7 +29,7 @@ test_expect_success 'overly-long path does not replace 
another by mistake' '
printf "$pat" "$blob_a" "$path_a" "$blob_z" "$path_z" |
git update-index --add --index-info &&
(
-   echo "$path_a"
+   echo "$path_a" &&
echo "$path_z"
) >expect &&
git ls-files >actual &&
diff --git a/t/t3008-ls-files-lazy-init-name-hash.sh 
b/t/t3008-ls-files-lazy-init-name-hash.sh
index 08af596ba6..64f047332b 100755
--- a/t/t3008-ls-files-lazy-init-name-hash.sh
+++ b/t/t3008-ls-files-lazy-init-name-hash.sh
@@ -14,10 +14,10 @@ LAZY_THREAD_COST=2000
 
 test_expect_success 'no buffer overflow in lazy_init_name_hash' '
(
-   test_seq $LAZY_THREAD_COST | sed "s/^/a_/"
-   echo b/b/b
-   test_seq $LAZY_THREAD_COST | sed "s/^/c_/"
-   test_seq 50 | sed "s/^/d_/" | tr "\n" "/"; echo d
+   test_seq $LAZY_THREAD_COST | sed "s/^/a_/" &&
+   echo b/b/b &&
+   test_seq $LAZY_THREAD_COST | sed "s/^/c_/" &&
+   test_seq 50 | sed "s/^/d_/" | tr "\n" "/" && echo d
) |
sed "s/^/100644 $EMPTY_BLOB /" |
git update-index --index-info &&
diff --git a/t/t3050-subprojects-fetch.sh b/t/t3050-subprojects-fetch.sh
index 2f5f41a012..f1f09abdd9 100755
--- a/t/t3050-subprojects-fetch.sh
+++ b/t/t3050-subprojects-fetch.sh
@@ -21,10 +21,10 @@ test_expect_success setup '
 
 test_expect_success clone '
git clone "file://$(pwd)/.git" cloned &&
-   (git rev-parse HEAD; git ls-files -s) >expected &&
+   (git rev-parse HEAD && git ls-files -s) >expected &&
(
cd cloned &&
-   (git rev-parse HEAD; git ls-files -s) >../actual
+   (git rev-parse HEAD && git ls-files -s) >../actual
) &&
test_cmp expected actual
 '
@@ -40,11 +40,11 @@ test_expect_success advance '
 '
 
 test_expect_success fetch '
-   (git rev-parse HEAD; git ls-files -s) >expected &&
+   (git rev-parse HEAD && git ls-files -s) >expected &&
(
cd cloned &&
git pull &&
-   (git rev-parse HEAD; git ls-files -s) >../actual
+   (git rev-parse HEAD && git ls-files -s) >../actual
) &&
test_cmp expected actual
 '
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 2d200fdf36..ac62dc0e8f 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -914,7 +914,7 @@ test_expect_success 'git notes copy --stdin' '
${indent}
${indent}yet another note
EOF
-   (echo $(git rev-parse HEAD~3) $(git rev-parse HEAD^); \
+   (echo $(git rev-parse HEAD~3) $(git rev-parse HEAD^) &&
echo $(git rev-parse HEAD~2) $(git rev-parse HEAD)) |
git notes copy --stdin &&
git log -2 >actual &&
@@ -939,7 +939,7 @@ test_expect_success 'git notes copy --for-rewrite 
(unconfigured)' '
EOF
test_commit 14th &&
test_commit 15th &&
-   (echo $(git rev-parse HEAD~3) $(git rev-parse HEAD^); \
+   (echo $(git rev-parse HEAD~3) $(git rev-parse HEAD^) &&
echo $(git rev-parse HEAD~2) $(git rev-parse HEAD)) |
git notes copy --for-rewrite=foo &&
git log -2 >actual &&
@@ -972,7 +972,7 @@ test_expect_success 'git notes copy --for-rewrite 
(enabled)' '
EOF
test_config notes.rewriteMode overwrite &&
test_config notes.rewriteRef "refs/notes/*" &&
-   (echo $(git rev-parse HEAD~3) $(git rev-parse HEAD^); \
+   (echo $(git rev-parse HEAD~3) $(git rev-parse HEAD^) &&
echo $(git rev-parse HEAD~2) $(git rev-parse HEAD)) |
g

[PATCH 23/29] t4000-t4999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t4001-diff-rename.sh   | 2 +-
 t/t4025-hunk-header.sh   | 8 
 t/t4041-diff-submodule-option.sh | 4 ++--
 t/t4060-diff-submodule-option-diff-format.sh | 2 +-
 t/t4121-apply-diffs.sh   | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index bf4030371a..c16486a9d4 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -180,7 +180,7 @@ test_expect_success 'setup for many rename source 
candidates' '
git add "path??" &&
test_tick &&
git commit -m "hundred" &&
-   (cat path1; echo new) >new-path &&
+   (cat path1 && echo new) >new-path &&
echo old >>path1 &&
git add new-path path1 &&
git diff -l 4 -C -C --cached --name-status >actual 2>actual.err &&
diff --git a/t/t4025-hunk-header.sh b/t/t4025-hunk-header.sh
index 7a3dbc1ea2..fa44e78869 100755
--- a/t/t4025-hunk-header.sh
+++ b/t/t4025-hunk-header.sh
@@ -12,12 +12,12 @@ NS="$N$N$N$N$N$N$N$N$N$N$N$N$N"
 test_expect_success setup '
 
(
-   echo "A $NS"
+   echo "A $NS" &&
for c in B C D E F G H I J K
do
echo "  $c"
-   done
-   echo "L  $NS"
+   done &&
+   echo "L  $NS" &&
for c in M N O P Q R S T U V
do
echo "  $c"
@@ -34,7 +34,7 @@ test_expect_success 'hunk header truncation with an overly 
long line' '
 
git diff | sed -n -e "s/^.*@@//p" >actual &&
(
-   echo " A $N$N$N$N$N$N$N$N$N2"
+   echo " A $N$N$N$N$N$N$N$N$N2" &&
echo " L  $N$N$N$N$N$N$N$N$N1"
) >expected &&
test_cmp actual expected
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 058ee0829d..4e3499ef84 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -498,7 +498,7 @@ test_expect_success 'given commit --submodule=short' '
 test_expect_success 'setup .git file for sm2' '
(cd sm2 &&
 REAL="$(pwd)/../.real" &&
-mv .git "$REAL"
+mv .git "$REAL" &&
 echo "gitdir: $REAL" >.git)
 '
 
@@ -527,7 +527,7 @@ test_expect_success 'diff --submodule with objects 
referenced by alternates' '
git commit -m "sub a"
) &&
(cd sub_alt &&
-   sha1_before=$(git rev-parse --short HEAD)
+   sha1_before=$(git rev-parse --short HEAD) &&
echo b >b &&
git add b &&
git commit -m b &&
diff --git a/t/t4060-diff-submodule-option-diff-format.sh 
b/t/t4060-diff-submodule-option-diff-format.sh
index 4b168d0ed7..0eba4620f0 100755
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
@@ -721,7 +721,7 @@ test_expect_success 'given commit' '
 test_expect_success 'setup .git file for sm2' '
(cd sm2 &&
 REAL="$(pwd)/../.real" &&
-mv .git "$REAL"
+mv .git "$REAL" &&
 echo "gitdir: $REAL" >.git)
 '
 
diff --git a/t/t4121-apply-diffs.sh b/t/t4121-apply-diffs.sh
index aff551a1d7..66368effd5 100755
--- a/t/t4121-apply-diffs.sh
+++ b/t/t4121-apply-diffs.sh
@@ -27,6 +27,6 @@ test_expect_success 'setup' \
 
 test_expect_success \
'check if contextually independent diffs for the same file apply' \
-   '( git diff test~2 test~1; git diff test~1 test~0 )| git apply'
+   '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply'
 
 test_done
-- 
2.18.0.419.gfe4b301394

[PATCH 19/29] t1000-t1999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t1004-read-tree-m-u-wf.sh |  8 
 t/t1005-read-tree-reset.sh  | 10 +-
 t/t1008-read-tree-overlay.sh|  2 +-
 t/t1020-subdirectory.sh |  2 +-
 t/t1050-large.sh|  6 +++---
 t/t1411-reflog-show.sh  |  6 +++---
 t/t1512-rev-parse-disambiguation.sh |  6 +++---
 7 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index c7ce5d8bb5..a479549fb6 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -212,10 +212,10 @@ test_expect_success 'D/F' '
read_tree_u_must_succeed -m -u branch-point side-b side-a &&
git ls-files -u >actual &&
(
-   a=$(git rev-parse branch-point:subdir/file2)
-   b=$(git rev-parse side-a:subdir/file2/another)
-   echo "100644 $a 1   subdir/file2"
-   echo "100644 $a 2   subdir/file2"
+   a=$(git rev-parse branch-point:subdir/file2) &&
+   b=$(git rev-parse side-a:subdir/file2/another) &&
+   echo "100644 $a 1   subdir/file2" &&
+   echo "100644 $a 2   subdir/file2" &&
echo "100644 $b 3   subdir/file2/another"
) >expect &&
test_cmp expect actual
diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
index 074568500a..83b09e1310 100755
--- a/t/t1005-read-tree-reset.sh
+++ b/t/t1005-read-tree-reset.sh
@@ -33,7 +33,7 @@ test_expect_success 'reset should remove remnants from a 
failed merge' '
git ls-files -s >expect &&
sha1=$(git rev-parse :new) &&
(
-   echo "100644 $sha1 1old"
+   echo "100644 $sha1 1old" &&
echo "100644 $sha1 3old"
) | git update-index --index-info &&
>old &&
@@ -48,7 +48,7 @@ test_expect_success 'two-way reset should remove remnants 
too' '
git ls-files -s >expect &&
sha1=$(git rev-parse :new) &&
(
-   echo "100644 $sha1 1old"
+   echo "100644 $sha1 1old" &&
echo "100644 $sha1 3old"
) | git update-index --index-info &&
>old &&
@@ -63,7 +63,7 @@ test_expect_success 'Porcelain reset should remove remnants 
too' '
git ls-files -s >expect &&
sha1=$(git rev-parse :new) &&
(
-   echo "100644 $sha1 1old"
+   echo "100644 $sha1 1old" &&
echo "100644 $sha1 3old"
) | git update-index --index-info &&
>old &&
@@ -78,7 +78,7 @@ test_expect_success 'Porcelain checkout -f should remove 
remnants too' '
git ls-files -s >expect &&
sha1=$(git rev-parse :new) &&
(
-   echo "100644 $sha1 1old"
+   echo "100644 $sha1 1old" &&
echo "100644 $sha1 3old"
) | git update-index --index-info &&
>old &&
@@ -93,7 +93,7 @@ test_expect_success 'Porcelain checkout -f HEAD should remove 
remnants too' '
git ls-files -s >expect &&
sha1=$(git rev-parse :new) &&
(
-   echo "100644 $sha1 1old"
+   echo "100644 $sha1 1old" &&
echo "100644 $sha1 3old"
) | git update-index --index-info &&
>old &&
diff --git a/t/t1008-read-tree-overlay.sh b/t/t1008-read-tree-overlay.sh
index 4c50ed955e..e74b185b6c 100755
--- a/t/t1008-read-tree-overlay.sh
+++ b/t/t1008-read-tree-overlay.sh
@@ -23,7 +23,7 @@ test_expect_success setup '
 
 test_expect_success 'multi-read' '
read_tree_must_succeed initial master side &&
-   (echo a; echo b/c) >expect &&
+   (echo a && echo b/c) >expect &&
git ls-files >actual &&
test_cmp expect actual
 '
diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index df3183ea1a..c2df75e495 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -148,7 +148,7 @@ test_expect_success 'GIT_PREFIX for built-ins' '
(
cd dir &&
echo "change" >two &&
-   GIT_EXTERNAL_DIFF=./diff git diff >../actual
+   GIT_EXTERNAL_DIFF=./diff git diff >../actual &&
git checkout -- two
) &&
test_cmp expect actual
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index f9eb143f43..1a9b21b293 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -108,7 +108,7 @@ test_expect_success 'packsize limit' '
test-tool genrandom "c" $(( 128 * 1024 )) >mid3 &&
git add mid1 mid2 mid3 &&
 
-   count=0
+   count=0 &&
for pi in .git/objects/pack/pack-*.idx
do
test -f "$pi" && count=$(( $count + 1 ))
@@ -116,8 +116,8 @@ test_expect_success 'packsize limit' '
test $count = 2 &&
 
(
-   git hash-obje

[PATCH 13/29] t/lib-submodule-update: fix broken "replace submodule must-fail" test

[Eric Sunshine]

This test has been dysfunctional since it was added by 259f3ee296
(lib-submodule-update.sh: define tests for recursing into submodules,
2017-03-14), however, problems went unnoticed due to a broken &&-chain
toward the end of the test.

The test wants to verify that replacing a submodule containing a .git
directory must fail. All other "must fail" tests in this script invoke
the supplied command as 'test_must_fail', however, this test neglects to
do so.

To make matters worse, the command actually succeeds even though it's
not supposed to. Presumably, this is a "known breakage", which means
that the entire test should be marked 'test_expect_failure', however, it
is instead marked 'test_expect_success'.

Fix both problems, as well as the broken &&-chain behind which these
problems hid.

While at it, fix broken &&-chains in a couple neighboring tests.

Signed-off-by: Eric Sunshine 
---
 t/lib-submodule-update.sh | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 1f38a85371..8a2edee1cb 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -755,7 +755,7 @@ test_submodule_recursing_with_args_common() {
: >sub1/untrackedfile &&
test_must_fail $command replace_sub1_with_file &&
test_superproject_content origin/add_sub1 &&
-   test_submodule_content sub1 origin/add_sub1
+   test_submodule_content sub1 origin/add_sub1 &&
test -f sub1/untracked_file
)
'
@@ -842,7 +842,7 @@ test_submodule_switch_recursing_with_args () {
cd submodule_update &&
git branch -t add_sub1 origin/add_sub1 &&
: >sub1 &&
-   echo sub1 >.git/info/exclude
+   echo sub1 >.git/info/exclude &&
$command add_sub1 &&
test_superproject_content origin/add_sub1 &&
test_submodule_content sub1 origin/add_sub1
@@ -959,7 +959,7 @@ test_submodule_forced_switch_recursing_with_args () {
)
'
# ... absorbing a .git directory.
-   test_expect_success "$command: replace submodule containing a .git 
directory with a directory must fail" '
+   test_expect_failure "$command: replace submodule containing a .git 
directory with a directory must fail" '
prolog &&
reset_work_tree_to_interested add_sub1 &&
(
@@ -967,9 +967,9 @@ test_submodule_forced_switch_recursing_with_args () {
git branch -t replace_sub1_with_directory 
origin/replace_sub1_with_directory &&
replace_gitfile_with_git_dir sub1 &&
rm -rf .git/modules/sub1 &&
-   $command replace_sub1_with_directory &&
+   test_must_fail $command replace_sub1_with_directory &&
test_superproject_content 
origin/replace_sub1_with_directory &&
-   test_submodule_content sub1 origin/modify_sub1
+   test_submodule_content sub1 origin/modify_sub1 &&
test_git_directory_exists sub1
)
'
-- 
2.18.0.419.gfe4b301394

[PATCH 26/29] t7000-t7999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t7001-mv.sh  |  2 +-
 t/t7201-co.sh  | 40 +-
 t/t7400-submodule-basic.sh |  2 +-
 t/t7406-submodule-update.sh|  4 +--
 t/t7408-submodule-reference.sh |  2 +-
 t/t7501-commit.sh  | 52 +-
 t/t7506-status-submodule.sh| 10 +++
 7 files changed, 56 insertions(+), 56 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index cc3fd2baf2..9e59e5a5dd 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -509,7 +509,7 @@ test_expect_success 'moving nested submodules' '
touch nested_level1 &&
git init &&
git add . &&
-   git commit -m "nested level 1"
+   git commit -m "nested level 1" &&
git submodule add ../sub_nested_nested &&
git commit -m "add nested level 2"
) &&
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 8d8a63a24b..94cb039a03 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -528,10 +528,10 @@ test_expect_success 'checkout with --merge' '
cat sample >filf &&
git checkout -m -- fild file filf &&
(
-   echo "<<< ours"
-   echo ourside
-   echo "==="
-   echo theirside
+   echo "<<< ours" &&
+   echo ourside &&
+   echo "===" &&
+   echo theirside &&
echo ">>> theirs"
) >merged &&
test_cmp expect fild &&
@@ -549,12 +549,12 @@ test_expect_success 'checkout with --merge, in diff3 -m 
style' '
cat sample >filf &&
git checkout -m -- fild file filf &&
(
-   echo "<<< ours"
-   echo ourside
-   echo "||| base"
-   echo original
-   echo "==="
-   echo theirside
+   echo "<<< ours" &&
+   echo ourside &&
+   echo "||| base" &&
+   echo original &&
+   echo "===" &&
+   echo theirside &&
echo ">>> theirs"
) >merged &&
test_cmp expect fild &&
@@ -572,10 +572,10 @@ test_expect_success 'checkout --conflict=merge, 
overriding config' '
cat sample >filf &&
git checkout --conflict=merge -- fild file filf &&
(
-   echo "<<< ours"
-   echo ourside
-   echo "==="
-   echo theirside
+   echo "<<< ours" &&
+   echo ourside &&
+   echo "===" &&
+   echo theirside &&
echo ">>> theirs"
) >merged &&
test_cmp expect fild &&
@@ -593,12 +593,12 @@ test_expect_success 'checkout --conflict=diff3' '
cat sample >filf &&
git checkout --conflict=diff3 -- fild file filf &&
(
-   echo "<<< ours"
-   echo ourside
-   echo "||| base"
-   echo original
-   echo "==="
-   echo theirside
+   echo "<<< ours" &&
+   echo ourside &&
+   echo "||| base" &&
+   echo original &&
+   echo "===" &&
+   echo theirside &&
echo ">>> theirs"
) >merged &&
test_cmp expect fild &&
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 401adaed32..76cf522a08 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -818,7 +818,7 @@ test_expect_success '../bar/a/b/c works with relative local 
path - ../foo/bar.gi
cp pristine-.git-config .git/config &&
cp pristine-.gitmodules .gitmodules &&
mkdir -p a/b/c &&
-   (cd a/b/c; git init) &&
+   (cd a/b/c && git init) &&
git config remote.origin.url ../foo/bar.git &&
git submodule add ../bar/a/b/c ./a/b/c &&
git submodule init &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 9e0d31700e..b141145fc2 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -865,9 +865,9 @@ test_expect_success 'submodule update places git-dir in 
superprojects git-dir re
 (cd submodule/subsubmodule &&
  git log > ../../expected
 ) &&
-(cd .git/modules/submodule/modules/subsubmodule
+(cd .git/modules/submodule/modules/subsubmodule &&
  git log > ../../../../../actual
-)
+) &&
 test_cmp actual expected
)
 '
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 08d9add05e..34ac28c056 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -148,7 +148,7 @@ test_expect_success 'preparing second superproject with a 
nested submodule plus
cd sup

[PATCH 16/29] t: use test_might_fail() instead of manipulating exit code manually

[Eric Sunshine]

These tests manually coerce the exit code of invoked commands to
"success" when they don't care if the command succeeds or fails since
failure of those commands should not cause the test to fail overall.
Such manual management predates the invention of test_might_fail().

An upcoming change will teach --chain-lint to check the &&-chain inside
subshells. This sort of manual exit code manipulation will trip up
--chain-lint due to the intentional break in the &&-chain. Therefore,
replace manual exit code management with test_might_fail() and a normal
&&-chain.

Signed-off-by: Eric Sunshine 
---
 t/t1507-rev-parse-upstream.sh | 6 +++---
 t/t1700-split-index.sh| 2 +-
 t/t4012-diff-binary.sh| 6 ++
 t/t5400-send-pack.sh  | 4 ++--
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 93c77eac45..349f6e10af 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -123,9 +123,9 @@ test_expect_success 'checkout -b new my-side@{u} forks from 
the same' '
 
 test_expect_success 'merge my-side@{u} records the correct name' '
 (
-   cd clone || exit
-   git checkout master || exit
-   git branch -D new ;# can fail but is ok
+   cd clone &&
+   git checkout master &&
+   test_might_fail git branch -D new &&
git branch -t new my-side@{u} &&
git merge -s ours new@{u} &&
git show -s --pretty=tformat:%s >actual &&
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 1e81b33b2e..39133bcbc8 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -435,7 +435,7 @@ test_expect_success 'writing split index with null sha1 
does not write cache tre
commit=$(git commit-tree $tree -p HEAD cache-tree.out || true) &&
+   test_might_fail test-tool dump-cache-tree >cache-tree.out &&
test_line_count = 0 cache-tree.out
 '
 
diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 0a8af76aab..6579c81216 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -102,10 +102,8 @@ test_expect_success 'apply binary patch' '
 
 test_expect_success 'diff --no-index with binary creation' '
echo Q | q_to_nul >binary &&
-   (: hide error code from diff, which just indicates differences
-git diff --binary --no-index /dev/null binary >current ||
-true
-   ) &&
+   # hide error code from diff, which just indicates differences
+   test_might_fail git diff --binary --no-index /dev/null binary >current 
&&
rm binary &&
git apply --binary expected &&
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 911eae1bf7..f1932ea431 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -86,7 +86,7 @@ test_expect_success 'push can be used to delete a ref' '
 test_expect_success 'refuse deleting push with denyDeletes' '
(
cd victim &&
-   ( git branch -D extra || : ) &&
+   test_might_fail git branch -D extra &&
git config receive.denyDeletes true &&
git branch extra master
) &&
@@ -119,7 +119,7 @@ test_expect_success 'override denyDeletes with git -c 
receive-pack' '
 test_expect_success 'denyNonFastforwards trumps --force' '
(
cd victim &&
-   ( git branch -D extra || : ) &&
+   test_might_fail git branch -D extra &&
git config receive.denyNonFastforwards true
) &&
victim_orig=$(cd victim && git rev-parse --verify master) &&
-- 
2.18.0.419.gfe4b301394

[PATCH 25/29] t6000-t6999: fix broken &&-chains in subshells

[Eric Sunshine]

Signed-off-by: Eric Sunshine 
---
 t/t6010-merge-base.sh|  2 +-
 t/t6029-merge-subtree.sh | 16 
 t/t6036-recursive-corner-cases.sh|  6 +++---
 t/t6042-merge-rename-corner-cases.sh |  8 
 t/t6043-merge-rename-directories.sh  |  2 +-
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index aa2d360ce3..44c726ea39 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -245,7 +245,7 @@ test_expect_success 'using reflog to find the fork point' '
git commit --allow-empty -m "Derived #$count" &&
git rev-parse HEAD >derived$count &&
git checkout -B base $E || exit 1
-   done
+   done &&
 
for count in 1 2 3
do
diff --git a/t/t6029-merge-subtree.sh b/t/t6029-merge-subtree.sh
index 3e692454a7..7d5bc78472 100755
--- a/t/t6029-merge-subtree.sh
+++ b/t/t6029-merge-subtree.sh
@@ -55,7 +55,7 @@ test_expect_success 'initial merge' '
git checkout -b work &&
git ls-files -s >actual &&
(
-   echo "100644 $o1 0  git-gui/git-gui.sh"
+   echo "100644 $o1 0  git-gui/git-gui.sh" &&
echo "100644 $o2 0  git.c"
) >expected &&
test_cmp expected actual
@@ -72,7 +72,7 @@ test_expect_success 'merge update' '
git pull -s subtree gui master2 &&
git ls-files -s >actual &&
(
-   echo "100644 $o3 0  git-gui/git-gui.sh"
+   echo "100644 $o3 0  git-gui/git-gui.sh" &&
echo "100644 $o2 0  git.c"
) >expected &&
test_cmp expected actual
@@ -88,8 +88,8 @@ test_expect_success 'initial ambiguous subtree' '
git checkout -b work2 &&
git ls-files -s >actual &&
(
-   echo "100644 $o1 0  git-gui/git-gui.sh"
-   echo "100644 $o1 0  git-gui2/git-gui.sh"
+   echo "100644 $o1 0  git-gui/git-gui.sh" &&
+   echo "100644 $o1 0  git-gui2/git-gui.sh" &&
echo "100644 $o2 0  git.c"
) >expected &&
test_cmp expected actual
@@ -101,8 +101,8 @@ test_expect_success 'merge using explicit' '
git pull -Xsubtree=git-gui gui master2 &&
git ls-files -s >actual &&
(
-   echo "100644 $o3 0  git-gui/git-gui.sh"
-   echo "100644 $o1 0  git-gui2/git-gui.sh"
+   echo "100644 $o3 0  git-gui/git-gui.sh" &&
+   echo "100644 $o1 0  git-gui2/git-gui.sh" &&
echo "100644 $o2 0  git.c"
) >expected &&
test_cmp expected actual
@@ -114,8 +114,8 @@ test_expect_success 'merge2 using explicit' '
git pull -Xsubtree=git-gui2 gui master2 &&
git ls-files -s >actual &&
(
-   echo "100644 $o1 0  git-gui/git-gui.sh"
-   echo "100644 $o3 0  git-gui2/git-gui.sh"
+   echo "100644 $o1 0  git-gui/git-gui.sh" &&
+   echo "100644 $o3 0  git-gui2/git-gui.sh" &&
echo "100644 $o2 0  git.c"
) >expected &&
test_cmp expected actual
diff --git a/t/t6036-recursive-corner-cases.sh 
b/t/t6036-recursive-corner-cases.sh
index b32ff8e1db..892cf08743 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -72,7 +72,7 @@ test_expect_success 'merge simple rename+criss-cross with no 
modifications' '
git rev-parse   >actual \
:2:three   :3:three &&
git hash-object >>actual\
-   three~HEAD three~R2^0
+   three~HEAD three~R2^0 &&
test_cmp expect actual
)
 '
@@ -148,7 +148,7 @@ test_expect_success 'merge criss-cross + rename merges with 
basic modification'
git rev-parse   >actual \
:2:three   :3:three &&
git hash-object >>actual\
-   three~HEAD three~R2^0
+   three~HEAD three~R2^0 &&
test_cmp expect actual
)
 '
@@ -228,7 +228,7 @@ test_expect_success 'git detects differently handled merges 
conflict' '
D:new_a  E:new_a &&
git rev-parse   >actual \
:2:new_a :3:new_a &&
-   test_cmp expect actual
+   test_cmp expect actual &&
 
git cat-file -p B:new_a >ours &&
git cat-file -p C:new_a >theirs &&
diff --git a/t/t6042-merge-rename-corner-cases.sh 
b/t/t6042-merge-rename-corner-cases.sh
index 1cbd946fc2..661b633478 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -352,7 +352,7 @@ test_expect_success 'rename/directory conflict + content 
merge conflict' '
base:file

[PATCH 10/29] t9001: fix broken "invoke hook" test

[Eric Sunshine]

This test has been dysfunctional since it was added by 6489660b4b
(send-email: support validate hook, 2017-05-12), however, the problem
went unnoticed due to a broken &&-chain late in the test.

The test wants to verify that a non-zero exit code from the
'sendemail-validate' hook causes git-send-email to abort with a
particular error message. A command which is expected to fail should be
run with 'test_must_fail', however, the tests neglects to do so.

Fix this problem, as well as the broken &&-chain behind which the
problem hid.

Signed-off-by: Eric Sunshine 
---
 t/t9001-send-email.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index e80eacbb1b..776769fe0d 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1966,11 +1966,11 @@ test_expect_success $PREREQ 'invoke hook' '
 
# Verify error message when a patch is rejected by the hook
sed -e "s/add master/x/" ../0001-add-master.patch 
>../another.patch &&
-   git send-email \
+   test_must_fail git send-email \
--from="Example " \
--to=nob...@example.com \
--smtp-server="$(pwd)/../fake.sendmail" \
-   ../another.patch 2>err
+   ../another.patch 2>err &&
test_i18ngrep "rejected by sendemail-validate hook" err
)
 '
-- 
2.18.0.419.gfe4b301394

[PATCH 05/29] t5505: modernize and simplify hard-to-digest test

[Eric Sunshine]

This test uses a subshell within a subshell but is formatted in such a
way as to suggests that the inner subshell is a sibling rather than a
child, which makes it difficult to digest the test's structure and
intent.

Worse, the inner subshell performs cleanup of actions from earlier in
the test, however, a failure between the initial actions and the cleanup
will prevent the cleanup from taking place.

Fix these problems by modernizing and simplifying the test and by using
test_when_finished() for the cleanup action.

Signed-off-by: Eric Sunshine 
---
 t/t5505-remote.sh | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index a6c0178f3a..3552b51b4c 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -348,17 +348,13 @@ URL: $(pwd)/one
 EOF
 
 test_expect_success 'prune --dry-run' '
-   (
-   cd one &&
-   git branch -m side2 side) &&
+   git -C one branch -m side2 side &&
+   test_when_finished "git -C one branch -m side side2" &&
(
cd test &&
git remote prune --dry-run origin >output &&
git rev-parse refs/remotes/origin/side2 &&
test_must_fail git rev-parse refs/remotes/origin/side &&
-   (
-   cd ../one &&
-   git branch -m side side2) &&
test_i18ncmp expect output
)
 '
-- 
2.18.0.419.gfe4b301394

[PATCH 06/29] t6036: fix broken "merge fails but has appropriate contents" tests

[Eric Sunshine]

These tests reference non-existent object "c" when they really mean to
be referencing "C", however, this error went unnoticed due to a broken
&&-chain later in the test. Fix these errors, as well as the broken
&&-chains behind which they hid.

Signed-off-by: Eric Sunshine 
---
 t/t6036-recursive-corner-cases.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t6036-recursive-corner-cases.sh 
b/t/t6036-recursive-corner-cases.sh
index b5621303d6..b32ff8e1db 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -506,10 +506,10 @@ test_expect_success 'merge of D & E2 fails but has 
appropriate contents' '
test_line_count = 2 out &&
 
git rev-parse >expect\
-   B:a   E2:a/file  c:a/file   A:ignore-me &&
+   B:a   E2:a/file  C:a/file   A:ignore-me &&
git rev-parse   >actual   \
:2:a  :3:a/file  :1:a/file  :0:ignore-me &&
-   test_cmp expect actual
+   test_cmp expect actual &&
 
test_path_is_file a~HEAD
)
@@ -533,10 +533,10 @@ test_expect_success 'merge of E2 & D fails but has 
appropriate contents' '
test_line_count = 2 out &&
 
git rev-parse >expect\
-   B:a   E2:a/file  c:a/file   A:ignore-me &&
+   B:a   E2:a/file  C:a/file   A:ignore-me &&
git rev-parse   >actual   \
:3:a  :2:a/file  :1:a/file  :0:ignore-me &&
-   test_cmp expect actual
+   test_cmp expect actual &&
 
test_path_is_file a~D^0
)
-- 
2.18.0.419.gfe4b301394

[PATCH 11/29] t9104: use "{...}" block around "||" expression rather than subshell

[Eric Sunshine]

This test uses "(... || svn ...)" as a shorthand for an if-then-else
statement. The subshell prevents it from breaking the top-level
&&-chain.

However, an upcoming change will teach --chain-lint to check the
&&-chain inside subshells. Although it takes special care to allow
"||" inside subshells, it only recognizes "(... || git ...)" and
"(... || test*), so the "||" in this test will trip up --chain-lint.

A test later in this same script employs the same "... || svn ..."
construct, however, it wraps it in a "{...}" block instead of a
subshell. Therefore, rather than adding "(... || svn ...)" as a yet
another --chain-lint special case, follow suit and make this test use
"{...}", as well.

Signed-off-by: Eric Sunshine 
---
 t/t9104-git-svn-follow-parent.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index 5e0ad19177..f9aa734d4e 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -53,10 +53,10 @@ test_expect_success 'init and fetch from one svn-remote' '
 '
 
 test_expect_success 'follow deleted parent' '
-(svn_cmd cp -m "resurrecting trunk as junk" \
+   { svn_cmd cp -m "resurrecting trunk as junk" \
"$svnrepo"/trunk@2 "$svnrepo"/junk ||
  svn cp -m "resurrecting trunk as junk" \
-   -r2 "$svnrepo"/trunk "$svnrepo"/junk) &&
+   -r2 "$svnrepo"/trunk "$svnrepo"/junk; } &&
 git config --add svn-remote.svn.fetch \
   junk:refs/remotes/svn/junk &&
 git svn fetch -i svn/thunk &&
-- 
2.18.0.419.gfe4b301394

[PATCH 08/29] t7400: fix broken "submodule add/reconfigure --force" test

[Eric Sunshine]

This test has been dysfunctional since it was added by 619acfc78c
(submodule add: extend force flag to add existing repos, 2016-10-06),
however, two problems early in the test went unnoticed due to a broken
&&-chain later in the test.

First, it tries configuring the submodule with repository "bogus-url",
however, "git submodule add" insists that the repository be either an
absolute URL or a relative pathname requiring prefix "./" or "../" (this
is true even with --force), but "bogus-url" does not meet those
criteria, thus the command fails.

Second, it then tries configuring a submodule with a path which is
.gitignore'd, which is disallowed. This restriction can be overridden
with --force, but the test neglects to use that option.

Fix both problems, as well as the broken &&-chain behind which they hid.

Signed-off-by: Eric Sunshine 
---
 t/t7400-submodule-basic.sh | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 812db137b8..401adaed32 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -171,12 +171,12 @@ test_expect_success 'submodule add to .gitignored path 
with --force' '
 test_expect_success 'submodule add to reconfigure existing submodule with 
--force' '
(
cd addtest-ignore &&
-   git submodule add --force bogus-url submod &&
-   git submodule add -b initial "$submodurl" submod-branch &&
-   test "bogus-url" = "$(git config -f .gitmodules 
submodule.submod.url)" &&
-   test "bogus-url" = "$(git config submodule.submod.url)" &&
+   git submodule add --force /bogus-url submod &&
+   git submodule add --force -b initial "$submodurl" submod-branch 
&&
+   test "/bogus-url" = "$(git config -f .gitmodules 
submodule.submod.url)" &&
+   test "/bogus-url" = "$(git config submodule.submod.url)" &&
# Restore the url
-   git submodule add --force "$submodurl" submod
+   git submodule add --force "$submodurl" submod &&
test "$submodurl" = "$(git config -f .gitmodules 
submodule.submod.url)" &&
test "$submodurl" = "$(git config submodule.submod.url)"
)
-- 
2.18.0.419.gfe4b301394

[PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Eric Sunshine]

These tests intentionally break the &&-chain to manually check the exit
code of invoked commands which they expect to fail, and invert that
local expected failure into a successful exit code for the test overall.
Such manual exit code manipulation predates the invention of
test_must_fail().

An upcoming change will teach --chain-lint to check the &&-chain inside
subshells. This sort of manual exit code checking will trip up
--chain-lint due to the intentional break in the &&-chain. Therefore,
replace the manual exit code management with test_must_fail() and a
normal &&-chain.

Signed-off-by: Eric Sunshine 
---
 t/t5405-send-pack-rewind.sh |  3 +--
 t/t9814-git-p4-rename.sh| 16 ++--
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
index 4bda18a662..235fb7686a 100755
--- a/t/t5405-send-pack-rewind.sh
+++ b/t/t5405-send-pack-rewind.sh
@@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not segfault' 
'
 
(
cd another &&
-   git push .. master:master
-   test $? = 1
+   test_must_fail git push .. master:master
)
 
 '
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index e7e0268e98..80aac5ab16 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -12,20 +12,8 @@ test_expect_success 'start p4d' '
 test_expect_success 'p4 help unknown returns 1' '
(
cd "$cli" &&
-   (
-   p4 help client >errs 2>&1
-   echo $? >retval
-   )
-   echo 0 >expected &&
-   test_cmp expected retval &&
-   rm retval &&
-   (
-   p4 help nosuchcommand >errs 2>&1
-   echo $? >retval
-   )
-   echo 1 >expected &&
-   test_cmp expected retval &&
-   rm retval
+   p4 help client &&
+   test_must_fail p4 help nosuchcommand
)
 '
 
-- 
2.18.0.419.gfe4b301394

[PATCH 07/29] t7201: drop pointless "exit 0" at end of subshell

[Eric Sunshine]

This test employs a for-loop inside a subshell and correctly aborts the
loop and fails the test overall (via "exit 1") if any iteration of the
for-loop fails. Otherwise, it exits the subshell with an explicit
"exit 0", presumably to indicate that all iterations of the for-loop
succeeded. The &&-chain is (perhaps deliberately) broken between the
for-loop and the "exit 0".

An upcoming change will teach --chain-lint to detect &&-chain breakage
inside subshells, thus the missing &&-chain link will run afoul of
--chain-lint. Rather than fixing the &&-chain breakage, instead just
drop the unnecessary "exit 0".

Signed-off-by: Eric Sunshine 
---
 t/t7201-co.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index ab9da61da3..8d8a63a24b 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -673,7 +673,6 @@ test_expect_success 'custom merge driver with checkout -m' '
do
grep $t arm || exit 1
done
-   exit 0
) &&
 
mv arm expect &&
-- 
2.18.0.419.gfe4b301394

Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Luke Diamand]

On 26 June 2018 at 08:29, Eric Sunshine  wrote:
> These tests intentionally break the &&-chain to manually check the exit
> code of invoked commands which they expect to fail, and invert that
> local expected failure into a successful exit code for the test overall.
> Such manual exit code manipulation predates the invention of
> test_must_fail().
>
> An upcoming change will teach --chain-lint to check the &&-chain inside
> subshells. This sort of manual exit code checking will trip up
> --chain-lint due to the intentional break in the &&-chain. Therefore,
> replace the manual exit code management with test_must_fail() and a
> normal &&-chain.
>
> Signed-off-by: Eric Sunshine 
> ---
>  t/t5405-send-pack-rewind.sh |  3 +--
>  t/t9814-git-p4-rename.sh| 16 ++--
>  2 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
> index 4bda18a662..235fb7686a 100755
> --- a/t/t5405-send-pack-rewind.sh
> +++ b/t/t5405-send-pack-rewind.sh
> @@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not 
> segfault' '
>
> (
> cd another &&
> -   git push .. master:master
> -   test $? = 1
> +   test_must_fail git push .. master:master
> )
>
>  '
> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> index e7e0268e98..80aac5ab16 100755
> --- a/t/t9814-git-p4-rename.sh
> +++ b/t/t9814-git-p4-rename.sh
> @@ -12,20 +12,8 @@ test_expect_success 'start p4d' '
>  test_expect_success 'p4 help unknown returns 1' '
> (
> cd "$cli" &&
> -   (
> -   p4 help client >errs 2>&1
> -   echo $? >retval
> -   )
> -   echo 0 >expected &&
> -   test_cmp expected retval &&
> -   rm retval &&
> -   (
> -   p4 help nosuchcommand >errs 2>&1
> -   echo $? >retval
> -   )
> -   echo 1 >expected &&
> -   test_cmp expected retval &&
> -   rm retval
> +   p4 help client &&
> +   test_must_fail p4 help nosuchcommand

This seems a lot more sensible. It works fine in my tests.

Thanks,
Luke

Re: [PATCH 06/29] t6036: fix broken "merge fails but has appropriate contents" tests

[Elijah Newren]

On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  wrote:
> These tests reference non-existent object "c" when they really mean to
> be referencing "C", however, this error went unnoticed due to a broken
> &&-chain later in the test. Fix these errors, as well as the broken
> &&-chains behind which they hid.
>
> Signed-off-by: Eric Sunshine 
> ---
>  t/t6036-recursive-corner-cases.sh | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t6036-recursive-corner-cases.sh 
> b/t/t6036-recursive-corner-cases.sh
> index b5621303d6..b32ff8e1db 100755
> --- a/t/t6036-recursive-corner-cases.sh
> +++ b/t/t6036-recursive-corner-cases.sh
> @@ -506,10 +506,10 @@ test_expect_success 'merge of D & E2 fails but has 
> appropriate contents' '
> test_line_count = 2 out &&
>
> git rev-parse >expect\
> -   B:a   E2:a/file  c:a/file   A:ignore-me &&
> +   B:a   E2:a/file  C:a/file   A:ignore-me &&
> git rev-parse   >actual   \
> :2:a  :3:a/file  :1:a/file  :0:ignore-me &&
> -   test_cmp expect actual
> +   test_cmp expect actual &&
>
> test_path_is_file a~HEAD
> )
> @@ -533,10 +533,10 @@ test_expect_success 'merge of E2 & D fails but has 
> appropriate contents' '
> test_line_count = 2 out &&
>
> git rev-parse >expect\
> -   B:a   E2:a/file  c:a/file   A:ignore-me &&
> +   B:a   E2:a/file  C:a/file   A:ignore-me &&
> git rev-parse   >actual   \
> :3:a  :2:a/file  :1:a/file  :0:ignore-me &&
> -   test_cmp expect actual
> +   test_cmp expect actual &&
>
> test_path_is_file a~D^0
> )

Eek, how did that become c:a/file when it was originally C:a/file?
Thanks for spotting the regression and fixing.

Reviewed-by: Elijah Newren 

Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Elijah Newren]

On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  wrote:
> These tests intentionally break the &&-chain to manually check the exit
> code of invoked commands which they expect to fail, and invert that
> local expected failure into a successful exit code for the test overall.
> Such manual exit code manipulation predates the invention of
> test_must_fail().
>
> An upcoming change will teach --chain-lint to check the &&-chain inside
> subshells. This sort of manual exit code checking will trip up
> --chain-lint due to the intentional break in the &&-chain. Therefore,
> replace the manual exit code management with test_must_fail() and a
> normal &&-chain.
>
> Signed-off-by: Eric Sunshine 
> ---
>  t/t5405-send-pack-rewind.sh |  3 +--
>  t/t9814-git-p4-rename.sh| 16 ++--
>  2 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
> index 4bda18a662..235fb7686a 100755
> --- a/t/t5405-send-pack-rewind.sh
> +++ b/t/t5405-send-pack-rewind.sh
> @@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not 
> segfault' '
>
> (
> cd another &&
> -   git push .. master:master
> -   test $? = 1
> +   test_must_fail git push .. master:master

test_must_fail or test_expect_code 1?

> )
>
>  '
> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> index e7e0268e98..80aac5ab16 100755
> --- a/t/t9814-git-p4-rename.sh
> +++ b/t/t9814-git-p4-rename.sh
> @@ -12,20 +12,8 @@ test_expect_success 'start p4d' '
>  test_expect_success 'p4 help unknown returns 1' '
> (
> cd "$cli" &&
> -   (
> -   p4 help client >errs 2>&1
> -   echo $? >retval
> -   )
> -   echo 0 >expected &&
> -   test_cmp expected retval &&
> -   rm retval &&
> -   (
> -   p4 help nosuchcommand >errs 2>&1
> -   echo $? >retval
> -   )
> -   echo 1 >expected &&
> -   test_cmp expected retval &&
> -   rm retval
> +   p4 help client &&
> +   test_must_fail p4 help nosuchcommand

same question?

> )
>  '

Overall, looks much nicer.

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Pratik Karki]

Hi,

On Tue, Jun 26, 2018 at 3:00 AM Johannes Schindelin
 wrote:

> Pratik refactored some code from sequencer.c into checkout.c/checkout.h
> today to do exactly that. It is not polished yet, but probably will be
> tomorrow. It provides a function `int detach_head_to(struct object_oid
> *oid, const char *action, const char *reflog_message)`. Maybe use that
> directly, once the commit is available?

The commit is here[1].

[1]: 
https://github.com/git/git/pull/505/commits/47031d4706bd1eccd2816ecdaaf6e9cd700909aa

Cheers,
Pratik

Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells

[Elijah Newren]

On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  wrote:
> The --chain-lint[1] option detects breakage in the top-level &&-chain of
> tests. This series undertakes the more complex task of teaching it to
> also detect &&-chain breakage within subshells. See patch 29/29 for the
> gory details of how that's done.

This is awesome.  It'll replace a hacky script I had that attempted to
remind me when I messed up, and fix its false positives to boot.
Plus, I won't forget to run it.

> Aside from identifying a rather significant number of &&-chain breaks,
> repairing those broken chains uncovered genuine bugs in several tests
> which were hidden by missing &&-chain links. Those bugs are also fixed
> by this series. I would appreciate if the following people would
> double-check my fixes:
>
> Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update"
> Jonathan Tan - 10/29 "t9001"
> Elijah Newren - 6/29 "t6036"

Commented on the patch in question; 6/29 looks good.

I also looked over the rest of the series.  Apart from the ones you
specifically called out as needing review by others besides me, and
the final patch which makes me feel like a sed neophyte, all but one
patch looked good to me.  I just have a small question for that
remaining patch, which I posted there.

Thanks,
Elijah

Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Eric Sunshine]

On Tue, Jun 26, 2018 at 4:58 AM Elijah Newren  wrote:
> On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  
> wrote:
> > [...] Therefore,
> > replace the manual exit code management with test_must_fail() and a
> > normal &&-chain.
> >
> > Signed-off-by: Eric Sunshine 
> > ---
> > diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
> > @@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not 
> > segfault' '> > -   git push .. master:master
> > -   test $? = 1
> > +   test_must_fail git push .. master:master
>
> test_must_fail or test_expect_code 1?

A legitimate question, and the answer is that it's a judgment call
based upon the spirit of the test.

Although test_expect_code() would make for a faithful literal
conversion, I don't think it agrees with the spirit of this test,
which wants to verify that the command correctly exits with an error
(in the general sense), as opposed to outright crashing (which it did
at one time). test_must_fail() is tailor-made for this use case.

Contrast this with patch 09/29 "t7810: use test_expect_code() instead
of hand-rolled comparison". Different exit codes from git-grep have
genuine different meanings, and that test is checking for a very
specific exit code, for which test_expect_code() is tailor-made.

> > diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> > @@ -12,20 +12,8 @@ test_expect_success 'start p4d' '
> >  test_expect_success 'p4 help unknown returns 1' '
> > (
> > cd "$cli" &&
> > -   (
> > -   p4 help client >errs 2>&1
> > -   echo $? >retval
> > -   )
> > -   echo 0 >expected &&
> > -   test_cmp expected retval &&
> > -   rm retval &&
> > -   (
> > -   p4 help nosuchcommand >errs 2>&1
> > -   echo $? >retval
> > -   )
> > -   echo 1 >expected &&
> > -   test_cmp expected retval &&
> > -   rm retval
> > +   p4 help client &&
> > +   test_must_fail p4 help nosuchcommand
>
> same question?

Same answer. Not shown in this patch, but just above the context lines
you will find this comment in the file:

# We rely on this behavior to detect for p4 move availability.

which means that the test is really interested in being able to
reliably detect if a sub-command is or is not available. So, despite
the (somewhat) misleading test title, this test doesn't care about the
exact error code but rather cares only that "p4 help nosuchcommand"
errors out, period. Hence, test_must_fail() again agrees with the
spirit of the test.

ag/rebase-i-append-todo-help, was Re: What's cooking in git.git (Jun 2018, #06; Mon, 25)

[Johannes Schindelin]

Hi Junio & Alban,

On Mon, 25 Jun 2018, Junio C Hamano wrote:

> * ag/rebase-i-append-todo-help (2018-06-14) 2 commits
>  - rebase--interactive: rewrite append_todo_help() in C
>  - Merge branch 'ag/rebase-p' into ag/rebase-i-append-todo-help
>  (this branch is used by ag/rebase-i-rewrite-todo.)
> 
>  Stepwise rewriting of the machinery of "rebase -i" into C continues.

I just reviewed this on GitHub (which gives me an interactive way to look
around outside of the diff context) and I think there are two things left
to do (which I mentioned to Alban on IRC):

- `msg = _(...)` should be `msg = N_(...)` instead, with the corresponding
  `_(msg)` later on, and

- to avoid cluttering sequencer.c, and to pave the way for future `rebase
  -i`-specific code, the `append_todo_help()` function should go into a
  new file called `interactive-rebase.c`.

Ciao,
Dscho

Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells

[Eric Sunshine]

On Tue, Jun 26, 2018 at 5:20 AM Elijah Newren  wrote:
> On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  
> wrote:
> > Aside from identifying a rather significant number of &&-chain breaks,
> > repairing those broken chains uncovered genuine bugs in several tests
> > which were hidden by missing &&-chain links. Those bugs are also fixed
> > by this series. I would appreciate if the following people would
> > double-check my fixes:
> >
> > Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update"
> > Jonathan Tan - 10/29 "t9001"
> > Elijah Newren - 6/29 "t6036"
>
> Commented on the patch in question; 6/29 looks good.
>
> I also looked over the rest of the series.  Apart from the ones you
> specifically called out as needing review by others besides me, and
> the final patch which makes me feel like a sed neophyte, all but one
> patch looked good to me.  I just have a small question for that
> remaining patch, which I posted there.

I guess you refer to your question[1] about whether test_must_fail()
is the correct choice over test_expect_code(). I just responded[2]
with a hopefully satisfactory answer.

[1]: 
https://public-inbox.org/git/CABPp-BFmfN6=e+3bakt-nh5hmu-368shgdnrnkrnmrvknx0...@mail.gmail.com/
[2]: 
https://public-inbox.org/git/CAPig+cRTG625H3CF1Zw30vQt2W8uKf1xLxVaQni2YbJ=xai...@mail.gmail.com/

[GSoC][PATCH 0/1] sequencer: print an error message if append_todo_help() fails

[Alban Gruin]

Currently, append_todo_help() does not warn the user if an error occurs
when trying to write to the todo file.  This patch addresses this
problem.

This patch is based on ag/rebase-i-append-todo-help.

Alban Gruin (1):
  sequencer: print an error message if append_todo_help() fails

 sequencer.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.18.0

[GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails

[Alban Gruin]

This adds an error when append_todo_help() fails to write its message to
the todo file.

Signed-off-by: Alban Gruin 
---
 sequencer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 7cc76332e..7c4bdbb99 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4380,6 +4380,9 @@ int append_todo_help(unsigned edit_todo, unsigned 
keep_empty)
}
 
ret = fputs(buf.buf, todo);
+   if (ret)
+   error_errno(_("Could not append help text to '%s'"), 
rebase_path_todo());
+
fclose(todo);
strbuf_release(&buf);
 
-- 
2.18.0

curious about wording in "man git-config", ENVIRONMENT

[Robert P. J. Day]

  ENVIRONMENT
GIT_CONFIG
  Take the configuration from the given file instead of
  .git/config. Using the "--global" option forces this to
  ~/.gitconfig. Using the "--system" option forces this to
  $(prefix)/etc/gitconfig.

  is the phrase "forces this to" really what you want to use here?
maybe i misunderstand what this option does, doesn't it simply mean
that it will use a different (specified) file from the default,
depending on the context (local, global, system)?

  it just seems weird to say that the option "forces" the use of what
are clearly the default files. thoughts?

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

[PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules

[Antonio Ospite]

Add a helper function to make it clearer that retrieving 'fetch'
configuration from the .gitmodules file is a special case supported
solely for backward compatibility purposes.

This change removes one direct use of 'config_from_gitmodules' in code
not strictly related to submodules, in the effort to communicate better
that .gitmodules is not to be used as a mechanism to store arbitrary
configuration in the repository that any command can retrieve.

Signed-off-by: Antonio Ospite 
---
 builtin/fetch.c| 15 +--
 submodule-config.c | 28 
 submodule-config.h |  2 ++
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..92a5d235d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -93,19 +93,6 @@ static int git_fetch_config(const char *k, const char *v, 
void *cb)
return git_default_config(k, v, cb);
 }
 
-static int gitmodules_fetch_config(const char *var, const char *value, void 
*cb)
-{
-   if (!strcmp(var, "submodule.fetchjobs")) {
-   max_children = parse_submodule_fetchjobs(var, value);
-   return 0;
-   } else if (!strcmp(var, "fetch.recursesubmodules")) {
-   recurse_submodules = parse_fetch_recurse_submodules_arg(var, 
value);
-   return 0;
-   }
-
-   return 0;
-}
-
 static int parse_refmap_arg(const struct option *opt, const char *arg, int 
unset)
 {
/*
@@ -1433,7 +1420,7 @@ int cmd_fetch(int argc, const char **argv, const char 
*prefix)
for (i = 1; i < argc; i++)
strbuf_addf(&default_rla, " %s", argv[i]);
 
-   config_from_gitmodules(gitmodules_fetch_config, NULL);
+   fetch_config_from_gitmodules(&max_children, &recurse_submodules);
git_config(git_fetch_config, NULL);
 
argc = parse_options(argc, argv, prefix,
diff --git a/submodule-config.c b/submodule-config.c
index b431555db..f44d6a777 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -688,3 +688,31 @@ void config_from_gitmodules(config_fn_t fn, void *data)
free(file);
}
 }
+
+struct fetch_config {
+   int *max_children;
+   int *recurse_submodules;
+};
+
+static int gitmodules_fetch_config(const char *var, const char *value, void 
*cb)
+{
+   struct fetch_config *config = cb;
+   if (!strcmp(var, "submodule.fetchjobs")) {
+   *(config->max_children) = parse_submodule_fetchjobs(var, value);
+   return 0;
+   } else if (!strcmp(var, "fetch.recursesubmodules")) {
+   *(config->recurse_submodules) = 
parse_fetch_recurse_submodules_arg(var, value);
+   return 0;
+   }
+
+   return 0;
+}
+
+void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
+{
+   struct fetch_config config = {
+   .max_children = max_children,
+   .recurse_submodules = recurse_submodules
+   };
+   config_from_gitmodules(gitmodules_fetch_config, &config);
+}
diff --git a/submodule-config.h b/submodule-config.h
index 5148801f4..cff297a75 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -66,4 +66,6 @@ int check_submodule_name(const char *name);
  */
 extern void config_from_gitmodules(config_fn_t fn, void *data);
 
+extern void fetch_config_from_gitmodules(int *max_children, int 
*recurse_submodules);
+
 #endif /* SUBMODULE_CONFIG_H */
-- 
2.18.0

[PATCH v2 4/6] submodule-config: make 'config_from_gitmodules' private

[Antonio Ospite]

Now that 'config_from_gitmodules' is not used in the open, it can be
marked as private.

Hopefully this will prevent its usage for retrieving arbitrary
configuration form the '.gitmodules' file.

Signed-off-by: Antonio Ospite 
---
 submodule-config.c |  8 
 submodule-config.h | 12 +---
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 9a2b13d8b..cd1f1e06a 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -673,14 +673,14 @@ void submodule_free(struct repository *r)
 }
 
 /*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
+ * Note: This function is private for a reason, the '.gitmodules' file should
+ * not be used as as a mechanism to retrieve arbitrary configuration stored in
+ * the repository.
  *
  * Runs the provided config function on the '.gitmodules' file found in the
  * working directory.
  */
-void config_from_gitmodules(config_fn_t fn, void *data)
+static void config_from_gitmodules(config_fn_t fn, void *data)
 {
if (the_repository->worktree) {
char *file = repo_worktree_path(the_repository, 
GITMODULES_FILE);
diff --git a/submodule-config.h b/submodule-config.h
index b6f19d0d4..dc7278eea 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -57,15 +57,13 @@ void submodule_free(struct repository *r);
 int check_submodule_name(const char *name);
 
 /*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
+ * Note: these helper functions exist solely to maintain backward
+ * compatibility with 'fetch' and 'update_clone' storing configuration in
+ * '.gitmodules'.
  *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
+ * New helpers to retrieve arbitrary configuration from the '.gitmodules' file
+ * should NOT be added.
  */
-extern void config_from_gitmodules(config_fn_t fn, void *data);
-
 extern void fetch_config_from_gitmodules(int *max_children, int 
*recurse_submodules);
 extern void update_clone_config_from_gitmodules(int *max_jobs);
 
-- 
2.18.0

[PATCH v2 1/6] config: move config_from_gitmodules to submodule-config.c

[Antonio Ospite]

The .gitmodules file is not meant as a place to store arbitrary
configuration to distribute with the repository.

Move config_from_gitmodules() out of config.c and into
submodule-config.c to make it even clearer that it is not a mechanism to
retrieve arbitrary configuration from the .gitmodules file.

Signed-off-by: Antonio Ospite 
---
 config.c   | 17 -
 config.h   | 10 --
 submodule-config.c | 17 +
 submodule-config.h | 11 +++
 4 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/config.c b/config.c
index a0a6ae198..fa78b1ff9 100644
--- a/config.c
+++ b/config.c
@@ -2172,23 +2172,6 @@ int git_config_get_pathname(const char *key, const char 
**dest)
return repo_config_get_pathname(the_repository, key, dest);
 }
 
-/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-void config_from_gitmodules(config_fn_t fn, void *data)
-{
-   if (the_repository->worktree) {
-   char *file = repo_worktree_path(the_repository, 
GITMODULES_FILE);
-   git_config_from_file(fn, file, data);
-   free(file);
-   }
-}
-
 int git_config_get_expiry(const char *key, const char **output)
 {
int ret = git_config_get_string_const(key, output);
diff --git a/config.h b/config.h
index 626d4654b..b95bb7649 100644
--- a/config.h
+++ b/config.h
@@ -215,16 +215,6 @@ extern int repo_config_get_maybe_bool(struct repository 
*repo,
 extern int repo_config_get_pathname(struct repository *repo,
const char *key, const char **dest);
 
-/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-extern void config_from_gitmodules(config_fn_t fn, void *data);
-
 extern int git_config_get_value(const char *key, const char **value);
 extern const struct string_list *git_config_get_value_multi(const char *key);
 extern void git_config_clear(void);
diff --git a/submodule-config.c b/submodule-config.c
index 388ef1f89..b431555db 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -671,3 +671,20 @@ void submodule_free(struct repository *r)
if (r->submodule_cache)
submodule_cache_clear(r->submodule_cache);
 }
+
+/*
+ * Note: This function exists solely to maintain backward compatibility with
+ * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
+ * NOT be used anywhere else.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+void config_from_gitmodules(config_fn_t fn, void *data)
+{
+   if (the_repository->worktree) {
+   char *file = repo_worktree_path(the_repository, 
GITMODULES_FILE);
+   git_config_from_file(fn, file, data);
+   free(file);
+   }
+}
diff --git a/submodule-config.h b/submodule-config.h
index ca1f94e2d..5148801f4 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -2,6 +2,7 @@
 #define SUBMODULE_CONFIG_CACHE_H
 
 #include "cache.h"
+#include "config.h"
 #include "hashmap.h"
 #include "submodule.h"
 #include "strbuf.h"
@@ -55,4 +56,14 @@ void submodule_free(struct repository *r);
  */
 int check_submodule_name(const char *name);
 
+/*
+ * Note: This function exists solely to maintain backward compatibility with
+ * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
+ * NOT be used anywhere else.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+extern void config_from_gitmodules(config_fn_t fn, void *data);
+
 #endif /* SUBMODULE_CONFIG_H */
-- 
2.18.0

[PATCH v2 6/6] submodule-config: reuse config_from_gitmodules in repo_read_gitmodules

[Antonio Ospite]

Reuse config_from_gitmodules in repo_read_gitmodules to remove some
duplication and also have a single point where the .gitmodules file is
read.

The change does not introduce any new behavior, the same gitmodules_cb
config callback is still used, which only deals with configuration
specific to submodules.

The check about the repo's worktree is removed from repo_read_gitmodules
because it's already performed in config_from_gitmodules.

The config_from_gitmodules function is moved up in the file —unchanged—
before its users to avoid a forward declaration.

Signed-off-by: Antonio Ospite 
---
 submodule-config.c | 50 +++---
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 602c46af2..77421a497 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -591,6 +591,23 @@ static void submodule_cache_check_init(struct repository 
*repo)
submodule_cache_init(repo->submodule_cache);
 }
 
+/*
+ * Note: This function is private for a reason, the '.gitmodules' file should
+ * not be used as as a mechanism to retrieve arbitrary configuration stored in
+ * the repository.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+static void config_from_gitmodules(config_fn_t fn, struct repository *repo, 
void *data)
+{
+   if (repo->worktree) {
+   char *file = repo_worktree_path(repo, GITMODULES_FILE);
+   git_config_from_file(fn, file, data);
+   free(file);
+   }
+}
+
 static int gitmodules_cb(const char *var, const char *value, void *data)
 {
struct repository *repo = data;
@@ -608,19 +625,11 @@ void repo_read_gitmodules(struct repository *repo)
 {
submodule_cache_check_init(repo);
 
-   if (repo->worktree) {
-   char *gitmodules;
-
-   if (repo_read_index(repo) < 0)
-   return;
-
-   gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
-
-   if (!is_gitmodules_unmerged(repo->index))
-   git_config_from_file(gitmodules_cb, gitmodules, repo);
+   if (repo_read_index(repo) < 0)
+   return;
 
-   free(gitmodules);
-   }
+   if (!is_gitmodules_unmerged(repo->index))
+   config_from_gitmodules(gitmodules_cb, repo, repo);
 
repo->submodule_cache->gitmodules_read = 1;
 }
@@ -672,23 +681,6 @@ void submodule_free(struct repository *r)
submodule_cache_clear(r->submodule_cache);
 }
 
-/*
- * Note: This function is private for a reason, the '.gitmodules' file should
- * not be used as as a mechanism to retrieve arbitrary configuration stored in
- * the repository.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-static void config_from_gitmodules(config_fn_t fn, struct repository *repo, 
void *data)
-{
-   if (repo->worktree) {
-   char *file = repo_worktree_path(repo, GITMODULES_FILE);
-   git_config_from_file(fn, file, data);
-   free(file);
-   }
-}
-
 struct fetch_config {
int *max_children;
int *recurse_submodules;
-- 
2.18.0

[PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules

[Antonio Ospite]

Generlize config_from_gitmodules to accept a repository as an argument.

This is in preparation to reuse the function in repo_read_gitmodules in
order to have a single point where the '.gitmodules' file is accessed.

Signed-off-by: Antonio Ospite 
---
 submodule-config.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index cd1f1e06a..602c46af2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -680,10 +680,10 @@ void submodule_free(struct repository *r)
  * Runs the provided config function on the '.gitmodules' file found in the
  * working directory.
  */
-static void config_from_gitmodules(config_fn_t fn, void *data)
+static void config_from_gitmodules(config_fn_t fn, struct repository *repo, 
void *data)
 {
-   if (the_repository->worktree) {
-   char *file = repo_worktree_path(the_repository, 
GITMODULES_FILE);
+   if (repo->worktree) {
+   char *file = repo_worktree_path(repo, GITMODULES_FILE);
git_config_from_file(fn, file, data);
free(file);
}
@@ -714,7 +714,7 @@ void fetch_config_from_gitmodules(int *max_children, int 
*recurse_submodules)
.max_children = max_children,
.recurse_submodules = recurse_submodules
};
-   config_from_gitmodules(gitmodules_fetch_config, &config);
+   config_from_gitmodules(gitmodules_fetch_config, the_repository, 
&config);
 }
 
 static int gitmodules_update_clone_config(const char *var, const char *value,
@@ -728,5 +728,5 @@ static int gitmodules_update_clone_config(const char *var, 
const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-   config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
+   config_from_gitmodules(gitmodules_update_clone_config, the_repository, 
&max_jobs);
 }
-- 
2.18.0

[PATCH v2 3/6] submodule-config: add helper to get 'update-clone' config from .gitmodules

[Antonio Ospite]

Add a helper function to make it clearer that retrieving 'update-clone'
configuration from the .gitmodules file is a special case supported
solely for backward compatibility purposes.

This change removes one direct use of 'config_from_gitmodules' for
options not strictly related to submodules: "submodule.fetchjobs" does
not describe a property of a submodule, but a behavior of other commands
when dealing with submodules, so it does not really belong to the
.gitmodules file.

This is in the effort to communicate better that .gitmodules is not to
be used as a mechanism to store arbitrary configuration in the
repository that any command can retrieve.

Signed-off-by: Antonio Ospite 
---
 builtin/submodule--helper.c |  8 
 submodule-config.c  | 14 ++
 submodule-config.h  |  1 +
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 20ae9191c..110a47eca 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1706,8 +1706,8 @@ static int update_clone_task_finished(int result,
return 0;
 }
 
-static int gitmodules_update_clone_config(const char *var, const char *value,
- void *cb)
+static int git_update_clone_config(const char *var, const char *value,
+  void *cb)
 {
int *max_jobs = cb;
if (!strcmp(var, "submodule.fetchjobs"))
@@ -1757,8 +1757,8 @@ static int update_clone(int argc, const char **argv, 
const char *prefix)
};
suc.prefix = prefix;
 
-   config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
-   git_config(gitmodules_update_clone_config, &max_jobs);
+   update_clone_config_from_gitmodules(&max_jobs);
+   git_config(git_update_clone_config, &max_jobs);
 
argc = parse_options(argc, argv, prefix, module_update_clone_options,
 git_submodule_helper_usage, 0);
diff --git a/submodule-config.c b/submodule-config.c
index f44d6a777..9a2b13d8b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -716,3 +716,17 @@ void fetch_config_from_gitmodules(int *max_children, int 
*recurse_submodules)
};
config_from_gitmodules(gitmodules_fetch_config, &config);
 }
+
+static int gitmodules_update_clone_config(const char *var, const char *value,
+ void *cb)
+{
+   int *max_jobs = cb;
+   if (!strcmp(var, "submodule.fetchjobs"))
+   *max_jobs = parse_submodule_fetchjobs(var, value);
+   return 0;
+}
+
+void update_clone_config_from_gitmodules(int *max_jobs)
+{
+   config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
+}
diff --git a/submodule-config.h b/submodule-config.h
index cff297a75..b6f19d0d4 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -67,5 +67,6 @@ int check_submodule_name(const char *name);
 extern void config_from_gitmodules(config_fn_t fn, void *data);
 
 extern void fetch_config_from_gitmodules(int *max_children, int 
*recurse_submodules);
+extern void update_clone_config_from_gitmodules(int *max_jobs);
 
 #endif /* SUBMODULE_CONFIG_H */
-- 
2.18.0

[PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config

[Antonio Ospite]

Hi,

this is version 2 of the series from
https://public-inbox.org/git/20180622162656.19338-1-...@ao2.it/

The .gitmodules file is not meant for arbitrary configuration, it should
be used only for submodules properties.

Plus, arbitrary git configuration should not be distributed with the
repository, and .gitmodules might be a possible "vector" for that.

The series tries to alleviate both these issues by moving the
'config_from_gitmodules' function from config.[ch] to submodule-config.c
and making it private.

This should discourage future code from using the function with
arbitrary config callbacks which might turn .gitmodules into a mechanism
to load arbitrary configuration stored in the repository.

Backward compatibility exceptions to the rules above are handled by
ad-hoc helpers.

Finally (in patch 6) some duplication is removed by using
'config_from_gitmodules' to load the submodules configuration in
'repo_read_gitmodules'.

Changes since v1:
  * Remove an extra space before an arrow operator in patch 2
  * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs
  * Add a note in the commit message of patch 6 about checking the
worktree before loading .gitmodules
  * Drop patch 7, it was meant as a cleanup but resulted in parsing the
.gitmodules file twice

The series has been rebased on commit ed843436d ("First batch for 2.19
cycle", 2018-06-25) , and the test suite passes after each commit.

Thanks to Brandon Williams and Stefan Beller for the input.

Ciao,
   Antonio

Antonio Ospite (6):
  config: move config_from_gitmodules to submodule-config.c
  submodule-config: add helper function to get 'fetch' config from
.gitmodules
  submodule-config: add helper to get 'update-clone' config from
.gitmodules
  submodule-config: make 'config_from_gitmodules' private
  submodule-config: pass repository as argument to
config_from_gitmodules
  submodule-config: reuse config_from_gitmodules in repo_read_gitmodules

 builtin/fetch.c | 15 +---
 builtin/submodule--helper.c |  8 ++--
 config.c| 17 -
 config.h| 10 -
 submodule-config.c  | 75 +++--
 submodule-config.h  | 12 ++
 6 files changed, 80 insertions(+), 57 deletions(-)

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: What's cooking in git.git (Jun 2018, #06; Mon, 25)

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 26, 2018 at 12:47 AM, Junio C Hamano  wrote:

Happy 2.18.0 everyone!

> * ab/fetch-tags-noclobber (2018-05-16) 9 commits
>  - fixup! push tests: assert re-pushing annotated tags
>  - fetch: stop clobbering existing tags without --force
>  - fetch tests: add a test clobbering tag behavior
>  - fetch tests: correct a comment "remove it" -> "remove them"
>  - push doc: correct lies about how push refspecs work
>  - push tests: assert re-pushing annotated tags
>  - push tests: add more testing for forced tag pushing
>  - push tests: fix logic error in "push" test assertion
>  - push tests: remove redundant 'git push' invocation
>
>  Expecting a reboot of the discussion to take it to some conclusion
>  and then a reroll.
>  cf. 
>  cf. 
>  cf. 
>  cf. 

I'm on vacation these days and won't submit a re-roll of this for 2-3
weeks, so I think it's best to eject it. I have a WIP re-roll of this.

> * ab/checkout-default-remote (2018-06-11) 8 commits
>  - checkout & worktree: introduce checkout.defaultRemote
>  - checkout: add advice for ambiguous "checkout "
>  - builtin/checkout.c: use "ret" variable for return
>  - checkout: pass the "num_matches" up to callers
>  - checkout.c: change "unique" member to "num_matches"
>  - checkout.c: introduce an *_INIT macro
>  - checkout.h: wrap the arguments to unique_tracking_name()
>  - checkout tests: index should be clean after dwim checkout

It would be nice to have this merged down. The only nit of the last
version was SZEDER's suggestion of using a test helper in
<20180605154501.13502-1-szeder@gmail.com>.

I agree, but as noted I won't be able to re-roll this for some time,
and it would be nice to have it in next or master by then, I can then
submit a tiny series on top to fix that "use a test helper" issue,
which is something that can be improved in both the patch I'm
submitting & in several existing places in the test suite, so I think
it makes sense to address that as a subsequent cleanup series
unrelated to this new feature.

Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect

[Christian Couder]

On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano  wrote:
> Tiago Botelho  writes:
>
>> +test_expect_success "--bisect-all --first-parent" '
>> +cat >expect1 <> +$(git rev-parse CC) (dist=2)
>> +$(git rev-parse EX) (dist=1)
>> +$(git rev-parse D) (dist=1)
>> +$(git rev-parse FX) (dist=0)
>> +EOF
>> +
>> +cat >expect2 <> +$(git rev-parse CC) (dist=2)
>> +$(git rev-parse D) (dist=1)
>> +$(git rev-parse EX) (dist=1)
>> +$(git rev-parse FX) (dist=0)
>> +EOF
>> +
>> +git rev-list --bisect-all --first-parent FX ^A >actual &&
>> +  ( test_cmp expect1 actual || test_cmp expect2 actual )
>> +'
>
> I hate to say this, but the above looks like a typical
> unmaintainable mess.
>
> What happens when you or somebody else later needs to update the
> graph to be tested to add one more commit (or even more)?  Would it
> be enough to add another "rev-parse" plus "dist=X" line in both
> expects?  Or do we see a trap for combinatorial explosion that
> requires us to add new expect$N?

What about the following then:

test_dist_order () {
file="$1"
n="$2"
while read -r hash dist
do
d=$(echo "$dist" | sed -e "s/(dist=\(.*\))/\1/")
case "$d" in
''|*[!0-9]*) return 1 ;;
*) ;;
esac
test "$d" -le "$n" || return 1
n="$d"
done <"$file"
}

test_expect_success "--bisect-all --first-parent" '
cat >expect actual &&
sort actual >actual_sorted &&
test_cmp expect_sorted actual_sorted &&
test_dist_order actual 2
'

This feels overkill to me, but it should scale if we ever make more
complex tests.

Re: [BUG] url schemes should be case-insensitive

[Jeff King]

On Mon, Jun 25, 2018 at 11:19:51AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > We seem to match url schemes case-sensitively:
> >
> >   $ git clone SSH://example.com/repo.git
> >   Cloning into 'repo'...
> >   fatal: Unable to find remote helper for 'SSH'
> >
> > whereas rfc3986 is clear that the scheme portion is case-insensitive.
> > We probably ought to match at least our internal ones with strcasecmp.
> 
> That may break if somebody at DevToolGroup@$BIGCOMPANY got cute and
> named their custom remote helper SSH:// that builds on top of the
> normal ssh:// protocol with something extra and gave it to their
> developers (and they named the http counterpart that has the same
> extra HTTP://, of course).

True, though I am on the fence whether that is a property worth
maintaining. AFAIK it was not planned and is just a "this is how it
happened to work" case that is (IMHO) doing the wrong thing.

c.f. https://xkcd.com/1172/

Is this a hypothetical, or do you know of a $BIGCOMPANY that uses this?
The only similar case I know of is contrib/persistent-https, where we
realized the better way to do it is:

  [url "my-custom-ssh://"]
  insteadOf = ssh://

> If we probe for git-remote-SSH first and
> then fall back to git-remote-ssh, then we won't break these people,
> though.  I agree that it may be a good bite-sized #leftoverbit
> material.

This is probably a little tricky to implement, since there is no
git-remote-ssh, and we handle it internally in a totally separate
code path. So "probe for git-remote-SSH" is probably not "try to run
it", but really "search the PATH for something plausible". That may be
good enough.

It may also interact in a funny way with our allowed-protocol code, if
"SSH" gets a pass as "ssh" under the default config, but actually runs
the otherwise-disallowed git-remote-SSH (though one would _hope_ if you
have such a git-remote-SSH that it behaves just like an ssh remote).

> > We could probably also give an advise() message in the above output,
> > suggesting that the problem is likely one of:
> >
> >   1. They misspelled the scheme.
> >
> >   2. They need to install the appropriate helper.
> >
> > This may be a good topic for somebody looking for low-hanging fruit to
> > get involved in development (I'd maybe call it a #leftoverbits, but
> > since I didn't start on it, I'm not sure if it counts as "left over" ;)).
> [..]
> It may probably be a good idea to do an advice, but I'd think
> "Untable to find remote helper for 'SSH'" may be clear enough.  If
> anything, perhaps saying "remote helper for 'SSH' protocol" would
> make it even clear?  I dunno.

I think it doesn't help much if the user does not know what a remote
helper is, or why Git is looking for one. Though at least it gives a
term to start searching for, I guess. The original motivation for my
mail was the real-world report at:

  https://github.com/git/git-scm.com/issues/1219

-Peff

Re: curious about wording in "man git-config", ENVIRONMENT

[Jeff King]

On Tue, Jun 26, 2018 at 06:18:26AM -0400, Robert P. J. Day wrote:

> 
>   ENVIRONMENT
> GIT_CONFIG
>   Take the configuration from the given file instead of
>   .git/config. Using the "--global" option forces this to
>   ~/.gitconfig. Using the "--system" option forces this to
>   $(prefix)/etc/gitconfig.
> 
>   is the phrase "forces this to" really what you want to use here?
> maybe i misunderstand what this option does, doesn't it simply mean
> that it will use a different (specified) file from the default,
> depending on the context (local, global, system)?
> 
>   it just seems weird to say that the option "forces" the use of what
> are clearly the default files. thoughts?

I agree it's weird. I think it's trying to mean "behaves as if it was
set to", but with the additional notion that the command-line argument
would take precedence over the environment (which is our usual rule).
But then we should just say those things explicitly.

Just looking at mentions of GIT_CONFIG in that manpage and knowing the
history, I think:

 - the environment section should say something like:

 GIT_CONFIG
   If set and no other specific-file options are given, behaves as
   if `--file=$GIT_CONFIG` was provided on the command-line.

 - possibly the manpage should mention that GIT_CONFIG is historical and
   should not be used in new code (we could also consider an actual
   deprecation period and removal of the feature, though aside from
   documentation confusion I do not think it is hurting anyone)

 - the description of --file should not mention it at all. Instead it
   should reference the "FILES" section which describes the whole lookup
   sequence

 - mention of GIT_CONFIG should be dropped from the FILES section. We
   don't want to point people towards using it. And if they come across
   it in the wild, they can find it in the ENVIRONMENT section.

 - references to "--global" should stop mentioning ~/.gitconfig,
   since in a post-XDG world it could be elsewhere (they're better to
   refer to the "--global" description or the FILES section)

 - references to "--system" should stop mentioning $(prefix)/etc/gitconfig,
   since it can be configured separately from the prefix (and in most
   packaged builds which set prefix=/usr, $(sysconfdir) is not
   $(prefix)/etc).

-Peff

Re: [GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails

[Johannes Schindelin]

Hi Alban,

On Tue, 26 Jun 2018, Alban Gruin wrote:

> This adds an error when append_todo_help() fails to write its message to
> the todo file.
> 
> Signed-off-by: Alban Gruin 

ACK.

We *may* want to fold that into the commit that adds `append_todo_help()`.
And, as I mentioned previously, I would love for that function to be used
as an excuse to introduce the long-overdue `interactive-rebase.c`
(`sequencer.c` is supposed to be the backend for cherry-pick and revert
and interactive rebase, but not a catch-all for *all* of those things, it
is already way too long to be readable, and I take blame for a large part
of that.)

Ciao,
Dscho

git rerere and diff3

[Nicolas Dechesne]

hi there,

i have noticed that merge.conflictstyle has an impact on the rerere
resolution. looking briefly at the source code, it seems that git
tries to discard the common ancestor diff3 bits, but what I am seeing
is that if i do the following then it fails:

1. from a clean rr-cache state, with merge.conflictsytle=diff3, git
merge , resolve the conflicts, then commit
2. undo the previous merge, remove merge.conflictstyle=diff3 (disable
diff3) and merge the *same* branch, then rerere won't fix the
conflicts

Is that the expected behavior? I am not familiar with git code, but
browsing rerere.c makes me think that it should have worked..

Of course, if I merge the same branch without modifying
merge.conflictstyle, then the merge conflicts are properly resolved by
rerere.

thanks!

Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect

[Johannes Schindelin]

Hi Chris,

On Tue, 26 Jun 2018, Christian Couder wrote:

> On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano  wrote:
> > Tiago Botelho  writes:
> >
> >> +test_expect_success "--bisect-all --first-parent" '
> >> +cat >expect1 < >> +$(git rev-parse CC) (dist=2)
> >> +$(git rev-parse EX) (dist=1)
> >> +$(git rev-parse D) (dist=1)
> >> +$(git rev-parse FX) (dist=0)
> >> +EOF
> >> +
> >> +cat >expect2 < >> +$(git rev-parse CC) (dist=2)
> >> +$(git rev-parse D) (dist=1)
> >> +$(git rev-parse EX) (dist=1)
> >> +$(git rev-parse FX) (dist=0)
> >> +EOF
> >> +
> >> +git rev-list --bisect-all --first-parent FX ^A >actual &&
> >> +  ( test_cmp expect1 actual || test_cmp expect2 actual )
> >> +'
> >
> > I hate to say this, but the above looks like a typical
> > unmaintainable mess.
> >
> > What happens when you or somebody else later needs to update the
> > graph to be tested to add one more commit (or even more)?  Would it
> > be enough to add another "rev-parse" plus "dist=X" line in both
> > expects?  Or do we see a trap for combinatorial explosion that
> > requires us to add new expect$N?
> 
> What about the following then:
> 
> test_dist_order () {
> file="$1"
> n="$2"
> while read -r hash dist
> do
> d=$(echo "$dist" | sed -e "s/(dist=\(.*\))/\1/")
> case "$d" in
> ''|*[!0-9]*) return 1 ;;
> *) ;;
> esac
> test "$d" -le "$n" || return 1
> n="$d"
> done <"$file"
> }
> 
> test_expect_success "--bisect-all --first-parent" '
> cat >expect < $(git rev-parse CC) (dist=2)
> $(git rev-parse EX) (dist=1)
> $(git rev-parse D) (dist=1)
> $(git rev-parse FX) (dist=0)
> EOF
> sort expect >expect_sorted &&
> git rev-list --bisect-all --first-parent FX ^A >actual &&
> sort actual >actual_sorted &&
> test_cmp expect_sorted actual_sorted &&
> test_dist_order actual 2
> '
> 
> This feels overkill to me, but it should scale if we ever make more
> complex tests.

I *think* that you misunderstand Junio. At least when I read this:

> $(git rev-parse CC) (dist=2)
> $(git rev-parse EX) (dist=1)
> $(git rev-parse D) (dist=1)
> $(git rev-parse FX) (dist=0)

I go: Huh?!?!?!?!?!

What's CC? Is it Christian Couder? Creative Commons? Crudely Complex?

The point, for me, is: if this test fails, at some stage in the future,
for any reason, it will be a major pain to even dissect what the test is
supposed to do. This is no good. And you can do better. A lot better. You
can write the test in a way that the head of a reader does not hurt, and
at the same time it is still obvious what it does, and obvious that it
does the right thing.

One thing that makes the brain stumble for certain is when you deviate
from the conventions, especially when it is for no good reason at all. In
this case (and I am not sure why you, as a long-time contributor, did not
spot this before public review):

- the titles of the test cases leave a lot of room for improvement,

- the lines are too long,

- the convention to end the `test_expect_success` line with an opening
  single quote is not heeded,

- the graph is definitely written in an unnecessarily different format
  than in the same test script!!!

- speaking of the graph: there is a perfectly fine commit graph already.
  Why on earth is it not reused?

In this particular case it even feels as if this test is not even testing
what it should test at all:

- it should verify that all of the commits in the first parent lineage are
  part of the list

- it should verify that none of the other commits are in the list

And that is really all there is to test. You can even write that in a much
easier way:

-- snip --
test_expect_success '--first-parent --bisect-all lists correct revs' '
git rev-list --first-parent --bisect-all F..E >revs &&
# E and e1..e8 need to be shown, F and f1..f4 not
test_line_count = 9 revs &&
for rev in E e1 e2 e3 e4 e5 e6 e7 e8
do
grep "^$(git rev-parse $rev) " revs || {
echo "$rev not shown" >&2 &&
return 1
}
done
'
-- snap --

To test more precisely for the order or the distance would be both
overkill and likely to be unreadable.

To test `--bisect-vars` here would be excessive, as the part that handles
that particular option is not even touched. All that is touched is the
logic in the bisect algorithm in conjunction with --first-parent. And that
is all that should be tested here.

With a test like the one I outlined above, I only have one more gripe
about the patch: the commit message does nothing to explain this part of
the diff:

+   if ((bisect_flags & BISECT_FIRST_PARENT)) {
+   if (weight(q) < 0)
+   q = NULL;
+   break;
+   }

And I would really, really like that to be explained in the comm

Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells

[Elijah Newren]

Hi Eric,

On Tue, Jun 26, 2018, 2:31 AM Eric Sunshine  wrote:
>
> On Tue, Jun 26, 2018 at 5:20 AM Elijah Newren  wrote:
> > On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  
> > wrote:
> > > Aside from identifying a rather significant number of &&-chain breaks,
> > > repairing those broken chains uncovered genuine bugs in several tests
> > > which were hidden by missing &&-chain links. Those bugs are also fixed
> > > by this series. I would appreciate if the following people would
> > > double-check my fixes:
> > >
> > > Stefan Bellar - 8/29 "t7400" and (especially) 13/29 "lib-submodule-update"
> > > Jonathan Tan - 10/29 "t9001"
> > > Elijah Newren - 6/29 "t6036"
> >
> > Commented on the patch in question; 6/29 looks good.
> >
> > I also looked over the rest of the series.  Apart from the ones you
> > specifically called out as needing review by others besides me, and
> > the final patch which makes me feel like a sed neophyte, all but one
> > patch looked good to me.  I just have a small question for that
> > remaining patch, which I posted there.
>
> I guess you refer to your question[1] about whether test_must_fail()
> is the correct choice over test_expect_code(). I just responded[2]
> with a hopefully satisfactory answer.

Yes, it does.  Thanks!

Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect

[Christian Couder]

Hi Dscho,

On Tue, Jun 26, 2018 at 4:10 PM, Johannes Schindelin
 wrote:
>
> On Tue, 26 Jun 2018, Christian Couder wrote:
>
>> On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano  wrote:
>>
>> > I hate to say this, but the above looks like a typical
>> > unmaintainable mess.
>> >
>> > What happens when you or somebody else later needs to update the
>> > graph to be tested to add one more commit (or even more)?  Would it
>> > be enough to add another "rev-parse" plus "dist=X" line in both
>> > expects?  Or do we see a trap for combinatorial explosion that
>> > requires us to add new expect$N?
>>
>> What about the following then:
>>
>> test_dist_order () {
>> file="$1"
>> n="$2"
>> while read -r hash dist
>> do
>> d=$(echo "$dist" | sed -e "s/(dist=\(.*\))/\1/")
>> case "$d" in
>> ''|*[!0-9]*) return 1 ;;
>> *) ;;
>> esac
>> test "$d" -le "$n" || return 1
>> n="$d"
>> done <"$file"
>> }
>>
>> test_expect_success "--bisect-all --first-parent" '
>> cat >expect <> $(git rev-parse CC) (dist=2)
>> $(git rev-parse EX) (dist=1)
>> $(git rev-parse D) (dist=1)
>> $(git rev-parse FX) (dist=0)
>> EOF
>> sort expect >expect_sorted &&
>> git rev-list --bisect-all --first-parent FX ^A >actual &&
>> sort actual >actual_sorted &&
>> test_cmp expect_sorted actual_sorted &&
>> test_dist_order actual 2
>> '
>>
>> This feels overkill to me, but it should scale if we ever make more
>> complex tests.
>
> I *think* that you misunderstand Junio. At least when I read this:
>
>> $(git rev-parse CC) (dist=2)
>> $(git rev-parse EX) (dist=1)
>> $(git rev-parse D) (dist=1)
>> $(git rev-parse FX) (dist=0)
>
> I go: Huh?!?!?!?!?!
>
> What's CC? Is it Christian Couder? Creative Commons? Crudely Complex?

I agree that the name of the commit could be improved.

> The point, for me, is: if this test fails, at some stage in the future,
> for any reason, it will be a major pain to even dissect what the test is
> supposed to do.

I think the test is quite simple and there is an ascii picture, so it
is not so difficult to understand.

> This is no good. And you can do better. A lot better. You
> can write the test in a way that the head of a reader does not hurt, and
> at the same time it is still obvious what it does, and obvious that it
> does the right thing.

Obviousness is often not the same for everybody.

> One thing that makes the brain stumble for certain is when you deviate
> from the conventions, especially when it is for no good reason at all. In
> this case (and I am not sure why you, as a long-time contributor, did not
> spot this before public review):

Please note that I never committed myself (like most of us who are not
maintainer actually) to reviewing everything bisect related (or
everything that Tiago works on).

> - the titles of the test cases leave a lot of room for improvement,
>
> - the lines are too long,
>
> - the convention to end the `test_expect_success` line with an opening
>   single quote is not heeded,

If you take a look at the beginning of the script you will see that
there are those problems there too.

I know that we should try to do better, but here I am mostly
interested in moving forward a feature that people have requested for
ages, not cleaning up those tests. If someone else like you or Junio
thinks that it would be a good time to clean things up a bit, then I
am ok with it, but that's not my main goal here.

> - the graph is definitely written in an unnecessarily different format
>   than in the same test script!!!

Just above you complain about things that are similar to the previous
tests and now you complain about things that are different...

> - speaking of the graph: there is a perfectly fine commit graph already.
>   Why on earth is it not reused?

Perhaps because it is more complex than needed to test this feature
and/or to understand what happens. And I don't think we require
everywhere only one commit graph per test script.

> In this particular case it even feels as if this test is not even testing
> what it should test at all:
>
> - it should verify that all of the commits in the first parent lineage are
>   part of the list

It does that.

> - it should verify that none of the other commits are in the list

It does that too.

> And that is really all there is to test.

I don't agree. I think that when possible, especially when testing
plumbing commands like rev-list, it is a good thing to test as many
things as possible at once.

> You can even write that in a much
> easier way:
>
> -- snip --
> test_expect_success '--first-parent --bisect-all lists correct revs' '
> git rev-list --first-parent --bisect-all F..E >revs &&
> # E and e1..e8 need to be shown, F and f1..f4 not
> test_line_count = 9 revs &&
> for rev in E e1 e2 e3 e4 e5 e6 e7 e8
> do
> grep "^$(git rev-parse $rev) " revs || {
> echo "$rev not shown"

[GSoC][PATCH v4 0/2] rebase -i: rewrite append_todo_help() in C

[Alban Gruin]

This patch rewrites append_todo_help() from shell to C. The C version
covers a bit more than the old shell version. To achieve that, some
parameters were added to rebase--helper.

This also introduce a new source file, rebase-interactive.c.

This is part of the effort to rewrite interactive rebase in C.

This is based on next, as of 2018-06-26.

Changes since v3:

 - Show an error message when append_todo_help() fails to edit the todo
   list.

 - Introducing rebase-interactive.c to contain functions necessary for
   interactive rebase.

Alban Gruin (2):
  sequencer: make two functions and an enum from sequencer.c public
  rebase--interactive: rewrite append_todo_help() in C

 Makefile   |  1 +
 builtin/rebase--helper.c   | 11 --
 git-rebase--interactive.sh | 52 ++---
 rebase-interactive.c   | 68 ++
 rebase-interactive.h   |  6 
 sequencer.c|  8 ++---
 sequencer.h|  6 
 7 files changed, 94 insertions(+), 58 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h

-- 
2.18.0

[GSoC][PATCH v4 2/2] rebase--interactive: rewrite append_todo_help() in C

[Alban Gruin]

This rewrites append_todo_help() from shell to C. It also incorporates
some parts of initiate_action() and complete_action() that also write
help texts to the todo file.

This also introduces the source file rebase-interactive.c. This file
will contain functions necessary for interactive rebase that are too
specific for the sequencer, and is part of libgit.a.

Two flags are added to rebase--helper.c: one to call
append_todo_help() (`--append-todo-help`), and another one to tell
append_todo_help() to write the help text suited for the edit-todo
mode (`--write-edit-todo`).

Finally, append_todo_help() is removed from git-rebase--interactive.sh
to use `rebase--helper --append-todo-help` instead.

Signed-off-by: Alban Gruin 
---
 Makefile   |  1 +
 builtin/rebase--helper.c   | 11 --
 git-rebase--interactive.sh | 52 ++---
 rebase-interactive.c   | 68 ++
 rebase-interactive.h   |  6 
 5 files changed, 86 insertions(+), 52 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h

diff --git a/Makefile b/Makefile
index 0cb6590f2..a281139ef 100644
--- a/Makefile
+++ b/Makefile
@@ -922,6 +922,7 @@ LIB_OBJS += protocol.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase-interactive.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..05e73e71d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "parse-options.h"
 #include "sequencer.h"
+#include "rebase-interactive.h"
 
 static const char * const builtin_rebase_helper_usage[] = {
N_("git rebase--helper []"),
@@ -12,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+   unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
= 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC
+   ADD_EXEC, APPEND_TODO_HELP
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +28,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge 
commits")),
OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
 N_("keep original branch points of cousins")),
+   OPT_BOOL(0, "write-edit-todo", &write_edit_todo,
+N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -45,6 +48,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_CMDMODE(0, "add-exec-commands", &command,
N_("insert exec commands in todo list"), ADD_EXEC),
+   OPT_CMDMODE(0, "append-todo-help", &command,
+   N_("insert the help in the todo list"), 
APPEND_TODO_HELP),
OPT_END()
};
 
@@ -84,5 +89,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
+   if (command == APPEND_TODO_HELP && argc == 1)
+   return !!append_todo_help(write_edit_todo, keep_empty);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..94c23a7af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
esac
 }
 
-append_todo_help () {
-   gettext "
-Commands:
-p, pick  = use commit
-r, reword  = use commit, but edit the commit message
-e, edit  = use commit, but stop for amending
-s, squash  = use commit, but meld into previous commit
-f, fixup  = like \"squash\", but discard this commit's log message
-x, exec  = run command (the rest of the line) using shell
-d, drop  = remove commit
-l, label  = label current HEAD with a name
-t, reset  = reset HEAD to a label
-m, merge [-C  | -c ]  [# ]
-.   create a merge commit using the original merge commit's
-.   messa

[GSoC][PATCH v4 1/2] sequencer: make two functions and an enum from sequencer.c public

[Alban Gruin]

This makes rebase_path_todo(), get_missign_commit_check_level() and the
enum check_level accessible outside sequencer.c.  This will be needed
for the rewrite of append_todo_help() from shell to C, as it will be in
a new library source file, rebase-interactive.c.

Signed-off-by: Alban Gruin 
---
 sequencer.c | 8 ++--
 sequencer.h | 6 ++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0a291c91f..881a4f7f4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -52,7 +52,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
  * the lines are processed, they are removed from the front of this
  * file and written to the tail of 'done'.
  */
-static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
+GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 /*
  * The rebase command lines that have already been processed. A line
  * is moved here when it is first handled, before any associated user
@@ -4223,11 +4223,7 @@ int transform_todos(unsigned flags)
return i;
 }
 
-enum check_level {
-   CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
-};
-
-static enum check_level get_missing_commit_check_level(void)
+enum check_level get_missing_commit_check_level(void)
 {
const char *value;
 
diff --git a/sequencer.h b/sequencer.h
index c5787c6b5..08397b0d1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -3,6 +3,7 @@
 
 const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
+const char *rebase_path_todo(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
@@ -57,6 +58,10 @@ struct replay_opts {
 };
 #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
 
+enum check_level {
+   CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
+};
+
 /* Call this to setup defaults before parsing command line options */
 void sequencer_init_config(struct replay_opts *opts);
 int sequencer_pick_revisions(struct replay_opts *opts);
@@ -79,6 +84,7 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
 
 int sequencer_add_exec_commands(const char *command);
 int transform_todos(unsigned flags);
+enum check_level get_missing_commit_check_level(void);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.18.0

[GSoC][PATCH v3 2/2] rebase-interactive: rewrite the edit-todo functionality in C

[Alban Gruin]

This rewrites the edit-todo functionality from shell to C.

To achieve that, a new command mode, `edit-todo`, is added, and the
`write-edit-todo` flag is removed, as the shell script does not need to
write the edit todo help message to the todo list anymore.

The shell version is then stripped in favour of a call to the helper.

Signed-off-by: Alban Gruin 
---
 builtin/rebase--helper.c   | 13 -
 git-rebase--interactive.sh | 11 +--
 rebase-interactive.c   | 31 +++
 rebase-interactive.h   |  1 +
 4 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 05e73e71d..731a64971 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
= 0;
+   unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC, APPEND_TODO_HELP
+   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -28,8 +28,6 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge 
commits")),
OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
 N_("keep original branch points of cousins")),
-   OPT_BOOL(0, "write-edit-todo", &write_edit_todo,
-N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +48,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("insert exec commands in todo list"), ADD_EXEC),
OPT_CMDMODE(0, "append-todo-help", &command,
N_("insert the help in the todo list"), 
APPEND_TODO_HELP),
+   OPT_CMDMODE(0, "edit-todo", &command,
+   N_("edit the todo list during an interactive 
rebase"),
+   EDIT_TODO),
OPT_END()
};
 
@@ -90,6 +91,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
if (command == APPEND_TODO_HELP && argc == 1)
-   return !!append_todo_help(write_edit_todo, keep_empty);
+   return !!append_todo_help(0, keep_empty);
+   if (command == EDIT_TODO && argc == 1)
+   return !!edit_todo_list(flags);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 94c23a7af..2defe607f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -108,16 +108,7 @@ initiate_action () {
 --continue
;;
edit-todo)
-   git stripspace --strip-comments <"$todo" >"$todo".new
-   mv -f "$todo".new "$todo"
-   collapse_todo_ids
-   git rebase--helper --append-todo-help --write-edit-todo
-
-   git_sequence_editor "$todo" ||
-   die "$(gettext "Could not execute editor")"
-   expand_todo_ids
-
-   exit
+   exec git rebase--helper --edit-todo
;;
show-current-patch)
exec git show REBASE_HEAD --
diff --git a/rebase-interactive.c b/rebase-interactive.c
index c79c819b9..ace8e095b 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -66,3 +66,34 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
 
return ret;
 }
+
+int edit_todo_list(unsigned flags)
+{
+   struct strbuf buf = STRBUF_INIT;
+   const char *todo_file = rebase_path_todo();
+   FILE *todo;
+
+   if (strbuf_read_file(&buf, todo_file, 0) < 0)
+   return error_errno(_("could not read '%s'."), todo_file);
+
+   strbuf_stripspace(&buf, 1);
+   todo = fopen_or_warn(todo_file, "w");
+   if (!todo) {
+   strbuf_release(&buf);
+   return 1;
+   }
+
+   strbuf_write(&buf, todo);
+   fclose(todo);
+   strbuf_release(&buf);
+
+   transform_todos(flags | TODO_LIST_SHORTEN_IDS);
+   ap

[GSoC][PATCH v3 0/2] rebase -i: rewrite the edit-todo functionality in C

[Alban Gruin]

This patch rewrites the edit-todo functionality from shell to C. This is
part of the effort to rewrite interactive rebase in C.

This patch is based on the fourth iteration of my series rewriting
append_todo_help() in C.

Changes since v2:

 - Moving edit_todo() from sequencer.c to interactive-rebase.c.

Alban Gruin (2):
  editor: add a function to launch the sequence editor
  rebase-interactive: rewrite the edit-todo functionality in C

 builtin/rebase--helper.c   | 13 -
 cache.h|  1 +
 editor.c   | 27 +--
 git-rebase--interactive.sh | 11 +--
 rebase-interactive.c   | 31 +++
 rebase-interactive.h   |  1 +
 strbuf.h   |  2 ++
 7 files changed, 69 insertions(+), 17 deletions(-)

-- 
2.18.0

[GSoC][PATCH v3 1/2] editor: add a function to launch the sequence editor

[Alban Gruin]

As part of the rewrite of interactive rebase, the sequencer will need to
open the sequence editor to allow the user to edit the todo list.
Instead of duplicating the existing launch_editor() function, this
refactors it to a new function, launch_specified_editor(), which takes
the editor as a parameter, in addition to the path, the buffer and the
environment variables.  launch_sequence_editor() is then added to launch
the sequence editor.

Signed-off-by: Alban Gruin 
---
 cache.h  |  1 +
 editor.c | 27 +--
 strbuf.h |  2 ++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 8b447652a..d70ae49ca 100644
--- a/cache.h
+++ b/cache.h
@@ -1409,6 +1409,7 @@ extern const char *fmt_name(const char *name, const char 
*email);
 extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
+extern const char *git_sequence_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
diff --git a/editor.c b/editor.c
index 9a9b4e12d..c985eee1f 100644
--- a/editor.c
+++ b/editor.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "config.h"
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
@@ -34,10 +35,21 @@ const char *git_editor(void)
return editor;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const 
*env)
+const char *git_sequence_editor(void)
 {
-   const char *editor = git_editor();
+   const char *editor = getenv("GIT_SEQUENCE_EDITOR");
+
+   if (!editor)
+   git_config_get_string_const("sequence.editor", &editor);
+   if (!editor)
+   editor = git_editor();
 
+   return editor;
+}
+
+static int launch_specified_editor(const char *editor, const char *path,
+  struct strbuf *buffer, const char *const 
*env)
+{
if (!editor)
return error("Terminal is dumb, but EDITOR unset");
 
@@ -95,3 +107,14 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
return error_errno("could not read file '%s'", path);
return 0;
 }
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const 
*env)
+{
+   return launch_specified_editor(git_editor(), path, buffer, env);
+}
+
+int launch_sequence_editor(const char *path, struct strbuf *buffer,
+  const char *const *env)
+{
+   return launch_specified_editor(git_sequence_editor(), path, buffer, 
env);
+}
diff --git a/strbuf.h b/strbuf.h
index 60a35aef1..66da9822f 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -575,6 +575,8 @@ extern void strbuf_add_unique_abbrev(struct strbuf *sb,
  * file's contents are not read into the buffer upon completion.
  */
 extern int launch_editor(const char *path, struct strbuf *buffer, const char 
*const *env);
+extern int launch_sequence_editor(const char *path, struct strbuf *buffer,
+ const char *const *env);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char 
*buf, size_t size);
 
-- 
2.18.0

Re: [PATCH 4/5] commit-graph: store graph in struct object_store

[Junio C Hamano]

Jonathan Tan  writes:

> As for whether both these functions are necessary in the first place, I
> think they are.

I do not mind their existence.  

I was wondering if they can share more implementation; such a design
would need s/the_commit_graph/the_repo->objstore->commit_graph/ only
once as a side effect.

Re: [PATCH v3 0/7] grep.c: teach --column to 'git-grep(1)'

[Junio C Hamano]

Taylor Blau  writes:

> On Mon, Jun 25, 2018 at 02:43:50PM -0400, Jeff King wrote:
>> On Fri, Jun 22, 2018 at 10:49:26AM -0500, Taylor Blau wrote:
>> > Since the last time, only a couple of things have changed at Peff's
>> > suggestions in [1]. The changes are summarized here, and an inter-diff
>> > is available below:
>> >
>> >   - Change "%zu" to PRIuMAX (and an appropriate cast into uintmax_t). I
>> > plan to send a follow-up patch to convert this back to "%zu" to see
>> > how people feel about it, but I wanted to keep that out of the
>> > present series in order to not hold things up.
>> ...
>> Jinxes aside, this interdiff looks good to me.
>
> Thanks; I hope that I haven't jinxed anything :-).
>
> I'm going to avoid sending the PRIuMAX -> "%zu" patch, since dscho
> points out that it's not available on Windows [1].

OK, so what I queued on 'pu' seems to be ready to advance, which is
good.  Keeping topics in flight on 'pu', unable to convince myself
that they are ready to advance to 'next', makes me feel uneasy and
unhappy, and having to worry about one less such topic is a good
news ;-)

Re: curious about wording in "man git-config", ENVIRONMENT

[Robert P. J. Day]

On Tue, 26 Jun 2018, Jeff King wrote:

> On Tue, Jun 26, 2018 at 06:18:26AM -0400, Robert P. J. Day wrote:
>
> >
> >   ENVIRONMENT
> > GIT_CONFIG
> >   Take the configuration from the given file instead of
> >   .git/config. Using the "--global" option forces this to
> >   ~/.gitconfig. Using the "--system" option forces this to
> >   $(prefix)/etc/gitconfig.
> >
> >   is the phrase "forces this to" really what you want to use here?
> > maybe i misunderstand what this option does, doesn't it simply mean
> > that it will use a different (specified) file from the default,
> > depending on the context (local, global, system)?
> >
> >   it just seems weird to say that the option "forces" the use of what
> > are clearly the default files. thoughts?
>
> I agree it's weird. I think it's trying to mean "behaves as if it
> was set to", but with the additional notion that the command-line
> argument would take precedence over the environment (which is our
> usual rule). But then we should just say those things explicitly.
>
> Just looking at mentions of GIT_CONFIG in that manpage and knowing
> the history, I think:

  ... snip ...

i'm just going to admit that i don't quite have the background to know
how to submit a patch to tidy things up based on Jeff's analysis, so
I'm going to leave this to someone higher up the food chain.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config

[Brandon Williams]

On 06/26, Antonio Ospite wrote:
> Hi,
> 
> this is version 2 of the series from
> https://public-inbox.org/git/20180622162656.19338-1-...@ao2.it/
> 
> The .gitmodules file is not meant for arbitrary configuration, it should
> be used only for submodules properties.
> 
> Plus, arbitrary git configuration should not be distributed with the
> repository, and .gitmodules might be a possible "vector" for that.
> 
> The series tries to alleviate both these issues by moving the
> 'config_from_gitmodules' function from config.[ch] to submodule-config.c
> and making it private.
> 
> This should discourage future code from using the function with
> arbitrary config callbacks which might turn .gitmodules into a mechanism
> to load arbitrary configuration stored in the repository.
> 
> Backward compatibility exceptions to the rules above are handled by
> ad-hoc helpers.
> 
> Finally (in patch 6) some duplication is removed by using
> 'config_from_gitmodules' to load the submodules configuration in
> 'repo_read_gitmodules'.
> 
> Changes since v1:
>   * Remove an extra space before an arrow operator in patch 2
>   * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs
>   * Add a note in the commit message of patch 6 about checking the
> worktree before loading .gitmodules
>   * Drop patch 7, it was meant as a cleanup but resulted in parsing the
> .gitmodules file twice

Thanks for making these changes, this version looks good to me!

-- 
Brandon Williams

Re: [PATCH 10/29] t9001: fix broken "invoke hook" test

[Jonathan Tan]

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index e80eacbb1b..776769fe0d 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -1966,11 +1966,11 @@ test_expect_success $PREREQ 'invoke hook' '
>  
>   # Verify error message when a patch is rejected by the hook
>   sed -e "s/add master/x/" ../0001-add-master.patch 
> >../another.patch &&
> - git send-email \
> + test_must_fail git send-email \
>   --from="Example " \
>   --to=nob...@example.com \
>   --smtp-server="$(pwd)/../fake.sendmail" \
> - ../another.patch 2>err
> + ../another.patch 2>err &&
>   test_i18ngrep "rejected by sendemail-validate hook" err

Thanks for catching this. Indeed, "git send-email" is supposed to fail
because the validate hook greps for the string "add master", which does
not exist in the e-mail to be sent. (Above this is a test that shows
that the same validate hook succeeds if the e-mail contains "add
master".) This looks correct to me.

Re: [BUG] url schemes should be case-insensitive

[Junio C Hamano]

Jeff King  writes:

>> > We seem to match url schemes case-sensitively:
>> >
>> >   $ git clone SSH://example.com/repo.git
>> >   Cloning into 'repo'...
>> >   fatal: Unable to find remote helper for 'SSH'
>> >
>> > whereas rfc3986 is clear that the scheme portion is case-insensitive.
>> > We probably ought to match at least our internal ones with strcasecmp.
>> 
>> That may break if somebody at DevToolGroup@$BIGCOMPANY got cute and
>> named their custom remote helper SSH:// that builds on top of the
>> normal ssh:// protocol with something extra and gave it to their
>> developers (and they named the http counterpart that has the same
>> extra HTTP://, of course).
>
> True, though I am on the fence whether that is a property worth
> maintaining. AFAIK it was not planned and is just a "this is how it
> happened to work" case that is (IMHO) doing the wrong thing.

FWIW, I fully agree with the assessment; sorry for not saying that
together with the devil's advocate comment to save a round-tip.

> It may also interact in a funny way with our allowed-protocol code, if
> "SSH" gets a pass as "ssh" under the default config, but actually runs
> the otherwise-disallowed git-remote-SSH (though one would _hope_ if you
> have such a git-remote-SSH that it behaves just like an ssh remote).

True.  I did not offhand recall how protocol whitelist matches the
protocol name with config, but transport.c::get_protocol_config()
seems to say that the  part of "protocol..allow" is case
sensitive, and we match known-safe (and known-unsafe "ext::")
protocols with strcmp() not strcasecmp().  We need to figure out the
implications of allowing SSH:// not to error out but pretending as
if it were ssh:// on those who have protocol.ssh.allow defined.

>> > We could probably also give an advise() message in the above output,
>> > suggesting that the problem is likely one of:
>> >
>> >   1. They misspelled the scheme.
>> >
>> >   2. They need to install the appropriate helper.
>> >
>> > This may be a good topic for somebody looking for low-hanging fruit to
>> > get involved in development (I'd maybe call it a #leftoverbits, but
>> > since I didn't start on it, I'm not sure if it counts as "left over" ;)).
>> [..]
>> It may probably be a good idea to do an advice, but I'd think
>> "Untable to find remote helper for 'SSH'" may be clear enough.  If
>> anything, perhaps saying "remote helper for 'SSH' protocol" would
>> make it even clear?  I dunno.
>
> I think it doesn't help much if the user does not know what a remote
> helper is, or why Git is looking for one.

True.  

$ git clone SSH://example.com/repo.git
fatal: unable to handle URL that begins with SSH://

would be clear enough, perhaps?  At least this line of change is a
small first step that would improve the situation without potential
to break anybody who has been abusing the case sensitivity loophole.

Re: [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C

[Junio C Hamano]

Alban Gruin  writes:

> This rewrites checkout_onto() from shell to C. The new version is called
> detach_onto(), given its role.

The name, given its role, may be good, but is the implementtaion
robust enough to fulfill the promise its name gives?

>   git rebase--helper --check-todo-list || {
>   ret=$?
> - checkout_onto
> + git rebase--helper --detach-onto "$onto_name" "$onto" \
> + "$orig_head" ${verbose:+--verbose}

Here, $onto_name is what the end-user gave us (e.g. it is
"master..." in "git rebase --onto=master... base"), while $onto is a
40-hex object name of the commit.  $orig_head is also a 40-hex
object name.

And this call shows how the above shell scriptlet calls into the
detach_onto() thing ...

> + if (command == DETACH_ONTO && argc == 4)
> + return !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);

... which is defined like so:

> +int detach_onto(struct replay_opts *opts,
> + const char *onto_name, const char *onto,
> + const char *orig_head, unsigned verbose)
> +{
> + struct object_id oid;
> + const char *action = reflog_message(opts, "start", "checkout %s", 
> onto_name);
> +
> + if (get_oid(orig_head, &oid))
> + return error(_("%s: not a valid OID"), orig_head);

Which means that this can be more strict to use get_oid_hex() to
catch possible mistakes in the caller.

> + if (run_git_checkout(opts, onto, verbose, action)) {

And this could be a bit problematic, as we can see below how the
"checkout" thing does not guarantee "detaching" at all ...

> + apply_autostash(opts);
> + sequencer_remove_state(opts);
> + return error(_("could not detach HEAD"));
> + }
> +
> + return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, 
> UPDATE_REFS_MSG_ON_ERR);
> +}
> +

... which can be seen here ...

> +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> + int verbose, const char *action)
> +{
> + struct child_process cmd = CHILD_PROCESS_INIT;
> +
> + cmd.git_cmd = 1;
> +
> + argv_array_push(&cmd.args, "checkout");
> + argv_array_push(&cmd.args, commit);
> + argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> + if (verbose)
> + return run_command(&cmd);
> + else
> + return run_command_silent_on_success(&cmd);
> +}

This drives the external command "git checkout" with _any_ string
the caller passes in "commit".  If the variable happens to have
'master', for example, it would be "git checkout master" and if you
have a branch with that name, it will not detach but check out the
branch to build on it.  It is a caller's responsibility to give a
suitable "commit" if it wants to use this helper to detach.

So perhaps the caller of this function in detach_onto() should pass
"%s^0" or even do something like

struct object_id onto_oid;
char onto_hex[GIT_MAX_HEXSZ + 1];

if (get_oid(onto, &onto_oid) || oid_to_hex_r(onto_hex, &onto_oid))
return error(...);
if (run_git_checkout(opts, onto_hex, verbose, action)) {
...

to ensure that it keeps the promise its name gives.

I can hear "Oh, but it is a bug in the caller to give anything that
won't result in detaching in 'onto'" but that is not a valid excuse,
given that this _public_ function is called "detach_onto".  Making
sure detachment happens is its responsibility, not its callers'.

Or we could do a cop-out alternative of commenting the function in *.h
file to say "onto must be given as 40-hex", with a code to make sure
the caller really gave us a 40-hex and not a branch name.  That is a
less ideal but probably acceptable alternative.

>  static const char rescheduled_advice[] =
>  N_("Could not execute the todo command\n"
>  "\n"
> diff --git a/sequencer.h b/sequencer.h
> index 35730b13e..9f0ac5e75 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -100,6 +100,10 @@ int update_head_with_reflog(const struct commit 
> *old_head,
>  void commit_post_rewrite(const struct commit *current_head,
>const struct object_id *new_head);
>  
> +int detach_onto(struct replay_opts *opts,
> + const char *onto_name, const char *onto,
> + const char *orig_head, unsigned verbose);
> +
>  #define SUMMARY_INITIAL_COMMIT   (1 << 0)
>  #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
>  void print_commit_summary(const char *prefix, const struct object_id *oid,

Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C

[Junio C Hamano]

Alban Gruin  writes:

 I do not think "base_commit" is a good name, either, though.  When I
 hear 'base' in the context of 'rebase', I would imagine that the
 speaker is talking about the bottom of the range of the commits to
 be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
 commits BASE..BRANCH on top of ONTO and then points BRANCH at the
 result), not the top of the range or the branch these commits are
 taken from.
>>> ...
> Now I really don’t know how to call this function.
> checkout_top_of_range(), perhaps?

If this is a straight rewrite of setup_reflog_action, i.e. the
division of labor between its caller and this function is such that
the caller blindly calls it without even checking if it got the
optional "branch to be rebased" argument and this function is
responsible to decide if the preparatory checkout of the named
branch is necessary, then any name that does not even hint that
checkout is done conditionally would not work well.

How about callilng it "prepare_branch_to_be_rebased()"?  This
function, at least the original setup_reflog_action, responds to

git rebase [--onto ONTO] UPSTREAM

by doing nothing (i.e. the result of preparation is to do nothing
because we are rebasing the commits between UPSTREAM and currently
checked out state on top of ONTO, or UPSTREAM if ONTO is not
given) and it responds to

git rebase [--onto ONTO] UPSTREAM BRANCH

by checking out BRANCH (most importantly, when given a concrete
branch name, it checks the branch out, and does not detach at the
commit at the tip of the branch), because that is the first
preparatory step to rebase the BRANCH.

Re: [PATCH] fetch-pack: support negotiation tip whitelist

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> During negotiation, fetch-pack eventually reports as "have" lines all
> commits reachable from all refs. Allow the user to restrict the commits
> sent in this way by providing a whitelist of tips; only the tips
> themselves and their ancestors will be sent.
>
> This feature is only supported for protocols that support connect or
> stateless-connect (such as HTTP with protocol v2).
>
> This will speed up negotiation when the repository has multiple
> relatively independent branches (for example, when a repository
> interacts with multiple repositories, such as with linux-next [1] and
> torvalds/linux [2]), and the user knows which local branch is likely to
> have commits in common with the upstream branch they are fetching.
>
> [1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/
> [2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/
>
> Signed-off-by: Jonathan Tan 
> ---

Very neat.  Thanks to Greg Thelen and Michel Lespinasse (cc-ed) for
this suggestion.

> This is based on jt/fetch-pack-negotiator, but if that branch is
> undesirable for whatever reason, I can port this to master.
>
>  builtin/fetch.c| 21 ++
>  fetch-pack.c   | 19 ++--
>  fetch-pack.h   |  7 ++
>  t/t5510-fetch.sh   | 55 ++
>  transport-helper.c |  3 +++
>  transport.c|  1 +
>  transport.h| 10 +
>  7 files changed, 114 insertions(+), 2 deletions(-)

Small nit: could this be documented in Documentation/fetch.txt as well?

Thanks,
Jonathan

Patch left unsnipped for reference.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..12daec0f3 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -63,6 +63,7 @@ static int shown_url = 0;
>  static struct refspec refmap = REFSPEC_INIT_FETCH;
>  static struct list_objects_filter_options filter_options;
>  static struct string_list server_options = STRING_LIST_INIT_DUP;
> +static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
>  
>  static int git_fetch_config(const char *k, const char *v, void *cb)
>  {
> @@ -174,6 +175,8 @@ static struct option builtin_fetch_options[] = {
>   TRANSPORT_FAMILY_IPV4),
>   OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
>   TRANSPORT_FAMILY_IPV6),
> + OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
> + N_("report that we have only objects reachable from 
> this object")),
>   OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
>   OPT_END()
>  };
> @@ -1075,6 +1078,24 @@ static struct transport *prepare_transport(struct 
> remote *remote, int deepen)
>  filter_options.filter_spec);
>   set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
>   }
> + if (negotiation_tip.nr) {
> + struct oid_array *oids;
> + if (transport->smart_options) {
> + int i;
> + oids = xcalloc(1, sizeof(*oids));
> + for (i = 0; i < negotiation_tip.nr; i++) {
> + struct object_id oid;
> + if (get_oid(negotiation_tip.items[i].string,
> + &oid))
> + die("%s is not a valid object",
> + negotiation_tip.items[i].string);
> + oid_array_append(oids, &oid);
> + }
> + transport->smart_options->negotiation_tips = oids;
> + } else {
> + warning("Ignoring --negotiation-tip because the 
> protocol does not support it.");
> + }
> + }
>   return transport;
>  }
>  
> diff --git a/fetch-pack.c b/fetch-pack.c
> index ba12085c4..c66bd49bd 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -213,6 +213,21 @@ static int next_flush(int stateless_rpc, int count)
>   return count;
>  }
>  
> +static void mark_tips(struct fetch_negotiator *negotiator,
> +   const struct oid_array *negotiation_tips)
> +{
> + int i;
> + if (!negotiation_tips) {
> + for_each_ref(rev_list_insert_ref_oid, negotiator);
> + return;
> + }
> +
> + for (i = 0; i < negotiation_tips->nr; i++)
> + rev_list_insert_ref(negotiator, NULL,
> + &negotiation_tips->oid[i]);
> + return;
> +}
> +
>  static int find_common(struct fetch_negotiator *negotiator,
>  struct fetch_pack_args *args,
>  int fd[2], struct object_id *result_oid,
> @@ -230,7 +245,7 @@ static int find_common(struct fetch_negotiator 
> *negotiator,
>   if (args->stateless_rpc && multi_ack == 1)
>   die(_("--stateless-rpc requires multi_ack_detailed"));
>  
> - for_each_ref(rev_lis

[PATCH] rebase -i: Fix white space in comments

[dana]

Fix a trivial white-space issue introduced by commit d48f97aa8
("rebase: reindent function git_rebase__interactive", 2018-03-23). This
affected the instructional comments displayed in the editor during an
interactive rebase.

Signed-off-by: dana 
---

Sorry if i've done any of this wrong; i've never used this work-flow
before. In any case, if it's not immediately obvious, this is the issue
i mean to fix:

BEFORE (2.17.1):

# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

AFTER (2.18.0):

# If you remove a line here THAT COMMIT WILL BE LOST.
#
#   However, if you remove everything, the rebase will be aborted.
#
#   
# Note that empty commits are commented out

The 2.18.0 version is particularly irritating because many editors
highlight the trailing tab in the penultimate line as a white-space
error.

Aside: It's not a new thing, but i've always felt like that last line
should end in a full stop. Maybe i'll send a patch for that too.

Cheers,
dana

 git-rebase--interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..a31af6d4c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -222,9 +222,9 @@ $comment_char $(eval_ngettext \
 EOF
append_todo_help
gettext "
-   However, if you remove everything, the rebase will be aborted.
+However, if you remove everything, the rebase will be aborted.
 
-   " | git stripspace --comment-lines >>"$todo"
+" | git stripspace --comment-lines >>"$todo"
 
if test -z "$keep_empty"
then
-- 
2.18.0

Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Johannes Sixt]

Am 26.06.2018 um 11:21 schrieb Eric Sunshine:

On Tue, Jun 26, 2018 at 4:58 AM Elijah Newren  wrote:

On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  wrote:

+   p4 help client &&
+   test_must_fail p4 help nosuchcommand


same question?


Same answer. Not shown in this patch, but just above the context lines
you will find this comment in the file:

 # We rely on this behavior to detect for p4 move availability.

which means that the test is really interested in being able to
reliably detect if a sub-command is or is not available. So, despite
the (somewhat) misleading test title, this test doesn't care about the
exact error code but rather cares only that "p4 help nosuchcommand"
errors out, period. Hence, test_must_fail() again agrees with the
spirit of the test.


test_must_fail ensures that only "proper" failures are diagnosed as 
expected; failures due to signals such as SEGV are not expected failures.


In the test suite we expect all programs that are not our "git" to work 
correctly; in particular, that they do not crash on anything that we ask 
them to operate on. Under this assumption, the protection given by 
test_must_fail is not needed.


Hence, these lines should actually be

p4 help client &&
! p4 help nosuchcommand

-- Hannes

Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Eric Sunshine]

On Tue, Jun 26, 2018 at 2:06 PM Johannes Sixt  wrote:
> Am 26.06.2018 um 11:21 schrieb Eric Sunshine:
> >> On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  
> >> wrote:
> >>> +   p4 help client &&
> >>> +   test_must_fail p4 help nosuchcommand
> > [...] So, despite
> > the (somewhat) misleading test title, this test doesn't care about the
> > exact error code but rather cares only that "p4 help nosuchcommand"
> > errors out, period. Hence, test_must_fail() again agrees with the
> > spirit of the test.
>
> test_must_fail ensures that only "proper" failures are diagnosed as
> expected; failures due to signals such as SEGV are not expected failures.
>
> In the test suite we expect all programs that are not our "git" to work
> correctly; in particular, that they do not crash on anything that we ask
> them to operate on. Under this assumption, the protection given by
> test_must_fail is not needed.
>
> Hence, these lines should actually be
>
> p4 help client &&
> ! p4 help nosuchcommand

Thanks for the comment; you're right, of course. I'll certainly make
this change if I have to re-roll for some other reason, but do you
feel that this itself is worth a re-roll?

Re: [PATCH v4 3/9] t3422: new testcases for checking when incompatible options passed

[Junio C Hamano]

Elijah Newren  writes:

> +# Rebase has lots of useful options like --whitepsace=fix, which are
> +# actually all built in terms of flags to git-am.  Since neither
> +# --merge nor --interactive (nor any options that imply those two) use
> +# git-am, using them together will result in flags like --whitespace=fix
> +# being ignored.  Make sure rebase warns the user and aborts instead.
> +#
> +
> +test_rebase_am_only () {
> + opt=$1
> + shift
> + test_expect_failure "$opt incompatible with --merge" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --merge A
> + "
> +
> + test_expect_failure "$opt incompatible with --strategy=ours" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --strategy=ours A
> + "
> +
> + test_expect_failure "$opt incompatible with --strategy-option=ours" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --strategy=ours A

This line is broken and it is carried over to later patches.  It
needs to be -Xours (or --strategy-option=ours, if we really want ot
be verbose).

> + "
> +
> + test_expect_failure "$opt incompatible with --interactive" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --interactive A
> + "
> +
> + test_expect_failure "$opt incompatible with --exec" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --exec 'true' A
> + "
> +
> +}

> +
> +test_rebase_am_only --whitespace=fix
> +test_rebase_am_only --ignore-whitespace
> +test_rebase_am_only --committer-date-is-author-date
> +test_rebase_am_only -C4

I was hesitant to hardcode what I perceive as limitations of non-am
rebase implementations with a test like this, but once somebody
fixes "rebase -i" for example to be capable of --whitespace=fix for
example, then we can just drop one line from the above four (and
write other tests for "rebase -i --whitespace=fix").  The
test_rebase_am_only is to help us make sure what is (still) not
supported by non-am rebases gets diagnosed as an error.

So my worry is totally unfounded, which is good.

> +test_expect_success '--preserve-merges incompatible with --signoff' '
> + git checkout B^0 &&
> + test_must_fail git rebase --preserve-merges --signoff A
> +'
> +
> +test_expect_failure '--preserve-merges incompatible with --rebase-merges' '
> + git checkout B^0 &&
> + test_must_fail git rebase --preserve-merges --rebase-merges A
> +'
> +
> +test_expect_failure '--rebase-merges incompatible with --strategy' '
> + git checkout B^0 &&
> + test_must_fail git rebase --rebase-merges -s resolve A
> +'
> +
> +test_expect_failure '--rebase-merges incompatible with --strategy-option' '
> + git checkout B^0 &&
> + test_must_fail git rebase --rebase-merges -Xignore-space-change A
> +'
> +
> +test_done

Re: [BUG] url schemes should be case-insensitive

[Jeff King]

On Tue, Jun 26, 2018 at 10:09:58AM -0700, Junio C Hamano wrote:

> > It may also interact in a funny way with our allowed-protocol code, if
> > "SSH" gets a pass as "ssh" under the default config, but actually runs
> > the otherwise-disallowed git-remote-SSH (though one would _hope_ if you
> > have such a git-remote-SSH that it behaves just like an ssh remote).
> 
> True.  I did not offhand recall how protocol whitelist matches the
> protocol name with config, but transport.c::get_protocol_config()
> seems to say that the  part of "protocol..allow" is case
> sensitive, and we match known-safe (and known-unsafe "ext::")
> protocols with strcmp() not strcasecmp().  We need to figure out the
> implications of allowing SSH:// not to error out but pretending as
> if it were ssh:// on those who have protocol.ssh.allow defined.

That function is actually a little tricky, because we feed it mostly
from string literals (so we end up in the ssh code path, and then feed
it "ssh"). But I think for remote-helpers we feed it literally from the
URL we got fed.

So yeah, we would not want to allow EXT::"rm -rf /" to slip past the
known-unsafe match. Any normalization should happen before then
(probably right in transport_helper_init).

Come to think of it, that's already sort-of an issue now. If you have a
case-insensitive filesystem, then EXT:: is going to pass this check, but
still run git-remote-ext. We're saved there somewhat by the fact that
the default is to reject unknown helpers in submodules (otherwise, we'd
have that horrible submodule bug all over again).

That goes beyond just cases, too. On HFS+ I wonder if I could ask for
"\u{0200}ext::" and run git-remote-ext.

> > I think it doesn't help much if the user does not know what a remote
> > helper is, or why Git is looking for one.
> 
> True.  
> 
>   $ git clone SSH://example.com/repo.git
>   fatal: unable to handle URL that begins with SSH://
> 
> would be clear enough, perhaps?  At least this line of change is a
> small first step that would improve the situation without potential
> to break anybody who has been abusing the case sensitivity loophole.

Yeah, certainly the advice is orthogonal to any behavior changes. The
original report complained of:

  $ git clone SSH://...
  fatal: 'remote-SSH' is not a git command. See 'git --help'.

but since 6b02de3b9d in 2010 we say:

  fatal: Unable to find remote helper for 'SSH'

So I actually wonder if there is something else going on. I find it hard
to believe the OP is using something older than Git v1.7.0. They did
appear to be on Windows, though. Is it possible our ENOENT detection
from start_command() is not accurate on Windows?

-Peff

Re: [PATCH] fetch-pack: support negotiation tip whitelist

[Junio C Hamano]

Jonathan Tan  writes:

> During negotiation, fetch-pack eventually reports as "have" lines all
> commits reachable from all refs. Allow the user to restrict the commits
> sent in this way by providing a whitelist of tips; only the tips
> themselves and their ancestors will be sent.
>
> This feature is only supported for protocols that support connect or
> stateless-connect (such as HTTP with protocol v2).
>
> This will speed up negotiation when the repository has multiple
> relatively independent branches (for example, when a repository
> interacts with multiple repositories, such as with linux-next [1] and
> torvalds/linux [2]), and the user knows which local branch is likely to
> have commits in common with the upstream branch they are fetching.
>
> [1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/
> [2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/
>
> Signed-off-by: Jonathan Tan 
> ---
> This is based on jt/fetch-pack-negotiator, but if that branch is
> undesirable for whatever reason, I can port this to master.
> ---
>  builtin/fetch.c| 21 ++
>  fetch-pack.c   | 19 ++--
>  fetch-pack.h   |  7 ++
>  t/t5510-fetch.sh   | 55 ++
>  transport-helper.c |  3 +++
>  transport.c|  1 +
>  transport.h| 10 +
>  7 files changed, 114 insertions(+), 2 deletions(-)

What's the plan to expose this "feature" to end-users?  There is no
end-user facing documentation added by this patch, and in-code
comments only talk about what (mechanical) effect the option has,
but not when a user may want to use the feature, or how the user
would best decide the set of commits to pass to this new option.

Would something like this

git fetch $(git for-each-ref \
--format=--nego-tip="%(objectname)" \
refs/remotes/linux-next/) \
linux-next

be an expected typical way to pull from one remote, exposing only
the tips of refs we got from that remote and not the ones we
obtained from other places?

Re: git rerere and diff3

[Junio C Hamano]

Nicolas Dechesne  writes:

> i have noticed that merge.conflictstyle has an impact on the rerere
> resolution. looking briefly at the source code, it seems that git
> tries to discard the common ancestor diff3 bits, but what I am seeing
> is that if i do the following then it fails:
>
> 1. from a clean rr-cache state, with merge.conflictsytle=diff3, git
> merge , resolve the conflicts, then commit
> 2. undo the previous merge, remove merge.conflictstyle=diff3 (disable
> diff3) and merge the *same* branch, then rerere won't fix the
> conflicts

It is possible that the conflict left when making the same merge are
actually different when using these two conflict styles.  IOW, if
the merge produces

<<<
side A
|||
common
===
side B
>>>

when diff3 style is chosen, but if the same merge results in

<<<
side A'
===
side B'
>>>

where side A' is not identical to side A (or B' and B are not
identical), then we will fail to find the previously recorded
resolution.

Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

[Junio C Hamano]

Eric Sunshine  writes:

> The --chain-lint option detects broken &&-chains by forcing the test to
> exit early (as the very first step) with a sentinel value. If that
> sentinel is the test's overall exit code, then the &&-chain is intact;
> if not, then the chain is broken. Unfortunately, this detection does not
> extend to &&-chains within subshells even when the subshell itself is
> properly linked into the outer &&-chain.
>
> Address this shortcoming by eliminating the subshell during the
> "linting" phase and incorporating its body directly into the surrounding
> &&-chain. To keep this transformation cheap, no attempt is made at
> properly parsing shell code. Instead, the manipulations are purely
> textual. For example:
>
> statement1 &&
> (
> statement2 &&
> statement3
> ) &&
> statement4
>
> is transformed to:
>
> statement1 &&
> statement2 &&
> statement3 &&
> statement4

so, with --chain-lint, we would transform this

mkdir -p a/b/c &&
(
cd a/b/c
rm -fr ../../*
) &&
statement 4

into this sequence

(exit $sentinel) &&
mkdir -p a/b/c &&
cd a/b/c
rm -fr ../../* &&
statement 4

and then rely on the non-zero exit to cancel all the remainder?

We didn't create nor cd to the t/trash$num/a/b/c thanks to the &&
chain, and end up running rm -fr ../../* from inside t/trash$num?

Hm

Re: [PATCH v2] filter-branch: skip commits present on --state-branch

[Ian Campbell]

On Mon, 2018-06-25 at 21:07 -0700, Michael Barabanov wrote:
> The commits in state:filter.map have already been processed, so don't
> filter them again. This makes incremental git filter-branch much
> faster.
> 
> Also add tests for --state-branch option.
> 
> Signed-off-by: Michael Barabanov 

Acked-by: Ian Campbell 

> ---
>  git-filter-branch.sh |  1 +
>  t/t7003-filter-branch.sh | 15 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index ccceaf19a..5c5afa2b9 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -372,6 +372,7 @@ while read commit parents; do
>   git_filter_branch__commit_count=$(($git_filter_branch__commi
> t_count+1))
>  
>   report_progress
> + test -f "$workdir"/../map/$commit && continue
>  
>   case "$filter_subdir" in
>   "")
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index ec4b160dd..e23de7d0b 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -107,6 +107,21 @@ test_expect_success 'test that the directory was
> renamed' '
>   test dir/D = "$(cat diroh/D.t)"
>  '
>  
> +V=$(git rev-parse HEAD)
> +
> +test_expect_success 'populate --state-branch' '
> + git filter-branch --state-branch state -f --tree-filter
> "touch file || :" HEAD
> +'
> +
> +W=$(git rev-parse HEAD)
> +
> +test_expect_success 'using --state-branch to skip already rewritten
> commits' '
> + test_when_finished git reset --hard $V &&
> + git reset --hard $V &&
> + git filter-branch --state-branch state -f --tree-filter
> "touch file || :" HEAD &&
> + test_cmp_rev $W HEAD
> +'
> +
>  git tag oldD HEAD~4
>  test_expect_success 'rewrite one branch, keeping a side branch' '
>   git branch modD oldD &&

Re: [PATCH 14/29] t: drop subshell with missing &&-chain in favor of simpler construct

[Junio C Hamano]

Eric Sunshine  writes:

> These tests employ a noisy subshell (with missing &&-chain) to feed
> input into Git commands:
>
> (echo a; echo b; echo c) | git some-command ...
>
> Drop the subshell in favor of a simple 'printf':
>
> printf "%s\n" a b c | git some-command ...

That's called test_write_lines, I think.

Re: git rerere and diff3

[Nicolas Dechesne]

On Tue, Jun 26, 2018 at 9:05 PM Junio C Hamano  wrote:
>
> Nicolas Dechesne  writes:
>
> > i have noticed that merge.conflictstyle has an impact on the rerere
> > resolution. looking briefly at the source code, it seems that git
> > tries to discard the common ancestor diff3 bits, but what I am seeing
> > is that if i do the following then it fails:
> >
> > 1. from a clean rr-cache state, with merge.conflictsytle=diff3, git
> > merge , resolve the conflicts, then commit
> > 2. undo the previous merge, remove merge.conflictstyle=diff3 (disable
> > diff3) and merge the *same* branch, then rerere won't fix the
> > conflicts
>
> It is possible that the conflict left when making the same merge are
> actually different when using these two conflict styles.  IOW, if
> the merge produces
>
> <<<
> side A
> |||
> common
> ===
> side B
> >>>
>
> when diff3 style is chosen, but if the same merge results in
>
> <<<
> side A'
> ===
> side B'
> >>>
>
> where side A' is not identical to side A (or B' and B are not
> identical), then we will fail to find the previously recorded
> resolution.

well, you're rigth.. that's what's happening...

=== with conflictstyle=merge
diff --cc arch/arm64/configs/defconfig
index 3cfa8ca26738,d53a1b00ad82..
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@@ -356,7 -334,7 +357,11 @@@ CONFIG_GPIO_PCA953X=
  CONFIG_GPIO_PCA953X_IRQ=y
  CONFIG_GPIO_MAX77620=y
  CONFIG_POWER_AVS=y
++<<< HEAD
 +CONFIG_ROCKCHIP_IODOMAIN=y
++===
+ CONFIG_QCOM_CPR=y
++>>> tracking-qcomlt-8016-dvfs
  CONFIG_POWER_RESET_MSM=y
  CONFIG_POWER_RESET_XGENE=y
  CONFIG_POWER_RESET_SYSCON=y

=== with conflictstyle=diff3
diff --cc arch/arm64/configs/defconfig
index 3cfa8ca26738,d53a1b00ad82..
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@@ -355,8 -333,8 +356,14 @@@ CONFIG_GPIO_XGENE_SB=
  CONFIG_GPIO_PCA953X=y
  CONFIG_GPIO_PCA953X_IRQ=y
  CONFIG_GPIO_MAX77620=y
++<<< HEAD
 +CONFIG_POWER_AVS=y
 +CONFIG_ROCKCHIP_IODOMAIN=y
++||| merged common ancestors
++===
+ CONFIG_POWER_AVS=y
+ CONFIG_QCOM_CPR=y
++>>> tracking-qcomlt-8016-dvfs
  CONFIG_POWER_RESET_MSM=y
  CONFIG_POWER_RESET_XGENE=y
  CONFIG_POWER_RESET_SYSCON=y

that explains it.. it was simpler than what I thought..

thanks!

Re: [PATCH 00/29] t: detect and fix broken &&-chains in subshells

[Junio C Hamano]

Eric Sunshine  writes:

> The --chain-lint[1] option detects breakage in the top-level &&-chain of
> tests. This series undertakes the more complex task of teaching it to
> also detect &&-chain breakage within subshells. See patch 29/29 for the
> gory details of how that's done.

I first looked at 29/29 and got heavily inclined to reject that
step, and then continued reading from 1/29 to around 15/29.  

I like these earlier changes that fix existing breakage, of course.
I also like many of the changes that simplify and/or modernise the
test scripts very much, but they are unusable as-is as long as their
justification is "chain-lint will start barfing on these constructs".

Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

[Eric Sunshine]

On Tue, Jun 26, 2018 at 3:15 PM Junio C Hamano  wrote:
> so, with --chain-lint, we would transform this
>
> mkdir -p a/b/c &&
> (
> cd a/b/c
> rm -fr ../../*
> ) &&
> statement 4
>
> into this sequence
>
> (exit $sentinel) &&
> mkdir -p a/b/c &&
> cd a/b/c
> rm -fr ../../* &&
> statement 4
>
> and then rely on the non-zero exit to cancel all the remainder?
>
> We didn't create nor cd to the t/trash$num/a/b/c thanks to the &&
> chain, and end up running rm -fr ../../* from inside t/trash$num?

Yes, I did take that into account and, no, I don't have a good answer
to the issue.

The existing --chain-lint already suffers the same shortcoming. Older
(or even new poorly-written) tests, even without subshells, can fall
victim already:

(exit $sentinel) &&
mkdir -p a/b/c &&
cd a/b/c
rm -fr ../../* &&
cd ../../.. &&
statement4

As in your example, 'mkdir' and 'cd' are skipped, but 'rm -fr ../../*' is not.

This snippet from the commit message of bb79af9d09 (t/test-lib:
introduce --chain-lint option, 2015-03-20):

When we encounter a failure of this check, we abort the test
script entirely. For one thing, we have no clue which subset
of the commands in the test snippet were actually run.

suggests that the issue was considered, in some form, even then
(though, it doesn't say explicitly that Peff had the 'rm -fr' case in
mind).

So, this isn't a new problem introduced by this series, though this
series may exacerbate it.

Thanks for thinking critically about it.

Re: [GSoC][PATCH 1/1] sequencer: print an error message if append_todo_help() fails

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Alban,
>
> On Tue, 26 Jun 2018, Alban Gruin wrote:
>
>> This adds an error when append_todo_help() fails to write its message to
>> the todo file.
>> 
>> Signed-off-by: Alban Gruin 
>
> ACK.
>
> We *may* want to fold that into the commit that adds `append_todo_help()`.

Absolutely.  This looks more like an "oops, I made a mess and here
is a fix on top", and even worse, it does not make an effort to help
readers where the mess was made (iow, which commit it goes on to
of); it is better to be squashed in.

I do not know offhand who Alban's mentors are, but one thing I think
is a good thing for them to teach is how to better organize the
changes with readers in mind.  The author of a patch series knows
his or her patches and how they relate to each other a lot better
than the readers of patches, who are reading not just his or her
patches but the ones from a lot wider set of contributors.  Even
though append-todo-help and edit-todo may have been developed as
separate steps in author's mind, it is criminal to send them as if
they are completely separate topics that can independently applied,
especially when one depends on the other.  It is a lot more helpful
to the readers if they were sent as a larger single series, because
doing so _will_ tell the readers which order the dependency goes.

> And, as I mentioned previously, I would love for that function to be used
> as an excuse to introduce the long-overdue `interactive-rebase.c`

I am not sure if I like this direction.  

As newbies are often very bad at coming up with APIs and naming
global functions, keeping everything as "static" inside a single
sequencer.c tends to avoid contaminating the global namespace.

Re: [BUG] url schemes should be case-insensitive

[Jeff King]

On Tue, Jun 26, 2018 at 02:27:39PM -0400, Jeff King wrote:

> So yeah, we would not want to allow EXT::"rm -rf /" to slip past the
> known-unsafe match. Any normalization should happen before then
> (probably right in transport_helper_init).
> 
> Come to think of it, that's already sort-of an issue now. If you have a
> case-insensitive filesystem, then EXT:: is going to pass this check, but
> still run git-remote-ext. We're saved there somewhat by the fact that
> the default is to reject unknown helpers in submodules (otherwise, we'd
> have that horrible submodule bug all over again).
> 
> That goes beyond just cases, too. On HFS+ I wonder if I could ask for
> "\u{0200}ext::" and run git-remote-ext.

That should be \u{200c}, of course, to get the correct sneaky character.
And the good news is that no, it doesn't work. We are protected by this
code in transport_get():

  /* maybe it is a foreign URL? */
  if (url) {
  const char *p = url;

  while (is_urlschemechar(p == url, *p))
  p++; 
  if (starts_with(p, "::"))
  helper = xstrndup(url, p - url);
  }

So we'll only allow remote-helper names with valid url characters, which
are basically [A-Za-z0-9+.-]. So I think we probably only have to worry
about true case issues, and not any kind of weird filesystem-specific
behaviors.

-Peff

Re: [PATCH 14/29] t: drop subshell with missing &&-chain in favor of simpler construct

[Eric Sunshine]

On Tue, Jun 26, 2018 at 3:31 PM Junio C Hamano  wrote:
> Eric Sunshine  writes:
> > These tests employ a noisy subshell (with missing &&-chain) to feed
> > input into Git commands:
> >
> > (echo a; echo b; echo c) | git some-command ...
> >
> > Drop the subshell in favor of a simple 'printf':
> >
> > printf "%s\n" a b c | git some-command ...
>
> That's called test_write_lines, I think.

Yep, that's better. Thanks.

Re: [PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules

[Junio C Hamano]

Antonio Ospite  writes:

> Add a helper function to make it clearer that retrieving 'fetch'
> configuration from the .gitmodules file is a special case supported
> solely for backward compatibility purposes.
> ...

Then perhaps the new public function deserves a comment stating
that?

> +struct fetch_config {
> + int *max_children;
> + int *recurse_submodules;
> +};
> +
> +static int gitmodules_fetch_config(const char *var, const char *value, void 
> *cb)
> +{
> + struct fetch_config *config = cb;
> + if (!strcmp(var, "submodule.fetchjobs")) {
> + *(config->max_children) = parse_submodule_fetchjobs(var, value);
> + return 0;
> + } else if (!strcmp(var, "fetch.recursesubmodules")) {
> + *(config->recurse_submodules) = 
> parse_fetch_recurse_submodules_arg(var, value);
> + return 0;
> + }
> +
> + return 0;
> +}
> +
> +void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
> +{
> + struct fetch_config config = {
> + .max_children = max_children,
> + .recurse_submodules = recurse_submodules
> + };

We started using designated initializers some time ago, and use of
it improves readability of something like this ;-)

> + config_from_gitmodules(gitmodules_fetch_config, &config);
> +}
> diff --git a/submodule-config.h b/submodule-config.h
> index 5148801f4..cff297a75 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -66,4 +66,6 @@ int check_submodule_name(const char *name);
>   */
>  extern void config_from_gitmodules(config_fn_t fn, void *data);
>  
> +extern void fetch_config_from_gitmodules(int *max_children, int 
> *recurse_submodules);
> +
>  #endif /* SUBMODULE_CONFIG_H */

Re: [PATCH v2 4/6] submodule-config: make 'config_from_gitmodules' private

[Junio C Hamano]

Antonio Ospite  writes:

> Now that 'config_from_gitmodules' is not used in the open, it can be
> marked as private.

Nice ;-)

Re: [PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules

[Junio C Hamano]

Antonio Ospite  writes:

> Generlize config_from_gitmodules to accept a repository as an argument.

generalize???

>
> This is in preparation to reuse the function in repo_read_gitmodules in
> order to have a single point where the '.gitmodules' file is accessed.
>
> Signed-off-by: Antonio Ospite 
> ---
>  submodule-config.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index cd1f1e06a..602c46af2 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -680,10 +680,10 @@ void submodule_free(struct repository *r)
>   * Runs the provided config function on the '.gitmodules' file found in the
>   * working directory.
>   */
> -static void config_from_gitmodules(config_fn_t fn, void *data)
> +static void config_from_gitmodules(config_fn_t fn, struct repository *repo, 
> void *data)
>  {
> - if (the_repository->worktree) {
> - char *file = repo_worktree_path(the_repository, 
> GITMODULES_FILE);
> + if (repo->worktree) {
> + char *file = repo_worktree_path(repo, GITMODULES_FILE);
>   git_config_from_file(fn, file, data);
>   free(file);
>   }
> @@ -714,7 +714,7 @@ void fetch_config_from_gitmodules(int *max_children, int 
> *recurse_submodules)
>   .max_children = max_children,
>   .recurse_submodules = recurse_submodules
>   };
> - config_from_gitmodules(gitmodules_fetch_config, &config);
> + config_from_gitmodules(gitmodules_fetch_config, the_repository, 
> &config);
>  }
>  
>  static int gitmodules_update_clone_config(const char *var, const char *value,
> @@ -728,5 +728,5 @@ static int gitmodules_update_clone_config(const char 
> *var, const char *value,
>  
>  void update_clone_config_from_gitmodules(int *max_jobs)
>  {
> - config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
> + config_from_gitmodules(gitmodules_update_clone_config, the_repository, 
> &max_jobs);
>  }

Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

[Jeff King]

On Tue, Jun 26, 2018 at 03:52:54PM -0400, Eric Sunshine wrote:

> The existing --chain-lint already suffers the same shortcoming. Older
> (or even new poorly-written) tests, even without subshells, can fall
> victim already:
> 
> (exit $sentinel) &&
> mkdir -p a/b/c &&
> cd a/b/c
> rm -fr ../../* &&
> cd ../../.. &&
> statement4
> 
> As in your example, 'mkdir' and 'cd' are skipped, but 'rm -fr ../../*' is not.
> 
> This snippet from the commit message of bb79af9d09 (t/test-lib:
> introduce --chain-lint option, 2015-03-20):
> 
> When we encounter a failure of this check, we abort the test
> script entirely. For one thing, we have no clue which subset
> of the commands in the test snippet were actually run.
> 
> suggests that the issue was considered, in some form, even then
> (though, it doesn't say explicitly that Peff had the 'rm -fr' case in
> mind).
>
> So, this isn't a new problem introduced by this series, though this
> series may exacerbate it.

One way this series might be worse in practice is that we tend not to
change process state too much outside of the subshells. So if we skip
some early commands and execute a later "rm", for example, it tends to
be in the same directory (and I think as time goes on we have been
cleaning up old tests which did a "cd foo && bar && cd .." into using a
subshell).

Whereas once you start collapsing subshells into the main logic chain,
there's a very high chance that the subshell is doing a "cd", since
that's typically the main reason for the subshell in the first place.
And with the current --chain-lint logic, that subshell is either
executed or not executed as a unit.

Obviously that's a bit of a hand-waving argument. If you've fixed all of
the existing cases without accidentally deleting your home directory,
then maybe it's not so likely to be a problem after all.

I'm not sure if there's a good solution, though. Even if you retained
the subshells and instead did a chain-lint inside each subshell, like
this:

  (exit 117) &&
  one &&
  (
(exit 117) &&
cd foo
two
  ) &&
  three


that doesn't really help. The fundamental issue is that we may skip the
"cd" inside the subshell. Whether it's in a subshell or not, that's
dangerous. True, we don't run "three" in this case, which is slightly
better. But it didn't expect to be in a different directory anyway. It's
running "two" that is dangerous.

-Peff

Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

[Jeff King]

On Tue, Jun 26, 2018 at 04:17:08PM -0400, Jeff King wrote:

> I'm not sure if there's a good solution, though. Even if you retained
> the subshells and instead did a chain-lint inside each subshell, like
> this:

So obviously that means "I don't think there's a good solution with this
approach".

That whole final patch simultaneously impresses and nauseates me. Your
commit message says "no attempt is made at properly parsing shell code",
but we come pretty darn close. I almost wonder if we'd be better off
just parsing some heuristic subset and making sure (via review or
linting) that our tests conform.

Another option is to not enable this slightly-more-dangerous linting by
default. But that would probably rob it of its usefulness, since it
would just fall to some brave soul to later crank up the linting and fix
everybody else's mistakes.

-Peff

Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config

[Junio C Hamano]

Brandon Williams  writes:

>> Changes since v1:
>>   * Remove an extra space before an arrow operator in patch 2
>>   * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs
>>   * Add a note in the commit message of patch 6 about checking the
>> worktree before loading .gitmodules
>>   * Drop patch 7, it was meant as a cleanup but resulted in parsing the
>> .gitmodules file twice
>
> Thanks for making these changes, this version looks good to me!

Yup, thanks, both.

Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

[Eric Sunshine]

On Tue, Jun 26, 2018 at 4:17 PM Jeff King  wrote:
> On Tue, Jun 26, 2018 at 03:52:54PM -0400, Eric Sunshine wrote:
> > So, this isn't a new problem introduced by this series, though this
> > series may exacerbate it.
>
> Whereas once you start collapsing subshells into the main logic chain,
> there's a very high chance that the subshell is doing a "cd", since
> that's typically the main reason for the subshell in the first place.
> And with the current --chain-lint logic, that subshell is either
> executed or not executed as a unit.
>
> Obviously that's a bit of a hand-waving argument. If you've fixed all of
> the existing cases without accidentally deleting your home directory,
> then maybe it's not so likely to be a problem after all.

Indeed, it could be that the "rm -fr" worry is tending toward the
hypothetical. Seasoned developers tend to be pretty careful and
usually avoid indiscriminately loose "rm -fr" invocations, so I'm
somewhat less worried about them. I do share the concern, though, that
newcomers crafting or extending tests could shoot themselves in the
foot with this. However, newcomers are also the ones most likely to
use the "cd foo && bar && cd .." idiom, so they are already at risk.

(As for not blasting my home directory when fixing all the existing
tests, I did run into a few cases where one or two "foreign" files
were deposited into the "t/" directory, but nothing was deleted or
overwritten.)

> I'm not sure if there's a good solution, though. Even if you retained
> the subshells and instead did a chain-lint inside each subshell, like
> this:
>
>   (exit 117) &&
>   one &&
>   (
> (exit 117) &&
> cd foo
> two
>   ) &&
>   three

I thought of that too, but the inner (exit 117) doesn't even get
invoked unless there is &&-chain breakage somewhere above that point
(for instance, if "one" lacks "&&"), so the inner (exit 117) doesn't
participate in the linting process at all.

> that doesn't really help. The fundamental issue is that we may skip the
> "cd" inside the subshell. Whether it's in a subshell or not, that's
> dangerous. True, we don't run "three" in this case, which is slightly
> better. But it didn't expect to be in a different directory anyway. It's
> running "two" that is dangerous.

Just thinking aloud...

Aside from "rm -fr", there are numerous ways to clobber files
unexpectedly when the "cd" is skipped:

echo x >../git.c
cp x ../git.c
mv x ../git.c
ln [-s] x ../git.c
/bin/rm ../git.c
some-cmd -o ../git.c

Some of these dangers can be de-thoothed during the linting phase by
defining do-nothing shell functions:

cp () { :; }
mv () { :; }
ln () { :; }

That, at least, makes the scariest case ("rm") much less so.

[PATCH v5 1/8] test-pkt-line: add unpack-sideband subcommand

[Brandon Williams]

Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.

Signed-off-by: Brandon Williams 
---
 t/helper/test-pkt-line.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 0f19e53c7..30775f986 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,3 +1,4 @@
+#include "cache.h"
 #include "pkt-line.h"
 
 static void pack_line(const char *line)
@@ -48,6 +49,36 @@ static void unpack(void)
}
 }
 
+static void unpack_sideband(void)
+{
+   struct packet_reader reader;
+   packet_reader_init(&reader, 0, NULL, 0,
+  PACKET_READ_GENTLE_ON_EOF |
+  PACKET_READ_CHOMP_NEWLINE);
+
+   while (packet_reader_read(&reader) != PACKET_READ_EOF) {
+   int band;
+   int fd;
+
+   switch (reader.status) {
+   case PACKET_READ_EOF:
+   break;
+   case PACKET_READ_NORMAL:
+   band = reader.line[0] & 0xff;
+   if (band < 1 || band > 2)
+   die("unexpected side band %d", band);
+   fd = band;
+
+   write_or_die(fd, reader.line + 1, reader.pktlen - 1);
+   break;
+   case PACKET_READ_FLUSH:
+   return;
+   case PACKET_READ_DELIM:
+   break;
+   }
+   }
+}
+
 int cmd_main(int argc, const char **argv)
 {
if (argc < 2)
@@ -57,6 +88,8 @@ int cmd_main(int argc, const char **argv)
pack(argc - 2, argv + 2);
else if (!strcmp(argv[1], "unpack"))
unpack();
+   else if (!strcmp(argv[1], "unpack-sideband"))
+   unpack_sideband();
else
die("invalid argument '%s'", argv[1]);
 
-- 
2.18.0.rc2.346.g013aa6912e-goog

[PATCH v5 3/8] upload-pack: test negotiation with changing repository

[Brandon Williams]

Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.

As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.

Signed-off-by: Brandon Williams 
---
 t/lib-httpd.sh |  1 +
 t/lib-httpd/apache.conf|  8 
 t/lib-httpd/one-time-sed.sh| 22 ++
 t/t5703-upload-pack-ref-in-want.sh | 68 ++
 4 files changed, 99 insertions(+)
 create mode 100644 t/lib-httpd/one-time-sed.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+   install_script one-time-sed.sh
 
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 724d9ae46..fe68d37bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -111,9 +111,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
 
+
+   SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+   SetEnv GIT_HTTP_EXPORT_ALL
+
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
 
Options FollowSymlinks
 
@@ -123,6 +128,9 @@ ScriptAlias /error/ error.sh/
 
   Options ExecCGI
 
+
+   Options ExecCGI
+
 
Options ExecCGI
 
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..8a9a5aca0
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+# If "one-time-sed" exists in $HTTPD_ROOT_PATH, run sed on the HTTP response,
+# using the contents of "one-time-sed" as the sed command to be run. If the
+# response was modified as a result, delete "one-time-sed" so that subsequent
+# HTTP responses are no longer modified.
+# 
+# This can be used to simulate the effects of the repository changing in
+# between HTTP request-response pairs.
+if [ -e one-time-sed ]; then "$GIT_EXEC_PATH/git-http-backend" >out
+
+   sed "$(cat one-time-sed)" out_modified
+
+   if diff out out_modified >/dev/null; then
+   cat out
+   else
+   cat out_modified
+   rm one-time-sed
+   fi
+else
+   "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5703-upload-pack-ref-in-want.sh 
b/t/t5703-upload-pack-ref-in-want.sh
index 0ef182970..a4fe0e7e4 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -150,4 +150,72 @@ test_expect_success 'want-ref with ref we already have 
commit for' '
check_output
 '
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+   (
+   git init "$REPO" &&
+   cd "$REPO" &&
+   >.git/git-daemon-export-ok &&
+   test_commit m1 &&
+   git tag -d m1 &&
+
+   # Local repo with many commits (so that negotiation will take
+   # more than 1 request/response pair)
+   git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo"; 
"$LOCAL_PRISTINE" &&
+   cd "$LOCAL_PRISTINE" &&
+   git checkout -b side &&
+   for i in $(seq 1 33); do test_commit s$i; done &&
+
+   # Add novel commits to upstream
+   git checkout master &&
+   cd "$REPO" &&
+   test_commit m2 &&
+   test_commit m3 &&
+   git tag -d m2 m3
+   ) &&
+   git -C "$LOCAL_PRISTINE" remote set-url origin 
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo"; &&
+   git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency() {
+   # Simulate that the server initially reports $2 as the ref
+   # corresponding to $1, and after that, $1 as the ref corresponding to
+   # $1. This corresponds to the real-life situation where the server's
+   # repository appears to change during negotiation, for example, when
+   # different servers in a load-balancing arrangement serve (stateless)
+   # RPCs during a single negotiation.
+   printf "s/%s/%s/" \
+  $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+  $(git -C "$REPO" rev-parse $2 

[PATCH v5 2/8] upload-pack: implement ref-in-want

[Brandon Williams]

Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids.  This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.

In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2.  This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref " parameter.  At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.

Signed-off-by: Brandon Williams 
---
 Documentation/config.txt|   7 ++
 Documentation/technical/protocol-v2.txt |  29 -
 t/t5703-upload-pack-ref-in-want.sh  | 153 
 upload-pack.c   |  66 ++
 4 files changed, 254 insertions(+), 1 deletion(-)
 create mode 100755 t/t5703-upload-pack-ref-in-want.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..fb1dd7428 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it 
is seen in the
 repository-level config (this is a safety measure against fetching from
 untrusted repositories).
 
+uploadpack.allowRefInWant::
+   If this option is set, `upload-pack` will support the `ref-in-want`
+   feature of the protocol version 2 `fetch` command.  This feature
+   is intended for the benefit of load-balanced servers which may
+   not have the same view of what OIDs their refs point to due to
+   replication delay.
+
 url..insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with . In cases where some site serves a
diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index 49bda76d2..b53cfc6ce 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -299,12 +299,21 @@ included in the client's request:
for use with partial clone and partial fetch operations. See
`rev-list` for possible "filter-spec" values.
 
+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+want-ref 
+   Indicates to the server that the client wants to retrieve a
+   particular ref, where  is the full name of a ref on the
+   server.
+
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
 header.
 
 output = *section
-section = (acknowledgments | shallow-info | packfile)
+section = (acknowledgments | shallow-info | wanted-refs | packfile)
  (flush-pkt | delim-pkt)
 
 acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -319,6 +328,10 @@ header.
 shallow = "shallow" SP obj-id
 unshallow = "unshallow" SP obj-id
 
+wanted-refs = PKT-LINE("wanted-refs" LF)
+ *PKT-LINE(wanted-ref LF)
+wanted-ref = obj-id SP refname
+
 packfile = PKT-LINE("packfile" LF)
   *PKT-LINE(%x01-03 *%x00-ff)
 
@@ -379,6 +392,20 @@ header.
* This section is only included if a packfile section is also
  included in the response.
 
+wanted-refs section
+   * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+   * Always begins with the section header "wanted-refs".
+
+   * The server will send a ref listing (" ") for
+ each reference requested using 'want-ref' lines.
+
+   * The server SHOULD NOT send any refs which were not requested
+ using 'want-ref' lines and a client MUST ignore refs which
+ weren't requested.
+
 packfile section
* This section is only included if the client has sent 'want'
  lines in its request and either requested that no more
diff --git a/t/t5703-upload-pack-ref-in-want.sh 
b/t/t5703-upload-pack-ref-in-want.sh
new file mode 100755
index 0..0ef182970
--- /dev/null
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description='upload-pack ref-in-want'
+
+. ./test-lib.sh
+
+get_actual_refs() {
+   sed -n '/wanted-refs/,/0001/p' actual_refs
+}
+
+get_actual_commits() {
+   sed -n '/packfile/,//p' o.pack &&
+   git index-pack o.pack &&
+   git verify-pack -v o.idx | grep commit | cut -c-40 | sort 
>actual_commits
+}
+
+check_output() {
+   get_actual_refs &&
+   tes

[PATCH v5 4/8] fetch: refactor the population of peer ref OIDs

[Brandon Williams]

Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..545635448 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,6 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail = &orefs;
 
+   struct string_list existing_refs = STRING_LIST_INIT_DUP;
const struct ref *remote_refs;
 
if (rs->nr)
@@ -458,7 +459,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = &rm->next;
}
 
-   return ref_remove_duplicates(ref_map);
+   ref_map = ref_remove_duplicates(ref_map);
+
+   for_each_ref(add_existing, &existing_refs);
+   for (rm = ref_map; rm; rm = rm->next) {
+   if (rm->peer_ref) {
+   struct string_list_item *peer_item =
+   string_list_lookup(&existing_refs,
+  rm->peer_ref->name);
+   if (peer_item) {
+   struct object_id *old_oid = peer_item->util;
+   oidcpy(&rm->peer_ref->old_oid, old_oid);
+   }
+   }
+   }
+   string_list_clear(&existing_refs, 1);
+
+   return ref_map;
 }
 
 #define STORE_REF_ERROR_OTHER 1
@@ -1110,14 +1127,10 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
 static int do_fetch(struct transport *transport,
struct refspec *rs)
 {
-   struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
-   struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
 
-   for_each_ref(add_existing, &existing_refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
 
-   for (rm = ref_map; rm; rm = rm->next) {
-   if (rm->peer_ref) {
-   struct string_list_item *peer_item =
-   string_list_lookup(&existing_refs,
-  rm->peer_ref->name);
-   if (peer_item) {
-   struct object_id *old_oid = peer_item->util;
-   oidcpy(&rm->peer_ref->old_oid, old_oid);
-   }
-   }
-   }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,
}
 
  cleanup:
-   string_list_clear(&existing_refs, 1);
return retcode;
 }
 
-- 
2.18.0.rc2.346.g013aa6912e-goog

[PATCH v5 5/8] fetch: refactor fetch_refs into two functions

[Brandon Williams]

Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them.  This is in preparation
for allowing additional processing of the fetched refs before updating
the local ref store.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 545635448..2fabfed0e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -968,9 +968,21 @@ static int fetch_refs(struct transport *transport, struct 
ref *ref_map)
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
-   ret |= store_updated_refs(transport->url,
-   transport->remote->name,
-   ref_map);
+   /*
+* Keep the new pack's ".keep" file around to allow the caller
+* time to update refs to reference the new objects.
+*/
+   return 0;
+   transport_unlock_pack(transport);
+   return ret;
+}
+
+/* Update local refs based on the ref values fetched from a remote */
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+   int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
 }
@@ -1116,7 +1128,8 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-   fetch_refs(transport, ref_map);
+   if (!fetch_refs(transport, ref_map))
+   consume_refs(transport, ref_map);
 
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1165,7 +1178,7 @@ static int do_fetch(struct transport *transport,
   transport->url);
}
}
-   if (fetch_refs(transport, ref_map)) {
+   if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) 
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
-- 
2.18.0.rc2.346.g013aa6912e-goog