threads / patch / 48104

patchfilter-branch: consider refs/replace can refer to an object other than commit

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

## tl;dr

20 messages between Mar 21, 2018 and Apr 8, 2018. Diffs are folded; open one to read it.

replies: 19people: 2as markdown or json

Yuki Kokubun· Mar 21, 2018, 10:35 UTC · lore

"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(+)
Show changes to 2 files +16 −0

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Yuki Kokubun· Mar 21, 2018, 15:31 UTC · re: Yuki Kokubun · lore

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

"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(-)
Show changes to 2 files +23 −1

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Junio C Hamano· Mar 21, 2018, 17:21 UTC · re: Yuki Kokubun · lore

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

Yuki Kokubun <orga.chem.job@gmail.com> writes:
Show 5 quoted lines
> "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.
Show 5 quoted lines
>  # 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.

Show 9 quoted lines
> +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.

Show 17 quoted lines
>  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?
Show 6 quoted lines
> +	git reset --hard HEAD &&
> +	git filter-branch -f -- --all >filter-output 2>&1 &&
> +	! fgrep fatal filter-output
> +'
> +
>  test_done
Yuki Kokubun· Mar 21, 2018, 20:00 UTC · re: Junio C Hamano · lore

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

Show 12 quoted lines
> 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.

Show 26 quoted lines
> 
> 	... 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.
Show 32 quoted lines
> 
> > +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.

Show 24 quoted lines
> 
> >  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.
Show 7 quoted lines
> 
> > +	git reset --hard HEAD &&
> > +	git filter-branch -f -- --all >filter-output 2>&1 &&
> > +	! fgrep fatal filter-output
> > +'
> > +
> >  test_done
Junio C Hamano· Mar 21, 2018, 20:00 UTC · re: Yuki Kokubun · lore

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

Yuki Kokubun <orga.chem.job@gmail.com> writes:
Show 15 quoted lines
>> 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?

Yuki Kokubun· Mar 22, 2018, 14:26 UTC · re: Junio C Hamano · lore

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

Show 24 quoted lines
> 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?

Junio C Hamano· Mar 22, 2018, 17:01 UTC · re: Yuki Kokubun · lore

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

Yuki Kokubun <orga.chem.job@gmail.com> writes:
Show 9 quoted lines
>> 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.

Show 6 quoted lines
> 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.

Show 14 quoted lines
> 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).
Yuki Kokubun· Mar 23, 2018, 02:15 UTC · re: Junio C Hamano · lore

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

Show 20 quoted lines
> 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.

Yuki Kokubun· Mar 23, 2018, 05:09 UTC · re: Yuki Kokubun · lore

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

"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(-)
Show changes to 2 files +26 −2

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Junio C Hamano· Mar 23, 2018, 20:20 UTC · re: Yuki Kokubun · lore

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

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.
Yuki Kokubun· Mar 24, 2018, 19:29 UTC · re: Junio C Hamano · lore

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

Show 7 quoted lines
>> "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.

Show 21 quoted lines
>> 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.

Yuki Kokubun· Mar 24, 2018, 19:41 UTC · re: Junio C Hamano · lore

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

"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(-)
Show changes to 2 files +26 −2

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Junio C Hamano· Mar 25, 2018, 16:25 UTC · re: Yuki Kokubun · lore

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

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.
Show 8 quoted lines
> 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.
Show 52 quoted lines
>  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
Yuki Kokubun· Mar 25, 2018, 16:39 UTC · re: Junio C Hamano · lore

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

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
Show 8 quoted lines
> 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.

Junio C Hamano· Mar 25, 2018, 20:24 UTC · re: Yuki Kokubun · lore

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

Yuki Kokubun <orga.chem.job@gmail.com> writes:
Show 24 quoted lines
> 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.
Yuki Kokubun· Mar 25, 2018, 16:54 UTC · re: Junio C Hamano · lore

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

"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(-)
Show changes to 2 files +26 −2

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Yuki Kokubun· Mar 25, 2018, 16:45 UTC · re: Yuki Kokubun · lore

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

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.

Junio C Hamano· Apr 8, 2018, 23:10 UTC · re: Yuki Kokubun · lore

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

Yuki Kokubun <orga.chem.job@gmail.com> writes:
Show 5 quoted lines
> 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 ;-)

Yuki Kokubun· Mar 25, 2018, 17:01 UTC · re: Yuki Kokubun · lore

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

"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(-)
Show changes to 2 files +26 −2

git-filter-branch.sh, t/t7003-filter-branch.sh

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
Junio C Hamano· Mar 25, 2018, 17:13 UTC · re: Yuki Kokubun · lore

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

Yuki Kokubun <orga.chem.job@gmail.com> writes:
Show 15 quoted lines
> "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.
Show 49 quoted lines
>
> 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

← back to recent threads