On Mon, Feb 13, 2023 at 12:42 AM Glen Choo <chooglen@xxxxxxxxxx> wrote:
>
> Calvin Wan <calvinwan@xxxxxxxxxx> writes:
>
> > Refactor out logic that sets up the diff_change call into a helper
> > function for a future patch.
>
> This seems underspecified; there are two diff_change calls in diff-lib,
> and the call in show_modified() is not changed in this patch.
>
> > +static int diff_change_helper(struct diff_options *options,
> > + unsigned newmode, unsigned dirty_submodule,
> > + int changed, struct index_state *istate,
> > + struct cache_entry *ce)
>
> The function name is very generic, and it's not clear:
>
> - What this does besides calling "diff_change()".
> - When I should call this instead of "diff_change()".
> - What the return value means.
>
> Both of these should be documented in a comment, and I also suggest
> renaming the function.
ack.
> > @@ -245,21 +269,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> > newmode = ce_mode_from_stat(ce, st.st_mode);
> > }
> >
> > - if (!changed && !dirty_submodule) {
> > - ce_mark_uptodate(ce);
> > - mark_fsmonitor_valid(istate, ce);
> > - if (!revs->diffopt.flags.find_copies_harder)
> > - continue;
> > - }
> > - oldmode = ce->ce_mode;
> > - old_oid = &ce->oid;
> > - new_oid = changed ? null_oid() : &ce->oid;
> > - diff_change(&revs->diffopt, oldmode, newmode,
> > - old_oid, new_oid,
> > - !is_null_oid(old_oid),
> > - !is_null_oid(new_oid),
> > - ce->name, 0, dirty_submodule);
> > -
> > + if (diff_change_helper(&revs->diffopt, newmode, dirty_submodule,
> > + changed, istate, ce))
> > + continue;
> > }
>
> If I'm reading the indentation correctly, the "continue" comes right
> before the end of the for-loop block, so it's a no-op and should be
> removed.
It is a no-op, but I left it in as future-proofing in case more code is
added after that block later. I'm not sure whether that line of
reasoning is enough to leave it in though.
