Re: [PATCH 1/5] "diff --check" should affect exit status
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:59
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH 1/5] "diff --check" should affect exit status · Wincent Colaiuta <hidden>
- 2016-06-15 · Re: [PATCH 1/5] "diff --check" should affect exit status · Junio C Hamano <hidden>
Junio C Hamano [off-list ref] writes:
I think highjacking the "did we encounter problems" return value of the entire callchain for the purpose of checkdiff is very ugly and wrong to begin with,...
How about this on top of your 1/5? It mostly is about reverting the damage to the higher level callchain. Instead, builtin_checkdiff() inspects the checkdiff data and sets the CHECK_FAILED flag to the diffopt structure. The callers are already checking that flag so there is nothing to change for them. --
diff --git a/diff.c b/diff.c
index 39109a6..fc496bf 100644
--- a/diff.c
+++ b/diff.c@@ -1456,7 +1456,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b, diff_free_filespec_data(two); } -static int builtin_checkdiff(const char *name_a, const char *name_b, +static void builtin_checkdiff(const char *name_a, const char *name_b, struct diff_filespec *one, struct diff_filespec *two, struct diff_options *o) {
@@ -1464,7 +1464,7 @@ static int builtin_checkdiff(const char *name_a, const char *name_b, struct checkdiff_t data; if (!two) - return 0; + return; memset(&data, 0, sizeof(data)); data.xm.consume = checkdiff_consume;
@@ -1493,7 +1493,8 @@ static int builtin_checkdiff(const char *name_a, const char *name_b, free_and_return: diff_free_filespec_data(one); diff_free_filespec_data(two); - return data.status; + if (data.status) + DIFF_OPT_SET(o, CHECK_FAILED); } struct diff_filespec *alloc_filespec(const char *path)
@@ -2078,14 +2079,14 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o, builtin_diffstat(name, other, p->one, p->two, diffstat, o, complete_rewrite); } -static int run_checkdiff(struct diff_filepair *p, struct diff_options *o) +static void run_checkdiff(struct diff_filepair *p, struct diff_options *o) { const char *name; const char *other; if (DIFF_PAIR_UNMERGED(p)) { /* unmerged */ - return 0; + return; } name = p->one->path;
@@ -2094,7 +2095,7 @@ static int run_checkdiff(struct diff_filepair *p, struct diff_options *o) diff_fill_sha1_info(p->one); diff_fill_sha1_info(p->two); - return builtin_checkdiff(name, other, p->one, p->two, o); + builtin_checkdiff(name, other, p->one, p->two, o); } void diff_setup(struct diff_options *options)
@@ -2596,17 +2597,17 @@ static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o, run_diffstat(p, o, diffstat); } -static int diff_flush_checkdiff(struct diff_filepair *p, +static void diff_flush_checkdiff(struct diff_filepair *p, struct diff_options *o) { if (diff_unmodified_pair(p)) - return 0; + return; if ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) || (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode))) - return 0; /* no tree diffs in patch format */ + return; /* no tree diffs in patch format */ - return run_checkdiff(p, o); + run_checkdiff(p, o); } int diff_queue_is_empty(void)
@@ -2725,19 +2726,16 @@ static int check_pair_status(struct diff_filepair *p) } } -static int flush_one_pair(struct diff_filepair *p, struct diff_options *opt) +static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt) { int fmt = opt->output_format; if (fmt & DIFF_FORMAT_CHECKDIFF) - return diff_flush_checkdiff(p, opt); + diff_flush_checkdiff(p, opt); else if (fmt & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS)) diff_flush_raw(p, opt); else if (fmt & DIFF_FORMAT_NAME) write_name_quoted(p->two->path, stdout, opt->line_termination); - - /* return value only matters with DIFF_FORMAT_CHECKDIFF */ - return 0; } static void show_file_mode_name(const char *newdelete, struct diff_filespec *fs)
@@ -2976,8 +2974,8 @@ void diff_flush(struct diff_options *options) DIFF_FORMAT_CHECKDIFF)) { for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (check_pair_status(p) && flush_one_pair(p, options)) - DIFF_OPT_SET(options, CHECK_FAILED); + if (check_pair_status(p)) + flush_one_pair(p, options); } separator++; }
--
1.5.4.rc0.1.g37d0