{"thread":{"id":"54322","subject":"[PATCH] diff: get rid of redundant 'dense' argument","startedAt":"2020-09-29T12:29:30Z","lastAt":"2020-09-29T18:54:41Z","messageCount":2,"participants":["Sergey Organov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406622","messageId":"20200929113122.14201-1-sorganov@gmail.com","threadId":"54322","inReplyTo":null,"subject":"[PATCH] diff: get rid of redundant 'dense' argument","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-09-29T11:31:22Z","receivedAt":"2020-09-29T12:29:30Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Get rid of 'dense' argument that is redundant for every function that has\n'struct rev_info *rev' argument as well, as the value of 'dense' passed is\nalways taken from 'rev->dense_combined_merges' field.\n\nThe only place where this was not the case is in 'submodule.c' where\n'diff_tree_combined_merge()' was called with '1' for 'dense' argument. However,\nat that call the 'revs' instance used is local to the function, and we now just\nset 'revs->dense_combined_merges' to 1 in this local instance.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n builtin/diff.c |  3 +--\n combine-diff.c | 21 +++++++++------------\n diff-lib.c     |  6 ++----\n diff.h         |  6 +++---\n log-tree.c     |  2 +-\n submodule.c    |  3 ++-\n 6 files changed, 18 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex cb98811c21db..cd4083fed96e 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -203,8 +203,7 @@ static int builtin_diff_combined(struct rev_info *revs,\n \t\trevs->dense_combined_merges = revs->combine_merges = 1;\n \tfor (i = 1; i < ents; i++)\n \t\toid_array_append(&parents, &ent[i].item->oid);\n-\tdiff_tree_combined(&ent[0].item->oid, &parents,\n-\t\t\t   revs->dense_combined_merges, revs);\n+\tdiff_tree_combined(&ent[0].item->oid, &parents, revs);\n \toid_array_clear(&parents);\n \treturn 0;\n }\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 002e0e5438bc..555b812a9975 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -923,7 +923,6 @@ static void dump_quoted_path(const char *head,\n \n static void show_combined_header(struct combine_diff_path *elem,\n \t\t\t\t int num_parent,\n-\t\t\t\t int dense,\n \t\t\t\t struct rev_info *rev,\n \t\t\t\t const char *line_prefix,\n \t\t\t\t int mode_differs,\n@@ -939,6 +938,7 @@ static void show_combined_header(struct combine_diff_path *elem,\n \tint added = 0;\n \tint deleted = 0;\n \tint i;\n+\tint dense = rev->dense_combined_merges;\n \n \tif (rev->loginfo && !rev->no_commit_id)\n \t\tshow_log(rev);\n@@ -1012,7 +1012,7 @@ static void show_combined_header(struct combine_diff_path *elem,\n }\n \n static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n-\t\t\t    int dense, int working_tree_file,\n+\t\t\t    int working_tree_file,\n \t\t\t    struct rev_info *rev)\n {\n \tstruct diff_options *opt = &rev->diffopt;\n@@ -1145,7 +1145,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t}\n \t}\n \tif (is_binary) {\n-\t\tshow_combined_header(elem, num_parent, dense, rev,\n+\t\tshow_combined_header(elem, num_parent, rev,\n \t\t\t\t     line_prefix, mode_differs, 0);\n \t\tprintf(\"Binary files differ\\n\");\n \t\tfree(result);\n@@ -1200,10 +1200,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t\t     textconv, elem->path, opt->xdl_opts);\n \t}\n \n-\tshow_hunks = make_hunks(sline, cnt, num_parent, dense);\n+\tshow_hunks = make_hunks(sline, cnt, num_parent, rev->dense_combined_merges);\n \n \tif (show_hunks || mode_differs || working_tree_file) {\n-\t\tshow_combined_header(elem, num_parent, dense, rev,\n+\t\tshow_combined_header(elem, num_parent, rev,\n \t\t\t\t     line_prefix, mode_differs, 1);\n \t\tdump_sline(sline, line_prefix, cnt, num_parent,\n \t\t\t   opt->use_color, result_deleted);\n@@ -1284,7 +1284,6 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n  */\n void show_combined_diff(struct combine_diff_path *p,\n \t\t       int num_parent,\n-\t\t       int dense,\n \t\t       struct rev_info *rev)\n {\n \tstruct diff_options *opt = &rev->diffopt;\n@@ -1294,7 +1293,7 @@ void show_combined_diff(struct combine_diff_path *p,\n \t\t\t\t  DIFF_FORMAT_NAME_STATUS))\n \t\tshow_raw_diff(p, num_parent, rev);\n \telse if (opt->output_format & DIFF_FORMAT_PATCH)\n-\t\tshow_patch_diff(p, num_parent, dense, 1, rev);\n+\t\tshow_patch_diff(p, num_parent, 1, rev);\n }\n \n static void free_combined_pair(struct diff_filepair *pair)\n@@ -1454,7 +1453,6 @@ static struct combine_diff_path *find_paths_multitree(\n \n void diff_tree_combined(const struct object_id *oid,\n \t\t\tconst struct oid_array *parents,\n-\t\t\tint dense,\n \t\t\tstruct rev_info *rev)\n {\n \tstruct diff_options *opt = &rev->diffopt;\n@@ -1581,8 +1579,7 @@ void diff_tree_combined(const struct object_id *oid,\n \t\t\t\tprintf(\"%s%c\", diff_line_prefix(opt),\n \t\t\t\t       opt->line_termination);\n \t\t\tfor (p = paths; p; p = p->next)\n-\t\t\t\tshow_patch_diff(p, num_parent, dense,\n-\t\t\t\t\t\t0, rev);\n+\t\t\t\tshow_patch_diff(p, num_parent, 0, rev);\n \t\t}\n \t}\n \n@@ -1600,7 +1597,7 @@ void diff_tree_combined(const struct object_id *oid,\n \tclear_pathspec(&diffopts.pathspec);\n }\n \n-void diff_tree_combined_merge(const struct commit *commit, int dense,\n+void diff_tree_combined_merge(const struct commit *commit,\n \t\t\t      struct rev_info *rev)\n {\n \tstruct commit_list *parent = get_saved_parents(rev, commit);\n@@ -1610,6 +1607,6 @@ void diff_tree_combined_merge(const struct commit *commit, int dense,\n \t\toid_array_append(&parents, &parent->item->object.oid);\n \t\tparent = parent->next;\n \t}\n-\tdiff_tree_combined(&commit->object.oid, &parents, dense, rev);\n+\tdiff_tree_combined(&commit->object.oid, &parents, rev);\n \toid_array_clear(&parents);\n }\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 346fdcf0b0ce..f95c6de75fc8 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -177,9 +177,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\ti--;\n \n \t\t\tif (revs->combine_merges && num_compare_stages == 2) {\n-\t\t\t\tshow_combined_diff(dpath, 2,\n-\t\t\t\t\t\t   revs->dense_combined_merges,\n-\t\t\t\t\t\t   revs);\n+\t\t\t\tshow_combined_diff(dpath, 2, revs);\n \t\t\t\tfree(dpath);\n \t\t\t\tcontinue;\n \t\t\t}\n@@ -361,7 +359,7 @@ static int show_modified(struct rev_info *revs,\n \t\tp->parent[1].status = DIFF_STATUS_MODIFIED;\n \t\tp->parent[1].mode = old_entry->ce_mode;\n \t\toidcpy(&p->parent[1].oid, &old_entry->oid);\n-\t\tshow_combined_diff(p, 2, revs->dense_combined_merges, revs);\n+\t\tshow_combined_diff(p, 2, revs);\n \t\tfree(p);\n \t\treturn 0;\n \t}\ndiff --git a/diff.h b/diff.h\nindex 49242d2733c0..dc6e09a55e62 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -454,11 +454,11 @@ struct combine_diff_path {\n \t\tst_mult(sizeof(struct combine_diff_parent), (n)))\n \n void show_combined_diff(struct combine_diff_path *elem, int num_parent,\n-\t\t\tint dense, struct rev_info *);\n+\t\t\tstruct rev_info *);\n \n-void diff_tree_combined(const struct object_id *oid, const struct oid_array *parents, int dense, struct rev_info *rev);\n+void diff_tree_combined(const struct object_id *oid, const struct oid_array *parents, struct rev_info *rev);\n \n-void diff_tree_combined_merge(const struct commit *commit, int dense, struct rev_info *rev);\n+void diff_tree_combined_merge(const struct commit *commit, struct rev_info *rev);\n \n void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const char *b);\n \ndiff --git a/log-tree.c b/log-tree.c\nindex cb8942fec181..1927f917ce94 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -885,7 +885,7 @@ int log_tree_diff_flush(struct rev_info *opt)\n \n static int do_diff_combined(struct rev_info *opt, struct commit *commit)\n {\n-\tdiff_tree_combined_merge(commit, opt->dense_combined_merges, opt);\n+\tdiff_tree_combined_merge(commit, opt);\n \treturn !opt->loginfo;\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex 543b1123ae12..b3bb59f06644 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -865,7 +865,8 @@ static void collect_changed_submodules(struct repository *r,\n \t\tdiff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n \t\tdiff_rev.diffopt.format_callback = collect_changed_submodules_cb;\n \t\tdiff_rev.diffopt.format_callback_data = &data;\n-\t\tdiff_tree_combined_merge(commit, 1, &diff_rev);\n+\t\tdiff_rev.dense_combined_merges = 1;\n+\t\tdiff_tree_combined_merge(commit, &diff_rev);\n \t}\n \n \treset_revision_walk();\n-- \n2.25.1\n\n"},{"id":"406637","messageId":"xmqqh7rgifbv.fsf@gitster.c.googlers.com","threadId":"54322","inReplyTo":"20200929113122.14201-1-sorganov@gmail.com","subject":"Re: [PATCH] diff: get rid of redundant 'dense' argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-29T18:54:28Z","receivedAt":"2020-09-29T18:54:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Get rid of 'dense' argument that is redundant for every function that has\n> 'struct rev_info *rev' argument as well, as the value of 'dense' passed is\n> always taken from 'rev->dense_combined_merges' field.\n>\n> The only place where this was not the case is in 'submodule.c' where\n> 'diff_tree_combined_merge()' was called with '1' for 'dense' argument. However,\n> at that call the 'revs' instance used is local to the function, and we now just\n> set 'revs->dense_combined_merges' to 1 in this local instance.\n\nInteresting.  This dates back to 91539833 (Log message printout\ncleanups, 2006-04-17), where show_patch_diff() in combine-diff.c\nthat used to take \"struct diff_options *\" was modified to take\n\"struct rev_info *\".  I think the codepath took \"int dense\" from\nthe beginning of the combined diff feature and I suspect this\nchange could have been made in that old commit.\n\nLooking at the history, it seems that it definititely could have\nbeen noticed when I reorganized the codepath involved in combined\nand dense combined patches in 0fe7c1de (built-in diff: assorted\nupdates., 2006-04-29).  Back when the callers of diff_tree_combined()\nwere only a few and it was more obvious that the singleton 'dense'\nwas (or would soon become) redundant.\n\nWill queue.  Thanks.\n\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  builtin/diff.c |  3 +--\n>  combine-diff.c | 21 +++++++++------------\n>  diff-lib.c     |  6 ++----\n>  diff.h         |  6 +++---\n>  log-tree.c     |  2 +-\n>  submodule.c    |  3 ++-\n>  6 files changed, 18 insertions(+), 23 deletions(-)\n>\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index cb98811c21db..cd4083fed96e 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -203,8 +203,7 @@ static int builtin_diff_combined(struct rev_info *revs,\n>  \t\trevs->dense_combined_merges = revs->combine_merges = 1;\n>  \tfor (i = 1; i < ents; i++)\n>  \t\toid_array_append(&parents, &ent[i].item->oid);\n> -\tdiff_tree_combined(&ent[0].item->oid, &parents,\n> -\t\t\t   revs->dense_combined_merges, revs);\n> +\tdiff_tree_combined(&ent[0].item->oid, &parents, revs);\n>  \toid_array_clear(&parents);\n>  \treturn 0;\n>  }\n> diff --git a/combine-diff.c b/combine-diff.c\n> index 002e0e5438bc..555b812a9975 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -923,7 +923,6 @@ static void dump_quoted_path(const char *head,\n>  \n>  static void show_combined_header(struct combine_diff_path *elem,\n>  \t\t\t\t int num_parent,\n> -\t\t\t\t int dense,\n>  \t\t\t\t struct rev_info *rev,\n>  \t\t\t\t const char *line_prefix,\n>  \t\t\t\t int mode_differs,\n> @@ -939,6 +938,7 @@ static void show_combined_header(struct combine_diff_path *elem,\n>  \tint added = 0;\n>  \tint deleted = 0;\n>  \tint i;\n> +\tint dense = rev->dense_combined_merges;\n>  \n>  \tif (rev->loginfo && !rev->no_commit_id)\n>  \t\tshow_log(rev);\n> @@ -1012,7 +1012,7 @@ static void show_combined_header(struct combine_diff_path *elem,\n>  }\n>  \n>  static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n> -\t\t\t    int dense, int working_tree_file,\n> +\t\t\t    int working_tree_file,\n>  \t\t\t    struct rev_info *rev)\n>  {\n>  \tstruct diff_options *opt = &rev->diffopt;\n> @@ -1145,7 +1145,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n>  \t\t}\n>  \t}\n>  \tif (is_binary) {\n> -\t\tshow_combined_header(elem, num_parent, dense, rev,\n> +\t\tshow_combined_header(elem, num_parent, rev,\n>  \t\t\t\t     line_prefix, mode_differs, 0);\n>  \t\tprintf(\"Binary files differ\\n\");\n>  \t\tfree(result);\n> @@ -1200,10 +1200,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n>  \t\t\t\t     textconv, elem->path, opt->xdl_opts);\n>  \t}\n>  \n> -\tshow_hunks = make_hunks(sline, cnt, num_parent, dense);\n> +\tshow_hunks = make_hunks(sline, cnt, num_parent, rev->dense_combined_merges);\n>  \n>  \tif (show_hunks || mode_differs || working_tree_file) {\n> -\t\tshow_combined_header(elem, num_parent, dense, rev,\n> +\t\tshow_combined_header(elem, num_parent, rev,\n>  \t\t\t\t     line_prefix, mode_differs, 1);\n>  \t\tdump_sline(sline, line_prefix, cnt, num_parent,\n>  \t\t\t   opt->use_color, result_deleted);\n> @@ -1284,7 +1284,6 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n>   */\n>  void show_combined_diff(struct combine_diff_path *p,\n>  \t\t       int num_parent,\n> -\t\t       int dense,\n>  \t\t       struct rev_info *rev)\n>  {\n>  \tstruct diff_options *opt = &rev->diffopt;\n> @@ -1294,7 +1293,7 @@ void show_combined_diff(struct combine_diff_path *p,\n>  \t\t\t\t  DIFF_FORMAT_NAME_STATUS))\n>  \t\tshow_raw_diff(p, num_parent, rev);\n>  \telse if (opt->output_format & DIFF_FORMAT_PATCH)\n> -\t\tshow_patch_diff(p, num_parent, dense, 1, rev);\n> +\t\tshow_patch_diff(p, num_parent, 1, rev);\n>  }\n>  \n>  static void free_combined_pair(struct diff_filepair *pair)\n> @@ -1454,7 +1453,6 @@ static struct combine_diff_path *find_paths_multitree(\n>  \n>  void diff_tree_combined(const struct object_id *oid,\n>  \t\t\tconst struct oid_array *parents,\n> -\t\t\tint dense,\n>  \t\t\tstruct rev_info *rev)\n>  {\n>  \tstruct diff_options *opt = &rev->diffopt;\n> @@ -1581,8 +1579,7 @@ void diff_tree_combined(const struct object_id *oid,\n>  \t\t\t\tprintf(\"%s%c\", diff_line_prefix(opt),\n>  \t\t\t\t       opt->line_termination);\n>  \t\t\tfor (p = paths; p; p = p->next)\n> -\t\t\t\tshow_patch_diff(p, num_parent, dense,\n> -\t\t\t\t\t\t0, rev);\n> +\t\t\t\tshow_patch_diff(p, num_parent, 0, rev);\n>  \t\t}\n>  \t}\n>  \n> @@ -1600,7 +1597,7 @@ void diff_tree_combined(const struct object_id *oid,\n>  \tclear_pathspec(&diffopts.pathspec);\n>  }\n>  \n> -void diff_tree_combined_merge(const struct commit *commit, int dense,\n> +void diff_tree_combined_merge(const struct commit *commit,\n>  \t\t\t      struct rev_info *rev)\n>  {\n>  \tstruct commit_list *parent = get_saved_parents(rev, commit);\n> @@ -1610,6 +1607,6 @@ void diff_tree_combined_merge(const struct commit *commit, int dense,\n>  \t\toid_array_append(&parents, &parent->item->object.oid);\n>  \t\tparent = parent->next;\n>  \t}\n> -\tdiff_tree_combined(&commit->object.oid, &parents, dense, rev);\n> +\tdiff_tree_combined(&commit->object.oid, &parents, rev);\n>  \toid_array_clear(&parents);\n>  }\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 346fdcf0b0ce..f95c6de75fc8 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -177,9 +177,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>  \t\t\ti--;\n>  \n>  \t\t\tif (revs->combine_merges && num_compare_stages == 2) {\n> -\t\t\t\tshow_combined_diff(dpath, 2,\n> -\t\t\t\t\t\t   revs->dense_combined_merges,\n> -\t\t\t\t\t\t   revs);\n> +\t\t\t\tshow_combined_diff(dpath, 2, revs);\n>  \t\t\t\tfree(dpath);\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> @@ -361,7 +359,7 @@ static int show_modified(struct rev_info *revs,\n>  \t\tp->parent[1].status = DIFF_STATUS_MODIFIED;\n>  \t\tp->parent[1].mode = old_entry->ce_mode;\n>  \t\toidcpy(&p->parent[1].oid, &old_entry->oid);\n> -\t\tshow_combined_diff(p, 2, revs->dense_combined_merges, revs);\n> +\t\tshow_combined_diff(p, 2, revs);\n>  \t\tfree(p);\n>  \t\treturn 0;\n>  \t}\n> diff --git a/diff.h b/diff.h\n> index 49242d2733c0..dc6e09a55e62 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -454,11 +454,11 @@ struct combine_diff_path {\n>  \t\tst_mult(sizeof(struct combine_diff_parent), (n)))\n>  \n>  void show_combined_diff(struct combine_diff_path *elem, int num_parent,\n> -\t\t\tint dense, struct rev_info *);\n> +\t\t\tstruct rev_info *);\n>  \n> -void diff_tree_combined(const struct object_id *oid, const struct oid_array *parents, int dense, struct rev_info *rev);\n> +void diff_tree_combined(const struct object_id *oid, const struct oid_array *parents, struct rev_info *rev);\n>  \n> -void diff_tree_combined_merge(const struct commit *commit, int dense, struct rev_info *rev);\n> +void diff_tree_combined_merge(const struct commit *commit, struct rev_info *rev);\n>  \n>  void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const char *b);\n>  \n> diff --git a/log-tree.c b/log-tree.c\n> index cb8942fec181..1927f917ce94 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -885,7 +885,7 @@ int log_tree_diff_flush(struct rev_info *opt)\n>  \n>  static int do_diff_combined(struct rev_info *opt, struct commit *commit)\n>  {\n> -\tdiff_tree_combined_merge(commit, opt->dense_combined_merges, opt);\n> +\tdiff_tree_combined_merge(commit, opt);\n>  \treturn !opt->loginfo;\n>  }\n>  \n> diff --git a/submodule.c b/submodule.c\n> index 543b1123ae12..b3bb59f06644 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -865,7 +865,8 @@ static void collect_changed_submodules(struct repository *r,\n>  \t\tdiff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n>  \t\tdiff_rev.diffopt.format_callback = collect_changed_submodules_cb;\n>  \t\tdiff_rev.diffopt.format_callback_data = &data;\n> -\t\tdiff_tree_combined_merge(commit, 1, &diff_rev);\n> +\t\tdiff_rev.dense_combined_merges = 1;\n> +\t\tdiff_tree_combined_merge(commit, &diff_rev);\n>  \t}\n>  \n>  \treset_revision_walk();\n"}]}