Justin Tobler <jltobler@xxxxxxxxx> writes: > 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] > 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. > void diff_addremove(struct diff_options *, > int addremove, > unsigned mode, > -- > 2.48.1
Attachment:
signature.asc
Description: PGP signature
