Created attachment 85952 [details] [review]
proposed patch without documentation

Documentation will be written if the patch is accepted.

What about shlex?  Seems to do the same in Python...

http://docs.python.org/lib/module-shlex.html

OTOH, g_shell_quote () appears not to have a Python equivalent...

Doh, you are right about shlex package.  I wish you told that _before_ I wrote the patch :-/

Should I then write a patch for wrapping g_shell_quote/g_shell_unquote?  It maybe makes sense to keep g_shell_parse_args then, but OTOH we can add a reference to shlex package in the documentation.

(In reply to comment #4)
> Doh, you are right about shlex package.  I wish you told that _before_ I wrote
> the patch :-/

But I bet it was a good learning experience.. ;-)

> 
> Should I then write a patch for wrapping g_shell_quote/g_shell_unquote?  It
> maybe makes sense to keep g_shell_parse_args then, but OTOH we can add a
> reference to shlex package in the documentation.
> 

g_shell_unquote can be accomplished with shlex as well (will return a list with only one element in that case).

g_shell_quote is fine.

Created attachment 85955 [details] [review]
new patch wrapping g_shell_quote() and g_shell_unquote()

Note that g_shell_unquote() is not the same as shlex.split().  Try e.g. passing string 'foo "bar"' to it (the outer quotes are for Python).  While I don't see a use for this behaviour right away, I think it should be there if only to match its counterpart.

(In reply to comment #6)
> Created an attachment (id=85955) [edit]
> new patch wrapping g_shell_quote() and g_shell_unquote()
> 
> Note that g_shell_unquote() is not the same as shlex.split().  Try e.g. passing
> string 'foo "bar"' to it (the outer quotes are for Python).

Really?  I don't see that:

>>> shlex.split(gobject.shell_quote('foo "bar"'))[0]
'foo "bar"'

Looks correct to me...

>  While I don't see
> a use for this behaviour right away, I think it should be there if only to
> match its counterpart.

OK, I don't have strong objections to wrapping g_shell_unquote, but I still believe it is unnecessary.  jdahlin, jpe, finlay, any objections?

In any case, the patch leaks memory; you must g_free the strings returned by g_shell_quote/unquote.

> Really?  I don't see that:

>>> shlex.split ('foo "bar"') [0]
'foo'
>>> gobject.shell_unquote ('foo "bar"')
'foo bar'

shell_unquote() "flattens" strings bringing quoted and unquoted arguments to the same level.  I don't quite see how this would be useful, but this is sure different from what shlex.split() does.  It is about (but not completely) the same as ' '.join (shlex.split (...)) but this is non-trivial and replaces all whitespace with single space.

> In any case, the patch leaks memory; you must g_free the strings returned by
> g_shell_quote/unquote.

Oops, indeed.  I'll edit the patch.

Created attachment 85957 [details] [review]
fixed patch that should not leak memory

(In reply to comment #8)
> > Really?  I don't see that:
> 
> >>> shlex.split ('foo "bar"') [0]
> 'foo'
> >>> gobject.shell_unquote ('foo "bar"')
> 'foo bar'
> 
> shell_unquote() "flattens" strings bringing quoted and unquoted arguments to
> the same level.  I don't quite see how this would be useful, but this is sure
> different from what shlex.split() does.

I don't see how that would be useful either, and I think glib code is just being stupid.  But, well, if that's what people think they want, it's their problem...

Well, at least it does do what you expect if you pass a single shell-quoted string :)  In that case it is indeed identical to shlex.split (...) [0] only having somewhat more explicit name.

I'm generally against adding bindings for glib functions which have python equivalents in the standard library or are trivially implemented in applications. 
This seems to be just such a case.

shell_quote() is not trivially implemented or at least I don't know a way to do quoting easily.  shell_unquote() is a different case at least for most common usage pattern.

For what it is worth, I consider this a trivial implementation of what it seems that g_shell_quote() is doing:

def shell_quote(s):
    return "'%s'" % s.replace("'", "'\\''")

Can we close this bug now?

It is correct, but I wouldn't consider it trivial.  print shell_quote('\\\'') gives:
'\'\'''

echo '\'\''' gives:
\'

why does echo '\'\''' print \' ? If I were to follow my intuition I would say that echo '\'\''' would be a syntax error, but it apparently works..

I think not having to remember the weird shell quoting rules is worth the extra 64 bytes (yes, I measured) of code size that these two functions are wasting.

Johan, if you're still against the patch then please change the patch status.

(In reply to comment #16)
> Johan, if you're still against the patch then please change the patch status.
> 

I am. But perhaps it's a good time to create a glib module soon.

I've done some work for a glib module:
https://launchpad.net/pyglib

The API wrapped are:
http://snipurl.com/1wczs

The doc covers 99% of the API.

Johan, can you please change the patch status? thanks.

FWIW, this bug should be closed as WONTFIX then, because the patch itself is not problematic as far as I can understand.

Closing as per above.