Re: [PATCH v2] filter-branch: Add more error-handling
flat view
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:09
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH v2] filter-branch: Add more error-handling · Eric Kidd <hidden>
- 2016-06-15 · Re: [PATCH v2] filter-branch: Add more error-handling · Junio C Hamano <hidden>
Eric Kidd [off-list ref] writes:
In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error ... Thank you to charon on #git for pointing me in the right direction.
The commit message is not a reception speech at Emmy Awards. If you want to do the speech, do so after the three-dash lines. Please just stick to what problem it tries to solve, how it does so, and what the outcome is.
This patch causes 'git filter-branch' to fail if the --commit-filter argument returns an error. A test case for this behavior is included.
That's a very good start for the description of the solution, which would be for the second paragraph. The problem description is missing.
Feedback on the original version of this patch was provided by Johannes Sixt and Johannes Schindelin.
Giving credits to others like this with a short sentence at the end is fine.
v2: Remove useless $ret variable Correctly check the first command in a pipeline, not the second Replace verbose 'die' messages with 'exit 1' in most cases
This goes after three-dashes; people who read "git log" output wouldn't
know nor care what was in v1.
Subject: Fix X under condition Z
X should do Y if condition Z holds, but it does not. This can result
in broken results such as W and V.
This patch fixes X by changing A, B and C.
Thanks for M, N and O for reviewing and suggesting improvements.
Signed-off-by: A U Thor [off-list ref]
quoted hunk
diff --git a/git-filter-branch.sh b/git-filter-branch.sh index 86eef56..fff07c8 100755 --- a/git-filter-branch.sh +++ b/git-filter-branch.sh@@ -221,7 +221,7 @@ die "" trap 'cd ../..; rm -rf "$tempdir"' 0 # Make sure refs/original is empty -git for-each-ref > "$tempdir"/backup-refs +git for-each-ref > "$tempdir"/backup-refs || exit 1
Why "exit 1", not "exit"?
quoted hunk
@@ -241,8 +241,9 @@ GIT_WORK_TREE=. export GIT_DIR GIT_WORK_TREE # The refs should be updated if their heads were rewritten -git rev-parse --no-flags --revs-only --symbolic-full-name --default HEAD "$@" | -sed -e '/^^/d' >"$tempdir"/heads +git rev-parse --no-flags --revs-only --symbolic-full-name \ + --default HEAD "$@" > "$tempdir"/raw-heads || exit 1
Likewise.
quoted hunk
@@ -315,10 +314,11 @@ while read commit parents; do die "tree filter failed: $filter_tree" ( - git diff-index -r --name-only $commit + git diff-index -r --name-only $commit && git ls-files --others - ) | - git update-index --add --replace --remove --stdin + ) > "$tempdir"/tree-state || exit 1 + git update-index --add --replace --remove --stdin \ + < "$tempdir"/tree-state || exit 1
Likewise.
quoted hunk
@@ -339,7 +339,8 @@ while read commit parents; do eval "$filter_msg" > ../message || die "msg filter failed: $filter_msg" @SHELL_PATH@ -c "$filter_commit" "git commit-tree" \ - $(git write-tree) $parentstr < ../message > ../map/$commit + $(git write-tree) $parentstr < ../message > ../map/$commit || + die "could not write rewritten commit"
Hmm, wouldn't commit-tree have issued its own error message already? If redirect failed, then the shell would have.
quoted hunk
@@ -407,7 +408,8 @@ do die "Could not rewrite $ref" ;; esac - git update-ref -m "filter-branch: backup" "$orig_namespace$ref" $sha1 + git update-ref -m "filter-branch: backup" "$orig_namespace$ref" $sha1 || + exit 1
Why "exit 1", not "exit"?
quoted hunk
@@ -483,7 +485,7 @@ test -z "$ORIG_GIT_INDEX_FILE" || { } if [ "$(is_bare_repository)" = false ]; then - git read-tree -u -m HEAD + git read-tree -u -m HEAD || exit 1
Likewise
quoted hunk
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh index cb04743..39affd9 100755 --- a/t/t7003-filter-branch.sh +++ b/t/t7003-filter-branch.sh@@ -48,6 +48,10 @@ test_expect_success 'result is really identical' ' test $H = $(git rev-parse HEAD) ' +test_expect_success 'Fail if commit filter fails' ' + ! git filter-branch -f --commit-filter "exit 1" HEAD +' +
"test_must_fail git ..." would be better here than "! git ...".