[PATCH] D64893: Ask confirmation when `git llvm push` will push multiple commits

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini marked 2 inline comments as done.
mehdi_amini added inline comments.


================
Comment at: llvm/utils/git-svn/git-llvm:453
+    if len(revs) != 1:
+        if not ask_confirm("Are you sure?"):
+            die("abort")
----------------
jyknight wrote:
> mehdi_amini wrote:
> > abrachet wrote:
> > > Shouldn't the message be more descriptive? Maybe it should be "Multiple commits are about to be pushed. Are you sure?" The mailing list thread was specifically for newer contributors, right? I could imagine someone using `git llvm push` for the first time would imagine that was a normal message that is always given.
> > The output would be:
> > 
> > ```
> > Pushing 2 monorepo commits:
> >   31a4e7393ea7 Ask confirmation when `git llvm push` will push multiple commits
> >   c5eb9c5e2018 dummy
> > Are you sure? (y/n): 
> > ```
> I agree, it'd be better to say:
> "Are you sure you want to create %d commits?"
Output is now:

```
Pushing 2 monorepo commits:
  b28f3db64111 Fix `git llvm` script when no arguments are supplied on Python 3
  aeef6550de82 Ask confirmation when `git llvm push` will push multiple commits
Are you sure you want to create 2 commits? (y/N): 
```


================
Comment at: llvm/utils/git-svn/git-llvm:30
+try:
+    exec("import __builtin__")  # To avoid IDE's grammar check
+except ImportError:
----------------
jyknight wrote:
> Instead of all the stuff with builtins, you can just do this:
> 
> ```
> try:
>     # Python 2
>     input = raw_input
> except NameError:
>     input = input
> ```
> 
> and then below just call input().
I used `read_input` instead of `input` for the local variable name otherwise Python complains about using a variable before defining it, PTAL.


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

https://reviews.llvm.org/D64893