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

fix #9284 (--- is incorrectly recognized as "the end of options") #9285

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dungsaga
Copy link
Contributor

@dungsaga dungsaga commented Dec 5, 2021

No description provided.

@codecov
Copy link

codecov bot commented Dec 5, 2021

Codecov Report

Merging #9285 (c25b125) into master (04b7b4b) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9285      +/-   ##
==========================================
- Coverage   90.19%   89.99%   -0.20%     
==========================================
  Files         151      151              
  Lines      170941   172719    +1778     
==========================================
+ Hits       154175   155440    +1265     
- Misses      16766    17279     +513     
Flag Coverage Δ
huge-clang-none 89.32% <ø> (-0.80%) ⬇️
huge-gcc-none 89.76% <100.00%> (+<0.01%) ⬆️
huge-gcc-testgui ?
huge-gcc-unittests 2.45% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/xxd/xxd.c 86.47% <100.00%> (+0.88%) ⬆️
src/gui_gtk_x11.c 57.89% <0.00%> (-5.65%) ⬇️
src/gui_gtk.c 28.24% <0.00%> (-3.22%) ⬇️
src/highlight.c 87.41% <0.00%> (-2.97%) ⬇️
src/session.c 81.84% <0.00%> (-2.13%) ⬇️
src/buffer.c 90.42% <0.00%> (-1.96%) ⬇️
src/gui.c 69.57% <0.00%> (-1.72%) ⬇️
src/filepath.c 89.04% <0.00%> (-1.63%) ⬇️
src/hardcopy.c 86.15% <0.00%> (-1.62%) ⬇️
src/libvterm/src/vterm.c 66.06% <0.00%> (-1.38%) ⬇️
... and 99 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04b7b4b...c25b125. Read the comment docs.

@brammool
Copy link
Contributor

brammool commented Dec 5, 2021

The xxd argument parsing is very basic, mostly only the first character is checked, thus instead of "-ps" you can also use "-ppp" or just "-p". Fixing one doesn't make much sense. Also, there should be a regression test.

@k-takata k-takata added the xxd for xxd and related label Dec 6, 2021
@dungsaga
Copy link
Contributor Author

@brammool I don't want to change the behavior of the argument parser in general. Its behavior is already documented. I just want to fix a small case when it's different from common convention.
I also added the tests for this fix.

@yegappan yegappan added this to the backlog milestone Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
xxd for xxd and related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants