threads / patch / 29109

patch, 2 partsparse-options: introduce OPT_SUBCOMMAND

Subject: [PATCH 1/2] parse-options: introduce OPT_SUBCOMMAND

## tl;dr

50 messages between Dec 8, 2011 and Dec 15, 2011. Diffs are folded; open one to read it.

replies: 49people: 5as markdown or json

Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · lore

[PATCH v2 0/6] Fix '&&' chaining in tests

Hi,
This follows-up $gmane/186481.  Thanks to Jonathan and Matthieu for
going through it.  Changes are:
1. Changes in response to Jonathan's review.
2. Squash similar patches and re-order.
Thanks.
-- Ram
Ramkumar Ramachandra (6):
  t3040 (subprojects-basic): modernize style
  t3030 (merge-recursive): use test_expect_code
  t1006 (cat-file): use test_cmp
  t3200 (branch): fix '&&' chaining
  t1510 (worktree): fix '&&' chaining
  test: fix '&&' chaining
 t/t1006-cat-file.sh                   |  119 +++++++++++++--------------
 t/t1007-hash-object.sh                |    2 +-
 t/t1013-loose-object-format.sh        |    2 +-
 t/t1300-repo-config.sh                |    2 +-
 t/t1412-reflog-loop.sh                |    2 +-
 t/t1501-worktree.sh                   |    6 +-
 t/t1510-repo-setup.sh                 |    4 +-
 t/t1511-rev-parse-caret.sh            |    2 +-
 t/t3030-merge-recursive.sh            |   72 ++---------------
 t/t3040-subprojects-basic.sh          |  144 ++++++++++++++++----------------
 t/t3200-branch.sh                     |    4 +-
 t/t3310-notes-merge-manual-resolve.sh |   10 +-
 t/t3400-rebase.sh                     |    4 +-
 t/t3418-rebase-continue.sh            |    4 +-
 t/t3419-rebase-patch-id.sh            |    2 +-
 15 files changed, 156 insertions(+), 223 deletions(-)
-- 
1.7.7.3
Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · re: Ramkumar Ramachandra · lore

Several git programs take long dash-less options on the command-line to indicate different modes of operation like:

  git stash show
  git bundle verify test.bundle
  git bisect start

Currently, the parse-options framework forbids the use of opts->long_name and OPT_PARSE_NODASH, and the parsing has to be done by hand as a result. Lift this restriction, and create a new OPT_SUBCOMMAND; this is built on top of OPTION_BIT to allow for the detection of more than one subcommand.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 parse-options.c          |    5 +++--
 parse-options.h          |    3 +++
 t/t0040-parse-options.sh |   31 +++++++++++++++++++++++++++++++
 test-parse-options.c     |    4 ++++
 4 files changed, 41 insertions(+), 2 deletions(-)
Show changes to 4 files +41 −2

parse-options.c, parse-options.h, t/t0040-parse-options.sh, test-parse-options.c

diff --git a/parse-options.c b/parse-options.c
index f0098eb..079616a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -278,6 +278,8 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,
 			continue;
 		if (options->short_name == arg[0] && arg[1] == '\0')
 			return get_value(p, options, OPT_SHORT);
+		if (options->long_name && !strcmp(options->long_name, arg))
+			return get_value(p, options, OPT_SHORT);
 	}
 	return -2;
 }
@@ -314,8 +316,7 @@ static void parse_options_check(const struct option *opts)
 		if (opts->flags & PARSE_OPT_NODASH &&
 		    ((opts->flags & PARSE_OPT_OPTARG) ||
 		     !(opts->flags & PARSE_OPT_NOARG) ||
-		     !(opts->flags & PARSE_OPT_NONEG) ||
-		     opts->long_name))
+		     !(opts->flags & PARSE_OPT_NONEG)))
 			err |= optbug(opts, "uses feature "
 					"not supported for dashless options");
 		switch (opts->type) {
diff --git a/parse-options.h b/parse-options.h
index 2e811dc..9267d46 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -123,6 +123,9 @@ struct option {
 #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
 #define OPT_BIT(s, l, v, h, b)      { OPTION_BIT, (s), (l), (v), NULL, (h), \
 				      PARSE_OPT_NOARG, NULL, (b) }
+#define OPT_SUBCOMMAND(l, v, h, i)  { OPTION_BIT, 0, (l), (v), NULL, (h), \
+				      PARSE_OPT_NONEG | PARSE_OPT_NOARG | \
+				      PARSE_OPT_NODASH | PARSE_OPT_HIDDEN, NULL, (i) }
 #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
 #define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index a1e4616..47a402e 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -55,6 +55,7 @@ mv expect expect.err
 
 cat > expect << EOF
 boolean: 2
+subcommand: 0
 integer: 1729
 timestamp: 0
 string: 123
@@ -74,6 +75,7 @@ test_expect_success 'short options' '
 
 cat > expect << EOF
 boolean: 2
+subcommand: 0
 integer: 1729
 timestamp: 0
 string: 321
@@ -103,6 +105,7 @@ test_expect_success 'missing required value' '
 
 cat > expect << EOF
 boolean: 1
+subcommand: 0
 integer: 13
 timestamp: 0
 string: 123
@@ -125,6 +128,7 @@ test_expect_success 'intermingled arguments' '
 
 cat > expect << EOF
 boolean: 0
+subcommand: 0
 integer: 2
 timestamp: 0
 string: (not set)
@@ -154,6 +158,7 @@ test_expect_success 'ambiguously abbreviated option' '
 
 cat > expect << EOF
 boolean: 0
+subcommand: 0
 integer: 0
 timestamp: 0
 string: 123
@@ -182,6 +187,7 @@ test_expect_success 'detect possible typos' '
 
 cat > expect <<EOF
 boolean: 0
+subcommand: 0
 integer: 0
 timestamp: 0
 string: (not set)
@@ -201,6 +207,7 @@ test_expect_success 'keep some options as arguments' '
 
 cat > expect <<EOF
 boolean: 0
+subcommand: 0
 integer: 0
 timestamp: 1
 string: default
@@ -222,6 +229,7 @@ test_expect_success 'OPT_DATE() and OPT_SET_PTR() work' '
 cat > expect <<EOF
 Callback: "four", 0
 boolean: 5
+subcommand: 0
 integer: 4
 timestamp: 0
 string: (not set)
@@ -250,6 +258,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
 
 cat > expect <<EOF
 boolean: 1
+subcommand: 0
 integer: 23
 timestamp: 0
 string: (not set)
@@ -274,6 +283,7 @@ test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '
 
 cat > expect <<EOF
 boolean: 6
+subcommand: 0
 integer: 0
 timestamp: 0
 string: (not set)
@@ -304,6 +314,26 @@ test_expect_success 'OPT_BOOLEAN() with PARSE_OPT_NODASH works' '
 
 cat > expect <<EOF
 boolean: 0
+subcommand: 4
+integer: 0
+timestamp: 0
+string: (not set)
+abbrev: 7
+verbose: 0
+quiet: no
+dry run: no
+file: (not set)
+EOF
+
+test_expect_success 'OPT_SUBCOMMAND() works' '
+	test-parse-options sub4 > output 2> output.err &&
+	test ! -s output.err &&
+	test_cmp expect output
+'
+
+cat > expect <<EOF
+boolean: 0
+subcommand: 0
 integer: 12345
 timestamp: 0
 string: (not set)
@@ -322,6 +352,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
 
 cat >expect <<EOF
 boolean: 0
+subcommand: 0
 integer: 0
 timestamp: 0
 string: (not set)
diff --git a/test-parse-options.c b/test-parse-options.c
index 36487c4..8d5fcd4 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -3,6 +3,7 @@
 #include "string-list.h"
 
 static int boolean = 0;
+static int subcommand = 0;
 static int integer = 0;
 static unsigned long timestamp;
 static int abbrev = 7;
@@ -40,6 +41,8 @@ int main(int argc, const char **argv)
 		OPT_BOOLEAN('b', "boolean", &boolean, "get a boolean"),
 		OPT_BIT('4', "or4", &boolean,
 			"bitwise-or boolean with ...0100", 4),
+		OPT_SUBCOMMAND("sub4", &subcommand,
+			"bitwise-or subcommand with ...0100", 4),
 		OPT_NEGBIT(0, "neg-or4", &boolean, "same as --no-or4", 4),
 		OPT_GROUP(""),
 		OPT_INTEGER('i', "integer", &integer, "get a integer"),
@@ -80,6 +83,7 @@ int main(int argc, const char **argv)
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	printf("boolean: %d\n", boolean);
+	printf("subcommand: %d\n", subcommand);
 	printf("integer: %u\n", integer);
 	printf("timestamp: %lu\n", timestamp);
 	printf("string: %s\n", string ? string : "(not set)");
-- 
1.7.7.3
Jonathan Nieder· Dec 8, 2011, 16:30 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 1/2] parse-options: introduce OPT_SUBCOMMAND

Hi,
Quick thoughts:
Ramkumar Ramachandra wrote:
> Currently, the parse-options framework forbids the use of
> opts->long_name and OPT_PARSE_NODASH, and the parsing has to be done
> by hand as a result.  Lift this restriction
This part seems like a sane idea to me.
> , and create a new
> OPT_SUBCOMMAND; this is built on top of OPTION_BIT to allow for the
> detection of more than one subcommand.

This part I am not convinced about. Usually each subcommand takes its own options, so I cannot see this OPT_SUBCOMMAND being actually useful for commands like "git stash" or "git remote".

Hope that helps, Jonathan

Ramkumar Ramachandra· Dec 8, 2011, 16:43 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2] parse-options: introduce OPT_SUBCOMMAND

Hi,
Jonathan Nieder wrote:
Show 8 quoted lines
> Ramkumar Ramachandra wrote:
>> , and create a new
>> OPT_SUBCOMMAND; this is built on top of OPTION_BIT to allow for the
>> detection of more than one subcommand.
>
> This part I am not convinced about.  Usually each subcommand takes its
> own options, so I cannot see this OPT_SUBCOMMAND being actually useful
> for commands like "git stash" or "git remote".

Hm, what difference does that make? We still have to parse the subcommand, and subsequently use an if-else construct to parse more options depending on the subcommand, no?

-- Ram
Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · re: Ramkumar Ramachandra · lore

[PATCH 1/6] t3040 (subprojects-basic): modernize style

Put the opening quote starting each test on the same line as the test_expect_* invocation. While at it:

- Indent the file with tabs, not spaces.
- Guard commands that prepare test input for individual tests in the
  same test_expect_success, so that their scope is clearer and errors
  at that stage can be caught.
- Use <<-\EOF in preference to <<EOF to save readers the trouble of
  looking for variable interpolations.
- Include "setup" in the titles of test assertions that prepare for
  later ones to make it more obvious which tests can be skipped.
- Chain commands with &&.  Breaks in a test assertion's && chain can
  potentially hide failures from earlier commands in the chain.
- Use test_expect_code() in preference to checking the exit status of
  various statements by hand.
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3040-subprojects-basic.sh |  144 +++++++++++++++++++++---------------------
 1 files changed, 72 insertions(+), 72 deletions(-)
Show changes to t/t3040-subprojects-basic.sh +72 −72
diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh
index f6973e9..0a4ff6d 100755
--- a/t/t3040-subprojects-basic.sh
+++ b/t/t3040-subprojects-basic.sh
@@ -3,81 +3,81 @@
 test_description='Basic subproject functionality'
 . ./test-lib.sh
 
-test_expect_success 'Super project creation' \
-    ': >Makefile &&
-    git add Makefile &&
-    git commit -m "Superproject created"'
-
-
-cat >expected <<EOF
-:000000 160000 00000... A	sub1
-:000000 160000 00000... A	sub2
-EOF
-test_expect_success 'create subprojects' \
-    'mkdir sub1 &&
-    ( cd sub1 && git init && : >Makefile && git add * &&
-    git commit -q -m "subproject 1" ) &&
-    mkdir sub2 &&
-    ( cd sub2 && git init && : >Makefile && git add * &&
-    git commit -q -m "subproject 2" ) &&
-    git update-index --add sub1 &&
-    git add sub2 &&
-    git commit -q -m "subprojects added" &&
-    git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
-    test_cmp expected current'
-
-git branch save HEAD
-
-test_expect_success 'check if fsck ignores the subprojects' \
-    'git fsck --full'
-
-test_expect_success 'check if commit in a subproject detected' \
-    '( cd sub1 &&
-    echo "all:" >>Makefile &&
-    echo "	true" >>Makefile &&
-    git commit -q -a -m "make all" ) && {
-        git diff-files --exit-code
-	test $? = 1
-    }'
-
-test_expect_success 'check if a changed subproject HEAD can be committed' \
-    'git commit -q -a -m "sub1 changed" && {
-	git diff-tree --exit-code HEAD^ HEAD
-	test $? = 1
-    }'
-
-test_expect_success 'check if diff-index works for subproject elements' \
-    'git diff-index --exit-code --cached save -- sub1
-    test $? = 1'
-
-test_expect_success 'check if diff-tree works for subproject elements' \
-    'git diff-tree --exit-code HEAD^ HEAD -- sub1
-    test $? = 1'
-
-test_expect_success 'check if git diff works for subproject elements' \
-    'git diff --exit-code HEAD^ HEAD
-    test $? = 1'
-
-test_expect_success 'check if clone works' \
-    'git ls-files -s >expected &&
-    git clone -l -s . cloned &&
-    ( cd cloned && git ls-files -s ) >current &&
-    test_cmp expected current'
-
-test_expect_success 'removing and adding subproject' \
-    'git update-index --force-remove -- sub2 &&
-    mv sub2 sub3 &&
-    git add sub3 &&
-    git commit -q -m "renaming a subproject" && {
-	git diff -M --name-status --exit-code HEAD^ HEAD
-	test $? = 1
-    }'
+test_expect_success 'setup: create superproject' '
+	: >Makefile &&
+	git add Makefile &&
+	git commit -m "Superproject created"
+'
+
+test_expect_success 'setup: create subprojects' '
+	mkdir sub1 &&
+	( cd sub1 && git init && : >Makefile && git add * &&
+	git commit -q -m "subproject 1" ) &&
+	mkdir sub2 &&
+	( cd sub2 && git init && : >Makefile && git add * &&
+	git commit -q -m "subproject 2" ) &&
+	git update-index --add sub1 &&
+	git add sub2 &&
+	git commit -q -m "subprojects added" &&
+	git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
+	git branch save HEAD &&
+	cat >expected <<-\EOF &&
+	:000000 160000 00000... A	sub1
+	:000000 160000 00000... A	sub2
+	EOF
+	test_cmp expected current
+'
+
+test_expect_success 'check if fsck ignores the subprojects' '
+	git fsck --full
+'
+
+test_expect_success 'check if commit in a subproject detected' '
+	( cd sub1 &&
+	echo "all:" >>Makefile &&
+	echo "	true" >>Makefile &&
+	git commit -q -a -m "make all" ) &&
+	test_expect_code 1 git diff-files --exit-code
+'
+
+test_expect_success 'check if a changed subproject HEAD can be committed' '
+	git commit -q -a -m "sub1 changed" &&
+	test_expect_code 1 git diff-tree --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'check if diff-index works for subproject elements' '
+	test_expect_code 1 git diff-index --exit-code --cached save -- sub1
+'
+
+test_expect_success 'check if diff-tree works for subproject elements' '
+	test_expect_code 1 git diff-tree --exit-code HEAD^ HEAD -- sub1
+'
+
+test_expect_success 'check if git diff works for subproject elements' '
+	test_expect_code 1 git diff --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'check if clone works' '
+	git ls-files -s >expected &&
+	git clone -l -s . cloned &&
+	( cd cloned && git ls-files -s ) >current &&
+	test_cmp expected current
+'
+
+test_expect_success 'removing and adding subproject' '
+	git update-index --force-remove -- sub2 &&
+	mv sub2 sub3 &&
+	git add sub3 &&
+	git commit -q -m "renaming a subproject" &&
+	test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD
+'
 
 # the index must contain the object name the HEAD of the
 # subproject sub1 was at the point "save"
-test_expect_success 'checkout in superproject' \
-    'git checkout save &&
-    git diff-index --exit-code --raw --cached save -- sub1'
+test_expect_success 'checkout in superproject' '
+	git checkout save &&
+	git diff-index --exit-code --raw --cached save -- sub1
+'
 
 # just interesting what happened...
 # git diff --name-status -M save master
-- 
1.7.7.3
Jonathan Nieder· Dec 8, 2011, 16:34 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style

Ramkumar Ramachandra wrote:
> Put the opening quote starting each test on the same line as the
> test_expect_* invocation.  While at it:

I suspect the above description, while it does describe your patch, does not describe the _reason_ that the patch exists or that someone would want to apply it. Isn't it something more like:

	Make the following changes pertaining to &&-chaining, for some
	good reason that I will describe:
	- ...
	While at it, clean up the style to fit the prevailing style.
	That means:
	- Put the opening quote starting each test on the same line as
	...
I didn't read over the patch again.  Has it changed since v1?
Ramkumar Ramachandra· Dec 9, 2011, 07:56 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style

Hi,
Jonathan Nieder wrote:
Show 9 quoted lines
> Ramkumar Ramachandra wrote:
>
>> Put the opening quote starting each test on the same line as the
>> test_expect_* invocation.  While at it:
>
> I suspect the above description, while it does describe your patch,
> does not describe the _reason_ that the patch exists or that someone
> would want to apply it.  Isn't it something more like:
> [...]
Right, fixed.
> I didn't read over the patch again.  Has it changed since v1?
No.  I refrained from making other style changes and/ or combining tests.
-- Ram
Junio C Hamano· Dec 9, 2011, 18:26 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 6 quoted lines
>> I suspect the above description, while it does describe your patch,
>> does not describe the _reason_ that the patch exists or that someone
>> would want to apply it.  Isn't it something more like:
>> [...]
>
> Right, fixed.

Does the "fixed" mean "I fixed it locally; please expect to see it in the next re-roll?"

Just a quick question, not asking you to always spell it that way.
Ramkumar Ramachandra· Dec 9, 2011, 18:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style

Hi Junio,
Junio C Hamano wrote:
Show 6 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>> Right, fixed.
>
> Does the "fixed" mean "I fixed it locally; please expect to see it in the
> next re-roll?"

Yes, it means exactly that :) I respond/ archive the review emails as soon as I fix the corresponding issue locally -- it's the only way to make sure that I don't miss anything (although I miss a few comments despite that).

-- Ram
Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/2] bundle: rewrite builtin to use parse-options

The git-bundle builtin currently parses command-line options by hand; this is both fragile and cryptic on failure. Since we now have an OPT_SUBCOMMAND, make use of it to parse the correct subcommand, while forbidding the use of more than one subcommand in the same invocation.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/bundle.c |  111 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 73 insertions(+), 38 deletions(-)
Show changes to builtin/bundle.c +73 −38
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 92a8a60..c977d9f 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "bundle.h"
 
 /*
@@ -9,57 +10,91 @@
  * bundle supporting "fetch", "pull", and "ls-remote".
  */
 
-static const char builtin_bundle_usage[] =
-  "git bundle create <file> <git-rev-list args>\n"
-  "   or: git bundle verify <file>\n"
-  "   or: git bundle list-heads <file> [<refname>...]\n"
-  "   or: git bundle unbundle <file> [<refname>...]";
+static const char * builtin_bundle_usage[] = {
+	"git bundle create <file> <git-rev-list args>",
+	"git bundle verify <file>",
+	"git bundle list-heads <file> [<refname>...]",
+	"git bundle unbundle <file> [<refname>...]",
+	NULL
+};
+
+enum bundle_subcommand {
+	BUNDLE_NONE = 0,
+	BUNDLE_CREATE = 1,
+	BUNDLE_VERIFY = 2,
+	BUNDLE_LIST_HEADS = 4,
+	BUNDLE_UNBUNDLE = 8
+};
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
-	struct bundle_header header;
-	const char *cmd, *bundle_file;
+	int prefix_length;
 	int bundle_fd = -1;
-	char buffer[PATH_MAX];
+	const char *bundle_file;
+	struct bundle_header header;
+	enum bundle_subcommand subcommand = BUNDLE_NONE;
 
-	if (argc < 3)
-		usage(builtin_bundle_usage);
+	struct option options[] = {
+		OPT_SUBCOMMAND("create", &subcommand,
+			"create a new bundle",
+			BUNDLE_CREATE),
+		OPT_SUBCOMMAND("verify", &subcommand,
+			"verify clean application of the bundle",
+			BUNDLE_VERIFY),
+		OPT_SUBCOMMAND("list-heads", &subcommand,
+			"list references defined in the bundle",
+			BUNDLE_LIST_HEADS),
+		OPT_SUBCOMMAND("unbundle", &subcommand,
+			"pass objects in the bundle to 'git index-pack'",
+			BUNDLE_UNBUNDLE),
+		OPT_END(),
+	};
 
-	cmd = argv[1];
-	bundle_file = argv[2];
-	argc -= 2;
-	argv += 2;
+	argc = parse_options(argc, argv, NULL,
+			options, builtin_bundle_usage,
+			PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
 
-	if (prefix && bundle_file[0] != '/') {
-		snprintf(buffer, sizeof(buffer), "%s/%s", prefix, bundle_file);
-		bundle_file = buffer;
-	}
+	if (argc < 2)
+		usage_with_options(builtin_bundle_usage, options);
 
-	memset(&header, 0, sizeof(header));
-	if (strcmp(cmd, "create") && (bundle_fd =
-				read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	/* The next parameter on the command line is bundle_file */
+	prefix_length = prefix ? strlen(prefix) : 0;
+	bundle_file = prefix_filename(prefix, prefix_length, argv[1]);
+	argc -= 1;
+	argv += 1;
 
-	if (!strcmp(cmd, "verify")) {
+	/* Read out bundle header, except in BUNDLE_CREATE case */
+	if (subcommand == BUNDLE_VERIFY || subcommand == BUNDLE_LIST_HEADS ||
+		subcommand == BUNDLE_UNBUNDLE) {
+		memset(&header, 0, sizeof(header));
+		bundle_fd = read_bundle_header(bundle_file, &header);
+		if (bundle_fd < 0)
+			die_errno(_("Failed to open bundle file '%s'"), bundle_file);
+	}
+
+	switch (subcommand) {
+	case BUNDLE_CREATE:
+		if (!startup_info->have_repository)
+			die(_("Need a repository to create a bundle."));
+		return create_bundle(&header, bundle_file, argc, argv);
+	case BUNDLE_VERIFY:
 		close(bundle_fd);
 		if (verify_bundle(&header, 1))
-			return 1;
+			return -1; /* Error already reported */
 		fprintf(stderr, _("%s is okay\n"), bundle_file);
-		return 0;
-	}
-	if (!strcmp(cmd, "list-heads")) {
+		break;
+	case BUNDLE_LIST_HEADS:
 		close(bundle_fd);
-		return !!list_bundle_refs(&header, argc, argv);
-	}
-	if (!strcmp(cmd, "create")) {
-		if (!startup_info->have_repository)
-			die(_("Need a repository to create a bundle."));
-		return !!create_bundle(&header, bundle_file, argc, argv);
-	} else if (!strcmp(cmd, "unbundle")) {
-		if (!startup_info->have_repository)
+		return list_bundle_refs(&header, argc, argv);
+	case BUNDLE_UNBUNDLE:
+		if (!startup_info->have_repository) {
+			close(bundle_fd);
 			die(_("Need a repository to unbundle."));
-		return !!unbundle(&header, bundle_fd, 0) ||
+		}
+		return unbundle(&header, bundle_fd, 0) ||
 			list_bundle_refs(&header, argc, argv);
-	} else
-		usage(builtin_bundle_usage);
+	default:
+		usage_with_options(builtin_bundle_usage, options);
+	}
+	return 0;
 }
-- 
1.7.7.3
Jonathan Nieder· Dec 8, 2011, 16:39 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Ramkumar Ramachandra wrote:
Show 10 quoted lines
> -	if (argc < 3)
> -		usage(builtin_bundle_usage);
> -
> -	cmd = argv[1];
> -	bundle_file = argv[2];
> -	argc -= 2;
> -	argv += 2;
> +	argc = parse_options(argc, argv, NULL,
> +			options, builtin_bundle_usage,
> +			PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);

Doesn't this make the usage completely confusing? Before, if I wanted to create bundle named "verify", I could write

	git bundle create verify

Afterwards, not only does that not work, but if I make a typo and write

	git bundle ceate verify
then it will act like "git bundle verify ceate".

I am starting to suspect the first half of patch 1/2 was not such a great idea, either. :) Do you have other examples of how it would be used?

Thanks for thining about these things, and hope that helps. Jonathan

Ramkumar Ramachandra· Dec 8, 2011, 16:47 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Hi,
Jonathan Nieder wrote:
Show 24 quoted lines
> Ramkumar Ramachandra wrote:
>
>> -     if (argc < 3)
>> -             usage(builtin_bundle_usage);
>> -
>> -     cmd = argv[1];
>> -     bundle_file = argv[2];
>> -     argc -= 2;
>> -     argv += 2;
>> +     argc = parse_options(argc, argv, NULL,
>> +                     options, builtin_bundle_usage,
>> +                     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
>
> Doesn't this make the usage completely confusing?  Before, if I wanted
> to create bundle named "verify", I could write
>
>        git bundle create verify
>
> Afterwards, not only does that not work, but if I make a typo and
> write
>
>        git bundle ceate verify
>
> then it will act like "git bundle verify ceate".

No it won't -- it'll just print out the usage because "verify" and "create" are mutually exclusive options. Sure, you can't create a bundle named "verify", but that's the compromise you'll have to make if you don't want to type out "--" with each option, no?

> I am starting to suspect the first half of patch 1/2 was not such a
> great idea, either. :)  Do you have other examples of how it would be
> used?

Hehe, my lack of foresight is disturbing as usual. I'm not yet sure it'll be useful.

-- Ram
Jonathan Nieder· Dec 8, 2011, 17:03 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Ramkumar Ramachandra wrote:
> Sure, you can't create a
> bundle named "verify", but that's the compromise you'll have to make
> if you don't want to type out "--" with each option, no?
No, that's not a compromise I'll have to make.  I'm not making it today.

Having to type "--" or prefix with "./" to escape ordinary filenames that do not start with "-" would be completely weird. This is important to me: I want you to see it, rather than relying on me as an authority figure to say it (after all, I'm not such a great authority anyway --- I make plenty of mistakes).

Ramkumar Ramachandra· Dec 8, 2011, 17:38 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Hi,
Jonathan Nieder wrote:
Show 8 quoted lines
> Ramkumar Ramachandra wrote:
>
>> Sure, you can't create a
>> bundle named "verify", but that's the compromise you'll have to make
>> if you don't want to type out "--" with each option, no?
>
> Having to type "--" or prefix with "./" to escape ordinary filenames
> that do not start with "-" would be completely weird.

Hm, do you have any suggestions to work around this? Can we use something like a parse-stopper after the the first subcommand is encountered, and treat the next argument as a non-subcommand (filename or whatever else)?

-- Ram
Jonathan Nieder· Dec 8, 2011, 17:59 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
Show 7 quoted lines
>> Having to type "--" or prefix with "./" to escape ordinary filenames
>> that do not start with "-" would be completely weird.
>
> Hm, do you have any suggestions to work around this?  Can we use
> something like a parse-stopper after the the first subcommand is
> encountered, and treat the next argument as a non-subcommand (filename
> or whatever else)?
What's the desired behavior?  Then we can talk about how to implement it.

If the goal is "use parse-options for a command that has subcommands", see builtin/notes.c.

Ramkumar Ramachandra· Dec 8, 2011, 19:39 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Hi,
Jonathan Nieder wrote:
> What's the desired behavior?  Then we can talk about how to implement it.
>
> If the goal is "use parse-options for a command that has subcommands",
> see builtin/notes.c.

Uses strcmp() to match argv[0]. And you can't specify the options for a certain subcommand before the subcommand itself on the command-line, although I don't consider this a serious limitation. I was going for something prettier with subcommand-specific help text, albeit a serious limitation. I'll try working towards this for a few more hours to see if anything useful comes out of it -- otherwise, I'll just drop this patch and focus on eliminating the ugliness in builtin/revert.c around '--continue', '--quit' parsing.

That being said, do you see value in lifting the restriction on opts->long_name and PARSE_OPTS_NODASH not allowed together? The restriction seems quite arbitrary, but I can't justify lifting it unless I can show some valid usecase.

Thanks.
-- Ram
Junio C Hamano· Dec 8, 2011, 23:48 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> That being said, do you see value in lifting the restriction on
> opts->long_name and PARSE_OPTS_NODASH not allowed together?  The
> restriction seems quite arbitrary, but I can't justify lifting it
> unless I can show some valid usecase.
True and true.

As to the first "true", it is because the nodash was introduced only to parse "find ... \( ... \)" style parentheses as if they are part of option sequence, and that use case only needed a single letter.

As to the second "true", it is because so far we didn't need anything longer.

I do not think the name of a subcommand is not a good use case example for it, by the way. Unlike parentheses on the command line of "find" that can come anywhere and there can be more than one, the subcommand must be the first thing on the command line and only one subcommand is given at one command invocation.

Jakub Narebski· Dec 9, 2011, 13:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
> > That being said, do you see value in lifting the restriction on
> > opts->long_name and PARSE_OPTS_NODASH not allowed together?  The
> > restriction seems quite arbitrary, but I can't justify lifting it
> > unless I can show some valid usecase.
> 
> True and true.
> 
> As to the first "true", it is because the nodash was introduced only to
> parse "find ... \( ... \)" style parentheses as if they are part of option
> sequence, and that use case only needed a single letter.
> 
> As to the second "true", it is because so far we didn't need anything
> longer.
> 
> I do not think the name of a subcommand is not a good use case example for
> it, by the way. Unlike parentheses on the command line of "find" that can
> come anywhere and there can be more than one, the subcommand must be the
> first thing on the command line and only one subcommand is given at one
> command invocation.

Well, I think it doesn't have to be true: there can be some options like e.g. '-n' / '--dry-run' that are common to all subcommands, and in my opinion they could come before subcommand name.

But if restriction that subcommand name must be first simplifies code, then let's do it this way.

I agree that subcommands are and must be mutually exclusive -- otherwise they better be implemented as options, not subcommands.

-- 
Jakub Narębski
Junio C Hamano· Dec 9, 2011, 18:24 UTC · re: Jakub Narebski · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Jakub Narebski <jnareb@gmail.com> writes:
> Well, I think it doesn't have to be true: there can be some options
> like e.g. '-n' / '--dry-run' that are common to all subcommands, and
> in my opinion they could come before subcommand name.
That is just a crazy talk.

If you have three subcommands a, b and c, and only a and b shares -n, c must learn to reject the option that does not apply. If you have more subcommands and you need to add an option to one of them, you are forcing logic to reject that new option to all other subcommands.

That is not a proper way to share option specification. Different subcommands support different options, and if you want to share some options among them, you add a new facility to _allow_ them to share _some_ options, while still allowing them to keep their own option specification table. Otherwise the resulting mess will be unmaintainable.

Ramkumar Ramachandra· Dec 15, 2011, 16:45 UTC · re: Jonathan Nieder · lore

[PATCH 0/2] Improve git-bundle builtin

Hi,

My OPT_SUBCOMMAND idea crashed and burned; I decided to salvage some of the work that went into improving the git-bundle builtin and put it into another series along with some additional tests. I hope this benefits people who use git-bundle to do incremental backups on their servers or otherwise.

A couple of thoughts:
1. There's a SP between the OBJID and the ref name in list-heads as
opposed to the TAB used by other git commands such as ls-remote,
diff-tree.  Will fixing it break someone's parser somewhere?
2. Is it worth fixing the "--stdin" tests?  What is the usecase?  A
quick blame points to f62e0a39 (t5704 (bundle): add tests for bundle
--stdin, 2010-04-19): Jonathan, Joey?
Cheers.
-- Ram
Ramkumar Ramachandra (2):
  t5704 (bundle): rewrite for larger coverage
  bundle: rewrite builtin to use parse-options
 builtin/bundle.c  |   91 +++++++++++++++++++++++++++++---------------------
 t/t5704-bundle.sh |   95 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 122 insertions(+), 64 deletions(-)
-- 
1.7.4.1
Ramkumar Ramachandra· Dec 15, 2011, 16:45 UTC · re: Ramkumar Ramachandra · lore

[PATCH 1/2] t5704 (bundle): rewrite for larger coverage

Rewrite the git-bundle testsuite to exercise more of its functionality.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5704-bundle.sh |   95 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 69 insertions(+), 26 deletions(-)
Show changes to t/t5704-bundle.sh +69 −26
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 728ccd8..09ff4f1 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -1,56 +1,99 @@
 #!/bin/sh
 
-test_description='some bundle related tests'
+test_description='Test git-bundle'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
+	git config core.logAllRefUpdates false &&
+	test_commit initial &&
+	git checkout -b branch &&
+	test_commit second &&
+	test_commit third &&
+	git checkout master &&
+	test_commit fourth file
+'
 
-	: > file &&
-	git add file &&
-	test_tick &&
-	git commit -m initial &&
-	test_tick &&
-	git tag -m tag tag &&
-	: > file2 &&
-	git add file2 &&
-	: > file3 &&
-	test_tick &&
-	git commit -m second &&
-	git add file3 &&
-	test_tick &&
-	git commit -m third
+test_expect_success 'create succeeds' '
+	git bundle create bundle second third &&
+	cat >expect <<-\EOF &&
+	OBJID	refs/tags/third
+	OBJID	refs/tags/second
+	EOF
+	{
+		git ls-remote bundle |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	test_cmp expect actual
+'
 
+test_expect_success 'verify succeeds' '
+	git bundle create bundle second third &&
+	git bundle verify bundle
 '
 
-test_expect_success 'tags can be excluded by rev-list options' '
+test_expect_success 'list-heads succeeds' '
+	git bundle create bundle second third &&
+	cat >expect <<-\EOF &&
+	OBJID refs/tags/second
+	OBJID refs/tags/third
+	EOF
+	{
+		git bundle list-heads bundle |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	test_cmp expect actual
+'
 
-	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
-	git ls-remote bundle > output &&
-	! grep tag output
+test_expect_success 'create dies on invalid bundle filename' '
+	mkdir adir &&
+	test_expect_code 128 git bundle create adir --all
+'
 
+test_expect_success 'disallow stray command-line options' '
+	test_must_fail git bundle create --junk bundle second third
 '
 
-test_expect_success 'die if bundle file cannot be created' '
+test_expect_failure 'disallow stray command-line arguments' '
+	git bundle create bundle second third &&
+	test_must_fail git bundle verify bundle junk
+'
 
-	mkdir adir &&
-	test_must_fail git bundle create adir --all
+test_expect_success 'create accepts rev-list options to narrow refs' '
+	git bundle create bundle --all -- file &&
+	cat >expect <<-\EOF &&
+	OBJID	HEAD
+	OBJID	refs/tags/fourth
+	OBJID	refs/heads/master
+	EOF
+	{
+		git ls-remote bundle |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	test_cmp expect actual
+'
 
+test_expect_success 'unbundle succeeds' '
+	saved=$(git rev-parse third) &&
+	git bundle create bundle second third fourth &&
+	git tag -d second third fourth &&
+	git branch -D branch &&
+	git reset --hard initial &&
+	git prune &&
+	test_must_fail git reset --hard $saved &&
+	git bundle unbundle bundle &&
+	git reset --hard $saved
 '
 
 test_expect_failure 'bundle --stdin' '
-
 	echo master | git bundle create stdin-bundle.bdl --stdin &&
 	git ls-remote stdin-bundle.bdl >output &&
 	grep master output
-
 '
 
 test_expect_failure 'bundle --stdin <rev-list options>' '
-
 	echo master | git bundle create hybrid-bundle.bdl --stdin tag &&
 	git ls-remote hybrid-bundle.bdl >output &&
 	grep master output
-
 '
 
 test_done
-- 
1.7.4.1
Jonathan Nieder· Dec 15, 2011, 21:16 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 1/2] t5704 (bundle): rewrite for larger coverage

Hi,
Ramkumar Ramachandra wrote:
> Rewrite

Always a scary word. Very rarely justified, especially when the original and rewritten versions of something are not going to be able to coexist for a period while the bugs are ironed out. It doesn't leave me optimistic.

>         the git-bundle testsuite to exercise more of its
> functionality.

Luckily, this goal suggests that I am going to see some new tests added, without the existing coverage being removed or mangled, so maybe I can ignore the fears awakened by the word "Rewrite". Let's see...

[...]
Show 7 quoted lines
> --- a/t/t5704-bundle.sh
> +++ b/t/t5704-bundle.sh
> @@ -1,56 +1,99 @@
>  #!/bin/sh
>  
> -test_description='some bundle related tests'
> +test_description='Test git-bundle'
Why?
>  . ./test-lib.sh
>  
>  test_expect_success 'setup' '
> +	git config core.logAllRefUpdates false &&
Why?
Show 7 quoted lines
> +	test_commit initial &&
> +	git checkout -b branch &&
> +	test_commit second &&
> +	test_commit third &&
> +	git checkout master &&
> +	test_commit fourth file
> +'

No explicit tags in the setup this time. Now all commits are referred to by tags, which worsens the coverage, since if some future change caused commits not referred to by a tag to be dropped, it would be missed. Paraphrasing

	>file &&
	git add file &&
	test_tick &&
	git commit -m initial &&
	git tag -m initial initial
to
	test_commit initial file

when not preparing to make some other change in the same place and if the original was not too confusing feels like gratuitous churn.

[...]
Show 12 quoted lines
> +test_expect_success 'create succeeds' '
> +	git bundle create bundle second third &&
> +	cat >expect <<-\EOF &&
> +	OBJID	refs/tags/third
> +	OBJID	refs/tags/second
> +	EOF
> +	{
> +		git ls-remote bundle |
> +		sed "s/$_x40/OBJID/g"
> +	} >actual &&
> +	test_cmp expect actual
> +'

A new test. What assertion is it testing? Why censor out the object names when comparing the expected object names to the actual ones, instead of computing the appropriate object names for the expected result? Is this new test useful, or does it cover ground already tested in t5510-fetch.sh?

> +test_expect_success 'verify succeeds' '
> +	git bundle create bundle second third &&
> +	git bundle verify bundle
>  '
A test for "git bundle verify" is a welcome addition.
Show 13 quoted lines
> +test_expect_success 'list-heads succeeds' '
> +	git bundle create bundle second third &&
> +	cat >expect <<-\EOF &&
> +	OBJID refs/tags/second
> +	OBJID refs/tags/third
> +	EOF
> +	{
> +		git bundle list-heads bundle |
> +		sed "s/$_x40/OBJID/g"
> +	} >actual &&
> +	test_cmp expect actual
> +'
> +

Based on 'git grep -e "git bundle list-heads" -- t', there don't seem to be any existing tests for "git bundle list-heads" except for t5510-fetch.sh, but I'm not sure what this adds on top of that one.

Show 5 quoted lines
> -test_expect_success 'tags can be excluded by rev-list options' '
> -
> -	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
> -	git ls-remote bundle > output &&
> -	! grep tag output
Dropped?
> +test_expect_success 'create dies on invalid bundle filename' '
> +	mkdir adir &&
> +	test_expect_code 128 git bundle create adir --all
> +'

How is "invalid bundle filename" a clearer explanation than "bundle file cannot be created"?

>  
> +test_expect_success 'disallow stray command-line options' '
> +	test_must_fail git bundle create --junk bundle second third
>  '

Ok. Might also be useful to check that no "--junk" or "bundle" file is created.

> +test_expect_failure 'disallow stray command-line arguments' '
> +	git bundle create bundle second third &&
> +	test_must_fail git bundle verify bundle junk
> +'

In this case, "stray command-line arguments" actually means "extra arguments to 'verify'", I guess?

What happens if I run "git bundle verify *.bundle" in a directory with multiple bundles? What should happen?

> +test_expect_success 'create accepts rev-list options to narrow refs' '
> +	git bundle create bundle --all -- file &&

I don't understand what "options to narrow refs" means. Does that mean options like --remotes=origin which yield refs from some subset of the ref namespace, unlike --all?

[...]
> +test_expect_success 'unbundle succeeds' '
A test for "git bundle unbundle" is a welcome addition.
[...]
Show 7 quoted lines
>  test_expect_failure 'bundle --stdin' '
> -
>  	echo master | git bundle create stdin-bundle.bdl --stdin &&
>  	git ls-remote stdin-bundle.bdl >output &&
>  	grep master output
> -
>  '

Seems like a gratuitous change to mix into a patch that introduces functional changes.

I found this hard to review, since it doesn't seem very focussed --- it mixes style cleanups, removal of code, and introduction of new code. I'd be way happier to see a new patch that just adds new tests to the script without potentially breaking anything on the way. Then if the style cleanups still seem important to you, they can be reviewed as a separate patch.

Hoping that clarifies a little, Jonathan

Ramkumar Ramachandra· Dec 15, 2011, 16:45 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/2] bundle: rewrite builtin to use parse-options

The git-bundle builtin currently parses command-line options by hand; this is fragile, and reports cryptic errors on failure. Use the parse-options library to do the parsing instead.

Encouraged-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/bundle.c  |   91 +++++++++++++++++++++++++++++++----------------------
 t/t5704-bundle.sh |    2 +-
 2 files changed, 54 insertions(+), 39 deletions(-)
Show changes to 2 files +54 −39

builtin/bundle.c, t/t5704-bundle.sh

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 92a8a60..13ed770 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "bundle.h"
 
 /*
@@ -9,57 +10,71 @@
  * bundle supporting "fetch", "pull", and "ls-remote".
  */
 
-static const char builtin_bundle_usage[] =
-  "git bundle create <file> <git-rev-list args>\n"
-  "   or: git bundle verify <file>\n"
-  "   or: git bundle list-heads <file> [<refname>...]\n"
-  "   or: git bundle unbundle <file> [<refname>...]";
+static const char * builtin_bundle_usage[] = {
+	"git bundle create <file> <git-rev-list args>",
+	"git bundle verify <file>",
+	"git bundle list-heads <file> [<refname>...]",
+	"git bundle unbundle <file> [<refname>...]",
+	NULL
+};
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
-	struct bundle_header header;
-	const char *cmd, *bundle_file;
+	int prefix_length;
 	int bundle_fd = -1;
-	char buffer[PATH_MAX];
+	const char *subcommand, *bundle_file;
+	struct bundle_header header;
+	struct option options[] = { OPT_END() };
 
-	if (argc < 3)
-		usage(builtin_bundle_usage);
+	argc = parse_options(argc, argv, prefix, options,
+			builtin_bundle_usage, PARSE_OPT_STOP_AT_NON_OPTION);
 
-	cmd = argv[1];
-	bundle_file = argv[2];
-	argc -= 2;
-	argv += 2;
+	if (argc < 2)
+		usage_with_options(builtin_bundle_usage, options);
+	subcommand = argv[0];
 
-	if (prefix && bundle_file[0] != '/') {
-		snprintf(buffer, sizeof(buffer), "%s/%s", prefix, bundle_file);
-		bundle_file = buffer;
-	}
+	argc = parse_options(argc, argv, prefix, options,
+			builtin_bundle_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	/* Disallow stray arguments */
+	if ((strcmp(subcommand, "create") && argc > 2) ||
+		(!strcmp(subcommand, "verify") && argc > 1))
+		usage_with_options(builtin_bundle_usage, options);
 
-	memset(&header, 0, sizeof(header));
-	if (strcmp(cmd, "create") && (bundle_fd =
-				read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	prefix_length = prefix ? strlen(prefix) : 0;
+	bundle_file = prefix_filename(prefix, prefix_length, argv[0]);
 
-	if (!strcmp(cmd, "verify")) {
+	/* Read out bundle header, except in the "create" case */
+	if (strcmp(subcommand, "create")) {
+		memset(&header, 0, sizeof(header));
+		bundle_fd = read_bundle_header(bundle_file, &header);
+		if (bundle_fd < 0)
+			die_errno(_("Failed to open bundle file '%s'"), bundle_file);
+	}
+
+	if (!strcmp(subcommand, "create")) {
+		if (!startup_info->have_repository)
+			die(_("Need a repository to create a bundle."));
+		return create_bundle(&header, bundle_file, argc, argv);
+	} else if (!strcmp(subcommand, "verify")) {
 		close(bundle_fd);
 		if (verify_bundle(&header, 1))
-			return 1;
+			return -1; /* Error already reported */
 		fprintf(stderr, _("%s is okay\n"), bundle_file);
-		return 0;
-	}
-	if (!strcmp(cmd, "list-heads")) {
+	} else if (!strcmp(subcommand, "list-heads")) {
 		close(bundle_fd);
-		return !!list_bundle_refs(&header, argc, argv);
-	}
-	if (!strcmp(cmd, "create")) {
-		if (!startup_info->have_repository)
-			die(_("Need a repository to create a bundle."));
-		return !!create_bundle(&header, bundle_file, argc, argv);
-	} else if (!strcmp(cmd, "unbundle")) {
-		if (!startup_info->have_repository)
+		return list_bundle_refs(&header, argc, argv);
+	} else if (!strcmp(subcommand, "unbundle")) {
+		if (!startup_info->have_repository) {
+			close(bundle_fd);
 			die(_("Need a repository to unbundle."));
-		return !!unbundle(&header, bundle_fd, 0) ||
+		}
+		return unbundle(&header, bundle_fd, 0) ||
 			list_bundle_refs(&header, argc, argv);
-	} else
-		usage(builtin_bundle_usage);
+	} else {
+		close(bundle_fd);
+		usage_with_options(builtin_bundle_usage, options);
+	}
+
+	return 0;
 }
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 09ff4f1..8e3f677 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -53,7 +53,7 @@ test_expect_success 'disallow stray command-line options' '
 	test_must_fail git bundle create --junk bundle second third
 '
 
-test_expect_failure 'disallow stray command-line arguments' '
+test_expect_success 'disallow stray command-line arguments' '
 	git bundle create bundle second third &&
 	test_must_fail git bundle verify bundle junk
 '
-- 
1.7.4.1
Jonathan Nieder· Dec 15, 2011, 21:29 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options

Ramkumar Ramachandra wrote:
> The git-bundle builtin currently parses command-line options by hand;
> this is fragile, and reports cryptic errors on failure.  Use the
> parse-options library to do the parsing instead.

I don't understand how this is fragile. I haven't actually run into error messages from "git bundle" I found to be cryptic, but if they are, they surely can be improved locally. Could you give an example or something?

> Encouraged-by: Jonathan Nieder <jrnieder@gmail.com>
No, not encouraged.

But parseoptification does have some nice benefits, so let's see how the patch looks...

[...]
Show 10 quoted lines
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/bundle.c  |   91 +++++++++++++++++++++++++++++++----------------------
>  t/t5704-bundle.sh |    2 +-
>  2 files changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 92a8a60..13ed770 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
[...]
> @@ -9,57 +10,71 @@
[...]
Show 15 quoted lines
>  int cmd_bundle(int argc, const char **argv, const char *prefix)
>  {
> -	struct bundle_header header;
> -	const char *cmd, *bundle_file;
> +	int prefix_length;
>  	int bundle_fd = -1;
> -	char buffer[PATH_MAX];
> -	if (argc < 3)
> -		usage(builtin_bundle_usage);
> +	const char *subcommand, *bundle_file;
> +	struct bundle_header header;
> +	struct option options[] = { OPT_END() };
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			builtin_bundle_usage, PARSE_OPT_STOP_AT_NON_OPTION);

No, just no. Using parse-options with an empty option table is complete overkill for handling the "-h" option. Without a lot more justification, this doesn't make it seem more sane or readable at all.

Stopping here. I wouldn't mind seeing "git bundle" being parseoptified, but not if the result looks like this.

I _do_ think that a systematic option-parsing library that handles subcommands would be something possible and probably useful for git. Its input might include a table with subcommand names, an option table for each, and a function to call when that subcommand is used:

	struct parseopt_subcommand subcmds[] = {
		{ "list", no_options, notes_list },
		{ "add", add_options, notes_add },
		{ "copy", copy_options, notes_copy },
		{ "append", append_options, notes_append },
		{ "edit", no_options, notes_edit },
		{ "show", no_options, notes_show },
		...
	};

Then "git notes -h" might be able to automatically generate a table of synopses, and "gite notes --help-all" might print help for all subcommands. Something like that.

Hope that helps, Jonathan

Jonathan Nieder· Dec 15, 2011, 20:54 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 0/2] Improve git-bundle builtin

Hi Ram,
Ramkumar Ramachandra wrote:
> 1. There's a SP between the OBJID and the ref name in list-heads as
> opposed to the TAB used by other git commands such as ls-remote,
> diff-tree.  Will fixing it break someone's parser somewhere?

I don't know. Would there be any advantage at all to changing the output format of the tool? Bad idea.

If the goal is to avoid confusion, perhaps a note in the documentation would help.

> 2. Is it worth fixing the "--stdin" tests?  What is the usecase?

Is "script that wants to list which revs to bundle, possibly exceeding the command-line length limit" not enough of a use case? Yes, I think it is very much worth fixing.

Thanks for looking at this. Jonathan

Junio C Hamano· Dec 15, 2011, 21:30 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 0/2] Improve git-bundle builtin

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 5 quoted lines
> A couple of thoughts:
>
> 1. There's a SP between the OBJID and the ref name in list-heads as
> opposed to the TAB used by other git commands such as ls-remote,
> diff-tree.  Will fixing it break someone's parser somewhere?

If somebody will get broken when you change something, that change is not "fixing" but just "changing". Why do you even want to "fix" it?

> 2. Is it worth fixing the "--stdin" tests?  What is the usecase?

Having to pack too many refs that would not fit on a command line length limit, which is not an unusual requirement.

Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/6] t3030 (merge-recursive): use test_expect_code

Use test_expect_code in preference to repeatedly checking exit codes by hand.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3030-merge-recursive.sh |   72 ++++----------------------------------------
 1 files changed, 6 insertions(+), 66 deletions(-)
Show changes to t/t3030-merge-recursive.sh +6 −66
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 55ef189..a5e3da7 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -285,17 +285,7 @@ test_expect_success 'merge-recursive simple' '
 	rm -fr [abcd] &&
 	git checkout -f "$c2" &&
 
-	git merge-recursive "$c0" -- "$c2" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c2" "$c1"
 '
 
 test_expect_success 'merge-recursive result' '
@@ -334,17 +324,7 @@ test_expect_success 'merge-recursive remove conflict' '
 	rm -fr [abcd] &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c5"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c5"
 '
 
 test_expect_success 'merge-recursive remove conflict' '
@@ -388,17 +368,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c4"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c4"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
@@ -422,17 +392,7 @@ test_expect_success 'merge-recursive d/f conflict the other way' '
 	git reset --hard &&
 	git checkout -f "$c4" &&
 
-	git merge-recursive "$c0" -- "$c4" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c4" "$c1"
 '
 
 test_expect_success 'merge-recursive d/f conflict result the other way' '
@@ -456,17 +416,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c6"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c6"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
@@ -490,17 +440,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c6" &&
 
-	git merge-recursive "$c0" -- "$c6" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c6" "$c1"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
-- 
1.7.7.3
Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · re: Ramkumar Ramachandra · lore

[PATCH 3/6] t1006 (cat-file): use test_cmp

Use test_cmp in preference to repeatedly comparing command outputs by hand.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1006-cat-file.sh |  119 ++++++++++++++++++++++++---------------------------
 1 files changed, 56 insertions(+), 63 deletions(-)
Show changes to t/t1006-cat-file.sh +56 −63
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index d8b7f2f..0ca6c43 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -36,66 +36,41 @@ $content"
     '
 
     test_expect_success "Type of $type is correct" '
-        test $type = "$(git cat-file -t $sha1)"
+	echo $type >expect &&
+	git cat-file -t $sha1 >actual &&
+	test_cmp expect actual
     '
 
     test_expect_success "Size of $type is correct" '
-        test $size = "$(git cat-file -s $sha1)"
+	echo $size >expect &&
+	git cat-file -s $sha1 >actual &&
+	test_cmp expect actual
     '
 
     test -z "$content" ||
     test_expect_success "Content of $type is correct" '
-	expect="$(maybe_remove_timestamp "$content" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts)"
-
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$content" $no_ts >expect &&
+	maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test_expect_success "Pretty content of $type is correct" '
-	expect="$(maybe_remove_timestamp "$pretty_content" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(git cat-file -p $sha1)" $no_ts)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$pretty_content" $no_ts >expect &&
+	maybe_remove_timestamp "$(git cat-file -p $sha1)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test -z "$content" ||
     test_expect_success "--batch output of $type is correct" '
-	expect="$(maybe_remove_timestamp "$batch_output" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)" $no_ts)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$batch_output" $no_ts >expect &&
+	maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test_expect_success "--batch-check output of $type is correct" '
-	expect="$sha1 $type $size"
-	actual="$(echo_without_newline $sha1 | git cat-file --batch-check)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	echo "$sha1 $type $size" >expect &&
+	echo_without_newline $sha1 | git cat-file --batch-check >actual &&
+	test_cmp expect actual
     '
 }
 
@@ -144,10 +119,13 @@ tag_size=$(strlen "$tag_content")
 
 run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_pretty_content" 1
 
-test_expect_success \
-    "Reach a blob from a tag pointing to it" \
-    "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
+test_expect_success "Reach a blob from a tag pointing to it" '
+    echo_without_newline "$hello_content" >expect &&
+    git cat-file blob "$tag_sha1" >actual &&
+    test_cmp expect actual
+'
 
+test_done
 for batch in batch batch-check
 do
     for opt in t s e p
@@ -175,30 +153,41 @@ do
 done
 
 test_expect_success "--batch-check for a non-existent named object" '
-    test "foobar42 missing
-foobar84 missing" = \
-    "$( ( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)"
+    cat >expect <<\-EOF &&
+foobar42 missing
+foobar84 missing
+EOF
+    $(echo foobar42; echo_without_newline foobar84) \
+    | git cat-file --batch-check >actual &&
+    test_cmp expect actual
 '
 
 test_expect_success "--batch-check for a non-existent hash" '
-    test "0000000000000000000000000000000000000042 missing
-0000000000000000000000000000000000000084 missing" = \
-    "$( ( echo 0000000000000000000000000000000000000042;
-         echo_without_newline 0000000000000000000000000000000000000084; ) \
-       | git cat-file --batch-check)"
+    cat >expect <<\-EOF &&
+0000000000000000000000000000000000000042 missing
+0000000000000000000000000000000000000084 missing
+EOF
+    $(echo 0000000000000000000000000000000000000042;
+    echo_without_newline 0000000000000000000000000000000000000084) \
+    | git cat-file --batch-check >actual &&
+    test_cmp expect actual
 '
 
 test_expect_success "--batch for an existent and a non-existent hash" '
-    test "$tag_sha1 tag $tag_size
+    cat >expect <<\-EOF &&
+tag_sha1 tag $tag_size
 $tag_content
-0000000000000000000000000000000000000000 missing" = \
-    "$( ( echo $tag_sha1;
-         echo_without_newline 0000000000000000000000000000000000000000; ) \
-       | git cat-file --batch)"
+0000000000000000000000000000000000000000 missing
+EOF
+    $(echo $tag_sha1; echo_without_newline 0000000000000000000000000000000000000000) \
+    | git cat-file --batch >actual &&
+    test_cmp expect_actual
 '
 
 test_expect_success "--batch-check for an emtpy line" '
-    test " missing" = "$(echo | git cat-file --batch-check)"
+    echo " missing" >expect &&
+    echo | git cat-file --batch-check >actual &&
+    test_cmp expect actual
 '
 
 batch_input="$hello_sha1
@@ -218,7 +207,10 @@ deadbeef missing
  missing"
 
 test_expect_success '--batch with multiple sha1s gives correct format' '
-	test "$(maybe_remove_timestamp "$batch_output" 1)" = "$(maybe_remove_timestamp "$(echo_without_newline "$batch_input" | git cat-file --batch)" 1)"
+	maybe_remove_timestamp "$batch_output" 1 >expect &&
+	maybe_remove_timestamp "$(echo_without_newline "$batch_input" \
+	| git cat-file --batch)" 1 >actual &&
+	test_cmp expect actual
 '
 
 batch_check_input="$hello_sha1
@@ -237,8 +229,9 @@ deadbeef missing
  missing"
 
 test_expect_success "--batch-check with multiple sha1s gives correct format" '
-    test "$batch_check_output" = \
-    "$(echo_without_newline "$batch_check_input" | git cat-file --batch-check)"
+    echo "$batch_check_output" >expect &&
+    echo_without_newline "$batch_check_input" | git cat-file --batch-check >actual &&
+    test_cmp expect actual
 '
 
 test_done
-- 
1.7.7.3
Matthieu Moy· Dec 8, 2011, 13:28 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 3/6] t1006 (cat-file): use test_cmp

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 8 quoted lines
>  test_expect_success "--batch-check with multiple sha1s gives correct format" '
> -    test "$batch_check_output" = \
> -    "$(echo_without_newline "$batch_check_input" | git cat-file --batch-check)"
> +    echo "$batch_check_output" >expect &&
> +    echo_without_newline "$batch_check_input" | git cat-file
> + --batch-check >actual &&
> +    test_cmp expect actual
>  '
Whitespace damage?
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Ramkumar Ramachandra· Dec 8, 2011, 13:33 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/6] t1006 (cat-file): use test_cmp

Hi Matthieu,
Matthieu Moy wrote:
Show 12 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>>  test_expect_success "--batch-check with multiple sha1s gives correct format" '
>> -    test "$batch_check_output" = \
>> -    "$(echo_without_newline "$batch_check_input" | git cat-file --batch-check)"
>> +    echo "$batch_check_output" >expect &&
>> +    echo_without_newline "$batch_check_input" | git cat-file
>> + --batch-check >actual &&
>> +    test_cmp expect actual
>>  '
>
> Whitespace damage?
Odd.  Email client issue?  Seems to be fine in the original message [1].
[1]: http://article.gmane.org/gmane.comp.version-control.git/186558
-- Ram
Matthieu Moy· Dec 8, 2011, 13:41 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 3/6] t1006 (cat-file): use test_cmp

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 18 quoted lines
> Hi Matthieu,
>
> Matthieu Moy wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>>>  test_expect_success "--batch-check with multiple sha1s gives correct format" '
>>> -    test "$batch_check_output" = \
>>> -    "$(echo_without_newline "$batch_check_input" | git cat-file
>>> --batch-check)"
>>> +    echo "$batch_check_output" >expect &&
>>> +    echo_without_newline "$batch_check_input" | git cat-file
>>> + --batch-check >actual &&
>>> +    test_cmp expect actual
>>>  '
>>
>> Whitespace damage?
>
> Odd.  Email client issue?

It seems so. Weird, Gnus usually doesn't do this sort of things to me. Sorry for the noise.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jonathan Nieder· Dec 8, 2011, 16:55 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 3/6] t1006 (cat-file): use test_cmp

Ramkumar Ramachandra wrote:
> Use test_cmp in preference to repeatedly comparing command outputs by
> hand.
That could mean one of several things.
It could mean:
	1. Use test_cmp instead of open-coding it.
	2. Use test_cmp instead of using our knowledge of the underlying
	   filesystem to retrieve the files from the block device, instead
	   of relying on the perfectly good operating system facilities
	   that could take care of it for us
	3. Use test_cmp instead of calling a human over to compare command
	   outputs by eye, which idiomatically might be described as "by
	   hand".

What I mean is, I actually don't have much of a clue what you mean by "by hand". Usually it means "not automated sufficiently", but I think that is not the entire problem here (since

	test "$expect" = "$actual"
looks no less automatic than
	printf '%s\n' "$expect" >expect &&
	printf '%s\n' "$actual" >actual &&
	test_cmp expect actual
to me).
> 
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
[...]
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -36,66 +36,41 @@ $content"
[...]
Show 14 quoted lines
> -	expect="$(maybe_remove_timestamp "$content" $no_ts)"
> -	actual="$(maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts)"
> -
> -        if test "z$expect" = "z$actual"
> -	then
> -		: happy
> -	else
> -		echo "Oops: expected $expect"
> -		echo "but got $actual"
> -		false
> -        fi
> +	maybe_remove_timestamp "$content" $no_ts >expect &&
> +	maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts >actual &&
> +	test_cmp expect actual

Most of the early part of patch proper looks sane from a quick glance. Wow, the whitespace is a little strange in the original.

[...]
Show 14 quoted lines
> @@ -175,30 +153,41 @@ do
>  done
>  
>  test_expect_success "--batch-check for a non-existent named object" '
> -    test "foobar42 missing
> -foobar84 missing" = \
> -    "$( ( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)"
> +    cat >expect <<\-EOF &&
> +foobar42 missing
> +foobar84 missing
> +EOF
> +    $(echo foobar42; echo_without_newline foobar84) \
> +    | git cat-file --batch-check >actual &&
> +    test_cmp expect actual
Style: the | character goes at the end of the first line (think of it
as a way to save backslashes until they're needed).
How could this $(...) command substitution possibly work?
Later tests have the same problem, so I'm stopping here.

Ciao, Jonathan

Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · re: Ramkumar Ramachandra · lore

[PATCH 4/6] t3200 (branch): fix '&&' chaining

Breaks in a test assertion's && chain can potentially hide failures from earlier commands in the chain. Fix these breaks.

Additionally, note that 'git branch --help' will fail when git manpages aren't already installed: guard the line with a 'test_might_fail'.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3200-branch.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t3200-branch.sh +2 −2
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index bc73c20..95066d0 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -22,7 +22,7 @@ test_expect_success \
 
 test_expect_success \
     'git branch --help should not have created a bogus branch' '
-     git branch --help </dev/null >/dev/null 2>/dev/null;
+     test_might_fail git branch --help </dev/null >/dev/null 2>/dev/null &&
      test_path_is_missing .git/refs/heads/--help
 '
 
@@ -88,7 +88,7 @@ test_expect_success \
 test_expect_success \
     'git branch -m n/n n should work' \
        'git branch -l n/n &&
-        git branch -m n/n n
+        git branch -m n/n n &&
 	test_path_is_file .git/logs/refs/heads/n'
 
 test_expect_success 'git branch -m o/o o should fail when o/p exists' '
-- 
1.7.7.3
Jonathan Nieder· Dec 8, 2011, 17:07 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 4/6] t3200 (branch): fix '&&' chaining

Ramkumar Ramachandra wrote:
> Breaks in a test assertion's && chain can potentially hide failures
> from earlier commands in the chain.  Fix these breaks.
>
> Additionally, note that 'git branch --help' will fail when git

This is not "Additionally, while we're here" but rather "In order to do so".

Show 7 quoted lines
> manpages aren't already installed: guard the line with a
> 'test_might_fail'.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t3200-branch.sh |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
For what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · re: Ramkumar Ramachandra · lore

[PATCH 5/6] t1510 (worktree): fix '&&' chaining

Breaks in a test assertion's && chain can potentially hide failures from earlier commands in the chain. Fix these breaks.

Additionally, note that 'unset' returns non-zero status when the variable passed was already unset on some shells: change these instances to 'safe_unset'.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1501-worktree.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to t/t1501-worktree.sh +3 −3
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 6384983..8e04e7c 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -48,7 +48,7 @@ test_expect_success 'setup: helper for testing rev-parse' '
 '
 
 test_expect_success 'setup: core.worktree = relative path' '
-	unset GIT_WORK_TREE;
+	safe_unset GIT_WORK_TREE &&
 	GIT_DIR=repo.git &&
 	GIT_CONFIG="$(pwd)"/$GIT_DIR/config &&
 	export GIT_DIR GIT_CONFIG &&
@@ -89,7 +89,7 @@ test_expect_success 'subdir of work tree' '
 '
 
 test_expect_success 'setup: core.worktree = absolute path' '
-	unset GIT_WORK_TREE;
+	safe_unset GIT_WORK_TREE &&
 	GIT_DIR=$(pwd)/repo.git &&
 	GIT_CONFIG=$GIT_DIR/config &&
 	export GIT_DIR GIT_CONFIG &&
@@ -334,7 +334,7 @@ test_expect_success 'absolute pathspec should fail gracefully' '
 '
 
 test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
-	>dummy_file
+	>dummy_file &&
 	echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file &&
 	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
 '
-- 
1.7.7.3
Jonathan Nieder· Dec 8, 2011, 17:12 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 5/6] t1510 (worktree): fix '&&' chaining

Ramkumar Ramachandra wrote:
Show 6 quoted lines
> Breaks in a test assertion's && chain can potentially hide failures
> from earlier commands in the chain.  Fix these breaks.
>
> Additionally, note that 'unset' returns non-zero status when the
> variable passed was already unset on some shells: change these
> instances to 'safe_unset'.

This is also not "Additionally", as in "As a separate change that maybe should have been another patch but I am too lazy". Rather, it is a necessary change that is part of the same task. So I would write:

	Breaks in a test assertion's && chain can potentially hide failures
	from earlier commands in the chain.  Fix these breaks.
	'unset' returns non-zero status when the variable passed was already
	unset on some shells, so now that the status is tested we need to
	change these instances to 'safe_unset'.
Erm, sane_unset, not safe_unset.  Did you even test this?
Show 5 quoted lines
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t1501-worktree.sh |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
For what it's worth, after those changes,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Junio C Hamano· Dec 9, 2011, 00:00 UTC · re: Jonathan Nieder · lore

Re: [PATCH 5/6] t1510 (worktree): fix '&&' chaining

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> This is also not "Additionally", as in "As a separate change that
> maybe should have been another patch but I am too lazy".  Rather, it
> is a necessary change that is part of the same task.  So I would
> write:
>
> 	Breaks in a test assertion's && chain can potentially hide failures
> 	from earlier commands in the chain.  Fix these breaks.
>
> 	'unset' returns non-zero status when the variable passed was already
> 	unset on some shells, so now that the status is tested we need to
> 	change these instances to 'safe_unset'.
>
> Erm, sane_unset, not safe_unset.  Did you even test this?
Thanks for careful reading and many constructive review comments.
Ramkumar Ramachandra· Dec 8, 2011, 13:10 UTC · re: Ramkumar Ramachandra · lore

[PATCH 6/6] test: fix '&&' chaining

Breaks in a test assertion's && chain can potentially hide failures from earlier commands in the chain. Fix instances of this in the following tests:

t3419 (rebase-patch-id) t3310 (notes-merge-manual-resolve) t3400 (rebase) t3418 (rebase-continue) t1511 (rev-parse-caret) t1510 (repo-setup) t1007 (hash-object) t1412 (reflog-loop) t1300 (repo-config) t1013 (loose-object-format)

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1007-hash-object.sh                |    2 +-
 t/t1013-loose-object-format.sh        |    2 +-
 t/t1300-repo-config.sh                |    2 +-
 t/t1412-reflog-loop.sh                |    2 +-
 t/t1510-repo-setup.sh                 |    4 ++--
 t/t1511-rev-parse-caret.sh            |    2 +-
 t/t3310-notes-merge-manual-resolve.sh |   10 +++++-----
 t/t3400-rebase.sh                     |    4 ++--
 t/t3418-rebase-continue.sh            |    4 ++--
 t/t3419-rebase-patch-id.sh            |    2 +-
 10 files changed, 17 insertions(+), 17 deletions(-)
Show changes to 10 files +17 −17

t/t1007-hash-object.sh, t/t1013-loose-object-format.sh, t/t1300-repo-config.sh, t/t1412-reflog-loop.sh, t/t1510-repo-setup.sh, t/t1511-rev-parse-caret.sh, t/t3310-notes-merge-manual-resolve.sh, t/t3400-rebase.sh, t/t3418-rebase-continue.sh, t/t3419-rebase-patch-id.sh

diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 6d52b82..f83df8e 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -189,7 +189,7 @@ for args in "-w --stdin-paths" "--stdin-paths -w"; do
 done
 
 test_expect_success 'corrupt tree' '
-	echo abc >malformed-tree
+	echo abc >malformed-tree &&
 	test_must_fail git hash-object -t tree malformed-tree
 '
 
diff --git a/t/t1013-loose-object-format.sh b/t/t1013-loose-object-format.sh
index 0a9cedd..fbf5f2f 100755
--- a/t/t1013-loose-object-format.sh
+++ b/t/t1013-loose-object-format.sh
@@ -34,7 +34,7 @@ assert_blob_equals() {
 }
 
 test_expect_success setup '
-	cp -R "$TEST_DIRECTORY/t1013/objects" .git/
+	cp -R "$TEST_DIRECTORY/t1013/objects" .git/ &&
 	git --version
 '
 
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 51caff0..0690e0e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -38,7 +38,7 @@ cat > expect << EOF
 	WhatEver = Second
 EOF
 test_expect_success 'similar section' '
-	git config Cores.WhatEver Second
+	git config Cores.WhatEver Second &&
 	test_cmp expect .git/config
 '
 
diff --git a/t/t1412-reflog-loop.sh b/t/t1412-reflog-loop.sh
index 647d888..3acd895 100755
--- a/t/t1412-reflog-loop.sh
+++ b/t/t1412-reflog-loop.sh
@@ -20,7 +20,7 @@ test_expect_success 'setup reflog with alternating commits' '
 '
 
 test_expect_success 'reflog shows all entries' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 		topic@{0} reset: moving to two
 		topic@{1} reset: moving to one
 		topic@{2} reset: moving to two
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index ec50a9a..80aedfc 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -603,7 +603,7 @@ test_expect_success '#22a: core.worktree = GIT_DIR = .git dir' '
 	# like case #6.
 
 	setup_repo 22a "$here/22a/.git" "" unset &&
-	setup_repo 22ab . "" unset
+	setup_repo 22ab . "" unset &&
 	mkdir -p 22a/.git/sub 22a/sub &&
 	mkdir -p 22ab/.git/sub 22ab/sub &&
 	try_case 22a/.git unset . \
@@ -742,7 +742,7 @@ test_expect_success '#28: core.worktree and core.bare conflict (gitfile case)' '
 # Case #29: GIT_WORK_TREE(+core.worktree) overrides core.bare (gitfile case).
 test_expect_success '#29: setup' '
 	setup_repo 29 non-existent gitfile true &&
-	mkdir -p 29/sub/sub 29/wt/sub
+	mkdir -p 29/sub/sub 29/wt/sub &&
 	(
 		cd 29 &&
 		GIT_WORK_TREE="$here/29" &&
diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index e043cb7..eaefc77 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -6,7 +6,7 @@ test_description='tests for ref^{stuff}'
 
 test_expect_success 'setup' '
 	echo blob >a-blob &&
-	git tag -a -m blob blob-tag `git hash-object -w a-blob`
+	git tag -a -m blob blob-tag `git hash-object -w a-blob` &&
 	mkdir a-tree &&
 	echo moreblobs >a-tree/another-blob &&
 	git add . &&
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 4ec4d11..4367197 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -389,7 +389,7 @@ test_expect_success 'abort notes merge' '
 	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == y)
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -525,9 +525,9 @@ EOF
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
 	# Refs are unchanged
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
-	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
-	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
+	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
+	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
 	# Mention refs/notes/m, and its current and expected value in output
 	grep -q "refs/notes/m" output &&
 	grep -q "$(git rev-parse refs/notes/m)" output &&
@@ -545,7 +545,7 @@ test_expect_success 'resolve situation by aborting the notes merge' '
 	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == w)
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6eaecec..c355533 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -172,8 +172,8 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 
 test_expect_success 'default to @{upstream} when upstream arg is missing' '
 	git checkout -b default topic &&
-	git config branch.default.remote .
-	git config branch.default.merge refs/heads/master
+	git config branch.default.remote . &&
+	git config branch.default.merge refs/heads/master &&
 	git rebase &&
 	test "$(git rev-parse default~1)" = "$(git rev-parse master)"
 '
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 1e855cd..2680375 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -51,7 +51,7 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
 	test_when_finished "rm -fr test-bin funny.was.run" &&
 	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF
+	cat >test-bin/git-merge-funny <<-EOF &&
 	#!$SHELL_PATH
 	case "\$1" in --opt) ;; *) exit 2 ;; esac
 	shift &&
@@ -77,7 +77,7 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 test_expect_success 'rebase --continue remembers --rerere-autoupdate' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-	git checkout master
+	git checkout master &&
 	test_commit "commit-new-file-F3" F3 3 &&
 	git config rerere.enabled true &&
 	test_must_fail git rebase -m master topic &&
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index bd8efaf..e70ac10 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -39,7 +39,7 @@ run()
 }
 
 test_expect_success 'setup' '
-	git commit --allow-empty -m initial
+	git commit --allow-empty -m initial &&
 	git tag root
 '
 
-- 
1.7.7.3
Jonathan Nieder· Dec 8, 2011, 17:18 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 6/6] test: fix '&&' chaining

Ramkumar Ramachandra wrote:
Show 6 quoted lines
> Breaks in a test assertion's && chain can potentially hide failures
> from earlier commands in the chain.  Fix instances of this in the
> following tests:
> 
> t3419 (rebase-patch-id)
> t3310 (notes-merge-manual-resolve)
[...]

The reader can read the diffstat, so I am not sure this list is very useful.

With the space gained, it might be helpful to mention that this patch only adds " &&" to the ends of lines and that any other kind of change here would be unintentional, to put the reader's mind at ease.

[...]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
The patch proper looks good, so if this is tested,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Junio C Hamano· Dec 8, 2011, 19:55 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH v2 0/6] Fix '&&' chaining in tests

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> This follows-up $gmane/186481.

I take that you meant "replaces". It was confusing especially because you seem to have included a few unrelated patches in the thread.

Ramkumar Ramachandra· Dec 9, 2011, 05:23 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/6] Fix '&&' chaining in tests

Hi,
Junio C Hamano wrote:
Show 6 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> This follows-up $gmane/186481.
>
> I take that you meant "replaces".  It was confusing especially because you
> seem to have included a few unrelated patches in the thread.

Yeah. Sorry- stray files were lying around from the previous 'git format-patch' invocation. Which brings me to: I wonder if it makes sense to (optionally) check that the directory is empty when executing 'git format-patch -o <dir>'. I sometimes forget to do it by hand.

-- Ram
Ramkumar Ramachandra· Dec 9, 2011, 11:29 UTC · re: Ramkumar Ramachandra · lore

[PATCH v3 0/6] Fix '&&' chaining in tests

This replaces the previous iteration of the series at $gmane/186553. Thanks to Jonathan for reviewing.

-- Ram
Ramkumar Ramachandra (6):
  t3040 (subprojects-basic): fix '&&' chaining, modernize style
  t3030 (merge-recursive): use test_expect_code
  t1006 (cat-file): use test_cmp
  t3200 (branch): fix '&&' chaining
  t1510 (worktree): fix '&&' chaining
  tests: fix '&&' chaining
 t/t1006-cat-file.sh                   |  119 +++++++++++++--------------
 t/t1007-hash-object.sh                |    2 +-
 t/t1013-loose-object-format.sh        |    2 +-
 t/t1300-repo-config.sh                |    2 +-
 t/t1412-reflog-loop.sh                |    2 +-
 t/t1501-worktree.sh                   |    6 +-
 t/t1510-repo-setup.sh                 |    4 +-
 t/t1511-rev-parse-caret.sh            |    2 +-
 t/t3030-merge-recursive.sh            |   72 ++---------------
 t/t3040-subprojects-basic.sh          |  144 ++++++++++++++++----------------
 t/t3200-branch.sh                     |    4 +-
 t/t3310-notes-merge-manual-resolve.sh |   10 +-
 t/t3400-rebase.sh                     |    4 +-
 t/t3418-rebase-continue.sh            |    4 +-
 t/t3419-rebase-patch-id.sh            |    2 +-
 15 files changed, 156 insertions(+), 223 deletions(-)
-- 
1.7.7.3
Ramkumar Ramachandra· Dec 9, 2011, 11:29 UTC · re: Ramkumar Ramachandra · lore

[PATCH 1/6] t3040 (subprojects-basic): fix '&&' chaining, modernize style

Breaks in a test assertion's && chain can potentially hide failures from earlier commands in the chain. Fix instances of this. While at it, clean up the style to fit the prevailing style. This means:

- Put the opening quote starting each test on the same line as the
  test_expect_* invocation.
- Indent the file with tabs, not spaces.
- Use test_expect_code() in preference to checking the exit status of
  various statements by hand.
- Guard commands that prepare test input for individual tests in the
  same test_expect_success, so that their scope is clearer and errors
  at that stage can be caught.
- Use <<-\EOF in preference to <<EOF to save readers the trouble of
  looking for variable interpolations.
- Include "setup" in the titles of test assertions that prepare for
  later ones to make it more obvious which tests can be skipped.
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3040-subprojects-basic.sh |  144 +++++++++++++++++++++---------------------
 1 files changed, 72 insertions(+), 72 deletions(-)
Show changes to t/t3040-subprojects-basic.sh +72 −72
diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh
index f6973e9..0a4ff6d 100755
--- a/t/t3040-subprojects-basic.sh
+++ b/t/t3040-subprojects-basic.sh
@@ -3,81 +3,81 @@
 test_description='Basic subproject functionality'
 . ./test-lib.sh
 
-test_expect_success 'Super project creation' \
-    ': >Makefile &&
-    git add Makefile &&
-    git commit -m "Superproject created"'
-
-
-cat >expected <<EOF
-:000000 160000 00000... A	sub1
-:000000 160000 00000... A	sub2
-EOF
-test_expect_success 'create subprojects' \
-    'mkdir sub1 &&
-    ( cd sub1 && git init && : >Makefile && git add * &&
-    git commit -q -m "subproject 1" ) &&
-    mkdir sub2 &&
-    ( cd sub2 && git init && : >Makefile && git add * &&
-    git commit -q -m "subproject 2" ) &&
-    git update-index --add sub1 &&
-    git add sub2 &&
-    git commit -q -m "subprojects added" &&
-    git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
-    test_cmp expected current'
-
-git branch save HEAD
-
-test_expect_success 'check if fsck ignores the subprojects' \
-    'git fsck --full'
-
-test_expect_success 'check if commit in a subproject detected' \
-    '( cd sub1 &&
-    echo "all:" >>Makefile &&
-    echo "	true" >>Makefile &&
-    git commit -q -a -m "make all" ) && {
-        git diff-files --exit-code
-	test $? = 1
-    }'
-
-test_expect_success 'check if a changed subproject HEAD can be committed' \
-    'git commit -q -a -m "sub1 changed" && {
-	git diff-tree --exit-code HEAD^ HEAD
-	test $? = 1
-    }'
-
-test_expect_success 'check if diff-index works for subproject elements' \
-    'git diff-index --exit-code --cached save -- sub1
-    test $? = 1'
-
-test_expect_success 'check if diff-tree works for subproject elements' \
-    'git diff-tree --exit-code HEAD^ HEAD -- sub1
-    test $? = 1'
-
-test_expect_success 'check if git diff works for subproject elements' \
-    'git diff --exit-code HEAD^ HEAD
-    test $? = 1'
-
-test_expect_success 'check if clone works' \
-    'git ls-files -s >expected &&
-    git clone -l -s . cloned &&
-    ( cd cloned && git ls-files -s ) >current &&
-    test_cmp expected current'
-
-test_expect_success 'removing and adding subproject' \
-    'git update-index --force-remove -- sub2 &&
-    mv sub2 sub3 &&
-    git add sub3 &&
-    git commit -q -m "renaming a subproject" && {
-	git diff -M --name-status --exit-code HEAD^ HEAD
-	test $? = 1
-    }'
+test_expect_success 'setup: create superproject' '
+	: >Makefile &&
+	git add Makefile &&
+	git commit -m "Superproject created"
+'
+
+test_expect_success 'setup: create subprojects' '
+	mkdir sub1 &&
+	( cd sub1 && git init && : >Makefile && git add * &&
+	git commit -q -m "subproject 1" ) &&
+	mkdir sub2 &&
+	( cd sub2 && git init && : >Makefile && git add * &&
+	git commit -q -m "subproject 2" ) &&
+	git update-index --add sub1 &&
+	git add sub2 &&
+	git commit -q -m "subprojects added" &&
+	git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
+	git branch save HEAD &&
+	cat >expected <<-\EOF &&
+	:000000 160000 00000... A	sub1
+	:000000 160000 00000... A	sub2
+	EOF
+	test_cmp expected current
+'
+
+test_expect_success 'check if fsck ignores the subprojects' '
+	git fsck --full
+'
+
+test_expect_success 'check if commit in a subproject detected' '
+	( cd sub1 &&
+	echo "all:" >>Makefile &&
+	echo "	true" >>Makefile &&
+	git commit -q -a -m "make all" ) &&
+	test_expect_code 1 git diff-files --exit-code
+'
+
+test_expect_success 'check if a changed subproject HEAD can be committed' '
+	git commit -q -a -m "sub1 changed" &&
+	test_expect_code 1 git diff-tree --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'check if diff-index works for subproject elements' '
+	test_expect_code 1 git diff-index --exit-code --cached save -- sub1
+'
+
+test_expect_success 'check if diff-tree works for subproject elements' '
+	test_expect_code 1 git diff-tree --exit-code HEAD^ HEAD -- sub1
+'
+
+test_expect_success 'check if git diff works for subproject elements' '
+	test_expect_code 1 git diff --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'check if clone works' '
+	git ls-files -s >expected &&
+	git clone -l -s . cloned &&
+	( cd cloned && git ls-files -s ) >current &&
+	test_cmp expected current
+'
+
+test_expect_success 'removing and adding subproject' '
+	git update-index --force-remove -- sub2 &&
+	mv sub2 sub3 &&
+	git add sub3 &&
+	git commit -q -m "renaming a subproject" &&
+	test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD
+'
 
 # the index must contain the object name the HEAD of the
 # subproject sub1 was at the point "save"
-test_expect_success 'checkout in superproject' \
-    'git checkout save &&
-    git diff-index --exit-code --raw --cached save -- sub1'
+test_expect_success 'checkout in superproject' '
+	git checkout save &&
+	git diff-index --exit-code --raw --cached save -- sub1
+'
 
 # just interesting what happened...
 # git diff --name-status -M save master
-- 
1.7.7.3
Ramkumar Ramachandra· Dec 9, 2011, 11:29 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/6] t3030 (merge-recursive): use test_expect_code

Use test_expect_code in preference to repeatedly checking exit codes by hand.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3030-merge-recursive.sh |   72 ++++----------------------------------------
 1 files changed, 6 insertions(+), 66 deletions(-)
Show changes to t/t3030-merge-recursive.sh +6 −66
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 55ef189..a5e3da7 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -285,17 +285,7 @@ test_expect_success 'merge-recursive simple' '
 	rm -fr [abcd] &&
 	git checkout -f "$c2" &&
 
-	git merge-recursive "$c0" -- "$c2" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c2" "$c1"
 '
 
 test_expect_success 'merge-recursive result' '
@@ -334,17 +324,7 @@ test_expect_success 'merge-recursive remove conflict' '
 	rm -fr [abcd] &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c5"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c5"
 '
 
 test_expect_success 'merge-recursive remove conflict' '
@@ -388,17 +368,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c4"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c4"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
@@ -422,17 +392,7 @@ test_expect_success 'merge-recursive d/f conflict the other way' '
 	git reset --hard &&
 	git checkout -f "$c4" &&
 
-	git merge-recursive "$c0" -- "$c4" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c4" "$c1"
 '
 
 test_expect_success 'merge-recursive d/f conflict result the other way' '
@@ -456,17 +416,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c6"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c6"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
@@ -490,17 +440,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c6" &&
 
-	git merge-recursive "$c0" -- "$c6" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c6" "$c1"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
-- 
1.7.7.3
Ramkumar Ramachandra· Dec 9, 2011, 11:29 UTC · re: Ramkumar Ramachandra · lore

[PATCH 3/6] t1006 (cat-file): use test_cmp

In testing, a common paradigm involves checking the expected output with the actual output: test-lib provides a test_cmp to show the diff between the two outputs. So, use this function in preference to calling a human over to compare command outputs by eye.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1006-cat-file.sh |  119 ++++++++++++++++++++++++---------------------------
 1 files changed, 56 insertions(+), 63 deletions(-)
Show changes to t/t1006-cat-file.sh +56 −63
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index d8b7f2f..5be3ce9 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -36,66 +36,41 @@ $content"
     '
 
     test_expect_success "Type of $type is correct" '
-        test $type = "$(git cat-file -t $sha1)"
+	echo $type >expect &&
+	git cat-file -t $sha1 >actual &&
+	test_cmp expect actual
     '
 
     test_expect_success "Size of $type is correct" '
-        test $size = "$(git cat-file -s $sha1)"
+	echo $size >expect &&
+	git cat-file -s $sha1 >actual &&
+	test_cmp expect actual
     '
 
     test -z "$content" ||
     test_expect_success "Content of $type is correct" '
-	expect="$(maybe_remove_timestamp "$content" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts)"
-
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$content" $no_ts >expect &&
+	maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test_expect_success "Pretty content of $type is correct" '
-	expect="$(maybe_remove_timestamp "$pretty_content" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(git cat-file -p $sha1)" $no_ts)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$pretty_content" $no_ts >expect &&
+	maybe_remove_timestamp "$(git cat-file -p $sha1)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test -z "$content" ||
     test_expect_success "--batch output of $type is correct" '
-	expect="$(maybe_remove_timestamp "$batch_output" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)" $no_ts)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$batch_output" $no_ts >expect &&
+	maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test_expect_success "--batch-check output of $type is correct" '
-	expect="$sha1 $type $size"
-	actual="$(echo_without_newline $sha1 | git cat-file --batch-check)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	echo "$sha1 $type $size" >expect &&
+	echo_without_newline $sha1 | git cat-file --batch-check >actual &&
+	test_cmp expect actual
     '
 }
 
@@ -144,10 +119,13 @@ tag_size=$(strlen "$tag_content")
 
 run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_pretty_content" 1
 
-test_expect_success \
-    "Reach a blob from a tag pointing to it" \
-    "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
+test_expect_success "Reach a blob from a tag pointing to it" '
+    echo_without_newline "$hello_content" >expect &&
+    git cat-file blob "$tag_sha1" >actual &&
+    test_cmp expect actual
+'
 
+test_done
 for batch in batch batch-check
 do
     for opt in t s e p
@@ -175,30 +153,41 @@ do
 done
 
 test_expect_success "--batch-check for a non-existent named object" '
-    test "foobar42 missing
-foobar84 missing" = \
-    "$( ( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)"
+    cat >expect <<\-EOF &&
+foobar42 missing
+foobar84 missing
+EOF
+    echo foobar42; echo_without_newline foobar84 | \
+    git cat-file --batch-check >actual &&
+    test_cmp expect actual
 '
 
 test_expect_success "--batch-check for a non-existent hash" '
-    test "0000000000000000000000000000000000000042 missing
-0000000000000000000000000000000000000084 missing" = \
-    "$( ( echo 0000000000000000000000000000000000000042;
-         echo_without_newline 0000000000000000000000000000000000000084; ) \
-       | git cat-file --batch-check)"
+    cat >expect <<\-EOF &&
+0000000000000000000000000000000000000042 missing
+0000000000000000000000000000000000000084 missing
+EOF
+    echo 0000000000000000000000000000000000000042;
+    echo_without_newline 0000000000000000000000000000000000000084 | \
+    git cat-file --batch-check >actual &&
+    test_cmp expect actual
 '
 
 test_expect_success "--batch for an existent and a non-existent hash" '
-    test "$tag_sha1 tag $tag_size
+    cat >expect <<\-EOF &&
+tag_sha1 tag $tag_size
 $tag_content
-0000000000000000000000000000000000000000 missing" = \
-    "$( ( echo $tag_sha1;
-         echo_without_newline 0000000000000000000000000000000000000000; ) \
-       | git cat-file --batch)"
+0000000000000000000000000000000000000000 missing
+EOF
+    echo $tag_sha1; echo_without_newline 0000000000000000000000000000000000000000 | \
+    git cat-file --batch >actual &&
+    test_cmp expect_actual
 '
 
 test_expect_success "--batch-check for an emtpy line" '
-    test " missing" = "$(echo | git cat-file --batch-check)"
+    echo " missing" >expect &&
+    echo | git cat-file --batch-check >actual &&
+    test_cmp expect actual
 '
 
 batch_input="$hello_sha1
@@ -218,7 +207,10 @@ deadbeef missing
  missing"
 
 test_expect_success '--batch with multiple sha1s gives correct format' '
-	test "$(maybe_remove_timestamp "$batch_output" 1)" = "$(maybe_remove_timestamp "$(echo_without_newline "$batch_input" | git cat-file --batch)" 1)"
+	maybe_remove_timestamp "$batch_output" 1 >expect &&
+	maybe_remove_timestamp "echo_without_newline "$batch_input" | \
+	git cat-file --batch" 1 >actual &&
+	test_cmp expect actual
 '
 
 batch_check_input="$hello_sha1
@@ -237,8 +229,9 @@ deadbeef missing
  missing"
 
 test_expect_success "--batch-check with multiple sha1s gives correct format" '
-    test "$batch_check_output" = \
-    "$(echo_without_newline "$batch_check_input" | git cat-file --batch-check)"
+    echo "$batch_check_output" >expect &&
+    echo_without_newline "$batch_check_input" | git cat-file --batch-check >actual &&
+    test_cmp expect actual
 '
 
 test_done
-- 
1.7.7.3
Junio C Hamano· Dec 9, 2011, 18:43 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 3/6] t1006 (cat-file): use test_cmp

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 32 quoted lines
> In testing, a common paradigm involves checking the expected output
> with the actual output: test-lib provides a test_cmp to show the diff
> between the two outputs.  So, use this function in preference to
> calling a human over to compare command outputs by eye.
>
> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t1006-cat-file.sh |  119 ++++++++++++++++++++++++---------------------------
>  1 files changed, 56 insertions(+), 63 deletions(-)
>
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index d8b7f2f..5be3ce9 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -144,10 +119,13 @@ tag_size=$(strlen "$tag_content")
>  
>  run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_pretty_content" 1
>  
> -test_expect_success \
> -    "Reach a blob from a tag pointing to it" \
> -    "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
> +test_expect_success "Reach a blob from a tag pointing to it" '
> +    echo_without_newline "$hello_content" >expect &&
> +    git cat-file blob "$tag_sha1" >actual &&
> +    test_cmp expect actual
> +'
>  
> +test_done
>  for batch in batch batch-check
>  do
>      for opt in t s e p
What is that test_done about?
Ramkumar Ramachandra· Dec 9, 2011, 18:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/6] t1006 (cat-file): use test_cmp

Hi,
Junio C Hamano wrote:
Show 36 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> In testing, a common paradigm involves checking the expected output
>> with the actual output: test-lib provides a test_cmp to show the diff
>> between the two outputs.  So, use this function in preference to
>> calling a human over to compare command outputs by eye.
>>
>> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>>  t/t1006-cat-file.sh |  119 ++++++++++++++++++++++++---------------------------
>>  1 files changed, 56 insertions(+), 63 deletions(-)
>>
>> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
>> index d8b7f2f..5be3ce9 100755
>> --- a/t/t1006-cat-file.sh
>> +++ b/t/t1006-cat-file.sh
>> @@ -144,10 +119,13 @@ tag_size=$(strlen "$tag_content")
>>
>>  run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_pretty_content" 1
>>
>> -test_expect_success \
>> -    "Reach a blob from a tag pointing to it" \
>> -    "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
>> +test_expect_success "Reach a blob from a tag pointing to it" '
>> +    echo_without_newline "$hello_content" >expect &&
>> +    git cat-file blob "$tag_sha1" >actual &&
>> +    test_cmp expect actual
>> +'
>>
>> +test_done
>>  for batch in batch batch-check
>>  do
>>      for opt in t s e p
>
> What is that test_done about?

:facepalm: crept in while testing -- I often (temporarily) use a test_done after the test I want to inspect so the others aren't unnecessarily executed hiding the error.

Thanks for catching this.
-- Ram
Ramkumar Ramachandra· Dec 9, 2011, 11:29 UTC · re: Ramkumar Ramachandra · lore

[PATCH 4/6] t3200 (branch): fix '&&' chaining

Breaks in a test assertion's && chain can potentially hide failures from earlier commands in the chain. Fix these breaks.

'git branch --help' will fail when git manpages aren't already installed; now that its status is tested, guard these instances with 'test_might_fail'.

Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3200-branch.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t3200-branch.sh +2 −2
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index bc73c20..95066d0 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -22,7 +22,7 @@ test_expect_success \
 
 test_expect_success \
     'git branch --help should not have created a bogus branch' '
-     git branch --help </dev/null >/dev/null 2>/dev/null;
+     test_might_fail git branch --help </dev/null >/dev/null 2>/dev/null &&
      test_path_is_missing .git/refs/heads/--help
 '
 
@@ -88,7 +88,7 @@ test_expect_success \
 test_expect_success \
     'git branch -m n/n n should work' \
        'git branch -l n/n &&
-        git branch -m n/n n
+        git branch -m n/n n &&
 	test_path_is_file .git/logs/refs/heads/n'
 
 test_expect_success 'git branch -m o/o o should fail when o/p exists' '
-- 
1.7.7.3
Ramkumar Ramachandra· Dec 9, 2011, 11:29 UTC · re: Ramkumar Ramachandra · lore

[PATCH 5/6] t1510 (worktree): fix '&&' chaining

Breaks in a test assertion's && chain can potentially hide failures from earlier commands in the chain. Fix these breaks.

'unset' returns non-zero status when the variable passed was already unset on some shells; now that its status is tested, change these instances to 'sane_unset'.

Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1501-worktree.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to t/t1501-worktree.sh +3 −3
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 6384983..e661147 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -48,7 +48,7 @@ test_expect_success 'setup: helper for testing rev-parse' '
 '
 
 test_expect_success 'setup: core.worktree = relative path' '
-	unset GIT_WORK_TREE;
+	sane_unset GIT_WORK_TREE &&
 	GIT_DIR=repo.git &&
 	GIT_CONFIG="$(pwd)"/$GIT_DIR/config &&
 	export GIT_DIR GIT_CONFIG &&
@@ -89,7 +89,7 @@ test_expect_success 'subdir of work tree' '
 '
 
 test_expect_success 'setup: core.worktree = absolute path' '
-	unset GIT_WORK_TREE;
+	sane_unset GIT_WORK_TREE &&
 	GIT_DIR=$(pwd)/repo.git &&
 	GIT_CONFIG=$GIT_DIR/config &&
 	export GIT_DIR GIT_CONFIG &&
@@ -334,7 +334,7 @@ test_expect_success 'absolute pathspec should fail gracefully' '
 '
 
 test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
-	>dummy_file
+	>dummy_file &&
 	echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file &&
 	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
 '
-- 
1.7.7.3
Ramkumar Ramachandra· Dec 9, 2011, 11:29 UTC · re: Ramkumar Ramachandra · lore

[PATCH 6/6] tests: fix '&&' chaining

Breaks in a test assertion's && chain can potentially hide failures from earlier commands in the chain. Fix instances of this by adding '&&' at the end of lines where they're missing; this patch doesn't intend to make any other changes.

Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1007-hash-object.sh                |    2 +-
 t/t1013-loose-object-format.sh        |    2 +-
 t/t1300-repo-config.sh                |    2 +-
 t/t1412-reflog-loop.sh                |    2 +-
 t/t1510-repo-setup.sh                 |    4 ++--
 t/t1511-rev-parse-caret.sh            |    2 +-
 t/t3310-notes-merge-manual-resolve.sh |   10 +++++-----
 t/t3400-rebase.sh                     |    4 ++--
 t/t3418-rebase-continue.sh            |    4 ++--
 t/t3419-rebase-patch-id.sh            |    2 +-
 10 files changed, 17 insertions(+), 17 deletions(-)
Show changes to 10 files +17 −17

t/t1007-hash-object.sh, t/t1013-loose-object-format.sh, t/t1300-repo-config.sh, t/t1412-reflog-loop.sh, t/t1510-repo-setup.sh, t/t1511-rev-parse-caret.sh, t/t3310-notes-merge-manual-resolve.sh, t/t3400-rebase.sh, t/t3418-rebase-continue.sh, t/t3419-rebase-patch-id.sh

diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 6d52b82..f83df8e 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -189,7 +189,7 @@ for args in "-w --stdin-paths" "--stdin-paths -w"; do
 done
 
 test_expect_success 'corrupt tree' '
-	echo abc >malformed-tree
+	echo abc >malformed-tree &&
 	test_must_fail git hash-object -t tree malformed-tree
 '
 
diff --git a/t/t1013-loose-object-format.sh b/t/t1013-loose-object-format.sh
index 0a9cedd..fbf5f2f 100755
--- a/t/t1013-loose-object-format.sh
+++ b/t/t1013-loose-object-format.sh
@@ -34,7 +34,7 @@ assert_blob_equals() {
 }
 
 test_expect_success setup '
-	cp -R "$TEST_DIRECTORY/t1013/objects" .git/
+	cp -R "$TEST_DIRECTORY/t1013/objects" .git/ &&
 	git --version
 '
 
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 51caff0..0690e0e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -38,7 +38,7 @@ cat > expect << EOF
 	WhatEver = Second
 EOF
 test_expect_success 'similar section' '
-	git config Cores.WhatEver Second
+	git config Cores.WhatEver Second &&
 	test_cmp expect .git/config
 '
 
diff --git a/t/t1412-reflog-loop.sh b/t/t1412-reflog-loop.sh
index 647d888..3acd895 100755
--- a/t/t1412-reflog-loop.sh
+++ b/t/t1412-reflog-loop.sh
@@ -20,7 +20,7 @@ test_expect_success 'setup reflog with alternating commits' '
 '
 
 test_expect_success 'reflog shows all entries' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 		topic@{0} reset: moving to two
 		topic@{1} reset: moving to one
 		topic@{2} reset: moving to two
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index ec50a9a..80aedfc 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -603,7 +603,7 @@ test_expect_success '#22a: core.worktree = GIT_DIR = .git dir' '
 	# like case #6.
 
 	setup_repo 22a "$here/22a/.git" "" unset &&
-	setup_repo 22ab . "" unset
+	setup_repo 22ab . "" unset &&
 	mkdir -p 22a/.git/sub 22a/sub &&
 	mkdir -p 22ab/.git/sub 22ab/sub &&
 	try_case 22a/.git unset . \
@@ -742,7 +742,7 @@ test_expect_success '#28: core.worktree and core.bare conflict (gitfile case)' '
 # Case #29: GIT_WORK_TREE(+core.worktree) overrides core.bare (gitfile case).
 test_expect_success '#29: setup' '
 	setup_repo 29 non-existent gitfile true &&
-	mkdir -p 29/sub/sub 29/wt/sub
+	mkdir -p 29/sub/sub 29/wt/sub &&
 	(
 		cd 29 &&
 		GIT_WORK_TREE="$here/29" &&
diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index e043cb7..eaefc77 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -6,7 +6,7 @@ test_description='tests for ref^{stuff}'
 
 test_expect_success 'setup' '
 	echo blob >a-blob &&
-	git tag -a -m blob blob-tag `git hash-object -w a-blob`
+	git tag -a -m blob blob-tag `git hash-object -w a-blob` &&
 	mkdir a-tree &&
 	echo moreblobs >a-tree/another-blob &&
 	git add . &&
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 4ec4d11..4367197 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -389,7 +389,7 @@ test_expect_success 'abort notes merge' '
 	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == y)
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -525,9 +525,9 @@ EOF
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
 	# Refs are unchanged
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
-	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
-	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
+	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
+	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
 	# Mention refs/notes/m, and its current and expected value in output
 	grep -q "refs/notes/m" output &&
 	grep -q "$(git rev-parse refs/notes/m)" output &&
@@ -545,7 +545,7 @@ test_expect_success 'resolve situation by aborting the notes merge' '
 	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == w)
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6eaecec..c355533 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -172,8 +172,8 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 
 test_expect_success 'default to @{upstream} when upstream arg is missing' '
 	git checkout -b default topic &&
-	git config branch.default.remote .
-	git config branch.default.merge refs/heads/master
+	git config branch.default.remote . &&
+	git config branch.default.merge refs/heads/master &&
 	git rebase &&
 	test "$(git rev-parse default~1)" = "$(git rev-parse master)"
 '
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 1e855cd..2680375 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -51,7 +51,7 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
 	test_when_finished "rm -fr test-bin funny.was.run" &&
 	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF
+	cat >test-bin/git-merge-funny <<-EOF &&
 	#!$SHELL_PATH
 	case "\$1" in --opt) ;; *) exit 2 ;; esac
 	shift &&
@@ -77,7 +77,7 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 test_expect_success 'rebase --continue remembers --rerere-autoupdate' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-	git checkout master
+	git checkout master &&
 	test_commit "commit-new-file-F3" F3 3 &&
 	git config rerere.enabled true &&
 	test_must_fail git rebase -m master topic &&
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index bd8efaf..e70ac10 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -39,7 +39,7 @@ run()
 }
 
 test_expect_success 'setup' '
-	git commit --allow-empty -m initial
+	git commit --allow-empty -m initial &&
 	git tag root
 '
 
-- 
1.7.7.3

← back to recent threads