@matheusbsbs / matheus-bagliano / commits / 25bd3acd04

diff: drop useless return from run_diff_{files,index} functions

Neither of these functions ever returns a value other than zero. Instead, they expect unrecoverable errors to exit immediately, and things like "--exit-code" are stored inside the diff_options struct to be handled later via diff_result_code(). Some callers do check the return values, but many don't bother. Let's drop the useless return values, which are misleading callers about how the functions work. This could be seen as a step in the wrong direction, as we might want to eventually "lib-ify" these to more cleanly return errors up the stack, in which case we'd have to add the return values back in. But there are some benefits to doing this now: 1. In the current code, somebody could accidentally add a "return -1" to one of the functions, which would be erroneously ignored by many callers. By removing the return code, the compiler can notice the mismatch and force the developer to decide what to do. Obviously the other option here is that we could start consistently checking the error code in every caller. But it would be dead code, and we wouldn't get any compile-time help in catching new cases. 2. It communicates the situation to callers, who may want to choose a different function. These functions are really thin wrappers for doing git-diff-files and git-diff-index within the process. But callers who care about recovering from an error here are probably better off using the underlying library functions, many of which do return errors. If somebody eventually wants to teach these functions to propagate errors, they'll have to switch back to returning a value, effectively reverting this patch. But at least then they will be starting with a level playing field: they know that they will need to inspect each caller to see how it should handle the error. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Aug 21, 2023 at 16:18 UTC 25bd3acd048152dcf2de5f446d2bd21b5fb42b09

10 files changed +27 -33

builtin/add.c

+1 -2

@@ -194,8 +194,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)

194 out = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);

195 rev.diffopt.file = xfdopen(out, "w");

196 rev.diffopt.close_file = 1;

197 - if (run_diff_files(&rev, 0))

198 - die(_("Could not write patch"));

197 + run_diff_files(&rev, 0);

198

199 if (launch_editor(file, NULL, NULL))

200 die(_("editing patch failed"));

builtin/describe.c

+3 -3

@@ -668,7 +668,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)

668 struct lock_file index_lock = LOCK_INIT;

669 struct rev_info revs;

670 struct strvec args = STRVEC_INIT;

671 - int fd, result;

671 + int fd;

672

673 setup_work_tree();

674 prepare_repo_settings(the_repository);

@@ -685,9 +685,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)

685 strvec_pushv(&args, diff_index_args);

686 if (setup_revisions(args.nr, args.v, &revs, NULL) != 1)

687 BUG("malformed internal diff-index command line");

688 - result = run_diff_index(&revs, 0);

688 + run_diff_index(&revs, 0);

689

690 - if (!diff_result_code(&revs.diffopt, result))

690 + if (!diff_result_code(&revs.diffopt, 0))

691 suffix = NULL;

692 else

693 suffix = dirty;

builtin/diff-files.c

+2 -2

@@ -82,8 +82,8 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)

83 if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)

84 die_errno("repo_read_index_preload");

85 - result = run_diff_files(&rev, options);

86 - result = diff_result_code(&rev.diffopt, result);

85 + run_diff_files(&rev, options);

86 + result = diff_result_code(&rev.diffopt, 0);

87 release_revisions(&rev);

88 return result;

89 }

builtin/diff-index.c

+2 -2

@@ -72,8 +72,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)

72 perror("repo_read_index");

73 return -1;

74 }

75 - result = run_diff_index(&rev, option);

76 - result = diff_result_code(&rev.diffopt, result);

75 + run_diff_index(&rev, option);

76 + result = diff_result_code(&rev.diffopt, 0);

77 release_revisions(&rev);

78 return result;

79 }

builtin/diff.c

+4 -2

@@ -168,7 +168,8 @@ static int builtin_diff_index(struct rev_info *revs,

168 } else if (repo_read_index(the_repository) < 0) {

169 die_errno("repo_read_cache");

170 }

171 - return run_diff_index(revs, option);

171 + run_diff_index(revs, option);

172 + return 0;

173 }

174

175 static int builtin_diff_tree(struct rev_info *revs,

@@ -289,7 +290,8 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv

290 0) < 0) {

291 die_errno("repo_read_index_preload");

292 }

292 - return run_diff_files(revs, options);

293 + run_diff_files(revs, options);

294 + return 0;

295 }

296

297 struct symdiff {

builtin/stash.c

+5 -9

@@ -1089,7 +1089,6 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,

1089 */

1090 static int check_changes_tracked_files(const struct pathspec *ps)

1091 {

1092 - int result;

1092 struct rev_info rev;

1093 struct object_id dummy;

1094 int ret = 0;

@@ -1111,14 +1110,14 @@ static int check_changes_tracked_files(const struct pathspec *ps)

1110 add_head_to_pending(&rev);

1111 diff_setup_done(&rev.diffopt);

1112

1114 - result = run_diff_index(&rev, DIFF_INDEX_CACHED);

1115 - if (diff_result_code(&rev.diffopt, result)) {

1113 + run_diff_index(&rev, DIFF_INDEX_CACHED);

1114 + if (diff_result_code(&rev.diffopt, 0)) {

1115 ret = 1;

1116 goto done;

1117 }

1118

1120 - result = run_diff_files(&rev, 0);

1121 - if (diff_result_code(&rev.diffopt, result)) {

1119 + run_diff_files(&rev, 0);

1120 + if (diff_result_code(&rev.diffopt, 0)) {

1121 ret = 1;

1122 goto done;

1123 }

@@ -1309,10 +1308,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps

1308

1309 add_pending_object(&rev, parse_object(the_repository, &info->b_commit),

1310 "");

1312 - if (run_diff_index(&rev, 0)) {

1313 - ret = -1;

1314 - goto done;

1315 - }

1311 + run_diff_index(&rev, 0);

1312

1313 cp_upd_index.git_cmd = 1;

1314 strvec_pushl(&cp_upd_index.args, "update-index",

builtin/submodule--helper.c

+2 -3

@@ -629,7 +629,6 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,

629 char *displaypath;

630 struct strvec diff_files_args = STRVEC_INIT;

631 struct rev_info rev = REV_INFO_INIT;

632 - int diff_files_result;

632 struct strbuf buf = STRBUF_INIT;

633 const char *git_dir;

634 struct setup_revision_opt opt = {

@@ -669,9 +668,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,

668 repo_init_revisions(the_repository, &rev, NULL);

669 rev.abbrev = 0;

670 setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);

672 - diff_files_result = run_diff_files(&rev, 0);

671 + run_diff_files(&rev, 0);

672

674 - if (!diff_result_code(&rev.diffopt, diff_files_result)) {

673 + if (!diff_result_code(&rev.diffopt, 0)) {

674 print_status(flags, ' ', path, ce_oid,

675 displaypath);

676 } else if (!(flags & OPT_CACHED)) {

diff-lib.c

+2 -4

@@ -96,7 +96,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,

96 return changed;

97 }

99 -int run_diff_files(struct rev_info *revs, unsigned int option)

99 +void run_diff_files(struct rev_info *revs, unsigned int option)

100 {

101 int entries, i;

102 int diff_unmerged_stage = revs->max_count;

@@ -272,7 +272,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)

272 diffcore_std(&revs->diffopt);

273 diff_flush(&revs->diffopt);

274 trace_performance_since(start, "diff-files");

275 - return 0;

275 }

276

277 /*

@@ -606,7 +605,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)

605 free_commit_list(merge_bases);

606 }

607

609 -int run_diff_index(struct rev_info *revs, unsigned int option)

608 +void run_diff_index(struct rev_info *revs, unsigned int option)

609 {

610 struct object_array_entry *ent;

611 int cached = !!(option & DIFF_INDEX_CACHED);

@@ -640,7 +639,6 @@ int run_diff_index(struct rev_info *revs, unsigned int option)

639 diffcore_std(&revs->diffopt);

640 diff_flush(&revs->diffopt);

641 trace_performance_leave("diff-index");

643 - return 0;

642 }

643

644 int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)

diff.h

+2 -2

@@ -637,11 +637,11 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);

637 #define DIFF_SILENT_ON_REMOVED 01

638 /* report racily-clean paths as modified */

639 #define DIFF_RACY_IS_MODIFIED 02

640 -int run_diff_files(struct rev_info *revs, unsigned int option);

640 +void run_diff_files(struct rev_info *revs, unsigned int option);

641

642 #define DIFF_INDEX_CACHED 01

643 #define DIFF_INDEX_MERGE_BASE 02

644 -int run_diff_index(struct rev_info *revs, unsigned int option);

644 +void run_diff_index(struct rev_info *revs, unsigned int option);

645

646 int do_diff_cache(const struct object_id *, struct diff_options *);

647 int diff_flush_patch_id(struct diff_options *, struct object_id *, int);

wt-status.c

+4 -4

@@ -2580,8 +2580,8 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)

2580 }

2581 rev_info.diffopt.flags.quick = 1;

2582 diff_setup_done(&rev_info.diffopt);

2583 - result = run_diff_files(&rev_info, 0);

2584 - result = diff_result_code(&rev_info.diffopt, result);

2583 + run_diff_files(&rev_info, 0);

2584 + result = diff_result_code(&rev_info.diffopt, 0);

2585 release_revisions(&rev_info);

2586 return result;

2587 }

@@ -2614,8 +2614,8 @@ int has_uncommitted_changes(struct repository *r,

2614 }

2615

2616 diff_setup_done(&rev_info.diffopt);

2617 - result = run_diff_index(&rev_info, DIFF_INDEX_CACHED);

2618 - result = diff_result_code(&rev_info.diffopt, result);

2617 + run_diff_index(&rev_info, DIFF_INDEX_CACHED);

2618 + result = diff_result_code(&rev_info.diffopt, 0);

2619 release_revisions(&rev_info);

2620 return result;

2621 }