From: Junio C Hamano <gitster@pobox•com>
To: Wincent Colaiuta <win@wincent•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 1/5] "diff --check" should affect exit status
Date: Thu, 13 Dec 2007 18:10:10 -0800 [thread overview]
Message-ID: <7vbq8uxbzx.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <7vir32ywyz.fsf@gitster.siamese.dyndns.org> (Junio C. Hamano's message of "Thu, 13 Dec 2007 15:51:48 -0800")
Junio C Hamano <gitster@pobox•com> 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
prev parent reply other threads:[~2007-12-14 2:11 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-12-13 13:32 [PATCH 0/5] "diff --check" and whitespace enhancements Wincent Colaiuta
2007-12-13 13:32 ` [PATCH 1/5] "diff --check" should affect exit status Wincent Colaiuta
2007-12-13 13:32 ` [PATCH 2/5] New version of pre-commit hook Wincent Colaiuta
2007-12-13 13:32 ` [PATCH 3/5] Unify whitespace checking Wincent Colaiuta
2007-12-13 13:32 ` [PATCH 4/5] Make "diff --check" output match "git apply" Wincent Colaiuta
2007-12-13 13:32 ` [PATCH 5/5] Add tests for "git diff --check" with core.whitespace options Wincent Colaiuta
2007-12-14 0:55 ` Junio C Hamano
2007-12-14 0:12 ` [PATCH 4/5] Make "diff --check" output match "git apply" Junio C Hamano
2007-12-13 13:40 ` [PATCH 3/5] Unify whitespace checking Andreas Ericsson
2007-12-14 0:03 ` Junio C Hamano
2007-12-14 7:36 ` Wincent Colaiuta
2007-12-14 0:17 ` [PATCH 2/5] New version of pre-commit hook Jakub Narebski
2007-12-14 7:24 ` Wincent Colaiuta
2007-12-14 10:23 ` Jakub Narebski
2007-12-13 19:45 ` [PATCH] Don't use the pager when running "git diff --check" Wincent Colaiuta
2007-12-14 4:51 ` Jeff King
2007-12-14 5:11 ` Junio C Hamano
2007-12-14 7:33 ` Junio C Hamano
2007-12-14 7:51 ` Wincent Colaiuta
2007-12-14 7:57 ` Junio C Hamano
2007-12-14 7:47 ` Wincent Colaiuta
2007-12-14 20:54 ` Junio C Hamano
2007-12-13 23:51 ` [PATCH 1/5] "diff --check" should affect exit status Junio C Hamano
2007-12-14 2:10 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vbq8uxbzx.fsf@gitster.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=win@wincent$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox