ghstack: use . instead of max(decendents(.)) for HEAD

[nemith]

Stack created with Sapling. Best reviewed with ReviewStack.

• -> ghstack: use . instead of max(decendents(.)) for HEAD #310

ghstack: use . instead of max(decendents(.)) for HEAD

The original ghstack used HEAD to represent the current commit in the stack for submitting prs. When adapting to sl HEAD was replaced with max(decedants(.)) which means operations will occur on everything in the stack which isn't desirable. This changed HEAD to mean the current revision . which seems to be more lile what should be intended.

Test Plan:

$ sl
o  5492cbc35  1 second ago  brandonbennett
│  qux
│
@  90e41acf3  1 second ago  brandonbennett
│  bar
│
o  3f858f7d7  2 seconds ago  brandonbennett
│  foo
│
o  b3cd6c01e  Nov 29 at 09:30  remote/main

$ sl ghstack submit -v
$ /opt/homebrew/bin/sl config paths.default
$ /opt/homebrew/bin/sl log -T '{node}' -r 'ancestor(., main)'
$ /opt/homebrew/bin/sl log -r 'max(descendants(.))' -T '{node}'
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-list --header '^b3cd6c01e96f7b2a50294d97cb8a60f327cd9875' 5492cbc354beb62cd332afb10580a196fc40842f
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-list --header '^b3cd6c01e96f7b2a50294d97cb8a60f327cd9875^@' b3cd6c01e96f7b2a50294d97cb8a60f327cd9875
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-parse '3f858f7d739ea8e0a7569f257a1e65669669e7fc~^{tree}'
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git config --get commit.gpgsign
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git commit-tree -p b3cd6c01e96f7b2a50294d97cb8a60f327cd9875 dae37a0b9fecafee09c60547d255659eb9b5ed44
$ /opt/homebrew/bin/sl config paths.default
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git push https://github.com/nemith/testsap.git f4cfcabfbd9332ecee2963d798356d2150f1e2f7:refs/heads/gh/nemith/13/head b3cd6c01e96f7b2a50294d97cb8a60f327cd9875:refs/heads/gh/nemith/13/base
^Cinterrupted!
 brandonbennett  HQ-QX0FQ102LC  ~  Code  testsap  %  sl ghstack submit -v
 brandonbennett  HQ-QX0FQ102LC  ~  Code  testsap  %  CHGDISABLE=1 ../sapling/eden/scm/sl ghstack submit -v
$ /Users/brandonbennett/Code/testsap/../sapling/eden/scm/sl config paths.default
$ /Users/brandonbennett/Code/testsap/../sapling/eden/scm/sl log -T '{node}' -r 'ancestor(., main)'
$ /Users/brandonbennett/Code/testsap/../sapling/eden/scm/sl log -r . -T '{node}'
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-list --header '^b3cd6c01e96f7b2a50294d97cb8a60f327cd9875' 90e41acf3e07f943e069e2e9f0986966dc618950
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-list --header '^b3cd6c01e96f7b2a50294d97cb8a60f327cd9875^@' b3cd6c01e96f7b2a50294d97cb8a60f327cd9875
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-parse '3f858f7d739ea8e0a7569f257a1e65669669e7fc~^{tree}'
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git config --get commit.gpgsign
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git commit-tree -p b3cd6c01e96f7b2a50294d97cb8a60f327cd9875 dae37a0b9fecafee09c60547d255659eb9b5ed44
$ /Users/brandonbennett/Code/testsap/../sapling/eden/scm/sl config paths.default
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git push https://github.com/nemith/testsap.git 48a4ce09bc3d1541c8961f7d0d1bf893d677ae96:refs/heads/gh/nemith/13/head b3cd6c01e96f7b2a50294d97cb8a60f327cd9875:refs/heads/gh/nemith/13/base
To https://github.com/nemith/testsap.git
 * [new branch]      48a4ce09bc3d1541c8961f7d0d1bf893d677ae96 -> gh/nemith/13/head
 * [new branch]      b3cd6c01e96f7b2a50294d97cb8a60f327cd9875 -> gh/nemith/13/base
Opened PR #16
$ /Users/brandonbennett/Code/testsap/../sapling/eden/scm/sl metaedit -q -T '{nodechanges|json}' -r 3f858f7d739ea8e0a7569f257a1e65669669e7fc -m 'foo

ghstack-source-id: dae37a0b9fecafee09c60547d255659eb9b5ed44
Pull Request resolved: https://github.com/nemith/testsap/pull/16'
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-list --header '7a2207df4337649d2525adfc47b79ae4f9e068c7^..e18ffe0b3c2e68e44ae62c1a76d3f699da8b77bb'
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-parse 'e18ffe0b3c2e68e44ae62c1a76d3f699da8b77bb~^{tree}'
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git commit-tree -p 48a4ce09bc3d1541c8961f7d0d1bf893d677ae96 defbe41b8836c18ffeaa72c5c748cdd1e0095911
$ /Users/brandonbennett/Code/testsap/../sapling/eden/scm/sl config paths.default
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git push https://github.com/nemith/testsap.git 9850b01b776593f4cee46d805ea77738cc10fc1c:refs/heads/gh/nemith/14/head 48a4ce09bc3d1541c8961f7d0d1bf893d677ae96:refs/heads/gh/nemith/14/base
To https://github.com/nemith/testsap.git
 * [new branch]      9850b01b776593f4cee46d805ea77738cc10fc1c -> gh/nemith/14/head
 * [new branch]      48a4ce09bc3d1541c8961f7d0d1bf893d677ae96 -> gh/nemith/14/base
Opened PR #17
$ /Users/brandonbennett/Code/testsap/../sapling/eden/scm/sl metaedit -q -T '{nodechanges|json}' -r e18ffe0b3c2e68e44ae62c1a76d3f699da8b77bb -m 'bar

ghstack-source-id: defbe41b8836c18ffeaa72c5c748cdd1e0095911
Pull Request resolved: https://github.com/nemith/testsap/pull/17'
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git rev-list --header 'd3fb6cff6249f5bfe563633418864b07e7cb636d^..d3fb6cff6249f5bfe563633418864b07e7cb636d'
# Updating https://github.com/nemith/testsap/pull/16
# Updating https://github.com/nemith/testsap/pull/17
$ /Users/brandonbennett/Code/testsap/../sapling/eden/scm/sl config paths.default
$ git --git-dir /Users/brandonbennett/Code/testsap/.sl/store/git push https://github.com/nemith/testsap.git --force 7a2207df4337649d2525adfc47b79ae4f9e068c7:refs/heads/gh/nemith/13/orig d3fb6cff6249f5bfe563633418864b07e7cb636d:refs/heads/gh/nemith/14/orig
To https://github.com/nemith/testsap.git
 * [new branch]      7a2207df4337649d2525adfc47b79ae4f9e068c7 -> gh/nemith/13/orig
 * [new branch]      d3fb6cff6249f5bfe563633418864b07e7cb636d -> gh/nemith/14/orig

# Summary of changes (ghstack 0.6.0.sapling)

 - Created https://github.com/nemith/testsap/pull/17
 - Created https://github.com/nemith/testsap/pull/16

Facebook employees can import your changes by running
(on a Facebook machine):

    ghimport -s https://github.com/nemith/testsap/pull/17

If you want to work on this diff stack on another machine:

    ghstack checkout https://github.com/nemith/testsap/pull/17

Fixes: #283

[facebook-github-bot]

Hi @nemith!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[bolinfest]

Question because I want to be sure I understand the change you are trying to make:

• if you have A,B,C where A is the top of the stack
• you swap the bottom commits so that you have A,C,B
• you are on C and run sl ghstack, what should happen?

If you only update the PRs for C and B, then A is "wrong" because it will still show A,B,C. Shouldn't all three PRs get updated?

[bolinfest]

Ah, I took a look at the discussion over at ezyang/ghstack#101. In my previous example, I think there should be different behavior depending on whether there is an existing PR associated with A when you run sl ghstack. That is:

• If descendant commits have PRs, they should also be updated, from . through the youngest descendant that has a PR associated with it.
• If descendant commits do not have PRs, then things should work as you have written.

[nemith]

I think that makes sense. I am not really sure the behavior for not updating a stack member or what would happen. In this case I assume it would be user error and something the user could fix (but perhaps it would be too damaging to the stack to work right).

How does ghstack development go from here? This behavior will mean more than a shim layer for running commands and not sure how the team wants to go with rebasing and importing upstream changes? Should there be a "SCM" abc that would define the behavior of sl vs hg? Should changes be submitted to ezyang first?

As of right now ghstack sorta breaks trunk based development for me and reviewstack isn't an option so I am sorta motivated to get this working.

[bolinfest]

@nemith I think this is the code to modify:

sapling/eden/scm/ghstack/submit.py

Lines 163 to 167 in 520df27

	# compute the stack of commits to process (reverse chronological order),
	stack = ghstack.git.parse_header(
	sh.git("rev-list", "--header", "^" + base, "HEAD"),
	github_url,
	)

I think that rather than trying to do funny stuff in sapling_shell.py, we should just do a test around this code like:

if sh.is_sapling():
    # Use special methods of SaplingShell to determine the appropriate hash
    # to assign to tip_ref. This is where we would check the descendants.
    # Doing this correctly like requires checking via PullRequestStore.find_pull_request()
    # in eden/scm/edenscm/ext/github/pullrequeststore.py.
    pass
else:
    tip_ref = "HEAD"

# compute the stack of commits to process (reverse chronological order),
stack = ghstack.git.parse_header(
    sh.git("rev-list", "--header", "^" + base, tip_ref),
    github_url,
)

[bolinfest]

I have a fix ready for this locally. The diff roughly looks like this:

-            top = self._run_sapling_command(['log', '-r', 'max(descendants(.))', '-T', '{node}'])
+            # Approximate `sl whereami`.
+            p1 = self.repo.dirstate.p1()
+            if p1 == nullid:
+                raise error.Abort(_("could not find a current commit hash"))
+
             for index, arg in enumerate(args):
                 if arg == 'HEAD':
-                    args[index] = top
+                    args[index] = hex(p1)

[nemith]

@bolinfest I can close this PR if you have a fix. It would probably take me more time to grok the code to do it properly.

[bolinfest]

Now that 21579f0 is in, I'm going to close this! We plan to do an official release this week, which will include this fix.