| Message ID | 20221108184200.2813458-5-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule: parallelize diff | expand |
Hi Calven On 08/11/2022 18:41, Calvin Wan wrote: > Flatten out the if statements in match_stat_with_submodule so the > logic is more readable and easier for future patches to add to. > orig_flags didn't need to be set if the cache entry wasn't a > GITLINK so defer setting it. Thanks for splitting this change out. I have to say I find the original quite a bit easier to read. If you're worried about the code added in the next commit being too indented perhaps you could move the body of the if statement into a separate function. Best Wishes Phillip > Signed-off-by: Calvin Wan <calvinwan@google.com> > --- > diff-lib.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/diff-lib.c b/diff-lib.c > index 2edea41a23..f5257c0c71 100644 > --- a/diff-lib.c > +++ b/diff-lib.c > @@ -73,18 +73,25 @@ static int match_stat_with_submodule(struct diff_options *diffopt, > unsigned *dirty_submodule) > { > int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option); > - if (S_ISGITLINK(ce->ce_mode)) { > - struct diff_flags orig_flags = diffopt->flags; > - if (!diffopt->flags.override_submodule_config) > - set_diffopt_flags_from_submodule_config(diffopt, ce->name); > - if (diffopt->flags.ignore_submodules) > - changed = 0; > - else if (!diffopt->flags.ignore_dirty_submodules && > - (!changed || diffopt->flags.dirty_submodules)) > - *dirty_submodule = is_submodule_modified(ce->name, > - diffopt->flags.ignore_untracked_in_submodules); > - diffopt->flags = orig_flags; > + struct diff_flags orig_flags; > + > + if (!S_ISGITLINK(ce->ce_mode)) > + goto ret; > + > + orig_flags = diffopt->flags; > + if (!diffopt->flags.override_submodule_config) > + set_diffopt_flags_from_submodule_config(diffopt, ce->name); > + if (diffopt->flags.ignore_submodules) { > + changed = 0; > + goto cleanup; > } > + if (!diffopt->flags.ignore_dirty_submodules && > + (!changed || diffopt->flags.dirty_submodules)) > + *dirty_submodule = is_submodule_modified(ce->name, > + diffopt->flags.ignore_untracked_in_submodules); > +cleanup: > + diffopt->flags = orig_flags; > +ret: > return changed; > } >
> Thanks for splitting this change out. I have to say I find the original > quite a bit easier to read. If you're worried about the code added in > the next commit being too indented perhaps you could move the body of > the if statement into a separate function. Then we're just swapping if statement depth for function call depth, which seems even worse. I think the confusion comes from adding the "ret:" part which is currently unnecessary so I can get rid of that this patch. Thanks
diff --git a/diff-lib.c b/diff-lib.c index 2edea41a23..f5257c0c71 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -73,18 +73,25 @@ static int match_stat_with_submodule(struct diff_options *diffopt, unsigned *dirty_submodule) { int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option); - if (S_ISGITLINK(ce->ce_mode)) { - struct diff_flags orig_flags = diffopt->flags; - if (!diffopt->flags.override_submodule_config) - set_diffopt_flags_from_submodule_config(diffopt, ce->name); - if (diffopt->flags.ignore_submodules) - changed = 0; - else if (!diffopt->flags.ignore_dirty_submodules && - (!changed || diffopt->flags.dirty_submodules)) - *dirty_submodule = is_submodule_modified(ce->name, - diffopt->flags.ignore_untracked_in_submodules); - diffopt->flags = orig_flags; + struct diff_flags orig_flags; + + if (!S_ISGITLINK(ce->ce_mode)) + goto ret; + + orig_flags = diffopt->flags; + if (!diffopt->flags.override_submodule_config) + set_diffopt_flags_from_submodule_config(diffopt, ce->name); + if (diffopt->flags.ignore_submodules) { + changed = 0; + goto cleanup; } + if (!diffopt->flags.ignore_dirty_submodules && + (!changed || diffopt->flags.dirty_submodules)) + *dirty_submodule = is_submodule_modified(ce->name, + diffopt->flags.ignore_untracked_in_submodules); +cleanup: + diffopt->flags = orig_flags; +ret: return changed; }
Flatten out the if statements in match_stat_with_submodule so the logic is more readable and easier for future patches to add to. orig_flags didn't need to be set if the cache entry wasn't a GITLINK so defer setting it. Signed-off-by: Calvin Wan <calvinwan@google.com> --- diff-lib.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)