Bugfix/help text and reorder rename args

[bmcdonnell]

Originally submitted against petervanderdoes/gitflow-avh:

c0b4b96 and d6e43fd add/revise some help text. I think it's a given that you'll want to pull those in, except maybe if you want to change the printout order or something.

cf3885e may be debatable. It flips the order of the rename sub-subcommand arguments to match git conventions--most compellingly, git branch --move [<oldbranch>] <newbranch> (where oldbranch is optional and first). In other cases, "old" isn't optional, but it's still first:

• git mv <source> <destination>
• git diff <oldcommit> [<newcommit>]
• git remote rename <old> <new>

Since rename was just introduced in 1.11.0, less than a year ago, it wasn't fully documented, and error messages should point users in the right direction, would you consider this change is a reasonably small disruption?

[ChrisJStone]

Why are we testing to see if the first variable passed in is a null string here when we perform the exact same test a few lines down for the second variable?

[bmcdonnell]

Because the user may provide 0, 1, or 2 args?

Also note, this PR did not change that line.

[ChrisJStone]

Nesting the if else block used to determin how to call rename inside of this else made the code a little harder for me to initially follow as I had to go back up and figure out the context of when it's called.

[bmcdonnell]

Propose an alternative?

[ChrisJStone]

If it is necessary to set both NAME and New_Name to empty strings this can be done with out having to use an else clause and nest the actual test used to determine the number of arguments passed in.
I think a better option would be to adopt a fail early mindset. Instead of waiting tell after we have attempted to assign the old and new branch names before we test for a failure we do it before even attempting the assignments. Since rename has to have at least one argument passed to it. There is no need to test how many arguments were passed if rename was called with zero arugments.

[ChrisJStone]

Thank you for adding these missing commands

[ChrisJStone]

Thank you for catching that new name and old name were reversed I think the initial idea was that since the old name is optional there was no need to have it first. However it does make more sense to follow the standard git convention.

[bmcdonnell]

Asides:

• Should I have submitted the PR here (ChrisJStone/gitflow-cjs), or on CJ-Systems/gitflow-cjs?
• I think your use of review approval and setting this PR to draft are unconventional. I think the more conventional approach would be for you to withhold approval on the review pending my addressing your review comments, and put/leave it in the "Request Changes" state until I do. Usually, IME, it's only the submitter who moves a PR to/from Draft state.

[ChrisJStone]

* Should I have submitted the PR here (ChrisJStone/gitflow-cjs), or on [CJ-Systems/gitflow-cjs](https://github.com/CJ-Systems/gitflow-cjs)?

While the official repo is at CJ-Systems/gitflow-cjs I do not mind PR's being submitted to my "personal dev" copy. If you feel it may cause issue or confusion for future PR's I have no problems with making my copy private.

* I think your use of review approval and setting this PR to draft are unconventional. I think the more conventional >approach would be for you to withhold approval on the review pending my addressing your review comments, and >put/leave it in the "Request Changes" state until I do. 

Thank you for the feedback on this. I typically don't use the webUI side of github and at the time I did not realize that even though the intent was to approve the change I had been commenting on it was going to "approve" the entire PR.

Usually, IME, it's only the submitter who moves a PR to/from Draft >state.

I shall keep that in mind thank you for letting me know.

[bmcdonnell]

If it is necessary to set both NAME and New_Name to empty strings this can be done with out having to use an else clause and nest the actual test used to determine the number of arguments passed in. I think a better option would be to adopt a fail early mindset. Instead of waiting tell after we have attempted to assign the old and new branch names before we test for a failure we do it before even attempting the assignments. Since rename has to have at least one argument passed to it. There is no need to test how many arguments were passed if rename was called with zero arugments.

This feels a bit like bikeshedding to me.

That said, I tried to make minimal changes to the code and to the approach. (See git difftool -y cf3885e^ cf3885e -- gitflow-common.) I think your request here would be a small refactor, a bit outside the scope of what I was trying to achieve.

One option would be to keep it as I have it, to keep the differences minimal in case gitflow-avh ever resurrects. Alternatively, you could make your proposed changes in a separate commit, and push onto my branch.

[bmcdonnell]

While the official repo is at CJ-Systems/gitflow-cjs

Oh, I'm not sure how I wound up on this one, then. Might as well at least finish our discussion here, then - if not merge it here, too.

I do not mind PR's being submitted to my "personal dev" copy. If you feel it may cause issue or confusion for future PR's I have no problems with making my copy private.

IMO, going forward, better to have them all in one place, on the "official" repo. I'm not sure what's the best way to do that, though. I don't think you'll be able to make it private since it's a fork of a public repo. Maybe disallow further issues and/or PRs on this one?

Thank you for the feedback on this.

Welcome. :)

I typically don't use the webUI side of github and at the time I did not realize that even though the intent was to approve the change I had been commenting on it was going to "approve" the entire PR.

Interesting. What interface did you use?

[bmcdonnell]

Usually, IME, it's only the submitter who moves a PR to/from Draft >state.

I shall keep that in mind thank you for letting me know.

I'm doubting myself about this bit now, since the way I moved it out of draft just now was by clicking "Ready for Review".

See also https://github.blog/changelog/2020-04-08-convert-pull-request-to-draft/

[ChrisJStone]

That said, I tried to make minimal changes to the code and to the approach.
I think your request here would be a small refactor, a bit outside the scope of what I was trying to achieve.

Fair enough, I can appreciate wanting to keep the changes simple that way if gitflow-avh ever does become active again, the changes can be easily committed there as well.

Interesting. What interface did you use?

I was using the vs code plugin Github Pull Requests and Issues. However I didn't like the comment dialog scrolling off screen when adding a comment for the entire file when you tried to scroll down.

I'll get this merged and pushed to both repos and see what I can do about preventing a similar situation again in the future.

Oh, I'm not sure how I wound up on this one, then.

I actually just assumed that you saw the gitflow-cjs and didn't realize that the first one was for my "personal" copy since it's listed first due to how github sorts the dropdown.