bug#55952: [PATCH] bindat (strz): Write null terminator after variable length string

[Eli Zaretskii <eliz@gnu.org>]

> Date: Wed, 15 Jun 2022 14:49:58 -0400
> Cc: 55952@debbugs.gnu.org, monnier@iro.umontreal.ca
> From: Richard Hansen <rhansen@rhansen.org>
> 
> >> The existing code advances the index for the terminator, it just 
> >> doesn't write 0 to that byte. So the existing code already signals 
> >> an error in that case unless the `strz` is the final field.
> > 
> > I don't see how this is relevant to the concern I expressed.
> 
> The point I was trying to make is that this patch doesn't change the behavior (in any significant way) in the case of an undersized output string. Perhaps the documentation could be improved, but I'd rather do that in a follow-up patch because it is outside of the scope of this patch.

I never had any objections to changing the behavior, nor in general to
the code changes in your patch.  My comments were only about the
documentation in the manual.

> > My concern is that you found it necessary to add a comment about 
> > writing the terminating null byte (which is a good thing), but didn't 
> > mention that aspect in the manual, not even as a hint.  I think it is 
> > noteworthy enough to be in the manual.
> 
> What do you mean?

I meant that the caveat about having enough space in the output string
for the terminating null byte is not explicitly mentioned where strz
and its handling during packing are documented.

> > Bottom line: I think this aspect of the code is important to mention 
> > in the manual.  The price is small, whereas the benefit could be 
> > significant.
> 
> I disagree -- I think the price is relatively high compared to the benefit. The pre-allocated length requirement is a requirement of only `bindat-pack` -- not of `bindat-type` or any of the type specifiers -- so it is best to keep that requirement with the documentation of `bindat-pack`. Indeed, users are unaware that packing to a pre-allocated string is even possible until they read the documentation for `bindat-pack` (except for references in the caveats documented for fixed-length `str` and `strz`, which I plan on addressing in a future patch).

I don't see any point in continuing to argue about this.  I have my
opinions about how our manuals should explain these issues, and your
disagreement, which is noted, doesn't change those opinions.  So I
installed your previous version, and then made the changes in the
manual I thought were pertinent.

Thanks.