Mercurial > hg
comparison hgext/phabricator.py @ 44717:3dc6a70779f2
phabricator: add an option to fold several commits into one review (issue6244)
Now that all of the pieces are in place, alter the user facing command to allow
it. This is the default behavior when using `arc`, but I much prefer the 1:1
approach, and I'm tempted to mark this advanced to limit its abuse. I started
out calling this `--no-stack` like the feature request suggested, but I found it
less obvious (especially when writing the code), so I went with the `hg fold`
analogue.
This will populate the `Commits` tab in the web UI with the hash of each commit
folded into the review. From experimentation, it seems to list them in the
order they are received from the extension instead of the actual parent/child
relationship. The extension sends them in sorted order, thanks to
`templatefilters.json()`. Since there's enough info there for them to put
things in the right order, JSON is unordered aside from lists (IIUC), and there
doesn't seem to be any harmful side effects, I guess we write this off as their
bug. It is simple enough to workaround by putting a check for `util.sortdict`
into `templatefilters.json()`, and don't resort in that case.
There are a handful of restrictions that are documented in the code, which
somebody could probably fix if they're interested. Notably, this requires the
(default) `--amend` option, because there's not an easy way to apply a local tag
across several commits. This also doesn't do preflight checking to ensure that
all previous commits that were part of a single review are selected when
updating. That seems expensive. What happens is the excluded commit is dropped
from the review, but it keeps the Differential Revision line in the commit
message. Not everything can be edited, so it doesn't seem worth making the code
even more complicated to handle this edge case.
There are a couple of "obsolete feature not enabled but X markers found!"
messages that appeared on Windows but not macOS. I have no idea what's going on
here, but that's an unrelated issue, so I conditionalized those lines.
Differential Revision: https://phab.mercurial-scm.org/D8314
author | Matt Harbison <matt_harbison@yahoo.com> |
---|---|
date | Wed, 08 Apr 2020 17:30:10 -0400 |
parents | 38f7b2f02f6d |
children | 0680b8a1992a |
comparison
equal
deleted
inserted
replaced
44716:ed81fa859426 | 44717:3dc6a70779f2 |
---|---|
557 | 557 |
558 # If this commit was the result of `hg fold` after submission, | 558 # If this commit was the result of `hg fold` after submission, |
559 # and now resubmitted with --fold, the easiest thing to do is | 559 # and now resubmitted with --fold, the easiest thing to do is |
560 # to leave the node clear. This only results in creating a new | 560 # to leave the node clear. This only results in creating a new |
561 # diff for the _same_ Differential Revision if this commit is | 561 # diff for the _same_ Differential Revision if this commit is |
562 # the first or last in the selected range. | 562 # the first or last in the selected range. If we picked a node |
563 # from the list instead, it would have to be the lowest if at | |
564 # the beginning of the --fold range, or the highest at the end. | |
565 # Otherwise, one or more of the nodes wouldn't be considered in | |
566 # the diff, and the Differential wouldn't be properly updated. | |
563 # If this commit is the result of `hg split` in the same | 567 # If this commit is the result of `hg split` in the same |
564 # scenario, there is a single oldnode here (and multiple | 568 # scenario, there is a single oldnode here (and multiple |
565 # newnodes mapped to it). That makes it the same as the normal | 569 # newnodes mapped to it). That makes it the same as the normal |
566 # case, as the edges of the newnode range cleanly maps to one | 570 # case, as the edges of the newnode range cleanly maps to one |
567 # oldnode each. | 571 # oldnode each. |
1248 b'comment', | 1252 b'comment', |
1249 b'', | 1253 b'', |
1250 _(b'add a comment to Revisions with new/updated Diffs'), | 1254 _(b'add a comment to Revisions with new/updated Diffs'), |
1251 ), | 1255 ), |
1252 (b'', b'confirm', None, _(b'ask for confirmation before sending')), | 1256 (b'', b'confirm', None, _(b'ask for confirmation before sending')), |
1257 (b'', b'fold', False, _(b'combine the revisions into one review')), | |
1253 ], | 1258 ], |
1254 _(b'REV [OPTIONS]'), | 1259 _(b'REV [OPTIONS]'), |
1255 helpcategory=command.CATEGORY_IMPORT_EXPORT, | 1260 helpcategory=command.CATEGORY_IMPORT_EXPORT, |
1256 ) | 1261 ) |
1257 def phabsend(ui, repo, *revs, **opts): | 1262 def phabsend(ui, repo, *revs, **opts): |
1276 behaviour:: | 1281 behaviour:: |
1277 | 1282 |
1278 [phabsend] | 1283 [phabsend] |
1279 confirm = true | 1284 confirm = true |
1280 | 1285 |
1286 By default, a separate review will be created for each commit that is | |
1287 selected, and will have the same parent/child relationship in Phabricator. | |
1288 If ``--fold`` is set, multiple commits are rolled up into a single review | |
1289 as if diffed from the parent of the first revision to the last. The commit | |
1290 messages are concatenated in the summary field on Phabricator. | |
1291 | |
1281 phabsend will check obsstore and the above association to decide whether to | 1292 phabsend will check obsstore and the above association to decide whether to |
1282 update an existing Differential Revision, or create a new one. | 1293 update an existing Differential Revision, or create a new one. |
1283 """ | 1294 """ |
1284 opts = pycompat.byteskwargs(opts) | 1295 opts = pycompat.byteskwargs(opts) |
1285 revs = list(revs) + opts.get(b'rev', []) | 1296 revs = list(revs) + opts.get(b'rev', []) |
1288 | 1299 |
1289 if not revs: | 1300 if not revs: |
1290 raise error.Abort(_(b'phabsend requires at least one changeset')) | 1301 raise error.Abort(_(b'phabsend requires at least one changeset')) |
1291 if opts.get(b'amend'): | 1302 if opts.get(b'amend'): |
1292 cmdutil.checkunfinished(repo) | 1303 cmdutil.checkunfinished(repo) |
1304 | |
1305 ctxs = [repo[rev] for rev in revs] | |
1306 | |
1307 fold = opts.get(b'fold') | |
1308 if fold: | |
1309 if len(revs) == 1: | |
1310 # TODO: just switch to --no-fold instead? | |
1311 raise error.Abort(_(b"cannot fold a single revision")) | |
1312 | |
1313 # There's no clear way to manage multiple commits with a Dxxx tag, so | |
1314 # require the amend option. (We could append "_nnn", but then it | |
1315 # becomes jumbled if earlier commits are added to an update.) It should | |
1316 # lock the repo and ensure that the range is editable, but that would | |
1317 # make the code pretty convoluted. The default behavior of `arc` is to | |
1318 # create a new review anyway. | |
1319 if not opts.get(b"amend"): | |
1320 raise error.Abort(_(b"cannot fold with --no-amend")) | |
1321 | |
1322 # Ensure the local commits are an unbroken range | |
1323 revrange = repo.revs(b'(first(%ld)::last(%ld))', revs, revs) | |
1324 if any(r for r in revs if r not in revrange) or any( | |
1325 r for r in revrange if r not in revs | |
1326 ): | |
1327 raise error.Abort(_(b"cannot fold non-linear revisions")) | |
1328 | |
1329 # It might be possible to bucketize the revisions by the DREV value, and | |
1330 # iterate over those groups when posting, and then again when amending. | |
1331 # But for simplicity, require all selected revisions to be for the same | |
1332 # DREV (if present). Adding local revisions to an existing DREV is | |
1333 # acceptable. | |
1334 drevmatchers = [ | |
1335 _differentialrevisiondescre.search(ctx.description()) | |
1336 for ctx in ctxs | |
1337 ] | |
1338 if len({m.group('url') for m in drevmatchers if m}) > 1: | |
1339 raise error.Abort( | |
1340 _(b"cannot fold revisions with different DREV values") | |
1341 ) | |
1293 | 1342 |
1294 # {newnode: (oldnode, olddiff, olddrev} | 1343 # {newnode: (oldnode, olddiff, olddrev} |
1295 oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs]) | 1344 oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs]) |
1296 | 1345 |
1297 confirm = ui.configbool(b'phabsend', b'confirm') | 1346 confirm = ui.configbool(b'phabsend', b'confirm') |
1321 diffmap = {} # {newnode: diff} | 1370 diffmap = {} # {newnode: diff} |
1322 | 1371 |
1323 # Send patches one by one so we know their Differential Revision PHIDs and | 1372 # Send patches one by one so we know their Differential Revision PHIDs and |
1324 # can provide dependency relationship | 1373 # can provide dependency relationship |
1325 lastrevphid = None | 1374 lastrevphid = None |
1326 for rev in revs: | 1375 for ctx in ctxs: |
1327 ui.debug(b'sending rev %d\n' % rev) | 1376 if fold: |
1328 ctx = repo[rev] | 1377 ui.debug(b'sending rev %d::%d\n' % (ctx.rev(), ctxs[-1].rev())) |
1378 else: | |
1379 ui.debug(b'sending rev %d\n' % ctx.rev()) | |
1329 | 1380 |
1330 # Get Differential Revision ID | 1381 # Get Differential Revision ID |
1331 oldnode, olddiff, revid = oldmap.get(ctx.node(), (None, None, None)) | 1382 oldnode, olddiff, revid = oldmap.get(ctx.node(), (None, None, None)) |
1332 oldbasenode = oldnode | 1383 oldbasenode, oldbasediff, oldbaserevid = oldnode, olddiff, revid |
1384 | |
1385 if fold: | |
1386 oldbasenode, oldbasediff, oldbaserevid = oldmap.get( | |
1387 ctxs[-1].node(), (None, None, None) | |
1388 ) | |
1389 | |
1333 if oldnode != ctx.node() or opts.get(b'amend'): | 1390 if oldnode != ctx.node() or opts.get(b'amend'): |
1334 # Create or update Differential Revision | 1391 # Create or update Differential Revision |
1335 revision, diff = createdifferentialrevision( | 1392 revision, diff = createdifferentialrevision( |
1336 [ctx], | 1393 ctxs if fold else [ctx], |
1337 revid, | 1394 revid, |
1338 lastrevphid, | 1395 lastrevphid, |
1339 oldbasenode, | 1396 oldbasenode, |
1340 oldnode, | 1397 oldnode, |
1341 olddiff, | 1398 olddiff, |
1342 actions, | 1399 actions, |
1343 opts.get(b'comment'), | 1400 opts.get(b'comment'), |
1344 ) | 1401 ) |
1345 diffmap[ctx.node()] = diff | 1402 |
1403 if fold: | |
1404 for ctx in ctxs: | |
1405 diffmap[ctx.node()] = diff | |
1406 else: | |
1407 diffmap[ctx.node()] = diff | |
1408 | |
1346 newrevid = int(revision[b'object'][b'id']) | 1409 newrevid = int(revision[b'object'][b'id']) |
1347 newrevphid = revision[b'object'][b'phid'] | 1410 newrevphid = revision[b'object'][b'phid'] |
1348 if revid: | 1411 if revid: |
1349 action = b'updated' | 1412 action = b'updated' |
1350 else: | 1413 else: |
1351 action = b'created' | 1414 action = b'created' |
1352 | 1415 |
1353 # Create a local tag to note the association, if commit message | 1416 # Create a local tag to note the association, if commit message |
1354 # does not have it already | 1417 # does not have it already |
1355 m = _differentialrevisiondescre.search(ctx.description()) | 1418 if not fold: |
1356 if not m or int(m.group('id')) != newrevid: | 1419 m = _differentialrevisiondescre.search(ctx.description()) |
1357 tagname = b'D%d' % newrevid | 1420 if not m or int(m.group('id')) != newrevid: |
1358 tags.tag( | 1421 tagname = b'D%d' % newrevid |
1359 repo, | 1422 tags.tag( |
1360 tagname, | 1423 repo, |
1361 ctx.node(), | 1424 tagname, |
1362 message=None, | 1425 ctx.node(), |
1363 user=None, | 1426 message=None, |
1364 date=None, | 1427 user=None, |
1365 local=True, | 1428 date=None, |
1366 ) | 1429 local=True, |
1430 ) | |
1367 else: | 1431 else: |
1368 # Nothing changed. But still set "newrevphid" so the next revision | 1432 # Nothing changed. But still set "newrevphid" so the next revision |
1369 # could depend on this one and "newrevid" for the summary line. | 1433 # could depend on this one and "newrevid" for the summary line. |
1370 newrevphid = querydrev(repo.ui, b'%d' % revid)[0][b'phid'] | 1434 newrevphid = querydrev(repo.ui, b'%d' % revid)[0][b'phid'] |
1371 newrevid = revid | 1435 newrevid = revid |
1372 action = b'skipped' | 1436 action = b'skipped' |
1373 | 1437 |
1374 drevids.append(newrevid) | 1438 drevids.append(newrevid) |
1375 lastrevphid = newrevphid | 1439 lastrevphid = newrevphid |
1440 | |
1441 if fold: | |
1442 for c in ctxs: | |
1443 if oldmap.get(c.node(), (None, None, None))[2]: | |
1444 action = b'updated' | |
1445 else: | |
1446 action = b'created' | |
1447 _print_phabsend_action(ui, c, newrevid, action) | |
1448 break | |
1376 | 1449 |
1377 _print_phabsend_action(ui, ctx, newrevid, action) | 1450 _print_phabsend_action(ui, ctx, newrevid, action) |
1378 | 1451 |
1379 # Update commit messages and remove tags | 1452 # Update commit messages and remove tags |
1380 if opts.get(b'amend'): | 1453 if opts.get(b'amend'): |
1381 unfi = repo.unfiltered() | 1454 unfi = repo.unfiltered() |
1382 drevs = callconduit(ui, b'differential.query', {b'ids': drevids}) | 1455 drevs = callconduit(ui, b'differential.query', {b'ids': drevids}) |
1383 with repo.wlock(), repo.lock(), repo.transaction(b'phabsend'): | 1456 with repo.wlock(), repo.lock(), repo.transaction(b'phabsend'): |
1384 wnode = unfi[b'.'].node() | 1457 wnode = unfi[b'.'].node() |
1385 mapping = {} # {oldnode: [newnode]} | 1458 mapping = {} # {oldnode: [newnode]} |
1459 newnodes = [] | |
1460 | |
1461 drevid = drevids[0] | |
1462 | |
1386 for i, rev in enumerate(revs): | 1463 for i, rev in enumerate(revs): |
1387 old = unfi[rev] | 1464 old = unfi[rev] |
1388 drevid = drevids[i] | 1465 if not fold: |
1466 drevid = drevids[i] | |
1389 drev = [d for d in drevs if int(d[b'id']) == drevid][0] | 1467 drev = [d for d in drevs if int(d[b'id']) == drevid][0] |
1390 newdesc = get_amended_desc(drev, old, False) | 1468 |
1469 newdesc = get_amended_desc(drev, old, fold) | |
1391 # Make sure commit message contain "Differential Revision" | 1470 # Make sure commit message contain "Differential Revision" |
1392 if old.description() != newdesc: | 1471 if old.description() != newdesc: |
1393 if old.phase() == phases.public: | 1472 if old.phase() == phases.public: |
1394 ui.warn( | 1473 ui.warn( |
1395 _(b"warning: not updating public commit %s\n") | 1474 _(b"warning: not updating public commit %s\n") |
1412 | 1491 |
1413 newnode = new.commit() | 1492 newnode = new.commit() |
1414 | 1493 |
1415 mapping[old.node()] = [newnode] | 1494 mapping[old.node()] = [newnode] |
1416 | 1495 |
1496 if fold: | |
1497 # Defer updating the (single) Diff until all nodes are | |
1498 # collected. No tags were created, so none need to be | |
1499 # removed. | |
1500 newnodes.append(newnode) | |
1501 continue | |
1502 | |
1417 _amend_diff_properties( | 1503 _amend_diff_properties( |
1418 unfi, drevid, [newnode], diffmap[old.node()] | 1504 unfi, drevid, [newnode], diffmap[old.node()] |
1419 ) | 1505 ) |
1420 # Remove local tags since it's no longer necessary | 1506 |
1421 tagname = b'D%d' % drevid | 1507 # Remove local tags since it's no longer necessary |
1422 if tagname in repo.tags(): | 1508 tagname = b'D%d' % drevid |
1423 tags.tag( | 1509 if tagname in repo.tags(): |
1424 repo, | 1510 tags.tag( |
1425 tagname, | 1511 repo, |
1426 nullid, | 1512 tagname, |
1427 message=None, | 1513 nullid, |
1428 user=None, | 1514 message=None, |
1429 date=None, | 1515 user=None, |
1430 local=True, | 1516 date=None, |
1517 local=True, | |
1518 ) | |
1519 elif fold: | |
1520 # When folding multiple commits into one review with | |
1521 # --fold, track even the commits that weren't amended, so | |
1522 # that their association isn't lost if the properties are | |
1523 # rewritten below. | |
1524 newnodes.append(old.node()) | |
1525 | |
1526 # If the submitted commits are public, no amend takes place so | |
1527 # there are no newnodes and therefore no diff update to do. | |
1528 if fold and newnodes: | |
1529 diff = diffmap[old.node()] | |
1530 | |
1531 # The diff object in diffmap doesn't have the local commits | |
1532 # because that could be returned from differential.creatediff, | |
1533 # not differential.querydiffs. So use the queried diff (if | |
1534 # present), or force the amend (a new revision is being posted.) | |
1535 if not olddiff or set(newnodes) != getlocalcommits(olddiff): | |
1536 _debug(ui, b"updating local commit list for D%d\n" % drevid) | |
1537 _amend_diff_properties(unfi, drevid, newnodes, diff) | |
1538 else: | |
1539 _debug( | |
1540 ui, | |
1541 b"local commit list for D%d is already up-to-date\n" | |
1542 % drevid, | |
1431 ) | 1543 ) |
1544 elif fold: | |
1545 _debug(ui, b"no newnodes to update\n") | |
1546 | |
1432 scmutil.cleanupnodes(repo, mapping, b'phabsend', fixphase=True) | 1547 scmutil.cleanupnodes(repo, mapping, b'phabsend', fixphase=True) |
1433 if wnode in mapping: | 1548 if wnode in mapping: |
1434 unfi.setparents(mapping[wnode][0]) | 1549 unfi.setparents(mapping[wnode][0]) |
1435 | 1550 |
1436 | 1551 |