ywangd / stash

StaSh - Shell for Pythonista
MIT License
1.94k stars 227 forks source link

Strange problem in git #57

Closed jsbain closed 9 years ago

jsbain commented 9 years ago

I'm stumped here. Git add doesn't work within stash. It works from the console, or long pressing play. I put a pdb.set_trace, and traced within stash! and everything seemed to go through fine, until it tried to write to the index. Is there any reason writing files would work within the stash environment? I recall some discussion about not using os.getcwd?

ywangd commented 9 years ago

Could you please give me the steps to reproduce the bug?

If you do not use _stash() to execute command in the git script (which I assume you do not), you don't have to worry about using is.chdir().

On Saturday, 10 January 2015, jsbain notifications@github.com wrote:

I'm stumped here. Git add doesn't work within stash. It works from the console, or long pressing play. I put a pdb.set_trace, and traced within stash! and everything seemed to go through fine, until it tried to write to the index. Is there any reason writing files would work within the stash environment? I recall some discussion about not using os.getcwd?

— Reply to this email directly or view it on GitHub https://github.com/ywangd/stash/issues/57.


                  Life is elsewhere
jsbain commented 9 years ago

Ok found it... Arguments are coming in as unicode. Porcelain has an assert that checks for args as instance of str, which unicode fails. I thought stash usually printed out Exceptions? I think maybe it strips the exception type, so a blank assertion doesn't show as an error?

ywangd commented 9 years ago

@jsbain This is the 2nd time that unicode causes a rather elusive error. First time is @briarfox had problems run python -m ModuleName in stash and it turned out to be and unicode v.s. str issue. Maybe I should add it as a warning to README and probably finally towards a developer document.

Stash only tries to print error messages not exception types. I did this hoping for a more authentic shell taste (by hiding python specific information). But I do agree it is necessary to show more exception information. See details in #59

jsbain commented 9 years ago

The problem is that normally the console, etc produce a string, while a TextView produces unicode by default. I believe ios uses utf-8 as the file system encoding, so unicode doesn't make sense there anyway for most bash things.

I'd suggest that all operations involving inp.text, like readline, get returned as str() or .encode('utf-8'), and any of the piping interfaces, to eliminate this sort of issue. I can't think of any case where a unicode string is what you'd really want, and it breaks the paradigm of being able to use generic python scripts as commands in stash, since sys.argv are str, not unicode. That would also obviate the need to replace unprintables in cat, I think.

On Jan 10, 2015, at 3:46 AM, ywangd notifications@github.com wrote:

@jsbain This is the 2nd time that unicode causes a rather elusive error. First time is @briarfox had problems rung python -m ModuleName in stash and it turned out to be and unicode v.s. str issue. Maybe I should add this warning to README and probably finally towards a developer document.

Stash only tries to print error messages not exception types. I did this hoping for a more authentic shell taste (by hiding python specific information). But I do agree it is necessary to show more exception information. See details in #59

— Reply to this email directly or view it on GitHub.

ywangd commented 9 years ago

@jsbain I thought about convert all strings read from TextField to str type before passing them to external scripts when @briarfox had the issue with python -m. But after some discussions (on Pythonista's forum), the decision was to let them stay as unicode as there may be iOS systems that do not use English as the primary language.

I do agree with you that this issue breaks the paradigm that common python scripts can run in stash without changes. So I am leaning towards to coerce all unicode to str before external script is called. I'd also like to get some inputs from @briarfox and @dgelessus before making the changes. I'll open a new ticket specifically for it.

I do not, however, think the change would not fix the issue with cat, the lines read by cat are all of str type. The issue really is binary v.s. text notunicode v.s. str.

jsbain commented 9 years ago

I think that UTF8 can still encode non English language characters, the str vs unicode is primarily a question of storage. When dealing with files, I guess we'd need to understand how the files are encoded.