| Message ID | 20230207181706.363453-6-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule: parallelize diff | expand |
Hi Calvin On 07/02/2023 18:17, Calvin Wan wrote: > 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(-) > > 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, I worry that having three integer parameters next to each other makes it very easy to mix them up with out getting any errors from the compiler because the types are all compatible. Could the last two be combined into a flags argument? A similar issues occurs in match_stat_with_submodule() in patch 7 Best Wishes Phillip 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);
> I worry that having three integer parameters next to each other makes it > very easy to mix them up with out getting any errors from the compiler > because the types are all compatible. Could the last two be combined > into a flags argument? A similar issues occurs in > match_stat_with_submodule() in patch 7 I'm not sure how much more I want to engineer a static helper function that is only being called in one other place. I also don't understand what you mean by combining the last two into paramters a flags argument.
Hi Calvin On 08/02/2023 23:12, Calvin Wan wrote: >> I worry that having three integer parameters next to each other makes it >> very easy to mix them up with out getting any errors from the compiler >> because the types are all compatible. Could the last two be combined >> into a flags argument? A similar issues occurs in >> match_stat_with_submodule() in patch 7 > > I'm not sure how much more I want to engineer a static helper function > that is only being called in one other place. I also don't understand what > you mean by combining the last two into paramters a flags argument. Are `dirty_submodule` and `changed` booleans? If so then you can have a single bit flags argument made up of #define SUBMODULE_DIRTY 1 #define SUBMODULE_CHANGED 2 Best Wishes Phillip
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(-)