From: Junio C Hamano <gitster@pobox•com>
To: Jonathan Tan <jonathantanmy@google•com>
Cc: "Kyle J. McKay" <mackyle@gmail•com>, Jeff King <peff@peff•net>,
Johannes Schindelin <Johannes.Schindelin@gmx•de>,
Git mailing list <git@vger•kernel.org>
Subject: Re: [PATCH] mailinfo.c: move side-effects outside of assert
Date: Mon, 19 Dec 2016 13:01:48 -0800 [thread overview]
Message-ID: <xmqqbmw7ocoz.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <d5690ac7-ff62-99b9-7e7e-929bd7f0433b@google.com> (Jonathan Tan's message of "Mon, 19 Dec 2016 12:54:15 -0800")
Jonathan Tan <jonathantanmy@google•com> writes:
>>> This is obviously an improvement, but it makes me wonder if we should be
>>> doing:
>>>
>>> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
>>> die("BUG: some explanation of why this can never happen");
>>>
>>> which perhaps documents the intended assumptions more clearly. A comment
>>> regarding the side effects might also be helpful.
>>
>> I wondered exactly the same thing myself. I was hoping Jonathan would
>> pipe in here with some analysis about whether this is:
>>
>> a) a super paranoid, just-in-case, can't really ever fail because by
>> the time we get to this code we've already effectively validated
>> everything that could cause check_header to return false in this case
>> ...
> The answer is "a". The only time that mi->inbody_header_accum is
> appended to is in check_inbody_header, and appending onto a blank
> mi->inbody_header_accum always happens when is_inbody_header is true
> (which guarantees a prefix that causes check_header to always return
> true).
>
> Peff's suggestion sounds reasonable to me, maybe with an error message
> like "BUG: inbody_header_accum, if not empty, must always contain a
> valid in-body header".
OK. So we do not expect it to fail, but we still do want the side
effect of that function (i.e. accmulation into the field).
Somebody care to send a final "agreed-upon" version?
next prev parent reply other threads:[~2016-12-19 21:02 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-17 19:54 [PATCH] mailinfo.c: move side-effects outside of assert Kyle J. McKay
2016-12-19 17:45 ` Johannes Schindelin
2016-12-19 20:03 ` Jeff King
2016-12-19 20:38 ` Kyle J. McKay
2016-12-19 20:54 ` Jonathan Tan
2016-12-19 21:01 ` Junio C Hamano [this message]
2016-12-19 23:13 ` [PATCH v2] " Kyle J. McKay
2016-12-19 23:26 ` Junio C Hamano
2016-12-19 23:54 ` [PATCH v3] " Kyle J. McKay
2016-12-20 14:12 ` [PATCH] " Johannes Schindelin
2016-12-20 16:45 ` Jeff King
2016-12-21 5:54 ` Kyle J. McKay
2016-12-21 15:55 ` Jeff King
2016-12-22 2:21 ` Kyle J. McKay
2016-12-22 3:34 ` Jeff King
2016-12-22 17:57 ` Junio C Hamano
2016-12-22 3:53 ` Kyle J. McKay
2016-12-22 3:59 ` Jeff King
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=xmqqbmw7ocoz.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=jonathantanmy@google$(echo .)com \
--cc=mackyle@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
/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