Merge ~racb/git-ubuntu:push-for-upload into git-ubuntu:master

PASSED: Continuous integration, rev:39ec1573ab470c1770e3e75472a8260a08ea7f0f
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/43/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: VM Reset
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/43//rebuild

FAILED: Continuous integration, rev:553e3707f0b096fa71602a8d5f8c37d2a6bfdb4f
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/44/
Executed test runs:
    SUCCESS: VM Setup
    FAILED: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/44//rebuild

FAILED: Continuous integration, rev:a367f05906c22654bee3243b756dd34d9f254dbf
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/45/
Executed test runs:
    SUCCESS: VM Setup
    FAILED: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/45//rebuild

Is there a reason this couldn't just be simply 'git ubuntu push'?

I think that would be both simpler and more intuitive (I was actually a bit uncertain if 'push-for-upload' implied it also ran dput, but 'git ubuntu push' would feel more analogous to 'git push').

---

Note the code docs in the tests use single quotes, while the one in push_for_upload.py has double quotes.

---

Can you also add an OUTPUT_FORMAT setting for debuild; I think that command just wrappers dpkg-buildpackage so probably uses the same values. (Maybe it could just be an alias.)

---

For --output-format the help text is just "Output format", which feels like it could be made more helpful. Could this be modified to indicate that the only allowed values for this are 'dpkg-buildpackage' or 'sbuild' (and/or 'debuild' if you accept by previous suggestion)?

---

The command's description is set as "Push a branch for subsequent upload with dput" but this feels a bit awkward and potentially confusing for new users. The description for main() feels clearer:

  "Push to a remote and return suitable rich history reference headers"

I know we're limited on line length, so here's a shot at saying that more briefly:

  "Push branch and print history-enriched headers for dput"

---

Whitespace indentation is off for first entry in the expected_result set for all three test cases.

---

Would it be difficult to add a test_main_without_lpuser() (or extend test_main_without_remote()) for the situation where neither lpuser nor a remote are specified, that the appropriate error message is generated?

---

The test cases are checking only for the expected results for dpkg-buildpackage style OUTPUT_FORMATS. Would it make sense to check for the sbuild results as well?

Maybe the args to the target.main() should be passed in via @pytest.mark.parametrize, and possibly expected_results too?

---

I haven't actually run the command or tests yet, but hopefully the above gives some useful food for thought.

Thank you for the review! As discussed, there are a bunch of open questions that I'd like leave open for now, landing the branch and releasing a beta, to get user feedback. This includes some of your review points, which I agree should be addressed before a final release.

> Is there a reason this couldn't just be simply 'git ubuntu push'?

> I think that would be both simpler and more intuitive (I was actually a bit uncertain if 'push-for-upload' implied it also ran dput, but 'git ubuntu push' would feel more analogous to 'git push').

I thought "git ubuntu push" would be assumed to be analogous to "git push", and therefore its output of "strange" command line parameters (which is its real purpose) would be confusing to new git-ubuntu users. So I didn't choose "push" as the subcommand name. I'm not sure I like "push-for-upload" either; I chose it for now because it also carries the implication that it's doing something different because you want to upload (as opposed to pushing for review, for example), and I was hoping "for" would imply that there wouldn't be additional side-effects such as a dput.

I'd like to land this as "push-for-upload" for now, but leave the subcommand name as part of the experiment and settle the final name further down the road.

> Note the code docs in the tests use single quotes, while the one in push_for_upload.py has double quotes.

I looked up PEP 8 and PEP 257, and they require double quotes. So let's make that our convention across the board. I won't bother updating code not touched by this MP, but from here on, double quotes it is for any new docstrings. I've updated this branch accordingly.

I would add to doc/STYLE.md except it already says "use PEP 8" and PEP 8 tells us to use double quotes, so it's already included by reference.

> Can you also add an OUTPUT_FORMAT setting for debuild; I think that command just wrappers dpkg-buildpackage so probably uses the same values. (Maybe it could just be an alias.)

Is it really worth doing this? Adding an alias needs more (alias-handling) code, dpkg-buildpackage (ie. debuild) is the default so nobody would need to change it for debuild anyway, and in any case debuild takes "dpkg-buildpackage compatible parameters" so if you consider the meaning of --output-format to be the output _format_ rather than the destination, it's correct to say --output-format=dpkg-buildpackage and pass the output to debuild anyway.

> For --output-format the help text is just "Output format", which feels like it could be made more helpful. Could this be modified to indicate that the only allowed values for this are 'dpkg-buildpackage' or 'sbuild' (and/or 'debuild' if you accept by previous suggestion)?

The only allowed values are already generated by the --help output:

    usage: git-ubuntu push-for-upload [-h] [-v] [--remote REMOTE] [--branch BRANCH]
                                      [--output-format {dpkg-buildpackage,sbuild}]

I'd prefer to stick to argparse convention here, and avoid overloading that into the help parameter as well. I do intend to improve the documentation generally (manpage, describe the arguments better, fix the default output of --help) but I'd like to defe...

PASSED: Continuous integration, rev:8b94d19a817afa5cb355af42a82706131c895ee5
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/49/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: VM Reset
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/49//rebuild