Re: [PATCH] last-modified: implement faster algorithm
- From
Jeff King <peff@peff.net>
- Date
- Oct 17, 2025, 06:37 UTC
- Message-ID
- <20251017063701.GA3091356@coredump.intra.peff.net>
- In-Reply-To
- <20251016-b4-toon-last-modified-faster-v1-1-85dca8a29e5c@iotcl.com>
On Thu, Oct 16, 2025 at 10:39:25AM +0200, Toon Claes wrote:
Show 7 quoted lines
> + for (i = 0; i < diff_queued_diff.nr; i++) {
> + struct diff_filepair *fp = diff_queued_diff.queue[i];
> + size_t k = path_idx(lm, fp->two->path);
> + if (0 <= k && bitmap_get(active_c, k))
> + bitmap_set(lm->scratch, k);
> + diff_free_filepair(fp);
> + }Just one little oddity while looking at this versus the old patches from Taylor. Here you call diff_free_filepair(). But later...
> + diff_queued_diff.nr = 0; > + diff_queue_clear(&diff_queued_diff);
...you call diff_queue_clear(), which frees the filepairs itself. It does the right thing, because you truncate the queue explicitly. But would it be simpler to just leave them in place and let the _clear() function clean up? I.e., this:
diff --git a/builtin/last-modified.c b/builtin/last-modified.c index 40e520ba18..47f2b0ed44 100644 --- a/builtin/last-modified.c +++ b/builtin/last-modified.c @@ -315,7 +315,6 @@ static void process_parent(struct last_modified *lm, size_t k = path_idx(lm, fp->two->path); if (0 <= k && bitmap_get(active_c, k)) bitmap_set(lm->scratch, k); - diff_free_filepair(fp); } for (i = 0; i < lm->all_paths_nr; i++) { if (bitmap_get(active_c, i) && !bitmap_get(lm->scratch, i)) @@ -331,7 +330,6 @@ static void process_parent(struct last_modified *lm, } memset(lm->scratch->words, 0x0, lm->scratch->word_alloc); - diff_queued_diff.nr = 0; diff_queue_clear(&diff_queued_diff); } which feels a lot more idiomatic to me. -Peff