Re: [PATCH v3] ls-files.c: add --dedup option

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2021, 04:37 UTC
Message-ID: <xmqqzh18mb4p.fsf@gitster.c.googlers.com>
In-Reply-To: <CAOLTT8RQWm-tpkj1aO1rPTCJApP5i+niQtNm_zMycSo5YT0B_w@mail.gmail.com>

胡哲宁 <adlternative@gmail.com> writes:

Show 7 quoted lines

> Here I may disagree with your point of view:
>                  if (errno != E_NOENT)
>                          error_errno("cannot lstat '%s'", fullname.buf);
> With this sentence included, the patch will fail the test:
> t/t3010-ls-files-killed-modified.sh.
> the errno maybe ENOTDIR when you try to lstat a file`r` with `lstat("r/f",&st);`
> So I temporarily removed the judgment of errno.

I didn't mean to give you a solution that is ready to be cut-and-pasted without thinking ;-)

If NOTDIR needs to also be excluded, then you can exclude it just like the above illustration excludes NOENT to solve the issue, right?

Show 11 quoted lines

>>  #2: consolidate two for loops into one.
>> ...
>>      This changes the semantics.  The original iterates the index
>>      twice, so you may see the same entry from --cached once and
>>      then again from --modified.  The updated one still will show
>>      the same entry twice but next to each other.
>>
> Well,This does change the semantics. I think people who used two
> for loops before may want to separate different outputs.
> Now, if you don’t use "--deduplicate", You may see six consecutive
> items under a combination of multiple options.

Yes, and that is intended and is a vast improvement from the current behaviour, which shows 3 in the first loop, bunch of unrelated entries from the rest of the first loop, yet another bunch of unrelated entries from the early part of the second loop and then finally shows 3 from the second loop. With the "single loop" update, at least, the entries are sorted by their path and it would make it easier to see (if the user cares to trigger both --cached and --modified, that is), no?