Hi Jaikiran,
"Editing" the value of the comment property, to remove or ignore blanks
for other special
characters makes the code more complex and adds a bunch of conditions.
Dropping
the comment if it doesn't have the allowed format is hard to track down,
because there's
no way to report it was dropped or why.
I would write the value of the comment property using the writeComments
method
so it is encoded the same as the other comment passed as an store argument.
That will handle all special characters and multi-line comments. It is
simpler to specify
and has the same predictable output as other comments.
if the property is non-empty
On 9/12/21 10:29 AM, Jaikiran Pai wrote:
...
This was discussed elsewhere, but after writing that, I'm now
thinking that we **should** honor the property even if it is blank.
It would be useful to disable the timestamp simply by setting the
property to the empty string; this will simply result in an empty
comment line. This would simplify the code (and the spec) by removing
conditions related to String::isBlank.
OK. I don't see any obvious issues with interpreting
empty/whitespace-only value for the system property to translate to an
empty comment line. To be clear, an empty comment line that gets
written out in such cases is *always* going to be the "#" character
followed by a line separator right? Irrespective of what or how many
whitespace characters are present in the property value? Or do you
want those characters to be carried over into that comment line that
gets written out? The reason I ask this is because I think we should
always write just the "#" followed by the line separator character in
such cases, which effectively means we will still need the
String::isBlank check which would then look something like:
if (propVal.isBlank()) {
bw.write("#");
bw.newLine();
}
Side question: do we want to do any escaping or vetting of the
property value?
One of the reasons why we didn't want to use the date in epoch seconds
or a formatted date string was to avoid the complexities that come
with managing that property value. So a free-form property value
seemed more appropriate and I think a free-form value still seems
appropriate, but I think we should keep any vetting to minimal. More
details below.
If for example it contains embedded newlines, this could result in a
malformed property file. Or, if carefully constructed, it could
contain additional property values. I think this is an unintended
consequence of changing the property value from a numeric time value
to a free-form string. We may want to reconsider this.
The specification on the load(Reader reader) method of the
java.util.Properties class has this to say about comment lines (and
lines in general).
(snipped relevant content):
There are two kinds of line, <i>natural lines</i> and <i>logical
lines</i>.
A natural line is defined as a line of characters that is terminated
either
by a set of line terminator characters ({@code \n} or {@code \r} or
{@code \r\n}) or by the end of the stream. A natural line may be
either a blank line, a comment line, or hold all or some of a
key-element pair. A logical
line holds all the data of a key-element pair, which may be spread
out across several adjacent natural lines by escaping
the line terminator sequence with a backslash character
{@code \}. Note that a comment line cannot be extended
in this manner; every natural line that is a comment must have
its own comment indicator, as described below.
With that knowledge about comment lines, I think what we should do is
disallow system property values that contain any form of line
terminator characters (\n or \r or \r\n). If the system property value
is _not_ blank (as specified by String::isBlank) and contains any of
these line terminator characters, we should simply ignore it and write
out the current date as the date comment. That, IMO, should prevent
any of these sneaky/rogue value that can end up either creating
additional property key/values to be written out or causing any
malformed properties file. Plus, that would help us keep the vetting
to minimal without involving too much complexity.
src/java.base/share/classes/java/util/Properties.java line 855:
853: * the value of this system property represents a formatted
854: * date time value that can be parsed back into a {@link
Date} using an appropriate
855: * {@link DateTimeFormatter}
With the property behavior added to normative text above, I don't
think we need this note anymore. We might want to leave something
here about the convention of putting a timestamp here, and an
implicit(?) requirement that it be formatted properly.
The newly updated PR has updated this @implNote to remove some of the
previous text and yet retain some hints on what would be an "ideal"
value for the system property value. But I think we should perhaps
just get rid of this @implNote since we are now proposing to allow
empty comment lines and free form text and this no longer is a "let's
use this system property to allow users to specify a fixed date"
enhancement.
good
-Jaikiran
