From: Toon Claes <toon@iotcl.com>
Date: Fri, 17 Oct 2025 10:47:41 GMT
Subject: Re: [PATCH] last-modified: implement faster algorithm
Message-ID: <87ms5pu7n6.fsf@iotcl.com>
In-Reply-To: <20251017063701.GA3091356@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Thu, Oct 16, 2025 at 10:39:25AM +0200, Toon Claes wrote:
>
>> +	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.

Hah, yes. I think I ended up in this situation because initially I was
only trying to fix memory leaks. Thanks, I will included these changes.

-- 
Cheers,
Toon