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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2021, 20:52 UTC
Message-ID: <xmqqr1mc7kyt.fsf@gitster.c.googlers.com>
In-Reply-To: <20210122154640.3791035-1-adlternative@gmail.com>

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.

Show 23 quoted lines

> @@ -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.