[PATCH] D73075: [utils] Add initial llvm-patch helper to manage patches.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/utils/llvm-patch:41
+
+    with open(os.path.expanduser(os.path.join('~', '.arcrc')), 'rt') as f:
+        d = json.loads(f.read())
----------------
Can we check if `.arcrc` exists before opening it?


================
Comment at: llvm/utils/llvm-patch:62-63
+        data = json.loads(response.read())
+        if not data['result']:
+            print('ERROR: {}'.format(data['error_info']))
+        return data['result']
----------------
paquette wrote:
> If `data['result']` is an empty string, this will still return something. Should there also be a check for if `data['result']` is empty?
- If `data['result']` is an empty string, this will still return something. Should there also be a check for if `data['result'] = ''`?
- Is there only data in `'error_info'` when `data['result']` is empty? Would it make more sense to just check if `data['error_info']` has something in it?
- `sys.exit(1)` on error?


================
Comment at: llvm/utils/llvm-patch:62-64
+        if not data['result']:
+            print('ERROR: {}'.format(data['error_info']))
+        return data['result']
----------------
If `data['result']` is an empty string, this will still return something. Should there also be a check for if `data['result']` is empty?


================
Comment at: llvm/utils/llvm-patch:77
+    """
+    revision_id = int(args.ID.replace('D', ''))
+
----------------
This will crash if `args.ID` is not an integer.


================
Comment at: llvm/utils/llvm-patch:82-84
+        print("ERROR: There are uncommitted changes in the repository!\n"
+              "       Can only apply a patch to a clean working directory.")
+        sys.exit(1)
----------------
Can we use `logging` for this?

E.g. https://docs.python.org/3.8/library/logging.html

Might make sense to add a function like `log_error_and_exit`?


================
Comment at: llvm/utils/llvm-patch:133
+             'reviews.llvm.org and apply it to the working directory.')
+    apply_parser.add_argument('ID', help='Differential ID, e.g. D5678')
+    apply_parser.set_defaults(func=apply_patch)
----------------
Can we verify the format of the ID in the parser itself using an argparse custom type? Then if someone types in some garbage, you can just die + produce a useful message in `parse_args()`.


================
Comment at: llvm/utils/llvm-patch:137-140
+    if not hasattr(args, 'func'):
+        parser.print_help()
+        print('llvm-patch: A subcommand is required')
+        sys.exit(1)
----------------
Can we use `argparse.ArgumentError` for this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73075/new/

https://reviews.llvm.org/D73075