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.
> @@ -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.
