[EditLine] Rewrite GetHistoryFilePath

[JDevlieghere]

Summary

Rewrite the GetHistoryFilePath implementation without relying on FileSpec in the spirit of our discussion in D61994.

[jingham]

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...

[JDevlieghere]

Jim's feedback

[jingham]

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.

[JDevlieghere]

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]

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

[jingham]

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...

[JDevlieghere]

In D62216#1511092, @jingham wrote:

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.

[jingham]

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...

[JDevlieghere]

Add comment describing lldb's behavior regarding history files

[labath]

Looks good to me.

In D62216#1511101, @JDevlieghere wrote:

In D62216#1511092, @jingham wrote:

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...

[JDevlieghere]

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!

lldb/source/Host/common/Editline.cpp

EditlineHistory(const std::string &prefix, uint32_t size, bool unique_entries)
       : m_history(NULL), m_event(), m_prefix(prefix), m_path() {
     m_history = history_winit();
     history_w(m_history, &m_event, H_SETSIZE, size);
     if (unique_entries)
       history_w(m_history, &m_event, H_SETUNIQUE, 1);
   }
 
   const char *GetHistoryFilePath() {
+    // Compute the history path lazily.
     if (m_path.empty() && m_history && !m_prefix.empty()) {
-      FileSpec parent_path("~/.lldb");
-      FileSystem::Instance().Resolve(parent_path);
-      char history_path[PATH_MAX];
-      if (!llvm::sys::fs::create_directory(parent_path.GetPath())) {
-        snprintf(history_path, sizeof(history_path), "~/.lldb/%s-history",
-                 m_prefix.c_str());
+      llvm::SmallString<128> home_directory;
+      llvm::sys::path::home_directory(home_directory);
+
+      llvm::SmallString<128> lldb_history_file = home_directory;
+      llvm::sys::path::append(lldb_history_file, ".lldb");
+      if (!llvm::sys::fs::create_directory(lldb_history_file)) {
+          std::string filename = m_prefix + "-history";
+          llvm::sys::path::append(lldb_history_file, filename);
       } else {
-        snprintf(history_path, sizeof(history_path), "~/%s-widehistory",
-                 m_prefix.c_str());
+          std::string filename = m_prefix + "-widehistory";
+          lldb_history_file = home_directory;
+          llvm::sys::path::append(lldb_history_file, filename);
       }
-      auto file_spec = FileSpec(history_path);
-      FileSystem::Instance().Resolve(file_spec);
-      m_path = file_spec.GetPath();
+      m_path = lldb_history_file.str();
     }
+
     if (m_path.empty())
-      return NULL;
+      return nullptr;
+
     return m_path.c_str();
   }
 
 public:
   ~EditlineHistory() {
     Save();
 
     if (m_history) {