Add labels feature

[qu1ck]

Adds ability to add arbitrary labels to torrents.
Torrents can be marked for example as "iso", "linux", "debian" or any combination of those labels at the same time. Front ends can then implement filtering by those labels, see this example implementation.

[ckerr]

Thanks for this PR!

This looks like a good start. There's still the open question of how to integrate the existing Mac client's labels into this, and how the non-Mac clients should make use of this, but that doesn't block adding the functionality to libtransmission.

Comments inline

[qu1ck]

Thanks for fantastic comments. I think I addressed them all, please take another look.

[mikedld]

tr_strsep takes a char** while you're passing char const** here. I'd like to raise this (and all the other non-deprecation warnings) to an error at some point (for CI builds only), but currently compilers issue a warning, so please pay attention to it:

.../utils/remote.c: In function ‘addLabels’:
.../utils/remote.c:654:31: warning: passing argument 1 of ‘tr_strsep’ from incompatible pointer type [-Wincompatible-pointer-types]
     while ((token = tr_strsep(&arg, ", ")) != NULL)
                               ^
In file included from .../libtransmission/crypto-utils.h:16:0,
                 from .../utils/remote.c:22:
.../libtransmission/utils.h:243:7: note: expected ‘char **’ but argument is of type ‘const char **’
 char* tr_strsep(char** str, char const* delim);
       ^~~~~~~~~

The reason for this is that tr_strsep modifies the input string by replacing a found delimiter character with a '\0' (null) character on each successful call.

[qu1ck]

I fixed it by creating temporary copy of arg. Let me know if you had something different in mind.

[mikedld]

Since we're allowing the use of ',' (comma) and ' ' (space) as label separators, need to validate the input here to make sure those characters aren't part of one of the list values.

[qu1ck]

At first I used space as another separator to automatically trim extra space from labels but now I realized that will prevent space in middle of the label as well, which was not my intention.
Now only comma is considered a separator and I do tr_strstrip() in remote.c.

Also added validation as requested.

[mikedld]

... There's still the open question of how to integrate the existing Mac client's labels into this ...

I believe "labels" (here; multiple per torrent) and "groups" (Mac client; one per torrent) should be separate things. The former is not required to be unique and may not even have the associated settings with it (thus storing it in .resume, not settings.json or other global file, is enough), while the latter is used to potentially assign some settings to a torrent. Whether "group" needs to be a special case of "label" or not is a separate discussion.

[qu1ck]

Addressed comments, PTAL.

[qu1ck]

Btw failure in MacOS build is a flake, I'm not sure if I can do anything about it.

[mikedld]

Btw failure in MacOS build is a flake, I'm not sure if I can do anything about it.

Looks like updating to Xcode 10 led to more than 1.5x build slowdown (seen in quarter range)... I've adjusted the timeout to 20 minutes and restarted the build. Irrelevant to this PR indeed.

[qu1ck]

Friendly ping to reviewers.

[ckerr]

Gave another review pass for the new ptrarray code. Thanks for those changes!

[ckerr]

preallocating 100 label slots seems like a lot?

[qu1ck]

I believe original author copied this from function below but 100 files is definitely more likely than 100 labels. I changed this to 10, does this work better?

[ckerr]

Leaving this as non-const was intentional because the return value isn't const.

If you're trying to do this to loop through a const array of labels, check out how tr_ptrArrayBase() is used e.g. in session.c:

    if (!tr_ptrArrayEmpty(&loadme))
    {    
        int const n = tr_ptrArraySize(&loadme);
        char const* const* paths = (char const* const*)tr_ptrArrayBase(&loadme);

        for (int i = 0; i < n; ++i) 
        {
            tr_list_append(&blocklists, tr_blocklistFileNew(paths[i], isEnabled));
        }
    }    

[qu1ck]

Thanks for the suggestion, yes that was my intent.

[qu1ck]

Thanks for review, I addressed the comments.

[qu1ck]

Thanks for the suggestion, yes that was my intent.

[qu1ck]

I believe original author copied this from function below but 100 files is definitely more likely than 100 labels. I changed this to 10, does this work better?

[ckerr]

@mikedld can you take another look?

[mikedld]

@ckerr I'm mostly satisfied, back to you.

[qu1ck]

Thanks for thorough review Mike. I think I addressed everything except the comment you had about potential memory leak in torrentSet/setLabels. I didn't understand where was the leak, could you check if it's still there and elaborate a bit how it happens?
I think reusing same tr_variant* for labels and files was not an issue otherwise there would be lots of mem leaks as other such variables are also reused later in torrentSet(). Am I missing something?

[mikedld]

Yeah, that memory leak comment was bogus, I removed it since then, sorry.

[mikedld]

Minor, but I'd stop on the first error we get, i.e.

Suggested change

	if (strchr(str, ',') != NULL)
	if (errmsg != NULL && strchr(str, ',') != NULL)

Same below: could wrap following for and if in if (errmsg != NULL) as well.

[mikedld]

Looks like I'm an idiot once again. It should've been errmsg == NULL :)

[qu1ck]

Looks like I need to rebase again. I'll take this chance to squash some commits to cleanup history.

[ckerr]

LGTM. Thanks for addressing all the feedback!

[nukejello]

Thank you for adding this!

[ronggang]

Good job. 👍👍

[Belphemur]

@qu1ck I know this PR is merged, but I would like to know if you could do another one to add:

An ENV VAR like TR_TORRENT_LABELS into

transmission/libtransmission/torrent.c

Line 2234 in d3b8982

static void torrentCallScript(tr_torrent const* tor, char const* script)

that would contain a string with all the labels comma separated.

Because right now, you can only get the labels through the rpc which would mean having the script getting the info on the torrent to get the labels.

I would love to add it myself if I'm given pointer on how to concatenate a tr_variantListStr.