[PATCH] filter-branch: consider refs/replace can refer to an object other than commit

[PATCH] filter-branch: consider refs/replace can refer to an object other than commit

[Yuki Kokubun]

"git filter-branch -- --all" can be confused when refs that refer to objects
other than commits exists.
Because "git rev-parse --all" that is internally used can return refs that
refer to an object other than commit. But it is not considered in the phase of
updating refs. Such refs can be created by "git replace" with objects other than
commits.

Signed-off-by: Yuki Kokubun <orga.chem.job@gmail.com>
---
 git-filter-branch.sh     |  3 +++
 t/t7003-filter-branch.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..a9fafe64f 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -479,6 +479,9 @@ do
 	# avoid rewriting a ref twice
 	test -f "$orig_namespace$ref" && continue
 
+	# refs/replace can point to a non commit object
+	test $(git cat-file -t "$ref") = commit || continue
+
 	sha1=$(git rev-parse "$ref"^0)
 	rewritten=$(map $sha1)
 
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 7cb60799b..efeaf5887 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -470,4 +470,17 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
 	git show HEAD:$ambiguous
 '
 
+test_expect_success 'rewrite repository including refs/replace that point to non commit object' '
+	test_when_finished "git reset --hard original" &&
+	tree=$(git rev-parse HEAD^{tree}) &&
+	test_when_finished "git replace -d $tree" &&
+	echo A >new &&
+	git add new &&
+	new_tree=$(git write-tree) &&
+	git replace $tree $new_tree &&
+	git reset --hard HEAD &&
+	git filter-branch -f -- --all >filter-output 2>&1 &&
+	! fgrep fatal filter-output
+'
+
 test_done
-- 
2.16.2.18.g09cb46d

[PATCH] filter-branch: consider refs can refer to an object other than commit or tag

[Yuki Kokubun]

"git filter-branch -- --all" can be confused when refs that refer to objects
other than commits or tags exists.
Because "git rev-parse --all" that is internally used can return refs that
refer to an object other than commit or tag. But it is not considered in the
phase of updating refs. Such refs can be created by "git replace" with
objects other than commits or trees.

Signed-off-by: Yuki Kokubun <orga.chem.job@gmail.com>
---
This patch fix the bug of the first patch.
The first patch did not consider tags.

 git-filter-branch.sh     | 11 ++++++++++-
 t/t7003-filter-branch.sh | 13 +++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..f7cd97b86 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -251,7 +251,16 @@ done < "$tempdir"/backup-refs
 
 # The refs should be updated if their heads were rewritten
 git rev-parse --no-flags --revs-only --symbolic-full-name \
-	--default HEAD "$@" > "$tempdir"/raw-heads || exit
+	--default HEAD "$@" > "$tempdir"/raw-objects || exit
+# refs/replace can refer to an object other than commit or tag
+while read ref
+do
+	type=$(git cat-file -t "$ref")
+	if test $type = commit || test $type = tag
+	then
+		echo "$ref"
+	fi
+done >"$tempdir"/raw-heads <"$tempdir"/raw-objects
 sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
 
 test -s "$tempdir"/heads ||
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 7cb60799b..efeaf5887 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -470,4 +470,17 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
 	git show HEAD:$ambiguous
 '
 
+test_expect_success 'rewrite repository including refs/replace that point to non commit object' '
+	test_when_finished "git reset --hard original" &&
+	tree=$(git rev-parse HEAD^{tree}) &&
+	test_when_finished "git replace -d $tree" &&
+	echo A >new &&
+	git add new &&
+	new_tree=$(git write-tree) &&
+	git replace $tree $new_tree &&
+	git reset --hard HEAD &&
+	git filter-branch -f -- --all >filter-output 2>&1 &&
+	! fgrep fatal filter-output
+'
+
 test_done
-- 
2.16.2.18.g09cb46d

Re: [PATCH] filter-branch: consider refs can refer to an object other than commit or tag

[Junio C Hamano]

Yuki Kokubun <orga.chem.job@gmail.com> writes:

> "git filter-branch -- --all" can be confused when refs that refer to objects
> other than commits or tags exists.
> Because "git rev-parse --all" that is internally used can return refs that
> refer to an object other than commit or tag. But it is not considered in the
> phase of updating refs.

Could you describe what the consequence of that is?  We have a ref
that points directly at a blob object, or a ref that points at a tag
object that points at a blob object.  The current code leaves both of
these refs in "$tempdir/heads".  Then...?

	... goes and looks ...

There is a loop that looks like this:

	while read ref
	do
		sha1=$(git rev-parse "$ref^0")
		...
	done <"$tempdir/heads"

which would break on anything but a commit-ish.

>  # The refs should be updated if their heads were rewritten
>  git rev-parse --no-flags --revs-only --symbolic-full-name \
> -	--default HEAD "$@" > "$tempdir"/raw-heads || exit
> +	--default HEAD "$@" > "$tempdir"/raw-objects || exit
> +# refs/replace can refer to an object other than commit or tag

Mention of replace refs in the proposed log message gives an easy to
understand example and is a good idea, but this in code comment does
not have to single out the replace refs.  A tag can also point at an
object with any type, e.g. "git tag v2.6.11-tree v2.6.11^{tree}"
would make "refs/tags/v2.6.11-tree" point at the tree at the top
level of the tree-ish "v2.6.11".  It probably is OK to drop this
comment altogether.

> +while read ref
> +do
> +	type=$(git cat-file -t "$ref")
> +	if test $type = commit || test $type = tag
> +	then
> +		echo "$ref"
> +	fi
> +done >"$tempdir"/raw-heads <"$tempdir"/raw-objects
>  sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads

So... is the idea to limit the set of refs to be rewritten to those
that point at commits and tags?  As I already alluded to, I do not
think you want to accept a ref that points at any tag object---only
the ones that point at a tag that points at a commit-ish, so that
the code will not barf when doing "$ref^0".

So perhaps

	git rev-parse --no-flags ... >"$tempdir/raw-heads" || exit

	while read ref
	do
		case "$ref" in ^?*) continue ;; esac
		if git rev-parse --verify "$ref^0" 2>/dev/null
                then
			echo "$ref"
		fi
	done >"$tempdir/heads" <"$tempdir/raw-heads"

or something?  Note that you do not need the "sed" as the loop
already excludes the negative revs.

>  test -s "$tempdir"/heads ||
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index 7cb60799b..efeaf5887 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -470,4 +470,17 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
>  	git show HEAD:$ambiguous
>  '
>  
> +test_expect_success 'rewrite repository including refs/replace that point to non commit object' '
> +	test_when_finished "git reset --hard original" &&
> +	tree=$(git rev-parse HEAD^{tree}) &&
> +	test_when_finished "git replace -d $tree" &&
> +	echo A >new &&
> +	git add new &&
> +	new_tree=$(git write-tree) &&
> +	git replace $tree $new_tree &&

Perhaps something like this here:

	git tag -a "tag to a tree" treetag $new_tree &&

can tell su how well it works with a tag that points at a tree?

> +	git reset --hard HEAD &&
> +	git filter-branch -f -- --all >filter-output 2>&1 &&
> +	! fgrep fatal filter-output
> +'
> +
>  test_done

Re: [PATCH] filter-branch: consider refs can refer to an object other than commit or tag

[Yuki Kokubun]

> Yuki Kokubun <orga.chem.job@gmail.com> writes:
> 
> > "git filter-branch -- --all" can be confused when refs that refer to objects
> > other than commits or tags exists.
> > Because "git rev-parse --all" that is internally used can return refs that
> > refer to an object other than commit or tag. But it is not considered in the
> > phase of updating refs.
> 
> Could you describe what the consequence of that is?  We have a ref
> that points directly at a blob object, or a ref that points at a tag
> object that points at a blob object.  The current code leaves both of
> these refs in "$tempdir/heads".  Then...?

Sorry, this is my wrong.
I wrongly thought only refs/replace can point at a blob or tree object.

> 
> 	... goes and looks ...
> 
> There is a loop that looks like this:
> 
> 	while read ref
> 	do
> 		sha1=$(git rev-parse "$ref^0")
> 		...
> 	done <"$tempdir/heads"
> 
> which would break on anything but a commit-ish.
> 
> >  # The refs should be updated if their heads were rewritten
> >  git rev-parse --no-flags --revs-only --symbolic-full-name \
> > -	--default HEAD "$@" > "$tempdir"/raw-heads || exit
> > +	--default HEAD "$@" > "$tempdir"/raw-objects || exit
> > +# refs/replace can refer to an object other than commit or tag
> 
> Mention of replace refs in the proposed log message gives an easy to
> understand example and is a good idea, but this in code comment does
> not have to single out the replace refs.  A tag can also point at an
> object with any type, e.g. "git tag v2.6.11-tree v2.6.11^{tree}"
> would make "refs/tags/v2.6.11-tree" point at the tree at the top
> level of the tree-ish "v2.6.11".  It probably is OK to drop this
> comment altogether.

OK, I'm gonna drop the incorrect comment.

> 
> > +while read ref
> > +do
> > +	type=$(git cat-file -t "$ref")
> > +	if test $type = commit || test $type = tag
> > +	then
> > +		echo "$ref"
> > +	fi
> > +done >"$tempdir"/raw-heads <"$tempdir"/raw-objects
> >  sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
> 
> So... is the idea to limit the set of refs to be rewritten to those
> that point at commits and tags?  As I already alluded to, I do not
> think you want to accept a ref that points at any tag object---only
> the ones that point at a tag that points at a commit-ish, so that
> the code will not barf when doing "$ref^0".
> 
> So perhaps
> 
> 	git rev-parse --no-flags ... >"$tempdir/raw-heads" || exit
> 
> 	while read ref
> 	do
> 		case "$ref" in ^?*) continue ;; esac
> 		if git rev-parse --verify "$ref^0" 2>/dev/null
>                 then
> 			echo "$ref"
> 		fi
> 	done >"$tempdir/heads" <"$tempdir/raw-heads"
> 
> or something?  Note that you do not need the "sed" as the loop
> already excludes the negative revs.

I feel using "git rev-parse --verify" is a good way as you said.
I'm gonna modify the patch to use it.

> 
> >  test -s "$tempdir"/heads ||
> > diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> > index 7cb60799b..efeaf5887 100755
> > --- a/t/t7003-filter-branch.sh
> > +++ b/t/t7003-filter-branch.sh
> > @@ -470,4 +470,17 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
> >  	git show HEAD:$ambiguous
> >  '
> >  
> > +test_expect_success 'rewrite repository including refs/replace that point to non commit object' '
> > +	test_when_finished "git reset --hard original" &&
> > +	tree=$(git rev-parse HEAD^{tree}) &&
> > +	test_when_finished "git replace -d $tree" &&
> > +	echo A >new &&
> > +	git add new &&
> > +	new_tree=$(git write-tree) &&
> > +	git replace $tree $new_tree &&
> 
> Perhaps something like this here:
> 
> 	git tag -a "tag to a tree" treetag $new_tree &&
> 
> can tell su how well it works with a tag that points at a tree?

Sounds good. I'm gonna add such tags to the test case.

> 
> > +	git reset --hard HEAD &&
> > +	git filter-branch -f -- --all >filter-output 2>&1 &&
> > +	! fgrep fatal filter-output
> > +'
> > +
> >  test_done

Re: [PATCH] filter-branch: consider refs can refer to an object other than commit or tag

[Junio C Hamano]

Yuki Kokubun <orga.chem.job@gmail.com> writes:

>> Yuki Kokubun <orga.chem.job@gmail.com> writes:
>> 
>> > "git filter-branch -- --all" can be confused when refs that refer to objects
>> > other than commits or tags exists.
>> > Because "git rev-parse --all" that is internally used can return refs that
>> > refer to an object other than commit or tag. But it is not considered in the
>> > phase of updating refs.
>> 
>> Could you describe what the consequence of that is?  We have a ref
>> that points directly at a blob object, or a ref that points at a tag
>> object that points at a blob object.  The current code leaves both of
>> these refs in "$tempdir/heads".  Then...?
>
> Sorry, this is my wrong.
> I wrongly thought only refs/replace can point at a blob or tree object.

No need to be sorry.  You still need to describe what (bad things)
happen if we do not filter out refs that do not point at committish
in the proposed log message.  

IOW, can you elaborate and clarify your "can be confused" at the
beginning?

Re: [PATCH] filter-branch: consider refs can refer to an object other than commit or tag

[Yuki Kokubun]

> Yuki Kokubun <orga.chem.job@gmail.com> writes:
> 
> >> Yuki Kokubun <orga.chem.job@gmail.com> writes:
> >> 
> >> > "git filter-branch -- --all" can be confused when refs that refer to objects
> >> > other than commits or tags exists.
> >> > Because "git rev-parse --all" that is internally used can return refs that
> >> > refer to an object other than commit or tag. But it is not considered in the
> >> > phase of updating refs.
> >> 
> >> Could you describe what the consequence of that is?  We have a ref
> >> that points directly at a blob object, or a ref that points at a tag
> >> object that points at a blob object.  The current code leaves both of
> >> these refs in "$tempdir/heads".  Then...?
> >
> > Sorry, this is my wrong.
> > I wrongly thought only refs/replace can point at a blob or tree object.
> 
> No need to be sorry.  You still need to describe what (bad things)
> happen if we do not filter out refs that do not point at committish
> in the proposed log message.  
> 
> IOW, can you elaborate and clarify your "can be confused" at the
> beginning?

I meant the confusion is abnormal messages from the output of "git filter-branch -- --all".
For example, this is an output of "git filter-branch -- --all":

Rewrite bcdbd016c77df3d5641a3cf820b2ed46ba7bf3b4 (5/5) (0 seconds passed, remaining 0 predicted)
WARNING: Ref 'refs/heads/master' is unchanged
WARNING: Ref 'refs/heads/no-newline' is unchanged
WARNING: Ref 'refs/heads/original' is unchanged
error: object 1bf53b49c26465454e4ac377f2ed3f91bb1d6ac1 is a tree, not a commit
error: object 1bf53b49c26465454e4ac377f2ed3f91bb1d6ac1 is a tree, not a commit
fatal: ambiguous argument 'refs/replace/8a2016f3730cad8309c110f819c855403ed0a5b9^0': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
WARNING: Ref 'refs/replace/8a2016f3730cad8309c110f819c855403ed0a5b9' is unchanged
WARNING: Ref 'refs/tags/add-file' is unchanged
WARNING: Ref 'refs/tags/file' is unchanged
error: object 1bf53b49c26465454e4ac377f2ed3f91bb1d6ac1 is a tree, not a commit
error: object 1bf53b49c26465454e4ac377f2ed3f91bb1d6ac1 is a tree, not a commit
fatal: ambiguous argument 'refs/tags/treetag^0': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
WARNING: Ref 'refs/tags/treetag' is unchanged

You can see a lot of terrible messages such as "error" and "fatal".
But on the whole, the result of "git filter-branch -- --all" is not so abnormal.
So, this is a just problem about abonormal messages.

I think this messages should be suppressed.
How do you feel about it?

Re: [PATCH] filter-branch: consider refs can refer to an object other than commit or tag

[Junio C Hamano]

Yuki Kokubun <orga.chem.job@gmail.com> writes:

>> Yuki Kokubun <orga.chem.job@gmail.com> writes:
>> 
>> >> Yuki Kokubun <orga.chem.job@gmail.com> writes:
>> >> 
>> >> > "git filter-branch -- --all" can be confused when refs that refer to objects
>> >> > other than commits or tags exists.
>> ...
>
> I meant the confusion is abnormal messages from the output of "git filter-branch -- --all".

OK, so it is not that the program logic gets confused and ends up
performing a wrong rewrite, but mostly that it gives confusing
messages.

> For example, this is an output of "git filter-branch -- --all":
>
> Rewrite bcdbd016c77df3d5641a3cf820b2ed46ba7bf3b4 (5/5) (0 seconds passed, remaining 0 predicted)
> WARNING: Ref 'refs/heads/master' is unchanged
> WARNING: Ref 'refs/heads/no-newline' is unchanged
> WARNING: Ref 'refs/heads/original' is unchanged

These are worth keeping, as I think existing users expect to see
them.

> error: object 1bf53b49c26465454e4ac377f2ed3f91bb1d6ac1 is a tree, not a commit
> error: object 1bf53b49c26465454e4ac377f2ed3f91bb1d6ac1 is a tree, not a commit
> fatal: ambiguous argument 'refs/replace/8a2016f3730cad8309c110f819c855403ed0a5b9^0': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'
> WARNING: Ref 'refs/replace/8a2016f3730cad8309c110f819c855403ed0a5b9' is unchanged
> WARNING: Ref 'refs/tags/add-file' is unchanged
> WARNING: Ref 'refs/tags/file' is unchanged
> error: object 1bf53b49c26465454e4ac377f2ed3f91bb1d6ac1 is a tree, not a commit
> error: object 1bf53b49c26465454e4ac377f2ed3f91bb1d6ac1 is a tree, not a commit
> fatal: ambiguous argument 'refs/tags/treetag^0': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'
> WARNING: Ref 'refs/tags/treetag' is unchanged

I think these warning messages should be kept, especially if we are
to keep the warning messages for the unchanged branches.  However,
the internal error messages are unwanted--these are implementation
details that reach the conclusion, i.e. the ref we were asked to
rewrite ended up being unchanged hence we did not touch it.

However, if we pre-filter to limit the refs in "$tempdir/heads" to
those that are committish (i.e. those that pass "$ref^0") like the
patch and subsequent discussion suggests, wouldn't we lose the
warning for these replace refs and non-committish tags.  We perhaps
could do something like:

	git rev-parse --no-flags ... >"$tempdir/raw-heads" || exit

	while read ref
	do
		case "$ref" in ^?*) continue ;; esac
		if git rev-parse --verify "$ref^0" 2>/dev/null
                then
			echo "$ref"
		else
			warn "WARNING: not rewriting '$ref' (not a committish)"
		fi
	done >"$tempdir/heads" <"$tempdir/raw-heads"

(note: the else clause is new, relative to my earlier suggestion).

Re: [PATCH] filter-branch: consider refs can refer to an object other than commit or tag

[Yuki Kokubun]

> However, if we pre-filter to limit the refs in "$tempdir/heads" to
> those that are committish (i.e. those that pass "$ref^0") like the
> patch and subsequent discussion suggests, wouldn't we lose the
> warning for these replace refs and non-committish tags.  We perhaps
> could do something like:
> 
> 	git rev-parse --no-flags ... >"$tempdir/raw-heads" || exit
> 
> 	while read ref
> 	do
> 		case "$ref" in ^?*) continue ;; esac
> 		if git rev-parse --verify "$ref^0" 2>/dev/null
>                 then
> 			echo "$ref"
> 		else
> 			warn "WARNING: not rewriting '$ref' (not a committish)"
> 		fi
> 	done >"$tempdir/heads" <"$tempdir/raw-heads"
> 
> (note: the else clause is new, relative to my earlier suggestion).

I agree these suggestions.
I'm gonna send a new patch that follow it.

[PATCH v2] filter-branch: fix errors caused by refs that cannot be used with ^0

[Yuki Kokubun]

"git filter-branch -- --all" print unwanted error messages when refs that
cannot be used with ^0 exist. Such refs can be created by "git replace" with
trees or blobs. Also, "git tag" with trees or blobs can create such refs.
---
 git-filter-branch.sh     | 14 ++++++++++++--
 t/t7003-filter-branch.sh | 14 ++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..41efecb28 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -251,8 +251,18 @@ done < "$tempdir"/backup-refs
 
 # The refs should be updated if their heads were rewritten
 git rev-parse --no-flags --revs-only --symbolic-full-name \
-	--default HEAD "$@" > "$tempdir"/raw-heads || exit
-sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
+	--default HEAD "$@" > "$tempdir"/raw-refs || exit
+while read ref
+do
+	case "$ref" in ^?*) continue ;; esac
+
+	if git rev-parse --verify "$ref"^0 >/dev/null 2>&1
+	then
+		echo "$ref"
+	else
+		warn "WARNING: not rewriting '$ref' (not a committish)"
+	fi
+done >"$tempdir"/heads <"$tempdir"/raw-refs
 
 test -s "$tempdir"/heads ||
 	die "You must specify a ref to rewrite."
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 7cb60799b..04f79f32b 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -470,4 +470,18 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
 	git show HEAD:$ambiguous
 '
 
+test_expect_success 'rewrite repository including refs that point at non-commit object' '
+	test_when_finished "git reset --hard original" &&
+	tree=$(git rev-parse HEAD^{tree}) &&
+	test_when_finished "git replace -d $tree" &&
+	echo A >new &&
+	git add new &&
+	new_tree=$(git write-tree) &&
+	git replace $tree $new_tree &&
+	git tag -a -m "tag to a tree" treetag $new_tree &&
+	git reset --hard HEAD &&
+	git filter-branch -f -- --all >filter-output 2>&1 &&
+	! fgrep fatal filter-output
+'
+
 test_done
-- 
2.16.2.18.g09cb46d

Re: [PATCH v2] filter-branch: fix errors caused by refs that cannot be used with ^0

[Junio C Hamano]

Yuki Kokubun <orga.chem.job@gmail.com> writes:

> "git filter-branch -- --all" print unwanted error messages when refs that
> cannot be used with ^0 exist.

It is not incorrect per-se, but if I were writing this, I'd say
"... when refs that point at objects that are not committish" or
something like that, as that is much closer to people (both end
users and Git developers) than the "^0" implementation detail.

> Such refs can be created by "git replace" with
> trees or blobs. Also, "git tag" with trees or blobs can create such refs.

Taking two paragraphs above together, you wrote an excellent
description of the problem to be solved (i.e. what would be seen by
users and under what condition the symptom would trigger).  If there
is a single obvious solution that would trivially follow from the
problem description, it is perfectly fine to stop here.  Otherwise,
it would help to describe how it is solved next.  Something as brief
like

	Filter these problematic refs out early, and warn that these
	refs are left unwritten while doing so.

would suffice in this case, I think.  We _could_ insert

	before they are seen by the logic to see which refs have
	been modified and which have been left intact (which is
	where the unwanted error messages come from),

between "early," and "and warn", if we wanted to.

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

The diff looks good.  

Please sign-off your patch (cf. Documentation/SubmittingPatches).

Thanks.

[PATCH v3] filter-branch: fix errors caused by refs that point at non-committish

[Yuki Kokubun]

"git filter-branch -- --all" print error messages when refs that point at
objects that are not committish. Such refs can be created by "git replace" with
trees or blobs. And also "git tag" with trees or blobs can create such refs.

Filter these problematic refs out early, before they are seen by the logic to
see which refs have been modified and which have been left intact (which is
where the unwanted error messages come from), and warn that these refs are left
unwritten while doing so.
---
 git-filter-branch.sh     | 14 ++++++++++++--
 t/t7003-filter-branch.sh | 14 ++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2..41efecb 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -251,8 +251,18 @@ done < "$tempdir"/backup-refs
 
 # The refs should be updated if their heads were rewritten
 git rev-parse --no-flags --revs-only --symbolic-full-name \
-	--default HEAD "$@" > "$tempdir"/raw-heads || exit
-sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
+	--default HEAD "$@" > "$tempdir"/raw-refs || exit
+while read ref
+do
+	case "$ref" in ^?*) continue ;; esac
+
+	if git rev-parse --verify "$ref"^0 >/dev/null 2>&1
+	then
+		echo "$ref"
+	else
+		warn "WARNING: not rewriting '$ref' (not a committish)"
+	fi
+done >"$tempdir"/heads <"$tempdir"/raw-refs
 
 test -s "$tempdir"/heads ||
 	die "You must specify a ref to rewrite."
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 7cb6079..04f79f3 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -470,4 +470,18 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
 	git show HEAD:$ambiguous
 '
 
+test_expect_success 'rewrite repository including refs that point at non-commit object' '
+	test_when_finished "git reset --hard original" &&
+	tree=$(git rev-parse HEAD^{tree}) &&
+	test_when_finished "git replace -d $tree" &&
+	echo A >new &&
+	git add new &&
+	new_tree=$(git write-tree) &&
+	git replace $tree $new_tree &&
+	git tag -a -m "tag to a tree" treetag $new_tree &&
+	git reset --hard HEAD &&
+	git filter-branch -f -- --all >filter-output 2>&1 &&
+	! fgrep fatal filter-output
+'
+
 test_done
-- 
1.9.1

Re: [PATCH v3] filter-branch: fix errors caused by refs that point at non-committish

[Junio C Hamano]

Yuki Kokubun <orga.chem.job@gmail.com> writes:

> "git filter-branch -- --all" print error messages when refs that point at
> objects that are not committish.

Grammo (third-person singular 'prints' misspelt without 's'; the
"when" clause has a complex subject but no verb).

Perhaps this will salvage what you meant:

	"git filter-branch -- --all" prints error messages when
	processing refs that point at objects that are not
	committish.

> Such refs can be created by "git replace" with
> trees or blobs. And also "git tag" with trees or blobs can create such refs.
>
> Filter these problematic refs out early, before they are seen by the logic to
> see which refs have been modified and which have been left intact (which is
> where the unwanted error messages come from), and warn that these refs are left
> unwritten while doing so.
> ---

Please sign-off your patch (cf. Documentation/SubmittingPatches).

Otherwise this round looks good.

Thanks.

>  git-filter-branch.sh     | 14 ++++++++++++--
>  t/t7003-filter-branch.sh | 14 ++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 1b7e4b2..41efecb 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -251,8 +251,18 @@ done < "$tempdir"/backup-refs
>  
>  # The refs should be updated if their heads were rewritten
>  git rev-parse --no-flags --revs-only --symbolic-full-name \
> -	--default HEAD "$@" > "$tempdir"/raw-heads || exit
> -sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
> +	--default HEAD "$@" > "$tempdir"/raw-refs || exit
> +while read ref
> +do
> +	case "$ref" in ^?*) continue ;; esac
> +
> +	if git rev-parse --verify "$ref"^0 >/dev/null 2>&1
> +	then
> +		echo "$ref"
> +	else
> +		warn "WARNING: not rewriting '$ref' (not a committish)"
> +	fi
> +done >"$tempdir"/heads <"$tempdir"/raw-refs
>  
>  test -s "$tempdir"/heads ||
>  	die "You must specify a ref to rewrite."
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index 7cb6079..04f79f3 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -470,4 +470,18 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
>  	git show HEAD:$ambiguous
>  '
>  
> +test_expect_success 'rewrite repository including refs that point at non-commit object' '
> +	test_when_finished "git reset --hard original" &&
> +	tree=$(git rev-parse HEAD^{tree}) &&
> +	test_when_finished "git replace -d $tree" &&
> +	echo A >new &&
> +	git add new &&
> +	new_tree=$(git write-tree) &&
> +	git replace $tree $new_tree &&
> +	git tag -a -m "tag to a tree" treetag $new_tree &&
> +	git reset --hard HEAD &&
> +	git filter-branch -f -- --all >filter-output 2>&1 &&
> +	! fgrep fatal filter-output
> +'
> +
>  test_done

Re: [PATCH v3] filter-branch: fix errors caused by refs that point at non-committish

[Yuki Kokubun]

References: <xmqqr2o8f7hd.fsf@gitster-ct.c.googlers.com>
	<xmqqvadmilx5.fsf@gitster-ct.c.googlers.com>
	<5ab46520.0352650a.cc02b.a177@mx.google.com>
	<20180323050913.5188-1-orga.chem.job@gmail.com> 
Content-Type: text/plain

> Grammo (third-person singular 'prints' misspelt without 's'; the
> "when" clause has a complex subject but no verb).
> 
> Perhaps this will salvage what you meant:
> 
> 	"git filter-branch -- --all" prints error messages when
> 	processing refs that point at objects that are not
> 	committish.

Thanks. I'm gonna fix it.

> Please sign-off your patch (cf. Documentation/SubmittingPatches).

OK I'm gonna add it.

I greatly appreciate for your kind review.
I couldn't write this patch without your help.
Thanks.

Re: [PATCH v3] filter-branch: fix errors caused by refs that point at non-committish

[Junio C Hamano]

Yuki Kokubun <orga.chem.job@gmail.com> writes:

> References: <xmqqr2o8f7hd.fsf@gitster-ct.c.googlers.com>
> 	<xmqqvadmilx5.fsf@gitster-ct.c.googlers.com>
> 	<5ab46520.0352650a.cc02b.a177@mx.google.com>
> 	<20180323050913.5188-1-orga.chem.job@gmail.com> 
> Content-Type: text/plain
>
>> Grammo (third-person singular 'prints' misspelt without 's'; the
>> "when" clause has a complex subject but no verb).
>> 
>> Perhaps this will salvage what you meant:
>> 
>> 	"git filter-branch -- --all" prints error messages when
>> 	processing refs that point at objects that are not
>> 	committish.
>
> Thanks. I'm gonna fix it.
>
>> Please sign-off your patch (cf. Documentation/SubmittingPatches).
>
> OK I'm gonna add it.
>
> I greatly appreciate for your kind review.
> I couldn't write this patch without your help.
> Thanks.

Heh, it's a team effort, and it is not like I am helping your effort
to build "open source contribution credit" for your coursework ;-)

Thank *you* for contributing to make Git a better system.

[PATCH v4] filter-branch: fix errors caused by refs that point at non-committish

[Yuki Kokubun]

"git filter-branch -- --all" prints error messages when processing refs that
point at objects that are not committish. Such refs can be created by
"git replace" with trees or blobs. And also "git tag" with trees or blobs can
create such refs.

Filter these problematic refs out early, before they are seen by the logic to
see which refs have been modified and which have been left intact (which is
where the unwanted error messages come from), and warn that these refs are left
unwritten while doing so.

Signed-off-by: Yuki Kokubun <orga.chem.job@gmail.com>
---
 git-filter-branch.sh     | 14 ++++++++++++--
 t/t7003-filter-branch.sh | 14 ++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..41efecb28 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -251,8 +251,18 @@ done < "$tempdir"/backup-refs
 
 # The refs should be updated if their heads were rewritten
 git rev-parse --no-flags --revs-only --symbolic-full-name \
-	--default HEAD "$@" > "$tempdir"/raw-heads || exit
-sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
+	--default HEAD "$@" > "$tempdir"/raw-refs || exit
+while read ref
+do
+	case "$ref" in ^?*) continue ;; esac
+
+	if git rev-parse --verify "$ref"^0 >/dev/null 2>&1
+	then
+		echo "$ref"
+	else
+		warn "WARNING: not rewriting '$ref' (not a committish)"
+	fi
+done >"$tempdir"/heads <"$tempdir"/raw-refs
 
 test -s "$tempdir"/heads ||
 	die "You must specify a ref to rewrite."
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 7cb60799b..04f79f32b 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -470,4 +470,18 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
 	git show HEAD:$ambiguous
 '
 
+test_expect_success 'rewrite repository including refs that point at non-commit object' '
+	test_when_finished "git reset --hard original" &&
+	tree=$(git rev-parse HEAD^{tree}) &&
+	test_when_finished "git replace -d $tree" &&
+	echo A >new &&
+	git add new &&
+	new_tree=$(git write-tree) &&
+	git replace $tree $new_tree &&
+	git tag -a -m "tag to a tree" treetag $new_tree &&
+	git reset --hard HEAD &&
+	git filter-branch -f -- --all >filter-output 2>&1 &&
+	! fgrep fatal filter-output
+'
+
 test_done
-- 
2.16.2.18.g09cb46d

Re: [PATCH v4] filter-branch: fix errors caused by refs that point at non-committish

[Yuki Kokubun]

References: <1521996898-7052-1-git-send-email-orga.chem.job@gmail.com>
Content-Type: text/plain

Sorry, I forgot add a line of "Reviewed-by".
I'm gonna send the fixed patch again.

Re: [PATCH v4] filter-branch: fix errors caused by refs that point at non-committish

[Junio C Hamano]

Yuki Kokubun <orga.chem.job@gmail.com> writes:

> References: <1521996898-7052-1-git-send-email-orga.chem.job@gmail.com>
> Content-Type: text/plain
>
> Sorry, I forgot add a line of "Reviewed-by".
> I'm gonna send the fixed patch again.

Do not worry too much about this.  Reviewed-by: added by patch
author is seen rather rarely, because the following sequence of
events need to happen:

 - The author sends a patch vN

 - A reviewer reviews vN and says "This is now Reviewed-by: me"

 - The author re-submits patch vN+1, where the only difference
   between vN and vN+1 is essentially _nothing_.  Except for the
   addition of "Reviewed-by:" by the reviewer.

But our typical patch injestion cycle is much faster and often the
last step does not happen ;-)

[PATCH v5] filter-branch: fix errors caused by refs that point at non-committish

[Yuki Kokubun]

"git filter-branch -- --all" prints error messages when processing refs that
point at objects that are not committish. Such refs can be created by
"git replace" with trees or blobs. And also "git tag" with trees or blobs can
create such refs.

Filter these problematic refs out early, before they are seen by the logic to
see which refs have been modified and which have been left intact (which is
where the unwanted error messages come from), and warn that these refs are left
unwritten while doing so.

Signed-off-by: Yuki Kokubun <orga.chem.job@gmail.com>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
---
 git-filter-branch.sh     | 14 ++++++++++++--
 t/t7003-filter-branch.sh | 14 ++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 1b7e4b2cd..41efecb28 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -251,8 +251,18 @@ done < "$tempdir"/backup-refs
 
 # The refs should be updated if their heads were rewritten
 git rev-parse --no-flags --revs-only --symbolic-full-name \
-	--default HEAD "$@" > "$tempdir"/raw-heads || exit
-sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
+	--default HEAD "$@" > "$tempdir"/raw-refs || exit
+while read ref
+do
+	case "$ref" in ^?*) continue ;; esac
+
+	if git rev-parse --verify "$ref"^0 >/dev/null 2>&1
+	then
+		echo "$ref"
+	else
+		warn "WARNING: not rewriting '$ref' (not a committish)"
+	fi
+done >"$tempdir"/heads <"$tempdir"/raw-refs
 
 test -s "$tempdir"/heads ||
 	die "You must specify a ref to rewrite."
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 7cb60799b..04f79f32b 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -470,4 +470,18 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
 	git show HEAD:$ambiguous
 '
 
+test_expect_success 'rewrite repository including refs that point at non-commit object' '
+	test_when_finished "git reset --hard original" &&
+	tree=$(git rev-parse HEAD^{tree}) &&
+	test_when_finished "git replace -d $tree" &&
+	echo A >new &&
+	git add new &&
+	new_tree=$(git write-tree) &&
+	git replace $tree $new_tree &&
+	git tag -a -m "tag to a tree" treetag $new_tree &&
+	git reset --hard HEAD &&
+	git filter-branch -f -- --all >filter-output 2>&1 &&
+	! fgrep fatal filter-output
+'
+
 test_done
-- 
2.16.2.18.g09cb46d

Re: [PATCH v4] filter-branch: fix errors caused by refs that point at non-committish

[Junio C Hamano]

Yuki Kokubun <orga.chem.job@gmail.com> writes:

> "git filter-branch -- --all" prints error messages when processing refs that
> point at objects that are not committish. Such refs can be created by
> "git replace" with trees or blobs. And also "git tag" with trees or blobs can
> create such refs.
>
> Filter these problematic refs out early, before they are seen by the logic to
> see which refs have been modified and which have been left intact (which is
> where the unwanted error messages come from), and warn that these refs are left
> unwritten while doing so.
>
> Signed-off-by: Yuki Kokubun <orga.chem.job@gmail.com>
> ---
>  git-filter-branch.sh     | 14 ++++++++++++--
>  t/t7003-filter-branch.sh | 14 ++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)

Good.  Will queue.  Thanks.

>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 1b7e4b2cd..41efecb28 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -251,8 +251,18 @@ done < "$tempdir"/backup-refs
>  
>  # The refs should be updated if their heads were rewritten
>  git rev-parse --no-flags --revs-only --symbolic-full-name \
> -	--default HEAD "$@" > "$tempdir"/raw-heads || exit
> -sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
> +	--default HEAD "$@" > "$tempdir"/raw-refs || exit
> +while read ref
> +do
> +	case "$ref" in ^?*) continue ;; esac
> +
> +	if git rev-parse --verify "$ref"^0 >/dev/null 2>&1
> +	then
> +		echo "$ref"
> +	else
> +		warn "WARNING: not rewriting '$ref' (not a committish)"
> +	fi
> +done >"$tempdir"/heads <"$tempdir"/raw-refs
>  
>  test -s "$tempdir"/heads ||
>  	die "You must specify a ref to rewrite."
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index 7cb60799b..04f79f32b 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -470,4 +470,18 @@ test_expect_success 'tree-filter deals with object name vs pathname ambiguity' '
>  	git show HEAD:$ambiguous
>  '
>  
> +test_expect_success 'rewrite repository including refs that point at non-commit object' '
> +	test_when_finished "git reset --hard original" &&
> +	tree=$(git rev-parse HEAD^{tree}) &&
> +	test_when_finished "git replace -d $tree" &&
> +	echo A >new &&
> +	git add new &&
> +	new_tree=$(git write-tree) &&
> +	git replace $tree $new_tree &&
> +	git tag -a -m "tag to a tree" treetag $new_tree &&
> +	git reset --hard HEAD &&
> +	git filter-branch -f -- --all >filter-output 2>&1 &&
> +	! fgrep fatal filter-output
> +'
> +
>  test_done

Re: [PATCH] filter-branch: consider refs can refer to an object other than commit or tag

[Yuki Kokubun]

>> "git filter-branch -- --all" print unwanted error messages when refs that
>> cannot be used with ^0 exist.
>
> It is not incorrect per-se, but if I were writing this, I'd say
> "... when refs that point at objects that are not committish" or
> something like that, as that is much closer to people (both end
> users and Git developers) than the "^0" implementation detail.

I agree that readability to users is important than the implementation detail.
So, I'm gonna change the commit message.

>> Such refs can be created by "git replace" with
>> trees or blobs. Also, "git tag" with trees or blobs can create such refs.
> 
> Taking two paragraphs above together, you wrote an excellent
> description of the problem to be solved (i.e. what would be seen by
> users and under what condition the symptom would trigger).  If there
> is a single obvious solution that would trivially follow from the
> problem description, it is perfectly fine to stop here.  Otherwise,
> it would help to describe how it is solved next.  Something as brief
> like
> 
> 	Filter these problematic refs out early, and warn that these
> 	refs are left unwritten while doing so.
> 
> would suffice in this case, I think.  We _could_ insert
> 
> 	before they are seen by the logic to see which refs have
> 	been modified and which have been left intact (which is
> 	where the unwanted error messages come from),
> 
> between "early," and "and warn", if we wanted to.

I think the detailed description is better than the shorter one in this case.
So I'm gonna follow to detailed one.