Re: [PATCH v1 03/27] revision: factor out initialization of diff-merge related settings

Junio C Hamano [off-list ref] writes:

Sergey Organov [off-list ref] writes:

quoted

Move initialization code related to diffing merges into new
init_diff_merge_revs() function.

Signed-off-by: Sergey Organov <redacted>
---
 revision.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 739295bb9ff4..bc568fb79778 100644
--- a/revision.c
+++ b/revision.c

@@ -1805,6 +1805,8 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 	return 1;
 }
 
+static void init_diff_merge_revs(struct rev_info *revs);
+

It is not wrong per-se, but I do not see why we do not define the
function here, not just forward-declare it like so.

I wanted to keep merge-diff functions definitions at one place to
simplify future refactoring by moving all of them into separate file, to
avoid scatter-gathering definitions from multiple places in the
original file.

quoted

 void repo_init_revisions(struct repository *r,
 			 struct rev_info *revs,
 			 const char *prefix)

@@ -1813,7 +1815,7 @@ void repo_init_revisions(struct repository *r,
 
 	revs->repo = r;
 	revs->abbrev = DEFAULT_ABBREV;
-	revs->ignore_merges = -1;
+	init_diff_merge_revs(revs);
 	revs->simplify_history = 1;
 	revs->pruning.repo = r;
 	revs->pruning.flags.recursive = 1;

@@ -2153,6 +2155,10 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
 	add_grep(revs, pattern, GREP_PATTERN_BODY);
 }
 
+static void init_diff_merge_revs(struct rev_info *revs) {
+	revs->ignore_merges = -1;
+}

Style.  A brace that begins a function body begins its own line.

Oops! There are a few cases in the git sources where this style is used,
and somehow my eye catch it and I did entire series in this style.

Now the question is: what do I do about it? Do I need to rework the
entire series in the correct style, or would it be enough to fix the
style as a separate commit at the end of the series?

In any case, I'd move this new definition before its sole caller; that
way we do not even need an extra forward-declaration.

I'd do it too, except in this particular case, first, it actually makes
some sense to have function definition here, see above, and second, it'd
disappear along with declaration in the next commit anyway. I'm OK to
change this though if you still find this issue essential.

Thanks,
-- Sergey Organov