In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error
that had slipped by the test suites because of a missing check on 'git
read-tree -u -m HEAD'.

I mentioned to Johannes Schindelin that there were several bugs of this
type in git-filter-branch, and he suggested that I send a patch.

I've tested this patch using t/t7003-filter-branch.sh, and it passes all
the existing tests.  But it's entirely possible that this patch contains
errors, and I would love input from people who have more experience with
sh and who know more about git-filter-branch.

In particular, the following hunk may change the public UI to
git-filter-branch, although I'm not sure whether the change is for
better or for worse.  As I understand it, this hunk would allow
$filter_commit to abort the rewriting process by returning a non-0 exit
status:

 	@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"
 done <../revs

I'd be happy to add a test case for what happens when $filter_commit
returns a non-0 exit status.  Is the old behavior preferable?
---
 git-filter-branch.sh |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

 I'm trying to do the constructive thing, and send patches instead of bug
 reports. :-) -Eric

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86eef56..9d50978 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 || die "Can't back up refs"
 while read sha1 type name
 do
 	case "$force,$name" in
@@ -242,7 +242,7 @@ 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 ...
[ message continues ]

I think it's OK to die if the commit filter fails.

But generally, I think it is not necessary to use 'die with error
message', a plain '|| exit' should be enough because an error will have

Here you shouldn't die. But unlike elsewhere, this case warrants an
explanation for the user:

	git read-tree -u -m HEAD ||

-- Hannes
--

[ message continues ]

I take this back. I was distracted by the 'exit $ret' that is visible in

But this exit statement is pointless: ret is initialized to zero and never
changed.

-- Hannes
--

[ message continues ]

Hi,


Thanks.  Although it lacks a Sign-off, and part of the commit message is 


This will catch errors in the update-index call, but neither in diff-index 
nor ls-files.

Otherwise, the patch looks good to me.

Ciao,
Dscho
--

[ message continues ]

In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error
that had slipped by the test suites because of a missing check on 'git
read-tree -u -m HEAD'.

I mentioned to Johannes Schindelin that there were several bugs of this
type in git-filter-branch, and he suggested that I send a patch.  I've
tested this patch using t/t7003-filter-branch.sh, and it passes all the
existing tests.

In two places, I've had to break apart pipelines in order to check the
error code for the first stage of the pipeline, as discussed here:

  http://kerneltrap.org/mailarchive/git/2009/1/28/4835614

Thank you to charon on #git for pointing me in the right direction.

This patch causes 'git filter-branch' to fail if the --commit-filter
argument returns an error.  A test case for this behavior is included.

Feedback on the original version of this patch was provided by Johannes
Sixt and Johannes Schindelin.

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

Signed-off-by: Eric Kidd <git@randomhacks.net>
---
 git-filter-branch.sh     |   26 ++++++++++++++------------
 t/t7003-filter-branch.sh |    4 ++++
 2 files changed, 18 insertions(+), 12 deletions(-)

 Thank you for the feedback!  I've tried to incorporate everybody's
 suggestions.  Please let me know if some of those 'exit 1' statements
 should be changed back to 'die'. -Eric

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
 while read sha1 type name
 do
 	case "$force,$name" in
@@ -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 ...
[ message continues ]

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

That's a very good start for the description of the solution, which would

Giving credits to others like this with a short sentence at the end is

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.





Hmm, wouldn't commit-tree have issued its own error message already?  If



"test_must_fail git ..." would be better here than "! git ...".
--

[ message continues ]

If a custom $filter_commit calls 'exit', there won't be an error
message from git. Of course, there may be an error message from the
custom $filter_commit, but I decided not to rely on that. Would you
prefer me to remove the error message?

I'll have another patch ready shortly, incorporating your suggestions.
My apologies for giving credit in the wrong part of the commit
message, and thank you for your feedback!

Cheers,
Eric
--

[ message continues ]

In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error
that had slipped by the test suites because of a missing check on 'git
read-tree -u -m HEAD'.

This patch attemps to add all the missing error checks to
git-filter-branch, and removes an existing $ret variable that did
nothing.  I've tested this patch using t/t7003-filter-branch.sh, and it
passes all the existing tests.

This patch also causes 'git filter-branch' to fail if the --commit-filter
argument returns an error.  A test case for this behavior is included.

In two places, I've had to break apart pipelines in order to check the
error code for the first stage of the pipeline, as discussed here:

  http://kerneltrap.org/mailarchive/git/2009/1/28/4835614

Feedback on this patch was provided by Johannes Sixt, Johannes Schindelin
and Junio C Hamano.  charon on #git helped with pipeline error handling.

Signed-off-by: Eric Kidd <git@randomhacks.net>

---
 git-filter-branch.sh     |   26 ++++++++++++++------------
 t/t7003-filter-branch.sh |    4 ++++
 2 files changed, 18 insertions(+), 12 deletions(-)

v3:
  Replaced 'exit 1' with 'exit' to use exit status of last command
  Use test_must_fail for unit test expecting failure

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

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86eef56..27b57b8 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
 while read sha1 type name
 do
 	case "$force,$name" in
@@ -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 ...
[ message continues ]

Hi,



I haven't checked, but is "$tempdir" not the working directory?  If so, 
this would lead to funny interaction with --tree-filter.  Rather, I'd 

That's not how it is supposed to be used.  Rather,

	test_expect_success $LABEL '
		test_must_fail git filter-branc $OPTIONS
	'

Thanks,
Dscho
--

[ message continues ]

On Wed, Feb 11, 2009 at 3:48 PM, Johannes Schindelin

The working directory actually lives one level down:

 tempdir=.git-rewrite
 workdir="$tempdir/t"

At the end of the script, git-filter-branch cleans up all of its
temporary files by deleting $tempdir. There's actually a fair bit of

Will fix. Thank you.

I really appreciate all this feedback from the git team. Thank you for
taking the time to help me get this right!

Cheers,
Eric
--

[ message continues ]

In commit 9273b56278e64dd47b1a96a705ddf46aeaf6afe3, I fixed an error
that had slipped by the test suites because of a missing check on 'git
read-tree -u -m HEAD'.

This patch attemps to add all the missing error checks to
git-filter-branch, and removes an existing $ret variable that did
nothing.  I've tested this patch using t/t7003-filter-branch.sh, and it
passes all the existing tests.

This patch also causes 'git filter-branch' to fail if the --commit-filter
argument returns an error.  A test case for this behavior is included.

In two places, I've had to break apart pipelines in order to check the
error code for the first stage of the pipeline, as discussed here:

  http://kerneltrap.org/mailarchive/git/2009/1/28/4835614

Feedback on this patch was provided by Johannes Sixt, Johannes Schindelin
and Junio C Hamano.  Thomas Rast helped with pipeline error handling.

Signed-off-by: Eric Kidd <git@randomhacks.net>

---
 git-filter-branch.sh     |   26 ++++++++++++++------------
 t/t7003-filter-branch.sh |    4 ++++
 2 files changed, 18 insertions(+), 12 deletions(-)

v4:
  Call test_must_fail from inside test_expect_success

v3:
  Replaced 'exit 1' with 'exit' to use exit status of last command
  Use test_must_fail for unit test expecting failure

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

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86eef56..27b57b8 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
 while read sha1 type name
 do
 	case "$force,$name" in
@@ -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 ...
[ message continues ]

I think you meant this as a sample to follow.  Can we add it to Documentation/SubmittingPatches?

-- 
Nanako Shiraishi, an unofficial project secretary
http://ivory.ap.teacup.com/nanako3/

--

[ message continues ]

I did mean it as such, but I doubt it is good enough to be in in the
document (primarily because I wrote it).

Just quoting the above verbatim does not make it clear that "Thanks for M,
N..." is usually not even wanted, but was merely a suggestion for this
specific case of Eric's commit, iow, _only if he wanted to_.  We need more
commentary like that, but with too much details, it would cease to be a
generic recommendation.

Also, the above is not suitable for new features at all as a template.
--

[ message continues ]