* [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Ramkumar Ramachandra @ 2011-12-08 13:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-1-git-send-email-artagnon@gmail.com>
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(-)
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
^ permalink raw reply related
* [PATCH 2/6] t3030 (merge-recursive): use test_expect_code
From: Ramkumar Ramachandra @ 2011-12-08 13:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-1-git-send-email-artagnon@gmail.com>
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(-)
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
^ permalink raw reply related
* [PATCH 4/6] t3200 (branch): fix '&&' chaining
From: Ramkumar Ramachandra @ 2011-12-08 13:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-1-git-send-email-artagnon@gmail.com>
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(-)
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
^ permalink raw reply related
* [PATCH 3/6] t1006 (cat-file): use test_cmp
From: Ramkumar Ramachandra @ 2011-12-08 13:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-1-git-send-email-artagnon@gmail.com>
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(-)
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
^ permalink raw reply related
* [PATCH 6/6] test: fix '&&' chaining
From: Ramkumar Ramachandra @ 2011-12-08 13:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-1-git-send-email-artagnon@gmail.com>
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(-)
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
^ permalink raw reply related
* [PATCH 5/6] t1510 (worktree): fix '&&' chaining
From: Ramkumar Ramachandra @ 2011-12-08 13:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-1-git-send-email-artagnon@gmail.com>
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(-)
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
^ permalink raw reply related
* Re: [PATCH 3/6] t1006 (cat-file): use test_cmp
From: Matthieu Moy @ 2011-12-08 13:28 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jonathan Nieder, Junio C Hamano, Git List
In-Reply-To: <1323349817-15737-6-git-send-email-artagnon@gmail.com>
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?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 3/6] t1006 (cat-file): use test_cmp
From: Ramkumar Ramachandra @ 2011-12-08 13:33 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Jonathan Nieder, Junio C Hamano, Git List
In-Reply-To: <vpqiplrchvi.fsf@bauges.imag.fr>
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? Seems to be fine in the original message [1].
[1]: http://article.gmane.org/gmane.comp.version-control.git/186558
-- Ram
^ permalink raw reply
* [PATCH 1/2] index_pack: indent find_unresolved_detals one level
From: pclouds @ 2011-12-08 13:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shawn Pearce,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CAJo=hJvrk3Jzg3dQhQnfbmKAFovLuEtJAP4rakHPFeuZ0T5R7g@mail.gmail.com>
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
The next patch puts most of the code in one level deeper. By indenting
separately, it'd be easier to see the actual changes.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/index-pack.c | 47 +++++++++++++++++++++++------------------------
1 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 103e19c..9855ada 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -585,37 +585,36 @@ static void find_unresolved_deltas(struct base_data *base,
base_spec.offset = base->obj->idx.offset;
find_delta_children(&base_spec,
&ofs_first, &ofs_last, OBJ_OFS_DELTA);
- }
- if (ref_last == -1 && ofs_last == -1) {
- free(base->data);
- return;
- }
+ if (ref_last == -1 && ofs_last == -1) {
+ free(base->data);
+ return;
+ }
- link_base_data(prev_base, base);
+ link_base_data(prev_base, base);
- for (i = ref_first; i <= ref_last; i++) {
- struct object_entry *child = objects + deltas[i].obj_no;
- struct base_data result;
+ for (i = ref_first; i <= ref_last; i++) {
+ struct object_entry *child = objects + deltas[i].obj_no;
+ struct base_data result;
- assert(child->real_type == OBJ_REF_DELTA);
- resolve_delta(child, base, &result);
- if (i == ref_last && ofs_last == -1)
- free_base_data(base);
- find_unresolved_deltas(&result, base);
- }
+ assert(child->real_type == OBJ_REF_DELTA);
+ resolve_delta(child, base, &result);
+ if (i == ref_last && ofs_last == -1)
+ free_base_data(base);
+ find_unresolved_deltas(&result, base);
+ }
- for (i = ofs_first; i <= ofs_last; i++) {
- struct object_entry *child = objects + deltas[i].obj_no;
- struct base_data result;
+ for (i = ofs_first; i <= ofs_last; i++) {
+ struct object_entry *child = objects + deltas[i].obj_no;
+ struct base_data result;
- assert(child->real_type == OBJ_OFS_DELTA);
- resolve_delta(child, base, &result);
- if (i == ofs_last)
- free_base_data(base);
- find_unresolved_deltas(&result, base);
+ assert(child->real_type == OBJ_OFS_DELTA);
+ resolve_delta(child, base, &result);
+ if (i == ofs_last)
+ free_base_data(base);
+ find_unresolved_deltas(&result, base);
+ }
}
-
unlink_base_data(base);
}
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 2/2] index-pack: a naive attempt to flatten find_unresolved_deltas
From: pclouds @ 2011-12-08 13:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shawn Pearce,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1323351638-4790-1-git-send-email-y>
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
This seems to work for me. But I think this approach uses (much?) more
memory because it turns a tree of calls into a flat list of calls and
keep the list until the end, while the recursive version only has to
keep one call chain at a time.
Any ideas how to improve it?
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/index-pack.c | 57 +++++++++++++++++++++++++++++++++++++------------
1 files changed, 43 insertions(+), 14 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 9855ada..d4c182f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -565,18 +565,31 @@ static void resolve_delta(struct object_entry *delta_obj,
nr_resolved_deltas++;
}
+struct fud_stack {
+ struct base_data base_;
+ struct base_data *base;
+ struct base_data *prev_base;
+};
+
static void find_unresolved_deltas(struct base_data *base,
struct base_data *prev_base)
{
+ struct fud_stack **stack = NULL;
+ int stk, stack_nr = 0, stack_alloc = 0;
int i, ref_first, ref_last, ofs_first, ofs_last;
- /*
- * This is a recursive function. Those brackets should help reducing
- * stack usage by limiting the scope of the delta_base union.
- */
- {
+ ALLOC_GROW(stack, stack_nr + 1, stack_alloc);
+ stack[stack_nr] = xmalloc(sizeof(struct fud_stack));
+ stack[stack_nr]->base = base;
+ stack[stack_nr]->prev_base = prev_base;
+ stack_nr++;
+
+ for (stk = 0; stk < stack_nr; stk++) {
union delta_base base_spec;
+ base = stack[stk]->base;
+ prev_base = stack[stk]->prev_base;
+
hashcpy(base_spec.sha1, base->obj->idx.sha1);
find_delta_children(&base_spec,
&ref_first, &ref_last, OBJ_REF_DELTA);
@@ -588,34 +601,50 @@ static void find_unresolved_deltas(struct base_data *base,
if (ref_last == -1 && ofs_last == -1) {
free(base->data);
- return;
+ free(stack[stk]);
+ stack[stk] = NULL;
+ continue;
}
link_base_data(prev_base, base);
+ ALLOC_GROW(stack, stack_nr +
+ (ref_last - ref_first + 1) +
+ (ofs_last - ofs_first + 1),
+ stack_alloc);
+
for (i = ref_first; i <= ref_last; i++) {
struct object_entry *child = objects + deltas[i].obj_no;
- struct base_data result;
assert(child->real_type == OBJ_REF_DELTA);
- resolve_delta(child, base, &result);
+ stack[stack_nr] = xmalloc(sizeof(struct fud_stack));
+ stack[stack_nr]->base = &stack[stack_nr]->base_;
+ resolve_delta(child, base, stack[stack_nr]->base);
if (i == ref_last && ofs_last == -1)
free_base_data(base);
- find_unresolved_deltas(&result, base);
+ stack[stack_nr]->prev_base = base;
+ stack_nr++;
}
for (i = ofs_first; i <= ofs_last; i++) {
struct object_entry *child = objects + deltas[i].obj_no;
- struct base_data result;
-
assert(child->real_type == OBJ_OFS_DELTA);
- resolve_delta(child, base, &result);
+ stack[stack_nr] = xmalloc(sizeof(struct fud_stack));
+ stack[stack_nr]->base = &stack[stack_nr]->base_;
+ resolve_delta(child, base, stack[stack_nr]->base);
if (i == ofs_last)
free_base_data(base);
- find_unresolved_deltas(&result, base);
+ stack[stack_nr]->prev_base = base;
+ stack_nr++;
}
}
- unlink_base_data(base);
+
+ for (stk = stack_nr - 1; stk >= 0; stk--)
+ if (stack[stk]) {
+ unlink_base_data(stack[stk]->base);
+ free(stack[stk]);
+ }
+ free(stack);
}
static int compare_delta_entry(const void *a, const void *b)
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: [PATCH 3/6] t1006 (cat-file): use test_cmp
From: Matthieu Moy @ 2011-12-08 13:41 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jonathan Nieder, Junio C Hamano, Git List
In-Reply-To: <CALkWK0=NFxAqmOkObqjVmBQ-TQ=hZhWi=ZScMEGibvS2Pu+XqQ@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 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/
^ permalink raw reply
* Re: [PATCH 0/5] cache-tree revisited
From: Thomas Rast @ 2011-12-08 14:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Carlos Martín Nieto
In-Reply-To: <cover.1323191497.git.trast@student.ethz.ch>
Thomas Rast wrote:
> Junio C Hamano wrote:
> > Ahh, I forgot all about that exchange.
> >
> > http://thread.gmane.org/gmane.comp.version-control.git/178480/focus=178515
> >
> > The cache-tree mechanism has traditionally been one of the more important
> > optimizations and it would be very nice if we can resurrect the behaviour
> > for "git commit" too.
>
> Oh, I buried that. Let's try something other than the aggressive
> strategy I had there: only compute cache-tree if
>
> * we know we're going to need it soon, and we're about to write out
> the index anyway (as in git-commit)
I had another idea: we could write out *just* a new cache-tree data
set at the end of git-commit.
Doing it the cheap way would mean rehashing the on-disk data without
actually touching it. (That might not be so bad, but then if your
index is small, why is writing it from scratch expensive?)
Doing it efficiently requires making the sha1 restartable, which is
entirely doable withblock-sha1/sha1.h (I haven't looked into
ppc/sha1.h). As far as I can see it's not feasible with openssl's
sha1.
That is, we would add a new index extension (say PSHA: partial SHA)
and structure the index as
signature
header
cache data
PSHA <sha state up until just before PSHA>
TREE ...
[REUC ...]
sha1 footer
Then it's easy to cheaply replace only the extensions, by restarting
the hashing from the PSHA data and re-emitting only the extension
data.
I think all the bits are in place, and it would be easy to do.
However, for it to make sense, we would have to make BLK_SHA1 the
default for the most-used platforms and also not mind extending the
SHA1 API. Do you think that would fly?
I thought about other ways to make the index writing restartable from
the middle, but the only clean approach I came up with would require a
format change to something like
signature
0 header
1 cache data
2 sha1 of 0..1
3 extension data A
4 sha1 of 2..3
5 extension data B
6 sha1 of 4..5
[possibly more]
7 end-of-index marker
8 sha1 of 6..7
etc.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: introduce OPT_SUBCOMMAND
From: Jonathan Nieder @ 2011-12-08 16:30 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-2-git-send-email-artagnon@gmail.com>
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
^ permalink raw reply
* Re: [PATCH 1/6] t3040 (subprojects-basic): modernize style
From: Jonathan Nieder @ 2011-12-08 16:34 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-3-git-send-email-artagnon@gmail.com>
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?
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Jonathan Nieder @ 2011-12-08 16:39 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-4-git-send-email-artagnon@gmail.com>
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".
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
^ permalink raw reply
* Re: [PATCH 2/2] index-pack: a naive attempt to flatten find_unresolved_deltas
From: Shawn Pearce @ 2011-12-08 16:42 UTC (permalink / raw)
To: pclouds; +Cc: git, Junio C Hamano
In-Reply-To: <4ee0be70.44f2320a.3fe8.632b@mx.google.com>
2011/12/8 <pclouds@gmail.com>:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> This seems to work for me. But I think this approach uses (much?) more
> memory because it turns a tree of calls into a flat list of calls and
> keep the list until the end, while the recursive version only has to
> keep one call chain at a time.
>
> Any ideas how to improve it?
Well, fortunately JGit is under a very liberal BSD license and has a
pretty efficient version of a heap based algorithm for delta
resolution. You could look at [1] for ideas. I am told the BSD license
is very compatible with the GPLv2. (Unlike the reverse.)
[1] http://egit.eclipse.org/w/?p=jgit.git;a=blob;f=org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java;hb=HEAD#l556
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: introduce OPT_SUBCOMMAND
From: Ramkumar Ramachandra @ 2011-12-08 16:43 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <20111208163049.GA2116@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> 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
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Ramkumar Ramachandra @ 2011-12-08 16:47 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <20111208163946.GB2394@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> 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
^ permalink raw reply
* Re: [PATCH 3/6] t1006 (cat-file): use test_cmp
From: Jonathan Nieder @ 2011-12-08 16:55 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-6-git-send-email-artagnon@gmail.com>
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"
[...]
> - 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.
[...]
> @@ -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
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Jonathan Nieder @ 2011-12-08 17:03 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <CALkWK0mmjKSzSbxq2i7=JvcB4LTro-MYDCwQLUUwqcf8qS0zPA@mail.gmail.com>
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).
^ permalink raw reply
* Re: [PATCH 4/6] t3200 (branch): fix '&&' chaining
From: Jonathan Nieder @ 2011-12-08 17:07 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-7-git-send-email-artagnon@gmail.com>
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".
> 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>
^ permalink raw reply
* Re: [PATCH 5/6] t1510 (worktree): fix '&&' chaining
From: Jonathan Nieder @ 2011-12-08 17:12 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-8-git-send-email-artagnon@gmail.com>
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 '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?
>
> 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>
^ permalink raw reply
* Re: [PATCH 6/6] test: fix '&&' chaining
From: Jonathan Nieder @ 2011-12-08 17:18 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <1323349817-15737-9-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> 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>
^ permalink raw reply
* Re: [PATCH 2/2] bundle: rewrite builtin to use parse-options
From: Ramkumar Ramachandra @ 2011-12-08 17:38 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Matthieu Moy, Git List
In-Reply-To: <20111208170319.GD2394@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> 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
^ permalink raw reply
* Re: Query on git commit amend
From: Junio C Hamano @ 2011-12-08 17:52 UTC (permalink / raw)
To: Viresh Kumar
Cc: Björn Steinbrink, Vijay Lakshminarayanan,
git@vger.kernel.org, Shiraz HASHIM, Vipin KUMAR
In-Reply-To: <4EDEFD66.4020404@st.com>
Viresh Kumar <viresh.kumar@st.com> writes:
> GIT_EDITOR=cat git commit --amend
>
> over
>
> git commit --amend -C HEAD
>
> ?
>
> ...
> But for single commit probably second one looks easier. Isn't it?
I saw "--amend -C HEAD" mentioned in some newbie-guide webpages, and I
think you just copy/learned from one of them, so it is not entirely your
fault, but the combination of --amend and "-C HEAD" is an idiotic thing
that happens to work, if you think about what exactly you are telling to
the command.
The point of --amend is twofold. One is to let you tweak the contents of
what is committed, which is not the topic of this thread, and the other is
to allow the user to reuse the log message from the commit being amended,
instead of typing the message from scratch.
The "-c <commit>" and its cousin "-C <commit>" options are about telling
Git that the user does _not_ want the other usual logic to come up with
the initial commit template (e.g. when committing anew, it may read the
log template file, when committing a merge, it may read the MERGE_MSG
prepared by fmt-merge-msg, and most importantly in this context, when
amending, the one that is prepared by the --amend logic is used) kick in
at all, and instead wants to start from the log message of the named
commit.
So by saying "--amend -C HEAD" you are saying "I want to reuse the log
message of the commit I am amending,... eh, scratch that, I instead want
to use the log message of the HEAD commit", as if the commit you are
amending and HEAD are two different things. That is idiotic.
And you say that only for the side effect that capital "-C" stops the
editor.
Compared to that idiotic statement, "EDITOR=: git commit --amend" is a lot
saner way to say the same thing in a more direct and straightforward
way. "I want to reuse the log message of the commit I am amending, and the
editor to use it while running that commit is the command 'true', i.e. the
one that does not really touch any line in the text file and successfully
exits, because I am not going to change anything".
Of course, if "git commit --amend" honoured "--no-edit", that is even more
direct, straightforward and intuitive way to say so ;-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox