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

[Mehdi AMINI via llvm-commits]

mehdi_amini marked 11 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:
> > > 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.
I added it to the docstring of `cmd_push`, so that now `git llvm push -h` prints:

```
$ git llvm push -h
usage: git llvm push [-h] [-n] [GIT_REVS]

Push changes back to SVN: this is extracted from Justin Lebar's script
available here: https://github.com/jlebar/llvm-repo-tools/ Note: a current
limitation is that git does not track file renames, so they will show up in SVN
as delete+add.

positional arguments:
  GIT_REVS       revs to push (default: everything not in the branch's
                 upstream, or not in origin/master if the branch lacks an
                 explicit upstream)

optional arguments:
  -h, --help     show this help message and exit
  -n, --dry-run  Do everything other than commit to svn. Leaves junk in the
                 svn repo, so probably will not work well if you try to commit
                 more than one rev.
```


================
Comment at: llvm/utils/git-svn/git-llvm:282
+    # Dispatch to the right subcommand
+    args.func(args)
----------------
jlebar wrote:
> 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`)?
It will print:

```
usage: git llvm [-h] [-q | -v] {push} ...
git llvm: error: too few arguments
```



https://reviews.llvm.org/D26334