[RFC/PATCH 0/4] working tree operations: support superprefix

[RFC/PATCH 0/4] working tree operations: support superprefix

[Stefan Beller]

As you may know, I am trying to implement "git checkout --recurse-submodules"
which recurses into submodules and update the submodules to the recorded
state of the superproject. I realized that such a huge change is a big endeavor,
and needs to be broken up into many series. 

Currently I plan to send about 4 series:

* This one; teaching the superprefix to working tree operations.

* structured tests, i.e. enhancing lib-submodule-update.sh to have a function
  to test for any corner case (File/Submodule conflict,
  Directory/Submodule conflict; non-existence of submodule commit; and such)

* the actual internal implementation, mostly touching unpack-trees.c,
  entry.c and helper functions in submodule.c
  
* Enabling commands to take advantage of the infrastructure provided.
  At that point we only need to add a --recurse-submodule flag in the
  respective command and add a line to its respective test
  
  "test_switch_submodule_recursing $cmd --recurse-submodule"

--
This patch series is the first of the four series.

It consists of 4 patches, the first 3 are refactoring to a modern style of Git,
the last patch is the actual patch that allows read-tree to be used with
the superprefix option. read-tree is already exposed there to make the
super-prefix code tested.

The series is based on master.

Thanks,
Stefan

Stefan Beller (4):
  read-tree: use OPT_BOOL instead of OPT_SET_INT
  t1000: modernize style
  t1001: modernize style
  unpack-trees: support super-prefix option

 builtin/read-tree.c         |  36 +--
 git.c                       |   2 +-
 t/t1000-read-tree-m-3way.sh | 648 +++++++++++++++++++++----------------------
 t/t1001-read-tree-m-2way.sh | 650 ++++++++++++++++++++++----------------------
 unpack-trees.c              |  39 ++-
 5 files changed, 699 insertions(+), 676 deletions(-)

-- 
2.11.0.rc2.30.g7c4be45.dirty

[PATCH 1/4] read-tree: use OPT_BOOL instead of OPT_SET_INT

[Stefan Beller]

All occurrences of OPT_SET_INT were setting the value to 1;
internally OPT_BOOL is just that.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/read-tree.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index fa6edb35b2..8ba64bc785 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -109,34 +109,34 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 		{ OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
 		  N_("write resulting index to <file>"),
 		  PARSE_OPT_NONEG, index_output_cb },
-		OPT_SET_INT(0, "empty", &read_empty,
-			    N_("only empty the index"), 1),
+		OPT_BOOL(0, "empty", &read_empty,
+			    N_("only empty the index")),
 		OPT__VERBOSE(&opts.verbose_update, N_("be verbose")),
 		OPT_GROUP(N_("Merging")),
-		OPT_SET_INT('m', NULL, &opts.merge,
-			    N_("perform a merge in addition to a read"), 1),
-		OPT_SET_INT(0, "trivial", &opts.trivial_merges_only,
-			    N_("3-way merge if no file level merging required"), 1),
-		OPT_SET_INT(0, "aggressive", &opts.aggressive,
-			    N_("3-way merge in presence of adds and removes"), 1),
-		OPT_SET_INT(0, "reset", &opts.reset,
-			    N_("same as -m, but discard unmerged entries"), 1),
+		OPT_BOOL('m', NULL, &opts.merge,
+			 N_("perform a merge in addition to a read")),
+		OPT_BOOL(0, "trivial", &opts.trivial_merges_only,
+			 N_("3-way merge if no file level merging required")),
+		OPT_BOOL(0, "aggressive", &opts.aggressive,
+			 N_("3-way merge in presence of adds and removes")),
+		OPT_BOOL(0, "reset", &opts.reset,
+			 N_("same as -m, but discard unmerged entries")),
 		{ OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
 		  N_("read the tree into the index under <subdirectory>/"),
 		  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
-		OPT_SET_INT('u', NULL, &opts.update,
-			    N_("update working tree with merge result"), 1),
+		OPT_BOOL('u', NULL, &opts.update,
+			 N_("update working tree with merge result")),
 		{ OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
 		  N_("gitignore"),
 		  N_("allow explicitly ignored files to be overwritten"),
 		  PARSE_OPT_NONEG, exclude_per_directory_cb },
-		OPT_SET_INT('i', NULL, &opts.index_only,
-			    N_("don't check the working tree after merging"), 1),
+		OPT_BOOL('i', NULL, &opts.index_only,
+			 N_("don't check the working tree after merging")),
 		OPT__DRY_RUN(&opts.dry_run, N_("don't update the index or the work tree")),
-		OPT_SET_INT(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
-			    N_("skip applying sparse checkout filter"), 1),
-		OPT_SET_INT(0, "debug-unpack", &opts.debug_unpack,
-			    N_("debug unpack-trees"), 1),
+		OPT_BOOL(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
+			 N_("skip applying sparse checkout filter")),
+		OPT_BOOL(0, "debug-unpack", &opts.debug_unpack,
+			 N_("debug unpack-trees")),
 		OPT_END()
 	};
 
-- 
2.11.0.rc2.30.g7c4be45.dirty

Re: [PATCH 1/4] read-tree: use OPT_BOOL instead of OPT_SET_INT

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> All occurrences of OPT_SET_INT were setting the value to 1;
> internally OPT_BOOL is just that.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/read-tree.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)

The result is much easier to read, partly because the "1" (true) is
at the very end of the line when OPT_SET_INT() is used for the
reader to notice that this is merely a boolean.

More importantly, as OPT_SET_INT() can be used multiple times on the
same variable (or field) to set it to different values, it is easier
to read if OPT_BOOL() is used when OPT_SET_INT() to 1 is not used
for that purpose.

Thanks.

>> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index fa6edb35b2..8ba64bc785 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -109,34 +109,34 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
>  		{ OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
>  		  N_("write resulting index to <file>"),
>  		  PARSE_OPT_NONEG, index_output_cb },
> -		OPT_SET_INT(0, "empty", &read_empty,
> -			    N_("only empty the index"), 1),
> +		OPT_BOOL(0, "empty", &read_empty,
> +			    N_("only empty the index")),
>  		OPT__VERBOSE(&opts.verbose_update, N_("be verbose")),
>  		OPT_GROUP(N_("Merging")),
> -		OPT_SET_INT('m', NULL, &opts.merge,
> -			    N_("perform a merge in addition to a read"), 1),
> -		OPT_SET_INT(0, "trivial", &opts.trivial_merges_only,
> -			    N_("3-way merge if no file level merging required"), 1),
> -		OPT_SET_INT(0, "aggressive", &opts.aggressive,
> -			    N_("3-way merge in presence of adds and removes"), 1),
> -		OPT_SET_INT(0, "reset", &opts.reset,
> -			    N_("same as -m, but discard unmerged entries"), 1),
> +		OPT_BOOL('m', NULL, &opts.merge,
> +			 N_("perform a merge in addition to a read")),
> +		OPT_BOOL(0, "trivial", &opts.trivial_merges_only,
> +			 N_("3-way merge if no file level merging required")),
> +		OPT_BOOL(0, "aggressive", &opts.aggressive,
> +			 N_("3-way merge in presence of adds and removes")),
> +		OPT_BOOL(0, "reset", &opts.reset,
> +			 N_("same as -m, but discard unmerged entries")),
>  		{ OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
>  		  N_("read the tree into the index under <subdirectory>/"),
>  		  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
> -		OPT_SET_INT('u', NULL, &opts.update,
> -			    N_("update working tree with merge result"), 1),
> +		OPT_BOOL('u', NULL, &opts.update,
> +			 N_("update working tree with merge result")),
>  		{ OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
>  		  N_("gitignore"),
>  		  N_("allow explicitly ignored files to be overwritten"),
>  		  PARSE_OPT_NONEG, exclude_per_directory_cb },
> -		OPT_SET_INT('i', NULL, &opts.index_only,
> -			    N_("don't check the working tree after merging"), 1),
> +		OPT_BOOL('i', NULL, &opts.index_only,
> +			 N_("don't check the working tree after merging")),
>  		OPT__DRY_RUN(&opts.dry_run, N_("don't update the index or the work tree")),
> -		OPT_SET_INT(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
> -			    N_("skip applying sparse checkout filter"), 1),
> -		OPT_SET_INT(0, "debug-unpack", &opts.debug_unpack,
> -			    N_("debug unpack-trees"), 1),
> +		OPT_BOOL(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
> +			 N_("skip applying sparse checkout filter")),
> +		OPT_BOOL(0, "debug-unpack", &opts.debug_unpack,
> +			 N_("debug unpack-trees")),
>  		OPT_END()
>  	};

[PATCH 2/4] t1000: modernize style

[Stefan Beller]

The preferred style in tests seems to be

test_expect_success 'short description, ended by 2 single quotes' '
	here comes the test &&
	and chains over many lines &&
	ended by a quote
'

Or going by the numbers:
    $ git -C t/ grep "' '$" | wc -l
    9796
    $ git -C t/ grep test_expect_ |wc -l
    11861

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t1000-read-tree-m-3way.sh | 648 +++++++++++++++++++++-----------------------
 1 file changed, 315 insertions(+), 333 deletions(-)

diff --git a/t/t1000-read-tree-m-3way.sh b/t/t1000-read-tree-m-3way.sh
index a0b79b4839..3c4d2d6045 100755
--- a/t/t1000-read-tree-m-3way.sh
+++ b/t/t1000-read-tree-m-3way.sh
@@ -128,29 +128,29 @@ cat >expected <<\EOF
 EOF
 
 check_result () {
-    git ls-files --stage | sed -e 's/ '"$_x40"' / X /' >current &&
-    test_cmp expected current
+	git ls-files --stage | sed -e 's/ '"$_x40"' / X /' >current &&
+	test_cmp expected current
 }
 
 # This is done on an empty work directory, which is the normal
 # merge person behaviour.
-test_expect_success \
-    '3-way merge with git read-tree -m, empty cache' \
-    "rm -fr [NDMALTS][NDMALTSF] Z &&
-     rm .git/index &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
+test_expect_success '3-way merge with git read-tree -m, empty cache' '
+	rm -fr [NDMALTS][NDMALTSF] Z &&
+	rm .git/index &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
 
 # This starts out with the first head, which is the normal
 # patch submitter behaviour.
-test_expect_success \
-    '3-way merge with git read-tree -m, match H' \
-    "rm -fr [NDMALTS][NDMALTSF] Z &&
-     rm .git/index &&
-     read_tree_must_succeed $tree_A &&
-     git checkout-index -f -u -a &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
+test_expect_success '3-way merge with git read-tree -m, match H' '
+	rm -fr [NDMALTS][NDMALTSF] Z &&
+	rm .git/index &&
+	read_tree_must_succeed $tree_A &&
+	git checkout-index -f -u -a &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
 
 : <<\END_OF_CASE_TABLE
 
@@ -208,322 +208,304 @@ DF (file) when tree B require DF to be a directory by having DF/DF
 
 END_OF_CASE_TABLE
 
-test_expect_success '1 - must not have an entry not in A.' "
-     rm -f .git/index XX &&
-     echo XX >XX &&
-     git update-index --add XX &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '2 - must match B in !O && !A && B case.' \
-    "rm -f .git/index NA &&
-     cp .orig-B/NA NA &&
-     git update-index --add NA &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B"
-
-test_expect_success \
-    '2 - matching B alone is OK in !O && !A && B case.' \
-    "rm -f .git/index NA &&
-     cp .orig-B/NA NA &&
-     git update-index --add NA &&
-     echo extra >>NA &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B"
-
-test_expect_success \
-    '3 - must match A in !O && A && !B case.' \
-    "rm -f .git/index AN &&
-     cp .orig-A/AN AN &&
-     git update-index --add AN &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '3 - matching A alone is OK in !O && A && !B case.' \
-    "rm -f .git/index AN &&
-     cp .orig-A/AN AN &&
-     git update-index --add AN &&
-     echo extra >>AN &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B"
-
-test_expect_success \
-    '3 (fail) - must match A in !O && A && !B case.' "
-     rm -f .git/index AN &&
-     cp .orig-A/AN AN &&
-     echo extra >>AN &&
-     git update-index --add AN &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '4 - must match and be up-to-date in !O && A && B && A!=B case.' \
-    "rm -f .git/index AA &&
-     cp .orig-A/AA AA &&
-     git update-index --add AA &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '4 (fail) - must match and be up-to-date in !O && A && B && A!=B case.' "
-     rm -f .git/index AA &&
-     cp .orig-A/AA AA &&
-     git update-index --add AA &&
-     echo extra >>AA &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '4 (fail) - must match and be up-to-date in !O && A && B && A!=B case.' "
-     rm -f .git/index AA &&
-     cp .orig-A/AA AA &&
-     echo extra >>AA &&
-     git update-index --add AA &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '5 - must match in !O && A && B && A==B case.' \
-    "rm -f .git/index LL &&
-     cp .orig-A/LL LL &&
-     git update-index --add LL &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '5 - must match in !O && A && B && A==B case.' \
-    "rm -f .git/index LL &&
-     cp .orig-A/LL LL &&
-     git update-index --add LL &&
-     echo extra >>LL &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '5 (fail) - must match A in !O && A && B && A==B case.' "
-     rm -f .git/index LL &&
-     cp .orig-A/LL LL &&
-     echo extra >>LL &&
-     git update-index --add LL &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '6 - must not exist in O && !A && !B case' "
-     rm -f .git/index DD &&
-     echo DD >DD &&
-     git update-index --add DD &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '7 - must not exist in O && !A && B && O!=B case' "
-     rm -f .git/index DM &&
-     cp .orig-B/DM DM &&
-     git update-index --add DM &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '8 - must not exist in O && !A && B && O==B case' "
-     rm -f .git/index DN &&
-     cp .orig-B/DN DN &&
-     git update-index --add DN &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '9 - must match and be up-to-date in O && A && !B && O!=A case' \
-    "rm -f .git/index MD &&
-     cp .orig-A/MD MD &&
-     git update-index --add MD &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '9 (fail) - must match and be up-to-date in O && A && !B && O!=A case' "
-     rm -f .git/index MD &&
-     cp .orig-A/MD MD &&
-     git update-index --add MD &&
-     echo extra >>MD &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '9 (fail) - must match and be up-to-date in O && A && !B && O!=A case' "
-     rm -f .git/index MD &&
-     cp .orig-A/MD MD &&
-     echo extra >>MD &&
-     git update-index --add MD &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '10 - must match and be up-to-date in O && A && !B && O==A case' \
-    "rm -f .git/index ND &&
-     cp .orig-A/ND ND &&
-     git update-index --add ND &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '10 (fail) - must match and be up-to-date in O && A && !B && O==A case' "
-     rm -f .git/index ND &&
-     cp .orig-A/ND ND &&
-     git update-index --add ND &&
-     echo extra >>ND &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '10 (fail) - must match and be up-to-date in O && A && !B && O==A case' "
-     rm -f .git/index ND &&
-     cp .orig-A/ND ND &&
-     echo extra >>ND &&
-     git update-index --add ND &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '11 - must match and be up-to-date in O && A && B && O!=A && O!=B && A!=B case' \
-    "rm -f .git/index MM &&
-     cp .orig-A/MM MM &&
-     git update-index --add MM &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '11 (fail) - must match and be up-to-date in O && A && B && O!=A && O!=B && A!=B case' "
-     rm -f .git/index MM &&
-     cp .orig-A/MM MM &&
-     git update-index --add MM &&
-     echo extra >>MM &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '11 (fail) - must match and be up-to-date in O && A && B && O!=A && O!=B && A!=B case' "
-     rm -f .git/index MM &&
-     cp .orig-A/MM MM &&
-     echo extra >>MM &&
-     git update-index --add MM &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '12 - must match A in O && A && B && O!=A && A==B case' \
-    "rm -f .git/index SS &&
-     cp .orig-A/SS SS &&
-     git update-index --add SS &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '12 - must match A in O && A && B && O!=A && A==B case' \
-    "rm -f .git/index SS &&
-     cp .orig-A/SS SS &&
-     git update-index --add SS &&
-     echo extra >>SS &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '12 (fail) - must match A in O && A && B && O!=A && A==B case' "
-     rm -f .git/index SS &&
-     cp .orig-A/SS SS &&
-     echo extra >>SS &&
-     git update-index --add SS &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '13 - must match A in O && A && B && O!=A && O==B case' \
-    "rm -f .git/index MN &&
-     cp .orig-A/MN MN &&
-     git update-index --add MN &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '13 - must match A in O && A && B && O!=A && O==B case' \
-    "rm -f .git/index MN &&
-     cp .orig-A/MN MN &&
-     git update-index --add MN &&
-     echo extra >>MN &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '14 - must match and be up-to-date in O && A && B && O==A && O!=B case' \
-    "rm -f .git/index NM &&
-     cp .orig-A/NM NM &&
-     git update-index --add NM &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '14 - may match B in O && A && B && O==A && O!=B case' \
-    "rm -f .git/index NM &&
-     cp .orig-B/NM NM &&
-     git update-index --add NM &&
-     echo extra >>NM &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '14 (fail) - must match and be up-to-date in O && A && B && O==A && O!=B case' "
-     rm -f .git/index NM &&
-     cp .orig-A/NM NM &&
-     git update-index --add NM &&
-     echo extra >>NM &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '14 (fail) - must match and be up-to-date in O && A && B && O==A && O!=B case' "
-     rm -f .git/index NM &&
-     cp .orig-A/NM NM &&
-     echo extra >>NM &&
-     git update-index --add NM &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-test_expect_success \
-    '15 - must match A in O && A && B && O==A && O==B case' \
-    "rm -f .git/index NN &&
-     cp .orig-A/NN NN &&
-     git update-index --add NN &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '15 - must match A in O && A && B && O==A && O==B case' \
-    "rm -f .git/index NN &&
-     cp .orig-A/NN NN &&
-     git update-index --add NN &&
-     echo extra >>NN &&
-     read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
-     check_result"
-
-test_expect_success \
-    '15 (fail) - must match A in O && A && B && O==A && O==B case' "
-     rm -f .git/index NN &&
-     cp .orig-A/NN NN &&
-     echo extra >>NN &&
-     git update-index --add NN &&
-     read_tree_must_fail -m $tree_O $tree_A $tree_B
-"
-
-# #16
-test_expect_success \
-    '16 - A matches in one and B matches in another.' \
-    'rm -f .git/index F16 &&
-    echo F16 >F16 &&
-    git update-index --add F16 &&
-    tree0=$(git write-tree) &&
-    echo E16 >F16 &&
-    git update-index F16 &&
-    tree1=$(git write-tree) &&
-    read_tree_must_succeed -m $tree0 $tree1 $tree1 $tree0 &&
-    git ls-files --stage'
+test_expect_success '1 - must not have an entry not in A.' '
+	rm -f .git/index XX &&
+	echo XX >XX &&
+	git update-index --add XX &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '2 - must match B in !O && !A && B case.' '
+	rm -f .git/index NA &&
+	cp .orig-B/NA NA &&
+	git update-index --add NA &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '2 - matching B alone is OK in !O && !A && B case.' '
+	rm -f .git/index NA &&
+	cp .orig-B/NA NA &&
+	git update-index --add NA &&
+	echo extra >>NA &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '3 - must match A in !O && A && !B case.' '
+	rm -f .git/index AN &&
+	cp .orig-A/AN AN &&
+	git update-index --add AN &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '3 - matching A alone is OK in !O && A && !B case.' '
+	rm -f .git/index AN &&
+	cp .orig-A/AN AN &&
+	git update-index --add AN &&
+	echo extra >>AN &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '3 (fail) - must match A in !O && A && !B case.' '
+	rm -f .git/index AN &&
+	cp .orig-A/AN AN &&
+	echo extra >>AN &&
+	git update-index --add AN &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '4 - must match and be up-to-date in !O && A && B && A!=B case.' '
+	rm -f .git/index AA &&
+	cp .orig-A/AA AA &&
+	git update-index --add AA &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '4 (fail) - must match and be up-to-date in !O && A && B && A!=B case.' '
+	rm -f .git/index AA &&
+	cp .orig-A/AA AA &&
+	git update-index --add AA &&
+	echo extra >>AA &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '4 (fail) - must match and be up-to-date in !O && A && B && A!=B case.' '
+	rm -f .git/index AA &&
+	cp .orig-A/AA AA &&
+	echo extra >>AA &&
+	git update-index --add AA &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '5 - must match in !O && A && B && A==B case.' '
+	rm -f .git/index LL &&
+	cp .orig-A/LL LL &&
+	git update-index --add LL &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '5 - must match in !O && A && B && A==B case.' '
+	rm -f .git/index LL &&
+	cp .orig-A/LL LL &&
+	git update-index --add LL &&
+	echo extra >>LL &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '5 (fail) - must match A in !O && A && B && A==B case.' '
+	rm -f .git/index LL &&
+	cp .orig-A/LL LL &&
+	echo extra >>LL &&
+	git update-index --add LL &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '6 - must not exist in O && !A && !B case' '
+	rm -f .git/index DD &&
+	echo DD >DD &&
+	git update-index --add DD &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '7 - must not exist in O && !A && B && O!=B case' '
+	rm -f .git/index DM &&
+	cp .orig-B/DM DM &&
+	git update-index --add DM &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '8 - must not exist in O && !A && B && O==B case' '
+	rm -f .git/index DN &&
+	cp .orig-B/DN DN &&
+	git update-index --add DN &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '9 - must match and be up-to-date in O && A && !B && O!=A case' '
+	rm -f .git/index MD &&
+	cp .orig-A/MD MD &&
+	git update-index --add MD &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '9 (fail) - must match and be up-to-date in O && A && !B && O!=A case' '
+	rm -f .git/index MD &&
+	cp .orig-A/MD MD &&
+	git update-index --add MD &&
+	echo extra >>MD &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '9 (fail) - must match and be up-to-date in O && A && !B && O!=A case' '
+	rm -f .git/index MD &&
+	cp .orig-A/MD MD &&
+	echo extra >>MD &&
+	git update-index --add MD &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '10 - must match and be up-to-date in O && A && !B && O==A case' '
+	rm -f .git/index ND &&
+	cp .orig-A/ND ND &&
+	git update-index --add ND &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '10 (fail) - must match and be up-to-date in O && A && !B && O==A case' '
+	rm -f .git/index ND &&
+	cp .orig-A/ND ND &&
+	git update-index --add ND &&
+	echo extra >>ND &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '10 (fail) - must match and be up-to-date in O && A && !B && O==A case' '
+	rm -f .git/index ND &&
+	cp .orig-A/ND ND &&
+	echo extra >>ND &&
+	git update-index --add ND &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '11 - must match and be up-to-date in O && A && B && O!=A && O!=B && A!=B case' '
+	rm -f .git/index MM &&
+	cp .orig-A/MM MM &&
+	git update-index --add MM &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '11 (fail) - must match and be up-to-date in O && A && B && O!=A && O!=B && A!=B case' '
+	rm -f .git/index MM &&
+	cp .orig-A/MM MM &&
+	git update-index --add MM &&
+	echo extra >>MM &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '11 (fail) - must match and be up-to-date in O && A && B && O!=A && O!=B && A!=B case' '
+	rm -f .git/index MM &&
+	cp .orig-A/MM MM &&
+	echo extra >>MM &&
+	git update-index --add MM &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '12 - must match A in O && A && B && O!=A && A==B case' '
+	rm -f .git/index SS &&
+	cp .orig-A/SS SS &&
+	git update-index --add SS &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '12 - must match A in O && A && B && O!=A && A==B case' '
+	rm -f .git/index SS &&
+	cp .orig-A/SS SS &&
+	git update-index --add SS &&
+	echo extra >>SS &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '12 (fail) - must match A in O && A && B && O!=A && A==B case' '
+	rm -f .git/index SS &&
+	cp .orig-A/SS SS &&
+	echo extra >>SS &&
+	git update-index --add SS &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '13 - must match A in O && A && B && O!=A && O==B case' '
+	rm -f .git/index MN &&
+	cp .orig-A/MN MN &&
+	git update-index --add MN &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '13 - must match A in O && A && B && O!=A && O==B case' '
+	rm -f .git/index MN &&
+	cp .orig-A/MN MN &&
+	git update-index --add MN &&
+	echo extra >>MN &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '14 - must match and be up-to-date in O && A && B && O==A && O!=B case' '
+	rm -f .git/index NM &&
+	cp .orig-A/NM NM &&
+	git update-index --add NM &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '14 - may match B in O && A && B && O==A && O!=B case' '
+	rm -f .git/index NM &&
+	cp .orig-B/NM NM &&
+	git update-index --add NM &&
+	echo extra >>NM &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '14 (fail) - must match and be up-to-date in O && A && B && O==A && O!=B case' '
+	rm -f .git/index NM &&
+	cp .orig-A/NM NM &&
+	git update-index --add NM &&
+	echo extra >>NM &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '14 (fail) - must match and be up-to-date in O && A && B && O==A && O!=B case' '
+	rm -f .git/index NM &&
+	cp .orig-A/NM NM &&
+	echo extra >>NM &&
+	git update-index --add NM &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '15 - must match A in O && A && B && O==A && O==B case' '
+	rm -f .git/index NN &&
+	cp .orig-A/NN NN &&
+	git update-index --add NN &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '15 - must match A in O && A && B && O==A && O==B case' '
+	rm -f .git/index NN &&
+	cp .orig-A/NN NN &&
+	git update-index --add NN &&
+	echo extra >>NN &&
+	read_tree_must_succeed -m $tree_O $tree_A $tree_B &&
+	check_result
+'
+
+test_expect_success '15 (fail) - must match A in O && A && B && O==A && O==B case' '
+	rm -f .git/index NN &&
+	cp .orig-A/NN NN &&
+	echo extra >>NN &&
+	git update-index --add NN &&
+	read_tree_must_fail -m $tree_O $tree_A $tree_B
+'
+
+test_expect_success '16 - A matches in one and B matches in another.' '
+	rm -f .git/index F16 &&
+	echo F16 >F16 &&
+	git update-index --add F16 &&
+	tree0=$(git write-tree) &&
+	echo E16 >F16 &&
+	git update-index F16 &&
+	tree1=$(git write-tree) &&
+	read_tree_must_succeed -m $tree0 $tree1 $tree1 $tree0 &&
+	git ls-files --stage
+'
 
 test_done
-- 
2.11.0.rc2.30.g7c4be45.dirty

Re: [PATCH 2/4] t1000: modernize style

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The preferred style in tests seems to be

s/seems to be/is/;

>
> test_expect_success 'short description, ended by 2 single quotes' '
> 	here comes the test &&
> 	and chains over many lines &&
> 	ended by a quote
> '

Thanks.  This is way overdue.  During the time the script has been
dormant for more than two years, we should have done this.

Re: [PATCH 2/4] t1000: modernize style

[Stefan Beller]

On Tue, Jan 10, 2017 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The preferred style in tests seems to be
>
> s/seems to be/is/;

If this is the only nit, mind to fix up the commit message locally?
(I was even unsure if we want to have this patch as part
of a larger series, as it is just refactoring for the sake of refactoring,
i.e. t1000 doesn't see a new test in this series, only t1001 does)

>
>>
>> test_expect_success 'short description, ended by 2 single quotes' '
>>       here comes the test &&
>>       and chains over many lines &&
>>       ended by a quote
>> '
>
> Thanks.  This is way overdue.  During the time the script has been
> dormant for more than two years, we should have done this.

agreed.

Re: [PATCH 2/4] t1000: modernize style

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Jan 10, 2017 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> The preferred style in tests seems to be
>>
>> s/seems to be/is/;
>
> If this is the only nit, mind to fix up the commit message locally?

Certainly.  It wasn't even meant as a "nitpick".  I was just
confirming your observation.

[PATCH 3/4] t1001: modernize style

[Stefan Beller]

The preferred style in tests seems to be

test_expect_success 'short description, ended by 2 single quotes' '
	here comes the test &&
	and chains over many lines &&
 	ended by a quote
'

Or going by the numbers:
    $ git -C t/ grep "' '$" | wc -l
    9796
    $ git -C t/ grep test_expect_ |wc -l
    11861

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t1001-read-tree-m-2way.sh | 641 ++++++++++++++++++++++----------------------
 1 file changed, 320 insertions(+), 321 deletions(-)

diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index db1b6f5cf4..7b70089705 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -14,10 +14,10 @@ all the combinations described in the two-tree merge "carry forward"
 rules, found in <Documentation/git read-tree.txt>.
 
 In the test, these paths are used:
-        bozbar  - in H, stays in M, modified from bozbar to gnusto
-        frotz   - not in H added in M
-        nitfol  - in H, stays in M unmodified
-        rezrov  - in H, deleted in M
+	bozbar  - in H, stays in M, modified from bozbar to gnusto
+	frotz   - not in H added in M
+	nitfol  - in H, stays in M unmodified
+	rezrov  - in H, deleted in M
 	yomin   - not in H or M
 '
 . ./test-lib.sh
@@ -60,336 +60,335 @@ EOF
 
 sed -e 's/bozbar/gnusto (earlier bozbar)/' bozbar-old >bozbar-new
 
-test_expect_success \
-    setup \
-    'echo frotz >frotz &&
-     echo nitfol >nitfol &&
-     cat bozbar-old >bozbar &&
-     echo rezrov >rezrov &&
-     echo yomin >yomin &&
-     git update-index --add nitfol bozbar rezrov &&
-     treeH=$(git write-tree) &&
-     echo treeH $treeH &&
-     git ls-tree $treeH &&
-
-     cat bozbar-new >bozbar &&
-     git update-index --add frotz bozbar --force-remove rezrov &&
-     git ls-files --stage >M.out &&
-     treeM=$(git write-tree) &&
-     echo treeM $treeM &&
-     git ls-tree $treeM &&
-     git diff-tree $treeH $treeM'
-
-test_expect_success \
-    '1, 2, 3 - no carry forward' \
-    'rm -f .git/index &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >1-3.out &&
-     test_cmp M.out 1-3.out &&
-     check_cache_at bozbar dirty &&
-     check_cache_at frotz dirty &&
-     check_cache_at nitfol dirty'
+test_expect_success 'setup' '
+	echo frotz >frotz &&
+	echo nitfol >nitfol &&
+	cat bozbar-old >bozbar &&
+	echo rezrov >rezrov &&
+	echo yomin >yomin &&
+	git update-index --add nitfol bozbar rezrov &&
+	treeH=$(git write-tree) &&
+	echo treeH $treeH &&
+	git ls-tree $treeH &&
+
+	cat bozbar-new >bozbar &&
+	git update-index --add frotz bozbar --force-remove rezrov &&
+	git ls-files --stage >M.out &&
+	treeM=$(git write-tree) &&
+	echo treeM $treeM &&
+	git ls-tree $treeM &&
+	git diff-tree $treeH $treeM
+'
 
+test_expect_success '1, 2, 3 - no carry forward' '
+	rm -f .git/index &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >1-3.out &&
+	test_cmp M.out 1-3.out &&
+	check_cache_at bozbar dirty &&
+	check_cache_at frotz dirty &&
+	check_cache_at nitfol dirty
+'
 echo '+100644 X 0	yomin' >expected
 
-test_expect_success \
-    '4 - carry forward local addition.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     git update-index --add yomin &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >4.out &&
-     test_must_fail git diff --no-index M.out 4.out >4diff.out &&
-     compare_change 4diff.out expected &&
-     check_cache_at yomin clean'
-
-test_expect_success \
-    '5 - carry forward local addition.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo yomin >yomin &&
-     git update-index --add yomin &&
-     echo yomin yomin >yomin &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >5.out &&
-     test_must_fail git diff --no-index M.out 5.out >5diff.out &&
-     compare_change 5diff.out expected &&
-     check_cache_at yomin dirty'
-
-test_expect_success \
-    '6 - local addition already has the same.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     git update-index --add frotz &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >6.out &&
-     test_cmp M.out 6.out &&
-     check_cache_at frotz clean'
-
-test_expect_success \
-    '7 - local addition already has the same.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo frotz >frotz &&
-     git update-index --add frotz &&
-     echo frotz frotz >frotz &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >7.out &&
-     test_cmp M.out 7.out &&
-     check_cache_at frotz dirty'
-
-test_expect_success \
-    '8 - conflicting addition.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo frotz frotz >frotz &&
-     git update-index --add frotz &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
-
-test_expect_success \
-    '9 - conflicting addition.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo frotz frotz >frotz &&
-     git update-index --add frotz &&
-     echo frotz >frotz &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
-
-test_expect_success \
-    '10 - path removed.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo rezrov >rezrov &&
-     git update-index --add rezrov &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >10.out &&
-     test_cmp M.out 10.out'
-
-test_expect_success \
-    '11 - dirty path removed.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo rezrov >rezrov &&
-     git update-index --add rezrov &&
-     echo rezrov rezrov >rezrov &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
-
-test_expect_success \
-    '12 - unmatching local changes being removed.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo rezrov rezrov >rezrov &&
-     git update-index --add rezrov &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
-
-test_expect_success \
-    '13 - unmatching local changes being removed.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo rezrov rezrov >rezrov &&
-     git update-index --add rezrov &&
-     echo rezrov >rezrov &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
+test_expect_success '4 - carry forward local addition.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	git update-index --add yomin &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >4.out &&
+	test_must_fail git diff --no-index M.out 4.out >4diff.out &&
+	compare_change 4diff.out expected &&
+	check_cache_at yomin clean
+'
+
+test_expect_success '5 - carry forward local addition.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo yomin >yomin &&
+	git update-index --add yomin &&
+	echo yomin yomin >yomin &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >5.out &&
+	test_must_fail git diff --no-index M.out 5.out >5diff.out &&
+	compare_change 5diff.out expected &&
+	check_cache_at yomin dirty
+'
+
+test_expect_success '6 - local addition already has the same.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	git update-index --add frotz &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >6.out &&
+	test_cmp M.out 6.out &&
+	check_cache_at frotz clean
+'
+
+test_expect_success '7 - local addition already has the same.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo frotz >frotz &&
+	git update-index --add frotz &&
+	echo frotz frotz >frotz &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >7.out &&
+	test_cmp M.out 7.out &&
+	check_cache_at frotz dirty
+'
+
+test_expect_success '8 - conflicting addition.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo frotz frotz >frotz &&
+	git update-index --add frotz &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
+
+test_expect_success '9 - conflicting addition.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo frotz frotz >frotz &&
+	git update-index --add frotz &&
+	echo frotz >frotz &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
+
+test_expect_success '10 - path removed.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo rezrov >rezrov &&
+	git update-index --add rezrov &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >10.out &&
+	test_cmp M.out 10.out
+'
+
+test_expect_success '11 - dirty path removed.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo rezrov >rezrov &&
+	git update-index --add rezrov &&
+	echo rezrov rezrov >rezrov &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
+
+test_expect_success '12 - unmatching local changes being removed.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo rezrov rezrov >rezrov &&
+	git update-index --add rezrov &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
+
+test_expect_success '13 - unmatching local changes being removed.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo rezrov rezrov >rezrov &&
+	git update-index --add rezrov &&
+	echo rezrov >rezrov &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
 
 cat >expected <<EOF
 -100644 X 0	nitfol
 +100644 X 0	nitfol
 EOF
 
-test_expect_success \
-    '14 - unchanged in two heads.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo nitfol nitfol >nitfol &&
-     git update-index --add nitfol &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >14.out &&
-     test_must_fail git diff --no-index M.out 14.out >14diff.out &&
-     compare_change 14diff.out expected &&
-     check_cache_at nitfol clean'
-
-test_expect_success \
-    '15 - unchanged in two heads.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo nitfol nitfol >nitfol &&
-     git update-index --add nitfol &&
-     echo nitfol nitfol nitfol >nitfol &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >15.out &&
-     test_must_fail git diff --no-index M.out 15.out >15diff.out &&
-     compare_change 15diff.out expected &&
-     check_cache_at nitfol dirty'
-
-test_expect_success \
-    '16 - conflicting local change.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo bozbar bozbar >bozbar &&
-     git update-index --add bozbar &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
-
-test_expect_success \
-    '17 - conflicting local change.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     echo bozbar bozbar >bozbar &&
-     git update-index --add bozbar &&
-     echo bozbar bozbar bozbar >bozbar &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
-
-test_expect_success \
-    '18 - local change already having a good result.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     cat bozbar-new >bozbar &&
-     git update-index --add bozbar &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >18.out &&
-     test_cmp M.out 18.out &&
-     check_cache_at bozbar clean'
-
-test_expect_success \
-    '19 - local change already having a good result, further modified.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     cat bozbar-new >bozbar &&
-     git update-index --add bozbar &&
-     echo gnusto gnusto >bozbar &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >19.out &&
-     test_cmp M.out 19.out &&
-     check_cache_at bozbar dirty'
-
-test_expect_success \
-    '20 - no local change, use new tree.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     cat bozbar-old >bozbar &&
-     git update-index --add bozbar &&
-     read_tree_twoway $treeH $treeM &&
-     git ls-files --stage >20.out &&
-     test_cmp M.out 20.out &&
-     check_cache_at bozbar dirty'
-
-test_expect_success \
-    '21 - no local change, dirty cache.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     cat bozbar-old >bozbar &&
-     git update-index --add bozbar &&
-     echo gnusto gnusto >bozbar &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
+test_expect_success '14 - unchanged in two heads.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo nitfol nitfol >nitfol &&
+	git update-index --add nitfol &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >14.out &&
+	test_must_fail git diff --no-index M.out 14.out >14diff.out &&
+	compare_change 14diff.out expected &&
+	check_cache_at nitfol clean
+'
+
+test_expect_success '15 - unchanged in two heads.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo nitfol nitfol >nitfol &&
+	git update-index --add nitfol &&
+	echo nitfol nitfol nitfol >nitfol &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >15.out &&
+	test_must_fail git diff --no-index M.out 15.out >15diff.out &&
+	compare_change 15diff.out expected &&
+	check_cache_at nitfol dirty
+'
+
+test_expect_success '16 - conflicting local change.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo bozbar bozbar >bozbar &&
+	git update-index --add bozbar &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
+
+test_expect_success '17 - conflicting local change.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	echo bozbar bozbar >bozbar &&
+	git update-index --add bozbar &&
+	echo bozbar bozbar bozbar >bozbar &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
+
+test_expect_success '18 - local change already having a good result.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	cat bozbar-new >bozbar &&
+	git update-index --add bozbar &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >18.out &&
+	test_cmp M.out 18.out &&
+	check_cache_at bozbar clean
+'
+
+test_expect_success '19 - local change already having a good result, further modified.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	cat bozbar-new >bozbar &&
+	git update-index --add bozbar &&
+	echo gnusto gnusto >bozbar &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >19.out &&
+	test_cmp M.out 19.out &&
+	check_cache_at bozbar dirty
+'
+
+test_expect_success '20 - no local change, use new tree.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	cat bozbar-old >bozbar &&
+	git update-index --add bozbar &&
+	read_tree_twoway $treeH $treeM &&
+	git ls-files --stage >20.out &&
+	test_cmp M.out 20.out &&
+	check_cache_at bozbar dirty
+'
+
+test_expect_success '21 - no local change, dirty cache.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	cat bozbar-old >bozbar &&
+	git update-index --add bozbar &&
+	echo gnusto gnusto >bozbar &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
 
 # This fails with straight two-way fast-forward.
-test_expect_success \
-    '22 - local change cache updated.' \
-    'rm -f .git/index &&
-     read_tree_must_succeed $treeH &&
-     git checkout-index -u -f -q -a &&
-     sed -e "s/such as/SUCH AS/" bozbar-old >bozbar &&
-     git update-index --add bozbar &&
-     if read_tree_twoway $treeH $treeM; then false; else :; fi'
+test_expect_success '22 - local change cache updated.' '
+	rm -f .git/index &&
+	read_tree_must_succeed $treeH &&
+	git checkout-index -u -f -q -a &&
+	sed -e "s/such as/SUCH AS/" bozbar-old >bozbar &&
+	git update-index --add bozbar &&
+	if read_tree_twoway $treeH $treeM; then false; else :; fi
+'
 
 # Also make sure we did not break DF vs DF/DF case.
-test_expect_success \
-    'DF vs DF/DF case setup.' \
-    'rm -f .git/index &&
-     echo DF >DF &&
-     git update-index --add DF &&
-     treeDF=$(git write-tree) &&
-     echo treeDF $treeDF &&
-     git ls-tree $treeDF &&
-
-     rm -f DF &&
-     mkdir DF &&
-     echo DF/DF >DF/DF &&
-     git update-index --add --remove DF DF/DF &&
-     treeDFDF=$(git write-tree) &&
-     echo treeDFDF $treeDFDF &&
-     git ls-tree $treeDFDF &&
-     git ls-files --stage >DFDF.out'
-
-test_expect_success \
-    'DF vs DF/DF case test.' \
-    'rm -f .git/index &&
-     rm -fr DF &&
-     echo DF >DF &&
-     git update-index --add DF &&
-     read_tree_twoway $treeDF $treeDFDF &&
-     git ls-files --stage >DFDFcheck.out &&
-     test_cmp DFDF.out DFDFcheck.out &&
-     check_cache_at DF/DF dirty &&
-     :'
-
-test_expect_success \
-    'a/b (untracked) vs a case setup.' \
-    'rm -f .git/index &&
-     : >a &&
-     git update-index --add a &&
-     treeM=$(git write-tree) &&
-     echo treeM $treeM &&
-     git ls-tree $treeM &&
-     git ls-files --stage >treeM.out &&
-
-     rm -f a &&
-     git update-index --remove a &&
-     mkdir a &&
-     : >a/b &&
-     treeH=$(git write-tree) &&
-     echo treeH $treeH &&
-     git ls-tree $treeH'
-
-test_expect_success \
-    'a/b (untracked) vs a, plus c/d case test.' \
-    'read_tree_u_must_fail -u -m "$treeH" "$treeM" &&
-     git ls-files --stage &&
-     test -f a/b'
-
-test_expect_success \
-    'a/b vs a, plus c/d case setup.' \
-    'rm -f .git/index &&
-     rm -fr a &&
-     : >a &&
-     mkdir c &&
-     : >c/d &&
-     git update-index --add a c/d &&
-     treeM=$(git write-tree) &&
-     echo treeM $treeM &&
-     git ls-tree $treeM &&
-     git ls-files --stage >treeM.out &&
-
-     rm -f a &&
-     mkdir a &&
-     : >a/b &&
-     git update-index --add --remove a a/b &&
-     treeH=$(git write-tree) &&
-     echo treeH $treeH &&
-     git ls-tree $treeH'
-
-test_expect_success \
-    'a/b vs a, plus c/d case test.' \
-    'read_tree_u_must_succeed -u -m "$treeH" "$treeM" &&
-     git ls-files --stage | tee >treeMcheck.out &&
-     test_cmp treeM.out treeMcheck.out'
+test_expect_success 'DF vs DF/DF case setup.' '
+	rm -f .git/index &&
+	echo DF >DF &&
+	git update-index --add DF &&
+	treeDF=$(git write-tree) &&
+	echo treeDF $treeDF &&
+	git ls-tree $treeDF &&
+
+	rm -f DF &&
+	mkdir DF &&
+	echo DF/DF >DF/DF &&
+	git update-index --add --remove DF DF/DF &&
+	treeDFDF=$(git write-tree) &&
+	echo treeDFDF $treeDFDF &&
+	git ls-tree $treeDFDF &&
+	git ls-files --stage >DFDF.out
+'
+
+test_expect_success 'DF vs DF/DF case test.' '
+	rm -f .git/index &&
+	rm -fr DF &&
+	echo DF >DF &&
+	git update-index --add DF &&
+	read_tree_twoway $treeDF $treeDFDF &&
+	git ls-files --stage >DFDFcheck.out &&
+	test_cmp DFDF.out DFDFcheck.out &&
+	check_cache_at DF/DF dirty &&
+	:
+'
+
+test_expect_success 'a/b (untracked) vs a case setup.' '
+	rm -f .git/index &&
+	: >a &&
+	git update-index --add a &&
+	treeM=$(git write-tree) &&
+	echo treeM $treeM &&
+	git ls-tree $treeM &&
+	git ls-files --stage >treeM.out &&
+
+	rm -f a &&
+	git update-index --remove a &&
+	mkdir a &&
+	: >a/b &&
+	treeH=$(git write-tree) &&
+	echo treeH $treeH &&
+	git ls-tree $treeH
+'
+
+test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
+	read_tree_u_must_fail -u -m "$treeH" "$treeM" &&
+	git ls-files --stage &&
+	test -f a/b
+'
+
+test_expect_success 'a/b vs a, plus c/d case setup.' '
+	rm -f .git/index &&
+	rm -fr a &&
+	: >a &&
+	mkdir c &&
+	: >c/d &&
+	git update-index --add a c/d &&
+	treeM=$(git write-tree) &&
+	echo treeM $treeM &&
+	git ls-tree $treeM &&
+	git ls-files --stage >treeM.out &&
+
+	rm -f a &&
+	mkdir a &&
+	: >a/b &&
+	git update-index --add --remove a a/b &&
+	treeH=$(git write-tree) &&
+	echo treeH $treeH &&
+	git ls-tree $treeH
+'
+
+test_expect_success 'a/b vs a, plus c/d case test.' '
+	read_tree_u_must_succeed -u -m "$treeH" "$treeM" &&
+	git ls-files --stage | tee >treeMcheck.out &&
+	test_cmp treeM.out treeMcheck.out
+'
 
 test_expect_success '-m references the correct modified tree' '
 	echo >file-a &&
-- 
2.11.0.rc2.30.g7c4be45.dirty

[PATCH 4/4] unpack-trees: support super-prefix option

[Stefan Beller]

Add support for the super-prefix option for commands that unpack trees.
For testing purposes enable it in read-tree, which has no other path
related output.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git.c                       |  2 +-
 t/t1001-read-tree-m-2way.sh |  9 +++++++++
 unpack-trees.c              | 39 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index dce529fcbf..acbabd1298 100644
--- a/git.c
+++ b/git.c
@@ -471,7 +471,7 @@ static struct cmd_struct commands[] = {
 	{ "prune-packed", cmd_prune_packed, RUN_SETUP },
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
-	{ "read-tree", cmd_read_tree, RUN_SETUP },
+	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 7b70089705..014ba39420 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -363,6 +363,15 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
 	test -f a/b
 '
 
+cat <<-EOF >expect &&
+	error: Updating 'fictional/a' would lose untracked files in it
+EOF
+
+test_expect_success 'read-tree supports the super-prefix' '
+	test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'a/b vs a, plus c/d case setup.' '
 	rm -f .git/index &&
 	rm -fr a &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 7a6df99d10..bc56195e27 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -52,6 +52,37 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 	  ? ((o)->msgs[(type)])      \
 	  : (unpack_plumbing_errors[(type)]) )
 
+static const char *super_prefixed(const char *path)
+{
+	/*
+	 * This is used for the error messages above.
+	 * We need to have exactly two buffer spaces.
+	 */
+	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
+	static int super_prefix_len = -1;
+	static unsigned idx = 0;
+
+	if (!get_super_prefix())
+		return path;
+
+	if (super_prefix_len < 0) {
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(buf); i++)
+			strbuf_addstr(&buf[i], get_super_prefix());
+
+		super_prefix_len = strlen(get_super_prefix());
+	}
+
+	if (++idx >= ARRAY_SIZE(buf))
+		idx = 0;
+
+	strbuf_setlen(&buf[idx], super_prefix_len);
+	strbuf_addstr(&buf[idx], path);
+
+	return buf[idx].buf;
+}
+
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 				  const char *cmd)
 {
@@ -172,7 +203,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
 			     const char *path)
 {
 	if (!o->show_all_errors)
-		return error(ERRORMSG(o, e), path);
+		return error(ERRORMSG(o, e), super_prefixed(path));
 
 	/*
 	 * Otherwise, insert in a list for future display by
@@ -196,7 +227,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
 			something_displayed = 1;
 			for (i = 0; i < rejects->nr; i++)
 				strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
-			error(ERRORMSG(o, e), path.buf);
+			error(ERRORMSG(o, e), super_prefixed(path.buf));
 			strbuf_release(&path);
 		}
 		string_list_clear(rejects, 0);
@@ -1918,7 +1949,9 @@ int bind_merge(const struct cache_entry * const *src,
 			     o->merge_size);
 	if (a && old)
 		return o->gently ? -1 :
-			error(ERRORMSG(o, ERROR_BIND_OVERLAP), a->name, old->name);
+			error(ERRORMSG(o, ERROR_BIND_OVERLAP),
+			      super_prefixed(a->name),
+			      super_prefixed(old->name));
 	if (!a)
 		return keep_entry(old, o);
 	else
-- 
2.11.0.rc2.30.g7c4be45.dirty

Re: [PATCH 4/4] unpack-trees: support super-prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Add support for the super-prefix option for commands that unpack trees.
> For testing purposes enable it in read-tree, which has no other path
> related output.

"path related output"?  I am not sure I understand this.

When read-tree reads N trees, or unpack_trees() is asked to "merge"
N trees, how does --super-prefix supposed to interact with the paths
in these trees?  Does the prefix added to paths in all trees?

Did you mean that read-tree (and unpack_trees() in general) is a
whole-tree operation and there is no path related input (i.e.
pathspec limiting), but if another command that started in the
superproject is running us in its submodule, it wants us to prefix
the path to the submodule when we report errors?

If that is what is going on, I think it makes sense, but it may be
asking too much from the readers to guess that from "Add support for
the super-prefix option".

> --- a/t/t1001-read-tree-m-2way.sh
> +++ b/t/t1001-read-tree-m-2way.sh
> @@ -363,6 +363,15 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
>  	test -f a/b
>  '
>  
> +cat <<-EOF >expect &&
> +	error: Updating 'fictional/a' would lose untracked files in it
> +EOF
> +
> +test_expect_success 'read-tree supports the super-prefix' '
> +	test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
> +	test_cmp expect actual
> +'
> +

Preparing the expected output "expect" outside test_expect_success
block is also old-school.  Move it inside the new test?

> diff --git a/unpack-trees.c b/unpack-trees.c
> index 7a6df99d10..bc56195e27 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -52,6 +52,37 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
>  	  ? ((o)->msgs[(type)])      \
>  	  : (unpack_plumbing_errors[(type)]) )
>  
> +static const char *super_prefixed(const char *path)
> +{
> +	/*
> +	 * This is used for the error messages above.
> +	 * We need to have exactly two buffer spaces.
> +	 */
> +	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
> +	static int super_prefix_len = -1;
> +	static unsigned idx = 0;
> +
> +	if (!get_super_prefix())
> +		return path;
> +
> +	if (super_prefix_len < 0) {
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(buf); i++)
> +			strbuf_addstr(&buf[i], get_super_prefix());
> +
> +		super_prefix_len = strlen(get_super_prefix());
> +	}
> +
> +	if (++idx >= ARRAY_SIZE(buf))
> +		idx = 0;
> +
> +	strbuf_setlen(&buf[idx], super_prefix_len);
> +	strbuf_addstr(&buf[idx], path);
> +
> +	return buf[idx].buf;
> +}

Hmph, as a reader of the code, I do not even want to wonder how
expensive get_super_prefix() is.  If the executable part of the
above were written like this, it would have been easier to
understand:

	if (super_prefix_len < 0) {
		if (!get_super_prefix())
			super_prefix_len = 0;
		else {
			int i;
			... prepare buf[] and set super_prefix_len ...;
                }
	}

        if (!super_prefix_len)
		return path;

	... use buf[] to do the prefixing and return it ...

Re: [PATCH 4/4] unpack-trees: support super-prefix option

[Stefan Beller]

On Wed, Jan 11, 2017 at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Add support for the super-prefix option for commands that unpack trees.
>> For testing purposes enable it in read-tree, which has no other path
>> related output.
>
> "path related output"?  I am not sure I understand this.

Well, s/path related output/output of paths/.

> When read-tree reads N trees, or unpack_trees() is asked to "merge"
> N trees, how does --super-prefix supposed to interact with the paths
> in these trees?  Does the prefix added to paths in all trees?

Internally the super-prefix is ignored. Only the (input and) output
is using that super prefix for messages.

It was introduced for grepping recursively into submodules, i.e.

invoke as

    git grep --recurse-submodules \
      -e path/inside/submodule/and/further/down
    # internally it invokes:
    git -C .. --super-prefix .. grep ..
    # which operates "just normal" except for the
    # input parsing and output

The use case for this patch is working tree related things, i.e.
    git checkout --recurse-submodules
    # internally when recursing we call "git read-tree -u", but
    # reporting could be:
    Your local changes to the following files would be overwritten by checkout:
      path/inside/submodule/file.txt


>
> Did you mean that read-tree (and unpack_trees() in general) is a
> whole-tree operation and there is no path related input (i.e.
> pathspec limiting), but if another command that started in the
> superproject is running us in its submodule, it wants us to prefix
> the path to the submodule when we report errors?

Yes. I tried to explain it better above, but you got it here.

>
> If that is what is going on, I think it makes sense, but it may be
> asking too much from the readers to guess that from "Add support for
> the super-prefix option".

I need to enhance the commit message by a lot then.

>
>> --- a/t/t1001-read-tree-m-2way.sh
>> +++ b/t/t1001-read-tree-m-2way.sh
>> @@ -363,6 +363,15 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
>>       test -f a/b
>>  '
>>
>> +cat <<-EOF >expect &&
>> +     error: Updating 'fictional/a' would lose untracked files in it
>> +EOF
>> +
>> +test_expect_success 'read-tree supports the super-prefix' '
>> +     test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
>> +     test_cmp expect actual
>> +'
>> +
>
> Preparing the expected output "expect" outside test_expect_success
> block is also old-school.  Move it inside the new test?

I looked into that. What is our current stance on using single/double quotes?
Most tests use single quotes for the test title as well as the test
instructions,
such that double quotes can be used naturally in there. But single quotes
cannot be used even escaped, such that we need to do a thing like

sq="'"

test_expect_success 'test title' '
    cat <<-EOF >expect &&
        error for ${sq}path${sq}
    EOF
    test instructions go here
'

though that is not used as often as I would think as
grep \${sq} yields t1507 and t3005.

>
> Hmph, as a reader of the code, I do not even want to wonder how
> expensive get_super_prefix() is.  If the executable part of the
> above were written like this, it would have been easier to
> understand:
>
>         if (super_prefix_len < 0) {
>                 if (!get_super_prefix())
>                         super_prefix_len = 0;
>                 else {
>                         int i;
>                         ... prepare buf[] and set super_prefix_len ...;
>                 }
>         }
>
>         if (!super_prefix_len)
>                 return path;
>
>         ... use buf[] to do the prefixing and return it ...
>

good point. I'll fix that.

Re: [PATCH 4/4] unpack-trees: support super-prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Preparing the expected output "expect" outside test_expect_success
>> block is also old-school.  Move it inside the new test?
>
> I looked into that. What is our current stance on using single/double quotes?

Using dq around executable part, i.e.

	test_expect_success title "
		echo \"'foo'\"
	"

is a million-times more grave sin even if the test initially does
not refer to any variable in its body.  Somebody will eventually
need to add a line that refers to a variable and either forgets to
quote \$ in front or properly quotes it.  The former makes the
variable interpolated while the arguments to the test_expect_success
is prepared (which is a bug) and the latter makes the result quite
ugly, like so:

	test_expect_success title "
		sq=\' var=foo &&
		echo \"\${sq}\$value\${sq}\"
	"

Enclosing the body always in sq-pair does mean something ugly like
this from the beginning once you need to use sq inside:

	test_expect_success title '
		sq='\'' &&
		echo "${sq}foo${sq}"
	'

or

	test_expect_success title '
		echo "'\''foo'\''"
	'

but the ugliness is localized to places where single quote is
absolutely needed, and '\'' is something eyes soon get used to as an
idiom [*1*].  It does not affect every reference of variables like
enclosing with dq does.


[Footnote]

*1* That "idiom"-ness is the reason why the last example above is
    not written like this:

	test_expect_success title '
		echo "'\'foo\''"
	'

    even though the sq pair surrounding "foo" is not technically
    necessary.  It lets us think of the string as almost always is
    inside sq context, with the single exception that we step
    outside sq context and append a sq by saying bs-sq (\').

Re: [PATCH 4/4] unpack-trees: support super-prefix option

[Stefan Beller]

On Wed, Jan 11, 2017 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> Preparing the expected output "expect" outside test_expect_success
>>> block is also old-school.  Move it inside the new test?
>>
>> I looked into that. What is our current stance on using single/double quotes?
>
> Using dq around executable part, i.e.
>
>         test_expect_success title "
>                 echo \"'foo'\"
>         "
>
> is a million-times more grave sin even if the test initially does
> not refer to any variable in its body.  Somebody will eventually
> need to add a line that refers to a variable and either forgets to
> quote \$ in front or properly quotes it.

agreed.

>  The former makes the
> variable interpolated while the arguments to the test_expect_success
> is prepared (which is a bug) and the latter makes the result quite
> ugly, like so:
>
>         test_expect_success title "
>                 sq=\' var=foo &&
>                 echo \"\${sq}\$value\${sq}\"
>         "
>
> Enclosing the body always in sq-pair does mean something ugly like
> this from the beginning once you need to use sq inside:
>
>         test_expect_success title '
>                 sq='\'' &&
>                 echo "${sq}foo${sq}"
>         '

This one fails
error: bug in the test script: broken &&-chain:
                sq=' &&
                echo "${sq}foo${sq}"
both other occurrences of using ${sq} are defining sq outside
(one as sq=\' and the other as sq="'")

So I think we either have to keep sq out of the test,
    sq="'"
    test_expect_success title '
         echo "${sq}foo${sq}"
    '

or do the echo/printf trick inside:

    test_expect_success title '
         sq=$(echo -e "\x27") &&
         echo "${sq}foo${sq}"
    '

Eh, I just realize the '\'' works inside here-doc, too.
Nevermind then, I'll resend it with just quoted sq in the here-doc then.

Thanks,
Stefan

[PATCHv2 4/4] unpack-trees: support super-prefix option

[Stefan Beller]

In the future we want to support working tree operations within submodules,
e.g. "git checkout --recurse-submodules", which will update the submodule
to the commit as recorded in its superproject. In the submodule the
unpack-tree operation is carried out as usual, but the reporting to the
user needs to prefix any path with the superproject. The mechanism for
this is the super-prefix. (see 74866d757, git: make super-prefix option)

Add support for the super-prefix option for commands that unpack trees
by wrapping any path output in unpacking trees in the newly introduced
super_prefixed function. This new function prefixes any path with the
super-prefix if there is one.  Assuming the submodule case doesn't happen
in the majority of the cases, we'd want to have a fast behavior for no
super prefix, i.e. no reallocation/copying, but just returning path.

Another aspect of introducing the `super_prefixed` function is to consider
who owns the memory and if this is the right place where the path gets
modified. As the super prefix ought to change the output behavior only and
not the actual unpack tree part, it is fine to be that late in the line.
As we get passed in 'const char *path', we cannot change the path itself,
which means in case of a super prefix we have to copy over the path.
We need two static buffers in that function as the error messages
contain at most two paths.

For testing purposes enable it in read-tree, which has no output
of paths other than an unpack-trees.c. These are all converted in
this patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This is only patchv4 that is rerolled, patches 1-3 remain as is.

The test uses '\'' now, and the super_prefixed function is rewritten
using the flow Junio suggested.

The commit message got enhanced.

Thanks,
Stefan

 git.c                       |  2 +-
 t/t1001-read-tree-m-2way.sh |  8 ++++++++
 unpack-trees.c              | 38 +++++++++++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index dce529fcbf..acbabd1298 100644
--- a/git.c
+++ b/git.c
@@ -471,7 +471,7 @@ static struct cmd_struct commands[] = {
 	{ "prune-packed", cmd_prune_packed, RUN_SETUP },
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
-	{ "read-tree", cmd_read_tree, RUN_SETUP },
+	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 7b70089705..5ededd8e40 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -363,6 +363,14 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
 	test -f a/b
 '
 
+test_expect_success 'read-tree supports the super-prefix' '
+	cat <<-EOF >expect &&
+		error: Updating '\''fictional/a'\'' would lose untracked files in it
+	EOF
+	test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'a/b vs a, plus c/d case setup.' '
 	rm -f .git/index &&
 	rm -fr a &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 7a6df99d10..0a24472359 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -52,6 +52,36 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 	  ? ((o)->msgs[(type)])      \
 	  : (unpack_plumbing_errors[(type)]) )
 
+static const char *super_prefixed(const char *path)
+{
+	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
+	static int super_prefix_len = -1;
+	static unsigned idx = 0;
+
+	if (super_prefix_len < 0) {
+		if (!get_super_prefix())
+			super_prefix_len = 0;
+		else {
+			int i;
+
+			super_prefix_len = strlen(get_super_prefix());
+			for (i = 0; i < ARRAY_SIZE(buf); i++)
+				strbuf_addstr(&buf[i], get_super_prefix());
+		}
+	}
+
+	if (!super_prefix_len)
+		return path;
+
+	if (++idx >= ARRAY_SIZE(buf))
+		idx = 0;
+
+	strbuf_setlen(&buf[idx], super_prefix_len);
+	strbuf_addstr(&buf[idx], path);
+
+	return buf[idx].buf;
+}
+
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 				  const char *cmd)
 {
@@ -172,7 +202,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
 			     const char *path)
 {
 	if (!o->show_all_errors)
-		return error(ERRORMSG(o, e), path);
+		return error(ERRORMSG(o, e), super_prefixed(path));
 
 	/*
 	 * Otherwise, insert in a list for future display by
@@ -196,7 +226,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
 			something_displayed = 1;
 			for (i = 0; i < rejects->nr; i++)
 				strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
-			error(ERRORMSG(o, e), path.buf);
+			error(ERRORMSG(o, e), super_prefixed(path.buf));
 			strbuf_release(&path);
 		}
 		string_list_clear(rejects, 0);
@@ -1918,7 +1948,9 @@ int bind_merge(const struct cache_entry * const *src,
 			     o->merge_size);
 	if (a && old)
 		return o->gently ? -1 :
-			error(ERRORMSG(o, ERROR_BIND_OVERLAP), a->name, old->name);
+			error(ERRORMSG(o, ERROR_BIND_OVERLAP),
+			      super_prefixed(a->name),
+			      super_prefixed(old->name));
 	if (!a)
 		return keep_entry(old, o);
 	else
-- 
2.11.0.259.g7b30ecf4f0

Re: [PATCHv2 4/4] unpack-trees: support super-prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This is only patchv4 that is rerolled, patches 1-3 remain as is.

Good timing, as I was about to send a reminder to suggest rerolling
4/4 alone ;-)

> +static const char *super_prefixed(const char *path)
> +{

There used to be a comment that explains why we keep two static
buffers around here.  Even though it is in the log message, the
in-code comment would save people trouble of having to go through
"git blame" output.

I'd say something like

	/*
	 * It is necessary and sufficient to have two static buffers
	 * as the return value of this function is fed to error()
	 * using the unpack_*_errors[] templates we can see above.
	 */

perhaps.

> +	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
> +	static int super_prefix_len = -1;
> +	static unsigned idx = 0;
> +

If we initialize this to 1 (or even better, "ARRAY_SIZE(buf) - 1"),
then we would use buf[0] first and then buf[1], which feels more
natural to me.

Other than that, this looks OK.  Will queue.

Thanks.

> +	if (super_prefix_len < 0) {
> +		if (!get_super_prefix())
> +			super_prefix_len = 0;
> +		else {
> +			int i;
> +
> +			super_prefix_len = strlen(get_super_prefix());
> +			for (i = 0; i < ARRAY_SIZE(buf); i++)
> +				strbuf_addstr(&buf[i], get_super_prefix());
> +		}
> +	}
> +
> +	if (!super_prefix_len)
> +		return path;
> +
> +	if (++idx >= ARRAY_SIZE(buf))
> +		idx = 0;
> +
> +	strbuf_setlen(&buf[idx], super_prefix_len);
> +	strbuf_addstr(&buf[idx], path);
> +
> +	return buf[idx].buf;
> +}
> +
>  void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>  				  const char *cmd)
>  {
> @@ -172,7 +202,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
>  			     const char *path)
>  {
>  	if (!o->show_all_errors)
> -		return error(ERRORMSG(o, e), path);
> +		return error(ERRORMSG(o, e), super_prefixed(path));
>  
>  	/*
>  	 * Otherwise, insert in a list for future display by
> @@ -196,7 +226,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
>  			something_displayed = 1;
>  			for (i = 0; i < rejects->nr; i++)
>  				strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
> -			error(ERRORMSG(o, e), path.buf);
> +			error(ERRORMSG(o, e), super_prefixed(path.buf));
>  			strbuf_release(&path);
>  		}
>  		string_list_clear(rejects, 0);
> @@ -1918,7 +1948,9 @@ int bind_merge(const struct cache_entry * const *src,
>  			     o->merge_size);
>  	if (a && old)
>  		return o->gently ? -1 :
> -			error(ERRORMSG(o, ERROR_BIND_OVERLAP), a->name, old->name);
> +			error(ERRORMSG(o, ERROR_BIND_OVERLAP),
> +			      super_prefixed(a->name),
> +			      super_prefixed(old->name));
>  	if (!a)
>  		return keep_entry(old, o);
>  	else

Re: [PATCHv2 4/4] unpack-trees: support super-prefix option

[Stefan Beller]

On Thu, Jan 12, 2017 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> This is only patchv4 that is rerolled, patches 1-3 remain as is.
>
> Good timing, as I was about to send a reminder to suggest rerolling
> 4/4 alone ;-)
>
>> +static const char *super_prefixed(const char *path)
>> +{
>
> There used to be a comment that explains why we keep two static
> buffers around here.  Even though it is in the log message, the
> in-code comment would save people trouble of having to go through
> "git blame" output.
>
> I'd say something like
>
>         /*
>          * It is necessary and sufficient to have two static buffers
>          * as the return value of this function is fed to error()
>          * using the unpack_*_errors[] templates we can see above.
>          */
>
> perhaps.

If you think it helps, I can reroll with such a comment.
At the time of fixing up for v4 I felt like it is overly verbose.
Such a comment is only useful in understanding the choice of 2.
I thought it was sufficient in the log as once you're interested in
that function you'd read the output of blame anyway.

On the other hand having statically allocated arrays of fixed size is
dangerous in terms of maintainability, i.e. down the road someone
thinks this is a good function to reuse and then they may run into
subtle bugs as they will not be aware of the internally static buffer
that is overwritten after a certain time.

>
>> +     static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
>> +     static int super_prefix_len = -1;
>> +     static unsigned idx = 0;
>> +
>
> If we initialize this to 1 (or even better, "ARRAY_SIZE(buf) - 1"),
> then we would use buf[0] first and then buf[1], which feels more
> natural to me.

Yes I agree, though I overcomplicating things just so that they feel
more natural seems wrong as well.

At the time I assumed that having a static variable initialized to 0
was slightly cheaper, as the init code just memsets all of .bss to 0
unlike the .data segment that has to be crafted manually.
But to get that variable into the .bss section reliably we'd have
to omit the "=0". (My compiler recognized that it can be put into
.bss because it is 0)

>
> Other than that, this looks OK.  Will queue.
>
> Thanks.

Thanks,
Stefan

Re: [RFC/PATCH 0/4] working tree operations: support superprefix

[Brian J. Davis]

Resending original as plain text as git@verger.kernel.org rejected HTML 
encoding as potential virus.  Apologies for dupes in inbox. Hopefully 
this works.

In response to a post at:

https://groups.google.com/forum/#!topic/git-users/BVLcKHhSUKo

I was asked to post here to explain a potential problem with current 
modules implementation.  Apologies if I am posting in the wrong place... 
so good bad or otherwise here goes:

+-------------------------------
With:

git push origin branchname

or

git push server2 branchname

I can push or pull from multiple servers so no problem as git supports this.

Now the problem with use of submodules

submodules are listed:

submodule.directory_to_
checkout/proj1_dir.url=https://git.someserver1/git/proj1_dir 
<https://git.someserver1/git/proj1_dir>

but if say I want to pull from some server 2 and do a

git submodule update --init --recursive

what I would get is from someserver1 not someserver2 as there is no 
"origin" support for submodules.  Is this correct?  If so can origin 
support be added to submodules?
+-------------------------------

So above was post from google group.  Maybe above is enough to explain 
the problem that can result, but if not here is a discussion as to why 
this can be confusing resulting to pushing or pulling from the incorrect 
server.

Lets say projects starts at origin on https://server0.org. Project is 
then brought in house to server https://indhouse.corp by developer A.

Developer A then changes the submodule projects to point to indhouse and 
changes submodules in super project to point to indhouse so everything 
is great.

Then Developer B clones from indhouse makes changes to submodule1 and 
submodule1 and pushes them to indhouse.  Dev A tells Dev B hey thoes 
changes are great why don't you push to public server0 so every one can 
get thoes changes.  Dev B then git push origin branch_name on submodule1 
and push origin on submodule 2 and superproject.  And everything is 
great ... right?

Yes by now those who know git know what dev B  forgot or more to the 
point what git does not support in a "clean" way.  For those who don't 
enter the life of dev C

So dev C clones from server0 and performs a git submodule update --init 
--recursive.  Now Dev C is on the www and can't see indhouse.corp and 
... kerpow... Dev B and Dev C just experienced one of the many SW mines 
on the battlefield.

I ask that git devs first see if I am correct with this assessment as I 
could be wrong... maybe there is a way of doing this... and if not add a 
feature to git to handle submodules and multiple origins cleanly.... and 
yes beating dev B with rubber chicken might be a solution though I am 
looking for a slightly better option.