Re: [PATCH v8 5/6] diff-lib: refactor out diff_change logic

From: Glen Choo <chooglen@google.com>
Date: Feb 13, 2023, 08:42 UTC
Message-ID: <kl6la61iardd.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230209000212.1892457-6-calvinwan@google.com>

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.

Show 23 quoted lines

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