Page MenuHomePhabricator

Ask confirmation when `git llvm push` will push multiple commits
ClosedPublic

Authored by mehdi_amini on Jul 17 2019, 3:38 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini created this revision.Jul 17 2019, 3:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 3:38 PM
mehdi_amini marked an inline comment as done.Jul 17 2019, 3:42 PM
mehdi_amini added inline comments.
llvm/utils/git-svn/git-llvm
107 ↗(On Diff #210443)

This does not work for me, if anyone can help?

Fix python 2/3 compatibility

abrachet added inline comments.
llvm/utils/git-svn/git-llvm
453 ↗(On Diff #210446)

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.

454 ↗(On Diff #210446)

Maybe "Did not proceed with pushing multiple commits" or at least "Aborting". Although I'm not sure any message is needed, probably just exiting is ok too.

abrachet added inline comments.Jul 17 2019, 9:17 PM
llvm/utils/git-svn/git-llvm
107 ↗(On Diff #210443)

Are you also getting a NameError? I'm not sure why this is (I don't know much about Python), but I can say that using raw_input works as expected.

mehdi_amini marked 3 inline comments as done.

Change abort message to "Aborting"

llvm/utils/git-svn/git-llvm
107 ↗(On Diff #210443)

It works for me in the current version with python 2.7 and 3.7 on MacOS

453 ↗(On Diff #210446)

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):
ymandel added inline comments.
llvm/utils/git-svn/git-llvm
116 ↗(On Diff #210481)

Maybe default to "no" (and indicate that with "(y/N)" in the query) when the user doesn't enter either one?

+1 for defaulting to No.
As a follow-up patch, maybe the script could offer to squash the commits for you.

default to No

jyknight added inline comments.Mon, Jul 29, 1:40 PM
llvm/utils/git-svn/git-llvm
453 ↗(On Diff #210446)

I agree, it'd be better to say:
"Are you sure you want to create %d commits?"

30 ↗(On Diff #210646)

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().

mehdi_amini marked 3 inline comments as done.
  • Improve output when asking the user a question
  • simplify python 2/3 compatibility
mehdi_amini marked 2 inline comments as done.Mon, Jul 29, 9:27 PM
mehdi_amini added inline comments.
llvm/utils/git-svn/git-llvm
453 ↗(On Diff #210446)

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):
30 ↗(On Diff #210646)

I used read_input instead of input for the local variable name otherwise Python complains about using a variable before defining it, PTAL.

jyknight accepted this revision.Tue, Jul 30, 8:12 AM
jyknight marked an inline comment as done.
jyknight added inline comments.
llvm/utils/git-svn/git-llvm
30 ↗(On Diff #210646)

Yeah -- what I said would only work at the module level, because python variable binding semantics are different inside functions than outside.

This revision is now accepted and ready to land.Tue, Jul 30, 8:12 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini marked an inline comment as done.