Re: [PATCH v7 6/7] diff-lib: refactor match_stat_with_submodule

[Phillip Wood <phillip.wood123@gmail.com>]

On 08/02/2023 08:18, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Feb 07 2023, Calvin Wan wrote:

> Anyway, having looked at this closely I think this patch should be
> dropped entirely. I don't understand how this refactoring is meant to
> make the end result easier to read, reason about, or how it helps the
> subsequent patch.

That's my feeling too c.f. 
<19f91fea-a2a9-7dc6-d940-cc10f384fe76@dunelm.org.uk>. This patch has 
improved since that comment on v4 but I still think we'd be better off 
without it.

Best Wishes

Phillip


> In addition to the above diff in 7/7 you do (and that's the change this
> is meant to help):
> 	
> 	 static int match_stat_with_submodule(struct diff_options *diffopt,
> 	 				     const struct cache_entry *ce,
> 	 				     struct stat *st, unsigned ce_option,
> 	-				     unsigned *dirty_submodule)
> 	+				     unsigned *dirty_submodule, int *defer_submodule_status,
> 	+				     unsigned *ignore_untracked)
> 	 {
> 	 	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
> 	 	struct diff_flags orig_flags;
> 	+	int defer = 0;
> 	
> 	 	if (!S_ISGITLINK(ce->ce_mode))
> 	-		return changed;
> 	+		goto ret;
> 	
> 	 	orig_flags = diffopt->flags;
> 	 	if (!diffopt->flags.override_submodule_config)
> 	@@ -86,11 +92,20 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
> 	 		goto cleanup;
> 	 	}
> 	 	if (!diffopt->flags.ignore_dirty_submodules &&
> 	-	    (!changed || diffopt->flags.dirty_submodules))
> 	-		*dirty_submodule = is_submodule_modified(ce->name,
> 	+	    (!changed || diffopt->flags.dirty_submodules)) {
> 	+		if (defer_submodule_status && *defer_submodule_status) {
> 	+			defer = 1;
> 	+			*ignore_untracked = diffopt->flags.ignore_untracked_in_submodules;
> 	+		} else {
> 	+			*dirty_submodule = is_submodule_modified(ce->name,
> 	 					 diffopt->flags.ignore_untracked_in_submodules);
> 	+		}
> 	+	}
> 	 cleanup:
> 	 	diffopt->flags = orig_flags;
> 	+ret:
> 	+	if (defer_submodule_status)
> 	+		*defer_submodule_status = defer;
> 	 	return changed;
> 	 }
> 
> But if I rebase out this 6/7 patch and solve the conflict for 7/7 it
> becomes:
> 	
> 	@@ -65,14 +66,20 @@ static int check_removed(const struct index_state *istate, const struct cache_en
> 	  * Return 1 when changes are detected, 0 otherwise. If the DIRTY_SUBMODULES
> 	  * option is set, the caller does not only want to know if a submodule is
> 	  * modified at all but wants to know all the conditions that are met (new
> 	- * commits, untracked content and/or modified content).
> 	+ * commits, untracked content and/or modified content). If
> 	+ * defer_submodule_status bit is set, dirty_submodule will be left to the
> 	+ * caller to set. defer_submodule_status can also be set to 0 in this
> 	+ * function if there is no need to check if the submodule is modified.
> 	  */
> 	 static int match_stat_with_submodule(struct diff_options *diffopt,
> 	 				     const struct cache_entry *ce,
> 	 				     struct stat *st, unsigned ce_option,
> 	-				     unsigned *dirty_submodule)
> 	+				     unsigned *dirty_submodule, int *defer_submodule_status,
> 	+				     unsigned *ignore_untracked)
> 	 {
> 	 	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
> 	+	int defer = 0;
> 	+
> 	 	if (S_ISGITLINK(ce->ce_mode)) {
> 	 		struct diff_flags orig_flags = diffopt->flags;
> 	 		if (!diffopt->flags.override_submodule_config)
> 	@@ -80,11 +87,20 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
> 	 		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);
> 	+			 (!changed || diffopt->flags.dirty_submodules)) {
> 	+			if (defer_submodule_status && *defer_submodule_status) {
> 	+				defer = 1;
> 	+				*ignore_untracked = diffopt->flags.ignore_untracked_in_submodules;
> 	+			} else {
> 	+				*dirty_submodule = is_submodule_modified(ce->name,
> 	+									 diffopt->flags.ignore_untracked_in_submodules);
> 	+			}
> 	+		}
> 	 		diffopt->flags = orig_flags;
> 	 	}
> 	+
> 	+	if (defer_submodule_status)
> 	+		*defer_submodule_status = defer;
> 	 	return changed;
> 	 }
> 	
> 
> I can see how there's some room for *a* refactoring to reduce the
> subsequent diff, but not by mutch.
> 
> But this commit didn't help at all. This whole "goto ret", and "goto
> cleanup" is just working around the fact that you pulled "orig_flags"
> out of the "if" scope. Normally the de-indentation would be worth it,
> but here it's not. The control flow becomes more complex to reason about
> as a result.
>