From 41f43b8243f42b9df2e98be8460646d4c0100ad3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 6 Dec 2024 11:27:19 +0100 Subject: global: mark code units that generate warnings with `-Wsign-compare` Mark code units that generate warnings with `-Wsign-compare`. This allows for a structured approach to get rid of all such warnings over time in a way that can be easily measured. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/unpack-objects.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'builtin/unpack-objects.c') diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 02b8d02f63..2197d6d933 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -1,4 +1,6 @@ #define USE_THE_REPOSITORY_VARIABLE +#define DISABLE_SIGN_COMPARE_WARNINGS + #include "builtin.h" #include "bulk-checkin.h" #include "config.h" -- cgit v1.3 From 1f7e6478dcd9e7462c70a5784ae0d41ab25ced11 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 17 Dec 2024 07:43:48 +0100 Subject: progress: stop using `the_repository` 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. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- 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(-) (limited to 'builtin/unpack-objects.c') diff --git a/builtin/blame.c b/builtin/blame.c index 867032e4c1..dd78288530 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1193,7 +1193,9 @@ parse_done: sb.found_guilty_entry = &found_guilty_entry; sb.found_guilty_entry_data = π if (show_progress) - pi.progress = start_delayed_progress(_("Blaming lines"), num_lines); + pi.progress = start_delayed_progress(the_repository, + _("Blaming lines"), + num_lines); assign_blame(&sb, opt); diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index bd70d052e7..8ca75262c5 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -305,6 +305,7 @@ static int graph_write(int argc, const char **argv, const char *prefix, oidset_init(&commits, 0); if (opts.progress) progress = start_delayed_progress( + the_repository, _("Collecting commits from input"), 0); while (strbuf_getline(&buf, stdin) != EOF) { diff --git a/builtin/fsck.c b/builtin/fsck.c index 0196c54eb6..7a4dcb0716 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, + "Verifying reverse pack-indexes", pack_count); pack_count = 0; } @@ -989,7 +992,8 @@ int cmd_fsck(int argc, total += p->num_objects; } - progress = start_progress(_("Checking objects"), total); + progress = start_progress(the_repository, + _("Checking objects"), total); } for (p = get_all_packs(the_repository); p; p = p->next) { diff --git a/builtin/index-pack.c b/builtin/index-pack.c index d773809c4c..05691104c3 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -282,7 +282,8 @@ static unsigned check_objects(void) max = get_max_object_index(); if (verbose) - progress = start_delayed_progress(_("Checking objects"), max); + progress = start_delayed_progress(the_repository, + _("Checking objects"), max); for (i = 0; i < max; i++) { foreign_nr += check_object(get_indexed_object(i)); @@ -1249,6 +1250,7 @@ static void parse_pack_objects(unsigned char *hash) if (verbose) progress = start_progress( + the_repository, progress_title ? progress_title : from_stdin ? _("Receiving objects") : _("Indexing objects"), nr_objects); @@ -1329,7 +1331,8 @@ static void resolve_deltas(struct pack_idx_option *opts) QSORT(ref_deltas, nr_ref_deltas, compare_ref_delta_entry); if (verbose || show_resolving_progress) - progress = start_progress(_("Resolving deltas"), + progress = start_progress(the_repository, + _("Resolving deltas"), nr_ref_deltas + nr_ofs_deltas); nr_dispatched = 0; diff --git a/builtin/log.c b/builtin/log.c index cb41a035c6..317335e60d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -2495,7 +2495,8 @@ int cmd_format_patch(int argc, rev.add_signoff = cfg.do_signoff; if (show_progress) - progress = start_delayed_progress(_("Generating patches"), total); + progress = start_delayed_progress(the_repository, + _("Generating patches"), total); while (0 <= --nr) { int shown; display_progress(progress, total - nr); diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 1c3b842651..d51c021d99 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1264,7 +1264,8 @@ static void write_pack_file(void) struct object_entry **write_order; if (progress > pack_to_stdout) - progress_state = start_progress(_("Writing objects"), nr_result); + progress_state = start_progress(the_repository, + _("Writing objects"), nr_result); ALLOC_ARRAY(written_list, to_pack.nr_objects); write_order = compute_write_order(); @@ -2400,7 +2401,8 @@ static void get_object_details(void) struct object_entry **sorted_by_offset; if (progress) - progress_state = start_progress(_("Counting objects"), + progress_state = start_progress(the_repository, + _("Counting objects"), to_pack.nr_objects); CALLOC_ARRAY(sorted_by_offset, to_pack.nr_objects); @@ -3220,7 +3222,8 @@ static void prepare_pack(int window, int depth) unsigned nr_done = 0; if (progress) - progress_state = start_progress(_("Compressing objects"), + progress_state = start_progress(the_repository, + _("Compressing objects"), nr_deltas); QSORT(delta_list, n, type_size_sort); ll_find_deltas(delta_list, n, window+1, depth, &nr_done); @@ -3648,7 +3651,8 @@ static void add_objects_in_unpacked_packs(void); static void enumerate_cruft_objects(void) { if (progress) - progress_state = start_progress(_("Enumerating cruft objects"), 0); + progress_state = start_progress(the_repository, + _("Enumerating cruft objects"), 0); add_objects_in_unpacked_packs(); add_unreachable_loose_objects(); @@ -3674,7 +3678,8 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs revs.ignore_missing_links = 1; if (progress) - progress_state = start_progress(_("Enumerating cruft objects"), 0); + progress_state = start_progress(the_repository, + _("Enumerating cruft objects"), 0); ret = add_unseen_recent_objects_to_traversal(&revs, cruft_expiration, set_cruft_mtime, 1); stop_progress(&progress_state); @@ -3693,7 +3698,8 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs if (prepare_revision_walk(&revs)) die(_("revision walk setup failed")); if (progress) - progress_state = start_progress(_("Traversing cruft objects"), 0); + progress_state = start_progress(the_repository, + _("Traversing cruft objects"), 0); nr_seen = 0; traverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL); @@ -4625,7 +4631,8 @@ int cmd_pack_objects(int argc, prepare_packing_data(the_repository, &to_pack); if (progress && !cruft) - progress_state = start_progress(_("Enumerating objects"), 0); + progress_state = start_progress(the_repository, + _("Enumerating objects"), 0); if (stdin_packs) { /* avoids adding objects in excluded packs */ ignore_packed_keep_in_core = 1; diff --git a/builtin/prune.c b/builtin/prune.c index aeff9ca1b3..1c357fffd8 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -64,7 +64,8 @@ static void perform_reachability_traversal(struct rev_info *revs) return; if (show_progress) - progress = start_delayed_progress(_("Checking connectivity"), 0); + progress = start_delayed_progress(the_repository, + _("Checking connectivity"), 0); mark_reachable_objects(revs, 1, expire, progress); stop_progress(&progress); initialized = 1; diff --git a/builtin/remote.c b/builtin/remote.c index b2b13a7dd2..95440bc9ff 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -820,7 +820,8 @@ static int mv(int argc, const char **argv, const char *prefix, * Count symrefs twice, since "renaming" them is done by * deleting and recreating them in two separate passes. */ - progress = start_progress(_("Renaming remote references"), + progress = start_progress(the_repository, + _("Renaming remote references"), rename.remote_branches->nr + rename.symrefs_nr); } for (i = 0; i < remote_branches.nr; i++) { diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 3196da7b2d..8a7db9b546 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -735,7 +735,8 @@ int cmd_rev_list(int argc, revs.limited = 1; if (show_progress) - progress = start_delayed_progress(show_progress, 0); + progress = start_delayed_progress(the_repository, + show_progress, 0); if (use_bitmap_index) { if (!try_bitmap_count(&revs, filter_provided_objects)) diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 2197d6d933..842a90353a 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -590,7 +590,8 @@ static void unpack_all(void) use(sizeof(struct pack_header)); if (!quiet) - progress = start_progress(_("Unpacking objects"), nr_objects); + progress = start_progress(the_repository, + _("Unpacking objects"), nr_objects); CALLOC_ARRAY(obj_list, nr_objects); begin_odb_transaction(); for (i = 0; i < nr_objects; i++) { diff --git a/commit-graph.c b/commit-graph.c index 0df66e5a24..2a2999a6b8 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( + 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 1c465a6041..3aec43fada 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, + _("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 10bb0321b1..91b77993c7 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, _("Performing inexact rename detection"), (uint64_t)num_destinations * (uint64_t)num_sources); } diff --git a/entry.c b/entry.c index 53a1c39358..81b321e53d 100644 --- a/entry.c +++ b/entry.c @@ -188,7 +188,9 @@ int finish_delayed_checkout(struct checkout *state, int show_progress) dco->state = CE_RETRY; if (show_progress) - progress = start_delayed_progress(_("Filtering content"), dco->paths.nr); + progress = start_delayed_progress(the_repository, + _("Filtering content"), + dco->paths.nr); while (dco->filters.nr > 0) { for_each_string_list_item(filter, &dco->filters) { struct string_list available_paths = STRING_LIST_INIT_DUP; diff --git a/midx-write.c b/midx-write.c index 0066594fa6..b3827b936b 100644 --- a/midx-write.c +++ b/midx-write.c @@ -1131,7 +1131,8 @@ static int write_midx_internal(struct repository *r, const char *object_dir, ctx.pack_paths_checked = 0; if (flags & MIDX_PROGRESS) - ctx.progress = start_delayed_progress(_("Adding packfiles to multi-pack-index"), 0); + ctx.progress = start_delayed_progress(r, + _("Adding packfiles to multi-pack-index"), 0); else ctx.progress = NULL; @@ -1539,7 +1540,9 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla CALLOC_ARRAY(count, m->num_packs); if (flags & MIDX_PROGRESS) - progress = start_delayed_progress(_("Counting referenced objects"), + progress = start_delayed_progress( + r, + _("Counting referenced objects"), m->num_objects); for (i = 0; i < m->num_objects; i++) { int pack_int_id = nth_midxed_pack_int_id(m, i); @@ -1549,7 +1552,9 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla stop_progress(&progress); if (flags & MIDX_PROGRESS) - progress = start_delayed_progress(_("Finding and deleting unreferenced packfiles"), + progress = start_delayed_progress( + r, + _("Finding and deleting unreferenced packfiles"), m->num_packs); for (i = 0; i < m->num_packs; i++) { char *pack_name; diff --git a/midx.c b/midx.c index f8a75cafd4..d91088efb8 100644 --- a/midx.c +++ b/midx.c @@ -907,7 +907,8 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag midx_report(_("incorrect checksum")); if (flags & MIDX_PROGRESS) - progress = start_delayed_progress(_("Looking for referenced packfiles"), + progress = start_delayed_progress(r, + _("Looking for referenced packfiles"), m->num_packs + m->num_packs_in_base); for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) { if (prepare_midx_pack(r, m, i)) @@ -927,7 +928,8 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag } if (flags & MIDX_PROGRESS) - progress = start_sparse_progress(_("Verifying OID order in multi-pack-index"), + progress = start_sparse_progress(r, + _("Verifying OID order in multi-pack-index"), m->num_objects - 1); for (curr = m; curr; curr = curr->base_midx) { @@ -959,14 +961,17 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag } if (flags & MIDX_PROGRESS) - progress = start_sparse_progress(_("Sorting objects by packfile"), + progress = start_sparse_progress(r, + _("Sorting objects by packfile"), m->num_objects); display_progress(progress, 0); /* TODO: Measure QSORT() progress */ QSORT(pairs, m->num_objects, compare_pair_pos_vs_id); stop_progress(&progress); if (flags & MIDX_PROGRESS) - progress = start_sparse_progress(_("Verifying object offsets"), m->num_objects); + progress = start_sparse_progress(r, + _("Verifying object offsets"), + m->num_objects); for (i = 0; i < m->num_objects + m->num_objects_in_base; i++) { struct object_id oid; struct pack_entry e; diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index 4f8be53c2b..a06a1f35c6 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, + "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, + "Selecting bitmap commits", 0); for (;;) { struct commit *chosen = NULL; diff --git a/pack-bitmap.c b/pack-bitmap.c index bceb6da042..48e3b99efb 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -2578,7 +2578,9 @@ void test_bitmap_walk(struct rev_info *revs) tdata.trees = ewah_to_bitmap(bitmap_git->trees); tdata.blobs = ewah_to_bitmap(bitmap_git->blobs); tdata.tags = ewah_to_bitmap(bitmap_git->tags); - tdata.prg = start_progress("Verifying bitmap entries", result_popcnt); + tdata.prg = start_progress(revs->repo, + "Verifying bitmap entries", + result_popcnt); tdata.seen = 0; traverse_commit_list(revs, &test_show_commit, &test_show_object, &tdata); diff --git a/preload-index.c b/preload-index.c index ab94d6f399..40ab2abafb 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, + _("Refreshing index"), + index->cache_nr); pthread_mutex_init(&pd.mutex, NULL); } diff --git a/progress.c b/progress.c index a8fdfceb5c..8d5ae70f3a 100644 --- a/progress.c +++ b/progress.c @@ -9,7 +9,6 @@ */ #define GIT_TEST_PROGRESS_ONLY -#define USE_THE_REPOSITORY_VARIABLE #define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" @@ -37,6 +36,7 @@ struct throughput { }; struct progress { + struct repository *repo; const char *title; uint64_t last_value; uint64_t total; @@ -254,10 +254,12 @@ void display_progress(struct progress *progress, uint64_t n) display(progress, n, NULL); } -static struct progress *start_progress_delay(const char *title, uint64_t total, +static struct progress *start_progress_delay(struct repository *r, + const char *title, uint64_t total, unsigned delay, unsigned sparse) { struct progress *progress = xmalloc(sizeof(*progress)); + progress->repo = r; progress->title = title; progress->total = total; progress->last_value = -1; @@ -270,7 +272,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total, progress->title_len = utf8_strwidth(title); progress->split = 0; set_progress_signal(); - trace2_region_enter("progress", title, the_repository); + trace2_region_enter("progress", title, r); return progress; } @@ -284,14 +286,16 @@ static int get_default_delay(void) return delay_in_secs; } -struct progress *start_delayed_progress(const char *title, uint64_t total) +struct progress *start_delayed_progress(struct repository *r, + const char *title, uint64_t total) { - return start_progress_delay(title, total, get_default_delay(), 0); + return start_progress_delay(r, title, total, get_default_delay(), 0); } -struct progress *start_progress(const char *title, uint64_t total) +struct progress *start_progress(struct repository *r, + const char *title, uint64_t total) { - return start_progress_delay(title, total, 0, 0); + return start_progress_delay(r, title, total, 0, 0); } /* @@ -303,15 +307,17 @@ struct progress *start_progress(const char *title, uint64_t total) * When "sparse" is set, stop_progress() will automatically force the done * message to show 100%. */ -struct progress *start_sparse_progress(const char *title, uint64_t total) +struct progress *start_sparse_progress(struct repository *r, + const char *title, uint64_t total) { - return start_progress_delay(title, total, 0, 1); + return start_progress_delay(r, title, total, 0, 1); } -struct progress *start_delayed_sparse_progress(const char *title, +struct progress *start_delayed_sparse_progress(struct repository *r, + const char *title, uint64_t total) { - return start_progress_delay(title, total, get_default_delay(), 1); + return start_progress_delay(r, title, total, get_default_delay(), 1); } static void finish_if_sparse(struct progress *progress) @@ -341,14 +347,14 @@ static void force_last_update(struct progress *progress, const char *msg) static void log_trace2(struct progress *progress) { - trace2_data_intmax("progress", the_repository, "total_objects", + trace2_data_intmax("progress", progress->repo, "total_objects", progress->total); if (progress->throughput) - trace2_data_intmax("progress", the_repository, "total_bytes", + trace2_data_intmax("progress", progress->repo, "total_bytes", progress->throughput->curr_total); - trace2_region_leave("progress", progress->title, the_repository); + trace2_region_leave("progress", progress->title, progress->repo); } void stop_progress_msg(struct progress **p_progress, const char *msg) diff --git a/progress.h b/progress.h index 3a945637c8..ed068c7bab 100644 --- a/progress.h +++ b/progress.h @@ -3,6 +3,7 @@ #include "gettext.h" struct progress; +struct repository; #ifdef GIT_TEST_PROGRESS_ONLY @@ -14,10 +15,14 @@ void progress_test_force_update(void); void display_throughput(struct progress *progress, uint64_t total); void display_progress(struct progress *progress, uint64_t n); -struct progress *start_progress(const char *title, uint64_t total); -struct progress *start_sparse_progress(const char *title, uint64_t total); -struct progress *start_delayed_progress(const char *title, uint64_t total); -struct progress *start_delayed_sparse_progress(const char *title, +struct progress *start_progress(struct repository *r, + const char *title, uint64_t total); +struct progress *start_sparse_progress(struct repository *r, + const char *title, uint64_t total); +struct progress *start_delayed_progress(struct repository *r, + const char *title, uint64_t total); +struct progress *start_delayed_sparse_progress(struct repository *r, + const char *title, uint64_t total); void stop_progress_msg(struct progress **p_progress, const char *msg); static inline void stop_progress(struct progress **p_progress) diff --git a/prune-packed.c b/prune-packed.c index d1c65ab10e..7dad2fc0c1 100644 --- a/prune-packed.c +++ b/prune-packed.c @@ -37,7 +37,8 @@ static int prune_object(const struct object_id *oid, const char *path, void prune_packed_objects(int opts) { if (opts & PRUNE_PACKED_VERBOSE) - progress = start_delayed_progress(_("Removing duplicate objects"), 256); + progress = start_delayed_progress(the_repository, + _("Removing duplicate objects"), 256); for_each_loose_file_in_objdir(repo_get_object_directory(the_repository), prune_object, NULL, prune_subdir, &opts); diff --git a/pseudo-merge.c b/pseudo-merge.c index 971f54cfe1..893b763fe4 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, + "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 15d79839c2..38c36caa7f 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, + _("Refresh index"), istate->cache_nr); trace_performance_enter(); diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c index 44be2645e9..1f75b7bd19 100644 --- a/t/helper/test-progress.c +++ b/t/helper/test-progress.c @@ -17,10 +17,14 @@ * * See 't0500-progress-display.sh' for examples. */ + +#define USE_THE_REPOSITORY_VARIABLE #define GIT_TEST_PROGRESS_ONLY + #include "test-tool.h" #include "parse-options.h" #include "progress.h" +#include "repository.h" #include "strbuf.h" #include "string-list.h" @@ -64,7 +68,7 @@ int cmd__progress(int argc, const char **argv) else die("invalid input: '%s'", line.buf); - progress = start_progress(title, total); + progress = start_progress(the_repository, title, total); } else if (skip_prefix(line.buf, "progress ", (const char **) &end)) { uint64_t item_count = strtoull(end, &end, 10); if (*end != '\0') diff --git a/unpack-trees.c b/unpack-trees.c index b3be5d542f..334cb84f65 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, + _("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 7cc9dbea46..1cf3da0219 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; -- cgit v1.3 From f66d1423f528403a33e8984f765801deb1b9cb97 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 16 Jan 2025 13:35:53 -0800 Subject: builtin: send usage() help text to standard output Using the show_usage_and_exit_if_asked() helper we introduced earlier, fix callers of usage() that want to show the help text when explicitly asked by the end-user. The help text now goes to the standard output stream for them. These are the bog standard "if we got only '-h', then that is a request for help" callers. Their if (argc == 2 && !strcmp(argv[1], "-h")) usage(message); are simply replaced with show_usage_and_exit_if_asked(argc, argv, message); With this, the built-ins tested by t0012 all send their help text to their standard output stream, so the check in t0012 that was half tightened earlier is now fully tightened to insist on standard error stream being empty. Acked-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/check-ref-format.c | 4 ++-- builtin/diff-files.c | 3 +-- builtin/diff-index.c | 3 +-- builtin/diff-tree.c | 3 +-- builtin/fast-import.c | 3 +-- builtin/get-tar-commit-id.c | 4 +++- builtin/index-pack.c | 3 +-- builtin/mailsplit.c | 4 ++-- builtin/merge-index.c | 7 ++++++- builtin/merge-ours.c | 3 +-- builtin/merge-recursive.c | 6 ++++++ builtin/pack-redundant.c | 3 +-- builtin/remote-ext.c | 2 ++ builtin/remote-fd.c | 1 + builtin/rev-list.c | 3 +-- builtin/rev-parse.c | 2 ++ builtin/unpack-objects.c | 2 ++ builtin/upload-archive.c | 3 +-- t/t0012-help.sh | 10 ++-------- 19 files changed, 37 insertions(+), 32 deletions(-) (limited to 'builtin/unpack-objects.c') diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c index cef1ffe3ce..5d80afeec0 100644 --- a/builtin/check-ref-format.c +++ b/builtin/check-ref-format.c @@ -64,8 +64,8 @@ int cmd_check_ref_format(int argc, BUG_ON_NON_EMPTY_PREFIX(prefix); - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(builtin_check_ref_format_usage); + show_usage_if_asked(argc, argv, + builtin_check_ref_format_usage); if (argc == 3 && !strcmp(argv[1], "--branch")) return check_ref_format_branch(argv[2]); diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 604b04bb2c..99b1749723 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -29,8 +29,7 @@ int cmd_diff_files(int argc, int result; unsigned options = 0; - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(diff_files_usage); + show_usage_if_asked(argc, argv, diff_files_usage); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ diff --git a/builtin/diff-index.c b/builtin/diff-index.c index ebc824602e..81c0bc8ed7 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -26,8 +26,7 @@ int cmd_diff_index(int argc, int i; int result; - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(diff_cache_usage); + show_usage_if_asked(argc, argv, diff_cache_usage); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 40804e7b48..e31cc797fe 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -122,8 +122,7 @@ int cmd_diff_tree(int argc, int read_stdin = 0; int merge_base = 0; - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(diff_tree_usage); + show_usage_if_asked(argc, argv, diff_tree_usage); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 0f86392761..2da46fecdc 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -3565,8 +3565,7 @@ int cmd_fast_import(int argc, { unsigned int i; - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(fast_import_usage); + show_usage_if_asked(argc, argv, fast_import_usage); reset_pack_idx_option(&pack_idx_opts); git_pack_config(); diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c index 6bec0d1854..e4cd1627b4 100644 --- a/builtin/get-tar-commit-id.c +++ b/builtin/get-tar-commit-id.c @@ -13,7 +13,7 @@ static const char builtin_get_tar_commit_id_usage[] = #define HEADERSIZE (2 * RECORDSIZE) int cmd_get_tar_commit_id(int argc, - const char **argv UNUSED, + const char **argv, const char *prefix, struct repository *repo UNUSED) { @@ -27,6 +27,8 @@ int cmd_get_tar_commit_id(int argc, BUG_ON_NON_EMPTY_PREFIX(prefix); + show_usage_if_asked(argc, argv, builtin_get_tar_commit_id_usage); + if (argc != 1) usage(builtin_get_tar_commit_id_usage); diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 0b62b2589f..d41b126ec0 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1897,8 +1897,7 @@ int cmd_index_pack(int argc, */ fetch_if_missing = 0; - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(index_pack_usage); + show_usage_if_asked(argc, argv, index_pack_usage); disable_replace_refs(); fsck_options.walk = mark_link; diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c index 41dd304731..264df6259a 100644 --- a/builtin/mailsplit.c +++ b/builtin/mailsplit.c @@ -284,6 +284,8 @@ int cmd_mailsplit(int argc, BUG_ON_NON_EMPTY_PREFIX(prefix); + show_usage_if_asked(argc, argv, git_mailsplit_usage); + for (argp = argv+1; *argp; argp++) { const char *arg = *argp; @@ -297,8 +299,6 @@ int cmd_mailsplit(int argc, continue; } else if ( arg[1] == 'f' ) { nr = strtol(arg+2, NULL, 10); - } else if ( arg[1] == 'h' ) { - usage(git_mailsplit_usage); } else if ( arg[1] == 'b' && !arg[2] ) { allow_bare = 1; } else if (!strcmp(arg, "--keep-cr")) { diff --git a/builtin/merge-index.c b/builtin/merge-index.c index 342699edb7..3314fb1336 100644 --- a/builtin/merge-index.c +++ b/builtin/merge-index.c @@ -75,6 +75,9 @@ static void merge_all(void) } } +static const char usage_string[] = +"git merge-index [-o] [-q] (-a | [--] [...])"; + int cmd_merge_index(int argc, const char **argv, const char *prefix UNUSED, @@ -87,8 +90,10 @@ int cmd_merge_index(int argc, */ signal(SIGCHLD, SIG_DFL); + show_usage_if_asked(argc, argv, usage_string); + if (argc < 3) - usage("git merge-index [-o] [-q] (-a | [--] [...])"); + usage(usage_string); repo_read_index(the_repository); diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c index 3ecd9172f1..97b8a792c7 100644 --- a/builtin/merge-ours.c +++ b/builtin/merge-ours.c @@ -23,8 +23,7 @@ int cmd_merge_ours(int argc, const char *prefix UNUSED, struct repository *repo UNUSED) { - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(builtin_merge_ours_usage); + show_usage_if_asked(argc, argv, builtin_merge_ours_usage); /* * The contents of the current index becomes the tree we diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c index 1dd295558b..abfc060e28 100644 --- a/builtin/merge-recursive.c +++ b/builtin/merge-recursive.c @@ -38,6 +38,12 @@ int cmd_merge_recursive(int argc, if (argv[0] && ends_with(argv[0], "-subtree")) o.subtree_shift = ""; + if (argc == 2 && !strcmp(argv[1], "-h")) { + struct strbuf msg = STRBUF_INIT; + strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]); + show_usage_if_asked(argc, argv, msg.buf); + } + if (argc < 4) usagef(builtin_merge_recursive_usage, argv[0]); diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index e046575871..3febe732f8 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -595,8 +595,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s struct strbuf idx_name = STRBUF_INIT; char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */ - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(pack_redundant_usage); + show_usage_if_asked(argc, argv, pack_redundant_usage); for (i = 1; i < argc; i++) { const char *arg = argv[i]; diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c index 33c8ae0fc7..bd2037f27d 100644 --- a/builtin/remote-ext.c +++ b/builtin/remote-ext.c @@ -202,6 +202,8 @@ int cmd_remote_ext(int argc, { BUG_ON_NON_EMPTY_PREFIX(prefix); + show_usage_if_asked(argc, argv, usage_msg); + if (argc != 3) usage(usage_msg); diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c index ae896eda57..39908546ba 100644 --- a/builtin/remote-fd.c +++ b/builtin/remote-fd.c @@ -64,6 +64,7 @@ int cmd_remote_fd(int argc, BUG_ON_NON_EMPTY_PREFIX(prefix); + show_usage_if_asked(argc, argv, usage_msg); if (argc != 3) usage(usage_msg); diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 3196da7b2d..28f148049f 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -542,8 +542,7 @@ int cmd_rev_list(int argc, const char *show_progress = NULL; int ret = 0; - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(rev_list_usage); + show_usage_if_asked(argc, argv, rev_list_usage); git_config(git_default_config, NULL); repo_init_revisions(the_repository, &revs, prefix); diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 949747a6b6..428c866c05 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -713,6 +713,8 @@ int cmd_rev_parse(int argc, int seen_end_of_options = 0; enum format_type format = FORMAT_DEFAULT; + show_usage_if_asked(argc, argv, builtin_rev_parse_usage); + if (argc > 1 && !strcmp("--parseopt", argv[1])) return cmd_parseopt(argc - 1, argv + 1, prefix); diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 2197d6d933..8faa6024b2 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -619,6 +619,8 @@ int cmd_unpack_objects(int argc, quiet = !isatty(2); + show_usage_if_asked(argc, argv, unpack_usage); + for (i = 1 ; i < argc; i++) { const char *arg = argv[i]; diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c index 9d76a31c8f..97d7c9522f 100644 --- a/builtin/upload-archive.c +++ b/builtin/upload-archive.c @@ -93,8 +93,7 @@ struct repository *repo UNUSED) BUG_ON_NON_EMPTY_PREFIX(prefix); - if (argc == 2 && !strcmp(argv[1], "-h")) - usage(upload_archive_usage); + show_usage_if_asked(argc, argv, upload_archive_usage); /* * Set up sideband subprocess. diff --git a/t/t0012-help.sh b/t/t0012-help.sh index 9c7ae9fd36..d3a0967e9d 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -257,14 +257,8 @@ do export GIT_CEILING_DIRECTORIES && test_expect_code 129 git -C sub $builtin -h >output 2>err ) && - if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT" - then - test_must_be_empty err && - test_grep usage output - else - test_grep usage output || - test_grep usage err - fi + test_must_be_empty err && + test_grep usage output ' done Date: Sun, 19 Jan 2025 08:23:37 -0500 Subject: packfile: factor out --pack_header argument parsing Both index-pack and unpack-objects accept a --pack_header argument. This is an undocumented internal argument used by receive-pack and fetch to pass along information about the header of the pack, which they've already read from the incoming stream. In preparation for a bugfix, let's factor the duplicated code into a common helper. The callers are still responsible for identifying the option. While this could likewise be factored out, it is more flexible this way (e.g., if they ever started using parse-options and wanted to handle both the stuck and unstuck forms). Likewise, the callers are responsible for reporting errors, though they both just call die(). I've tweaked unpack-objects to match index-pack in marking the error for translation. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 14 +++----------- builtin/unpack-objects.c | 16 ++++------------ packfile.c | 17 +++++++++++++++++ packfile.h | 6 ++++++ 4 files changed, 30 insertions(+), 23 deletions(-) (limited to 'builtin/unpack-objects.c') diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 763b01372a..bab42dfc2a 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1801,18 +1801,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) nr_threads = 1; } } else if (starts_with(arg, "--pack_header=")) { - struct pack_header *hdr; - char *c; - - hdr = (struct pack_header *)input_buffer; - hdr->hdr_signature = htonl(PACK_SIGNATURE); - hdr->hdr_version = htonl(strtoul(arg + 14, &c, 10)); - if (*c != ',') - die(_("bad %s"), arg); - hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10)); - if (*c) + if (parse_pack_header_option(arg + 14, + input_buffer, + &input_len) < 0) die(_("bad %s"), arg); - input_len = sizeof(*hdr); } else if (!strcmp(arg, "-v")) { verbose = 1; } else if (!strcmp(arg, "--progress-title")) { diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 08fa2a7a74..3161447274 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -15,6 +15,7 @@ #include "progress.h" #include "decorate.h" #include "fsck.h" +#include "packfile.h" static int dry_run, quiet, recover, has_errors, strict; static const char unpack_usage[] = "git unpack-objects [-n] [-q] [-r] [--strict]"; @@ -639,18 +640,9 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED) continue; } if (starts_with(arg, "--pack_header=")) { - struct pack_header *hdr; - char *c; - - hdr = (struct pack_header *)buffer; - hdr->hdr_signature = htonl(PACK_SIGNATURE); - hdr->hdr_version = htonl(strtoul(arg + 14, &c, 10)); - if (*c != ',') - die("bad %s", arg); - hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10)); - if (*c) - die("bad %s", arg); - len = sizeof(*hdr); + if (parse_pack_header_option(arg + 14, + buffer, &len) < 0) + die(_("bad %s"), arg); continue; } if (skip_prefix(arg, "--max-input-size=", &arg)) { diff --git a/packfile.c b/packfile.c index 813584646f..e2bdadc7cb 100644 --- a/packfile.c +++ b/packfile.c @@ -2294,3 +2294,20 @@ int is_promisor_object(const struct object_id *oid) } return oidset_contains(&promisor_objects, oid); } + +int parse_pack_header_option(const char *in, unsigned char *out, unsigned int *len) +{ + struct pack_header *hdr; + char *c; + + hdr = (struct pack_header *)out; + hdr->hdr_signature = htonl(PACK_SIGNATURE); + hdr->hdr_version = htonl(strtoul(in, &c, 10)); + if (*c != ',') + return -1; + hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10)); + if (*c) + return -1; + *len = sizeof(*hdr); + return 0; +} diff --git a/packfile.h b/packfile.h index eb18ec15db..41f38b4832 100644 --- a/packfile.h +++ b/packfile.h @@ -210,4 +210,10 @@ int is_promisor_object(const struct object_id *oid); int load_idx(const char *path, const unsigned int hashsz, void *idx_map, size_t idx_size, struct packed_git *p); +/* + * Parse a --pack_header option as accepted by index-pack and unpack-objects, + * turning it into the matching bytes we'd find in a pack. + */ +int parse_pack_header_option(const char *in, unsigned char *out, unsigned int *len); + #endif -- cgit v1.3 From f1299bff26a20b70bb5b8440526a2bd3c6de298a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 19 Jan 2025 08:25:47 -0500 Subject: index-pack, unpack-objects: use get_be32() for reading pack header Both of these commands read the incoming pack into a static unsigned char buffer in BSS, and then parse it by casting the start of the buffer to a struct pack_header. This can result in SIGBUS on some platforms if the compiler doesn't place the buffer in a position that is properly aligned for 4-byte integers. This reportedly happens with unpack-objects (but not index-pack) on sparc64 when compiled with clang (but not gcc). But we are definitely in the wrong in both spots; since the buffer's type is unsigned char, we can't depend on larger alignment. When it works it is only because we are lucky. We'll fix this by switching to get_be32() to read the headers (just like the last few commits similarly switched us to put_be32() for writing into the same buffer). It would be nice to factor this out into a common helper function, but the interface ends up quite awkward. Either the caller needs to hardcode how many bytes we'll need, or it needs to pass us its fill()/use() functions as pointers. So I've just fixed both spots in the same way; this is not code that is likely to be repeated a third time (most of the pack reading code uses an mmap'd buffer, which should be properly aligned). I did make one tweak to the shared code: our pack_version_ok() macro expects us to pass the big-endian value we'd get by casting. We can introduce a "native" variant which uses the host integer ordering. Reported-by: Koakuma Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 12 +++++++----- builtin/unpack-objects.c | 13 +++++++------ pack.h | 3 ++- 3 files changed, 16 insertions(+), 12 deletions(-) (limited to 'builtin/unpack-objects.c') diff --git a/builtin/index-pack.c b/builtin/index-pack.c index bab42dfc2a..5f0ff1ce04 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -363,16 +363,18 @@ static const char *open_pack_file(const char *pack_name) static void parse_pack_header(void) { - struct pack_header *hdr = fill(sizeof(struct pack_header)); + unsigned char *hdr = fill(sizeof(struct pack_header)); /* Header consistency check */ - if (hdr->hdr_signature != htonl(PACK_SIGNATURE)) + if (get_be32(hdr) != PACK_SIGNATURE) die(_("pack signature mismatch")); - if (!pack_version_ok(hdr->hdr_version)) + hdr += 4; + if (!pack_version_ok_native(get_be32(hdr))) die(_("pack version %"PRIu32" unsupported"), - ntohl(hdr->hdr_version)); + get_be32(hdr)); + hdr += 4; - nr_objects = ntohl(hdr->hdr_entries); + nr_objects = get_be32(hdr); use(sizeof(struct pack_header)); } diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 3161447274..fc3de6dac8 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -576,15 +576,16 @@ static void unpack_one(unsigned nr) static void unpack_all(void) { int i; - struct pack_header *hdr = fill(sizeof(struct pack_header)); + unsigned char *hdr = fill(sizeof(struct pack_header)); - nr_objects = ntohl(hdr->hdr_entries); - - if (ntohl(hdr->hdr_signature) != PACK_SIGNATURE) + if (get_be32(hdr) != PACK_SIGNATURE) die("bad pack file"); - if (!pack_version_ok(hdr->hdr_version)) + hdr += 4; + if (!pack_version_ok_native(get_be32(hdr))) die("unknown pack file version %"PRIu32, - ntohl(hdr->hdr_version)); + get_be32(hdr)); + hdr += 4; + nr_objects = get_be32(hdr); use(sizeof(struct pack_header)); if (!quiet) diff --git a/pack.h b/pack.h index 3ab9e3f60c..33b42fdad5 100644 --- a/pack.h +++ b/pack.h @@ -13,7 +13,8 @@ struct repository; */ #define PACK_SIGNATURE 0x5041434b /* "PACK" */ #define PACK_VERSION 2 -#define pack_version_ok(v) ((v) == htonl(2) || (v) == htonl(3)) +#define pack_version_ok(v) pack_version_ok_native(ntohl(v)) +#define pack_version_ok_native(v) ((v) == 2 || (v) == 3) struct pack_header { uint32_t hdr_signature; uint32_t hdr_version; -- cgit v1.3 From 98046591b96a213e05d17569b1645e772df91b90 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 19 Jan 2025 08:25:53 -0500 Subject: index-pack, unpack-objects: use skip_prefix to avoid magic number When parsing --pack_header=, we manually skip 14 bytes to the data. Let's use skip_prefix() to do this automatically. Note that we overwrite our pointer to the front of the string, so we have to add more context to the error message. We could avoid this by declaring an extra pointer to hold the value, but I think the modified message is actually preferable; it should give translators a bit more context. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 6 +++--- builtin/unpack-objects.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'builtin/unpack-objects.c') diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 5f0ff1ce04..d80e054581 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1802,11 +1802,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) warning(_("no threads support, ignoring %s"), arg); nr_threads = 1; } - } else if (starts_with(arg, "--pack_header=")) { - if (parse_pack_header_option(arg + 14, + } else if (skip_prefix(arg, "--pack_header=", &arg)) { + if (parse_pack_header_option(arg, input_buffer, &input_len) < 0) - die(_("bad %s"), arg); + die(_("bad --pack_header: %s"), arg); } else if (!strcmp(arg, "-v")) { verbose = 1; } else if (!strcmp(arg, "--progress-title")) { diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index fc3de6dac8..028cfe175e 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -640,10 +640,10 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED) fsck_set_msg_types(&fsck_options, arg); continue; } - if (starts_with(arg, "--pack_header=")) { - if (parse_pack_header_option(arg + 14, + if (skip_prefix(arg, "--pack_header=", &arg)) { + if (parse_pack_header_option(arg, buffer, &len) < 0) - die(_("bad %s"), arg); + die(_("bad --pack_header: %s"), arg); continue; } if (skip_prefix(arg, "--max-input-size=", &arg)) { -- cgit v1.3