Re: [PATCH v6] ls-files.c: add --deduplicate option

>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> > In order to provide users a better experience
> > when viewing information about files in the index
> > and the working tree, the `--deduplicate` option will suppress
> > some duplicate name under some conditions.
>
> Now is it just a single patch squashing everything together?
> That does not look like it.
>
> > @@ -317,7 +318,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
> >       for (i = 0; i < repo->index->cache_nr; i++) {
> >               const struct cache_entry *ce = repo->index->cache[i];
> >               struct stat st;
> > -             int err;
> > +             int stat_err;
> >
> >               construct_fullname(&fullname, repo, ce);
> >
> > @@ -326,25 +327,43 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
> >                       continue;
> >               if (ce->ce_flags & CE_UPDATE)
> >                       continue;
> > -             if (show_cached || show_stage) {
> > -                     if (!show_unmerged || ce_stage(ce))
> > +             if ((show_cached || show_stage) &&
> > +                     (!show_unmerged || ce_stage(ce))) {
> >                               show_ce(repo, dir, ce, fullname.buf,
> >                                       ce_stage(ce) ? tag_unmerged :
> >                                       (ce_skip_worktree(ce) ? tag_skip_worktree :
> >                                               tag_cached));
> > +                     if (show_cached && skipping_duplicates)
> > +                             goto skip_to_next_name;
>
> Why should this be so complex?  You are dropping skipping_duplicates
> when the output is not name-only, so shouldn't this look more like
>

>                 if ((show_cached || show_stage) &&
>                     (!show_unmerged || ce_stage(ce)) {
>                         show_ce(...);
>                         if (skipping_duplicates)
>                                 goto skip_to_next_name;
>                 }
>
> It seems that this still depends on the 2/3 from the previous
> iteration, against which I suggested to merge the conditions of
> nested if statements into one.  That should be done in the updated
> 2/3, not in this step, no?
>
> >               }
> > +             if (!show_deleted && !show_modified)
> > +                     continue;
>
> And this one also belongs to the step 2/3 that consolidates the two
> loops into one.
>
> I think you'd need to start from the three patches in v5, "rebase -i"
> not just [3/3] but at least [2/3], too.
>
> Thanks.