| Message ID | 20201108213838.4880-4-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-log: implement new --diff-merge options | expand |
Sergey Organov <sorganov@gmail.com> writes: > Move initialization code related to diffing merges into new > init_diff_merge_revs() function. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > 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. > 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. In any case, I'd move this new definition before its sole caller; that way we do not even need an extra forward-declaration. > static int parse_diff_merge_opts(struct rev_info *revs, const char **argv) { > int argcount; > const char *optarg;
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Move initialization code related to diffing merges into new >> init_diff_merge_revs() function. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> 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. > >> 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
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); + 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; +} + static int parse_diff_merge_opts(struct rev_info *revs, const char **argv) { int argcount; const char *optarg;
Move initialization code related to diffing merges into new init_diff_merge_revs() function. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- revision.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)