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

From: Derrick Stolee <derrickstolee@github.com>
Date: Oct 28, 2022, 15:49 UTC
Message-ID: <313a6619-273c-066e-c3da-a519b38c03af@github.com>
In-Reply-To: <pull.1368.v2.git.git.1666917961644.gitgitgadget@gmail.com>

On 10/27/2022 8:46 PM, Anh Le via GitGitGadget wrote:

Show 10 quoted lines

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

Show 35 quoted lines

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