mirror of
https://github.com/facebook/sapling.git
synced 2024-10-10 00:45:18 +03:00
Do not mark commits as "landed as" if it contains local changes
Summary: If the user has modified the commit (ex. amend) and not sending it to Phabricator, we shouldn't mark the local commit as "landed" because it might contain changes that do not exist in the "master" branch. Reviewed By: quark-zju Differential Revision: D29401200 fbshipit-source-id: fc09921decdcffb8fc16e65d6a975c5a4154a96d
This commit is contained in:
Navid Qaragozlou
Facebook GitHub Bot
parent
a93602fa3f
commit
538bb71548
@ -170,39 +170,60 @@ class Client(object):
|
||||
]
|
||||
|
||||
def getlandednodes(self, repo, diffids, timeout=10):
|
||||
"""Get landed nodes for diffids. Return {diffid: node}"""
|
||||
"""Get landed nodes for diffids. Return {diffid: node}, {diffid: set(node)}"""
|
||||
if not diffids:
|
||||
return {}
|
||||
query = """
|
||||
query DiffToCommitQuery($diffids: [String!]!){
|
||||
phabricator_diff_query(query_params: {
|
||||
numbers: $diffids
|
||||
}) {
|
||||
results {
|
||||
nodes {
|
||||
number
|
||||
phabricator_diff_commit {
|
||||
nodes {
|
||||
commit_identifier
|
||||
return {}, {}
|
||||
if self._mock:
|
||||
ret = self._mocked_responses.pop()
|
||||
else:
|
||||
query = """
|
||||
query DiffToCommitQuery($diffids: [String!]!){
|
||||
phabricator_diff_query(query_params: {
|
||||
numbers: $diffids
|
||||
}) {
|
||||
results {
|
||||
nodes {
|
||||
number
|
||||
phabricator_versions {
|
||||
nodes {
|
||||
local_commits {
|
||||
primary_commit {
|
||||
commit_identifier
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
phabricator_diff_commit {
|
||||
nodes {
|
||||
commit_identifier
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
"""
|
||||
params = {"diffids": diffids}
|
||||
ret = self._client.query(timeout, query, params)
|
||||
# Example result:
|
||||
# { "data": {
|
||||
# "phabricator_diff_query": [
|
||||
# { "results": {
|
||||
# "nodes": [
|
||||
# { "phabricator_diff_commit": {
|
||||
# "nodes": [
|
||||
# { "commit_identifier": "9396e4a63208eb034b8b9cca909f9914cb2fbe85" } ] } } ] } } ] } }
|
||||
"""
|
||||
params = {"diffids": diffids}
|
||||
ret = self._client.query(timeout, query, params)
|
||||
# Example result:
|
||||
# { "data": {
|
||||
# "phabricator_diff_query": [
|
||||
# { "results": {"nodes": [{
|
||||
# "number": 123,
|
||||
# "phabricator_versions": {
|
||||
# "nodes": [
|
||||
# {"local_commits": [{"primary_commit": {"commit_identifier": "d131c2d7408acf233a4b2db04382005434346421"}}]},
|
||||
# {"local_commits": [{"primary_commit": {"commit_identifier": "a421db7622bf0c454ab19479f166fd4a3a4a41f5"}}]},
|
||||
# {"local_commits": []}]},
|
||||
# "phabricator_diff_commit": {
|
||||
# "nodes": [
|
||||
# { "commit_identifier": "9396e4a63208eb034b8b9cca909f9914cb2fbe85" } ] } } ] } } ] } }
|
||||
return self._getlandednodes(repo, ret)
|
||||
|
||||
def _getlandednodes(self, repo, ret):
|
||||
difftonode = {}
|
||||
difftoglobalrev = {}
|
||||
difftolocalcommits = {} # {str: set(node)}
|
||||
for result in ret["data"]["phabricator_diff_query"][0]["results"]["nodes"]:
|
||||
try:
|
||||
diffid = "%s" % result["number"]
|
||||
@ -216,6 +237,13 @@ class Client(object):
|
||||
elif identifier.isdigit():
|
||||
# This is probably a globalrev.
|
||||
difftoglobalrev[diffid] = identifier
|
||||
allversionnodes = result["phabricator_versions"]["nodes"]
|
||||
for version in allversionnodes:
|
||||
versioncommits = version["local_commits"]
|
||||
for commit in versioncommits:
|
||||
difftolocalcommits.setdefault(diffid, set()).add(
|
||||
bin(commit["primary_commit"]["commit_identifier"])
|
||||
)
|
||||
except (KeyError, IndexError, TypeError):
|
||||
# Not fatal.
|
||||
continue
|
||||
@ -235,8 +263,7 @@ class Client(object):
|
||||
node = globalrevtonode.get(globalrev)
|
||||
if node:
|
||||
difftonode[diffid] = node
|
||||
|
||||
return difftonode
|
||||
return difftonode, difftolocalcommits
|
||||
|
||||
def getrevisioninfo(self, timeout, signalstatus, *revision_numbers):
|
||||
rev_numbers = self._normalizerevisionnumbers(revision_numbers)
|
||||
|
||||
@ -31,6 +31,10 @@ from .phabstatus import COMMITTEDSTATUS, getdiffstatus
|
||||
cmdtable = {}
|
||||
command = registrar.command(cmdtable)
|
||||
|
||||
configtable = {}
|
||||
configitem = registrar.configitem(configtable)
|
||||
configitem("pullcreatemarkers", "check-local-versions", default=True)
|
||||
|
||||
|
||||
def _isrevert(message, diffid):
|
||||
result = ("Revert D%s" % diffid) in message
|
||||
@ -44,11 +48,11 @@ def _cleanuplanded(repo, dryrun=False):
|
||||
This uses mutation and visibility directly.
|
||||
"""
|
||||
limit = repo.ui.configint("pullcreatemarkers", "diff-limit", 100)
|
||||
difftodraft = {} # {str: node}
|
||||
difftodraft = {} # {str: {node}}
|
||||
for ctx in repo.set("sort(draft() - obsolete(), -rev)"):
|
||||
diffid = diffprops.parserevfromcommitmsg(ctx.description()) # str or None
|
||||
if diffid and not _isrevert(ctx.description(), diffid):
|
||||
difftodraft.setdefault(diffid, []).append(ctx.node())
|
||||
difftodraft.setdefault(diffid, set()).add(ctx.node())
|
||||
# Bound the number of diffs we query from Phabricator.
|
||||
if len(difftodraft) >= limit:
|
||||
break
|
||||
@ -56,7 +60,9 @@ def _cleanuplanded(repo, dryrun=False):
|
||||
ui = repo.ui
|
||||
try:
|
||||
client = graphql.Client(repo=repo)
|
||||
difftopublic = client.getlandednodes(repo, list(difftodraft.keys()))
|
||||
difftopublic, difftolocal = client.getlandednodes(
|
||||
repo, list(difftodraft.keys())
|
||||
)
|
||||
except KeyboardInterrupt:
|
||||
ui.warn(
|
||||
_(
|
||||
@ -76,8 +82,11 @@ def _cleanuplanded(repo, dryrun=False):
|
||||
mutationentries = []
|
||||
tohide = set()
|
||||
markedcount = 0
|
||||
checklocalversions = ui.configbool("pullcreatemarkers", "check-local-versions")
|
||||
for diffid, draftnodes in sorted(difftodraft.items()):
|
||||
publicnode = difftopublic.get(diffid)
|
||||
if checklocalversions:
|
||||
draftnodes = draftnodes & difftolocal.get(diffid)
|
||||
if publicnode is None or publicnode not in unfi:
|
||||
continue
|
||||
# skip it if the local repo does not think it's a public commit.
|
||||
@ -86,7 +95,9 @@ def _cleanuplanded(repo, dryrun=False):
|
||||
# sanity check - the public commit should have a sane commit message.
|
||||
if diffprops.parserevfromcommitmsg(unfi[publicnode].description()) != diffid:
|
||||
continue
|
||||
draftnodestr = ", ".join(short(d) for d in draftnodes)
|
||||
draftnodestr = ", ".join(
|
||||
short(d) for d in sorted(draftnodes)
|
||||
) # making output deterministic
|
||||
if ui.verbose:
|
||||
ui.write(
|
||||
_("marking D%s (%s) as landed as %s\n")
|
||||
|
||||
@ -0,0 +1,89 @@
|
||||
#chg-compatible
|
||||
|
||||
Setup
|
||||
$ configure modern
|
||||
$ enable pullcreatemarkers
|
||||
|
||||
Configure arc...
|
||||
$ echo '{}' > .arcrc
|
||||
$ echo '{"config" : {"default" : "https://a.com/api"}, "hosts" : {"https://a.com/api/" : { "user" : "testuser", "oauth" : "garbage_cert"}}}' > .arcconfig
|
||||
|
||||
Test that hg pull creates mutation records for landed diffs
|
||||
$ mkcommit() {
|
||||
> echo "$1" > "$1"
|
||||
> hg add "$1"
|
||||
> echo "add $1" > msg
|
||||
> echo "" >> msg
|
||||
> [ -z "$2" ] || echo "Differential Revision: https://phabricator.fb.com/D$2" >> msg
|
||||
> hg ci -l msg
|
||||
> }
|
||||
$ mkamend() {
|
||||
> hg log -r. -T'{desc}\n' > msg
|
||||
> echo "Reviewed By: someone" >> msg
|
||||
> hg ci --amend -l msg
|
||||
> }
|
||||
|
||||
Set up repository with 1 public and 2 local commits
|
||||
$ newremoterepo
|
||||
$ setconfig paths.default=test:e1
|
||||
$ mkcommit initial 123 # 123 is the phabricator rev number (see function above)
|
||||
$ hg debugmakepublic 'desc(init)'
|
||||
$ mkcommit b 123
|
||||
$ mkcommit c 123
|
||||
|
||||
|
||||
Setup phabricator response
|
||||
$ cat > $TESTTMP/mockduit << EOF
|
||||
> [{
|
||||
> "data": {
|
||||
> "phabricator_diff_query": [
|
||||
> {
|
||||
> "results": {
|
||||
> "nodes": [
|
||||
> {
|
||||
> "number": 123,
|
||||
> "phabricator_versions": {
|
||||
> "nodes": [
|
||||
> {"local_commits": [{"primary_commit": {"commit_identifier": "d131c2d7408acf233a4b2db04382005434346421"}}]},
|
||||
> {"local_commits": [{"primary_commit": {"commit_identifier": "a421db7622bf0c454ab19479f166fd4a3a4a41f5"}}]},
|
||||
> {"local_commits": []}
|
||||
> ]
|
||||
> },
|
||||
> "phabricator_diff_commit": {
|
||||
> "nodes": [
|
||||
> {"commit_identifier": "23bffadc9066efde1d8e9f53ee3d5ea9da04ff1b"}
|
||||
> ]
|
||||
> }
|
||||
> }
|
||||
> ]
|
||||
> }
|
||||
> }
|
||||
> ]
|
||||
> },
|
||||
> "extensions": {
|
||||
> "is_final": true
|
||||
> }
|
||||
> }]
|
||||
> EOF
|
||||
|
||||
Test that commit hashes matching GraphQL are marked as landed
|
||||
$ HG_ARC_CONDUIT_MOCK=$TESTTMP/mockduit hg debugmarklanded --verbose --dry-run
|
||||
marking D123 (a421db7622bf, d131c2d7408a) as landed as 23bffadc9066
|
||||
marked 2 commits as landed
|
||||
(this is a dry-run, nothing was actually done)
|
||||
|
||||
Setup amend local commit
|
||||
$ mkamend
|
||||
|
||||
Test that if the commit hash is changed, then it's no longer marked as landed.
|
||||
$ HG_ARC_CONDUIT_MOCK=$TESTTMP/mockduit hg debugmarklanded --verbose --dry-run
|
||||
marking D123 (a421db7622bf) as landed as 23bffadc9066
|
||||
marked 1 commit as landed
|
||||
(this is a dry-run, nothing was actually done)
|
||||
|
||||
Test that original behavior of marking local commits as landed even if hashes don't match GraphQL preserves
|
||||
$ setconfig pullcreatemarkers.check-local-versions=False
|
||||
$ HG_ARC_CONDUIT_MOCK=$TESTTMP/mockduit hg debugmarklanded --verbose --dry-run
|
||||
marking D123 (3b86866eb2ba, a421db7622bf) as landed as 23bffadc9066
|
||||
marked 2 commits as landed
|
||||
(this is a dry-run, nothing was actually done)
|
||||
Loading…
Reference in New Issue
Block a user