Page MenuHomePhabricator

[EditLine] Rewrite GetHistoryFilePath
ClosedPublic

Authored by JDevlieghere on May 21 2019, 12:55 PM.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

JDevlieghere created this revision.May 21 2019, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2019, 12:55 PM

Why are we calling this "widehistory" because we couldn't write into the .lldb directory? I know that's the way it was but it makes no sense. I guess it would make sense to call it widehistory if edit line was in wchar mode, that way you wouldn't ask a non wchar edit line to read a wchar history file...

Jim's feedback

jingham added a comment.EditedMay 21 2019, 3:04 PM

I don't think that's quite right. I think the behavior should be:

1) Try to make a .lldb directory

    a) If you can, then
        i) if wchar support
               ~/.lldb/.lldb-widehistory
        ii) else
              ~/.lldb/lldb-history
   b) else
       i) if wchar support
            ~/.lldb-widehistory
      ii) else
            ~/.lldb-history

Unless I'm reading it wrong, your patch bails in case b.

I figured if we're changing this anyway, we could as well change the behavior. I think it's reasonable to not have history when we cannot create the .lldb directory. Before I wiped my directory, I saw several (5?) files in .lldb and I'm not convinced we should pollute people's home directory with them. In reality, how likely is it anyway that we can write to ~ but not create a directory?

davide added a subscriber: davide.May 21 2019, 3:17 PM

What if ~ is not writable either? I think we shouldn't fallback.

Most other programs write their history files in ~. So we are being a little odd in offering to put them in ~/.lldb, though I agree that is convenient.

But if putting files in ~/.lldb ticked somebody off enough that they made a .lldb directory that was read only, your create_directory would fail - since it explicitly asks for x & w - and I don't think we should punish them with no history...

If ~ is not writeable, then for now we should return an empty path and not do history.

Note, however, for history files a better solution would be to allow a way to specify the history location. Right now, if you run multiple lldb sessions at the same time they fight over the history file, which isn't great. If folks could set the history file they could assign different history files to different sessions. That would also be a way to work around a read-only or non-existent ~. But that is out of scope for this patch...

Most other programs write their history files in ~. So we are being a little odd in offering to put them in ~/.lldb, though I agree that is convenient.

But if putting files in ~/.lldb ticked somebody off enough that they made a .lldb directory that was read only, your create_directory would fail - since it explicitly asks for x & w - and I don't think we should punish them with no history...
If ~ is not writeable, then for now we should return an empty path and not do history.

This sounds contrived at best. Who would even expect that there would be a fallback for that? If somebody really did create a read-only .lldb directory, it sounds to me like they don't want history at all.

Note, however, for history files a better solution would be to allow a way to specify the history location. Right now, if you run multiple lldb sessions at the same time they fight over the history file, which isn't great. If folks could set the history file they could assign different history files to different sessions. That would also be a way to work around a read-only or non-existent ~. But that is out of scope for this patch...

Yup, that sounds like a good future direction.

Then put in a comment saying something like "LLDB ONLY stores history files in .lldb and if you don't like that..." If you are instituting a policy which is not common you should at least document it...

Add comment describing lldb's behavior regarding history files

labath accepted this revision.May 22 2019, 1:28 AM

Looks good to me.

Most other programs write their history files in ~. So we are being a little odd in offering to put them in ~/.lldb, though I agree that is convenient.

But if putting files in ~/.lldb ticked somebody off enough that they made a .lldb directory that was read only, your create_directory would fail - since it explicitly asks for x & w - and I don't think we should punish them with no history...
If ~ is not writeable, then for now we should return an empty path and not do history.

This sounds contrived at best. Who would even expect that there would be a fallback for that? If somebody really did create a read-only .lldb directory, it sounds to me like they don't want history at all.

My thoughts exactly. :)

Also, when we say "no history", we mean "no persistent history", right? I'd expect one would still be able to use the history of commands typed in the current session in this scenario...

This revision is now accepted and ready to land.May 22 2019, 1:28 AM

Also, when we say "no history", we mean "no persistent history", right? I'd expect one would still be able to use the history of commands typed in the current session in this scenario...

Yup!

This revision was automatically updated to reflect the committed changes.