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

Test(git-authors): add unit test #1088

Closed
vanpipy opened this issue Oct 17, 2023 · 12 comments · Fixed by #1098
Closed

Test(git-authors): add unit test #1088

vanpipy opened this issue Oct 17, 2023 · 12 comments · Fixed by #1098

Comments

@vanpipy
Copy link
Collaborator

vanpipy commented Oct 17, 2023

File

git-authors

git-authors has a weird behavior that invoke the command directly without AUTHORS or target file and the result will be insert into the git-authors file, then open it via the default editor. Take a look here. I don't think it a good case to use the command.

Testcases

  1. update the existed AUTHORS file with invoking directly.
  2. insert the authors information in the target file when invoking it with a parameter. update with another PR.
  3. list all authors
  4. list all authors without the email

There is no the function which open the authors file via the default editor any more.

References

@spacewander
Copy link
Collaborator

Is it relative to 47e9402?
CC @hyperupcall

It is weird as find . -mindepth 1 -maxdepth 1 -iregex '.*\(authors\|contributors\).*' is expected to just look up the file in the current directory.

BTW, find . -mindepth 1 -maxdepth 1 -iregex '.*\(authors\|contributors\).*' seems broken in Mac. It doesn't show up the AUTHORS file:

~/git/git-extras(master ✗)14:47 find . -mindepth 1 -maxdepth 1 -iregex '.*\(authors\|contributors\).*' | head -n1
~/git/git-extras(master ✗)14:47 ls | grep -E 'authors|contributors' -i
AUTHORS

@hyperupcall
Copy link
Collaborator

hyperupcall commented Oct 18, 2023

@spacewander Yes, it does look up the file in the current directory, because that was the behavior previously before I changed it. Maybe it's worth changing it to look up the file relative to the Git worktree root.

I tested the command on Linux and it works, so maybe there is a difference

From find(1):

The regular expressions understood by find are by default Emacs Regular Expressions (except that `.' matches new‐line), but this can be changed with the -regextype option.

> find -regextype help
find: Unknown regular expression type ‘help’; valid types are ‘findutils-default’, ‘ed’, ‘emacs’, ‘gnu-awk’, ‘grep’, ‘posix-awk’, ‘awk’, ‘posix-basic’, ‘posix-egrep’, ‘egrep’, ‘posix-extended’, ‘posix-minimal-basic’, ‘sed’.

For example, it could be that the functionality for Emacs Regular Expressions is dynamically loaded, and the library is installed in most Linux distros, but not macOS, so on macOS maybe uses Extended Regular Expressions by default (which doesn't require escaping the end or close capture group syntax).

@hyperupcall
Copy link
Collaborator

Now that I think about it, I remember previously running into errors on macOS where Bash regular expressions behaved differently than compared to Linux. I don't know exactly which libraries or versions is used to interpret the regular expression (BRE, ERE).

@hyperupcall
Copy link
Collaborator

I wonder if changing the line to changelog="$(find . -mindepth 1 -maxdepth 1 -regextype posix-extended -iregex '.*(change|history).*' | head -n1)" fixes things.

I didn't set -regextype originally because I didn't want to set the regextype to one that isn't supported on platforms other than Linux.

@vanpipy
Copy link
Collaborator Author

vanpipy commented Oct 18, 2023

alright, it looks a history here and I think there is no more bad effect for the user work with git-extras cause the git-authors may a cold command for some repos and our ci runs on ubuntu only. Thanks your research and I will do a little bit digging. Thanks guys!

@spacewander
Copy link
Collaborator

I wonder if changing the line to changelog="$(find . -mindepth 1 -maxdepth 1 -regextype posix-extended -iregex '.*(change|history).*' | head -n1)" fixes things.

I didn't set -regextype originally because I didn't want to set the regextype to one that isn't supported on platforms other than Linux.

It doesn't work with the BSD find, which is the find in Mac:

~/git/git-extras(master ✗)16:56 find . -mindepth 1 -maxdepth 1 -regextype posix-extended -iregex '.*(change|history).*' | head -n1
find: -regextype: unknown primary or operator

@spacewander
Copy link
Collaborator

@hyperupcall
I tested the find . -iregex today on Mac. It seems the regex engine in BSD find doesn't recognize the | in the expression. Do you have any suggestions?

@hyperupcall
Copy link
Collaborator

hyperupcall commented Oct 23, 2023

There might be two alternatives:

BSD man page seems to indivate that -E is supported to interpret regexes as ERE:

changelog="$(find -E . -mindepth 1 -maxdepth 1 -iregex '.*(change|history).*' | head -n1)"

I'm actually thinking... it would be better not to use regular expressions. The -o expression is specified by POSIX even:

find . -mindepth 1 -maxdepth 1 \( -iname '*change*' -o -iname '*history*' \)

@hyperupcall
Copy link
Collaborator

I have made #1094, which should fix this

@vanpipy
Copy link
Collaborator Author

vanpipy commented Oct 24, 2023

File

git-authors

git-authors has a weird behavior that invoke the command directly without AUTHORS or target file and the result will be insert into the git-authors file, then open it via the default editor. Take a look here. I don't think it a good case to use the command.

Testcases

  1. update the existed AUTHORS file with invoking directly.
  2. insert the authors information in the target file when invoking it with a parameter.
  3. list all authors
  4. list all authors without the email

I recommand to transform the case 1 and 2 to below:

  • move git-authors to git-authors --output authors.txt to update the AUTHORS file.

because the empty argument means update a file or output something to a file but the arguments else did not work with it. It does not make sense.

@spacewander
Copy link
Collaborator

File

git-authors
git-authors has a weird behavior that invoke the command directly without AUTHORS or target file and the result will be insert into the git-authors file, then open it via the default editor. Take a look here. I don't think it a good case to use the command.

Testcases

  1. update the existed AUTHORS file with invoking directly.
  2. insert the authors information in the target file when invoking it with a parameter.
  3. list all authors
  4. list all authors without the email

I recommand to transform the case 1 and 2 to below:

  • move git-authors to git-authors --output authors.txt to update the AUTHORS file.

because the empty argument means update a file or output something to a file but the arguments else did not work with it. It does not make sense.

Agree to provide an argument to specify the output manually is better. Also, we need to keep the compatibility with the old behavior.

@vanpipy
Copy link
Collaborator Author

vanpipy commented Oct 27, 2023

Agree to provide an argument to specify the output manually is better. Also, we need to keep the compatibility with the old behavior.

copy that. Finally, i can generate the usecases lol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants