Re: [PATCH v2] index: add trace2 region for clear skip worktree

[Derrick Stolee <derrickstolee@github.com>]

On 10/27/2022 8:46 PM, Anh Le via GitGitGadget wrote:
> From: Anh Le <anh@canva.com>
> 
> In a large repository using sparse checkout, checking
> whether a file marked with skip worktree is present
> on disk and its skip worktree bit should be cleared
> can take a considerable amount of time. Add a trace2
> region to surface this information, keeping a count of how many paths
> have been checked and separately
> keep counts for after a full index is
> materialised.
You have some strange line wrapping here. You can wrap them better
using several editors, but here is brief technique if your core.editor
is vim (do within 'git commit --amend'):

1. Use "SHIFT+V" to get into visual mode.
2. Use up and down arrows to select the entire paragraph.
3. Type "G" then "Q" to word wrap the selection.
4. Save and close the editor.
>  	int i;
> +	int path_counts[2] = {0, 0};
> +	int restarted = 0;

This count mechanism is a good one. Nice and simple.

>  
>  	if (!core_apply_sparse_checkout ||
>  	    sparse_expect_files_outside_of_patterns)
>  		return;
>  
> +	trace2_region_enter("index", "clear_skip_worktree_from_present_files", istate->repo);
>  restart:
>  	for (i = 0; i < istate->cache_nr; i++) {
>  		struct cache_entry *ce = istate->cache[i];
>  
> -		if (ce_skip_worktree(ce) &&
> -		    path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {
> -			if (S_ISSPARSEDIR(ce->ce_mode)) {
> -				ensure_full_index(istate);
> -				goto restart;
> +		if (ce_skip_worktree(ce)) {
> +			path_counts[restarted]++;
> +			if (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {
> +				if (S_ISSPARSEDIR(ce->ce_mode)) {
> +					ensure_full_index(istate);
> +					restarted = 1;
> +					goto restart;
> +				}
> +				ce->ce_flags &= ~CE_SKIP_WORKTREE;
>  			}
> -			ce->ce_flags &= ~CE_SKIP_WORKTREE;
>  		}
>  	}
> +
> +	if (path_counts[0] > 0) {
> +		trace2_data_intmax("index", istate->repo, "clear_skip_worktree_from_present_files/path_count", path_counts[0]);
> +	}
> +	if (restarted) {
> +		trace2_data_intmax("index", istate->repo, "clear_skip_worktree_from_present_files/full_index/path_count", path_counts[1]);
> +	}

A few style things:

1. Use "if (path_counts[0])" to detect a non-zero value.
2. Don't use { } around a single-line block.
3. Your lines are quite long, due a lot from your long keys.
   Shorten the keys (maybe "sparse_path_count" and "restarted_count"
   is enough context) and consider using a line break in
   the middle of the parameters.

> +	trace2_region_leave("index", "clear_skip_worktree_from_present_files", istate->repo);

Thanks,
-Stolee