Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update: fixed typos in commit message #347

Closed

Conversation

SyntevoAlex
Copy link

Commit 1/2: t0028: fix test for UTF-16-LE-BOM
Commit 2/2: t0028: add more tests
Please refer to individual commit messages for more information.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 19, 2019

Welcome to GitGitGadget

Hi @SyntevoAlex, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that this Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form /allow <username>.

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions.

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via:

curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

@SyntevoAlex
Copy link
Author

@dscho Please allow the use of GitGitGadget for me

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say anything about the new tests, but that's for the Git mailing list to say, anyway ;-)

@SyntevoAlex
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 23, 2019

Submitted as pull.347.git.gitgitgadget@gmail.com

@SyntevoAlex SyntevoAlex changed the title t0028 fix test + more tests Update: fixed typos in commit message Sep 23, 2019
@SyntevoAlex
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 23, 2019

Submitted as pull.347.v2.git.gitgitgadget@gmail.com

According to its name, the test is designed for UTF-16-LE-BOM.
However, possibly due to copy&paste oversight, it was using UTF-32.

While the test succeeds (extra \000\000 are interpreted as NUL),
I myself had an unrelated problem which caused the test to fail.
When analyzing the failure I was quite puzzled by the fact that the
test is obviously bugged. And it seems that I'm not alone:
https://public-inbox.org/git/CAH8yC8kSakS807d4jc_BtcUJOrcVT4No37AXSz=jePxhw-o9Dg@mail.gmail.com/T/#u

Fix the test to follow its original intention.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
@SyntevoAlex SyntevoAlex force-pushed the #0189_t0028_fixes branch 2 times, most recently from f2fd3c6 to e48ca7b Compare September 24, 2019 10:13
After I discovered that UTF-16-LE-BOM test was bugged, I decided that
better tests are required. Possibly the best option here is to compare
git results against hardcoded ground truth.

The new tests also cover more interesting chars where (ANSI != UTF-8).

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
@SyntevoAlex
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 24, 2019

Submitted as pull.347.v3.git.gitgitgadget@gmail.com

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 26, 2019

On the Git mailing list, Torsten Bögershausen wrote (reply to this):

On Tue, Sep 24, 2019 at 03:40:28AM -0700, Alexandr Miloslavskiy via GitGit=
Gadget wrote:
> Commit 1/2: t0028: fix test for UTF-16-LE-BOM Commit 2/2: t0028: add mor=
e
> tests Please refer to individual commit messages for more information.
>
> Alexandr Miloslavskiy (2):
>   t0028: fix test for UTF-16-LE-BOM
>   t0028: add more tests
>

Thanks for the update -
Reviewed-by:  Torsten B=F6gershausen <tboegi@web.de>

@@ -40,7 +40,7 @@ test_expect_success 'setup test files' '
printf "$text" | write_utf16 >test.utf16.raw &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> After I discovered that UTF-16-LE-BOM test was bugged, I decided that

s/bugged/buggy/ perhaps?  Usually people do not place hidden
listening devices in tests ;-)

> better tests are required. Possibly the best option here is to compare
> git results against hardcoded ground truth.
> ...
> +test_commit_utf8_checkout_other "UTF-8"        "Test =D0=A2=D0=B5=D1=81=
=D1=82" "\124\145\163\164\040\320\242\320\265\321\201\321\202"
> +test_commit_utf8_checkout_other "UTF-16LE"     "Test =D0=A2=D0=B5=D1=81=
=D1=82" "\124\000\145\000\163\000\164\000\040\000\042\004\065\004\101\004=
\102\004"
> +test_commit_utf8_checkout_other "UTF-16BE"     "Test =D0=A2=D0=B5=D1=81=
=D1=82" "\000\124\000\145\000\163\000\164\000\040\004\042\004\065\004\101=
\004\102"
> +test_commit_utf8_checkout_other "UTF-16LE-BOM" "Test =D0=A2=D0=B5=D1=81=
=D1=82" "\377\376\124\000\145\000\163\000\164\000\040\000\042\004\065\004=
\101\004\102\004"
> +test_commit_utf8_checkout_other "UTF-16BE-BOM" "Test =D0=A2=D0=B5=D1=81=
=D1=82" "\376\377\000\124\000\145\000\163\000\164\000\040\004\042\004\065=
\004\101\004\102"
> +test_commit_utf8_checkout_other "UTF-32LE"     "Test =D0=A2=D0=B5=D1=81=
=D1=82" "\124\000\000\000\145\000\000\000\163\000\000\000\164\000\000\000=
\040\000\000\000\042\004\000\000\065\004\000\000\101\004\000\000\102\004\=
000\000"
> +test_commit_utf8_checkout_other "UTF-32BE"     "Test =D0=A2=D0=B5=D1=81=
=D1=82" "\000\000\000\124\000\000\000\145\000\000\000\163\000\000\000\164=
\000\000\000\040\000\000\004\042\000\000\004\065\000\000\004\101\000\000\=
004\102"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):

On 28.09.2019 6:47, Junio C Hamano wrote:
> s/bugged/buggy/ perhaps?  Usually people do not place hidden
> listening devices in tests ;-)

Yes, hinting those hidden listening devices was an oversight. Thanks for 
your help in putting them back undercover!

I understand that you already changed the commit message, so I'm not 
submitting updated patch.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 30, 2019

This branch is now known as am/t0028-utf16-tests.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 30, 2019

This patch series was integrated into pu via git@e4dff99.

@gitgitgadget gitgitgadget bot added the pu label Sep 30, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 2, 2019

This patch series was integrated into pu via git@7d81b61.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 3, 2019

This patch series was integrated into pu via git@88d7a0f.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2019

This patch series was integrated into pu via git@6ed56a1.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2019

This patch series was integrated into pu via git@ce68bef.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 6, 2019

This patch series was integrated into pu via git@9bef41e.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 6, 2019

This patch series was integrated into pu via git@0c7fbc6.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 7, 2019

This patch series was integrated into pu via git@a22681f.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 8, 2019

This patch series was integrated into pu via git@6b6bdcf.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 9, 2019

This patch series was integrated into pu via git@252c003.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 9, 2019

This patch series was integrated into pu via git@03171f3.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 9, 2019

This patch series was integrated into next via git@453900a.

@gitgitgadget gitgitgadget bot added the next label Oct 9, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 11, 2019

This patch series was integrated into pu via git@3a52cad.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

This patch series was integrated into pu via git@6ed610b.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

This patch series was integrated into next via git@6ed610b.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

This patch series was integrated into master via git@6ed610b.

@gitgitgadget gitgitgadget bot added the master label Oct 15, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

Closed via 6ed610b.

@gitgitgadget gitgitgadget bot closed this Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants