| Message ID | 20230209000212.1892457-6-calvinwan@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule: parallelize diff | expand |
On Thu, Feb 09 2023, Calvin Wan wrote: > + diff_change(options, oldmode, newmode, > + old_oid, new_oid, > + !is_null_oid(old_oid), > + !is_null_oid(new_oid), > + ce->name, 0, dirty_submodule); Nit: This has odd not-our-usual-style indentation (to align with the "("). I didn't spot it before, but I vaguely recall seeing something like this in another one of your patches, but maybe I misrecall. In case not maybe some editor settings need tweaking? I haven't looked carefully at the rest to see if the same issue occurs in other code here. > - 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); So in this case it's not new code, but code moving, note the four spaces after the sequence of tabs that aren't in your version. So perhaps your editor on re-indentation is configured not to just strip off the leading \t to re-indent (which is all that's needed here) but strips all whitespace, then re-indents after its own mind?
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. > @@ -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.
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.
Calvin Wan <calvinwan@google.com> writes: >> > @@ -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. I don't think it is. If we haven't thought of the reason why we would need to skip code, that seems like YAGNI to me. As a matter of personal taste, I wouldn't leave a trailing "continue" in an earlier patch even if I were going to change it in a later patch, because it looks too much like an unintentional mistake.
diff --git a/diff-lib.c b/diff-lib.c index dec040c366..7101cfda3f 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -88,6 +88,31 @@ static int match_stat_with_submodule(struct diff_options *diffopt, return changed; } +static int diff_change_helper(struct diff_options *options, + unsigned newmode, unsigned dirty_submodule, + int changed, struct index_state *istate, + struct cache_entry *ce) +{ + unsigned int oldmode; + const struct object_id *old_oid, *new_oid; + + if (!changed && !dirty_submodule) { + ce_mark_uptodate(ce); + mark_fsmonitor_valid(istate, ce); + if (!options->flags.find_copies_harder) + return 1; + } + oldmode = ce->ce_mode; + old_oid = &ce->oid; + new_oid = changed ? null_oid() : &ce->oid; + diff_change(options, oldmode, newmode, + old_oid, new_oid, + !is_null_oid(old_oid), + !is_null_oid(new_oid), + ce->name, 0, dirty_submodule); + return 0; +} + int run_diff_files(struct rev_info *revs, unsigned int option) { int entries, i; @@ -105,11 +130,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option) diff_unmerged_stage = 2; entries = istate->cache_nr; for (i = 0; i < entries; i++) { - unsigned int oldmode, newmode; + unsigned int newmode; struct cache_entry *ce = istate->cache[i]; int changed; unsigned dirty_submodule = 0; - const struct object_id *old_oid, *new_oid; if (diff_can_quit_early(&revs->diffopt)) break; @@ -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; } diffcore_std(&revs->diffopt); diff_flush(&revs->diffopt);
Refactor out logic that sets up the diff_change call into a helper function for a future patch. Signed-off-by: Calvin Wan <calvinwan@google.com> --- diff-lib.c | 46 +++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 17 deletions(-)