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

[Mehdi AMINI via llvm-commits]

mehdi_amini marked 22 inline comments as done.
mehdi_amini added a comment.

Thanks Justin!



================
Comment at: llvm/utils/git-svn/git-llvm:1
+#!/usr/bin/env python
+#
----------------
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"?


================
Comment at: llvm/utils/git-svn/git-llvm:76
+    dash_dash = argv[idx:]
+    argv = argv[:idx]
+
----------------
jlebar wrote:
> Do we need this at all?  None of the commands at the moment take positional arguments.
I can kill this for now. TBH I just started from `git-clang-format`.


================
Comment at: llvm/utils/git-svn/git-llvm:133
+    svn_dirs = list(GIT_TO_SVN_DIR.values())
+    run('svn', 'update', *svn_dirs)
+    eprint("svn staging area ready in '%s'" % (svn_root))
----------------
jlebar wrote:
> We may want to think about whether and how we should change this so that it will work if we later add a directory to GIT_TO_SVN_DIR.  I think it may just be a matter of changing the other `svn update` command, below.  Maybe at that point it should be factored out into a separate function.
I didn't get what you mean here?


================
Comment at: llvm/utils/git-svn/git-llvm:154
+
+VERBOSE = False
+
----------------
jlebar wrote:
> Please rearrange this file applying some sort of logical ordering.
> 
> Usually Python programs do a rough topological sort, with `main` at the bottom.
Done.
(I took this from `git-clang-format`. Is there some sort of official convention (like pep8) that would mention something?)


================
Comment at: llvm/utils/git-svn/git-llvm:285
+
+def run(*args, **kwargs):
+  stdin = kwargs.pop('stdin', '')
----------------
jlebar wrote:
> run() and shell() seem to do basically the same thing?
Merged!


================
Comment at: llvm/utils/git-svn/git-llvm:305
+  if stderr:
+    print >>sys.stderr, stderr.rstrip()
+  sys.exit(2)
----------------
jlebar wrote:
> Can we import print_fn from __future__ instead of using the old-style print syntax?  The new thing is IMO easier to understand, and it also eases the py3 conversion.
It was included: this patch would have thrown at runtime. Fixed.


https://reviews.llvm.org/D26334