Skip to content

Add history persistence to file#23

Open
tobia wants to merge 2 commits into
soveran:masterfrom
tobia:master
Open

Add history persistence to file#23
tobia wants to merge 2 commits into
soveran:masterfrom
tobia:master

Conversation

@tobia
Copy link
Copy Markdown

@tobia tobia commented Feb 23, 2020

Hey there. Thank you for this great tool. I added history saving and loading to file (~/.local/share/clac/history, configurable with XDG env) because I often need to review or amend previous calculations, so I guess other people would find it useful too. Besides, I didn't do much, because Linenoise already has the required functions.

Copy link
Copy Markdown

@waltertross waltertross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry, still not used to github)

Copy link
Copy Markdown

@waltertross waltertross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleared my previous all too hasty review. This one I'm happy with.

Comment thread clac.c

/* Config */
#define BUFFER_MAX 1024
#define HIST_DIR "clac"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define HIST_DIR "clac"
#define CLAC_DIR "clac"

Comment thread clac.c

static sds history_config() {
sds directory = NULL;
sds filename = NULL;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sds filename = NULL;
sds filename = NULL;
int directory_ok;

Comment thread clac.c
sds filename = NULL;

if (getenv("XDG_DATA_HOME") != NULL) {
directory = buildpath("%s/%s", getenv("XDG_DATA_HOME"), HIST_DIR);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
directory = buildpath("%s/%s", getenv("XDG_DATA_HOME"), HIST_DIR);
directory = buildpath("%s/%s", getenv("XDG_DATA_HOME"), CLAC_DIR);

Comment thread clac.c
directory = buildpath("%s/%s", getenv("XDG_DATA_HOME"), HIST_DIR);
filename = buildpath("%s/%s", getenv("XDG_DATA_HOME"), HIST_FILE);
} else if (getenv("HOME") != NULL) {
directory = buildpath("%s/.local/share/%s", getenv("HOME"), HIST_DIR);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
directory = buildpath("%s/.local/share/%s", getenv("HOME"), HIST_DIR);
directory = buildpath("%s/.local/share/%s", getenv("HOME"), CLAC_DIR);

Comment thread clac.c
return NULL;
}

mkdir(directory, 0700);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mkdir(directory, 0700);
directory_ok = mkdir(directory, 0700) == 0 || errno == EEXIST;

Comment thread clac.c
mkdir(directory, 0700);
sdsfree(directory);

return filename;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return filename;
return directory_ok ? filename : NULL;

Comment thread clac.c
#include <errno.h>
#include <math.h>
#include <sys/stat.h>
#include <sys/types.h>
Copy link
Copy Markdown

@waltertross waltertross Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we delete this line? It doesn't seem to be used (compilation passes without it).

@tobia
Copy link
Copy Markdown
Author

tobia commented Feb 25, 2020

I didn't handle the mkdir return value because there's not much we can do, the app should just ignore any error condition on that directory. If Linenoise can save its history there, it will do so, otherwise it won't.

As for the imports, I copied them from man 2 mkdir

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants