Displaying tags that can have multiple values

[Aidavdw]

Add functionality to display e.g. multiple artists.
Separation is done with ||, in line with the style that other popular mpc clients use (ncmpcpp, rmpc).

Example output of mpc status for a song with two artists using this fix:

Froukje || S10 - Ik Haat Hem Voor Jou
[playing] #4/4   1:27/2:35 (56%)
volume:100%   repeat: off   random: off   single: off   consume: off

Fixes #120.

[Aidavdw]

Also tested new implementation, it correctly cuts off artists names that are too long.
Please have a look again!

[MaxKellermann]

With this line, song_value() returns an empty string instead of NULL when there is no tag value - did you intend to change the API contract or was this accidental?

[Aidavdw]

whoops! That is indeed accidental.

[MaxKellermann]

PS: this line is actually more buggy than that - it returns a string that's not null-terminated, but your API docs say it's guaranteed to be null-terminated.

[Aidavdw]

Alright, fixed the two more points you brought up.
Tested for empty tags as well, and it looks like it works as intended now.

[MaxKellermann]

This looks good now - but can you please fold all fixup commits into the first commit?
Making separate commits for separate aspects is a good idea, but here it's really just fixup commits for mistakes in the first commit.

[Aidavdw]

This looks good now - but can you please fold all fixup commits into the first commit? Making separate commits for separate aspects is a good idea, but here it's really just fixup commits for mistakes in the first commit.

Yeah! I thought github allows for a squash merge, but i can do it manually from this side as well.

[MaxKellermann]

I don't use any web interface for anything (other than commenting here). I use git, not GitHub - GitHub is just a random git server.

[Aidavdw]

I meant the git function, which is accessible from the github interface but also from git itself :)
Anyway, should be ready now.

[MaxKellermann]

I meant the git function, which is accessible from the github interface but also from git itself

But that would make your job mine. And then happens what happens with your branch: just look at the confusing commit message.

"display tags with multiple values in song formatting
Allows displaying multiple artists, albums, etc.
style changes based on feedback
changed implementation of multiple tag concatenation to look more like the implementation used in ncmpc
Separated into its own function. Also changed separator to a comma.
clearly show that formatting buffer is arbitrarily sized
copy tags returns nullptr if nothing added."

When somebody looks at this commit in 10 years, what will they make from it?

[Aidavdw]

In my humble opinion, it is not only the end result but also the process that matters, as that too can give insight into development history.
I understand now that you prefer to keep commit history to the point.

[MaxKellermann]

There is no historical value in knowing what mistakes you made in commits that were never merged.
Your commit message also doesn't say what these texts mean - you did not specify that these are about unmerged stuff that was not fit for merging. You just concatenated all the fixup commit messages into one, and that long commit message does not describe the commit, it just confuses readers.

[MaxKellermann]

clang found another bug in your code:

../../src/song_format.c:137:7: error: variable 'value' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
  137 |                 if (added_text != NULL) {
      |                     ^~~~~~~~~~~~~~~~~~
../../src/song_format.c:142:6: note: uninitialized use occurs here
  142 |         if (value != NULL)
      |             ^~~~~
../../src/song_format.c:137:3: note: remove the 'if' if its condition is always true
  137 |                 if (added_text != NULL) {
      |                 ^~~~~~~~~~~~~~~~~~~~~~~
../../src/song_format.c:96:19: note: initialize the variable 'value' to silence this warning
   96 |         const char *value;
      |                          ^
      |                           = NULL

And indeed, you forgot to initialize the variable in one code path.

[Aidavdw]

Good catch. Curious, seems to not have been detected on my machine.
Added the initialisation.

[MaxKellermann]

Unit tests fail.

[MaxKellermann]

GH actions currently doesn't show details about the unit test failures; I just pushed a change that prints those details. Next time you push, rebase first!

[Aidavdw]

rebased an fixed error that caused it to fail.

[MaxKellermann]

Good to have unit tests - it caught another mistake.
Since you implemented a new feature, it would be very useful to have a unit test for that. Can you add tests, please? Should be in the same commit.

[Aidavdw]

Added unit test.