public inbox for cygwin-apps@cygwin.com
 help / color / mirror / Atom feed
From: Federico Kircheis <federico.kircheis@gmail.com>
To: cygwin-apps@cygwin.com
Subject: Re: cygport development
Date: Sun, 17 May 2020 19:54:56 +0200	[thread overview]
Message-ID: <c89185a0-79ae-96f1-06a0-21916c14f916@gmail.com> (raw)
In-Reply-To: <1e2bd9cc5346a540b1a355847af57476f6ccedab.camel@cygwin.com>

Thank you for the feedback.

> This patch is clearly not limited to the protection of data, as it
> quotes variables that could in no way contain a space or have anything
> to do with file paths. 

Could you please point me to which variables are unrelated to files.

AFAIK i quoted files and arguments, which can all contain whitespace.

For example I did quote ${unpack_file_path}, but not ${unpack_cmd}.

> As mentioned multiple times, using filenames
> ore directories with spaces is asking for trouble, and I have no
> interest in trying to support such a case. 

The first commit makes sure that no information is lost while processing 
file with spaces or other characters that cause globbing. This prevents 
writing or modifying the wrong files, which is what happened to me.

The second commit add exit in case `cd` fails, which prevents other 
errors afterwards.

Those modification do not add support for path with whitespace, as I was 
still unable to compile the software, they did however prevent cygport 
to delete unrelated data (or create data in the wrong location).


> I'm willing to consider a
> *limited* patch that makes sure that cygport doesn't do something it
> shouldn't in that case, but that's about it.

Also because if the underlying tool like `make` does not support spaces, 
it has no benefit.

The most minimal patch I can imagine is exiting if `cd` fails (just the 
second commit).
Would you accept that?
But please also consider my other arguments.

> Yaakov

PS:

A "nice" side-effect to quoting most variables that could contain white 
space is that static-analyzers like shellcheck will emit less 
diagnostic, making it easier to discover potential errors.


  reply	other threads:[~2020-05-17 17:55 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-14 13:25 Federico Kircheis
2019-07-14 17:11 ` Brian Inglis
2019-07-14 19:39   ` [PATCH 1/2] Add support for path with spaces Federico Kircheis
2019-07-14 19:39     ` [PATCH 2/2] Exit in case `cd` fails Federico Kircheis
2019-09-28 11:56   ` cygport development Federico Kircheis
2019-10-13 16:41     ` Achim Gratz
2019-10-14  8:55       ` Federico Kircheis
2019-10-14 17:15         ` Doug Henderson
2020-05-12 14:59         ` Federico Kircheis
2020-05-15  4:55           ` Yaakov Selkowitz
2020-05-17 17:54             ` Federico Kircheis [this message]
2020-05-29  4:38               ` Federico Kircheis
2020-06-12  7:55                 ` Federico Kircheis
2020-06-29 16:04                   ` Federico Kircheis
2021-11-06 14:53                     ` Federico Kircheis

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c89185a0-79ae-96f1-06a0-21916c14f916@gmail.com \
    --to=federico.kircheis@gmail.com \
    --cc=cygwin-apps@cygwin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).