Merge branch 'debian/experimental' into debian/master

9 files changed, 195 insertions, 144 deletions

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 0c30038..564a5c2 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -1,5 +1,5 @@
 -   repo: https://github.com/pre-commit/pre-commit-hooks.git
-    sha: ff65d01841ad012d0a9aa1dc451fc4539d8b7baf
+    sha: 414cfa7b2322cf1c46cd33a49e9da833ad785473
     hooks:
     -   id: trailing-whitespace
     -   id: end-of-file-fixer
@@ -13,14 +13,15 @@
     -   id: requirements-txt-fixer
     -   id: flake8
         files: ^git-phab$
-        args: ["--max-complexity=40"]
+        args:
+        - --max-complexity=40
 -   repo: https://github.com/pre-commit/pre-commit.git
-    sha: 495e21b24dfc73624c8c7a16bf974da54e3217e7
+    sha: 6e5ac079273c1499add3a85d9b5394d0f1ef1520
     hooks:
     -   id: validate_config
     -   id: validate_manifest
 -   repo: https://github.com/asottile/reorder_python_imports.git
-    sha: 017e2f64306853ec7f000db52b8280da27eb3b96
+    sha: v0.3.2
     hooks:
     -   id: reorder-python-imports
         language_version: python2.7
diff --git a/README.md b/README.md
index a50b4bc..a0b0898 100644
--- a/README.md
+++ b/README.md
@@ -130,3 +130,13 @@ You can now cleanup your branches:
   Task 'T3436' has been closed, do you want to delete branch 'xclaesse/wip/phab/T3436-fix-a-bug'? [yn] y
     -> Branch xclaesse/wip/phab/T3436-fix-a-bug was deleted
 ```
+
+HOW TO SET UP YOUR PROJECT
+==========================
+
+First of all, you need to add an `.arcconfig` to your project repository. This file
+is the same one as used by [arcanist] and you should follow their '[Configuring a New
+Project]' documentation to set write the configuration file.
+
+  [Configuring a New Project]: https://secure.phabricator.com/book/phabricator/article/arcanist_new_project/
+  [arcanist]: https://secure.phabricator.com/book/phabricator/article/arcanist/
diff --git a/debian/changelog b/debian/changelog
index ca3fa48..bef0df9 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+git-phab (2.1.0-1) experimental; urgency=medium
+
+  * New upstream release.
+  * debian/patches: drop patches merged upstream.
+
+ -- Héctor Orón Martínez <zumbi@debian.org>  Wed, 31 May 2017 12:44:20 +0200
+
 git-phab (2.0.0-2) unstable; urgency=medium
 
   * Apply an upstream patch:
diff --git a/debian/patches/0001-Document-apply-subcommand.patch b/debian/patches/0001-Document-apply-subcommand.patch
deleted file mode 100644
index c0ba83a..0000000
--- a/debian/patches/0001-Document-apply-subcommand.patch
+++ /dev/null
@@ -1,43 +0,0 @@
-From ead72cf2e109535c5eadcae2d1f47adac32a2df1 Mon Sep 17 00:00:00 2001
-From: Andrew Shadura <andrew.shadura@collabora.co.uk>
-Date: Sat, 26 Nov 2016 16:56:56 +0300
-Subject: [PATCH] Document 'apply' subcommand.
-
-Signed-off-by: Andrew Shadura <andrew.shadura@collabora.co.uk>
-
-Differential Revision: https://phabricator.freedesktop.org/D1510
----
- git-phab.txt | 10 ++++++++++
- 1 file changed, 10 insertions(+)
-
-diff --git a/git-phab.txt b/git-phab.txt
-index 0247776..75f4b0a 100644
---- a/git-phab.txt
-+++ b/git-phab.txt
-@@ -15,6 +15,7 @@ SYNOPSIS
-                 ['<revision range>']
- *git phab log* [-h] [<revision range>]
- *git phab fetch* [-h] ['<T123>']
-+*git phab apply* [-h] ['<(T|D)123>'] [-n] [-o '<directory>']
- *git phab checkout* [-h] ['<T123>']
- *git phab browse* [-h] ['objects' ['objects' ...]]
- *git phab clean* [-h]
-@@ -75,6 +76,15 @@ A new branch can then be created using, for example:
- 
- See also *checkout* command.
- 
-+*apply*::
-+
-+Apply a revision and its dependencies.
-++
-+With `--no-dependencies` (or `-n`), revision's dependencies will not be applied.
-++
-+With `--output-directory` (or `-n`), patches aren't applied to the repository,
-+but exported to a directory instead.
-+
- *checkout*::
- 
- Same as *fetch* but also create a new branch and check it out. If an existing
--- 
-2.9.3
-
diff --git a/debian/patches/0002-Explicitly-set-revision-title-when-creating.patch b/debian/patches/0002-Explicitly-set-revision-title-when-creating.patch
deleted file mode 100644
index 727a87c..0000000
--- a/debian/patches/0002-Explicitly-set-revision-title-when-creating.patch
+++ /dev/null
@@ -1,31 +0,0 @@
-From 7575cd81ef1712ae39030282773c7c1facab6282 Mon Sep 17 00:00:00 2001
-From: Daniel Stone <daniels@collabora.com>
-Date: Thu, 16 Mar 2017 12:37:01 +0000
-Subject: [PATCH] Explicitly set revision title when creating
-
-When creating a revision through the Conduit API, newer versions of
-Phabricator require we explicitly set the title in the field, rather
-than pulling it back from the commit message parsing.
-
-Signed-off-by: Daniel Stone <daniels@collabora.com>
-
-Differential Revision: https://phabricator.freedesktop.org/D1694
----
- git-phab | 1 +
- 1 file changed, 1 insertion(+)
-
-diff --git a/git-phab b/git-phab
-index d562066..0e38619 100755
---- a/git-phab
-+++ b/git-phab
-@@ -1105,6 +1105,7 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
-             corpus=arc_message)
- 
-         fields = parsed_message["fields"]
-+        fields["title"] = subject
-         if not revision_id:
-             revision = phab.differential.createrevision(fields=fields,
-                                                         diffid=diff.diffid)
--- 
-2.9.3
-
diff --git a/debian/patches/series b/debian/patches/series
deleted file mode 100644
index 0a29e50..0000000
--- a/debian/patches/series
+++ /dev/null
@@ -1,2 +0,0 @@
-0001-Document-apply-subcommand.patch
-0002-Explicitly-set-revision-title-when-creating.patch
diff --git a/git-phab b/git-phab
index 11054ee..ee9ff67 100755
--- a/git-phab
+++ b/git-phab
@@ -22,6 +22,7 @@
 # http://www.gnu.org/licenses/.
 
 import base64
+import configparser
 import logging
 import socket
 import tempfile
@@ -78,6 +79,39 @@ class Colors:
         cls.ENDC = '\033[0m'
 
 
+def stash(func):
+    def wrapper(self, *args):
+        needs_stash = self.repo.is_dirty()
+        if needs_stash:
+            if not self.autostash:
+                self.die(
+                    "Repository is dirty. Aborting.\n"
+                    "You can use `--autostash` to automatically"
+                    " stash uncommitted changes\n"
+                    "You can also `git config [--global] phab.autostash true`"
+                    " to make it permanent")
+            print("Stashing current changes before attaching patches")
+            self.repo.git.stash()
+        try:
+            func(self, *args)
+        finally:
+            if needs_stash:
+                print("Restoring stashed changes")
+                stash_name = "stash@{0}"
+                if self.repo.is_dirty():
+                    # This might happen if some linting tool starts
+                    # changing the code.
+                    stash_name = "stash@{1}"
+                    print("Some more changes have been done"
+                          " during the process, stashing them"
+                          " and going back to the state before attaching.\n"
+                          " You can see those with `git stash show stash@{0}`")
+                    self.repo.git.stash()
+                self.repo.git.stash('pop', stash_name)
+
+    return wrapper
+
+
 class GitPhab:
 
     def __init__(self):
@@ -92,6 +126,7 @@ class GitPhab:
         self.output_directory = None
         self.phab_repo = None
         self.staging_url = None
+        self.autostash = False
 
         self.repo = git.Repo(os.getcwd(), search_parent_directories=True)
         self.read_arcconfig()
@@ -104,6 +139,13 @@ class GitPhab:
         if self._phabricator:
             return self._phabricator
 
+        if self.arcrc:
+            try:
+                with open(self.arcrc) as f:
+                    phabricator.ARCRC.update(json.load(f))
+            except FileNotFoundError:
+                self.die("Failed to load a given arcrc file, %s" % self.arcrc)
+
         needs_credential = False
         try:
             host = self.phabricator_uri + "/api/"
@@ -531,6 +573,13 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
 
         if self.remote:
             self.validate_remote()
+
+        try:
+            self.autostash |= self.repo.config_reader().get_value(
+                'phab', 'autostash')
+        except configparser.NoOptionError:
+            pass
+
         # Try to guess the task from branch name
         if self.repo.head.is_detached:
             self.die("HEAD is currently detached. Aborting.")
@@ -600,9 +649,17 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
                 remoteURIs=uris)
 
             if len(reply) > 1:
-                self.die("Multiple repositories returned for remote URIs "
-                         "({}).\nYou should check your Phabricator "
-                         "configuration.".format(', '.join(uris)))
+                tracking = self.repo.head.reference.tracking_branch()
+                # Use the remote that this branch is tracking.
+                uris = [remote.url for remote in self.repo.remotes
+                        if remote.name == tracking.remote_name]
+                reply = self.phabricator.repository.query(
+                    remoteURIs=uris)
+
+                if len(reply) > 1:
+                    self.die("Multiple repositories returned for remote URIs "
+                             "({}).\nYou should check your Phabricator "
+                             "configuration.".format(', '.join(uris)))
 
         try:
             self.phab_repo = reply[0]
@@ -900,10 +957,12 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
             filetype = "3"
             metadata = {
                 "old:file:size": diff.a_blob.size if diff.a_blob else 0,
-                "old:file:mime-type": diff.a_blob.mime_type if diff.a_blob else '',
+                "old:file:mime-type": diff.a_blob.mime_type if diff.a_blob else
+                '',
                 "old:binary-phid": a_phab_file.response if a_phab_file else '',
                 "new:file:size": diff.b_blob.size if diff.b_blob else 0,
-                "new:file:mime-type": diff.b_blob.mime_type if diff.b_blob else '',
+                "new:file:mime-type": diff.b_blob.mime_type if diff.b_blob else
+                '',
                 "new:binary-phid": b_phab_file.response if b_phab_file else '',
             }
 
@@ -959,23 +1018,29 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
 
         return diff
 
+    def get_diff_staging_ref(self, diffid):
+        return "refs/tags/phabricator/diff/%s" % diffid
+
     def push_diff_to_staging(self, diff, commit):
         if not self.staging_url:
             print("    * %sNo staging repo set, not pushing diff %s%s" % (
                 Colors.FAIL, diff.diffid, Colors.ENDC))
-            return
+            return None
 
         print("    * Pushing diff %d on the staging repo... " %
               diff.diffid, end='')
         try:
-            self.repo.git.push(
-                self.staging_url, "%s:refs/tags/phabricator/diff/%s" % (
-                    commit.hexsha, diff.diffid))
+            remote_ref = self.get_diff_staging_ref(diff.diffid)
+            self.repo.git.push(self.staging_url, "%s:%s" % (commit.hexsha,
+                                                            remote_ref))
             print("%sOK%s" % (Colors.OKGREEN, Colors.ENDC))
+
+            return remote_ref
         except git.exc.GitCommandError as e:
             print("%sERROR %s(%s)" % (Colors.FAIL,
                                       Colors.ENDC,
-                                      e.stderr.decode("utf-8")))
+                                      e.stderr.strip("\n")))
+        return None
 
     def update_local_commit_info(self, diff, commit):
         commit_infos = {
@@ -1045,8 +1110,8 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
         # (avoiding making query on the server when not needed)
         if last_revision_id and \
                 self.repo.head.commit.parents[0] not in proposed_commits and \
-                not self.phabricator.differential.query(ids=[last_revision_id],
-                                                        status="status-closed"):
+                not self.phabricator.differential.query(
+                    ids=[last_revision_id], status="status-closed"):
             body.append("Depends on D%s" % last_revision_id)
         phab_fields.append("Projects: %s" % ','.join(self.project_phids))
 
@@ -1062,9 +1127,17 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
             arc_message = phab.differential.getcommitmessage(
                 edit="create", fields=phab_fields).response
 
-            arc_message = arc_message.replace(
-                "<<Replace this line with your Revision Title>>",
-                self.format_field(subject, True))
+            subject_formatted = self.format_field(subject, True)
+            # The substitution below should cover:
+            # "<<Replace this line with your Revision Title>>"
+            # "<<Replace this line with your revision title>"
+            arc_message = re.sub(
+                "<<Replace this line with your Revision Title>>?",
+                subject_formatted,
+                arc_message,
+                flags=re.I)
+            assert subject_formatted in arc_message
+
             if summary != '':
                 arc_message = arc_message.replace(
                     "Summary: ",
@@ -1090,6 +1163,7 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
             corpus=arc_message)
 
         fields = parsed_message["fields"]
+        fields["title"] = subject
         if not revision_id:
             revision = phab.differential.createrevision(fields=fields,
                                                         diffid=diff.diffid)
@@ -1114,7 +1188,6 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
                 message = "\n".join(message)
 
             fields["summary"] = summary
-            fields["title"] = subject
             if linter_message:
                 message += "\n\n%s" % linter_message
 
@@ -1123,10 +1196,42 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
                 diffid=diff.diffid,
                 message=message), diff
 
-    def do_attach(self):
-        if self.repo.is_dirty():
-            self.die("Repository is dirty. Aborting.")
+    def update_task_branch_uri(self, staging_remote_refname):
+        summary = ""
+        remote_uri = None
+        if staging_remote_refname and self.task:
+            remote_uri = "%s#%s" % (self.staging_url, staging_remote_refname)
+        elif self.remote and self.task:
+            try:
+                branch = self.get_wip_branch()
+                remote = self.repo.remote(self.remote)
+                if self.prompt('Push HEAD to %s/%s?' % (remote, branch)):
+                    info = remote.push('HEAD:refs/heads/' + branch,
+                                       force=True)[0]
+                    if not info.flags & info.ERROR:
+                        summary += "  * Branch pushed to %s/%s\n" % (remote,
+                                                                     branch)
+                    else:
+                        print("-> Could not push branch %s/%s: %s" % (
+                            remote, branch, info.summary))
+
+                remote_uri = "%s#%s" % (self.remote_url, branch)
+            except Exception as e:
+                summary += "  * Failed: push wip branch: %s\n" % e
+
+        if remote_uri:
+            try:
+                self.phabricator.maniphest.update(
+                    id=int(self.task[1:]),
+                    auxiliary={"std:maniphest:git:uri-branch": remote_uri})
+            except:
+                print("-> Failed to set std:maniphest:git:uri-branch to %s"
+                      % remote_uri)
 
+        return summary
+
+    @stash
+    def do_attach(self):
         # If we are in branch "T123" and user does "git phab attach -t T456",
         # that's suspicious. Better stop before doing a mistake.
         if self.branch_task and self.branch_task != self.task:
@@ -1193,6 +1298,7 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
         orig_branch = self.repo.head.reference
 
         patch_attachement_failure = False
+        staging_remote_refname = None
         try:
             # Detach HEAD from the branch; this gives a cleaner reflog for the
             # branch
@@ -1233,7 +1339,8 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
 
                     self.repo.git.commit("-n", amend=True, message=msg)
                     self.update_local_commit_info(diff, self.repo.head.object)
-                    self.push_diff_to_staging(diff, self.repo.head.object)
+                    staging_remote_refname = self.push_diff_to_staging(
+                        diff, self.repo.head.object)
                     print("%s-> OK%s" % (Colors.OKGREEN, Colors.ENDC))
 
                     summary += self.format_commit(self.repo.head.commit) + "\n"
@@ -1251,31 +1358,8 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
             self.repo.head.reset(index=True, working_tree=True)
             raise
 
-        if self.remote and self.task and not patch_attachement_failure:
-            try:
-                branch = self.get_wip_branch()
-                remote = self.repo.remote(self.remote)
-                if self.prompt('Push HEAD to %s/%s?' % (remote, branch)):
-                    info = remote.push('HEAD:refs/heads/' + branch,
-                                       force=True)[0]
-                    if not info.flags & info.ERROR:
-                        summary += "  * Branch pushed to %s/%s\n" % (remote,
-                                                                     branch)
-                    else:
-                        print("-> Could not push branch %s/%s: %s" % (
-                            remote, branch, info.summary))
-
-                uri = "%s#%s" % (self.remote_url, branch)
-                try:
-                    self.phabricator.maniphest.update(
-                        id=int(self.task[1:]),
-                        auxiliary={"std:maniphest:git:uri-branch": uri})
-                except:
-                    print("-> Failed to set std:maniphest:git:uri-branch to %s"
-                          % uri)
-
-            except Exception as e:
-                summary += "  * Failed: push wip branch: %s\n" % e
+        if not patch_attachement_failure:
+            summary += self.update_task_branch_uri(staging_remote_refname)
 
         if self.task and not self.branch_task:
             # Check if we already have a branch for this task
@@ -1481,9 +1565,8 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
             return False
 
         try:
-            self.repo.git.fetch(
-                self.staging_url, "refs/tags/phabricator/diff/%s" %
-                diff["id"])
+            self.repo.git.fetch(self.staging_url,
+                                self.get_diff_staging_ref(diff["id"]))
         except git.exc.GitCommandError as e:
             print(e)
             return False
@@ -1555,10 +1638,9 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
             os.unlink(filename)
             n += 1
 
+    @stash
     def do_apply(self):
-        if self.repo.is_dirty():
-            self.die("Repository is dirty. Aborting.")
-        elif not self.differential and not self.task:
+        if not self.differential and not self.task:
             self.die("No task or revision provided. Aborting.")
 
         if self.differential:
@@ -1632,13 +1714,13 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
 
         return (commit, remote, branch)
 
-    def create_fake_fetch(self, revision, diff):
-        current_branch = self.repo.active_branch
+    def checkout_base_revision(self, diff):
         base_commit = diff.get("sourceControlBaseRevision")
         if base_commit:
             try:
                 self.repo.git.checkout(base_commit)
             except git.exc.GitCommandError:
+                print("Could not get base commit %s" % base_commit)
                 base_commit = None
 
         if not base_commit:
@@ -1650,11 +1732,18 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
                       Colors.ENDC))
             self.repo.git.checkout(self.repo.head.commit.hexsha)
 
+    def create_fake_fetch(self, revision, diff):
+        current_branch = self.repo.active_branch
         pq = self.get_differentials_to_apply_for_revision()
+
+        checkout_base_revision = True
         if pq:
             n = 0
             while pq != []:
                 (r, d) = pq.pop()
+                if checkout_base_revision:
+                    self.checkout_base_revision(d)
+                    checkout_base_revision = False
 
                 filename = self.write_patch_file(r, d)
                 print("Applying D{}".format(r['id']))
@@ -1813,10 +1902,8 @@ Paste API Token from that page and press <enter>: """ % self.phabricator_uri)
 
                     print("  -> Branch %s was deleted" % branch.name)
 
+    @stash
     def do_land(self):
-        if self.repo.is_dirty():
-            self.die("Repository is dirty. Aborting.")
-
         if self.task:
             commit, remote, remote_branch_name = self.fetch_from_task()
             branch = self.repo.active_branch
@@ -1917,6 +2004,7 @@ def check_dependencies_versions():
                  git.__version__, Colors.ENDC))
         exit(1)
 
+
 if __name__ == '__main__':
     check_dependencies_versions()
     parser = argparse.ArgumentParser(description='Phabricator integration.')
@@ -1966,6 +2054,12 @@ if __name__ == '__main__':
         help="commit or revision range to attach. When not specified, "
              "the tracking branch is used") \
         .completer = DisabledCompleter
+    attach_parser.add_argument(
+        '--autostash', action="store_true",
+        help="Automatically stash not committed changes."
+        " You can also `git config [--global] phab.autostash true` "
+        "to make it permanent") \
+        .completer = DisabledCompleter
 
     apply_parser = subparsers.add_parser(
         'apply', help="Apply a revision and its dependencies"
@@ -1982,6 +2076,12 @@ if __name__ == '__main__':
         '--no-dependencies', "-n", action="store_true",
         help="Do not apply dependencies of a revision.") \
         .completer = DisabledCompleter
+    apply_parser.add_argument(
+        '--autostash', action="store_true",
+        help="Automatically stash not committed changes."
+        " You can also `git config [--global] phab.autostash true` "
+        "to make it always happen") \
+        .completer = DisabledCompleter
 
     log_parser = subparsers.add_parser(
         'log', help="Show commit logs with their differential ID")
@@ -2027,6 +2127,12 @@ if __name__ == '__main__':
         'task', metavar='<T123>', nargs='?',
         help="The task to land") \
         .completer = DisabledCompleter
+    land_parser.add_argument(
+        '--autostash', action="store_true",
+        help="Automatically stash not committed changes."
+        " You can also `git config [--global] phab.autostash true` "
+        "to make it always happen") \
+        .completer = DisabledCompleter
 
     argcomplete.autocomplete(parser)
 
diff --git a/git-phab.txt b/git-phab.txt
index 0247776..92e0bda 100644
--- a/git-phab.txt
+++ b/git-phab.txt
@@ -15,7 +15,7 @@ SYNOPSIS
                 ['<revision range>']
 *git phab log* [-h] [<revision range>]
 *git phab fetch* [-h] ['<T123>']
-*git phab checkout* [-h] ['<T123>']
+*git phab apply* [-h] ['<(T|D)123>'] [-n] [-o '<directory>']
 *git phab browse* [-h] ['objects' ['objects' ...]]
 *git phab clean* [-h]
 *git phab land* [-h] [--no-push]
@@ -72,14 +72,17 @@ This only fetch and print the commit id, it won't create or checkout a branch.
 A new branch can then be created using, for example:
 
   git checkout -b my-branch FETCH_HEAD
++
+With `--checkout` (or `-c`), fetch and checkout in a branch.
 
-See also *checkout* command.
-
-*checkout*::
+*apply*::
 
-Same as *fetch* but also create a new branch and check it out. If an existing
-branch is found for the same task it will prompt to reset that branch to the
-newly fetched commit, then checkout is as well.
+Apply a revision and its dependencies.
++
+With `--no-dependencies` (or `-n`), revision's dependencies will not be applied.
++
+With `--output-directory` (or `-o`), patches aren't applied to the repository,
+but exported to a directory instead.
 
 *browse*::
 
diff --git a/setup.py b/setup.py
index a8cb697..95ef1b1 100644
--- a/setup.py
+++ b/setup.py
@@ -11,7 +11,7 @@ else:
 
 setup(
     name="git-phab",
-    version="2.0.0",
+    version="2.1.0",
     author="Xavier Claessens",
     author_email="xavier.claessens@collabora.com",
     description=("Git subcommand to integrate with phabricator"),