On 25/02/12 01:06AM, Karthik Nayak wrote: > 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 was mostly to avoid changing the `add_remove_fn_t` and `change_fn_t` types that store `diff_addremove()` and `diff_change()` in `diff_options`. The `file_add_remove()` and `file_change()` functions, which also can be set in `diff_options`, do not ever queue file pairs so I don't think returning `diff_filepair` makes much sense there. > 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? Patrick has suggested we avoid using the global `diff_queue_struct` implicitly. Currently, in the next version I'm planning to keep the separate functions as `diff_queue_addremove()` and `diff_queue_change()`, but also accept `diff_queue_struct` as an argument. > 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. I'll add in the next version. Thanks -Justin
