[PATCH] D26334: Add some facilities to work with a git monorepo (experimental setup)

[Justin Lebar via llvm-commits]

jlebar added a comment.

This looks good to me!  My only remaining nontrivial concern is about the error handling in the parser.

Our version of phab has a bug where it sometimes won't delete comments.  Sometimes those comments do show up when I hit "submit", and sometimes they don't.  I have a comment that I've tried to delete that starts with:

> I am afraid that if the only notice we have about this limitation is in the HTML documentation,

If this shows up when I hit "submit", you can ignore it, I decided it's not a big deal and mean to delete it.



================
Comment at: llvm/utils/git-svn/git-llvm:1
+#!/usr/bin/env python
+#
----------------
mehdi_amini wrote:
> jlebar wrote:
> > mehdi_amini wrote:
> > > jlebar wrote:
> > > > jlebar wrote:
> > > > > Can we mention that this is derived from my work and link to the original repository in the commit message?  I would appreciate that, and it may also reduce confusion among users.
> > > > It is important that we retain the warning (somewhere) that this tool does not handle renames properly.
> > > > Can we mention that this is derived from my work and link to the original repository in the commit message? I would appreciate that, and it may also reduce confusion among users.
> > > 
> > > Of course, I wanted to add this in the commit message but forgot in the end.
> > > Note that my idea is that the script should evolve with other subcommand than `push`, and persist even after the move to github (for example I mentioned `llvm show r12345` to continue interacting with SVN rev).
> > > 
> > > > It is important that we retain the warning (somewhere) that this tool does not handle renames properly.
> > > 
> > > Sure.
> > > 
> > > Git does not record rename in the first place anyway, it there more to this than "rename are not shown as rename in SVN"?
> > > Git does not record rename in the first place anyway, it there more to this than "rename are not shown as rename in SVN"?
> > 
> > Right, but AIUI git-svn will notice renames using whatever your current git settings are and commits them as renames in SVN.
> > 
> > I think we're on the same page about this?  I agree all that's necessary is to warn somewhere that "rename**s** are not shown as rename**s** in SVN."
> > 
> > Since it should be possible to mirror git-svn's behavior, perhaps it should be a TODO?
> I added it to the doc.
I am afraid that if the only notice we have about this limitation is in the HTML documentation,

(a) many people won't see it (because they're just going to run `git llvm -h`,
(b) maintainers of this code will not be aware of the limitation, and
(c) if we ever fix this problem, we will not necessarily remember to update the HTML documentation.

That's why I was hoping we could put something inside of this file, like a TODO, or a note in the `--help` / docstring.

I know this is a fine point, and I'm sorry to belabor it, but I have found that the documentation that is most likely to be up to date is the documentation that lives closest to the code.  I am concerned about putting important notes in a separate file for the same reasons I would be concerned about duplicating code into two files.


================
Comment at: llvm/utils/git-svn/git-llvm:96
+    if elapsed >= .1:
+        log_verbose('Warning: command took %0.1fs' % elapsed)
+
----------------
mehdi_amini wrote:
> jlebar wrote:
> > Nit, some commands (like svn update) quite reasonably take more than .1s.  So I don't think we should beg the question [1] and say "warning".
> > 
> > [1] https://en.wikipedia.org/wiki/Begging_the_question
> The `.1` seems arbitrary, since this is behind the `-v` flag, what about always printing it?
Always printing the time elapsed with -v sounds fine to me.


================
Comment at: llvm/utils/git-svn/git-llvm:22
+
+Requires Python 2.7
+"""
----------------
Maybe we can check the Python version in addition to or instead of this comment (`sys.version >= (2,7)`)?


================
Comment at: llvm/utils/git-svn/git-llvm:238
+            return tail
+        d = head
+
----------------
Can we move this helper up?  Kind of weird that it's here.


================
Comment at: llvm/utils/git-svn/git-llvm:245
+        prog='git llvm', formatter_class=argparse.RawDescriptionHelpFormatter,
+        description=__doc__)
+    subcommands = p.add_subparsers(title='subcommands',
----------------
Ok, the `__doc__` will now be printed out when you run `git llvm -h`, so maybe it shouldn't redirect people to looking at the output of `-h`.  :)


================
Comment at: llvm/utils/git-svn/git-llvm:253
+    group.add_argument('-v', '--verbose', action='store_true',
+                       help='print more information')
+
----------------
Maybe a better name than `group`?  `verbosity_group`?


================
Comment at: llvm/utils/git-svn/git-llvm:279
+    if QUIET and VERBOSE:
+        die('Error -v and -q cannot be used together: make your mind!')
+
----------------
Because you're using a mutually_exclusive_group, you don't need this anymore, right?


================
Comment at: llvm/utils/git-svn/git-llvm:282
+    # Dispatch to the right subcommand
+    args.func(args)
----------------
What happens if you just run `git llvm`?  It seems like args.func would be None?  Maybe we should print a better error message (via `parser.error`)?


https://reviews.llvm.org/D26334