[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 inline comments.


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


================
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:
> 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.
OK, fixed. That's a one liner though, I didn't feel like pulling it in a separate function. See line 156, was: `svn(svn_repo, 'update')` and is now `svn(svn_repo, 'update', *list(GIT_TO_SVN_DIR.values()))`.



================
Comment at: llvm/utils/git-svn/git-llvm:40
+TODO
+'''
+
----------------
jlebar wrote:
> 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.
Removed. It was a remainder of git-clang-format (desc was used for argparse, I replaced with `__doc__`).


================
Comment at: llvm/utils/git-svn/git-llvm:96
+    if elapsed >= .1:
+        log_verbose('Warning: command took %0.1fs' % elapsed)
+
----------------
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?


================
Comment at: llvm/utils/git-svn/git-llvm:143
+    # filter empty entries
+    revs = [r for r in revs if r != '']
+    if not revs:
----------------
jlebar wrote:
> Don't you just want to strip the output of `git` instead?
For some reason, I couldn't get it to not have an empty entry after the split, even by adding `strip()` or `rstrip()` before.
However replacing `split('\n')` with `splitlines()` address the issue.


================
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()))
----------------
jlebar wrote:
> 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.
Fixed.


================
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')
----------------
jlebar wrote:
> 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.
I think we can wait for someone able to commit this at the top of the SVN repo.


Repository:
  rL LLVM

https://reviews.llvm.org/D26334