diff mbox series

[v3,3/8,RFC] ls-files: error out on -i unless -o or -c are specified

Message ID 44a1322c44026e675ea254a00f3b50d4955ac56e.1620503945.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Directory traversal fixes | expand

Commit Message

Elijah Newren May 8, 2021, 7:58 p.m. UTC 
From: Elijah Newren <newren@gmail.com>

ls-files --ignored can be used together with either --others or
--cached.  After being perplexed for a bit and digging in to the code, I
assumed that ls-files -i was just broken and not printing anything and
had a nice patch ready to submit when I finally realized that -i can be
used with --cached to find tracked ignores.

While that was a mistake on my part, and a careful reading of the
documentation could have made this more clear, I suspect this is an
error others are likely to make as well.  In fact, of two uses in our
testsuite, I believe one of the two did make this error.  In t1306.13,
there are NO tracked files, and all the excludes built up and used in
that test and in previous tests thus have to be about untracked files.
However, since they were looking for an empty result, the mistake went
unnoticed as their erroneous command also just happened to give an empty
answer.

-i will most the time be used with -o, which would suggest we could just
make -i imply -o in the absence of either a -o or -c, but that would be
a backward incompatible break.  Instead, let's just flag -i without
either a -o or -c as an error, and update the two relevant testcases to
specify their intent.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/ls-files.c          | 3 +++
 t/t1306-xdg-files.sh        | 2 +-
 t/t3003-ls-files-exclude.sh | 4 ++--
 3 files changed, 6 insertions(+), 3 deletions(-)

Comments

Junio C Hamano May 10, 2021, 5:09 a.m. UTC | #1 
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -748,6 +748,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>  	if (pathspec.nr && error_unmatch)
>  		ps_matched = xcalloc(pathspec.nr, 1);
>  
> +	if ((dir.flags & DIR_SHOW_IGNORED) && !show_others && !show_cached)
> +		die("ls-files --ignored is usually used with --others, but --cached is the default.  Please specify which you want.");
> +

So "git ls-files -i" would suddenly start erroring out and users are
to scramble and patch their scripts?

More importantly, the message does not make much sense.  "I is
usually used with O" is very true, but the mention of "usually" here
means it is not an error for "I" to be used without "O".  That part
is very understandable and correct.

But I do not know what "but --cached is the default" part wants to
say.  If it is the _default_, and (assuming that what I read in the
proposed log message is correct) the combination of "-i -c" is valid,
then I would understand the message if the code were more like this:

	if ((dir.flags & DIR_SHOW_IGNORED) &&
	    !show_others && !show_cached) {
		show_cached = 1; /* default */
		warning("ls-files -i given without -o/-c; defaulting to -i -c");
	}

If we are not defaulting to cached, then

	die("ls-files -i must be used with either -o or -c");

would also make sense.

The variant presented in the patch does not make sense to me.

> diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
> index dd87b43be1a6..40d3c42618c0 100755
> --- a/t/t1306-xdg-files.sh
> +++ b/t/t1306-xdg-files.sh
> @@ -116,7 +116,7 @@ test_expect_success 'Exclusion in a non-XDG global ignore file' '
>  test_expect_success 'Checking XDG ignore file when HOME is unset' '
>  	(sane_unset HOME &&
>  	 git config --unset core.excludesfile &&
> -	 git ls-files --exclude-standard --ignored >actual) &&
> +	 git ls-files --exclude-standard --ignored --others >actual) &&
>  	test_must_be_empty actual
>  '
>  
> diff --git a/t/t3003-ls-files-exclude.sh b/t/t3003-ls-files-exclude.sh
> index d5ec333131f9..c41c4f046abf 100755
> --- a/t/t3003-ls-files-exclude.sh
> +++ b/t/t3003-ls-files-exclude.sh
> @@ -29,11 +29,11 @@ test_expect_success 'add file to gitignore' '
>  '
>  check_all_output
>  
> -test_expect_success 'ls-files -i lists only tracked-but-ignored files' '
> +test_expect_success 'ls-files -i -c lists only tracked-but-ignored files' '
>  	echo content >other-file &&
>  	git add other-file &&
>  	echo file >expect &&
> -	git ls-files -i --exclude-standard >output &&
> +	git ls-files -i -c --exclude-standard >output &&
>  	test_cmp expect output
>  '
Elijah Newren May 11, 2021, 5:40 p.m. UTC | #2 
On Sun, May 9, 2021 at 10:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -748,6 +748,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
> >       if (pathspec.nr && error_unmatch)
> >               ps_matched = xcalloc(pathspec.nr, 1);
> >
> > +     if ((dir.flags & DIR_SHOW_IGNORED) && !show_others && !show_cached)
> > +             die("ls-files --ignored is usually used with --others, but --cached is the default.  Please specify which you want.");
> > +
>
> So "git ls-files -i" would suddenly start erroring out and users are
> to scramble and patch their scripts?

Thus the reason I marked this as "RFC" and called it out in the cover
letter for folks to comment on.

I figured that if I was having difficulty using it correctly and even
our own testsuite showed that 50% of such invocations were wrong
(despite being reviewed[1]), then it seems likely to me that erroring
out to inform folks of this problem might be warranted.  But, if folks
disagree, I can switch it to a warning instead.

[1] https://lore.kernel.org/git/20120724133227.GA14422@sigill.intra.peff.net/#t

> More importantly, the message does not make much sense.  "I is
> usually used with O" is very true, but the mention of "usually" here
> means it is not an error for "I" to be used without "O".  That part
> is very understandable and correct.
>
> But I do not know what "but --cached is the default" part wants to
> say.  If it is the _default_, and (assuming that what I read in the
> proposed log message is correct) the combination of "-i -c" is valid,
> then I would understand the message if the code were more like this:
>
>         if ((dir.flags & DIR_SHOW_IGNORED) &&
>             !show_others && !show_cached) {
>                 show_cached = 1; /* default */
>                 warning("ls-files -i given without -o/-c; defaulting to -i -c");
>         }
>
> If we are not defaulting to cached, then
>
>         die("ls-files -i must be used with either -o or -c");
>
> would also make sense.

Ooh, that wording is much nicer.  I'll adopt the latter suggestion,
but let me know if you'd rather I went the warning route.
Junio C Hamano May 11, 2021, 10:32 p.m. UTC | #3 
Elijah Newren <newren@gmail.com> writes:

>> If we are not defaulting to cached, then
>>
>>         die("ls-files -i must be used with either -o or -c");
>>
>> would also make sense.
>
> Ooh, that wording is much nicer.  I'll adopt the latter suggestion,
> but let me know if you'd rather I went the warning route.

Even though warning would be safer, I have no strong prefeference.
Either way will resolve my puzzlement.

Thanks.
diff mbox series

Patch

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 60a2913a01e9..9f74b1ab2e69 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -748,6 +748,9 @@  int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (pathspec.nr && error_unmatch)
 		ps_matched = xcalloc(pathspec.nr, 1);
 
+	if ((dir.flags & DIR_SHOW_IGNORED) && !show_others && !show_cached)
+		die("ls-files --ignored is usually used with --others, but --cached is the default.  Please specify which you want.");
+
 	if ((dir.flags & DIR_SHOW_IGNORED) && !exc_given)
 		die("ls-files --ignored needs some exclude pattern");
 
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index dd87b43be1a6..40d3c42618c0 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -116,7 +116,7 @@  test_expect_success 'Exclusion in a non-XDG global ignore file' '
 test_expect_success 'Checking XDG ignore file when HOME is unset' '
 	(sane_unset HOME &&
 	 git config --unset core.excludesfile &&
-	 git ls-files --exclude-standard --ignored >actual) &&
+	 git ls-files --exclude-standard --ignored --others >actual) &&
 	test_must_be_empty actual
 '
 
diff --git a/t/t3003-ls-files-exclude.sh b/t/t3003-ls-files-exclude.sh
index d5ec333131f9..c41c4f046abf 100755
--- a/t/t3003-ls-files-exclude.sh
+++ b/t/t3003-ls-files-exclude.sh
@@ -29,11 +29,11 @@  test_expect_success 'add file to gitignore' '
 '
 check_all_output
 
-test_expect_success 'ls-files -i lists only tracked-but-ignored files' '
+test_expect_success 'ls-files -i -c lists only tracked-but-ignored files' '
 	echo content >other-file &&
 	git add other-file &&
 	echo file >expect &&
-	git ls-files -i --exclude-standard >output &&
+	git ls-files -i -c --exclude-standard >output &&
 	test_cmp expect output
 '