Re: [PATCH] test_cmp: diagnose incorrect arguments

From: Shourya Shukla <shouryashukla.oo@gmail.com>
Date: Aug 9, 2020, 08:32 UTC
Message-ID: <20200809083227.GA11219@konoha>
In-Reply-To: <20200809060810.31370-1-sunshine@sunshineco.com>

Hello Eric,

Show 10 quoted lines

>  test_cmp() {
> -	eval "$GIT_TEST_CMP" '"$@"'
> +	test $# -eq 2 || BUG "test_cmp requires two arguments"
> +	if ! eval "$GIT_TEST_CMP" '"$@"'
> +	then
> +		test -e "$1" || BUG "test_cmp 'expect' file missing"
> +		test -e "$2" || BUG "test_cmp 'actual' file missing"
> +		return 1
> +	fi
>  }

I reckon we could be just a little bit more precise here by bugging out with the exact filename which is missing instead of 'expect' or 'actual' so that the user has more idea as to what happened. What do you think?

BTW, I looked up the 'test_i18ncmp' function as well and if we have this small loophole you mentioned above, I think maybe we could make a similar fix for it too. What I mean is that in case of absence of the required locale, it should error out kind of like what we did above

    BUG "locale missing"

so that the user it is clear to the user what was the failure point. Though I will be honest that I have not really encountered a locale related error or seen what the error looks like, so maybe we can ignore this suggestion.

Regards, Shourya Shukla