idiff: allow users to specify a directory as the 2nd argument #4015
Conversation
The Unix "diff" utility allows you to run:
diff path/to/file1.txt path/to/dir
When "diff" sees a directory it appends "file1.txt" to the
directory so that this command is treated as if the user ran:
diff path/to/file1.txt path/to/dir/file1.txt
Teach "idiff" to detect when a directory is specified and expand
the path to include the file basename from the first argument.
Closes: AcademySoftwareFoundation#4009
Signed-off-by: David Aguilar <davvid@gmail.com>
Let users know that the 2nd argument to "idiff" can be either an image filename or a directory. Related-to: AcademySoftwareFoundation#4009 Helped-by: @CheeksTheGeek Signed-off-by: David Aguilar <davvid@gmail.com>
|
@CheeksTheGeek here's my take on the idiff feature. I didn't originally update the usage/help string, so I took inspiration from CheeksTheGeek@c7522b7 and adjusted this PR to do something similar. I mentioned you in the commit message for that tweak. One notable difference between this commit and the one linked above is that this is checking for |
| second += Filesystem::filename(first); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The Filesystem class didn't already provide this functionality so adding this to idiff seemed like the right place to keep this for now. If other code ends up needing to build paths then moving this to the Filesystem class might be worth it at that point in time. It didn't seem worth extending its public API just for this one small tweak, though.
There was a problem hiding this comment.
Also, I believe Windows will happily accept / as a path separator so I'm not sure if the extra #if defined(_MSC_VER) complexity is worth it. Backslashes are idiomatic on Windows which is why I included it.
davvid
changed the title
|
The failed check looks to be configuration or a CI glitch: It was just that one permutation that hit this error. |
|
Don't worry about the icc/icx CI failures. That's a problem on the Intel side, they updated their signature key but didn't re-sign some of the older versions they had on that repo. It's on them to fix, I think. We will accept PRs that pass all the other CI tests. |
|
Fixes #4009 |
|
BTW, I built this on my end and tested, works! This is cool, now my "diff" muscle memory will work with "idiff" as well. |
|
I think the commits themselves have I'll also take a stab at updating |
Use "input1" and "input2" in the usage example to match the description immediately below it. Add a sentence explaining that "input2" can also be a directory. Related-to: AcademySoftwareFoundation#4009 Signed-off-by: David Aguilar <davvid@gmail.com>
|
I pushed in a new commit that updates |
lgritz
merged commit 0d2e990
into
AcademySoftwareFoundation:master
…cademySoftwareFoundation#4015) Teach `idiff` to accept a directory as the 2nd argument. Fixes AcademySoftwareFoundation#4009 ## Tests Functional tests for `idiff` don't currently exist. Manual testing verifies it works as advertised. --------- Signed-off-by: David Aguilar <davvid@gmail.com>
Description
Teach
idiffto accept a directory as the 2nd argument.Fixes #4009
Tests
Functional tests for
idiffdon't currently exist. Manual testing verifies it works as advertised.Checklist:
(adding new test cases if necessary). (N/A)
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options). (N/A)
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.