[PATCH] D26334: Add some facilities to work with a git monorepo (experimental setup)
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Sun Nov 6 12:05:43 PST 2016
jlebar added inline comments.
================
Comment at: llvm/utils/git-svn/git-llvm:1
+#!/usr/bin/env python
+#
----------------
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.
================
Comment at: llvm/utils/git-svn/git-llvm:1
+#!/usr/bin/env python
+#
----------------
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.
================
Comment at: llvm/utils/git-svn/git-llvm:76
+ dash_dash = argv[idx:]
+ argv = argv[:idx]
+
----------------
Do we need this at all? None of the commands at the moment take positional arguments.
================
Comment at: llvm/utils/git-svn/git-llvm:87
+
+ parser_push = subparsers.add_parser('push', help='push back changes to the LLVM SVN repository')
+ parser_push.add_argument(
----------------
push changes back to the LLVM SVN repository
================
Comment at: llvm/utils/git-svn/git-llvm:112
+ args = p.parse_args(argv)
+ VERBOSE = not args.quiet
+ args.func(args)
----------------
We have a global `-q` and a per-command `--verbose`? That does not seem right.
================
Comment at: llvm/utils/git-svn/git-llvm:116
+def svn_push(args):
+
+ # Get the git root
----------------
Remove blank line (or if you want it, please be consistent everywhere).
================
Comment at: llvm/utils/git-svn/git-llvm:118
+ # Get the git root
+ git_root = run('git','rev-parse','--show-toplevel')
+ if not os.path.isdir(git_root):
----------------
Spaces after commas
================
Comment at: llvm/utils/git-svn/git-llvm:118
+ # Get the git root
+ git_root = run('git','rev-parse','--show-toplevel')
+ if not os.path.isdir(git_root):
----------------
jlebar wrote:
> Spaces after commas
Please use the `git` helper consistently (or get rid of it).
================
Comment at: llvm/utils/git-svn/git-llvm:120
+ if not os.path.isdir(git_root):
+ eprint("Can't get git root dir")
+ sys.exit(1)
----------------
s/get/find/
================
Comment at: llvm/utils/git-svn/git-llvm:124
+ # We need a staging area for SVN, let's hide it in the .git directory.
+ svn_root = os.path.join(git_root, ".git", "upstream-svn")
+
----------------
Nit, I think we should namespace the directory we create (i.e. "llvm-upstream-svn" or something).
================
Comment at: llvm/utils/git-svn/git-llvm:124
+ # We need a staging area for SVN, let's hide it in the .git directory.
+ svn_root = os.path.join(git_root, ".git", "upstream-svn")
+
----------------
jlebar wrote:
> Nit, I think we should namespace the directory we create (i.e. "llvm-upstream-svn" or something).
Please be consistent with respect to `'` or `"`. Personally, I prefer single quotes because it's less visually dense.
================
Comment at: llvm/utils/git-svn/git-llvm:129
+ os.makedirs(svn_root)
+ eprint("This is a one-time initialization, please be patient for a few minutes...")
+ os.chdir(svn_root)
----------------
Please be consistent with whether we write statuses to stderr or stdout.
================
Comment at: llvm/utils/git-svn/git-llvm:129
+ os.makedirs(svn_root)
+ eprint("This is a one-time initialization, please be patient for a few minutes...")
+ os.chdir(svn_root)
----------------
jlebar wrote:
> Please be consistent with whether we write statuses to stderr or stdout.
Please wrap lines to 80 characters.
================
Comment at: llvm/utils/git-svn/git-llvm:130
+ eprint("This is a one-time initialization, please be patient for a few minutes...")
+ os.chdir(svn_root)
+ run('svn', 'checkout', '--depth=immediates', 'https://llvm.org/svn/llvm-project/', '.')
----------------
I'm kind of uncomfortable with all the chdir'ing we're doing. (It was tenuous before, as well, but now we're doing even more.) I think it would be clearer if we did a single chdir (maybe to the git root) and then specified an explicit cwd for all other commands.
================
Comment at: llvm/utils/git-svn/git-llvm:131
+ os.chdir(svn_root)
+ run('svn', 'checkout', '--depth=immediates', 'https://llvm.org/svn/llvm-project/', '.')
+ svn_dirs = list(GIT_TO_SVN_DIR.values())
----------------
Please use the `svn` helper consistently, or get rid of it.
================
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))
----------------
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.
================
Comment at: llvm/utils/git-svn/git-llvm:137
+ eprint("Can't initialize svn staging dir (%s)" % (svn_root))
+ sys.exit(1)
+
----------------
Please pull this out into a helper function.
================
Comment at: llvm/utils/git-svn/git-llvm:143
+ rev_range=args.rev_range
+ dry_run=args.dry_run
+ revs = get_revs_to_push(rev_range)
----------------
Spaces around `=`.
================
Comment at: llvm/utils/git-svn/git-llvm:154
+
+VERBOSE = False
+
----------------
Please rearrange this file applying some sort of logical ordering.
Usually Python programs do a rough topological sort, with `main` at the bottom.
================
Comment at: llvm/utils/git-svn/git-llvm:220
+ # Unfortunately it appears there's no svn equivalent for git clean, so we have
+ # to do it ourselves. Because I'm paranoid, prompt before deleting anything.
+ to_delete = []
----------------
This prompt may not be necessary now that you're putting the svn checkout inside the .git dir.
================
Comment at: llvm/utils/git-svn/git-llvm:285
+
+def run(*args, **kwargs):
+ stdin = kwargs.pop('stdin', '')
----------------
run() and shell() seem to do basically the same thing?
================
Comment at: llvm/utils/git-svn/git-llvm:288
+ verbose = kwargs.pop('verbose', True)
+ strip = kwargs.pop('strip', True)
+ for name in kwargs:
----------------
Can we use .get instead?
================
Comment at: llvm/utils/git-svn/git-llvm:305
+ if stderr:
+ print >>sys.stderr, stderr.rstrip()
+ sys.exit(2)
----------------
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.
https://reviews.llvm.org/D26334
More information about the llvm-commits
mailing list