git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/4] diff-lib: refactor functions

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 23, 2022, 20:36 UTC
Message-ID
<xmqqleq9u8u3.fsf@gitster.g>
In-Reply-To
<20220922232947.631309-4-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
Show 48 quoted lines
> Flatten out the if statements in match_stat_with_submodule so the
> logic is more readable and easier for future patches to add to.
>
> Move code that updates relevant variables from the end of
> run_diff_files to finish_run_diff_files. A future patch will utilize
> said function.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  diff-lib.c | 71 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index 7eb66a417a..2e148b79e6 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -73,21 +73,50 @@ 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 = diffopt->flags;
> +	if (!S_ISGITLINK(ce->ce_mode))
> +		goto cleanup;
> +	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;
>  	return changed;
>  }

Unlike the original, this always makes two needless structure assignments for anything that is not a submodule.

Can we fix that regression before moving forward?

Even when ce_mode is a gitlink, if .ignore_submodules bit is set, the two structure assignments between diffopt->flags and orig_flags become necessary, so the original was already doing extra copies but we do not have to make it worse.

Show 25 quoted lines
> +static void finish_run_diff_files(struct rev_info *revs,
> +						  struct cache_entry *ce,
> +						  struct index_state *istate,
> +						  int changed, int dirty_submodule,
> +						  unsigned int newmode)
> +{
> +	unsigned int oldmode;
> +	const struct object_id *old_oid, *new_oid;
> +
> +	if (!changed && !dirty_submodule) {
> +			ce_mark_uptodate(ce);
> +			if (!S_ISGITLINK(ce->ce_mode))
> +				mark_fsmonitor_valid(istate, ce);
> +			if (!revs->diffopt.flags.find_copies_harder)
> +				return;
> +		}
> +		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);
> +}

Strange indentation. It is unclear why this bottom 1/3 of the loop body of run_diff_files() need to be a separate helper function, while the top 2/3 does not. The resulting loop (below) becomes very hard to follow because the reader cannot tell when diff_change() is called and when it is not.

Overall, I see this change detrimental to diff-lib API at this step in the series. Later steps may show something more rewarding than the downsides we see here, hopefully.

Previous: Calvin WanNext: Calvin Wan
Message 19 of 32 in “submodule: parallelize status”
  1. 0/4 submodule: parallelize statusCalvin Wan, Sep 22, 2022
  2. 1/4 run-command: add pipe_output to run_processes_parallelCalvin Wan, Sep 22, 2022
  3. Ævar Arnfjörð BjarmasonSep 23, 2022
  4. Calvin WanSep 26, 2022
  5. Ævar Arnfjörð BjarmasonSep 27, 2022
  6. Junio C HamanoSep 23, 2022
  7. Calvin WanSep 26, 2022
  8. Junio C HamanoSep 27, 2022
  9. Calvin WanSep 27, 2022
  10. Junio C HamanoSep 27, 2022
  11. Ævar Arnfjörð BjarmasonSep 27, 2022
  12. Calvin WanSep 27, 2022
  13. Ævar Arnfjörð BjarmasonSep 27, 2022
  14. Calvin WanSep 27, 2022
  15. Ævar Arnfjörð BjarmasonSep 28, 2022
  16. Calvin WanSep 29, 2022
  17. 2/4 submodule: move status parsing into functionCalvin Wan, Sep 22, 2022
  18. 3/4 diff-lib: refactor functionsCalvin Wan, Sep 22, 2022
  19. Junio C HamanoSep 23, 2022
  20. Calvin WanSep 26, 2022
  21. 4/4 diff-lib: parallelize run_diff_files for submodulesCalvin Wan, Sep 22, 2022
  22. Ævar Arnfjörð BjarmasonSep 23, 2022
  23. Junio C HamanoSep 24, 2022
  24. Calvin WanSep 26, 2022
  25. Junio C HamanoSep 23, 2022
  26. Calvin WanSep 26, 2022
  27. Phillip WoodSep 25, 2022
  28. Junio C HamanoSep 26, 2022
  29. Calvin WanSep 26, 2022
  30. Emily ShafferSep 27, 2022
  31. Junio C HamanoSep 23, 2022
  32. Calvin WanSep 26, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.