Closed Bug 1926924 Opened 9 days ago Closed 8 days ago

moz-phab creates Phabricator diffs from currently checked out commit instead of the actual commit

Categories

(Conduit :: moz-phab, defect)

Production
Desktop
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bthrall, Assigned: shtrom)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files)

moz-phab version (from moz-phab version): MozPhab 1.8.0 (Python 3.10.12, Linux)

When I run moz-phab submit from the last commit in a stack of commits, it sends incorrect diffs to Phabricator. Instead of the diff that I see in each commit, the Phabricator diff looks like the differences between the currently checked out commit in my working directory and the commit for the Phabricator patch.

For example, see D218444. This diff was submitted with my working directory checked out to D226717 and includes all the changes from D218444 through D226717. It should only include the changes for D218444 (see here).

The local commit message seems to correctly refer to the Phabricator patch because I see updates in Phabricator when I run moz-phab submit:

Bug 1724236 - Move profiler label and JSAutoRealm out of JSExecutionContext r=arai!

Differential Revision: https://phabricator.services.mozilla.com/D218444

This problem affected almost all the patches in the stack, but for some reason not the last two, D222299 and D226717.

Checking out the commit associated with the Phabricator patch and doing moz-phab submit -s -f sends the correct diff to Phabricator.

Flags: needinfo?(omehani)
Assignee: nobody → omehani
Status: NEW → ASSIGNED
Flags: needinfo?(omehani)

I suspect this is due to this commit https://github.com/mozilla-conduit/review/commit/2a4296d749e43e5eb8da833837eeca46257e4274, which impacts both Git and Hg. Git doesn't need it anymore, but it looks like Hg does.

This step was removed from the main submit logic, as it was no longer
needed by git, and looked unecessary for hg. Not so!

When computing the diffs, the Hg logic seems to take into account the
current state of the working directory. This leads to incorrect diffs
for anything but the last commit of a stack.

You can give hg two revisions and it will compute the diff between them, with -r x::y

I have a reproduction. This only happens for commits that change the same files.

Attachment #9433217 - Attachment description: mercurial: checkout target commit before get_diff (Bug 1926924) r?sheehan → mercurial: checkout target commit before get_diff (Bug 1926924) r=sheehan
Attachment #9433242 - Attachment description: test_integration_hg: add regression test for diff generation without checkout (Bug 1926924) r?sheehan → test_integration_hg: add regression test for diff generation without checkout (Bug 1926924) r=sheehan

This is fixed in moz-phab 1.8.1.

Status: ASSIGNED → RESOLVED
Closed: 8 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: