Hi Usman
On 30/10/2024 16:19, Usman Akinyemi wrote:
On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
On 21/10/2024 13:20, Patrick Steinhardt wrote:
On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
From: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx>
These are a bit curious. As your test demonstrates, we retrieve the
values from the "gitattributes" file. And given that the file tends to be
checked into the repository, you can now basically break somebody elses
commands by having an invalid value in there.
That makes me think that we likely shouldn't die here. We may print a
warning, but other than that we should likely continue and use the
DEFAULT_CONFLICT_MARKER_SIZE.
I think using a warning here is a good idea, we should probably fix the
whitespace attributes to do the same. If you have
* whitespace=indent-with-non-tab,tab-in-indent
in .gitattributes then "git diff" dies with
fatal: cannot enforce both tab-in-indent and indent-with-non-tab
Anyway that's not really related to this series but I thought I'd add it
as #leftoverbits for future reference.
Thanks for working on this Usman, what is queued in next looks good to me.
I just checked it. I will be glad to work on it.
If you want to work on this that's great, but please don't feel any
obligation to do so.
I also noticed that the test used for testing used a different
approach(test_must_fail) compared to the one I wrote which used
test_grep. Should I change the test also ?
I'm not sure which test you are looking at but I assume it is using
test_must_fail because the command being tested is expected to die. If
we change the code to print a warning instead then we'd need to capture
stderr and use test_grep or test_cmp. Note that we only want to print a
warning when parsing .gitattributes, the other callers of
parse_whitespace_rule() still want to die. Also we should decide what
value to use when the user provides both - neither indent-with-non-tab
or tab-in-indent are on by default so it's not clear exactly what we
should do.
Also, when should someone redirect a warning/failure into a file then
use test_grep or just used test_must_fail ?
You must use test_must_fail if you expect a git command to fail, if you
expect the command to print a warning but exit successfully you should
not use test_must_fail. So if you expect a command to fail and print an
error or warning then you'd do
test_must_fail git my failing command 2>err &&
test_grep "error message" err
test_must_fail checks that the command fails, but reports an error if
the command is killed by a signal such as SIGSEV.
Best Wishes
Phillip
Thank you
Usman Akinyemi
Best Wishes
Phillip
Patrick
