1504534 - [git] moz-phab should follow the same range rules as git

Status: RESOLVED FIXED

(Conduit :: moz-phab, enhancement)

Description

[Jean-Yves Avenard [:jya]]

when doing moz-phab submit SHA1_start SHA1_end 

both commits are inclusive. However, git range start is never inclusive normally.

the commit with SHA1_start shouldn't be included in the review.

This is similar to bug 1497712, however somehow an exception was made to make the start SHA1 if provided inclusive. I believe this is incorrect and rather confusing as no git tools ever does so.

moz-phab submit HEAD~2 HEAD
should be identical in behaviour as moz-phab submit HEAD~2

Comment 1

[Jean-Yves Avenard [:jya]]

An example where moz-phab behaviour strongly differs to usual git usage:

git diff HEAD~2 HEAD
provides identical output to: git diff HEAD~2 (assuming there's no uncommited change)

Comment 2

[Piotr Zalewa [:zalun]]

The behavior with SHA1 is by design inclusive which is different from git experience. It is to keep consistency with the hg support. We will discuss this at the meeting and decide if we want to split the experience.

* 39783a4 C
* c0bde99 B
* af6ba14 A

Changes implemented from c0bde99
```
$ git diff c0bde99
diff --git a/X b/X
index 6178079..f2ad6c7 100644
--- a/X
+++ b/X
@@ -1 +1 @@
-b
+c
```
Submit commits implementing changes from c0bde99 (inclusive)
```
$ moz-phab submit -b 1 c0bde99
Submitting 2 commits:
39783a48fe5d Bug 1 - C
c0bde998654a Bug 1 - B
```

I don't get the part about HEAD~2. It is identical to the git behavior.

Changes implemented by the last 2 commits:
```
$ git diff HEAD~2
diff --git a/X b/X
index 7898192..f2ad6c7 100644
--- a/X
+++ b/X
@@ -1 +1 @@
-a
+c
```
Submit the last 2 commits:
```
$ moz-phab submit -b 1 HEAD~2
Submitting 2 commits:
39783a48fe5d Bug 1 - C
c0bde998654a Bug 1 - B
```
```
$ moz-phab submit -b 1 HEAD~2 HEAD
Submitting 2 commits:
39783a48fe5d Bug 1 - C
c0bde998654a Bug 1 - B
```

Comment 3

[Jean-Yves Avenard [:jya]]

Hmmm, I could swear this used to submit 3 commits, not 2 like now.

however, things are still inconsistent, like:
HEAD~2 is SHA1 c35784311904eba3de079094cce8fa0018953545 on my local repo:

$ git log HEAD~2
commit c35784311904eba3de079094cce8fa0018953545
Author: Collin Wing <[e-mail]>
Date:   Mon Nov 12 02:10:10 2018 +0000

    Bug 1504751 Migrate about:networking to Fluent r=jaws,flod
    
    Differential Revision: https://phabricator.services.mozilla.com/D10977


$ ../moz-phab submit c35784311904eba3de079094cce8fa0018953545
Submitting 3 commits:
0ad23d7c86df Bug 1482059 - P2. Use new VPx frame parser to detect content change. r?bryce!
752b662b3ca7 Bug 1482059 - P1. Implement VP8/VP9 frame header parser. r?TD-Linux!
c35784311904 Bug 1504751 Migrate about:networking to Fluent r=jaws,flod

$ ../moz-phab submit HEAD~2
Submitting 2 commits:
0ad23d7c86df Bug 1482059 - P2. Use new VPx frame parser to detect content change. r?bryce!
752b662b3ca7 Bug 1482059 - P1. Implement VP8/VP9 frame header parser. r?TD-Linux!

There are the same commit, yet using HEAD~2 vs its actual SHA1 produce different result.

Comment 4

[Piotr Zalewa [:zalun]]

Yes - that was the first part of #c2

Comment 5

[Piotr Zalewa [:zalun]]

We've discussed this within the team.
We are gonna change the current behavior and follow the VCS's log.
Exclusive for Git and inclusive for Mercurial.

Comment 6

[Piotr Zalewa [:zalun]]

Attachment: Bug 1504534 - Follow local VCS `land` behavior in range selection. r=glob,dylan

Added test_integration for Git

Note:
The update test is skipped as the generated commit message has an
additional line