Re: [PATCH v2 1/3] diff: return diff_filepair from diff queue helpers

From: Karthik Nayak <karthik.188@gmail.com>
Date: Feb 12, 2025, 09:06 UTC
Message-ID: <CAOLa=ZRssYP8U+kyTYdUY_-WGCLLk4mVxQRbgu87g7AdxPJbZw@mail.gmail.com>
In-Reply-To: <20250212041825.2455031-2-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 9 quoted lines

> The `diff_addremove()` and `diff_change()` functions setup and queue
> diffs, but do not return the `diff_filepair` added to the queue. In a
> subsequent commit, modifications to `diff_filepair` need to take place
> in certain cases after being queued.
>
> Split out the queuing operations into `diff_filepair_addremove()` and
> `diff_filepair_change()` which also return a handle to the queued
> `diff_filepair`.
>

This patch keeps `diff_addremove()` and `diff_change()` while introducing two new functions which return the `diff_filepair`. Just a thought, why not replace them? The users `diff_addremove()` and `diff_change()` could simply call the new functions and ignore the return value?

This would be messy if there were a lot of users of `diff_addremove()` and `diff_change()`, but I only see a few callers. Wouldn't it be cleaner to just replace?

The patch looks good to me otherwise.

[snip]

Show 23 quoted lines

> diff --git a/diff.h b/diff.h
> index 0a566f5531..6ea63f01e7 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -508,6 +508,21 @@ void diff_set_default_prefix(struct diff_options *options);
>
>  int diff_can_quit_early(struct diff_options *);
>
> +struct diff_filepair *diff_filepair_addremove(struct diff_options *,
> +					      int addremove, unsigned mode,
> +					      const struct object_id *oid,
> +					      int oid_valid, const char *fullpath,
> +					      unsigned dirty_submodule);
> +
> +struct diff_filepair *diff_filepair_change(struct diff_options *,
> +					   unsigned mode1, unsigned mode2,
> +					   const struct object_id *old_oid,
> +					   const struct object_id *new_oid,
> +					   int old_oid_valid, int new_oid_valid,
> +					   const char *fullpath,
> +					   unsigned dirty_submodule1,
> +					   unsigned dirty_submodule2);
> +

Nit: would be nice to have some comments to describe what these
functions do.

Show 5 quoted lines

>  void diff_addremove(struct diff_options *,
>  		    int addremove,
>  		    unsigned mode,
> --
> 2.48.1