[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 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 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
+   /^[ 

[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

[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

[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 &&
-   

[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 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-fil

[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 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

[PATCH 03/29] t1300: use sane_unset() to avoid breaking &&-chain

[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

Re: [PATCH v1 0/9] Introducing remote ODBs

[Eric Sunshine]

On Mon, Jun 25, 2018 at 5:49 PM Junio C Hamano  wrote:
> Christian Couder  writes:
> > This is a follow up from the patch series called "odb remote" [...]
>
> 5702.20 and 5702.21 seems to fail in standalone test, when these are
> directly queued on top of Git v2.18.0; I haven't looked into the
> failure myself (yet).

In addition to the t5702 failures, I'm also seeing failures of
t0410.1, t5616.6 and t5616.7 at the tip of 'pu' as of [1], all of
which seem to be related to these changes.

[1]: https://public-inbox.org/git/xmqqin66mql6@gitster-ct.c.googlers.com/

Re: [PATCH v2 11/24] midx: read pack names into array

[Eric Sunshine]

On Mon, Jun 25, 2018 at 10:35 AM Derrick Stolee  wrote:
> diff --git a/midx.c b/midx.c
> @@ -210,6 +227,20 @@ static void sort_packs_by_name(char **pack_names, 
> uint32_t nr_packs, uint32_t *p
> +static size_t write_midx_pack_lookup(struct hashfile *f,
> +char **pack_names,
> +uint32_t nr_packs)
> +{
> +   uint32_t i, cur_len = 0;
> +
> +   for (i = 0; i < nr_packs; i++) {
> +   hashwrite_be32(f, cur_len);
> +   cur_len += strlen(pack_names[i]) + 1;
> +   }
> +
> +   return sizeof(uint32_t) * (size_t)nr_packs;
> +}

This static function is never used, thus breaks the build with DEVELOPER=1:

midx.c:567:15: error: ‘write_midx_pack_lookup’ defined but not used
[-Werror=unused-function]
cc1: all warnings being treated as errors

Re: [PATCH] Makefile: tweak sed invocation

[Eric Sunshine]

On Mon, Jun 25, 2018 at 3:18 PM Alejandro R. Sedeño  wrote:
> With GNU sed, the r command doesn't care if a space separates it and
> the filename it reads from.
>
> With SunOS sed, the space is required.

MacOS and the various BSD's ship with BSD 'sed', not GNU 'sed', so it
seemed prudent to check this change against them as well, which I did,
and can report that it does not cause any regression on those
platforms.

Therefore, the patch looks good. Thanks.

> Signed-off-by: Alejandro R. Sedeño 
> ---
> diff --git a/Makefile b/Makefile
> @@ -2109,7 +2109,7 @@ $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES 
> GIT-PERL-HEADER GIT-VERSION-FILE
> $(QUIET_GEN)$(RM) $@ $@+ && \
> sed -e '1{' \
> -e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \
> -   -e 'rGIT-PERL-HEADER' \
> +   -e 'r GIT-PERL-HEADER' \
> -e 'G' \
> -e '}' \
> -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \

Re: [PATCH] Documentation: declare "core.ignorecase" as internal variable

[Eric Sunshine]

On Sun, Jun 24, 2018 at 6:05 AM Marc Strapetz  wrote:
> The current description of "core.ignorecase" reads like an option which
> is intended to be changed by the user while it's actually expected to
> be set by Git only [1].
>
> [1] https://marc.info/?l=git=152972992729761=2

Thanks for following up the discussion with a patch.

The commit message, unfortunately, doesn't explain the "why" of this
change in enough detail for someone to understand the issue without
chasing down that link (which could go stale, or the reader might be
offline). Incorporating Bryan's explanation[1] directly into the
commit message would likely be a good idea if you happen to re-roll.

Git on Windows is not designed to run with anything other than
core.ignoreCase=true, and attempting to do so will cause
unexpected behavior. In other words, it's not a behavior toggle so
user's can request the functionality to work one way or the other;
it's an implementation detail that `git init` and `git clone` set
when a repository is created purely so they don't have to probe
the file system each time you run a `git` command.

[1]: 
https://public-inbox.org/git/cagyf7-gvcn8ehmgtazcdjnyndflwvh8hvbdmzqju40nze0n...@mail.gmail.com/

> Signed-off-by: Marc Strapetz 

Re: [PATCH v2 3/4] branch: deprecate "-l" option

[Eric Sunshine]

On Fri, Jun 22, 2018 at 05:24:14AM -0400, Jeff King wrote:
> Let's deprecate "-l" in hopes of eventually re-purposing it
> to "--list".
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -36,6 +36,7 @@ static const char * const builtin_branch_usage[] = {
> +static int used_deprecated_reflog_option;
> @@ -573,6 +574,14 @@ static int edit_branch_description(const char 
> *branch_nam> +static int deprecated_reflog_option_cb(const struct option *opt,
> +const char *arg, int unset)
> +{
> + used_deprecated_reflog_option = 1;
> + *(int *)opt->value = !unset;
> + return 0;
> +}
> @@ -615,7 +624,13 @@ int cmd_branch(int argc, const char **argv, const char 
> *p> + OPT_BOOL(0, "create-reflog", , N_("create the 
> branch's reflog")),
> + {
> + OPTION_CALLBACK, 'l', NULL, , NULL,
> + N_("deprecated synonym for --create-reflog"),
> + PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
> + deprecated_reflog_option_cb
> @@ -688,6 +703,11 @@ int cmd_branch(int argc, const char **argv, const char 
> *p> + if (used_deprecated_reflog_option && !list) {
> + warning("the '-l' alias for '--create-reflog' is deprecated;");
> + warning("it will be removed in a future version of Git");
> + }

I wonder if it would be better and cleaner to limit the visibility of
this change to cmd_branch(), rather than spreading it across a global
variable, a callback function, and cmd_branch(). Perhaps, like this:

--- >8 ---
diff --git a/builtin/branch.c b/builtin/branch.c
index 5217ba3bde..893e5f481a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -584,6 +584,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
int icase = 0;
static struct ref_sorting *sorting = NULL, **sorting_tail = 
struct ref_format format = REF_FORMAT_INIT;
+   int deprecated_reflog_option = 0;
 
struct option options[] = {
OPT_GROUP(N_("Generic options")),
@@ -615,7 +616,9 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BIT('c', "copy", , N_("copy a branch and its reflog"), 
1),
OPT_BIT('C', NULL, , N_("copy a branch, even if target 
exists"), 2),
OPT_BOOL(0, "list", , N_("list branch names")),
-   OPT_BOOL('l', "create-reflog", , N_("create the branch's 
reflog")),
+   OPT_BOOL(0, "create-reflog", , N_("create the branch's 
reflog")),
+   OPT_HIDDEN_BOOL('l', NULL, _reflog_option,
+   N_("deprecated synonym for --create-reflog")),
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
OPT__FORCE(, N_("force creation, move/rename, deletion"), 
PARSE_OPT_NOCOMPLETE),
@@ -688,6 +691,12 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (list)
setup_auto_pager("branch", 1);
 
+   if (deprecated_reflog_option && !list) {
+   reflog = 1;
+   warning("the '-l' alias for '--create-reflog' is deprecated;");
+   warning("it will be removed in a future version of Git");
+   }
+
if (delete) {
if (!argc)
die(_("branch name required"));
--- >8 ---

Re: [PATCH] Documentation: Spelling and grammar fixes

[Eric Sunshine]

On Fri, Jun 22, 2018 at 2:50 AM Ville Skyttä  wrote:
> Signed-off-by: Ville Skyttä 
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -354,7 +354,7 @@ advice.*::
> ignoredHook::
> -   Advice shown if an hook is ignored because the hook is not
> +   Advice shown if a hook is ignored because the hook is not

British vs. American English, I believe. Since the project leans
toward[1] American English, this change is probably reasonable in the
larger scope of this patch.

[1]: https://public-inbox.org/git/xmqq4m9gpebm@gitster.mtv.corp.google.com/

> diff --git a/Documentation/technical/api-gitattributes.txt 
> b/Documentation/technical/api-gitattributes.txt
> @@ -146,7 +146,7 @@ To get the values of all attributes associated with a 
> file:
>  * Iterate over the `attr_check.items[]` array to examine
>the attribute names and values.  The name of the attribute
> -  described by a  `attr_check.items[]` object can be retrieved via
> +  described by an `attr_check.items[]` object can be retrieved via

This also collapses the space-space to a single space, which is good.

All the other changes look fine, as well. Thanks.

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

[Eric Sunshine]

On Thu, Jun 21, 2018 at 4:05 PM Junio C Hamano  wrote:
> Elijah Newren  writes:
> > + # This is indented with HT SP HT.
> > + echo "  foo();" >>foo &&
>
> I often wonder, whenever I see a need for a comment like this, if
> saying the same thing in code to make it visible is cleaner and less
> error prone way to do so, i.e. e.g.
>
> echo "_ _foo();" | tr "_" "\011" >>foo &&

Or use q_to_tab() from test-lib-functions.h:

echo "q qfoo();" | q_to_tab

Re: t5562: gzip -k is not portable

[Eric Sunshine]

On Tue, Jun 19, 2018 at 2:06 PM Junio C Hamano  wrote:
> Torsten Bögershausen  writes:
> > t5562 fails here under MacOS:
> > "gzip -k"  is not portable.

Very odd. Stock /usr/bin/gzip on my MacOS 10.12.6 _does_ recognize -k,
and the test does pass.

> Sigh.  Perhaps -c would help.  Or do BSD implementations also lack -c?

MacOS and BSD do support -c, so this solution would also work (and is
"cleaner" the the other proposal).

> diff --git a/t/t5562-http-backend-content-length.sh 
> b/t/t5562-http-backend-content-length.sh
> @@ -61,9 +61,9 @@ test_expect_success 'setup' '
>  test_expect_success GZIP 'setup, compression related' '
> -   gzip -k fetch_body &&
> +   gzip -c fetch_body >fetch_body.gz &&
> test_copy_bytes 10 fetch_body.gz.trunc &&
> -   gzip -k push_body &&
> +   gzip -c push_body >push_body.gz &&
> test_copy_bytes 10 push_body.gz.trunc
>  '

Re: t5310 broken under Mac OS

[Eric Sunshine]

On Tue, Jun 19, 2018 at 1:33 PM Torsten Bögershausen  wrote:
> expecting success:
>  git repack -ad &&
>  git rev-list --use-bitmap-index --count --all >expect &&
>  bitmap=$(ls .git/objects/pack/*.bitmap) &&
>  test_when_finished "rm -f $bitmap" &&
>  head -c 512 <$bitmap >$bitmap.tmp &&
>  mv $bitmap.tmp $bitmap &&
>  git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
>  test_cmp expect actual &&
>  test_i18ngrep corrupt stderr
>
> override r--r--r--  tb/staff for
> .git/objects/pack/pack-8886db3fce4f9657c1a43fee7d3ea4f2a4b5be2d.bitmap?
> (y/n [n]) not overwritten
> error: 'grep corrupt stderr' didn't find a match in:

This is fixed by [1], isn't it?

[1]: https://public-inbox.org/git/20180616191400.gb8...@sigill.intra.peff.net/

Re: [PATCH 2/6] git-p4: python3: replace dict.has_key(k) with "k in dict"

[Eric Sunshine]

On Tue, Jun 19, 2018 at 4:04 AM Luke Diamand  wrote:
> Python3 does not have the dict.has_key() function, so replace all
> such calls with "k in dict". This will still work with python2.6
> and python2.7.
>
> Converted using 2to3 (plus some hand-editing)
>
> Signed-off-by: Luke Diamand 
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -3141,7 +3141,7 @@ def importP4Labels(self, stream, p4Labels):
> -if change.has_key('change'):
> +if 'change' in change:

Very existential.

All these changes look sensible (as one might expect from automated conversion).

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

[Eric Sunshine]

On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam
 wrote:
> On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote:
> > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich  wrote:
> >> Should we put the part about MacOS's make into the commit
> >> message? Seems like relevant information for future readers.
> >
> > No. The bit of commentary mentioning MacOS's very old 'make' was just
> > talking about a possible alternate way of implementing the change.
> > That alternative was not chosen, so talking about old 'make' in the
> > commit message would be confusing for readers.
>
> Interesting. Documentation/SubmittinPatches reads:
>
> The body should provide a meaningful commit message, which:
> 
> . alternate solutions considered but discarded, if any.
>
> The consensus has changed, maybe? In which case, should we remove that
> statement from there?

Whether or not to talk about alternate solutions in the commit message
is a judgment call. Same for deciding what belongs in the commit
message proper and what belongs in the "commentary" section of a
patch. A patch author should strive to convey the problem succinctly
in the commit message, to not overload the reader with unnecessary (or
confusing) information, while, at the same time, not be sparing with
information which is genuinely needed to understand the problem and
solution.

Often, this can be done without talking about alternatives; often even
without spelling out the solution in detail or at all since the
solution may be "obvious", given a well-written problem description.
Complex cases, or cases in which multiple solutions may be or seem
valid, on the other hand, might warrant talking about those alternate
solutions, so we probably don't want to drop that bullet point.
Perhaps, instead, it can be re-worded a bit to make it sound something
other than mandatory (but I can't think of a good way to phrase it;
maybe you can?).

Re: [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options

[Eric Sunshine]

On Sun, Jun 17, 2018 at 1:59 AM Elijah Newren  wrote:
> git rebase has many options that only work with one of its three backends.
> It also has a few other pairs of incompatible options.  Document these.
>
> Signed-off-by: Elijah Newren 
> ---
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> @@ -487,6 +510,51 @@ recreates the topic branch with fresh commits so it can 
> be remerged
> +Flags only understood by the interactive backend:
> + [...]
> + * --edit-todo
> + * --root + --onto

What does "+" mean? Is it "and"?

> +Other incompatible flag pairs:
> +
> + * --preserve-merges && --interactive
> + * --preserve-merges && --signoff
> + * --preserve-merges && --rebase-merges
> + * --rebase-merges && --strategy

It's somewhat odd seeing "programmer" notation in end-user
documentation. Perhaps replace "&&" with "and"?

Re: [PATCH] doc: fix typos in documentation and release notes

[Eric Sunshine]

On Sat, Jun 16, 2018 at 11:36 PM Karthikeyan  wrote:
> On Sun, Jun 17, 2018, 8:55 AM Eric Sunshine  wrote:
>> Please sign-off[1] your patch so it can be included in the project.
>
> Thanks. I am a beginner using Gmail and I have used SubmitToGit for
> this. Is it possible to do this using SubmitToGit or should I do git
> commit --amend -s to sign off and make a force push to submit it
> again from SubmitToGit as newer one?

Amending the commit with sign-off and re-submit should work fine.
Alternately, you can reply to this email thread with your sign off
in-line, like this:

Signed-off-by: Your Name 

Re: Is NO_ICONV misnamed or is it broken?

[Eric Sunshine]

On Sat, Jun 16, 2018 at 10:57 PM Christian Couder
 wrote:
> On Fri, Jun 15, 2018 at 12:47 AM, Mahmoud Al-Qudsi  
> wrote:
> > ~> make NO_ICONV=1
> > # ommitted
> > LINK git-credential-store
> > /usr/bin/ld: cannot find -liconv
> I think 597c9cc540 (Flatten tools/ directory to make build procedure
> simpler., 2005-09-07) which introduces NEEDS_LIBICONV is even older
> than the commit that introduced NO_ICONV (see above), so you might
> want to play with NEEDS_LIBICONV too and see if it works better for
> you.
>
> I CC'ed the people involved in related commits. Maybe they can give
> you a better answer. It might also help if you could tell us on which
> OS/Platform and perhaps for which purpose you want to compile Git.

For completeness, for those reading this thread, a patch fixing this
issue has already been sent[1] to the list.

[1]: 
https://public-inbox.org/git/20180615022503.34111-1-sunsh...@sunshineco.com/

Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts

[Eric Sunshine]

On Sat, Jun 16, 2018 at 12:08 PM Elijah Newren  wrote:
> Subject: [PATCH] sequencer: do not squash 'reword' commits when we hit
>  conflicts
> [...]
> Signed-off-by: Elijah Newren 
> ---
> diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
> @@ -0,0 +1,44 @@
> +test_expect_success 'reword comes after a conflict preserves commit' '

s/comes//

> +   git checkout stuff^0 &&
> +
> +   set_fake_editor &&
> +   test_must_fail env FAKE_LINES="reword 2" \
> +   git rebase -i -v master &&

If some command fails between here and "git rebase --continue" below,
then the test will exit with an in-progress (dangling) rebase, which
could confuse tests added later to this script. I wonder, therefore,
if it would make sense to insert the following cleanup before "git
rebase -i":

test_when_finished "reset_rebase" &&

> +   git checkout --theirs file-2 &&
> +   git add file-2 &&
> +   FAKE_COMMIT_MESSAGE="feature_b_reworded" git rebase --continue &&
> +
> +   test "$(git log -1 --format=%B)" = "feature_b_reworded" &&
> +   test $(git rev-list --count HEAD) = 2
> +'

Re: [PATCH] doc: fix typos in documentation and release notes

[Eric Sunshine]

On Sat, Jun 16, 2018 at 2:08 PM Xtreak  wrote:
> doc: fix typos in documentation and release notes

Thanks for the patch. All the fixes look "obviously correct".

Please sign-off[1] your patch so it can be included in the project.

[1]: 
https://github.com/git/git/blob/v2.8.1/Documentation/SubmittingPatches#L234-L286

Re: What's cooking in git.git (Jun 2018, #04; Fri, 15)

[Eric Sunshine]

On Fri, Jun 15, 2018 at 4:27 PM Junio C Hamano  wrote:
> * es/make-no-iconv (2018-06-15) 1 commit
>  - Makefile: make NO_ICONV really mean "no iconv"
>
>  "make NO_ICONV=NoThanks" did not override NEEDS_LIBICONV

Nit: The double-negative is a bit confusing. s/NoThanks/YesPlease/
would be easier to grok and would be more idiomatic (taking
config.mak.uname into account).

>  (i.e. linkage of -lintl, -liconv, etc. that are platform-specific
>  tweaks), which has been corrected.

Re: [PATCH] tests: clean after SANITY tests

[Eric Sunshine]

On Fri, Jun 15, 2018 at 2:30 PM Junio C Hamano  wrote:
> Some of our tests try to make sure Git behaves sensibly in a
> read-only directory, by dropping 'w' permission bit before doing a
> test and then restoring it after it is done.  The latter is needed
> for the test framework to clean after itself without leaving a
> leftover directory that cannot be removed.
> [...]
> Signed-off-by: Junio C Hamano 
> ---
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> @@ -287,6 +287,7 @@ test_expect_success 'init notices EEXIST (2)' '
>  test_expect_success POSIXPERM,SANITY 'init notices EPERM' '
> +   test_when_finished "chmod +w newdir" &&
> rm -fr newdir &&
> mkdir newdir &&
> chmod -w newdir &&

When reading this, I was wondering if there was a "rm -fr newdir" at
the end of the test which could be removed now that it uses
test_when_finished(). Checking beyond the shown context, I see that it
doesn't bother with removal since the next test removes the directory
as a preparatory step. Even before the addition of
test_when_finished() to restore write permission, the subsequent
test's removal of the directory worked because this is test is only
run when POSIXPERM is met, and POSIX allows for such an operation.
Okay, looks good.

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

[Eric Sunshine]

On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich  wrote:
> On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote:
> > This patch is extra noisy due to the indentation change. Viewing it with
> > "git diff -w" helps. An alternative to re-indenting would have been to
> > "undefine NEEDS_LIBICONV", however, 'undefine' was added to GNU make in
> > 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.
>
> Should we put the part about MacOS's make into the commit
> message? Seems like relevant information for future readers.

No. The bit of commentary mentioning MacOS's very old 'make' was just
talking about a possible alternate way of implementing the change.
That alternative was not chosen, so talking about old 'make' in the
commit message would be confusing for readers. More importantly,
although that alternative would have made a less noisy patch, the
actual result would have made the Makefile itself noisier and uglier,
particularly for people just reading the Makefile in the future,
people who did not read the patch. Specifically, these alternatives
were considered:

ifdef NO_ICONV
undefine NEEDS_LIBICONV
endif
ifdef NEEDS_LIBICONV
...as before...
endif

and:

ifdef NO_ICONV
NEEDS_LIBICONV=
endif
ifeq ($(NEEDS_LIBICONV),)
...as before...
endif

Both of which are uglier for a future reader of Makefile than the
end-result actually implemented by this patch:

ifndef NO_ICONV
ifdef NEEDS_LIBICONV
...as before...
endif
endif

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

[Eric Sunshine]

On Fri, Jun 15, 2018 at 12:20 AM Jeff King  wrote:
> We have OLD_ICONV, too, which should probably do nothing if NO_ICONV is
> set. I think that works OK. We end up setting -DOLD_ICONV on the command
> line, but that's only consider inside "#ifndef NO_ICONV" within the
> code.

Right, that was my conclusion, as well. Since it works as is, I'm not
sure suppressing -DOLD_ICONV in Makefile is worth the extra patch
noise. I can re-roll with that change too, if someone thinks it's
worthwhile, though.

[PATCH] Makefile: make NO_ICONV really mean "no iconv"

[Eric Sunshine]

The Makefile tweak NO_ICONV is meant to allow Git to be built without
iconv in case iconv is not installed or is otherwise dysfunctional.
However, NO_ICONV's disabling of iconv is incomplete and can incorrectly
allow "-liconv" to slip into the linker flags when NEEDS_LIBICONV is
defined, which breaks the build when iconv is not installed.

On some platforms, iconv lives directly in libc, whereas, on others it
resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the
Makefile to add "-liconv" to the linker flags. config.mak.uname
automatically defines NEEDS_LIBICONV for platforms which require it.
The adding of "-liconv" is done unconditionally, despite NO_ICONV.

Work around this problem by making NO_ICONV take precedence over
NEEDS_LIBICONV.

Reported by: Mahmoud Al-Qudsi 
Signed-off-by: Eric Sunshine 
---

This patch is extra noisy due to the indentation change. Viewing it with
"git diff -w" helps. An alternative to re-indenting would have been to
"undefine NEEDS_LIBICONV", however, 'undefine' was added to GNU make in
3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.

Reported here: 
https://public-inbox.org/git/cacctrkepbgycbxnen5nz+cs-tidyye_dkdwttxpp0cueeic...@mail.gmail.com/T/#u

 Makefile | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index 1d27f36365..e4b503d259 100644
--- a/Makefile
+++ b/Makefile
@@ -1351,17 +1351,19 @@ ifdef APPLE_COMMON_CRYPTO
LIB_4_CRYPTO += -framework Security -framework CoreFoundation
 endif
 endif
-ifdef NEEDS_LIBICONV
-   ifdef ICONVDIR
-   BASIC_CFLAGS += -I$(ICONVDIR)/include
-   ICONV_LINK = -L$(ICONVDIR)/$(lib) 
$(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)
-   else
-   ICONV_LINK =
-   endif
-   ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
-   ICONV_LINK += -lintl
+ifndef NO_ICONV
+   ifdef NEEDS_LIBICONV
+   ifdef ICONVDIR
+   BASIC_CFLAGS += -I$(ICONVDIR)/include
+   ICONV_LINK = -L$(ICONVDIR)/$(lib) 
$(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)
+   else
+   ICONV_LINK =
+   endif
+   ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
+   ICONV_LINK += -lintl
+   endif
+   EXTLIBS += $(ICONV_LINK) -liconv
endif
-   EXTLIBS += $(ICONV_LINK) -liconv
 endif
 ifdef NEEDS_LIBGEN
EXTLIBS += -lgen
-- 
2.18.0.rc1.256.g331a1db143

Re: (Bug report + fix) gitk "IgnCase" search doesn't ignore case

[Eric Sunshine]

On Thu, Jun 14, 2018 at 02:53:03PM +0200, Juan Navarro wrote:
> Gitk "find commit" search function doesn't follow the "IgnCase" option that
> is selectable with a combo selector on the right side of the window; it
> should be searching in a case-insensitive way, but it doesn't.
> 
> Steps to reproduce:
> [...]
> 3. In the "Find commit" bar, select "changing lines matching"
> 4. In the right side of the same bar, select "IgnCase"
> [...]
> 
> Proposed solution is almost trivial: check if the "IgnCase" option is
> enabled, and in that case add the flag "-i" to the git command. Now that we
> are at it, it's probably correct to add that option to all search modes.
> A diff is attached to this email, hoping that someone is able to apply it
> (sorry I'm not familiarized with contributing with patch files, but the diff
> is pretty small anyways).

Thanks for reporting this.

A different way to interpret the situation is that the user-interface
is being inconsistent. For instance, the "fields" pop-up next to the
"exact/ignore-case/regexp" pop-up does not seem to make sense for
search types other than "containing", so it probably ought to be
disabled for anything other than "containing". By extension, one could
argue that the "exact/ignore-case/regexp" pop-up also ought be
disabled for non-"containing" searches. The fact that they are not
disabled confuses people into thinking that they should be functional
for all searches, not just for "containing" searches, even though such
functionality was never implemented (and indeed, may be difficult to
implement fully).

Your proposed fix handles only the "ignore case" item; it does not
implement "regexp" functionality, so it could be considered
incomplete. A more complete fix would also disable the "regexp" item
to avoid misleading users, and to head off future bug reports similar
to this one saying that "regexp" doesn't work for non-"containing"
searches. (Bonus points for also disabling the "fields" pop-up for
non-"containing" searches when it's not applicable.)

Below is your fix wrapped up as a proper patch and sent in-line rather
than as an attachment. It's also slightly simplified by dropping the
unnecessary extra variable. You'll need to sign-off on the patch if it
is ever to be accepted. You can do so by adding it after my sign-off.
If you don't feel like re-sending the patch with your sign-off, you
can alternately reply to this email with a "Signed-off-by: Your Name
" line.

Note, however, that the gitk project is, at best, deeply slumbering,
so it's not clear when or even if patches will be incorporated.
(Indeed, other recent gitk-related patches sent to the mailing list
have not yet been picked up.)

--- >8 ---
From: Juan Navarro 
Subject: [PATCH] gitk: support "ignore case" for non-"containing" searches

The "Exact/Ignore Case/Regexp" pop-up control only affects "containing"
searches. Other types of searches ("touching paths", "adding/removing
strings", "changing lines matching") ignore this option. Improve the
user experience by also recognizing "ignore case" for non-"containing"
searches.

Note: This change only implements the "ignore case" option for these
other search types; it does not add support for the "regexp" option
(which still only affects "containing" searches). A more complete "fix"
would improve the user experience even more by making the UI more
consistent; namely, by disabling options which don't make sense or are
not easily implemented for non-"containing" searches. In particular, the
"regexp" pop-up item and the neighboring "fields" pop-up control ought
perhaps be disabled when a non-"containing" search is selected.

[es: wrote commit message; slightly simplified proposed "fix"]

Signed-off-by: Eric Sunshine 
---
diff --git a/gitk-git/gitk b/gitk-git/gitk
index a14d7a16b2..fbb75f7390 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -4806,6 +4806,9 @@ proc do_file_hl {serial} {
# must be "containing:", i.e. we're searching commit info
return
 }
+if {$findtype eq [mc "IgnCase"]} {
+   set gdtargs [linsert $gdtargs 0 "-i"]
+}
 set cmd [concat | git diff-tree -r -s --stdin $gdtargs]
 set filehighlight [open $cmd r+]
 fconfigure $filehighlight -blocking 0
-- 
2.18.0.rc1.256.g331a1db143

Re: [PATCH v2] packfile: Correct zlib buffer handling

[Eric Sunshine]

On Wed, Jun 13, 2018 at 2:32 PM Junio C Hamano  wrote:
> Eric Sunshine  writes:
> > On this project, the character mnemonic "NUL" is typically used, not
> > "null" or "NULL" (which is typically reserved for pointers), so:
> > s/null/NUL/g
>
> Correct but I did not think it is a per-project preference; rather,
> "NUL is the name of the byte" is universal ;-)

Yes, the _mnemonic_ NUL is universal, but the character itself is
sometimes named or described as the "null character". I was just being
pedantic when "this project", by which I meant that we (on this
project) prefer the mnemonic "NUL" over longhand "null character",
whereas other projects may perhaps prefer "null character" or not
care.

Re: [RFC PATCH v2 2/4] git-credential-netrc: minor whitespace cleanup in test script

[Eric Sunshine]

On Wed, Jun 13, 2018 at 1:21 PM Todd Zullinger  wrote:
> Eric Sunshine wrote:
> > Since you're touching all the tests in this script anyhow, perhaps
> > modernize them [...]
> > (Not necessarily worth a re-roll.)
>
> These tests were based on similar test_external tests which
> use perl. like t0202 & t9700.  Both examples use the same
> formatting (and use of 'set up').  Perhaps a later clean up
> can adjust all three tests?

Whichever course of action works for you and Junio is fine. In this
case, it's such a minor bit of additional work to modernize the two
tests in this script that it would make sense to do so in this patch
if you happen to re-roll (and if you agree with me), but is itself
probably not worth a re-roll (as mentioned above).

Re: [PATCH v2] packfile: Correct zlib buffer handling

[Eric Sunshine]

A couple comments if you happen to re-roll...

On Wed, Jun 13, 2018 at 10:22 AM Jeremy Linton  wrote:
> The buffer being passed to zlib includes a null terminator that

On this project, the character mnemonic "NUL" is typically used, not
"null" or "NULL" (which is typically reserved for pointers), so:
s/null/NUL/g

> git needs to keep in place. unpack_compressed_entry() attempts to
> detect the case that the source buffer hasn't been fully consumed
> by checking to see if the destination buffer has been over consumed.
>
> This causes a problem, that more recent zlib patches have been
> poisoning the unconsumed portions of the buffer which overwrites
> the null, while correctly returning length and status.
>
> Let's replace the null at the end of the buffer to assure that
> if its been overwritten by zlib it doesn't result in problems for
> git.
>
> Signed-off-by: Jeremy Linton 
> ---
> diff --git a/packfile.c b/packfile.c
> @@ -1433,6 +1433,8 @@ static void *unpack_compressed_entry(struct packed_git 
> *p,
> +   buffer[size] = 0; /* assure that the buffer is still terminated */

I think we normally use '\0' for NUL on this project rather than simply 0.

The comment is also effectively pure noise since it merely repeats
what the code already states clearly (especially when the code says
"buffer[size] = '\0';"), so dropping the comment altogether would be
reasonable.

Re: [RFC PATCH v2 2/4] git-credential-netrc: minor whitespace cleanup in test script

[Eric Sunshine]

On Tue, Jun 12, 2018 at 11:10 PM, Todd Zullinger  wrote:
> Signed-off-by: Todd Zullinger 
> ---
> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh 
> b/contrib/credential/netrc/t-git-credential-netrc.sh
> index 58191a62f8..c5661087fe 100755
> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
> @@ -17,15 +17,15 @@
> # set up test repository
>
> test_expect_success \
> -'set up test repository' \
> -'git config --add gpg.program test.git-config-gpg'
> +   'set up test repository' \
> +   'git config --add gpg.program test.git-config-gpg'

Since you're touching all the tests in this script anyhow, perhaps
modernize them so the title and opening quote of the test body are on
the same line as test_expect_success, and the closing body quote is on
a line of its own?

test_expect_sucess 'setup test repository' '
...test body...
'

I also changed "set up" to "setup" to follow existing practice.

(Not necessarily worth a re-roll.)

> # The external test will outputs its own plan
> test_external_has_tap=1
>
> test_external \
> -'git-credential-netrc' \
> -perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
> +   'git-credential-netrc' \
> +   perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
>
> test_done
>  )

Re: [PATCHv2 0/6] git-p4: some small fixes updated

[Eric Sunshine]

On Tue, Jun 12, 2018 at 5:49 PM, Luke Diamand  wrote:
> Thanks. While on the subject of git-p4, I thought I should mention
> that I've been looking at getting git-p4 to work with Python3.
>
> I've got some low risk easy (mostly automated) changes which get it to
> the point where it compiles. After that I have to figure out how to
> fix the byte-vs-string unicode problem that Python3 brings. [...]

See how reposurgeon handles the problem[1]. It defines polystr() and
polybytes() functions which coerce strings and byte sequences as
needed. It's not pretty, but it works.

[1]: https://gitlab.com/esr/reposurgeon/blob/master/reposurgeon#L100

Re: [PATCH] t7400: encapsulate setup code in test_expect_success

[Eric Sunshine]

On Tue, Jun 12, 2018 at 5:25 PM, Stefan Beller  wrote:
> When running t7400 in a shell you observe more output than expected:
> ...
> ok 10 - submodule add
> [master (root-commit) d79ce16] one
>  Author: A U Thor 
>  1 file changed, 1 insertion(+)
>  create mode 100644 one.t
> ok 11 - redirected submodule add does not show progress
> ...
> Fix the output by encapsulating the setup code in test_expect_success
>
> Signed-off-by: Stefan Beller 
> ---
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> @@ -126,8 +126,10 @@ test_expect_success 'submodule add' '
> -test_create_repo parent &&
> -test_commit -C parent one
> +test_expect_success 'setup parent and one repository for further tests' '

Nit: "for further tests" is implied for actions performed by a "setup"
function, so a bit redundant to say so.

> +   test_create_repo parent &&
> +   test_commit -C parent one
> +'

Re: How to delete files and directories from git commit history?

[Eric Sunshine]

On Tue, Jun 12, 2018 at 4:26 PM, Christian Couder
 wrote:
> On Tue, Jun 12, 2018 at 9:44 PM, Steve Litt  wrote:
>
>> My project (call it myproject) had a directory (call it docs/propdir)
>> that was unnecessary for the project, and I've decided I don't want to
>> offer the files in that directory as free software. So I need to delete
>> docs/propdir from all commits in the repository. I did the following,
>> while in my working repository's myproject directory:
>>
>> git filter-branch --tree-filter 'rm -rf docs/propdir' HEAD
>
>> What command do I do to remove all mention of doc/propdir and its
>> files from my git history?
>
> Did you check the "CHECKLIST FOR SHRINKING A REPOSITORY" section of
> the filter-branch man/help page?

Also see BFG Repo Cleaner for an alternative.
https://rtyley.github.io/bfg-repo-cleaner/

Re: [PATCH v9] json_writer: new routines to create JSON data

[Eric Sunshine]

On Tue, Jun 12, 2018 at 4:22 PM,   wrote:
> Add "struct json_writer" and a series of jw_ routines to compose JSON
> data into a string buffer.  The resulting string may then be printed by
> commands wanting to support a JSON-like output format.
>
> The json_writer is limited to correctly formatting structured data for
> output.  It does not attempt to build an object model of the JSON data.
>
> We say "JSON-like" because we do not enforce the Unicode (usually UTF-8)
> requirement on string fields.  Internally, Git does not necessarily have
> Unicode/UTF-8 data for most fields, so it is currently unclear the best
> way to enforce that requirement.  For example, on Linx pathnames can

s/Linx/Linux/

> contain arbitrary 8-bit character data, so a command like "status" would
> not know how to encode the reported pathnames.  We may want to revisit
> this (or double encode such strings) in the future.
>
> Signed-off-by: Jeff Hostetler 
> ---
> diff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c
> @@ -0,0 +1,564 @@
> +void get_s(int line_nr, char **s_in)

s/^/static/

> +{
> +   *s_in = strtok(NULL, " ");
> +   if (!*s_in)
> +   die("line[%d]: expected: ", line_nr);
> +}
> +
> +void get_i(int line_nr, intmax_t *s_in)

s/^/static/

> +{
> +   char *s;
> +   char *endptr;
> +
> +   get_s(line_nr, );
> +
> +   *s_in = strtol(s, , 10);
> +   if (*endptr || errno == ERANGE)
> +   die("line[%d]: invalid integer value", line_nr);
> +}
> +
> +void get_d(int line_nr, double *s_in)

s/^/static/

> +{
> +   char *s;
> +   char *endptr;
> +
> +   get_s(line_nr, );
> +
> +   *s_in = strtod(s, );
> +   if (*endptr || errno == ERANGE)
> +   die("line[%d]: invalid float value", line_nr);
> +}

Re: [PATCH] builtin/send-pack: populate the default configs

[Eric Sunshine]

On Tue, Jun 12, 2018 at 2:16 PM, Jonathan Nieder  wrote:
> Masaya Suzuki wrote:
>> builtin/send-pack didn't call git_default_config, and because of this
>> git push --signed didn't respect the username and email in gitconfig in
>> the HTTP transport.
>>
>> Signed-off-by: Masaya Suzuki 
>> ---
> send-pack is not a command served by a daemon so this is less
> potentially scary than the corresponding potential change to
> upload-pack or receive-pack.  Some configuration this brings in:
>
> - core.askpass: allows specifying an arbitrary command to use to
>   ask for a password.  Respecting this setting should be very useful,
>   even if it would be scary in a daemon.
>
> - core.pager: allows specifying an arbitrary command to use as a
>   pager, if pagination is on (but it shouldn't be on).
> - core.logallrefupdates: whether to create reflogs for new refs
>   (including new remote-tracking refs). Good.
> - core.abbrev: what length of abbreviations to use when printing
>   abbreviated object ids (good).
> - core.compression, core.packedgitwindowsize, etc: pack generation
>   tunables (good).
> - advice.*: would allow us to make "git push" produce configurable
>   advice (good!)
> - etc

This summary probably ought to be in the commit message itself (plus
additional commentary implied by the ending "etc."). It's exactly the
sort of information someone looking at this patch in the future will
want to know to feel assured that such behavior changes were taken
into account by the patch author.

Re: [PATCH] builtin/send-pack: report gpg config errors

[Eric Sunshine]

On Tue, Jun 12, 2018 at 1:53 PM, Stefan Beller  wrote:
> Any caller except of git_gpg_config() except the one in send_pack_config()

Drop the first "except"?

Also: s/Any caller/All callers/

> handles the return value of git_gpg_config(). Also handle the return value
> there.
>
> Signed-off-by: Stefan Beller 

Re: [PATCH 01/10] t: add tool to translate hash-related values

[Eric Sunshine]

On Mon, Jun 11, 2018 at 9:05 PM, brian m. carlson
 wrote:
> On Mon, Jun 11, 2018 at 03:47:43AM -0400, Eric Sunshine wrote:
>> The word "translate" is very generic and is (at least in my mind)
>> strongly associated with i18n/l10n, so the name test_translate() may
>> be confusing for readers. Perhaps test_oid_lookup() or test_oid_get()
>> or even just test_oid()?
>
> test_oid would be fine.  One note is that this doesn't always produce
> OIDs; sometimes it will produce other values, but as long as you don't
> think that's too confusing, I'm fine with it.

It was surprising to see it used for non-OID's (such as hash
characteristics), but not hard to deal with.

One could also view this as a generic key/value cache (not specific to
OID's) with overriding super-key (the hash algorithm, in this case),
which would allow for more generic name than test_oid(), but we don't
have to go there presently.

>> This is a very expensive lookup since it invokes a heavyweight command
>> (perl, in this case) for *every* OID it needs to retrieve from the
>> file. Windows users, especially, will likely not be happy about this.
>> See below for an alternative.
>
> I agree perl would be expensive if it were invoked frequently, but
> excepting SHA1-prerequisite tests, this function is invoked 32 times in
> the entire testsuite.
>
> One of the reasons I chose perl was because we have a variety of cases
> where we'll need spaces in values, and those tend to be complex in
> shell.

Can you give examples of cases in which values will contain spaces? It
wasn't obvious from this patch series that such a need would arise.

Are these values totally free-form? If not, some character (such as
"_", "-", ".", etc.) could act as a stand-in for space. That shouldn't
be too hard to handle.

>> Here's what I had envisioned when reading your emails about OID lookup
>> table functionality:
>>
>> --- >8 ---
>> test_oid_cache () {
>> while read tag rest
>> do
>> case $tag in \#*) continue ;; esac
>>
>> for x in $rest
>> do
>> k=${x%:*}
>> v=${x#*:}
>> if test "$k" = $test_hash_algo
>> then
>> eval "test_oid_$tag=$v"
>> break
>> fi
>> done
>> done
>> }
>
> Using shell variables like this does have the downside that we're
> restricted to only characters allowed in shell variables.  That was
> something I was trying to avoid, but it certainly isn't fatal.

Is that just a general concern or do you have specific "weird" keys in mind?

>> test_detect_hash() would detect the hash algorithm and record it
>> instead of having to determine it each time an OID needs to be
>> "translated". It probably would be called by test-lib.sh.
>
> We'll probably have to deal with multiple hashes in the future,
> including for input and output, but this could probably be coerced to
> handle that case.

My original version of test_oid_cache() actually allowed for that by
caching _all_ information from the tables rather than only values
corresponding to $test_hash_algo. It looked like this:

--- >8 ---
test_oid_cache () {
while read tag rest
do
case $tag in \#*) continue ;; esac

for x in $rest
do
eval "test_oid_${tag}_${x%:*}=${x#*:}"
done
done
}
--- >8 ---

The hash algorithm is incorporated into the cache variable name like
this: "test_oid_hexsz_sha256"

>> And, when specifying values from which to choose based upon hash
>> algorithm:
>>
>> $(test_oid bored sha1:deadbeef NewHash:feedface)
>
> This syntax won't exactly be usable, because we have to deal with spaces
> in values.  It shouldn't be too much of a problem to just use
> test_oid_cache at the top of the file, though.

See above about possibly using a stand-in character for space.

>> A nice property of how this implementation caches values is that you
>> don't need test_oid() for really simple cases. You can just access the
>> variable directly. For instance: $test_oid_hexsz
>
> Because we're going to need multiple hash support in the future, for
> input, output, and on-disk, I feel like this is not a good direction for
> us to go in the testsuite.  Internally, those variable names are likely
> to change.

Indeed, this isn't a real selling point; I only just thought of it
while composing the mail. Going through the API is more robust.
(Although, see above how the revised test_oid_cache() incorporates the
hash algorithm into the cache variable name.)

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Eric Sunshine]

[cc:+torsten]

On Mon, Jun 11, 2018 at 9:46 PM, Anthony Sottile  wrote:
> On Wed, Jun 6, 2018 at 10:22 AM, Eric Sunshine  
> wrote:
>> Thanks for pointing that out. In that case, it's following existing
>> practice, thus certainly not worth a re-roll.
>
> Anything else for me to do here? (sorry! not super familiar with the process)

I don't think so. Nothing in the review warranted a re-roll, and
(importantly) Torsten gave his Acked-by:, so the next step is to wait
for Junio to pick up the patch. He's been offline for a bit, so it
might take a some time for him to catch up since the list has been
plenty busy in his absence.

It's a good idea to check the "What's Cooking" summaries Junio sends
to the mailing list once in a while to check the progress of your
patch. If you don't see it show up in the summary within a couple
weeks or so, it wouldn't hurt to ping again (as you did here).

Re: [PATCH 05/10] t0064: make hash size independent

[Eric Sunshine]

On Mon, Jun 4, 2018 at 7:52 PM, brian m. carlson
 wrote:
> Compute test values of the appropriate size instead of hard-coding
> 40-character values.  Rename the echo20 function to echoid, since the
> values may be of varying sizes.
>
> Signed-off-by: brian m. carlson 
> ---
> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
> @@ -3,30 +3,30 @@
> -echo20 () {
> +echoid () {
> prefix="${1:+$1 }"
> shift
> while test $# -gt 0
> do
> -   echo "$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
> +   echo "$prefix$ZERO_OID" | sed -e "s/00/$1/g"
> shift
> done
>  }
>
>
>  test_expect_success 'lookup with almost duplicate values' '
> +   # n-1 5s
> +   root=$(test_translate 555 \
> +   
> 555) &&

This is rather unwieldy and ugly. How about simply re-using echoid()
to compute the value, like this:

root=$(echoid "" 55) &&
root=${root%5} &&

That is, use echoid() to generate the all-5's OID of correct length
and then chop the last '5' off the end.

> {
> -   echo "append " &&
> -   echo "append 555f" &&
> -   echo20 lookup 55
> +   id1="${root}5" &&
> +   id2="${root}f" &&
> +   echo "append $id1" &&
> +   echo "append $id2" &&
> +   echoid lookup 55

Re: [PATCH 03/10] t0002: abstract away SHA-1-specific constants

[Eric Sunshine]

On Mon, Jun 4, 2018 at 7:52 PM, brian m. carlson
 wrote:
> Adjust the test so that it computes variables for object IDs instead of
> using hard-coded hashes.
>
> Signed-off-by: brian m. carlson 
> ---
> diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
> @@ -89,14 +89,16 @@ test_expect_success 'enter_repo non-strict mode' '
> cd enter_repo &&
> test_tick &&
> test_commit foo &&
> +   git rev-parse HEAD >head-revision &&
> mv .git .realgit &&
> echo "gitdir: .realgit" >.git
> ) &&
> +   head=$(cat enter_repo/head-revision) &&

Stashing the value temporarily in a file ("head-revision") is
unnecessary and somewhat ugly. Just grab it directly after the
subshell exits:

(
cd enter_repo &&
...
) &&
head=$(git -C enter_repo rev-parse HEAD) &&
...

Re: [PATCH 01/10] t: add tool to translate hash-related values

[Eric Sunshine]

On Mon, Jun 04, 2018 at 11:52:20PM +, brian m. carlson wrote:
> Add a test function helper, test_translate, that will produce its first
> argument if the hash in use is SHA-1 and the second if its argument is
> NewHash.  Implement a mode that can read entries from a file as well for
> reusability across tests.

The word "translate" is very generic and is (at least in my mind)
strongly associated with i18n/l10n, so the name test_translate() may
be confusing for readers. Perhaps test_oid_lookup() or test_oid_get()
or even just test_oid()?

> Signed-off-by: brian m. carlson 
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -1147,3 +1147,43 @@ depacketize () {
> +test_translate_f_ () {
> + local file="$TEST_DIRECTORY/translate/$2" &&
> + perl -e '
> + $delim = "\t";
> + ($hoidlen, $file, $arg) = @ARGV;
> + open($fh, "<", $file) or die "open: $!";
> + while (<$fh>) {
> + # Allow specifying other delimiters.
> + $delim = $1 if /^#!\sdelimiter\s(.)/;
> + next if /^#/;
> + @fields = split /$delim/, $_, 3;
> + if ($fields[0] eq $arg) {
> + print($hoidlen == 40 ? $fields[1] : $fields[2]);
> + last;
> + }
> + }
> + ' "$1" "$file" "$3"
> +}

This is a very expensive lookup since it invokes a heavyweight command
(perl, in this case) for *every* OID it needs to retrieve from the
file. Windows users, especially, will likely not be happy about this.
See below for an alternative.

> +# Without -f, print the first argument if we are using SHA-1 and the second 
> if
> +# we're using NewHash.
> +# With -f FILE ARG, read the (by default) tab-delimited file from
> +# t/translate/FILE, finding the first field matching ARG and printing either 
> the
> +# second or third field depending on the hash in use.
> +test_translate () {
> + local hoidlen=$(printf "%s" "$EMPTY_BLOB" | wc -c) &&
> + if [ "$1" = "-f" ]
> + then
> + shift &&
> + test_translate_f_ "$hoidlen" "$@"
> + else
> + if [ "$hoidlen" -eq 40 ]
> + then
> + printf "%s" "$1"
> + else
> + printf "%s" "$2"
> + fi
> + fi
> +}

This is less flexible than I had expected, allowing for only SHA1 and
NewHash. When you had written about OID lookup table functionality in
email previously, my impression was that the tables would allow values
for arbitrary hash algorithms. Such flexibility would allow people to
experiment with hash algorithms without having to once again retrofit
the test suite machinery.

Here's what I had envisioned when reading your emails about OID lookup
table functionality:

--- >8 ---
test_detect_hash () {
test_hash_algo=...
}

test_oid_cache () {
while read tag rest
do
case $tag in \#*) continue ;; esac

for x in $rest
do
k=${x%:*}
v=${x#*:}
if test "$k" = $test_hash_algo
then
eval "test_oid_$tag=$v"
break
fi
done
done
}

test_oid () {
if test $# -gt 1
then
test_oid_cache <<-EOF
$*
EOF
fi
eval "echo \$test_oid_$1"
}
--- >8 ---

test_detect_hash() would detect the hash algorithm and record it
instead of having to determine it each time an OID needs to be
"translated". It probably would be called by test-lib.sh.

test_oid_cache() reads a table of OID's and caches them so that
subsequent look-ups are fast. It would be called (possibly multiple
times) by any script needing to "translate" OID's. Each line of the
table has form "tag algo:value ...". Lines starting with '#' are
comments (as in your implementation). Since it reads stdin, it works
equally well reading OID tables from files and here-docs, which
provides extra flexibility for test authors. For instance:

test_oid_cache 

Re: [PATCH] fetch-pack: don't try to fetch peeled values with --all

[Eric Sunshine]

On Mon, Jun 11, 2018 at 12:47 AM, Jeff King  wrote:
> Subject: fetch-pack: don't try to fetch peeled values with --all
> [...]
> Original report and test from Kirill Smelkov.
>
> Signed-off-by: Kirill Smelkov 
> Signed-off-by: Jeff King 
> ---
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' '
> +test_expect_success 'test --all wrt tag to non-commits' '
> +   blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) &&
> +   git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 &&
> +   tree_sha1=$(printf "100644 blob $blob_sha1\tfile\n" | git mktree) &&

Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even
simpler, just 'blob' and 'tree'.

> +   git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 &&
> +   mkdir fetchall &&
> +   (
> +   cd fetchall &&
> +   git init &&
> +   git fetch-pack --all .. &&

Simpler:

git init fetchall &&
(
cd fetchall &&
git fetch-pack --all .. &&

Although, I see that this script already has a mix of the two styles
(simpler and not-so-simple), so...

> +   git cat-file blob $blob_sha1 >/dev/null &&
> +   git cat-file tree $tree_sha1 >/dev/null
> +   )
> +'

Re: [PATCH] fsck: avoid looking at NULL blob->object

[Eric Sunshine]

On Sat, Jun 9, 2018 at 4:32 AM, Jeff King  wrote:
> Commit 159e7b080b (fsck: detect gitmodules files,
> 2018-05-02) taught fsck to look at the content of
> .gitmodules files. If the object turns out not to be a blob
> at all, we just complain and punt on checking the content.
> And since this was such an obvious and trivial code path, I
> didn't even bother to add a test.
> [...]
> Signed-off-by: Jeff King 
> ---
> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
> @@ -151,4 +151,22 @@ test_expect_success 'fsck detects symlinked .gitmodules 
> file' '
> +test_expect_success 'fsck detects non-blob .gitmodules' '
> +   git init non-blob &&
> +   (
> +   cd non-blob &&
> +
> +   # As above, make the funny directly to avoid index 
> restrictions.

Is there a word missing after "funny"?

> +   mkdir subdir &&
> +   cp ../.gitmodules subdir/file &&
> +   git add subdir/file &&
> +   git commit -m ok &&
> +   tree=$(git ls-tree HEAD | sed s/subdir/.gitmodules/ | git 
> mktree) &&
> +   commit=$(git commit-tree $tree) &&

I see that this is just mirroring the preceding test, but do you need
to assign to variable 'commit' which is never consulted by anything
later in the test?

> +   test_must_fail git fsck 2>output &&
> +   grep gitmodulesBlob output
> +   )
> +'

Re: [PATCH v8 1/2] json_writer: new routines to create data in JSON format

[Eric Sunshine]

On Thu, Jun 7, 2018 at 10:12 AM,   wrote:
> Add a series of jw_ routines and "struct json_writer" structure to compose
> JSON data.  The resulting string data can then be output by commands wanting
> to support a JSON output format.
> [...]
> Signed-off-by: Jeff Hostetler 
> ---
> diff --git a/t/t0019-json-writer.sh b/t/t0019-json-writer.sh
> @@ -0,0 +1,236 @@
> +test_expect_success 'simple object' '
> +   cat >expect <<-\EOF &&
> +   {"a":"abc","b":42,"c":3.14,"d":true,"e":false,"f":null}
> +   EOF
> +   test-json-writer >actual \
> +   @object \
> +   @object-string a abc \
> +   @object-int b 42 \
> +   @object-double c 2 3.140 \
> +   @object-true d \
> +   @object-false e \
> +   @object-null f \
> +   @end &&
> +   test_cmp expect actual
> +'

To make it easier on people writing these tests, it might be nice for
this to be less noisy by getting rid of "@" and "\". To get rid of
"\", the test program could grab its script commands from stdin (one
instruction per line) rather than from argv[]. For instance:

test-json-writer >actual <<-\EOF &&
object
object-string a abc
...
end
EOF

Not a big deal, and certainly not worth a re-roll.

Re: [PATCH v8 2/2] json-writer: t0019: add perl unit test

[Eric Sunshine]

On Thu, Jun 7, 2018 at 10:12 AM,   wrote:
> Test json-writer output using perl script.
>
> Signed-off-by: Jeff Hostetler 
> ---
> diff --git a/t/t0019/parse_json.perl b/t/t0019/parse_json.perl
> @@ -0,0 +1,52 @@
> +#!/usr/bin/perl
> +use strict;
> +use warnings;
> +use JSON;

This new script is going to have to be protected by a prerequisite
since the JSON module is not part of the standard Perl installation,
thus will not necessarily be installed everywhere (it isn't on any of
my machines, for instance). Something like:

test_lazy_prereq PERLJSON '
perl -MJSON -e "exit 0"
'

which would be used like this:

test_expect_success PERLJSON 'parse JSON using Perl' '
...
'

Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Eric Sunshine]

On Wed, Jun 6, 2018 at 3:16 PM, Duy Nguyen  wrote:
> On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine  
> wrote:
>> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
>> git-branch-diff[1] which computes differences between two versions of a
>> patch series. Such a diff can be a useful aid for reviewers when
>> inserted into a cover letter. However, doing so requires manual
>> generation (invoking git-branch-diff) and copy/pasting the result into
>> the cover letter.
>
> Another option which I wanted to go is delegate part of cover letter
> generation to a hook (or just a config key that contains a shell
> command). This way it's easier to customize cover letters. We could
> still have a good fallback that does shortlog, diffstat and tbdiff.

It is common on this mailing list to turn down requests for new hooks
when the requested functionality could just as easily be implemented
via a wrapper script. So, my knee-jerk reaction is that a hook to
customize the cover letter may be overkill when the same functionality
could likely be implemented relatively easily by a shell script which
invokes git-format-patch and customizes the cover letter
after-the-fact. Same argument regarding a config key holding a shell
command. But, perhaps there are cases which don't occur to me which
could be helped by a config variable or such.

Of course, by the same reasoning, the --range-diff functionality
implemented by this patch series, which is just a convenience, could
be handled by a wrapper script, thus is not strictly needed. On the
other hand, given that interdiffs and range-diffs are so regularly
used in re-rolls on this list (and perhaps other mailing list-based
projects) may be argument enough in favor of having such an option
built into git-format-patch.

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Eric Sunshine]

On Wed, Jun 6, 2018 at 1:18 PM, Anthony Sottile  wrote:
> On Wed, Jun 6, 2018 at 10:17 AM, Eric Sunshine  
> wrote:
>> On Wed, Jun 6, 2018 at 1:15 PM, Eric Sunshine  
>> wrote:
>>> On Mon, Jun 4, 2018 at 4:17 PM, Anthony Sottile  wrote:
>>>> +   for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
>>>
>>> Simpler: printf "%s\n" I am all CRLF | append_cr >allcrlf &&
>>
>> Or even simpler:
>>
>> printf "%s\r\n" I am all CRLF >allcrlf &&
>
> Yeah, I just copied the line in my test from another test in this file
> which was doing a ~similar thing. [...]

Thanks for pointing that out. In that case, it's following existing
practice, thus certainly not worth a re-roll.

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Eric Sunshine]

On Wed, Jun 6, 2018 at 1:15 PM, Eric Sunshine  wrote:
> On Mon, Jun 4, 2018 at 4:17 PM, Anthony Sottile  wrote:
>> +   for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
>
> Simpler: printf "%s\n" I am all CRLF | append_cr >allcrlf &&

Or even simpler:

printf "%s\r\n" I am all CRLF >allcrlf &&

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Eric Sunshine]

On Mon, Jun 4, 2018 at 4:17 PM, Anthony Sottile  wrote:
> A regression introduced in 8462ff43e42ab67cecd16fdfb59451a53cc8a945 caused
> autocrlf rewrites to produce a warning message despite setting safecrlf=false.
>
> Signed-off-by: Anthony Sottile 
> ---
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> @@ -98,6 +98,16 @@ test_expect_success 'safecrlf: git diff demotes 
> safecrlf=true to warn' '
> +test_expect_success 'safecrlf: no warning with safecrlf=false' '
> +   git config core.autocrlf input &&
> +   git config core.safecrlf false &&

I was going to suggest test_config() for these rather than bare
git-config, but I see other tests in this file already use the bare
form, so this is following existing practice.

> +   for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&

Simpler: printf "%s\n" I am all CRLF | append_cr >allcrlf &&

(probably not worth a re-roll)

> +   git add allcrlf 2>err &&
> +   test_must_be_empty err
> +'

Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails

[Eric Sunshine]

On Tue, Jun 5, 2018 at 6:59 AM Luke Diamand  wrote:
> On 5 June 2018 at 11:49, Merland Romain  wrote:
> >> +# now check that we can actually talk to the server
> >> +global p4_access_checked
> >> +if not p4_access_checked:
> >> +p4_access_checked = True
> >> +p4_check_access()
> >> +
> >
> > Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()'
> > It seems to me more logical
>
> Like this:
>
> +p4_check_access()
> +p4_access_checked = True
>
> You need to set p4_access_checked first so that it doesn't go and try
> to check the p4 access before running "p4 login -s", which would then
> get stuck forever.

Such subtlety may deserve an in-code comment.

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Eric Sunshine]

On Tue, Jun 5, 2018 at 6:56 AM Luke Diamand  wrote:
> On 5 June 2018 at 10:54, Eric Sunshine  wrote:
> > On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  wrote:
> >> +m = re.search('Too many rows scanned \(over (\d+)\)', 
> >> data)
> >> +if not m:
> >> +m = re.search('Request too large \(over (\d+)\)', 
> >> data)
> >
> > Does 'p4' localize these error messages?
>
> That's a good question.
>
> It turns out that Perforce open-sourced the P4 client in 2014 (I only
> recently found this out) so we can actually look at the code now!
>
> Here's the code:
>
> // ErrorId graveyard: retired/deprecated ErrorIds.

Hmm, the "too many rows" error you're seeing is retired/deprecated(?).

> ErrorId MsgDb::MaxResults  = { ErrorOf( ES_DB, 32,
> E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
> 'p4 help maxresults'." } ;//NOTRANS
> ErrorId MsgDb::MaxScanRows = { ErrorOf( ES_DB, 61,
> E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
> see 'p4 help maxscanrows'." } ;//NOTRANS
>
> I don't think there's actually a way to make it return any language
> other than English though. [...]
> So I think probably the language is always English.

The "NOTRANS" annotation on the error messages is reassuring.

Re: [PATCHv1 3/3] git-p4: auto-size the block

[Eric Sunshine]

On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  wrote:
> git-p4 originally would fetch changes in one query. On large repos this
> could fail because of the limits that Perforce imposes on the number of
> items returned and the number of queries in the database.
>
> To fix this, git-p4 learned to query changes in blocks of 512 changes,
> However, this can be very slow - if you have a few million changes,
> with each chunk taking about a second, it can be an hour or so.
>
> Although it's possible to tune this value manually with the
> "--changes-block-size" option, it's far from obvious to ordinary users
> that this is what needs doing.
>
> This change alters the block size dynamically by looking for the
> specific error messages returned from the Perforce server, and reducing
> the block size if the error is seen, either to the limit reported by the
> server, or to half the current block size.
>
> That means we can start out with a very large block size, and then let
> it automatically drop down to a value that works without error, while
> still failing correctly if some other error occurs.
>
> Signed-off-by: Luke Diamand 
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -48,7 +48,8 @@ def __str__(self):
>  # Grab changes in blocks of this many revisions, unless otherwise requested
> -defaultBlockSize = 512
> +# The code should now automatically reduce the block size if it is required
> +defaultBlockSize = 1<<20

As worded, the new comment only has value to someone who knew the old
behavior (before this patch), not for someone reading the code fresh.
However, if reworded, it might be meaningful to all readers (new and
old):

# The block size is reduced automatically if required

> @@ -983,10 +985,24 @@ def p4ChangesForPaths(depotPaths, changeRange, 
> requestedBlockSize):
> +try:
> +result = p4CmdList(cmd, errors_as_exceptions=True)
> +except P4RequestSizeException as e:
> +if not block_size:
> +block_size = e.limit
> +elif block_size > e.limit:
> +block_size = e.limit
> +else:
> +block_size = max(2, block_size // 2)
> +
> +if verbose: print("block size error, retry with block size 
> {0}".format(block_size))

Perhaps: s/retry/retrying/

> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
> @@ -129,6 +129,7 @@ test_expect_success 'Create a repo with multiple depot 
> paths' '
> +test_expect_success 'Clone repo with self-sizing block size' '
> +   test_when_finished cleanup_git &&
> +   git p4 clone --changes-block-size=100 //depot@all 
> --destination="$git" &&
> +   (
> +   cd "$git" &&
> +   git log --oneline > log &&
> +   test_line_count \> 10 log
> +   )
> +'

Or, without a subshell (and dropping whitespace after redirection operator):

git -C git log --oneline >log &&
test_line_count \> 10 log

(not worth a re-roll)

Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

[Eric Sunshine]

On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  wrote:
> This change lays some groundwork for better handling of rowcount errors
> from the server, where it fails to send us results because we requested
> too many.
>
> It adds an option to p4CmdList() to return errors as a Python exception.
>
> The exceptions are derived from P4Exception (something went wrong),
> P4ServerException (the server sent us an error code) and
> P4RequestSizeException (we requested too many rows/results from the
> server database).
>
> This makes makes the code that handles the errors a bit easier.
>
> The default behavior is unchanged; the new code is enabled with a flag.
>
> Signed-off-by: Luke Diamand 
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -566,10 +566,30 @@ def isModeExec(mode):
> +class P4ServerException(Exception):
> +""" Base class for exceptions where we get some kind of marshalled up 
> result from the server """
> +def __init__(self, exit_code, p4_result):
> +super(P4ServerException, self).__init__(exit_code)
> +self.p4_result = p4_result
> +self.code = p4_result[0]['code']
> +self.data = p4_result[0]['data']

The subsequent patches never seem to access any of these fields, so
it's difficult to judge whether it's worthwhile having 'code' and
'data' bits split out like this.

> @@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', 
> cb=None, skip_info=False):
>  if exitCode != 0:
> -entry = {}
> -entry["p4ExitCode"] = exitCode
> -result.append(entry)
> +if errors_as_exceptions:
> +if len(result) > 0:
> +data = result[0].get('data')
> +if data:
> +m = re.search('Too many rows scanned \(over (\d+)\)', 
> data)
> +if not m:
> +m = re.search('Request too large \(over (\d+)\)', 
> data)

Does 'p4' localize these error messages?

> +if m:
> +limit = int(m.group(1))
> +raise P4RequestSizeException(exitCode, result, limit)
> +
> +raise P4ServerException(exitCode, result)
> +else:
> +raise P4Exception(exitCode)
> +else:
> +entry = {}
> +entry["p4ExitCode"] = exitCode
> +result.append(entry)

Re: [PATCHv1 2/2] git-p4: add option to disable syncing of p4/master with p4

[Eric Sunshine]

On Tue, Jun 5, 2018 at 4:29 AM Luke Diamand  wrote:
> Add an option to the git-p4 submit command to disable syncing
> with Perforce.
>
> This is useful for the case where a git-p4 mirror has been setup
> on a server somewhere, running from (e.g.) cron, and developers
> then clone from this. Having the local cloned copy also sync
> from Perforce just isn't useful.
>
> Signed-off-by: Luke Diamand 
> ---
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> @@ -369,6 +369,11 @@ These options can be used to modify 'git p4 submit' 
> behavior.
> +--disable-p4sync::
> +Disable the automatic sync of p4/master from Perforce after commit have

s/commit/commits/

> +been submitted. Implies --disable-rebase. Can also be set with
> +git-p4.disableP4Sync. Sync with origin/master still goes ahead if 
> possible.
> diff --git a/git-p4.py b/git-p4.py
> @@ -1368,7 +1368,9 @@ def __init__(self):
> +optparse.make_option("--disable-p4sync", 
> dest="disable_p4sync", action="store_true",
> + help="Skip perforce sync of p4/master 
> after submit or shelve"),

s/perforce/Perforce/

Re: [PATCH] add -p: fix counting empty context lines in edited patches

[Eric Sunshine]

On Mon, Jun 4, 2018 at 6:08 AM, Phillip Wood  wrote:
> On 01/06/18 21:03, Eric Sunshine wrote:
>> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood  
>> wrote:
>>> +   } elsif ($mode eq ' ' or $_ eq "\n") {
>>
>> Based upon a very cursory read of parts of git-add-interactive.perl,
>> do I understand correctly that we don't have to worry about $_ ever
>> being "\r\n" on Windows?
>
> Good question, I think the short answer no. If my understanding of the
> newline section of perlport [1] is correct then on Windows "\n" eq
> "\012" and the io layer replaces "\015\012" with "\n" when reading in
> 'text' mode (which I think is the default if you don't specify one when
> opening the file/process or with binmode()).

That was my interpretation, as well (though I didn't audit the code closely).

> As "\n" is only one
> character it would perhaps be better to test '$mode' rather than '$_'
> above - what do you think.

That could be clearer. As a reviewer, I had to spend extra brain
cycles wondering why $mode was used everywhere else but $_ in just
this one place. (Not that it was difficult to figure out.)

>>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>>> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
>>> +test_expect_success 'setup file' '
>>> +   [...]
>>> +'
>>> +test_expect_success 'setup patch' '
>>> +   [...]
>>> +'
>>> +test_expect_success 'setup expected' '
>>> +   [...]
>>> +'
>>> +test_expect_success 'edit can strip spaces from empty context lines' '
>>> +   test_write_lines e n q | git add -p 2>error &&
>>> +   test_must_be_empty error &&
>>> +   git diff >output &&
>>> +   diff_cmp expected output
>>> +'
>>
>> I would have expected all the setup work to be contained directly in
>> the sole test which needs it rather than spread over three tests (two
>> of which are composed of a single command). Not a big deal, and not
>> worth a re-roll.
>
> Good point I was torn between that and matching the existing style in
> that file seems to be to create a million ancillary tests to do the set-up.

I see what you mean. Following existing practice in the file makes
sense, though breaking from that practice by bundling all the setup
into the single test which uses it wouldn't hurt either. It's a
judgment call (and not worrying about too much).

Re: [PATCH 19/22] sequencer.c: mark more strings for translation

[Eric Sunshine]

On Sun, Jun 3, 2018 at 11:14 AM, Duy Nguyen  wrote:
> On Sun, Jun 3, 2018 at 10:16 AM, Eric Sunshine  
> wrote:
>> On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  
>> wrote:
>>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>>> -   fprintf(stderr, "Could not apply %s... %.*s\n",
>>> +   fprintf_ln(stderr, _("Could not apply %s... %.*s"),
>>
>> Did you want to downcase "Could" for consistency with other error
>> messages, or was this left as-is intentionally?
>
> I'm not sure. Others start with a prefix (like "error:",
> "warning:"). This is a bit different and makes me hesitate.

Yep, I realized after hitting Send that this wasn't an error/warning
message so probably shouldn't be changed.

Re: [PATCH 03/22] builtin/config.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> There are also some minor adjustments in the strings.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -746,7 +746,7 @@ int cmd_config(int argc, const char **argv, const char 
> *prefix)
> if (ret == CONFIG_NOTHING_SET)
> error(_("cannot overwrite multiple values with a 
> single value\n"
> -   "   Use a regexp, --add or --replace-all to 
> change %s."), argv[0]);
> +   "   Use a regexp, --add or --replace-all to 
> change %s"), argv[0]);

Perhaps?

cannot overwrite multiple values with a single value;
use a regexp, --add or --replace-all to change %s

> @@ -819,7 +819,7 @@ int cmd_config(int argc, const char **argv, const char 
> *prefix)
> if (ret == 0)
> -   die("No such section!");
> +   die(_("no such section!"));
> @@ -830,7 +830,7 @@ int cmd_config(int argc, const char **argv, const char 
> *prefix)
> if (ret == 0)
> -   die("No such section!");
> +   die(_("no such section!"));

In other patches, you dropped the trailing "!"; perhaps do so for
these two also?

Maybe even:

die(_("no such section: %s", whatever);

Though, that may be out of scope of this patch series.

Re: [PATCH 09/22] connect.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> There are also some rephrasing and breaking sentences to help
> translators.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/connect.c b/connect.c
> @@ -921,7 +928,7 @@ static enum protocol parse_connect_url(const char 
> *url_orig, char **ret_host,
> if (!path || !*path)
> -   die("No path specified. See 'man git-pull' for valid url 
> syntax");
> +   die(_("no path specified. See 'man git-pull' for valid url 
> syntax"));

Perhaps:

no path specified; see 'man git-pull' for valid url syntax

?

Re: [PATCH 08/22] config.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/config.c b/config.c
> @@ -461,7 +461,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)
> envw = xstrdup(env);
>
> if (sq_dequote_to_argv(envw, , , ) < 0) {
> -   ret = error("bogus format in " CONFIG_DATA_ENVIRONMENT);
> +   ret = error(("bogus format in %s"), CONFIG_DATA_ENVIRONMENT);

s/((/(_(/

Re: [PATCH 11/22] dir.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/dir.c b/dir.c
> @@ -560,7 +560,7 @@ int report_path_error(const char *ps_matched,
> -   error("pathspec '%s' did not match any file(s) known to git.",
> +   error(_("pathspec '%s' did not match any file(s) known to 
> git."),

Drop the trailing period for consistency with other messages you've changed.

Re: [PATCH 10/22] convert.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/convert.c b/convert.c
> @@ -203,11 +203,11 @@ static void check_global_conv_flags_eol(const char 
> *path, enum crlf_action crlf_
> if (conv_flags & CONV_EOL_RNDTRP_DIE)
> -   die(_("CRLF would be replaced by LF in %s."), path);
> +   die(_("CRLF would be replaced by LF in %s"), path);
> else if (conv_flags & CONV_EOL_RNDTRP_WARN)
> warning(_("CRLF will be replaced by LF in %s.\n"
>   "The file will have its original line"
> - " endings in your working directory."), 
> path);
> + " endings in your working directory"), 
> path);
> @@ -217,7 +217,7 @@ static void check_global_conv_flags_eol(const char *path, 
> enum crlf_action crlf_
> else if (conv_flags & CONV_EOL_RNDTRP_WARN)
> warning(_("LF will be replaced by CRLF in %s.\n"
>   "The file will have its original line"
> - " endings in your working directory."), 
> path);
> + " endings in your working directory"), 
> path);
> }
>  }

For these two, perhaps:

 ... replace by  in %s;
the file will have its original line
endings in your working directory

> @@ -492,8 +492,8 @@ static int encode_to_worktree(const char *path, const 
> char *src, size_t src_len,
> dst = reencode_string_len(src, src_len, enc, default_encoding,
>   _len);
> if (!dst) {
> -   error("failed to encode '%s' from %s to %s",
> -   path, default_encoding, enc);
> +   error(_("failed to encode '%s' from %s to %s"),
> + path, default_encoding, enc);

The whitespace change on the second line fixes alignment with the
opening '('. Okay.

Re: [PATCH 06/22] builtin/replace.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/builtin/replace.c b/builtin/replace.c
> @@ -456,10 +456,10 @@ static int create_graft(int argc, const char **argv, 
> int force, int gentle)
> -   if (remove_signature()) {
> -   warning(_("the original commit '%s' has a gpg signature."), 
> old_ref);
> -   warning(_("the signature will be removed in the replacement 
> commit!"));
> -   }
> +   if (remove_signature())
> +   warning(_("the original commit '%s' has a gpg signature.\n"
> + "The signature will be removed in the replacement 
> commit!"),
> +   old_ref);

It's kind of weird to drop capitalization of the first sentence but
not the second. Also, you dropped trailing "!" in some other patches;
do you want to do so here? Perhaps, instead:

the original commit '%s' has a gpg signature;
the signature will be removed in the replacement commit

Re: [PATCH 22/22] transport-helper.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> @@ -196,13 +196,13 @@ static struct child_process *get_helper(struct 
> transport *transport)
> -   die("Unknown mandatory capability %s. This remote "
> -   "helper probably needs newer version of Git.",
> +   die(_("Unknown mandatory capability %s. This remote "
> + "helper probably needs newer version of Git."),
> capname);

Did you want to downcase these and drop period for consistency with
others? Perhaps:

unknown mandatory capability '%s'; this remote
helper probably needs newer version of Git

Re: [PATCH 21/22] transport.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/transport.c b/transport.c
> @@ -875,7 +875,7 @@ struct transport *transport_get(struct remote *remote, 
> const char *url)
> ret->progress = isatty(2);
>
> if (!remote)
> -   die("No remote provided to transport_get()");
> +   BUG("No remote provided to transport_get()");

Did you want to downcase "No" or just didn't bother since it's not
intended for end-user?

It looks like most callers of transport_get() won't pass NULL for
'remote' so this makes sense as a BUG(). A couple callers,
archive.c:run_remote_archiver() and fetch-object.c:fetch_refs(), get
the remote via remote_get() but don't check for NULL before
dereferencing it (before even calling this function), so will crash if
remote_get() ever returns NULL (assuming that it ever could in those
cases).

Re: [PATCH 19/22] sequencer.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2597,15 +2597,17 @@ static int error_with_patch(struct commit *commit,
> -   fprintf(stderr, "You can amend the commit now, with\n"
> -   "\n"
> -   "  git commit --amend %s\n"
> -   "\n"
> -   "Once you are satisfied with your changes, run\n"
> -   "\n"
> -   "  git rebase --continue\n", 
> gpg_sign_opt_quoted(opts));
> +   fprintf(stderr,
> +   _("You can amend the commit now, with\n"
> + "\n"
> + "  git commit --amend %s\n"
> + "\n"
> + "Once you are satisfied with your changes, run\n"
> + "\n"
> + "  git rebase --continue\n"),
> +   gpg_sign_opt_quoted(opts));
> } else if (exit_code)
> -   fprintf(stderr, "Could not apply %s... %.*s\n",
> +   fprintf_ln(stderr, _("Could not apply %s... %.*s"),

Did you want to downcase "Could" for consistency with other error
messages, or was this left as-is intentionally?

Re: [PATCH 20/22] sha1-file.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/sha1-file.c b/sha1-file.c
> @@ -71,17 +71,17 @@ static void git_hash_sha1_final(unsigned char *hash, 
> git_hash_ctx *ctx)
>  static void git_hash_unknown_init(git_hash_ctx *ctx)
>  {
> -   die("trying to init unknown hash");
> +   die(_("trying to init unknown hash"));
>  }
>
>  static void git_hash_unknown_update(git_hash_ctx *ctx, const void *data, 
> size_t len)
>  {
> -   die("trying to update unknown hash");
> +   die(_("trying to update unknown hash"));
>  }
>
>  static void git_hash_unknown_final(unsigned char *hash, git_hash_ctx *ctx)
>  {
> -   die("trying to finalize unknown hash");
> +   die(_("trying to finalize unknown hash"));
>  }

The above three are indicative of programmer error, aren't they? If
so, they ought not be translated (and perhaps changed to BUG at some
point).

>  const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
> @@ -378,8 +378,8 @@ static int alt_odb_usable(struct raw_object_store *o,
>
> /* Detect cases where alternate disappeared */
> if (!is_directory(path->buf)) {
> -   error("object directory %s does not exist; "
> - "check .git/objects/info/alternates.",
> +   error(_("object directory %s does not exist; "
> +   "check .git/objects/info/alternates."),

Perhaps drop the trailing period as you did in other cases.

>   path->buf);
> return 0;
> }

Re: [PATCH 16/22] refs.c: mark more strings for translation

[Eric Sunshine]

On Sat, Jun 2, 2018 at 12:32 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/refs.c b/refs.c
> @@ -1845,7 +1845,7 @@ int ref_update_reject_duplicates(struct string_list 
> *refnames,
> if (!cmp) {
> strbuf_addf(err,
> -   "multiple updates for ref '%s' not 
> allowed.",
> +   _("multiple updates for ref '%s' not 
> allowed."),

In other messages in this patch, you dropped the period at the end of
the message. Perhaps do so here too.

> refnames->items[i].string);
> return 1;
> } else if (cmp > 0) {
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> @@ -390,7 +390,7 @@ test_expect_success 'Query "master@{2005-05-26 23:33:01}" 
> (middle of history wit
> test_when_finished "rm -f o e" &&
> git rev-parse --verify "master@{2005-05-26 23:33:01}" >o 2>e &&
> test $B = $(cat o) &&
> -   test "warning: Log for ref $m has gap after $gd." = "$(cat e)"
> +   test_i18ngrep -F "warning: log for ref $m has gap after $gd" e
>  '

The change from '$(cat e)' to bare 'e' caught me off guard for a
moment, but use of test_i18ngrep explains it. Okay.

> diff --git a/t/t3310-notes-merge-manual-resolve.sh 
> b/t/t3310-notes-merge-manual-resolve.sh
> @@ -541,9 +541,9 @@ EOF
> -   grep -q "refs/notes/m" output &&
> -   grep -q "$(git rev-parse refs/notes/m)" output &&
> -   grep -q "$(git rev-parse NOTES_MERGE_PARTIAL^1)" output &&
> +   test_i18ngrep -q "refs/notes/m" output &&
> +   test_i18ngrep -q "$(git rev-parse refs/notes/m)" output &&
> +   test_i18ngrep -q "$(git rev-parse NOTES_MERGE_PARTIAL^1)" output &&

I hadn't seen test_i18ngrep take argument -q elsewhere, so I wondered
if it handled it correctly. Checking the implementation, I see that it
does pass it along to the underlying grep. Okay.

I also wondered briefly if it made sense to drop -q here since it
doesn't necessarily help debugging the test upon failure, but I
suppose such a change is outside the scope of this patch series, so
probably better not to.

Re: [PATCH v6 8/8] checkout & worktree: introduce checkout.defaultRemote

[Eric Sunshine]

On Sat, Jun 2, 2018 at 7:50 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Introduce a checkout.defaultRemote setting which can be used to
> designate a remote to prefer (via checkout.defaultRemote=origin) when
> running e.g. "git checkout master" to mean origin/master, even though
> there's other remotes that have the "master" branch.
> [...]
> Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
> mention this new config setting to the user, the full output on my
> git.git is now (the last paragraph is new):
>
> $ ./git --exec-path=$PWD checkout master
> error: pathspec 'master' did not match any file(s) known to git.
> hint: The argument 'master' matched more than one remote tracking branch.

In v6, the "The argument" prefix has been dropped from the hint, so
this commit message needs a tweak to match.

> hint: We found 26 remotes with a reference that matched. So we fell back
> hint: on trying to resolve the argument as a path, but failed there too!
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
> @@ -87,7 +87,23 @@ test_expect_success 'checkout of branch from multiple 
> remotes fails with advice'
> -   test_i18ngrep ! "^hint: " stderr
> +   test_i18ngrep ! "^hint: " stderr &&
> +   # Make sure the likes of checkout -p don not print this hint

s/don/do/

> +   git checkout -p foo 2>stderr &&
> +   test_i18ngrep ! "^hint: " stderr &&
> +   status_uno_is_clean
> +'

Re: [PATCH v5 7/8] checkout: add advice for ambiguous "checkout "

[Eric Sunshine]

On Fri, Jun 1, 2018 at 5:10 PM, Ævar Arnfjörð Bjarmason
 wrote:
> As the "checkout" documentation describes:
>
> If  is not found but there does exist a tracking branch in
> exactly one remote (call it ) with a matching name, treat
> as equivalent to [...] /
> This is a really useful feature. The problem is that when you and

s/and/add/

> another remote (e.g. a fork) git won't find a unique branch name

Missing comma: s/)/),/

> anymore, and will instead print this nondescript message:

Perhaps s/nondescript/unhelpful/ ?

> $ git checkout master
> error: pathspec 'master' did not match any file(s) known to git
>
> Now it will, on my git.git checkout, print:
>
> $ ./git --exec-path=$PWD checkout master
> error: pathspec 'master' did not match any file(s) known to git.
> hint: The argument 'master' matched more than one remote tracking branch.
> hint: We found 26 remotes with a reference that matched. So we fell back
> hint: on trying to resolve the argument as a path, but failed there too!
> hint:
> hint: If you meant to check out a remote tracking branch on e.g. 'origin'

Missing commas: s/on e.g. 'origin'/on, e.g. 'origin',/

> hint: you can do so by fully-qualifying the name with the --track option:

s/fully-qualifying/fully qualifying/

> hint:
> hint: git checkout --track origin/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Aside from the s/and/add/ botch, all of the above are tiny nits which
don't actually impact the meaning of the commit message, thus not
really important.

> ---
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> @@ -1267,6 +1268,18 @@ int cmd_checkout(int argc, const char **argv, const 
> char *prefix)
> +   if (ret && dwim_remotes_matched > 1 &&
> +   advice_checkout_ambiguous_remote_branch_name)
> +   advise(_("The argument '%s' matched more than one 
> remote tracking branch.\n"

You could drop "The argument" prefix, without hurting the meaning at
all, in order to gain a bit more horizontal space for the '%s'
interpolation. (Not worth a re-roll.)

> +"We found %d remotes with a reference that 
> matched. So we fell back\n"
> +"on trying to resolve the argument as a 
> path, but failed there too!\n"
> +"\n"
> +"If you meant to check out a remote tracking 
> branch on e.g. 'origin'\n"
> +"you can do so by fully-qualifying the name 
> with the --track option:\n"
> +"\n"
> +"git checkout --track origin/"),
> +  argv[0],
> +  dwim_remotes_matched);

Re: [PATCH v5 4/8] checkout.[ch]: change "unique" member to "num_matches"

[Eric Sunshine]

On Fri, Jun 1, 2018 at 5:10 PM, Ævar Arnfjörð Bjarmason
 wrote:
> checkout.[ch]: change "unique" member to "num_matches"

checkout.c: change...

> Internally track how many matches we find in the check_tracking_name()
> callback. Nothing uses this now, but it will be made use of in a later
> change.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Re: [PATCH v5 3/8] checkout.[ch]: introduce an *_INIT macro

[Eric Sunshine]

On Fri, Jun 1, 2018 at 5:10 PM, Ævar Arnfjörð Bjarmason
 wrote:
> checkout.[ch]: introduce an *_INIT macro

   checkout.c: introduce...

> Add an *_INIT macro for the tracking_name_data similar to what exists
> elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
> will make it more idiomatic in later changes to add more fields to the
> struct & its initialization macro.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Re: [PATCH] add -p: fix counting empty context lines in edited patches

[Eric Sunshine]

On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood  wrote:
> recount_edited_hunk() introduced in commit 2b8ea7f3c7 ("add -p:
> calculate offset delta for edited patches", 2018-03-05) required all
> context lines to start with a space, empty lines are not counted. This
> was intended to avoid any recounting problems if the user had
> introduced empty lines at the end when editing the patch. However this
> introduced a regression into 'git add -p' as it seems it is common for
> editors to strip the trailing whitespace from empty context lines when
> patches are edited thereby introducing empty lines that should be
> counted. 'git apply' knows how to deal with such empty lines and POSIX
> states that whether or not there is an space on an empty context line
> is implementation defined [1].
>
> Fix the regression by counting lines consist solely of a newline as

s/consist//
--or--
s/consist/that &/

> well as lines starting with a space as context lines and add a test to
> prevent future regressions.
>
> Signed-off-by: Phillip Wood 
> ---
>  git-add--interactive.perl  |  2 +-
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {
> -   } elsif ($mode eq ' ') {
> +   } elsif ($mode eq ' ' or $_ eq "\n") {

Based upon a very cursory read of parts of git-add-interactive.perl,
do I understand correctly that we don't have to worry about $_ ever
being "\r\n" on Windows?

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
> +test_expect_success 'setup file' '
> +   test_write_lines a "" b "" c >file &&
> +   git add file &&
> +   test_write_lines a "" d "" c >file
> +'
> +
> +test_expect_success 'setup patch' '
> +   SP=" " &&
> +   NULL="" &&
> +   cat >patch <<-EOF
> +   [...]
> +   EOF
> +'
> +
> +test_expect_success 'setup expected' '
> +   cat >expected <<-EOF
> +   [...]
> +   EOF
> +'
> +
> +test_expect_success 'edit can strip spaces from empty context lines' '
> +   test_write_lines e n q | git add -p 2>error &&
> +   test_must_be_empty error &&
> +   git diff >output &&
> +   diff_cmp expected output
> +'

I would have expected all the setup work to be contained directly in
the sole test which needs it rather than spread over three tests (two
of which are composed of a single command). Not a big deal, and not
worth a re-roll.

Re: [PATCH v4 9/9] checkout & worktree: introduce checkout.defaultRemote

[Eric Sunshine]

On Thu, May 31, 2018 at 5:49 PM, Stefan Beller  wrote:
>> I considered splitting this into checkout.defaultRemote and
>> worktree.defaultRemote, but it's probably less confusing to break our
>> own rules that anything shared between config should live in core.*
>> than have two config settings, and I couldn't come up with a short
>> name under core.* that made sense (core.defaultRemoteForCheckout?).
>
>   core.dwimRemote ? It's a bit cryptic, though.

This option started out as 'core.dwimRemote' in the very first version
of this series[1], but someone argued against it for several reasons
and suggested 'defaultRemote' instead[2].

[1]: https://public-inbox.org/git/20180502105452.17583-1-ava...@gmail.com/
[2]: 
https://public-inbox.org/git/capig+ctzyyc-1_txl2prfof6haktuxxm+g5excbys5fcdmd...@mail.gmail.com/

Re: [PATCH v4 8/9] checkout: add advice for ambiguous "checkout "

[Eric Sunshine]

On Thu, May 31, 2018 at 3:52 PM, Ævar Arnfjörð Bjarmason
 wrote:
> As the "checkout" documentation describes:
>
> If  is not found but there does exist a tracking branch in
> exactly one remote (call it ) with a matching name, treat
> as equivalent to [...] /
> This is a really useful feature, the problem is that when you another

s/, the/. The/
s/you/& add/

> remote (e.g. a fork) git won't find a unique branch name anymore, and
> will instead print this nondescript message:
>
> $ git checkout master
> error: pathspec 'master' did not match any file(s) known to git
>
> Now it will, on my git.git checkout, print:
>
> $ ./git --exec-path=$PWD checkout master
> error: pathspec 'master' did not match any file(s) known to git.
> hint: The argument 'master' matched more than one remote tracking branch.
> hint: We found 26 remotes with a reference that matched. So we fell back
> hint: on trying to resolve the argument as a path, but failed there too!
> hint:
> hint: Perhaps you meant fully qualify the branch name? E.g. origin/

s/meant/& to/

> hint: instead of ?
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> @@ -1269,6 +1270,16 @@ int cmd_checkout(int argc, const char **argv, const 
> char *prefix)
> +   if (ret && dwim_remotes_matched > 1 &&
> +   advice_checkout_ambiguous_remote_branch_name)
> +   advise(_("The argument '%s' matched more than one 
> remote tracking branch.\n"
> +"We found %d remotes with a reference that 
> matched. So we fell back\n"
> +"on trying to resolve the argument as a 
> path, but failed there too!\n"
> +"\n"
> +"Perhaps you meant fully qualify the branch 
> name? E.g. origin/\n"

s/meant/& to/

> +"instead of ?"),

Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count

[Eric Sunshine]

On Wed, May 30, 2018 at 5:03 PM, Stefan Beller  wrote:
> On Wed, May 30, 2018 at 1:44 PM, Eric Sunshine  
> wrote:
>>> Unrelated to this patch: how does this series cope with range diffs
>>> that are not in commit-ish but patches on the file system?
>>
>> I'm not following. Can you provide a concrete example to get me up to speed?
>
> I was just wondering if things like
>
> git format-patch --range-diff=v3-00*.patch --reroll-count=4 -3
>
> would be supported, but that doesn't seem to be the case, now that I read
> the whole series. I don't think it is crucial to support right now.

Okay, that's what I thought you meant (upon thinking harder about it
after hitting Send). git-range-diff doesn't support that mode of
operation (nor does 'tbdiff', as far as I can tell), so as a thin
wrapper around git-range-diff, "git format-patch --range-diff" doesn't
support it either.

Perhaps that sort of functionality could implement later by someone,
if desired. In the meantime, the following (manual procedure) would
approximate it:

git checkout --detach 
git am v3-00*.patch
git format-patch --range-diff=...

> This whole series is reviewed by me and I think it is good for inclusion
> once we have a reroll of the range-diff series and aligned the command
> name to call.

Thanks.

Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count

[Eric Sunshine]

On Wed, May 30, 2018 at 3:03 PM, Stefan Beller  wrote:
> On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine  
> wrote:
>> The --range-diff option introduces the embedded range-diff generically
>> as "Changes since previous version:", however, we can do better when
>> --reroll-count is specified by emitting "Changes since v{n}:" instead.
>
> A very similar option that I used before finding reroll count is
> --subject-prefix
> I still use that for RFC/WIP tags, but I sometimes used it with "PATCHv2"
> as an argument, too.
>
> Would we want to extend the niceties of this patch to that workflow?

I would not include such functionality directly in this patch, as the
two ideas are only superficially related ("computing the previous
version number by *some* mechanism") but not related in actual
implementation.

Computing the previous version number by consulting --reroll-count, as
done by this patch, is deterministic and was just low-hanging fruit.
What you suggest probably involves heuristics and parsing, thus ought
to be done in its own patch (or patches). It's also the sort of
incremental improvement that can be done later (rather than in this
initial implementation) if someone deems it desirable.

BTW: You can use "git format-patch --rfc" for RFC patches (in fact, I
did so for this series).

> Unrelated to this patch: how does this series cope with range diffs
> that are not in commit-ish but patches on the file system?

I'm not following. Can you provide a concrete example to get me up to speed?

Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range

[Eric Sunshine]

On Wed, May 30, 2018 at 2:58 PM, Stefan Beller  wrote:
> On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine  
> wrote:
>> When submitting a revised a patch series, the --range-diff option embeds
>> a range-diff in the cover letter showing changes since the previous
>> version of the patch series. The argument to --range-diff is a simple
>> revision naming the tip of the previous series, which works fine if the
>> previous and current versions of the patch series share a common base.
>>
>> However, it fails if the revision ranges of the old and new versions of
>> the series are disjoint. To address this shortcoming, extend
>> --range-diff to also accept an explicit revision range for the previous
>> series. For example:
>>
>> git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2
>>
>> Signed-off-by: Eric Sunshine 
>> ---
>>  static void infer_diff_ranges(struct argv_array *args,
>>   const char *prev,
>> + struct commit *origin,
>>   struct commit *head)
>>  {
>> -   argv_array_pushf(args, "%s...%s", prev,
>> -oid_to_hex(>object.oid));
>> +   if (strstr(prev, "..")) {
>> +   if (!origin)
>> +   die(_("failed to infer range-diff ranges"));
>
>>  static int get_range_diff(struct strbuf *sb,
>> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, 
>> int use_stdout,
>> /* might die from bad user input so try before creating cover letter 
>> */
>> if (range_diff) {
>> struct argv_array ranges = ARGV_ARRAY_INIT;
>> -   infer_diff_ranges(, range_diff, head);
>> +   infer_diff_ranges(, range_diff, origin, head);
>
> This is way before the check for 'if (origin) emit_diffstat' as done in
> patch 1, as we want to do this early. We need to check the non-NULLness
> in infer_diff_ranges [...]

Correct.

> Would it make sense to give a better error message in:
>
>> +   if (!origin)
>> +   die(_("failed to infer range-diff ranges"));
>
> above? (The failure sounds like it could be anything, but the
> reason for it is actually well understood: The origin was
> computed and as the boundary commit of the given range,
> or NULL if there is no boundary commit or multiple commits.

I'm not entirely happy with the generic error message either but
didn't come up with anything better which was both succinct and
actually helpful. I was hoping that a reviewer might suggest something
better.

> If we find not exactly one boundary, we cannot compute the
> range to give to the range-diff tool.

I considered allowing the argument to --range-diff to be much more
complex: to provide a way for the user to supply all information
needed for the invocation of git-range-diff (basically, to manually
supply arguments to git-range-diff) for cases when inference wasn't
possible. I even went so far as to consider allowing the
'creation-weight' to be specified as part of the --range-diff
argument. However, that sort of complexity is both difficult to
explain (document) and tends to be painful for users to specify (type
correctly).

Therefore, in the end, I opted for simplicity: namely, handle the
common cases with straight-forward minimal-learning-curve standard
options. Both --range-diff=v3 and --range-diff=v3~4..v3 are easy to
document and understand, as is the distinct --creation-weight= option.
This seems a good balance between extreme flexibility and relative
simplicity for handling common needs. (And, --range-diff is just a
convenience, after all; more complex scenarios can still be handled
manually by invoking git-range-diff followed by copy/paste.)

> Stepping back to the error message, I have no good
> suggestion on what to say there. Maybe we'd want to
> refactor the code in cmd_format_patch, that computes the
> origin:
>
> if (prepare_revision_walk())
> die(_("revision walk setup failed"));
> rev.boundary = 1;
> while ((commit = get_revision()) != NULL) {
> if (commit->object.flags & BOUNDARY) {
> boundary_count++;
> origin = (boundary_count == 1) ? commit : NULL;
> continue;
> }
>
> We could prefix that with
>
> need_origin = (range_diff && strstr(prev, "..")  || emit_diff_stat;
>
> and then if need_origin is about to be unset again we could issue
> a warning and die early after the loop warning about bad DAG form?
>
> I guess that can wait for a follow up patch.

I agree. The feature can be improved and made more fancy incrementally
as we gain better understanding of areas which need improvement. As a
first step, I wanted to keep it minimal but usable for the most common
cases.

Thanks for the review.

Re: [PATCH] log: prevent error if line range ends past end of file

[Eric Sunshine]

On Tue, May 29, 2018 at 1:30 AM,   wrote:
> If the -L option is used to specify a line range in git log, and the end
> of the range is past the end of the file, git will fail with a fatal
> error. This commit prevents such behaviour - instead we perform the log
> for existing lines within the specified range.
>
> This commit also fixes a corner case where -L ,-n:file would be treated
> as a log over the whole file. Now we treat this as -L 1,-n:file and
> blame the first line of the file instead.
>
> Signed-off-by: Isabella Stephens 
> ---
> diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
> @@ -86,12 +85,7 @@ test_expect_success '-L ,Y (Y == nlines)' '
>  test_expect_success '-L ,Y (Y == nlines + 1)' '
> n=$(expr $(wc -l  -   test_must_fail git log -L ,$n:b.c
> -'
> -
> -test_expect_success '-L ,Y (Y == nlines + 2)' '
> -   n=$(expr $(wc -l  -   test_must_fail git log -L ,$n:b.c
> +   git log -L ,$n:b.c
>  '

Not sure why you removed the 'n+2' test which was added intentionally,
along with the 'n' and 'n+1' tests, to probe the boundary handling for
correctness. By eliminating 'n+2', coverage is reduced and, even
though your change might be correct at this boundary, some future
breaking change might go undetected.

Re: [PATCH] blame: prevent error if range ends past end of file

[Eric Sunshine]

On Tue, May 29, 2018 at 1:30 AM,   wrote:
> If the -L option is used to specify a line range in git blame, and the
> end of the range is past the end of the file, git will fail with a fatal
> error. This commit prevents such behavior - instead we display the blame
> for existing lines within the specified range.

Makes sense; the new behavior is intuitive and more friendly.

> Tests and documentation are ammended accordingly.

s/ammended/amended/

> This commit also fixes two corner cases. Blaming -L n,-(n+1) now blames
> the first n lines of a file rather than from n to the end of the file.
> Blaming -L ,-n will be treated as -L 1,-n and blame the first line of
> the file, rather than blaming the whole file.
>
> Signed-off-by: Isabella Stephens 
> ---
> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
> @@ -152,6 +152,16 @@ Also you can use a regular expression to specify the 
> line range:
>  which limits the annotation to the body of the `hello` subroutine.
>
> +A range that begins or ends outside the bounds of the file will
> +blame the relevant lines. For example:
> +
> +   git blame -L 10,-20 foo
> +   git blame -L 10,+20 foo
> +
> +will respectively blame the first 10 and last 11 lines of a
> +20 line file. However, blaming a line range that is entirely
> +outside the bounds of the file will fail.

This documentation seems misplaced. Rather than inserting it after the
discussion of -L/regex/, a more natural place would be just above
-L/regex/ where -L, is discussed.

However, I am not at all convinced that this behavior should be
documented to this level of detail. Doing so assigns too much emphasis
to what should be intuitive, thus wastes readers' time wondering why
it is so heavily emphasized. At _most_, I would think you could say
merely:

A range that begins or ends outside the bounds of the file
will be clipped to the file's extent.

and drop the example and discussion of the example results altogether.

In fact, because this new behavior is what most users will intuitively
expect, it might be perfectly reasonable to not say anything about it
at all (that is, don't modify git-blame.txt).

Thanks.

[RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

[Eric Sunshine]

When submitting a revised version of a patch series, it can be helpful
(to reviewers) to include a summary of changes since the previous
attempt in the form of an interdiff or range-diff, however, doing so
involves manually copy/pasting the diff into the cover letter.

Add a --range-diff option to automate this process for range-diffs. The
argument to --range-diff specifies the tip of the previous attempt
against which to generate the range-diff. For example:

git format-patch --cover-letter --range-diff=v1 -3 v2

(At this early stage, the previous attempt and the patch series being
formatted must share a common base, however, a subsequent enhancement
will make it possible to specify an explicit revision range for the
previous attempt.)

Signed-off-by: Eric Sunshine 
---
 Documentation/git-format-patch.txt | 10 ++
 builtin/log.c  | 55 --
 t/t7910-branch-diff.sh | 15 
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 6cbe462a77..f4c70e6b64 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -23,6 +23,7 @@ SYNOPSIS
   [(--reroll-count|-v) ]
   [--to=] [--cc=]
   [--[no-]cover-letter] [--quiet] [--notes[=]]
+  [--range-diff=]
   [--progress]
   []
   [  |  ]
@@ -228,6 +229,15 @@ feeding the result to `git send-email`.
containing the branch description, shortlog and the overall diffstat.  
You can
fill in a description in the file before sending it out.
 
+--range-diff=::
+   As a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])
+   into the cover letter showing the differences between the previous
+   version of the patch series and the series currently being formatted.
+   `previous` is a single revision naming the tip of the previous
+   series which shares a common base with the series being formatted (for
+   example `git format-patch --cover-letter --range-diff=feature/v1 -3
+   feature/v2`).
+
 --notes[=]::
Append the notes (see linkgit:git-notes[1]) for the commit
after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index e01a256c11..460c31a293 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -28,6 +28,7 @@
 #include "mailmap.h"
 #include "gpg-interface.h"
 #include "progress.h"
+#include "run-command.h"
 
 #define MAIL_DEFAULT_WRAP 72
 
@@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev)
return branch;
 }
 
+static void infer_diff_ranges(struct argv_array *args,
+ const char *prev,
+ struct commit *head)
+{
+   argv_array_pushf(args, "%s...%s", prev,
+oid_to_hex(>object.oid));
+}
+
+static int get_range_diff(struct strbuf *sb,
+ const struct argv_array *ranges)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "branch-diff", "--no-color", NULL);
+   argv_array_pushv(, ranges->argv);
+   return capture_command(, sb, 0);
+}
+
 static void emit_diffstat(struct rev_info *rev,
  struct commit *origin, struct commit *head)
 {
@@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
  struct commit *origin,
  int nr, struct commit **list,
  const char *branch_name,
- int quiet)
+ int quiet,
+ const char *range_diff)
 {
const char *committer;
const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
@@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
int need_8bit_cte = 0;
struct pretty_print_context pp = {0};
struct commit *head = list[0];
+   struct strbuf diff = STRBUF_INIT;
 
if (!cmit_fmt_is_mail(rev->commit_format))
die(_("Cover letter needs email format"));
 
committer = git_committer_info(0);
 
+   /* might die from bad user input so try before creating cover letter */
+   if (range_diff) {
+   struct argv_array ranges = ARGV_ARRAY_INIT;
+   infer_diff_ranges(, range_diff, head);
+   if (get_range_diff(, ))
+   die(_("failed to generate range-diff"));
+   argv_array_clear();
+   }
+
if (!use_stdout &&
open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", 
rev, quiet))
-   return;

[RFC PATCH 5/5] format-patch: add --creation-weight tweak for --range-diff

[Eric Sunshine]

When generating a range-diff, matching up commits between two version of
a patch series involves heuristics, thus may give unexpected results.
git-branch-diff allows tweaking the heuristic via --creation-weight.
Follow suit by accepting --creation-weight in combination with
--range-diff when generating a range-diff for a cover-letter.

Signed-off-by: Eric Sunshine 
---
 Documentation/git-format-patch.txt |  8 +++-
 builtin/log.c  | 19 +++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 25026ae26e..7ed9ec9dae 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -23,7 +23,7 @@ SYNOPSIS
   [(--reroll-count|-v) ]
   [--to=] [--cc=]
   [--[no-]cover-letter] [--quiet] [--notes[=]]
-  [--range-diff=]
+  [--range-diff= [--creation-weight=]]
   [--progress]
   []
   [  |  ]
@@ -240,6 +240,12 @@ feeding the result to `git send-email`.
disjoint (for example `git format-patch --cover-letter
--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
 
+--creation-weight=::
+   Used with `--range-diff`, tweak the heuristic which matches up commits
+   between the previous and current series of patches by adjusting the
+   creation/deletion cost fudge factor. See linkgit:git-branch-diff[1])
+   for details.
+
 --notes[=]::
Append the notes (see linkgit:git-notes[1]) for the commit
after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index 3089d3a50a..2c49011b51 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1012,12 +1012,16 @@ static void infer_diff_ranges(struct argv_array *args,
 }
 
 static int get_range_diff(struct strbuf *sb,
- const struct argv_array *ranges)
+ const struct argv_array *ranges,
+ const char *creation_weight)
 {
struct child_process cp = CHILD_PROCESS_INIT;
 
cp.git_cmd = 1;
argv_array_pushl(, "branch-diff", "--no-color", NULL);
+   if (creation_weight)
+   argv_array_pushf(,
+"--creation-weight=%s", creation_weight);
argv_array_pushv(, ranges->argv);
return capture_command(, sb, 0);
 }
@@ -1047,7 +1051,8 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
  int nr, struct commit **list,
  const char *branch_name,
  int quiet,
- const char *range_diff)
+ const char *range_diff,
+ const char *creation_weight)
 {
const char *committer;
const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
@@ -1070,7 +1075,7 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
if (range_diff) {
struct argv_array ranges = ARGV_ARRAY_INIT;
infer_diff_ranges(, range_diff, origin, head);
-   if (get_range_diff(, ))
+   if (get_range_diff(, , creation_weight))
die(_("failed to generate range-diff"));
argv_array_clear();
}
@@ -1495,6 +1500,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
int show_progress = 0;
struct progress *progress = NULL;
const char *range_diff = NULL;
+   const char *creation_weight = NULL;
 
const struct option builtin_format_patch_options[] = {
{ OPTION_CALLBACK, 'n', "numbered", , NULL,
@@ -1570,6 +1576,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
 N_("show progress while generating patches")),
OPT_STRING(0, "range-diff", _diff, N_("rev-range"),
   N_("show changes against  in cover 
letter")),
+   OPT_STRING(0, "creation-weight", _weight, N_("factor"),
+  N_("fudge factor by which creation is weighted")),
OPT_END()
};
 
@@ -1664,6 +1672,9 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
die (_("--subject-prefix/--rfc and -k are mutually 
exclusive."));
rev.preserve_subject = keep_subject;
 
+   if (creation_weight && !range_diff)
+   die(_("--creation-weight requires --range-diff"));
+
argc = setup_revisions(argc, argv, , _r_opt);
if (argc > 1)
die (_("unrecognized argument: %s"), argv[1]);
@@ -1827,7 +1838

[RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count

[Eric Sunshine]

The --range-diff option introduces the embedded range-diff generically
as "Changes since previous version:", however, we can do better when
--reroll-count is specified by emitting "Changes since v{n}:" instead.

Signed-off-by: Eric Sunshine 
---
 builtin/log.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index e38cf06050..3089d3a50a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1121,7 +1121,11 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
 
if (diff.len) {
FILE *fp = rev->diffopt.file;
-   fputs(_("Changes since previous version:"), fp);
+   if (rev->reroll_count <= 0)
+   fputs(_("Changes since previous version:"), fp);
+   else /* RFC may be v0, so allow -v1 to diff against v0 */
+   fprintf(fp, _("Changes since v%d:"),
+   rev->reroll_count - 1);
fputs("\n\n", fp);
fputs(diff.buf, fp);
fputc('\n', fp);
-- 
2.17.1.1235.ge295dfb56e

[RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range

[Eric Sunshine]

When submitting a revised a patch series, the --range-diff option embeds
a range-diff in the cover letter showing changes since the previous
version of the patch series. The argument to --range-diff is a simple
revision naming the tip of the previous series, which works fine if the
previous and current versions of the patch series share a common base.

However, it fails if the revision ranges of the old and new versions of
the series are disjoint. To address this shortcoming, extend
--range-diff to also accept an explicit revision range for the previous
series. For example:

git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2

Signed-off-by: Eric Sunshine 
---
 Documentation/git-format-patch.txt |  8 +---
 builtin/log.c  | 16 +---
 t/t7910-branch-diff.sh |  1 +
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index f4c70e6b64..25026ae26e 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -233,10 +233,12 @@ feeding the result to `git send-email`.
As a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])
into the cover letter showing the differences between the previous
version of the patch series and the series currently being formatted.
-   `previous` is a single revision naming the tip of the previous
-   series which shares a common base with the series being formatted (for
+   `previous` can be a single revision naming the tip of the previous
+   series if it shares a common base with the series being formatted (for
example `git format-patch --cover-letter --range-diff=feature/v1 -3
-   feature/v2`).
+   feature/v2`), or a revision range if the two versions of the series are
+   disjoint (for example `git format-patch --cover-letter
+   --range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
 
 --notes[=]::
Append the notes (see linkgit:git-notes[1]) for the commit
diff --git a/builtin/log.c b/builtin/log.c
index 460c31a293..e38cf06050 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -995,10 +995,20 @@ static char *find_branch_name(struct rev_info *rev)
 
 static void infer_diff_ranges(struct argv_array *args,
  const char *prev,
+ struct commit *origin,
  struct commit *head)
 {
-   argv_array_pushf(args, "%s...%s", prev,
-oid_to_hex(>object.oid));
+   if (strstr(prev, "..")) {
+   if (!origin)
+   die(_("failed to infer range-diff ranges"));
+   argv_array_push(args, prev);
+   argv_array_pushf(args, "%s..%s",
+oid_to_hex(>object.oid),
+oid_to_hex(>object.oid));
+   } else {
+   argv_array_pushf(args, "%s...%s", prev,
+oid_to_hex(>object.oid));
+   }
 }
 
 static int get_range_diff(struct strbuf *sb,
@@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
/* might die from bad user input so try before creating cover letter */
if (range_diff) {
struct argv_array ranges = ARGV_ARRAY_INIT;
-   infer_diff_ranges(, range_diff, head);
+   infer_diff_ranges(, range_diff, origin, head);
if (get_range_diff(, ))
die(_("failed to generate range-diff"));
argv_array_clear();
diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh
index edbd69b6f8..c0e8668ed9 100755
--- a/t/t7910-branch-diff.sh
+++ b/t/t7910-branch-diff.sh
@@ -155,5 +155,6 @@ format_patch () {
 }
 
 format_patch 'B...C' topic
+format_patch 'A..B A..C' master..topic
 
 test_done
-- 
2.17.1.1235.ge295dfb56e

[RFC PATCH 1/5] format-patch: allow additional generated content in make_cover_letter()

[Eric Sunshine]

make_cover_letter() returns early when it lacks sufficient state to emit
a diffstat, which makes it difficult to extend the function to reliably
emit additional generated content. Work around this shortcoming by
factoring diffstat-printing logic out to its own function and calling it
as needed without otherwise inhibiting normal control flow.

Signed-off-by: Eric Sunshine 
---
 builtin/log.c | 43 +++
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 71f68a3e4f..e01a256c11 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -992,6 +992,26 @@ static char *find_branch_name(struct rev_info *rev)
return branch;
 }
 
+static void emit_diffstat(struct rev_info *rev,
+ struct commit *origin, struct commit *head)
+{
+   struct diff_options opts;
+
+   memcpy(, >diffopt, sizeof(opts));
+   opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
+   opts.stat_width = MAIL_DEFAULT_WRAP;
+
+   diff_setup_done();
+
+   diff_tree_oid(>tree->object.oid,
+ >tree->object.oid,
+ "", );
+   diffcore_std();
+   diff_flush();
+
+   fprintf(rev->diffopt.file, "\n");
+}
+
 static void make_cover_letter(struct rev_info *rev, int use_stdout,
  struct commit *origin,
  int nr, struct commit **list,
@@ -1005,7 +1025,6 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
struct strbuf sb = STRBUF_INIT;
int i;
const char *encoding = "UTF-8";
-   struct diff_options opts;
int need_8bit_cte = 0;
struct pretty_print_context pp = {0};
struct commit *head = list[0];
@@ -1055,25 +1074,9 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
 
shortlog_output();
 
-   /*
-* We can only do diffstat with a unique reference point
-*/
-   if (!origin)
-   return;
-
-   memcpy(, >diffopt, sizeof(opts));
-   opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
-   opts.stat_width = MAIL_DEFAULT_WRAP;
-
-   diff_setup_done();
-
-   diff_tree_oid(>tree->object.oid,
- >tree->object.oid,
- "", );
-   diffcore_std();
-   diff_flush();
-
-   fprintf(rev->diffopt.file, "\n");
+   /* We can only do diffstat with a unique reference point */
+   if (origin)
+   emit_diffstat(rev, origin, head);
 }
 
 static const char *clean_message_id(const char *msg_id)
-- 
2.17.1.1235.ge295dfb56e

[RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Eric Sunshine]

Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
git-branch-diff[1] which computes differences between two versions of a
patch series. Such a diff can be a useful aid for reviewers when
inserted into a cover letter. However, doing so requires manual
generation (invoking git-branch-diff) and copy/pasting the result into
the cover letter.

This series fully automates the process by adding a --range-diff option
to git-format-patch. It is RFC for a couple reasons:

* The final name for the 'tbdiff' replacement has not yet been nailed
  down. The name git-branch-diff is moribund[2]; Dscho favors merging
  the functionality into git-branch as a new --diff option[3]; others
  prefer a standalone command named git-range-diff or
  git-series-diff[4] or similar.

* I have some ideas for future enhancements and want to be careful not
  to lock in a UI which doesn't mesh well with them (though I think the
  current UI turned out reasonably well). First, I foresee a
  complementary --interdiff option for inserting an interdiff-style diff
  for cases when that style is easier to read or simply more
  appropriate. Second, although the current patch series only supports
  --range-diff for the cover letter, some people insert interdiff- or
  tbdiff-style diffs (indented) into the commentary section of
  standalone patches. Although this often makes for a noisy mess, it is
  periodically useful.

This series is built atop js/branch-diff in 'pu'.

[1]: 
https://public-inbox.org/git/cover.1525448066.git.johannes.schinde...@gmx.de/
[2]: 
https://public-inbox.org/git/nycvar.qro.7.76.6.1805061401260...@tvgsbejvaqbjf.bet/
[3]: 
https://public-inbox.org/git/nycvar.qro.7.76.6.1805062155120...@tvgsbejvaqbjf.bet/
[4]: https://public-inbox.org/git/xmqqin7gzbkb@gitster-ct.c.googlers.com/

Eric Sunshine (5):
  format-patch: allow additional generated content in
make_cover_letter()
  format-patch: add --range-diff option to embed diff in cover letter
  format-patch: extend --range-diff to accept revision range
  format-patch: teach --range-diff to respect -v/--reroll-count
  format-patch: add --creation-weight tweak for --range-diff

 Documentation/git-format-patch.txt |  18 +
 builtin/log.c  | 119 -
 t/t7910-branch-diff.sh |  16 
 3 files changed, 132 insertions(+), 21 deletions(-)

-- 
2.17.1.1235.ge295dfb56e

Re: [PATCH v2 04/11] help: add --config to list all available config

[Eric Sunshine]

On Sat, May 26, 2018 at 9:55 AM, Nguyễn Thái Ngọc Duy  wrote:
> Sometimes it helps to list all available config vars so the user can
> search for something they want. The config man page can also be used
> but it's harder to search if you want to focus on the variable name,
> for example.
>
> This is not the best way to collect the available config since it's
> not precise. Ideally we should have a centralized list of config in C
> code (pretty much like 'struct option'), but that's a lot more work.
> This will do for now.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/help.c b/help.c
> @@ -409,6 +409,62 @@ void list_common_guides_help(void)
> +void list_config_help(void)
> +{
> +   for (p = config_name_list; *p; p++) {
> +   const char *var = *p;
> +   struct strbuf sb = STRBUF_INIT;
> +
> +   for (e = slot_expansions; e->prefix; e++) {
> +
> +   strbuf_reset();

Style nit: unwanted blank line

> +   strbuf_addf(, "%s.%s", e->prefix, e->placeholder);
> +   if (!strcasecmp(var, sb.buf)) {
> +   e->fn(, e->prefix);
> +   e->found++;
> +   break;
> +   }
> +   }
> +   strbuf_release();
> +   if (!e->prefix)
> +   string_list_append(, var);
> +   }

Re: [PATCH v2 1/4] diff: ignore --ita-[in]visible-in-index when diffing worktree-to-tree

[Eric Sunshine]

On Sat, May 26, 2018 at 8:08 AM, Nguyễn Thái Ngọc Duy  wrote:
> This option is supposed to fix the diff of "diff-files" (not reporting
> ita entries as new files) and "diff-index --cached " ( showing

s/(\s/(/

> ita entries as present in the index with empty content) but not
> "diff-index ".
>
> When --ita-invisible-in-index is set on "git diff-index ",
> unpack_trees() will eventually call oneway_diff() on the ita entry
> with the same code flow as "diff-index --cached ". We want to
> ignore the ita entry for "diff-index --cached " but not
> "diff-index " since the latter will examine and produce a diff
> based on worktree entry's (real) content, not ita index entry's
> (empty) content.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

Re: [PATCH] packfile: Correct zlib buffer handling

[Eric Sunshine]

On Fri, May 25, 2018 at 7:17 PM, Jeremy Linton  wrote:
> The buffer being passed to zlib includes a null terminator that
> git needs to keep in place. unpack_compressed_entry() attempts to
> detect the case that the source buffer hasn't been fully consumed
> by checking to see if the destination buffer has been over consumed.
>
> This yields two problems, first a single byte overrun won't be detected
> properly because the Z_STREAM_END will then be set, but the null
> terminator will have been overwritten. The other problem is that
> more recent zlib patches have been poisoning the unconsumed portions
> of the buffers which also overwrites the null, while correctly
> returning length and status.
>
> Lets rely on the fact that the source buffer will only be fully

s/Lets/Let's/

> consumed when the when the destination buffer is inflated to the

s/when the when the/when the/

> correct size. We can do this by passing zlib the correct buffer size
> and properly checking the return status. The latter check actually
> already exists if the buffer size is correct.
>
> Signed-off-by: Jeremy Linton 

Re: [PATCH v2 3/5] config doc: elaborate on what transfer.fsckObjects does

[Eric Sunshine]

On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason
 wrote:
> The existing documentation led the user to believe that all we were
> doing were basic reachability sanity checks, but that hasn't been true
> for a very long time. Update the description to match reality, and
> note the caveat that there's a quarantine for accepting pushes, but
> not for fetching.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -3341,8 +3341,16 @@ transfer.fsckObjects::
>  When set, the fetch or receive will abort in the case of a malformed
> +object or a link to a nonexistent object. In addition, various other
> +issues are checked for, including legacy issues (see `fsck.`),
> +and potential security issues like the existence of a `.GIT` directory
> +(see the release notes for v2.2.1 for details). Other sanity and
> +security checks may be added in future releases.
> ++
> +On the receiving side, failing fsckObjects will make those objects
> +unreachable, see "QUARANTINE ENVIRONMENT" in
> +linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
> +instead be left unreferenced in the repository.

This version looks better. Thanks.