git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 1/4] t7003: ensure --prune-empty can prune root commit
@ 2017-02-23  8:27 Devin J. Pohly
  2017-02-23  8:27 ` [PATCH 2/4] t7003: ensure --prune-empty removes entire branch when applicable Devin J. Pohly
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Devin J. Pohly @ 2017-02-23  8:27 UTC (permalink / raw)
  To: git; +Cc: Devin J. Pohly

New test to expose a bug in filter-branch whereby the root commit is
never pruned, even though its tree is empty and --prune-empty is given.

The setup isn't exactly pretty, but I couldn't think of a simpler way to
create a parallel commit graph sans the first commit.

Signed-off-by: Devin J. Pohly <djpohly@gmail.com>
---
 t/t7003-filter-branch.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index cb8fbd8e5..2dfe46250 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -313,6 +313,27 @@ test_expect_success 'Tag name filtering allows slashes in tag names' '
 	git cat-file tag X/2 > actual &&
 	test_cmp expect actual
 '
+test_expect_success 'setup --prune-empty comparisons' '
+	git checkout --orphan master-no-a &&
+	git rm -rf . &&
+	unset test_tick &&
+	test_tick &&
+	GIT_COMMITTER_DATE="@0 +0000" GIT_AUTHOR_DATE="@0 +0000" &&
+	test_commit --notick B B.t B Bx &&
+	git checkout -b branch-no-a Bx &&
+	test_commit D D.t D Dx &&
+	mkdir dir &&
+	test_commit dir/D dir/D.t dir/D dir/Dx &&
+	test_commit E E.t E Ex &&
+	git checkout master-no-a &&
+	test_commit C C.t C Cx &&
+	git checkout branch-no-a &&
+	git merge Cx -m "Merge tag '\''C'\'' into branch" &&
+	git tag Fx &&
+	test_commit G G.t G Gx &&
+	test_commit H H.t H Hx &&
+	git checkout branch
+'
 
 test_expect_success 'Prune empty commits' '
 	git rev-list HEAD > expect &&
@@ -341,6 +362,15 @@ test_expect_success 'prune empty works even without index/tree filters' '
 	test_cmp expect actual
 '
 
+test_expect_success '--prune-empty is able to prune root commit' '
+	git rev-list branch-no-a >expect &&
+	git branch testing H &&
+	git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t" testing &&
+	git rev-list testing >actual &&
+	git branch -D testing &&
+	test_cmp expect actual
+'
+
 test_expect_success '--remap-to-ancestor with filename filters' '
 	git checkout master &&
 	git reset --hard A &&
-- 
2.11.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/4] t7003: ensure --prune-empty removes entire branch when applicable
  2017-02-23  8:27 [PATCH 1/4] t7003: ensure --prune-empty can prune root commit Devin J. Pohly
@ 2017-02-23  8:27 ` Devin J. Pohly
  2017-02-23  8:27 ` [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits Devin J. Pohly
  2017-02-23  8:27 ` [PATCH 4/4] p7000: add test for filter-branch with --prune-empty Devin J. Pohly
  2 siblings, 0 replies; 15+ messages in thread
From: Devin J. Pohly @ 2017-02-23  8:27 UTC (permalink / raw)
  To: git; +Cc: Devin J. Pohly

Sanity check before changing the logic in git_commit_non_empty_tree.

Signed-off-by: Devin J. Pohly <djpohly@gmail.com>
---
 t/t7003-filter-branch.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 2dfe46250..7cb60799b 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -371,6 +371,13 @@ test_expect_success '--prune-empty is able to prune root commit' '
 	test_cmp expect actual
 '
 
+test_expect_success '--prune-empty is able to prune entire branch' '
+	git branch prune-entire B &&
+	git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
+	test_path_is_missing .git/refs/heads/prune-entire &&
+	test_must_fail git reflog exists refs/heads/prune-entire
+'
+
 test_expect_success '--remap-to-ancestor with filename filters' '
 	git checkout master &&
 	git reset --hard A &&
-- 
2.11.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-02-23  8:27 [PATCH 1/4] t7003: ensure --prune-empty can prune root commit Devin J. Pohly
  2017-02-23  8:27 ` [PATCH 2/4] t7003: ensure --prune-empty removes entire branch when applicable Devin J. Pohly
@ 2017-02-23  8:27 ` Devin J. Pohly
  2017-02-23 21:17   ` Junio C Hamano
  2017-02-23  8:27 ` [PATCH 4/4] p7000: add test for filter-branch with --prune-empty Devin J. Pohly
  2 siblings, 1 reply; 15+ messages in thread
From: Devin J. Pohly @ 2017-02-23  8:27 UTC (permalink / raw)
  To: git; +Cc: Devin J. Pohly

Previously, the git_commit_non_empty_tree function would always pass any
commit with no parents to git-commit-tree, regardless of whether the
tree was nonempty.  The new commit would then be recorded in the
filter-branch revision map, and subsequent commits which leave the tree
untouched would be correctly filtered.

With this change, parentless commits with an empty tree are correctly
pruned, and an empty file is recorded in the revision map, signifying
that it was rewritten to "no commits."  This works naturally with the
parent mapping for subsequent commits.

Signed-off-by: Devin J. Pohly <djpohly@gmail.com>
---
 Documentation/git-filter-branch.txt | 14 ++++++--------
 git-filter-branch.sh                |  2 ++
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
index 0a09698c0..6e4bb0220 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -167,14 +167,12 @@ to other tags will be rewritten to point to the underlying commit.
 	project root. Implies <<Remap_to_ancestor>>.
 
 --prune-empty::
-	Some kind of filters will generate empty commits, that left the tree
-	untouched.  This switch allow git-filter-branch to ignore such
-	commits.  Though, this switch only applies for commits that have one
-	and only one parent, it will hence keep merges points. Also, this
-	option is not compatible with the use of `--commit-filter`. Though you
-	just need to use the function 'git_commit_non_empty_tree "$@"' instead
-	of the `git commit-tree "$@"` idiom in your commit filter to make that
-	happen.
+	Some filters will generate empty commits that leave the tree untouched.
+	This option instructs git-filter-branch to remove such commits if they
+	have exactly one or zero non-pruned parents; merge commits will
+	therefore remain intact.  This option cannot be used together with
+	`--commit-filter`, though the same effect can be achieved by using the
+	provided `git_commit_non_empty_tree` function in a commit filter.
 
 --original <namespace>::
 	Use this option to set the namespace where the original commits
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86b2ff1e0..2b8cdba15 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -46,6 +46,8 @@ git_commit_non_empty_tree()
 {
 	if test $# = 3 && test "$1" = $(git rev-parse "$3^{tree}"); then
 		map "$3"
+	elif test $# = 1 && test "$1" = 4b825dc642cb6eb9a060e54bf8d69288fbee4904; then
+		:
 	else
 		git commit-tree "$@"
 	fi
-- 
2.11.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/4] p7000: add test for filter-branch with --prune-empty
  2017-02-23  8:27 [PATCH 1/4] t7003: ensure --prune-empty can prune root commit Devin J. Pohly
  2017-02-23  8:27 ` [PATCH 2/4] t7003: ensure --prune-empty removes entire branch when applicable Devin J. Pohly
  2017-02-23  8:27 ` [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits Devin J. Pohly
@ 2017-02-23  8:27 ` Devin J. Pohly
  2017-03-03  7:56   ` Jeff King
  2 siblings, 1 reply; 15+ messages in thread
From: Devin J. Pohly @ 2017-02-23  8:27 UTC (permalink / raw)
  To: git; +Cc: Devin J. Pohly

Signed-off-by: Devin J. Pohly <djpohly@gmail.com>
---
 t/perf/p7000-filter-branch.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/perf/p7000-filter-branch.sh b/t/perf/p7000-filter-branch.sh
index 15ee5d1d5..b029586cc 100755
--- a/t/perf/p7000-filter-branch.sh
+++ b/t/perf/p7000-filter-branch.sh
@@ -16,4 +16,9 @@ test_perf 'noop filter' '
 	git filter-branch -f base..HEAD
 '
 
+test_perf 'noop prune-empty' '
+	git checkout --detach tip &&
+	git filter-branch -f --prune-empty base..HEAD
+'
+
 test_done
-- 
2.11.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-02-23  8:27 ` [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits Devin J. Pohly
@ 2017-02-23 21:17   ` Junio C Hamano
  2017-02-23 21:33     ` Devin J. Pohly
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2017-02-23 21:17 UTC (permalink / raw)
  To: Johannes Schindelin, Charles Bailey, Jeff King, Pierre Habouzit
  Cc: git, Devin J. Pohly

"Devin J. Pohly" <djpohly@gmail.com> writes:

> Previously, the git_commit_non_empty_tree function would always pass any
> commit with no parents to git-commit-tree, regardless of whether the
> tree was nonempty.  The new commit would then be recorded in the
> filter-branch revision map, and subsequent commits which leave the tree
> untouched would be correctly filtered.
>
> With this change, parentless commits with an empty tree are correctly
> pruned, and an empty file is recorded in the revision map, signifying
> that it was rewritten to "no commits."  This works naturally with the
> parent mapping for subsequent commits.
>
> Signed-off-by: Devin J. Pohly <djpohly@gmail.com>
> ---

I am not sure if a root that records an empty tree should be pruned
with --prune-empty to begin with.

When we are pruning consecutive commits in the other parts of the
history because they have identical (presumably non-empty) trees,
should an empty root that the original history wanted to create be
pruned because before the commit it was void, after the commit it is
empty?  Should "void" (lack of any tree) and "empty" (the tree is
there, but it does not have anything in it) be treated the same?
Shouldn't root be treated as a bit more special thing?

I myself do not have a good answer to the above questions.

I think the updated code makes sense, provided if we decide that
void to empty is just like transitioning between two identical
(presumably non-empty) trees.  The updated documentation is a lot
more readable as well.

Comments from those who have been involved in filter-branch?

>  Documentation/git-filter-branch.txt | 14 ++++++--------
>  git-filter-branch.sh                |  2 ++
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
> index 0a09698c0..6e4bb0220 100644
> --- a/Documentation/git-filter-branch.txt
> +++ b/Documentation/git-filter-branch.txt
> @@ -167,14 +167,12 @@ to other tags will be rewritten to point to the underlying commit.
>  	project root. Implies <<Remap_to_ancestor>>.
>  
>  --prune-empty::
> -	Some kind of filters will generate empty commits, that left the tree
> -	untouched.  This switch allow git-filter-branch to ignore such
> -	commits.  Though, this switch only applies for commits that have one
> -	and only one parent, it will hence keep merges points. Also, this
> -	option is not compatible with the use of `--commit-filter`. Though you
> -	just need to use the function 'git_commit_non_empty_tree "$@"' instead
> -	of the `git commit-tree "$@"` idiom in your commit filter to make that
> -	happen.
> +	Some filters will generate empty commits that leave the tree untouched.
> +	This option instructs git-filter-branch to remove such commits if they
> +	have exactly one or zero non-pruned parents; merge commits will
> +	therefore remain intact.  This option cannot be used together with
> +	`--commit-filter`, though the same effect can be achieved by using the
> +	provided `git_commit_non_empty_tree` function in a commit filter.
>  
>  --original <namespace>::
>  	Use this option to set the namespace where the original commits
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 86b2ff1e0..2b8cdba15 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -46,6 +46,8 @@ git_commit_non_empty_tree()
>  {
>  	if test $# = 3 && test "$1" = $(git rev-parse "$3^{tree}"); then
>  		map "$3"
> +	elif test $# = 1 && test "$1" = 4b825dc642cb6eb9a060e54bf8d69288fbee4904; then
> +		:
>  	else
>  		git commit-tree "$@"
>  	fi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-02-23 21:17   ` Junio C Hamano
@ 2017-02-23 21:33     ` Devin J. Pohly
  2017-03-02 19:36       ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Devin J. Pohly @ 2017-02-23 21:33 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, Charles Bailey, Jeff King, Pierre Habouzit,
	git

On Thu, Feb 23, 2017 at 01:17:49PM -0800, Junio C Hamano wrote:
> "Devin J. Pohly" <djpohly@gmail.com> writes:
> 
> > Previously, the git_commit_non_empty_tree function would always pass any
> > commit with no parents to git-commit-tree, regardless of whether the
> > tree was nonempty.  The new commit would then be recorded in the
> > filter-branch revision map, and subsequent commits which leave the tree
> > untouched would be correctly filtered.
> >
> > With this change, parentless commits with an empty tree are correctly
> > pruned, and an empty file is recorded in the revision map, signifying
> > that it was rewritten to "no commits."  This works naturally with the
> > parent mapping for subsequent commits.
> >
> > Signed-off-by: Devin J. Pohly <djpohly@gmail.com>
> > ---
> 
> I am not sure if a root that records an empty tree should be pruned
> with --prune-empty to begin with.
> 
> When we are pruning consecutive commits in the other parts of the
> history because they have identical (presumably non-empty) trees,
> should an empty root that the original history wanted to create be
> pruned because before the commit it was void, after the commit it is
> empty?  Should "void" (lack of any tree) and "empty" (the tree is
> there, but it does not have anything in it) be treated the same?
> Shouldn't root be treated as a bit more special thing?
>

The case I had in mind was a filter which happened to remove all changes
from any parentless commit (see the testcase added to t7003).  It would
not necessarily have been an empty commit in the original history.

Use case/motivation: I am splitting my dotfiles repo to migrate to vcsh,
and the original first commit (which only touches a few files) appears
in every branch.  In the branches which do not include those files, the
commit is empty but still present.

I think your point is interesting too, though.  If a commit is also
TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
reasonable that someone might want to leave it in the filtered branch as
an empty commit while pruning empt*ied* commits.  I would imagine that
as another option (--prune-newly-empty?).

> 
> I myself do not have a good answer to the above questions.
> 
> I think the updated code makes sense, provided if we decide that
> void to empty is just like transitioning between two identical
> (presumably non-empty) trees.  The updated documentation is a lot
> more readable as well.
> 
> Comments from those who have been involved in filter-branch?
> 
> >  Documentation/git-filter-branch.txt | 14 ++++++--------
> >  git-filter-branch.sh                |  2 ++
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
> > index 0a09698c0..6e4bb0220 100644
> > --- a/Documentation/git-filter-branch.txt
> > +++ b/Documentation/git-filter-branch.txt
> > @@ -167,14 +167,12 @@ to other tags will be rewritten to point to the underlying commit.
> >  	project root. Implies <<Remap_to_ancestor>>.
> >  
> >  --prune-empty::
> > -	Some kind of filters will generate empty commits, that left the tree
> > -	untouched.  This switch allow git-filter-branch to ignore such
> > -	commits.  Though, this switch only applies for commits that have one
> > -	and only one parent, it will hence keep merges points. Also, this
> > -	option is not compatible with the use of `--commit-filter`. Though you
> > -	just need to use the function 'git_commit_non_empty_tree "$@"' instead
> > -	of the `git commit-tree "$@"` idiom in your commit filter to make that
> > -	happen.
> > +	Some filters will generate empty commits that leave the tree untouched.
> > +	This option instructs git-filter-branch to remove such commits if they
> > +	have exactly one or zero non-pruned parents; merge commits will
> > +	therefore remain intact.  This option cannot be used together with
> > +	`--commit-filter`, though the same effect can be achieved by using the
> > +	provided `git_commit_non_empty_tree` function in a commit filter.
> >  
> >  --original <namespace>::
> >  	Use this option to set the namespace where the original commits
> > diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> > index 86b2ff1e0..2b8cdba15 100755
> > --- a/git-filter-branch.sh
> > +++ b/git-filter-branch.sh
> > @@ -46,6 +46,8 @@ git_commit_non_empty_tree()
> >  {
> >  	if test $# = 3 && test "$1" = $(git rev-parse "$3^{tree}"); then
> >  		map "$3"
> > +	elif test $# = 1 && test "$1" = 4b825dc642cb6eb9a060e54bf8d69288fbee4904; then
> > +		:
> >  	else
> >  		git commit-tree "$@"
> >  	fi

-- 
<><

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-02-23 21:33     ` Devin J. Pohly
@ 2017-03-02 19:36       ` Junio C Hamano
  2017-03-02 21:18         ` Devin J. Pohly
                           ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Junio C Hamano @ 2017-03-02 19:36 UTC (permalink / raw)
  To: Devin J. Pohly; +Cc: Johannes Schindelin, Charles Bailey, Jeff King, git

"Devin J. Pohly" <djpohly@gmail.com> writes:

> I think your point is interesting too, though.  If a commit is also
> TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
> reasonable that someone might want to leave it in the filtered branch as
> an empty commit while pruning empt*ied* commits.  I would imagine that
> as another option (--prune-newly-empty?).

I was hoping to hear from others who may care about filter-branch to
comment on this topic to help me decide, but I haven't heard
anything, so here is my tentative thinking.

I am leaning to:

 * Take your series as-is, which would mean --prune-empty will
   change the behaviour to unconditionally lose the empty root.

 * Then, people who care deeply about it can add a new option that
   prunes commits that become empty while keeping the originally
   empty ones.

Thoughts?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-03-02 19:36       ` Junio C Hamano
@ 2017-03-02 21:18         ` Devin J. Pohly
  2017-03-02 21:39           ` Junio C Hamano
  2017-03-02 23:28         ` Jacob Keller
  2017-03-03  7:55         ` Jeff King
  2 siblings, 1 reply; 15+ messages in thread
From: Devin J. Pohly @ 2017-03-02 21:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, Charles Bailey, Jeff King, git

On Thu, Mar 02, 2017 at 11:36:18AM -0800, Junio C Hamano wrote:
> "Devin J. Pohly" <djpohly@gmail.com> writes:
> 
> > I think your point is interesting too, though.  If a commit is also
> > TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
> > reasonable that someone might want to leave it in the filtered branch as
> > an empty commit while pruning empt*ied* commits.  I would imagine that
> > as another option (--prune-newly-empty?).
> 
> I was hoping to hear from others who may care about filter-branch to
> comment on this topic to help me decide, but I haven't heard
> anything, so here is my tentative thinking.
> 
> I am leaning to:
> 
>  * Take your series as-is, which would mean --prune-empty will
>    change the behaviour to unconditionally lose the empty root.
> 
>  * Then, people who care deeply about it can add a new option that
>    prunes commits that become empty while keeping the originally
>    empty ones.
> 
> Thoughts?

Sounds good to me.  I would be willing to work on a new option if needed
(to "atone" for changing existing behavior), so you can loop me in if
there are any complaints.

-- 
<><

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-03-02 21:18         ` Devin J. Pohly
@ 2017-03-02 21:39           ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2017-03-02 21:39 UTC (permalink / raw)
  To: Devin J. Pohly; +Cc: Johannes Schindelin, Charles Bailey, Jeff King, git

"Devin J. Pohly" <djpohly@gmail.com> writes:

> On Thu, Mar 02, 2017 at 11:36:18AM -0800, Junio C Hamano wrote:
>> "Devin J. Pohly" <djpohly@gmail.com> writes:
>> 
>> > I think your point is interesting too, though.  If a commit is also
>> > TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
>> > reasonable that someone might want to leave it in the filtered branch as
>> > an empty commit while pruning empt*ied* commits.  I would imagine that
>> > as another option (--prune-newly-empty?).
>> 
>> I was hoping to hear from others who may care about filter-branch to
>> comment on this topic to help me decide, but I haven't heard
>> anything, so here is my tentative thinking.
>> 
>> I am leaning to:
>> 
>>  * Take your series as-is, which would mean --prune-empty will
>>    change the behaviour to unconditionally lose the empty root.
>> 
>>  * Then, people who care deeply about it can add a new option that
>>    prunes commits that become empty while keeping the originally
>>    empty ones.
>> 
>> Thoughts?
>
> Sounds good to me.  I would be willing to work on a new option if needed
> (to "atone" for changing existing behavior), so you can loop me in if
> there are any complaints.

Thanks.  I'll wait for others who know filter-branch better than me
to say something for a few days before doing anything, though.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-03-02 19:36       ` Junio C Hamano
  2017-03-02 21:18         ` Devin J. Pohly
@ 2017-03-02 23:28         ` Jacob Keller
  2017-03-03  7:55         ` Jeff King
  2 siblings, 0 replies; 15+ messages in thread
From: Jacob Keller @ 2017-03-02 23:28 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Devin J. Pohly, Johannes Schindelin, Charles Bailey, Jeff King,
	Git mailing list

On Thu, Mar 2, 2017 at 11:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Devin J. Pohly" <djpohly@gmail.com> writes:
>
>> I think your point is interesting too, though.  If a commit is also
>> TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
>> reasonable that someone might want to leave it in the filtered branch as
>> an empty commit while pruning empt*ied* commits.  I would imagine that
>> as another option (--prune-newly-empty?).
>
> I was hoping to hear from others who may care about filter-branch to
> comment on this topic to help me decide, but I haven't heard
> anything, so here is my tentative thinking.
>
> I am leaning to:
>
>  * Take your series as-is, which would mean --prune-empty will
>    change the behaviour to unconditionally lose the empty root.
>

This new behavior is how I expected prune-empty to behave, so seeing
that it did not already behave this way was surprising.

Thanks,
Jake

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-03-02 19:36       ` Junio C Hamano
  2017-03-02 21:18         ` Devin J. Pohly
  2017-03-02 23:28         ` Jacob Keller
@ 2017-03-03  7:55         ` Jeff King
  2017-03-03 20:30           ` Devin J. Pohly
  2017-03-03 20:43           ` Junio C Hamano
  2 siblings, 2 replies; 15+ messages in thread
From: Jeff King @ 2017-03-03  7:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Devin J. Pohly, Johannes Schindelin, Charles Bailey, git

On Thu, Mar 02, 2017 at 11:36:18AM -0800, Junio C Hamano wrote:

> "Devin J. Pohly" <djpohly@gmail.com> writes:
> 
> > I think your point is interesting too, though.  If a commit is also
> > TREESAME to its parent(s?) in the _pre-filtered_ branch, it seems
> > reasonable that someone might want to leave it in the filtered branch as
> > an empty commit while pruning empt*ied* commits.  I would imagine that
> > as another option (--prune-newly-empty?).
> 
> I was hoping to hear from others who may care about filter-branch to
> comment on this topic to help me decide, but I haven't heard
> anything, so here is my tentative thinking.
> 
> I am leaning to:
> 
>  * Take your series as-is, which would mean --prune-empty will
>    change the behaviour to unconditionally lose the empty root.
> 
>  * Then, people who care deeply about it can add a new option that
>    prunes commits that become empty while keeping the originally
>    empty ones.
> 
> Thoughts?

Sorry, this was on my to-review list but the sha1 stuff has been much
more exciting. :)

The behavior that Devin proposes is what I would have expected to
happen. Between "prune-existing-empty" and "prune-became-empty", I've
never had a use for the distinction. But as I think this is similar to
the way cherry-pick distinguished between "redundant" and "empty", I
guess some people have.

I agree that it should be a new, separate option, as it's orthogonal to
dealing with the root commit (i.e., somebody is equally likely to want
to preserve an already-empty commit from the middle of history).

The change to filter-branch itself looks obviously correct. The only
objectionable thing I noticed in the test additions is that the early
ones should be marked test_expect_failure until the fix from 3/4 flips
them to "success". Otherwise it breaks bisectability.

-Peff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] p7000: add test for filter-branch with --prune-empty
  2017-02-23  8:27 ` [PATCH 4/4] p7000: add test for filter-branch with --prune-empty Devin J. Pohly
@ 2017-03-03  7:56   ` Jeff King
  2017-03-03 20:34     ` Devin J. Pohly
  0 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2017-03-03  7:56 UTC (permalink / raw)
  To: Devin J. Pohly; +Cc: git

On Thu, Feb 23, 2017 at 02:27:36AM -0600, Devin J. Pohly wrote:

> +test_perf 'noop prune-empty' '
> +	git checkout --detach tip &&
> +	git filter-branch -f --prune-empty base..HEAD
> +'

I don't mind adding this, but of curiosity, does it show anything
interesting?

-Peff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-03-03  7:55         ` Jeff King
@ 2017-03-03 20:30           ` Devin J. Pohly
  2017-03-03 20:43           ` Junio C Hamano
  1 sibling, 0 replies; 15+ messages in thread
From: Devin J. Pohly @ 2017-03-03 20:30 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Johannes Schindelin, Charles Bailey, git

On Fri, Mar 03, 2017 at 02:55:35AM -0500, Jeff King wrote:
> The only objectionable thing I noticed in the test additions is that
> the early ones should be marked test_expect_failure until the fix from
> 3/4 flips them to "success". Otherwise it breaks bisectability.
> 
> -Peff

Good point.  Will fix in a v2 set then.

-- 
<><

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] p7000: add test for filter-branch with --prune-empty
  2017-03-03  7:56   ` Jeff King
@ 2017-03-03 20:34     ` Devin J. Pohly
  0 siblings, 0 replies; 15+ messages in thread
From: Devin J. Pohly @ 2017-03-03 20:34 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Fri, Mar 03, 2017 at 02:56:05AM -0500, Jeff King wrote:
> On Thu, Feb 23, 2017 at 02:27:36AM -0600, Devin J. Pohly wrote:
> 
> > +test_perf 'noop prune-empty' '
> > +	git checkout --detach tip &&
> > +	git filter-branch -f --prune-empty base..HEAD
> > +'
> 
> I don't mind adding this, but of curiosity, does it show anything
> interesting?
> 
> -Peff

Nothing surprising; the overhead for the change was minimal.  I wasn't
sure what the practice is for adding unit/perf tests, so I erred on the
side of covering everything.

I will leave this as the last commit in the series so that it can be
dropped or merged as you see fit.

-- 
<><

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
  2017-03-03  7:55         ` Jeff King
  2017-03-03 20:30           ` Devin J. Pohly
@ 2017-03-03 20:43           ` Junio C Hamano
  1 sibling, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2017-03-03 20:43 UTC (permalink / raw)
  To: Devin J. Pohly, Jeff King; +Cc: Johannes Schindelin, Charles Bailey, git

Jeff King <peff@peff.net> writes:

> The change to filter-branch itself looks obviously correct. The only
> objectionable thing I noticed in the test additions is that the early
> ones should be marked test_expect_failure until the fix from 3/4 flips
> them to "success". Otherwise it breaks bisectability.

I'll squash in the necessary changes to flip between expect_success
and expect_failure in the appropriate places and re-queue on 'pu'.

Thanks.


commit 07cac4a5fdfeeb3c1c8385e222100d575a4460b0
Author: Junio C Hamano <gitster@pobox.com>
Date:   Fri Mar 3 11:41:36 2017 -0800

    fixup! t7003: ensure --prune-empty can prune root commit

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 2dfe462501..45372a1cba 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -362,7 +362,7 @@ test_expect_success 'prune empty works even without index/tree filters' '
 	test_cmp expect actual
 '
 
-test_expect_success '--prune-empty is able to prune root commit' '
+test_expect_failure '--prune-empty is able to prune root commit' '
 	git rev-list branch-no-a >expect &&
 	git branch testing H &&
 	git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t" testing &&


commit 562ed048c681686426ca95e0e550378b48aa4852
Author: Junio C Hamano <gitster@pobox.com>
Date:   Fri Mar 3 12:11:25 2017 -0800

    fixup! t7003: ensure --prune-empty removes entire branch when applicable

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index a774a8e4b3..40526d1716 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -371,7 +371,7 @@ test_expect_failure '--prune-empty is able to prune root commit' '
 	test_cmp expect actual
 '
 
-test_expect_success '--prune-empty is able to prune entire branch' '
+test_expect_failure '--prune-empty is able to prune entire branch' '
 	git branch prune-entire B &&
 	git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
 	test_path_is_missing .git/refs/heads/prune-entire &&


commit 520534c4035a13c54229dab0320e745d18635ef3
Author: Junio C Hamano <gitster@pobox.com>
Date:   Fri Mar 3 12:39:58 2017 -0800

    fixup! filter-branch: fix --prune-empty on parentless commits

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 40526d1716..7cb60799be 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -362,7 +362,7 @@ test_expect_success 'prune empty works even without index/tree filters' '
 	test_cmp expect actual
 '
 
-test_expect_failure '--prune-empty is able to prune root commit' '
+test_expect_success '--prune-empty is able to prune root commit' '
 	git rev-list branch-no-a >expect &&
 	git branch testing H &&
 	git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t" testing &&
@@ -371,7 +371,7 @@ test_expect_failure '--prune-empty is able to prune root commit' '
 	test_cmp expect actual
 '
 
-test_expect_failure '--prune-empty is able to prune entire branch' '
+test_expect_success '--prune-empty is able to prune entire branch' '
 	git branch prune-entire B &&
 	git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
 	test_path_is_missing .git/refs/heads/prune-entire &&

^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-03-03 20:48 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-23  8:27 [PATCH 1/4] t7003: ensure --prune-empty can prune root commit Devin J. Pohly
2017-02-23  8:27 ` [PATCH 2/4] t7003: ensure --prune-empty removes entire branch when applicable Devin J. Pohly
2017-02-23  8:27 ` [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits Devin J. Pohly
2017-02-23 21:17   ` Junio C Hamano
2017-02-23 21:33     ` Devin J. Pohly
2017-03-02 19:36       ` Junio C Hamano
2017-03-02 21:18         ` Devin J. Pohly
2017-03-02 21:39           ` Junio C Hamano
2017-03-02 23:28         ` Jacob Keller
2017-03-03  7:55         ` Jeff King
2017-03-03 20:30           ` Devin J. Pohly
2017-03-03 20:43           ` Junio C Hamano
2017-02-23  8:27 ` [PATCH 4/4] p7000: add test for filter-branch with --prune-empty Devin J. Pohly
2017-03-03  7:56   ` Jeff King
2017-03-03 20:34     ` Devin J. Pohly

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).