git ubuntu review subcommand

[Nish Aravamudan]

For review of MPs

Take a --mp/--merge-proposal flag which will create a new repository with the appropriate refs pulled down and lint that, possibly other checks.

[Nish Aravamudan (nacc)]

On hold until the larger refactor for main can be done, which will resolve the review commetns.

[Christian Ehrhardt  (paelzer)]

Just came by "this need" as well and wanted to ack and +1 on the prio/need.

We had a case where reviewing https://code.launchpad.net/~ahasenack/ubuntu/+source/bind9/+git/bind9/+merge/329548 with "a classic" checkout as e.g. described on the page ends up dirty.
That is confusing at best.
I'd think that a git ubuntu review could do all that as Nish outlined.
Making the full tree available (for comparison and history), fetching the MP and checking it out while considering all quirks needed.

[15:21] <andreas> cpaelzer: how can a git clone checkout be dirty from the start?
[15:21] <andreas> I got the same, btw
[15:21] <andreas> http://pastebin.ubuntu.com/25472136/
[15:22] <cpaelzer> andreas: that surpasses my git-foo as well for now
[15:22] <andreas> rbasak: do you know? ^
[15:23] <rbasak> andreas: any .gitattributes files?
[15:24] <andreas> maybe, those files all end with ^M$
[15:24] <andreas> $ cat .gitattributes
[15:24] <andreas> *.vcxproj.in eol=crlf
[15:24] <andreas> :/
[15:24] <andreas> so what now?
[15:24] <rbasak> git ubuntu clone is supposed to work around that IIRC.
[15:24] <andreas> rbasak: it does it seems, this happens when you git clone
[15:25] <rbasak> You can look up what it does.
[15:25] <andreas> following the launcphad instructions when you visit a page like https://code.launchpad.net/~ahasenack/ubuntu/+source/bind9/+git/bind9/+ref/artful-bind9-merge-9.10.3.dfsg.P4-12.6
[15:25] <rbasak> I think it puts an override in .git/info/gitattributes or something like that
[15:25] <rbasak> $ cat .git/info/attributes
[15:25] <rbasak> * -ident
[15:25] <rbasak> * -text
[15:25] <rbasak> * -eol
[15:26] <andreas> I have that in the git ubuntu clone
[15:27] <andreas> rbasak: do cpaelzer or I need to do something about this? He was reviewing, and used git glone and saw this huge diff
[15:27] <rbasak> This is why we have git ubuntu clone.
[15:27] <rbasak> We had to wrap it to avoid problems like this.
[15:28] <cpaelzer> I never used it for the reviews actually
[15:28] <cpaelzer> do we have that wrapped into the git ubuntu cli already?
[15:29] <rbasak> You could "git ubuntu clone bind9" and then "git fetch ..." and then examine FETCH_HEAD.
[15:29] <cpaelzer> or TL;DR what would be the (better) command to use to fetch the MP for review - if possible along all of bind9 in USDI anyway
[15:29] <rbasak> Or "git ubuntu remote add ahasenack" and then "git fetch ahasenack <branch name>" IIRC.
[15:29] <cpaelzer> ah yeah I remember using the remote add
[15:29] <cpaelzer> and in general that is close to what I'd want to do
[15:29] <rbasak> Perhaps we should make it so that you can "git ubuntu clone <MP URL>"
[15:30] <cpaelzer> that would be nice
[15:30] <cpaelzer> or even
[15:30] <cpaelzer> git ubuntu review MP
[15:30] <rbasak> Please file bugs.
[15:30] <cpaelzer> making it a clone + MP fetch, checking out the MP

Well the bug exists already so bump+1 here.

[Robie Basak (racb)]

I completely agree with the principle that we should have this capability.

I wonder though about CLI design and not surprising the user in being able to predict whether a subcommand creates a new subdirectory or is expected to operate on a current working tree.

What if "git ubuntu clone" were the only command that creates a new local repository (creating a subdirectory etc) and all other subcommands only operate on an existing repository except with a really explicit option (eg. --create or --clone)? This would make which behaviour is about to happen very predictable.

Then this functionality might be in "git ubuntu review" if it exists on an existing cloned repository (eg. you already have a clone on it but would like to review an MP from someone else), but you could still have an automatic clone with either "git ubuntu review --clone ..." or "git ubuntu clone <MP URL>".

[Christian Ehrhardt  (paelzer)]

Workaround until provided:
For a case like a MP at [1], which states on the LP page to do:
$ git clone -b artful-bind9-merge-9.10.3.dfsg.P4-12.6 https://git.launchpad.net/~ahasenack/ubuntu/+source/bind9

From that we take the user and the branch name for the steps you need:
user: ahasenack
branch: artful-bind9-merge-9.10.3.dfsg.P4-12.6

Instead do:
git ubuntu clone bind9
cd bind9
# user
git ubuntu remote add ahasenack
# branch
git checkout ahasenack/artful-bind9-merge-9.10.3.dfsg.P4-12.6

[1]: https://code.launchpad.net/~ahasenack/ubuntu/+source/bind9/+git/bind9/+merge/329548

[Nish Aravamudan (nacc)]

On Tue, Sep 5, 2017 at 7:41 AM, Robie Basak <email address hidden> wrote:
> I completely agree with the principle that we should have this
> capability.
>
> I wonder though about CLI design and not surprising the user in being
> able to predict whether a subcommand creates a new subdirectory or is
> expected to operate on a current working tree.

Agreed here.

> What if "git ubuntu clone" were the only command that creates a new
> local repository (creating a subdirectory etc) and all other subcommands
> only operate on an existing repository except with a really explicit
> option (eg. --create or --clone)?

I believe right now, the only commands that "create" a local
repository are clone/import* (the latter is really 3 subcommands). I
think it's fine for the importer to create a repository without an
explicit option, as it's not really an importer otherwise. And reusing
a directory is the rarer case, IMO.

> This would make which behaviour is
> about to happen very predictable.
>
> Then this functionality might be in "git ubuntu review" if it exists on
> an existing cloned repository (eg. you already have a clone on it but
> would like to review an MP from someone else), but you could still have
> an automatic clone with either "git ubuntu review --clone ..." or "git
> ubuntu clone <MP URL>".

Agreed on the former option. I don't like implementing code that will
have to parse the argument to `git ubuntu clone` to decide if it's a
srcpkg name or an MP URL.

To be clear, to enable this, we would have a --clone flag for `git
ubuntu review` and would probably need `git ubuntu review` without the
--clone flag to fetch both the source and target remotes (adding them
as necessary) (if the latter is ~usd-import-team, then fetch pkg
instead) and running the lint on the requested branch?

[Robie Basak (racb)]

On Thu, Sep 07, 2017 at 09:05:22PM -0000, Nish Aravamudan wrote:
> On Tue, Sep 5, 2017 at 7:41 AM, Robie Basak <email address hidden> wrote:
> > What if "git ubuntu clone" were the only command that creates a new
> > local repository (creating a subdirectory etc) and all other subcommands
> > only operate on an existing repository except with a really explicit
> > option (eg. --create or --clone)?
>
> I believe right now, the only commands that "create" a local
> repository are clone/import* (the latter is really 3 subcommands). I
> think it's fine for the importer to create a repository without an
> explicit option, as it's not really an importer otherwise. And reusing
> a directory is the rarer case, IMO.

I hadn't really thought about the import subcommand. I'm not sure it's
consistent for it to do what it does now, but let's not worry about that
for 1.0. I think it's enough of a corner case for CLI UX to not be so
important for it.

> > This would make which behaviour is
> > about to happen very predictable.
> >
> > Then this functionality might be in "git ubuntu review" if it exists on
> > an existing cloned repository (eg. you already have a clone on it but
> > would like to review an MP from someone else), but you could still have
> > an automatic clone with either "git ubuntu review --clone ..." or "git
> > ubuntu clone <MP URL>".
>
> Agreed on the former option. I don't like implementing code that will
> have to parse the argument to `git ubuntu clone` to decide if it's a
> srcpkg name or an MP URL.

Implementing it may be a little ugly, but I think that "do what I mean"
is fine if not ambiguous from a CLI UX perspective and there would be no
ambiguity here. But that doesn't mean that we have to implement it, so
if you aren't convinced then there's no need for us to implement it for
1.0 so we can defer any decision about it.

> To be clear, to enable this, we would have a --clone flag for `git
> ubuntu review` and would probably need `git ubuntu review` without the
> --clone flag to fetch both the source and target remotes (adding them
> as necessary) (if the latter is ~usd-import-team, then fetch pkg
> instead) and running the lint on the requested branch?

If I understand you correctly, yes. So:

Without --clone: fail if not in repository (whatever "in" means). Fetch
source and target as you said, with magic for ~usd-import-team -> pkg.
Run lint.

With --clone: fail if in a respository (or at least require --force). Do
equivalent of "git ubuntu clone". Go into directory. Also fetch source
if not ~usd-import-team. Fetch target. Run lint.

Does that seem right to you?

[Nish Aravamudan (nacc)]

On 18.09.2017 [10:12:10 -0000], Robie Basak wrote:
> On Thu, Sep 07, 2017 at 09:05:22PM -0000, Nish Aravamudan wrote:
> > On Tue, Sep 5, 2017 at 7:41 AM, Robie Basak <email address hidden> wrote:
> > > What if "git ubuntu clone" were the only command that creates a new
> > > local repository (creating a subdirectory etc) and all other subcommands
> > > only operate on an existing repository except with a really explicit
> > > option (eg. --create or --clone)?
> >
> > I believe right now, the only commands that "create" a local
> > repository are clone/import* (the latter is really 3 subcommands). I
> > think it's fine for the importer to create a repository without an
> > explicit option, as it's not really an importer otherwise. And reusing
> > a directory is the rarer case, IMO.
>
> I hadn't really thought about the import subcommand. I'm not sure it's
> consistent for it to do what it does now, but let's not worry about that
> for 1.0. I think it's enough of a corner case for CLI UX to not be so
> important for it.
>
> > > This would make which behaviour is
> > > about to happen very predictable.
> > >
> > > Then this functionality might be in "git ubuntu review" if it exists on
> > > an existing cloned repository (eg. you already have a clone on it but
> > > would like to review an MP from someone else), but you could still have
> > > an automatic clone with either "git ubuntu review --clone ..." or "git
> > > ubuntu clone <MP URL>".
> >
> > Agreed on the former option. I don't like implementing code that will
> > have to parse the argument to `git ubuntu clone` to decide if it's a
> > srcpkg name or an MP URL.
>
> Implementing it may be a little ugly, but I think that "do what I mean"
> is fine if not ambiguous from a CLI UX perspective and there would be no
> ambiguity here. But that doesn't mean that we have to implement it, so
> if you aren't convinced then there's no need for us to implement it for
> 1.0 so we can defer any decision about it.
>
> > To be clear, to enable this, we would have a --clone flag for `git
> > ubuntu review` and would probably need `git ubuntu review` without the
> > --clone flag to fetch both the source and target remotes (adding them
> > as necessary) (if the latter is ~usd-import-team, then fetch pkg
> > instead) and running the lint on the requested branch?
>
> If I understand you correctly, yes. So:
>
> Without --clone: fail if not in repository (whatever "in" means). Fetch
> source and target as you said, with magic for ~usd-import-team -> pkg.
> Run lint.
>
> With --clone: fail if in a respository (or at least require --force). Do
> equivalent of "git ubuntu clone". Go into directory. Also fetch source
> if not ~usd-import-team. Fetch target. Run lint.
>
> Does that seem right to you?

Yeah, I think this is what we landed last week :)