Re: [PATCH v8 5/6] diff-lib: refactor out diff_change logic

[Calvin Wan <calvinwan@google.com>]

On Mon, Feb 13, 2023 at 12:42 AM Glen Choo <chooglen@google.com> wrote:
>
> Calvin Wan <calvinwan@google.com> 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.