[RFC] add two commands to reload cache

[Johannes Weißl]

Hey,

I think everyone of us has done this sometimes:
rm -f ~/.cmus/cache
Enough :-)!

How about a new command (or a parameter to update-cache, e.g. -f for
force), that does the same thing without having to start/stop cmus? The
command would remove every cache-entry (not just from the tracks
currently in library), and reload tags for the tracks in the library.

update-cache works only if modification time has changed, but many
tagging tools don't do that.

What would also be handy: something like win-reload-cache (or
win-update-cache -f), that only reloads tags for marked tracks...
This is currently done by the :run command already (but only if
modification time has changed).

What do you think? Do you prefer:
1) new commands, e.g.:
- reload-cache
- win-reload-cache
2. parameters, e.g.:
- update-cache -f
- win-update-cache -f (new)


Johannes

[Gregory Petrosyan]

Post by Johannes Weißl
Hey,
rm -f ~/.cmus/cache
Enough :-)!
How about a new command (or a parameter to update-cache, e.g. -f for
force), that does the same thing without having to start/stop cmus? The
command would remove every cache-entry (not just from the tracks
currently in library), and reload tags for the tracks in the library.
update-cache works only if modification time has changed, but many
tagging tools don't do that.
What would also be handy: something like win-reload-cache (or
win-update-cache -f), that only reloads tags for marked tracks...
This is currently done by the :run command already (but only if
modification time has changed).
  - reload-cache
  - win-reload-cache
  - update-cache -f
  - win-update-cache -f  (new)

I'd go with update-cache -f, since I see this more like a "developer
convenience" thing, and this option changes as little as possible.

BTW, I was thinking a bit about not using mtime for this at all —
something like hash(size, 5-7 random fragments at fixed offsets)
maybe... But I guess it can be too expensive to do for a
multi-thousand library. On the other hand, if it is not, such a scan
on every startup (in background) can almost eliminate the need for
:update-cache.

                Gregory

[Johannes Weißl]

Post by Gregory Petrosyan
I'd go with update-cache -f, since I see this more like a "developer
convenience" thing, and this option changes as little as possible.

Ok! I know that developers would use it a lot, but I think almost every
user has done that at least a few times...
What about the win-update-cache command?

Post by Gregory Petrosyan
BTW, I was thinking a bit about not using mtime for this at all —
something like hash(size, 5-7 random fragments at fixed offsets)
maybe... But I guess it can be too expensive to do for a
multi-thousand library. On the other hand, if it is not, such a scan
on every startup (in background) can almost eliminate the need for
:update-cache.

Hmm, good idea, but I don't think it will work. Audio files usually only
change at tiny parts. It is difficult to say where, because the
tag-header can be at the beginning or at the end of the file. Also many
formats have padded headers (for speed), so not every byte would shift
if e.g. the artist has more characters. Usually mtime is a good
indicator...

What would be possible is to "watch" a directory using inotify, but I
don't think it is worth it (not portable, too little advantage, add
~/audio is simple and very fast).


Johannes

[Jason Woofenden]

Post by Gregory Petrosyan

Post by Johannes Weißl
Hey,
rm -f ~/.cmus/cache
Enough :-)!
How about a new command (or a parameter to update-cache, e.g. -f for
force), that does the same thing without having to start/stop cmus? The
command would remove every cache-entry (not just from the tracks
currently in library), and reload tags for the tracks in the library.
update-cache works only if modification time has changed, but many
tagging tools don't do that.
What would also be handy: something like win-reload-cache (or
win-update-cache -f), that only reloads tags for marked tracks...
This is currently done by the :run command already (but only if
modification time has changed).
  - reload-cache
  - win-reload-cache
  - update-cache -f
  - win-update-cache -f  (new)

I'd go with update-cache -f, since I see this more like a "developer
convenience" thing, and this option changes as little as possible.
BTW, I was thinking a bit about not using mtime for this at all —
something like hash(size, 5-7 random fragments at fixed offsets)
maybe... But I guess it can be too expensive to do for a
multi-thousand library. On the other hand, if it is not, such a scan
on every startup (in background) can almost eliminate the need for
:update-cache.

ooh, that sounds cool. Reading some bytes might be overkill. But
checking the size and mtime sounds like a cool solution to some
taggers restoring the old mtime after changing tags. We get the
size with the same syscall that gives us the mtime right?

I'm thinking that the only case where the file has changed, but
it's still the same size and same mtime, would be when you use a
weird tagger, and you change one character in a tag (by
capitalizing, or fixing an accent or something) in which case
checking a few random bytes is unlikely to help. Is there another
use case?

I guess I'm mostly interested in two things:

1) fast and works most of the time (mtime+size sounds good)

2) works no matter what (slow is acceptable). For this I think we
should just ditch the cache completely and read everything from the
files again.

Reading some bytes sounds like it's in the middle somewhere.

Take care, - Jason

[Johannes Weißl]

Post by Jason Woofenden
ooh, that sounds cool. Reading some bytes might be overkill. But
checking the size and mtime sounds like a cool solution to some
taggers restoring the old mtime after changing tags. We get the
size with the same syscall that gives us the mtime right?

Yes, but...

Post by Jason Woofenden
I'm thinking that the only case where the file has changed, but
it's still the same size and same mtime, would be when you use a
weird tagger, and you change one character in a tag (by
capitalizing, or fixing an accent or something) in which case
checking a few random bytes is unlikely to help. Is there another
use case?

I think that many formats (at least id3v2 [1]) have the possibility to pad
the tag field with zeros, so changing tags is faster. In this case the file
size would not change...

[1] http://id3lib.sourceforge.net/id3guide.html#padding

Post by Jason Woofenden
1) fast and works most of the time (mtime+size sounds good)
2) works no matter what (slow is acceptable). For this I think we
should just ditch the cache completely and read everything from the
files again.

Agreed!


Johannes

[Johannes Weißl]

Post by Gregory Petrosyan
I'd go with update-cache -f, since I see this more like a "developer
convenience" thing, and this option changes as little as possible.

Ok, patches are ready! What do you think?


Johannes

[Johannes Weißl]

Same as quit, rm -f ~/.cmus/cache, start cmus.
---
Doc/cmus.txt | 8 ++++++--
cache.c | 14 +++++++++++---
cache.h | 2 +-
cmus.c | 9 +++++++--
cmus.h | 2 +-
command_mode.c | 5 +++--
job.c | 4 +++-
job.h | 4 ++++
8 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/Doc/cmus.txt b/Doc/cmus.txt
index 477d656..255fe3d 100644
--- a/Doc/cmus.txt
+++ b/Doc/cmus.txt
@@ -590,8 +590,12 @@ unbind [-f] <context> <key>
unmark
Unmark all tracks (see *mark*).

-update-cache
- Update track metadata cache (~/.cmus/cache).
+update-cache [-f]
+ Update track metadata cache (~/.cmus/cache). Only files with changed
+ modification time or removed files are considered.
+
+ -f
+ Update all files. Same as quit, rm -f ~/.cmus/cache, start cmus.

view <name or 1-7>
Switches active view.
diff --git a/cache.c b/cache.c
index 5f2b30d..9b5c85d 100644
--- a/cache.c
+++ b/cache.c
@@ -424,7 +424,7 @@ struct track_info *cache_get_ti(const char *filename)
return ti;
}

-struct track_info **cache_refresh(int *count)
+struct track_info **cache_refresh(int *count, int force)
{
struct track_info **tis = get_track_infos();
int i, n = total;
@@ -446,7 +446,7 @@ struct track_info **cache_refresh(int *count)

if (!is_url(ti->filename)) {
rc = stat(ti->filename, &st);
- if (!rc && ti->mtime == st.st_mtime) {
+ if (!rc && !force && ti->mtime == st.st_mtime) {
// unchanged
tis[i] = NULL;
continue;
@@ -459,8 +459,16 @@ struct track_info **cache_refresh(int *count)

if (!rc) {
// changed
- struct track_info *new_ti = ip_get_ti(ti->filename);
+ struct track_info *new_ti;

+ // clear cache-only entries
+ if (force && ti->ref == 1) {
+ track_info_unref(ti);
+ tis[i] = NULL;
+ continue;
+ }
+
+ new_ti = ip_get_ti(ti->filename);
if (new_ti) {
add_ti(new_ti, hash);
new++;
diff --git a/cache.h b/cache.h
index 680f0b1..71ac2a0 100644
--- a/cache.h
+++ b/cache.h
@@ -31,6 +31,6 @@ int cache_init(void);
int cache_close(void);
struct track_info *cache_get_ti(const char *filename);
void cache_remove_ti(struct track_info *ti);
-struct track_info **cache_refresh(int *count);
+struct track_info **cache_refresh(int *count, int force);

#endif
diff --git a/cmus.c b/cmus.c
index 0d869fd..f998dee 100644
--- a/cmus.c
+++ b/cmus.c
@@ -250,9 +250,14 @@ static int update_cb(void *data, struct track_info *ti)
return 0;
}

-void cmus_update_cache(void)
+void cmus_update_cache(int force)
{
- worker_add_job(JOB_TYPE_LIB, do_update_cache_job, free_update_cache_job, NULL);
+ struct update_cache_data *data;
+
+ data = xnew(struct update_cache_data, 1);
+ data->force = force;
+
+ worker_add_job(JOB_TYPE_LIB, do_update_cache_job, free_update_cache_job, data);
}

void cmus_update_lib(void)
diff --git a/cmus.h b/cmus.h
index 6cf5fbd..7ef14b0 100644
--- a/cmus.h
+++ b/cmus.h
@@ -76,7 +76,7 @@ void cmus_add(add_ti_cb, const char *name, enum file_type ft, int jt);
int cmus_save(for_each_ti_cb for_each_ti, const char *filename);
int cmus_save_ext(for_each_ti_cb for_each_ti, const char *filename);

-void cmus_update_cache(void);
+void cmus_update_cache(int force);
void cmus_update_lib(void);
void cmus_update_tis(struct track_info **tis, int nr);

diff --git a/command_mode.c b/command_mode.c
index 642d21e..f1e928e 100644
--- a/command_mode.c
+++ b/command_mode.c
@@ -613,7 +613,8 @@ static void cmd_unmark(char *arg)

static void cmd_update_cache(char *arg)
{
- cmus_update_cache();
+ int flag = parse_flags((const char **)&arg, "f");
+ cmus_update_cache(flag == 'f');
}

static void cmd_cd(char *arg)
@@ -2439,7 +2440,7 @@ struct command commands[] = {
{ "tqueue", cmd_tqueue, 0, 1, NULL, 0, 0 },
{ "unbind", cmd_unbind, 1, 1, expand_unbind_args, 0, 0 },
{ "unmark", cmd_unmark, 0, 0, NULL, 0, 0 },
- { "update-cache", cmd_update_cache,0, 0, NULL, 0, 0 },
+ { "update-cache", cmd_update_cache,0, 1, NULL, 0, 0 },
{ "view", cmd_view, 1, 1, NULL, 0, 0 },
{ "vol", cmd_vol, 1, 2, NULL, 0, 0 },
{ "w", cmd_save, 0, 1, expand_load_save, 0, CMD_UNSAFE },
diff --git a/job.c b/job.c
index d10e391..6a10bdb 100644
--- a/job.c
+++ b/job.c
@@ -280,11 +280,12 @@ void free_update_job(void *data)

void do_update_cache_job(void *data)
{
+ struct update_cache_data *d = data;
struct track_info **tis;
int i, count;

cache_lock();
- tis = cache_refresh(&count);
+ tis = cache_refresh(&count, d->force);
editable_lock();
for (i = 0; i < count; i++) {
struct track_info *new, *old = tis[i];
@@ -308,4 +309,5 @@ void do_update_cache_job(void *data)

void free_update_cache_job(void *data)
{
+ free(data);
}
diff --git a/job.h b/job.h
index c94369b..a9807b0 100644
--- a/job.h
+++ b/job.h
@@ -33,6 +33,10 @@ struct update_data {
struct track_info **ti;
};

+struct update_cache_data {
+ unsigned int force : 1;
+};
+
void do_add_job(void *data);
void free_add_job(void *data);
void do_update_job(void *data);

[Johannes Weißl]

If n tracks are removed from cache because they are not longer existant,
the last n tracks of the cache are not refreshed (because "total"
decreases).
---
cache.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cache.c b/cache.c
index b6d843e..5f2b30d 100644
--- a/cache.c
+++ b/cache.c
@@ -427,9 +427,9 @@ struct track_info *cache_get_ti(const char *filename)
struct track_info **cache_refresh(int *count)
{
struct track_info **tis = get_track_infos();
- int i;
+ int i, n = total;

- for (i = 0; i < total; i++) {
+ for (i = 0; i < n; i++) {
unsigned int hash;
struct track_info *ti = tis[i];
struct stat st;

[Johannes Weißl]

needed for next commits
---
command_mode.c | 58 ++++++++++++++++++++++++++++----------------------------
1 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/command_mode.c b/command_mode.c
index f1e928e..a1d26d7 100644
--- a/command_mode.c
+++ b/command_mode.c
@@ -947,18 +947,21 @@ error:
return NULL;
}

-static struct track_info **sel_tis;
-static int sel_tis_alloc;
-static int sel_tis_nr;
+struct track_info_selection {
+ struct track_info **tis;
+ int tis_alloc;
+ int tis_nr;
+};

static int add_ti(void *data, struct track_info *ti)
{
- if (sel_tis_nr == sel_tis_alloc) {
- sel_tis_alloc = sel_tis_alloc ? sel_tis_alloc * 2 : 8;
- sel_tis = xrenew(struct track_info *, sel_tis, sel_tis_alloc);
+ struct track_info_selection *sel = data;
+ if (sel->tis_nr == sel->tis_alloc) {
+ sel->tis_alloc = sel->tis_alloc ? sel->tis_alloc * 2 : 8;
+ sel->tis = xrenew(struct track_info *, sel->tis, sel->tis_alloc);
}
track_info_ref(ti);
- sel_tis[sel_tis_nr++] = ti;
+ sel->tis[sel->tis_nr++] = ti;
return 0;
}

@@ -966,6 +969,7 @@ static void cmd_run(char *arg)
{
char **av, **argv;
int ac, argc, i, run, files_idx = -1;
+ struct track_info_selection sel = { .tis = NULL };

if (cur_view > QUEUE_VIEW) {
info_msg("Command execution is supported only in views 1-4");
@@ -978,48 +982,44 @@ static void cmd_run(char *arg)
}

/* collect selected files (struct track_info) */
- sel_tis = NULL;
- sel_tis_alloc = 0;
- sel_tis_nr = 0;
-
editable_lock();
switch (cur_view) {
case TREE_VIEW:
__tree_for_each_sel(add_ti, NULL, 0);
break;
case SORTED_VIEW:
- __editable_for_each_sel(&lib_editable, add_ti, NULL, 0);
+ __editable_for_each_sel(&lib_editable, add_ti, &sel, 0);
break;
case PLAYLIST_VIEW:
- __editable_for_each_sel(&pl_editable, add_ti, NULL, 0);
+ __editable_for_each_sel(&pl_editable, add_ti, &sel, 0);
break;
case QUEUE_VIEW:
- __editable_for_each_sel(&pq_editable, add_ti, NULL, 0);
+ __editable_for_each_sel(&pq_editable, add_ti, &sel, 0);
break;
}
editable_unlock();

- if (sel_tis_nr == 0) {
+ if (sel.tis_nr == 0) {
/* no files selected, do nothing */
free_str_array(av);
return;
}
- sel_tis[sel_tis_nr] = NULL;
+ sel.tis[sel.tis_nr] = NULL;

/* build argv */
- argv = xnew(char *, ac + sel_tis_nr + 1);
+ argv = xnew(char *, ac + sel.tis_nr + 1);
argc = 0;
if (files_idx == -1) {
/* add selected files after rest of the args */
for (i = 0; i < ac; i++)
argv[argc++] = av[i];
- for (i = 0; i < sel_tis_nr; i++)
- argv[argc++] = sel_tis[i]->filename;
+ for (i = 0; i < sel.tis_nr; i++)
+ argv[argc++] = sel.tis[i]->filename;
} else {
for (i = 0; i < files_idx; i++)
argv[argc++] = av[i];
- for (i = 0; i < sel_tis_nr; i++)
- argv[argc++] = sel_tis[i]->filename;
+ for (i = 0; i < sel.tis_nr; i++)
+ argv[argc++] = sel.tis[i]->filename;
for (i = files_idx; i < ac; i++)
argv[argc++] = av[i];
}
@@ -1029,8 +1029,8 @@ static void cmd_run(char *arg)
d_print("ARG: '%s'\n", argv[i]);

run = 1;
- if (confirm_run && (sel_tis_nr > 1 || strcmp(argv[0], "rm") == 0)) {
- if (!yes_no_query("Execute %s for the %d selected files? [y/N]", arg, sel_tis_nr)) {
+ if (confirm_run && (sel.tis_nr > 1 || strcmp(argv[0], "rm") == 0)) {
+ if (!yes_no_query("Execute %s for the %d selected files? [y/N]", arg, sel.tis_nr)) {
info_msg("Aborted");
run = 0;
}
@@ -1053,23 +1053,23 @@ static void cmd_run(char *arg)
switch (cur_view) {
case TREE_VIEW:
case SORTED_VIEW:
- /* this must be done before sel_tis are unreffed */
+ /* this must be done before sel.tis are unreffed */
free_str_array(av);
free(argv);

/* remove non-existed files, update tags for changed files */
- cmus_update_tis(sel_tis, sel_tis_nr);
+ cmus_update_tis(sel.tis, sel.tis_nr);

- /* we don't own sel_tis anymore! */
+ /* we don't own sel.tis anymore! */
return;
}
}
}
free_str_array(av);
free(argv);
- for (i = 0; sel_tis[i]; i++)
- track_info_unref(sel_tis[i]);
- free(sel_tis);
+ for (i = 0; sel.tis[i]; i++)
+ track_info_unref(sel.tis[i]);
+ free(sel.tis);
}

static void cmd_shell(char *arg)

[Johannes Weißl]

---
Doc/cmus.txt | 4 ++++
cmus.c | 3 ++-
cmus.h | 2 +-
command_mode.c | 20 +++++++++++++++++++-
job.c | 5 +++--
job.h | 1 +
6 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/Doc/cmus.txt b/Doc/cmus.txt
index 255fe3d..187427a 100644
--- a/Doc/cmus.txt
+++ b/Doc/cmus.txt
@@ -711,6 +711,10 @@ win-update (*u*)

Only works in views 1-2 and 5, does nothing in other views.

+win-update-cache [-f]
+ Same as *update-cache*, but only for marked / selected tracks.
+ Only works in views 1-2, does nothing in other views.
+

@h1 CONFIGURATION OPTIONS

diff --git a/cmus.c b/cmus.c
index f998dee..6d104e0 100644
--- a/cmus.c
+++ b/cmus.c
@@ -276,7 +276,7 @@ void cmus_update_lib(void)
worker_add_job(JOB_TYPE_LIB, do_update_job, free_update_job, data);
}

-void cmus_update_tis(struct track_info **tis, int nr)
+void cmus_update_tis(struct track_info **tis, int nr, int force)
{
struct update_data *data;

@@ -284,6 +284,7 @@ void cmus_update_tis(struct track_info **tis, int nr)
data->size = nr;
data->used = nr;
data->ti = tis;
+ data->force = force;
worker_add_job(JOB_TYPE_LIB, do_update_job, free_update_job, data);
}

diff --git a/cmus.h b/cmus.h
index 7ef14b0..ee29789 100644
--- a/cmus.h
+++ b/cmus.h
@@ -78,7 +78,7 @@ int cmus_save_ext(for_each_ti_cb for_each_ti, const char *filename);

void cmus_update_cache(int force);
void cmus_update_lib(void);
-void cmus_update_tis(struct track_info **tis, int nr);
+void cmus_update_tis(struct track_info **tis, int nr, int force);

int cmus_is_playlist(const char *filename);
int cmus_is_playable(const char *filename);
diff --git a/command_mode.c b/command_mode.c
index a1d26d7..5593990 100644
--- a/command_mode.c
+++ b/command_mode.c
@@ -1058,7 +1058,7 @@ static void cmd_run(char *arg)
free(argv);

/* remove non-existed files, update tags for changed files */
- cmus_update_tis(sel.tis, sel.tis_nr);
+ cmus_update_tis(sel.tis, sel.tis_nr, 0);

/* we don't own sel.tis anymore! */
return;
@@ -1668,6 +1668,23 @@ static void cmd_win_pg_up(char *arg)
editable_unlock();
}

+static void cmd_win_update_cache(char *arg)
+{
+ struct track_info_selection sel = { .tis = NULL };
+ int flag = parse_flags((const char **)&arg, "f");
+
+ if (cur_view != TREE_VIEW && cur_view != SORTED_VIEW)
+ return;
+
+ editable_lock();
+ view_for_each_sel[cur_view](add_ti, &sel, 0);
+ editable_unlock();
+ if (sel.tis_nr == 0)
+ return;
+ sel.tis[sel.tis_nr] = NULL;
+ cmus_update_tis(sel.tis, sel.tis_nr, flag == 'f');
+}
+
static void cmd_win_top(char *arg)
{
editable_lock();
@@ -2462,6 +2479,7 @@ struct command commands[] = {
{ "win-top", cmd_win_top, 0, 0, NULL, 0, 0 },
{ "win-up", cmd_win_up, 0, 0, NULL, 0, 0 },
{ "win-update", cmd_win_update, 0, 0, NULL, 0, 0 },
+ { "win-update-cache", cmd_win_update_cache,0, 1, NULL, 0, 0 },
{ "wq", cmd_quit, 0, 1, NULL, 0, 0 },
{ NULL, NULL, 0, 0, 0, 0, 0 }
};
diff --git a/job.c b/job.c
index 6a10bdb..6df02ee 100644
--- a/job.c
+++ b/job.c
@@ -250,7 +250,7 @@ void do_update_job(void *data)

/* stat follows symlinks, lstat does not */
rc = stat(ti->filename, &s);
- if (rc || ti->mtime != s.st_mtime) {
+ if (rc || d->force || ti->mtime != s.st_mtime) {
editable_lock();
lib_remove(ti);
editable_unlock();
@@ -262,7 +262,8 @@ void do_update_job(void *data)
if (rc) {
d_print("removing dead file %s\n", ti->filename);
} else {
- d_print("mtime changed: %s\n", ti->filename);
+ if (ti->mtime != s.st_mtime)
+ d_print("mtime changed: %s\n", ti->filename);
cmus_add(lib_add_track, ti->filename, FILE_TYPE_FILE, JOB_TYPE_LIB);
}
}
diff --git a/job.h b/job.h
index a9807b0..3795c28 100644
--- a/job.h
+++ b/job.h
@@ -31,6 +31,7 @@ struct update_data {
size_t size;
size_t used;
struct track_info **ti;
+ unsigned int force : 1;
};

struct update_cache_data {

[Gregory Petrosyan]

Post by Johannes Weißl

Post by Gregory Petrosyan
I'd go with update-cache -f, since I see this more like a "developer
convenience" thing, and this option changes as little as possible.

Ok, patches are ready! What do you think?

They look great, thanks -- merged to master!

Gregory