git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 01/14] progress: stop using `the_repository`

From
Toon Claes <toon@iotcl.com>
Date
Jan 6, 2025, 20:57 UTC
Message-ID
<87v7urk6dl.fsf@iotcl.com>
In-Reply-To
<20241217-pks-use-the-repository-conversion-v1-1-0dba48bcc239@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 9 quoted lines
> Stop using `the_repository` in the "progress" subsystem by passing in a
> repository when initializing `struct progress`. Furthermore, store a
> pointer to the repository in that struct so that we can pass it to the
> trace2 API when logging information.
>
> Adjust callers accordingly by using `the_repository`. While there may be
> some callers that have a repository available in their context, this
> trivial conversion allows for easier verification and bubbles up the use
> of `the_repository` by one level.

I'm not sure I agree here. Below I've marked all places where I think we are able to get the repo from somewhere else than `the_repository`. For example, looking at diffcore-rename.c, the already present calls to trace2_*() use `options->repo`, why shouldn't we do the same?

I understand what your angle is, you want to bubble up `the_repository` and make the changes easier to reason about, but it feels to me we're creating extra work. If most people disagree with me, I'm happy to take your approach.

Show 64 quoted lines
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/blame.c          |  4 +++-
>  builtin/commit-graph.c   |  1 +
>  builtin/fsck.c           | 12 ++++++++----
>  builtin/index-pack.c     |  7 +++++--
>  builtin/log.c            |  3 ++-
>  builtin/pack-objects.c   | 21 ++++++++++++++-------
>  builtin/prune.c          |  3 ++-
>  builtin/remote.c         |  3 ++-
>  builtin/rev-list.c       |  3 ++-
>  builtin/unpack-objects.c |  3 ++-
>  commit-graph.c           | 20 +++++++++++++++++---
>  delta-islands.c          |  3 ++-
>  diffcore-rename.c        |  1 +
>  entry.c                  |  4 +++-
>  midx-write.c             | 11 ++++++++---
>  midx.c                   | 13 +++++++++----
>  pack-bitmap-write.c      |  6 ++++--
>  pack-bitmap.c            |  4 +++-
>  preload-index.c          |  4 +++-
>  progress.c               | 34 ++++++++++++++++++++--------------
>  progress.h               | 13 +++++++++----
>  prune-packed.c           |  3 ++-
>  pseudo-merge.c           |  3 ++-
>  read-cache.c             |  3 ++-
>  t/helper/test-progress.c |  6 +++++-
>  unpack-trees.c           |  4 +++-
>  walker.c                 |  3 ++-
>  27 files changed, 136 insertions(+), 59 deletions(-)
>
> [snip]
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 0196c54eb68ee54c22de72d64b3f31602594e50b..7a4dcb0716052ff1b9236ea66b8901960fe1c55d 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -197,7 +197,8 @@ static int traverse_reachable(void)
>  	unsigned int nr = 0;
>  	int result = 0;
>  	if (show_progress)
> -		progress = start_delayed_progress(_("Checking connectivity"), 0);
> +		progress = start_delayed_progress(the_repository,
> +						  _("Checking connectivity"), 0);
>  	while (pending.nr) {
>  		result |= traverse_one_object(object_array_pop(&pending));
>  		display_progress(progress, ++nr);
> @@ -703,7 +704,8 @@ static void fsck_object_dir(const char *path)
>  		fprintf_ln(stderr, _("Checking object directory"));
>  
>  	if (show_progress)
> -		progress = start_progress(_("Checking object directories"), 256);
> +		progress = start_progress(the_repository,
> +					  _("Checking object directories"), 256);
>  
>  	for_each_loose_file_in_objdir(path, fsck_loose, fsck_cruft, fsck_subdir,
>  				      &cb_data);
> @@ -879,7 +881,8 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
>  	if (show_progress) {
>  		for (struct packed_git *p = get_all_packs(r); p; p = p->next)
>  			pack_count++;
> -		progress = start_delayed_progress("Verifying reverse pack-indexes", pack_count);
> +		progress = start_delayed_progress(the_repository,
I think we should use `r` here.
Show 14 quoted lines
> +						  "Verifying reverse pack-indexes", pack_count);
>  		pack_count = 0;
>  	}
>
> [snip]
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 0df66e5a243390bc1224b28e2b55c541f9d93fb1..2a2999a6b886905276a0c39dda6135f0c92aa361 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1534,6 +1534,7 @@ static void close_reachable(struct write_commit_graph_context *ctx)
>  
>  	if (ctx->report_progress)
>  		ctx->progress = start_delayed_progress(
Shall we use `ctx->r` here? And same for all other changes in this file.
Show 124 quoted lines
> +					the_repository,
>  					_("Loading known commits in commit graph"),
>  					ctx->oids.nr);
>  	for (i = 0; i < ctx->oids.nr; i++) {
> @@ -1551,6 +1552,7 @@ static void close_reachable(struct write_commit_graph_context *ctx)
>  	 */
>  	if (ctx->report_progress)
>  		ctx->progress = start_delayed_progress(
> +					the_repository,
>  					_("Expanding reachable commits in commit graph"),
>  					0);
>  	for (i = 0; i < ctx->oids.nr; i++) {
> @@ -1571,6 +1573,7 @@ static void close_reachable(struct write_commit_graph_context *ctx)
>  
>  	if (ctx->report_progress)
>  		ctx->progress = start_delayed_progress(
> +					the_repository,
>  					_("Clearing commit marks in commit graph"),
>  					ctx->oids.nr);
>  	for (i = 0; i < ctx->oids.nr; i++) {
> @@ -1688,6 +1691,7 @@ static void compute_topological_levels(struct write_commit_graph_context *ctx)
>  	if (ctx->report_progress)
>  		info.progress = ctx->progress
>  			      = start_delayed_progress(
> +					the_repository,
>  					_("Computing commit graph topological levels"),
>  					ctx->commits.nr);
>  
> @@ -1722,6 +1726,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
>  	if (ctx->report_progress)
>  		info.progress = ctx->progress
>  			      = start_delayed_progress(
> +					the_repository,
>  					_("Computing commit graph generation numbers"),
>  					ctx->commits.nr);
>  
> @@ -1798,6 +1803,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
>  
>  	if (ctx->report_progress)
>  		progress = start_delayed_progress(
> +			the_repository,
>  			_("Computing commit changed paths Bloom filters"),
>  			ctx->commits.nr);
>  
> @@ -1877,6 +1883,7 @@ int write_commit_graph_reachable(struct object_directory *odb,
>  	data.commits = &commits;
>  	if (flags & COMMIT_GRAPH_WRITE_PROGRESS)
>  		data.progress = start_delayed_progress(
> +			the_repository,
>  			_("Collecting referenced commits"), 0);
>  
>  	refs_for_each_ref(get_main_ref_store(the_repository), add_ref_to_set,
> @@ -1908,7 +1915,8 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
>  			       "Finding commits for commit graph in %"PRIuMAX" packs",
>  			       pack_indexes->nr),
>  			    (uintmax_t)pack_indexes->nr);
> -		ctx->progress = start_delayed_progress(progress_title.buf, 0);
> +		ctx->progress = start_delayed_progress(the_repository,
> +						       progress_title.buf, 0);
>  		ctx->progress_done = 0;
>  	}
>  	for (i = 0; i < pack_indexes->nr; i++) {
> @@ -1959,6 +1967,7 @@ static void fill_oids_from_all_packs(struct write_commit_graph_context *ctx)
>  {
>  	if (ctx->report_progress)
>  		ctx->progress = start_delayed_progress(
> +			the_repository,
>  			_("Finding commits for commit graph among packed objects"),
>  			ctx->approx_nr_objects);
>  	for_each_packed_object(ctx->r, add_packed_commits, ctx,
> @@ -1977,6 +1986,7 @@ static void copy_oids_to_commits(struct write_commit_graph_context *ctx)
>  	ctx->num_extra_edges = 0;
>  	if (ctx->report_progress)
>  		ctx->progress = start_delayed_progress(
> +			the_repository,
>  			_("Finding extra edges in commit graph"),
>  			ctx->oids.nr);
>  	oid_array_sort(&ctx->oids);
> @@ -2136,6 +2146,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>  			       get_num_chunks(cf)),
>  			    get_num_chunks(cf));
>  		ctx->progress = start_delayed_progress(
> +			the_repository,
>  			progress_title.buf,
>  			st_mult(get_num_chunks(cf), ctx->commits.nr));
>  	}
> @@ -2348,6 +2359,7 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)
>  
>  	if (ctx->report_progress)
>  		ctx->progress = start_delayed_progress(
> +					the_repository,
>  					_("Scanning merged commits"),
>  					ctx->commits.nr);
>  
> @@ -2392,7 +2404,8 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)
>  		current_graph_number--;
>  
>  		if (ctx->report_progress)
> -			ctx->progress = start_delayed_progress(_("Merging commit-graph"), 0);
> +			ctx->progress = start_delayed_progress(the_repository,
> +							       _("Merging commit-graph"), 0);
>  
>  		merge_commit_graph(ctx, g);
>  		stop_progress(&ctx->progress);
> @@ -2874,7 +2887,8 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
>  		if (!(flags & COMMIT_GRAPH_VERIFY_SHALLOW))
>  			total += g->num_commits_in_base;
>  
> -		progress = start_progress(_("Verifying commits in commit graph"),
> +		progress = start_progress(the_repository,
> +					  _("Verifying commits in commit graph"),
>  					  total);
>  	}
>  
> diff --git a/delta-islands.c b/delta-islands.c
> index 1c465a6041914538bcb8be51c500653d8fa1a626..3aec43fada36f7052b825dcb2ac0e1ad79f028b7 100644
> --- a/delta-islands.c
> +++ b/delta-islands.c
> @@ -267,7 +267,8 @@ void resolve_tree_islands(struct repository *r,
>  	QSORT(todo, nr, tree_depth_compare);
>  
>  	if (progress)
> -		progress_state = start_progress(_("Propagating island marks"), nr);
> +		progress_state = start_progress(the_repository,
Here we can use `r`.
Show 13 quoted lines
> +						_("Propagating island marks"), nr);
>  
>  	for (i = 0; i < nr; i++) {
>  		struct object_entry *ent = todo[i].entry;
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 10bb0321b10d5896aaa6a26a624d2066598bf51f..91b77993c7827f9ddc7b444b42f480b8209fd821 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -1567,6 +1567,7 @@ void diffcore_rename_extended(struct diff_options *options,
>  	trace2_region_enter("diff", "inexact renames", options->repo);
>  	if (options->show_rename_progress) {
>  		progress = start_delayed_progress(
> +				the_repository,
Can we use `options->repo` here?

If we do, we ideally should also replace occurrences of `the_repository->hash_algo` in this function, although I realize that's outside the scope of this commit.

Show 16 quoted lines
>  				_("Performing inexact rename detection"),
>  				(uint64_t)num_destinations * (uint64_t)num_sources);
>  	}
> 
> [snip]
>
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 4f8be53c2bd75f83a0555e2a5510c2bbca07b36d..a06a1f35c619b3b01e63a506a6d4312e14cf181c 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -590,7 +590,8 @@ int bitmap_writer_build(struct bitmap_writer *writer)
>  	int closed = 1; /* until proven otherwise */
>  
>  	if (writer->show_progress)
> -		writer->progress = start_progress("Building bitmaps",
> +		writer->progress = start_progress(the_repository,

Unsure, but can we use `writer->to_pack->repo`? Although I see some trace2_* functions also use `the_repository`, so consistency in this function would be nice.

Show 10 quoted lines
> +						  "Building bitmaps",
>  						  writer->selected_nr);
>  	trace2_region_enter("pack-bitmap-write", "building_bitmaps_total",
>  			    the_repository);
> @@ -710,7 +711,8 @@ void bitmap_writer_select_commits(struct bitmap_writer *writer,
>  	}
>  
>  	if (writer->show_progress)
> -		writer->progress = start_progress("Selecting bitmap commits", 0);
> +		writer->progress = start_progress(the_repository,
Same, can we use `writer->to_pack->repo`?
Show 17 quoted lines
> +						  "Selecting bitmap commits", 0);
>  
>  	for (;;) {
>  		struct commit *chosen = NULL;
>
> [snip]
>
> diff --git a/preload-index.c b/preload-index.c
> index ab94d6f39967ea4358f51ff8384aa60927bfe259..40ab2abafb8de500a5f2ec678a584a5fd5e1bc16 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -132,7 +132,9 @@ void preload_index(struct index_state *index,
>  
>  	memset(&pd, 0, sizeof(pd));
>  	if (refresh_flags & REFRESH_PROGRESS && isatty(2)) {
> -		pd.progress = start_delayed_progress(_("Refreshing index"), index->cache_nr);
> +		pd.progress = start_delayed_progress(the_repository,
Can we use `index->repo` here?
Show 12 quoted lines
> [snip]
>
> diff --git a/pseudo-merge.c b/pseudo-merge.c
> index 971f54cfe1a895aed00f6d0a65c62aafc83a0cc8..893b763fe45490875ea226eaffff0c7cb1dafb06 100644
> --- a/pseudo-merge.c
> +++ b/pseudo-merge.c
> @@ -459,7 +459,8 @@ void select_pseudo_merges(struct bitmap_writer *writer)
>  		return;
>  
>  	if (writer->show_progress)
> -		progress = start_progress("Selecting pseudo-merge commits",
> +		progress = start_progress(the_repository,
Also a candidate for `writer->to_pack->repo`?
Show 14 quoted lines
> +					  "Selecting pseudo-merge commits",
>  					  writer->pseudo_merge_groups.nr);
>  
>  	refs_for_each_ref(get_main_ref_store(the_repository),
> diff --git a/read-cache.c b/read-cache.c
> index 15d79839c205176f9161f537aa706dac44b3023c..38c36caa7fef4d44da74c29e059839d88426df15 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1523,7 +1523,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,
>  	int t2_sum_scan = 0;
>  
>  	if (flags & REFRESH_PROGRESS && isatty(2))
> -		progress = start_delayed_progress(_("Refresh index"),
> +		progress = start_delayed_progress(the_repository,
I think also `istate->repo` would work here.
Show 17 quoted lines
> +						  _("Refresh index"),
>  						  istate->cache_nr);
>  
>  	trace_performance_enter();
>
> [snip]
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index b3be5d542f5fc5a02b8838101f7334ff44b2c626..334cb84f6531b588688d5a43c538c8d1a5f7e768 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -372,7 +372,8 @@ static struct progress *get_progress(struct unpack_trees_options *o,
>  			total++;
>  	}
>  
> -	return start_delayed_progress(_("Updating files"), total);
> +	return start_delayed_progress(the_repository,
Maybe also use `index->repo` here?
Show 29 quoted lines
> +				      _("Updating files"), total);
>  }
>  
>  static void setup_collided_checkout_detection(struct checkout *state,
> @@ -1773,6 +1774,7 @@ static int clear_ce_flags(struct index_state *istate,
>  	strbuf_reset(&prefix);
>  	if (show_progress)
>  		istate->progress = start_delayed_progress(
> +					the_repository,
>  					_("Updating index flags"),
>  					istate->cache_nr);
>  
> diff --git a/walker.c b/walker.c
> index 7cc9dbea46d64d6bd3336025d640f284a6202157..1cf3da02193531a17fd11dbd2e8aadf36f38b200 100644
> --- a/walker.c
> +++ b/walker.c
> @@ -172,7 +172,8 @@ static int loop(struct walker *walker)
>  	uint64_t nr = 0;
>  
>  	if (walker->get_progress)
> -		progress = start_delayed_progress(_("Fetching objects"), 0);
> +		progress = start_delayed_progress(the_repository,
> +						  _("Fetching objects"), 0);
>  
>  	while (process_queue) {
>  		struct object *obj = process_queue->item;
>
> -- 
> 2.48.0.rc0.184.g0fc57dec57.dirty
Previous: Karthik NayakNext: Patrick Steinhardt
Message 4 of 26 in “Stop using `the_repository` in some trivial cases”
  1. 00/14 Stop using `the_repository` in some trivial casesPatrick Steinhardt, Dec 17, 2024
  2. 01/14 progress: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  3. Karthik NayakDec 31, 2024
  4. Toon ClaesJan 6, 2025
  5. Patrick SteinhardtJan 7, 2025
  6. 02/14 pager: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  7. shejialuoDec 17, 2024
  8. Karthik NayakDec 31, 2024
  9. 03/14 trace: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  10. 04/14 serve: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  11. 05/14 send-pack: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  12. 06/14 server-info: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  13. shejialuoDec 17, 2024
  14. 07/14 diagnose: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  15. 08/14 mailinfo: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  16. 09/14 credential: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  17. 10/14 resolve-undo: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  18. 11/14 tmp-objdir: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  19. 12/14 add-interactive: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  20. 13/14 graph: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  21. 14/14 match-trees: stop using `the_repository`Patrick Steinhardt, Dec 17, 2024
  22. shejialuoDec 17, 2024
  23. Patrick SteinhardtDec 27, 2024
  24. Karthik NayakJan 7, 2025
  25. Toon ClaesJan 7, 2025
  26. Junio C HamanoJan 7, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.