[PATCH] D26334: Add some facilities to work with a git monorepo (experimental setup)
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 7 09:35:11 PST 2016
jlebar added a comment.
Sorry for all the nits; I'm sort of picky about Python. :)
================
Comment at: llvm/utils/git-svn/git-llvm:1
+#!/usr/bin/env python
+#
----------------
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?
================
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))
----------------
mehdi_amini wrote:
> 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?
I mean: Will this script continue to work if we add a new directory to GIT_TO_SVN_DIR?
I think it will not because after doing so we would have to run `svn update parallel-libs/trunk`, but we only do that when creating the svn directory for the first time.
So I was wondering if the *other* `svn update` command, which we run every time, should do `svn update cfe/trunk llvm/trunk blah/trunk` just like this first one does, which (I think?) would let us pick up new GIT_TO_SVN_DIRs.
Then I was wondering, if so, if we should pull out that `svn update` command into a helper function, because code duplication.
================
Comment at: llvm/utils/git-svn/git-llvm:154
+
+VERBOSE = False
+
----------------
mehdi_amini wrote:
> 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?)
> Is there some sort of official convention (like pep8) that would mention something?
Not to my knowledge. But it sort of follows from the fact that
```
if __name__ == '__main__':
main()
```
has to appear at the very bottom of the file (because only at that point have all of our functions been declared). Because the call to main() appears at the bottom, it sort of makes sense for `main` itself to appear nearby. Then everything else follows from there.
(It also sort of makes sense for constants like VERBOSE to appear at the top of the file, out of parallelism with other languages if nothing else.)
================
Comment at: llvm/utils/git-svn/git-llvm:12
+
+r"""
+git-llvm integration
----------------
'r' isn't necessary here (and docstrings canonically don't have them unless it's necessary).
================
Comment at: llvm/utils/git-svn/git-llvm:40
+TODO
+'''
+
----------------
I'm not sure we need this? The docstring at the top of the file should be sufficient -- you can access it as `__doc__` and so on.
================
Comment at: llvm/utils/git-svn/git-llvm:96
+ if elapsed >= .1:
+ log_verbose('Warning: command took %0.1fs' % elapsed)
+
----------------
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
================
Comment at: llvm/utils/git-svn/git-llvm:143
+ # filter empty entries
+ revs = [r for r in revs if r != '']
+ if not revs:
----------------
Don't you just want to strip the output of `git` instead?
================
Comment at: llvm/utils/git-svn/git-llvm:159
+ if len(sp) != 2:
+ raise RuntimeError('Unexpected svn status line: %s' % line)
+ os.remove(os.path.join(svn_repo, sp[1].strip()))
----------------
This isn't your change, but I think this will break if you try to touch a file with a space in its name, and we do have such files in the tree. I guess we could fix it later if you wanted to leave a TODO.
================
Comment at: llvm/utils/git-svn/git-llvm:160
+ raise RuntimeError('Unexpected svn status line: %s' % line)
+ os.remove(os.path.join(svn_repo, sp[1].strip()))
+ svn(svn_repo, 'update')
----------------
Nit, this isn't quite right if a filename starts or ends with " ". Which, I grant, is dumb. But I sort of worry about a file named ` ../info` (that is, a file named `info` in a directory named "` ..`"), which would cause us to delete your `.git/info`. Maybe there's a worse attack.
================
Comment at: llvm/utils/git-svn/git-llvm:176
+ if not os.path.isdir(svn_root):
+ die('Can\'t initialize svn staging dir (%s)' % (svn_root))
+
----------------
Nit, the usual convention I've seen is to use double-quotes when the string contains an apostrophe, on the grounds that that's cleaner than backslash-escaping it.
================
Comment at: llvm/utils/git-svn/git-llvm:176
+ if not os.path.isdir(svn_root):
+ die('Can\'t initialize svn staging dir (%s)' % (svn_root))
+
----------------
jlebar wrote:
> Nit, the usual convention I've seen is to use double-quotes when the string contains an apostrophe, on the grounds that that's cleaner than backslash-escaping it.
No need to put `svn_root` in parens. I don't object if you want to put them everywhere (`%` formatting definitely has its quirks, and using parens makes them go away, although at the expense of more typing, so I usually leave them out), but we should be consistent.
================
Comment at: llvm/utils/git-svn/git-llvm:179
+
+def svn_push(args):
+ '''Push changes back to SVN: this is extracted from Justin Lebar's script
----------------
The convention I've seen in other similar programs is to call these top-level functions `cmd_foo`, where `foo` is the name typed on the command line (i.e. `git llvm foo`).
I kind of like this convention; see the confusion I had about `svn_push` vs `push` below.
================
Comment at: llvm/utils/git-svn/git-llvm:182
+ available here: https://github.com/jlebar/llvm-repo-tools/
+ '''
+ # Get the git root
----------------
I'm going to take down the script since this will be canonical, so this won't be helpful, I think. Although I do appreciate it. :)
================
Comment at: llvm/utils/git-svn/git-llvm:186
+ if not os.path.isdir(git_root):
+ die('Can\'t find git root dir')
+
----------------
Same here with `\'`.
================
Comment at: llvm/utils/git-svn/git-llvm:204
+ clean_and_update_svn(svn_root)
+ push(svn_root, r, dry_run)
+
----------------
Maybe `svn_push_one_rev`, or `push_one_rev_to_svn` or something? Otherwise it wasn't clear to me how these two functions interacted, but I originally believed that `push` called `svn_push`, because `push` matched the name of the top-level command (i.e. `git llvm push`) and because it appears below `svn_push` in the topological sort.
(May be worth fixing the topo sort too.)
================
Comment at: llvm/utils/git-svn/git-llvm:240
+ raise RuntimeError('patch left a .orig/.rej file in the svn repo: '
+ '%s.' % line)
+ for l in (l for l in status_lines if l.startswith('?')):
----------------
This is actually no longer necessary. We needed this when I was using `patch`, but `git apply`'s man page says it will never leave `.rej` files around unless you pass `--reject`, hooray.
================
Comment at: llvm/utils/git-svn/git-llvm:252
+ else:
+ log('Would have committed %s to svn, if this weren\'t a dry run.'
+ % rev)
----------------
Same here with `\'`.
================
Comment at: llvm/utils/git-svn/git-llvm:261
+ description=desc)
+ subparsers = p.add_subparsers(title='subcommands',
+ description='valid subcommands',
----------------
Should this var be called `subcommands`?
================
Comment at: llvm/utils/git-svn/git-llvm:267
+ p.add_argument('-v', '--verbose', action='count', default=0,
+ help='print more information')
+
----------------
Why are we doing `count` as opposed to `store_true`? VERBOSE and QUIET are nominally bools, per the top of the file, but then below we store an `int` into them...
================
Comment at: llvm/utils/git-svn/git-llvm:287
+ 'an explicit upstream)')
+ parser_push.set_defaults(func=svn_push)
+ args = p.parse_args(argv)
----------------
Huh, I've never seen that before. That is cool!
================
Comment at: llvm/utils/git-svn/git-llvm:293
+ if QUIET and VERBOSE:
+ die('Error -v and -q cannot be used together: make your mind!')
+
----------------
Should be
> make *up* your mind
But you probably want https://docs.python.org/2.7/library/argparse.html#mutual-exclusion
https://reviews.llvm.org/D26334
More information about the llvm-commits
mailing list