git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] mailinfo.c: move side-effects outside of assert

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 19, 2016, 21:01 UTC
Message-ID
<xmqqbmw7ocoz.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<d5690ac7-ff62-99b9-7e7e-929bd7f0433b@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 25 quoted lines
>>> 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?
Previous: Jonathan TanNext: Kyle J. McKay
Message 6 of 18 in “mailinfo.c: move side-effects outside of assert”
  1. mailinfo.c: move side-effects outside of assertKyle J. McKay, Dec 17, 2016
  2. Johannes SchindelinDec 19, 2016
  3. Jeff KingDec 19, 2016
  4. Kyle J. McKayDec 19, 2016
  5. Jonathan TanDec 19, 2016
  6. Junio C HamanoDec 19, 2016
  7. mailinfo.c: move side-effects outside of assertKyle J. McKay, Dec 19, 2016
  8. Junio C HamanoDec 19, 2016
  9. mailinfo.c: move side-effects outside of assertKyle J. McKay, Dec 19, 2016
  10. Johannes SchindelinDec 20, 2016
  11. Jeff KingDec 20, 2016
  12. Kyle J. McKayDec 21, 2016
  13. Jeff KingDec 21, 2016
  14. Kyle J. McKayDec 22, 2016
  15. Jeff KingDec 22, 2016
  16. Junio C HamanoDec 22, 2016
  17. Kyle J. McKayDec 22, 2016
  18. Jeff KingDec 22, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.