1601653 - Improve automatic end commit detection

Status: RESOLVED WORKSFORME

(Conduit :: moz-phab, enhancement)

Description

[Marc Streckfuß [:MeFisto94]]

When someone has multiple commits on one bug (or multiple commits in general on his head) and one of the parent commits already got approved on phabricator (In my case both commits were at least pushed once to phabricator), then invoking moz-phab fails, because it correctly detects that one of those commits/revisions is already closed.

This however prevents the child commits from being committed and thus one has to specify the start and end argument and therefore copy and paste the hg revisions etc.

I propose to improve the automatic end detection in one of these ways:

1. Just making a closed revision less critical and just skip it
2. When searching for an end commit, stop on the last parent that is not yet closed.

I am not familiar enough with mercurial to properly evaluate the first approach though:
I assume here we have the problem that it'd be possible to commit the parent of an already closed commit, which according to my understanding should be illegal(?) as otherwise the children would have to be rebased on these changes, which is impossible as they already have been landed.

Do you see any problem with approach number 2?

Comment 1

[Piotr Zalewa [:zalun]]

The revision should be closed in Phabricator after a commit has landed.
moz-phab will not use landed commits in a detected stack.
Is this not working for you?

Comment 2

[Marc Streckfuß [:MeFisto94]]

I just got an update this morning to moz-phab 0.1.65 and not it looks fixed (but still confusing):

$ moz-phab
Submitting 2 commits for review:
(D47999) 505268:45e5767b4455 Bug 1353652 - Initial Draft of MPRIS API Provider (Media API on Linux)
!! Missing reviewers
(D55281) 504786:ef57a54fb706 Bug 1353652 - Fix unified builds
!! Missing reviewers
Warning: found 7 untracked files (will not be submitted):
Submit to https://phabricator.services.mozilla.com (YES/No/Always)? 

Updating revision D55281:
504786:ef57a54fb706 Bug 1353652 - Fix unified builds

Updating revision D47999:
505268:45e5767b4455 Bug 1353652 - Initial Draft of MPRIS API Provider (Media API on Linux)
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
(activating bookmark moz-phab_45e5767b4455)

Completed
(D47999) 505268:45e5767b4455 Bug 1353652 - Initial Draft of MPRIS API Provider (Media API on Linux)
-> https://phabricator.services.mozilla.com/D47999
(D55281) 504786:ef57a54fb706 Bug 1353652 - Fix unified builds
-> https://phabricator.services.mozilla.com/D55281

The Fix Build Commit has been landed already. Maybe it was related to an earlier moz-phab version or it was because I was trying to commit in between the fix being landed or the reviews being positive and the actual revision being closed fully.

What I did notice is that it still updates the revisions

1. The changes were none, so it did: https://phabricator.services.mozilla.com/D47999?vs=202518&id=203956#toc
2. It still uploaded a new change to the already closed revision, so this happened: https://phabricator.services.mozilla.com/D55281?vs=202247&id=203955#toc

So something is not entirely right there

Comment 3

[Piotr Zalewa [:zalun]]

We recognize the local stack before the prompt for submission.
If it isn't right it's OK to choose the range.
Please reopen if it would happen again or there is a way to replicate.