Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits

"Devin J. Pohly" [off-list ref] 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?

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

"Devin J. Pohly" [off-list ref] writes:

quoted

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.

-- 
<><

On Thu, Mar 2, 2017 at 11:36 AM, Junio C Hamano [off-list ref] wrote:

"Devin J. Pohly" [off-list ref] writes:

quoted

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

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

"Devin J. Pohly" [off-list ref] writes:

quoted

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

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.

-- 
<><