Add quotes when comparing strings in configure script

In ticket:16651#comment:28 missing quotes caused parsing errors. The configure script still contains instances of strings being compared without quotes. AFAIK these remaining cases do not cause any problems right now but it is better to fix them now to avoid problems in the future.

changed milestone to %Tor: 0.2.8.x-final in legacy/trac

added TorCoreTeam201603 in Legacy / Trac component::core tor/tor in Legacy / Trac easy in Legacy / Trac milestone::Tor: 0.2.8.x-final in Legacy / Trac owner::zerosion in Legacy / Trac points::small-remaning in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac severity::trivial in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac version::tor 0.2.7.5 in Legacy / Trac labels

Trac:
Version: Tor: unspecified to Tor: 0.2.7.5
Keywords: N/A deleted, TorCoreTeam201512 added
Milestone: N/A to Tor: 0.2.8.x-final

Trac:
Keywords: N/A deleted, easy added

I'd look at a patch for these if we got one, but AFAIK nobody's looking at them right now, and there's no reason to expect them to get done this month.

Trac:
Keywords: TorCoreTeam201512 deleted, N/A added

Trac:
Username: zerosion
Status: new to assigned
Owner: N/A to zerosion

Replying to nickm:

I'd look at a patch for these if we got one, but AFAIK nobody's looking at them right now, and there's no reason to expect them to get done this month.

Ok, I just assigned myself to solve this ticket. This is the first time I contribute using this system, so if I'm doing things wrong I'd like to know.

Trac:
Username: zerosion

Sounds good; just make sure you put the ticket into needs_review once the patch is written.

Trac:
Username: zerosion

0001-Quotes-added-on-configure-script-s-string-comparison.patch

Trac:
Username: zerosion

Trac:
Username: zerosion
Status: assigned to needs_review

Replying to nickm:

Sounds good; just make sure you put the ticket into needs_review once the patch is written. Ok, I think it's done, though I don't have a Windows machine to test it on. Whatever you need me to change, I'm here to help.

Trac:
Username: zerosion

ok; I'll be reviewing tickets again once 0.2.8.1-alpha is out -- probably in a day or two.

I'm curious about why you wrap single-word literal strings in quotes, I'm sure you know this is unnecessary.

I guess it can be argued that it's a bit more future-proof (if later that single-word string has to be turned into a multi-word string, one doesn't have to remember to add the quotes, they are already there); also it looks a bit more regular.

But then again you also remove them in equally superfluous cases where those same arguments would apply.

For example you do:

-if test "$bwin32" = cross; then
+if test "$bwin32" = "cross"; then

But also:

-case "$host_os" in
+case $host_os in

What ho?

Different cypherpunks here.

I agree with [comment:10 comment:10] that strings in switch statements should stay quoted or be quoted if they are not. The nitpicks that i have are to also quote the test parameters at the top of configure.ac; i.e.

if test -f /etc/redhat-release ; then
  if test -f /usr/kerberos/include ; then
    CPPFLAGS="$CPPFLAGS -I/usr/kerberos/include"
  fi
fi

and to add indentation to the lines you have wrapped in your patch for readability; i.e.

-if test x$enable_gcc_warnings = xyes || test x$enable_gcc_warnings_advisory = xyes; then
+if test "x$enable_gcc_warnings" = "xyes" ||
+test "x$enable_gcc_warnings_advisory" = "xyes"; then

becomes

-if test x$enable_gcc_warnings = xyes || test x$enable_gcc_warnings_advisory = xyes; then
+if test "x$enable_gcc_warnings" = "xyes" ||
+  test "x$enable_gcc_warnings_advisory" = "xyes"; then

I'm with you both, I mean, in the case ... in statements I was trying to generalize, but I'd prefer to quote them to avoid errors. Also, I think indentation for readability is a good point.

I'm publishing the patch as soon as I can. Thanks for the advice.

Trac:
Username: zerosion

Trac:
Username: zerosion

0001-Quotes-added-on-configure-script-s-string-comparison.2.patch

Trac:
Username: zerosion

Done. I've seen a lot of syntax irregularities, like tabs mixed with spaces and different indentation along the file. Should we open a ticket to handle this, or it's fine?

Trac:
Username: zerosion

Replying to zerosion:

Done.

Code review:

I think it's good to quote $host in the unlikely event this external info has spaces or special characters. Similarly, I would accept other patches that quoted external or user-specified input.

I think it's ok to quote literal file paths that don't currently have spaces. This is consistent with quoting user-specified paths, in case they have spaces or special characters.

I can't see the point of quoting or changing the quotes around internal variables or internal string literals. These are the majority of the changes in the patch, and I'm not sure if they gain us anything. They need to be weighed against the risk that a misplaced quote breaks the build on some obscure configuration. Was there a reason for these changes?

And finally, if we're going to tidy up whitespace, let's consider that in another ticket. (I'm not sure if we will want to, see my comments below.)

I've seen a lot of syntax irregularities, like tabs mixed with spaces and different indentation along the file. Should we open a ticket to handle this, or it's fine?

Let's open a ticket to document it, but set it to low priority. The issue with re-flowing the entire file is that it makes it hard to tell when the last non-whitespace change was made to each line.

Trac:
Status: needs_review to needs_revision

Replying to teor:

I can't see the point of quoting or changing the quotes around internal variables or internal string literals. These are the majority of the changes in the patch, and I'm not sure if they gain us anything. They need to be weighed against the risk that a misplaced quote breaks the build on some obscure configuration. Was there a reason for these changes? As stated in the ticket description, ticket:16651#comment:28 mentions a case where internal variables included spaces and caused issues. This was the reason for opening this ticket in order to prevent similar instances from causing problems in the future.

The issue with re-flowing the entire file is that it makes it hard to tell when the last non-whitespace change was made to each line. I believe git has options to ignore whitespace changes so they do not interfere when tracking down changes. Furthermore, the tor codebase has numerous commits which purely fix whitespace problems to appease make check-spaces. In these cases change traceability isn't taken into account either so IMO we shouldn't start with it now.

Replying to cypherpunks:

Replying to teor:

I can't see the point of quoting or changing the quotes around internal variables or internal string literals. These are the majority of the changes in the patch, and I'm not sure if they gain us anything. They need to be weighed against the risk that a misplaced quote breaks the build on some obscure configuration. Was there a reason for these changes? As stated in the ticket description, ticket:16651#comment:28 mentions a case where internal variables included spaces and caused issues. This was the reason for opening this ticket in order to prevent similar instances from causing problems in the future.

I understand now. I'd be happy for the patch to be merged as-is.

The issue with re-flowing the entire file is that it makes it hard to tell when the last non-whitespace change was made to each line. I believe git has options to ignore whitespace changes so they do not interfere when tracking down changes. Furthermore, the tor codebase has numerous commits which purely fix whitespace problems to appease make check-spaces. In these cases change traceability isn't taken into account either so IMO we shouldn't start with it now.

Fair enough. Please make whitespace changes in a separate commit. (And ideally on another ticket.)

If we can work out a way to keep the whitespace consistent, ideally using make check-spaces, then that will help us ensure that the initial change is correct, and that future changes don't re-introduce whitespace issues.

Trac:
Status: needs_revision to needs_review

Trac:
Points: N/A to small-remaning

Merged; and added a changes file. Thanks!

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

2 are missing still :

~/devel/tor $ grep -e ' xyes' -e ' xno' configure.ac
AM_CONDITIONAL(UNITTESTS_ENABLED, test x$enable_unittests != xno)
AM_CONDITIONAL(COVERAGE_ENABLED, test x$enable_coverage = xyes)

Trac:
Reviewer: N/A to N/A

Trac:
Resolution: implemented to N/A
Status: closed to reopened

Let's merge after 0.2.8-rc

Trac:
Status: reopened to needs_review

Trac:
Keywords: N/A deleted, TorCoreTeam201603 added

Fixed in c8eb39d6. (Those lines were redundant with the two before them, so I just removed them.)

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

moved from legacy/trac#17744 (moved)

added Bug First Contribution labels and removed 1 deleted label

removed 1 deleted label