Re: [JGIT PATCH 11/12] Cleanup Config's MAGIC_EMPTY_VALUE to be more safe

[Robin Rosenberg <robin.rosenberg@dewire.com>]

tisdag 21 juli 2009 22:19:29 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> The magic value "%%magic%%empty%%" is just too magic; if it ever
> did appear as a value in a key string Config would have treated
> it as a true value instead of as a string value.  We also had to
> special case conversions of it to the empty string in a string
> context.  Instead we create a special String object using the
> empty string as a template, and use reference equality against
> that to indicate the magic empty value.
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  .../src/org/spearce/jgit/lib/Config.java           |   28 +++++++++----------
>  1 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Config.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Config.java
> index 974ffea..e4528b1 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Config.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Config.java
> @@ -76,9 +76,14 @@
>  	private Map<String, Object> byName;
>  
>  	/**
> -	 * Magic value indicating a missing entry
> +	 * Magic value indicating a missing entry.
> +	 * <p>
> +	 * This value is tested for reference equality in some contexts, so we
> +	 * must ensure it is a special copy of the empty string.  It also must
> +	 * be treated like the empty string.
>  	 */
> -	private static final String MAGIC_EMPTY_VALUE = "%%magic%%empty%%";
> +	private static final String MAGIC_EMPTY_VALUE = new StringBuilder(0)
> +			.toString();

Can we be sure an implementation doesn't "optimize" toString() here? But an
explicit new String() shouldn't be..?

-- robin



>  	/**
>  	 * The constructor for configuration file
> @@ -293,7 +298,7 @@ public boolean getBoolean(final String section, String subsection,
>  		if (n == null)
>  			return defaultValue;
>  
> -		if (MAGIC_EMPTY_VALUE.equals(n) || "yes".equalsIgnoreCase(n)
> +		if (MAGIC_EMPTY_VALUE == n || "yes".equalsIgnoreCase(n)
>  				|| "true".equalsIgnoreCase(n) || "1".equals(n)
>  				|| "on".equalsIgnoreCase(n)) {
>  			return true;
> @@ -321,11 +326,7 @@ public boolean getBoolean(final String section, String subsection,
>  	 */
>  	public String getString(final String section, String subsection,
>  			final String name) {
> -		String val = getRawString(section, subsection, name);
> -		if (MAGIC_EMPTY_VALUE.equals(val)) {
> -			return "";
> -		}
> -		return val;
> +		return getRawString(section, subsection, name);
>  	}
>  
>  	/**
> @@ -345,16 +346,13 @@ public String getString(final String section, String subsection,
>  		if (o instanceof List) {
>  			final List lst = (List) o;
>  			final String[] r = new String[lst.size()];
> -			for (int i = 0; i < r.length; i++) {
> -				final String val = ((Entry) lst.get(i)).value;
> -				r[i] = MAGIC_EMPTY_VALUE.equals(val) ? "" : val;
> -			}
> +			for (int i = 0; i < r.length; i++)
> +				r[i] = ((Entry) lst.get(i)).value;
>  			return r;
>  		}
>  
>  		if (o instanceof Entry) {
> -			final String val = ((Entry) o).value;
> -			return new String[] { MAGIC_EMPTY_VALUE.equals(val) ? "" : val };
> +			return new String[] { ((Entry) o).value };
>  		}
>  
>  		if (baseConfig != null)
> @@ -700,7 +698,7 @@ protected void printConfig(final PrintWriter r) {
>  				}
>  				r.print(e.name);
>  				if (e.value != null) {
> -					if (!MAGIC_EMPTY_VALUE.equals(e.value)) {
> +					if (MAGIC_EMPTY_VALUE != e.value) {
>  						r.print(" = ");
>  						r.print(escapeValue(e.value));
>  					}