| Message ID | pull.675.v5.git.git.1576504942.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | git-p4: Usability enhancements | expand |
"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes: > Some user interaction with git-p4 is not as user-friendly as the rest of the > ... > > Ben Keene (4): > git-p4: yes/no prompts should sanitize user text > git-p4: show detailed help when parsing options fail > git-p4: wrap patchRCSKeywords test to revert changes on failure > git-p4: failure because of RCS keywords should show help The reviews on the list seem to be in favor of these and I didn't see much wrong (I think I fixed up an indented empty line) in the series. I'd appreciate a blessing from a git-p4 expert, though. Thanks.
On Mon, 16 Dec 2019 at 20:39, Junio C Hamano <gitster@pobox.com> wrote: > > "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > Some user interaction with git-p4 is not as user-friendly as the rest of the > > ... > > > > Ben Keene (4): > > git-p4: yes/no prompts should sanitize user text > > git-p4: show detailed help when parsing options fail > > git-p4: wrap patchRCSKeywords test to revert changes on failure > > git-p4: failure because of RCS keywords should show help > > The reviews on the list seem to be in favor of these and I didn't > see much wrong (I think I fixed up an indented empty line) in the > series. I'd appreciate a blessing from a git-p4 expert, though. > $ git log --reverse --oneline --abbrev-commit origin/maint..origin/bk/p4-misc-usability e2aed5fd5b git-p4: yes/no prompts should sanitize user text - looks good to me 608e380502 git-p4: show detailed help when parsing options fail - also looks good to me c4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure - why not just catch the exception, and then drop out of the "if-" condition and fall into the cleanup section at the bottom of that function (line 1976)? As it stands, this is duplicating the cleanup code now. 89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of RCS keywords should show help - strictly speaking, the code does not actually check if there *are* any RCS keywords, it just checks if the filetype means that RCS kws *would* be expanded *if* they were present. The conflict might be just because....there's a conflict. As it stands this will be giving misleading advice. I would get it to check to see if there really are any RCS keywords in the file. > Thanks.
Luke Diamand <luke@diamand.org> writes: > $ git log --reverse --oneline --abbrev-commit > origin/maint..origin/bk/p4-misc-usability > e2aed5fd5b git-p4: yes/no prompts should sanitize user text > - looks good to me > > 608e380502 git-p4: show detailed help when parsing options fail > - also looks good to me > > c4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure > - why not just catch the exception, and then drop out of the "if-" > condition and fall into the cleanup section at the bottom of that > function (line 1976)? As it stands, this is duplicating the cleanup > code now. > > 89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of > RCS keywords should show help > - strictly speaking, the code does not actually check if there *are* > any RCS keywords, it just checks if the filetype means that RCS kws > *would* be expanded *if* they were present. The conflict might be just > because....there's a conflict. As it stands this will be giving > misleading advice. I would get it to check to see if there really are > any RCS keywords in the file. Thanks. Ben, let's keep the first two and discard the rest for now, which can later be replaced with updated ones.
On 12/25/2019 2:13 PM, Junio C Hamano wrote: > Luke Diamand <luke@diamand.org> writes: > >> $ git log --reverse --oneline --abbrev-commit >> origin/maint..origin/bk/p4-misc-usability >> e2aed5fd5b git-p4: yes/no prompts should sanitize user text >> - looks good to me >> >> 608e380502 git-p4: show detailed help when parsing options fail >> - also looks good to me >> >> c4dc935311 git-p4: wrap patchRCSKeywords test to revert changes on failure >> - why not just catch the exception, and then drop out of the "if-" >> condition and fall into the cleanup section at the bottom of that >> function (line 1976)? As it stands, this is duplicating the cleanup >> code now. >> >> 89c88c0ecf (origin/bk/p4-misc-usability) git-p4: failure because of >> RCS keywords should show help >> - strictly speaking, the code does not actually check if there *are* >> any RCS keywords, it just checks if the filetype means that RCS kws >> *would* be expanded *if* they were present. The conflict might be just >> because....there's a conflict. As it stands this will be giving >> misleading advice. I would get it to check to see if there really are >> any RCS keywords in the file. > Thanks. Ben, let's keep the first two and discard the rest for now, > which can later be replaced with updated ones. That works for me. So, are there any changes that I should make at this time, or just let the rest die off?
Ben Keene <seraphire@gmail.com> writes: >> Thanks. Ben, let's keep the first two and discard the rest for now, >> which can later be replaced with updated ones. > That works for me. So, are there any changes that I should make at > this time, or just let the rest die off? I don't think of any, from this side. You can of course spend time on salvaging and polishing these remaining patches for resubmission in future cycle(s). Thanks.
Some user interaction with git-p4 is not as user-friendly as the rest of the Git ecosystem. Here are three areas that can be improved on: 1) When a patch fails and the user is prompted, there is no sanitization of the user input so for a "yes/no" question, if the user enters "YES" instead of a lowercase "y", they will be re-prompted to enter their answer. Commit 1 addresses this by sanitizing the user text by trimming and lowercasing their input before testing. Now "YES" will succeed! 2) If the command line arguments are incorrect for git-p4, the program reports that there was a syntax error, but doesn't show what the correct syntax is. Commit 2 displays the context help for the failed command. 3) If Git generates an error while attempting to clean up the RCS Keyword expansions, it currently leaves P4 in an invalid state. Files that were checked out by P4 are not revereted. Commit 3 adds and exception handler that catches this condition and issues a P4 Revert for the files that were previously edited. 4) Git can handle scraping the RCS Keyword expansions out of source files when it is preparing to submit them to P4. However, if the config value "git-p4.attemptRCSCleanup" isn't set, it will just report that it fails. Commit 4 adds a helpful suggestion, that the user might want to set git-p4.attemptRCSCleanup. Revisions ========= v3 - Implemented the various suggestions from Luke and Denton. I did not add additional exception handling for the EOFError in the prompt method. I do believe that it is a good idea, but that would change the logic handling of the existing code to handle this new "no answer" condition and I didn't want to introduce that at this time. v4 - Whitespace clean up and commit clarifications. Submit 3 suggested some clarifications to the commit test and revealed some whitespace errors. v5 - Fixed typo in a commit message. (Invalid attribute to Thanks-to: Denton Liu liu.denton@gmail.com [liu.denton@gmail.com]) Ben Keene (4): git-p4: yes/no prompts should sanitize user text git-p4: show detailed help when parsing options fail git-p4: wrap patchRCSKeywords test to revert changes on failure git-p4: failure because of RCS keywords should show help git-p4.py | 93 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 34 deletions(-) base-commit: ad05a3d8e5a6a06443836b5e40434262d992889a Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-675%2Fseraphire%2Fseraphire%2Fp4-usability-v5 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-675/seraphire/seraphire/p4-usability-v5 Pull-Request: https://github.com/git/git/pull/675 Range-diff vs v4: 1: 6c23cd5684 ! 1: 7e0145fa32 git-p4: yes/no prompts should sanitize user text @@ -26,7 +26,7 @@ choices, remove the loop from the calling code that handles response verification. - Thanks-to: Denton Liu <Denton Liu> + Thanks-to: Denton Liu <liu.denton@gmail.com> Signed-off-by: Ben Keene <seraphire@gmail.com> diff --git a/git-p4.py b/git-p4.py 2: bfdd3dc517 = 2: 4960d1fa22 git-p4: show detailed help when parsing options fail 3: 20f6398693 = 3: 81a09a1228 git-p4: wrap patchRCSKeywords test to revert changes on failure 4: c78e2e4db1 = 4: 4c4b783fd5 git-p4: failure because of RCS keywords should show help