Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use whole path as logfile when defined via -f #1455

Merged
merged 4 commits into from
Dec 11, 2020
Merged

Conversation

jubalh
Copy link
Member

@jubalh jubalh commented Dec 11, 2020

profanity -f my created ~/.local/share/profanity/logs/my.log`.

It would be nicer if one could define the actual path, so one can choose
another directory or even use /dev/null.

Fixes #1442

`profanity -f my` created ~/.local/share/profanity/logs/my.log`.

It would be nicer if one could define the actual path, so one can choose
another directory or even use /dev/null.

Fixes #1442
@jubalh jubalh added this to the 0.10.0 milestone Dec 11, 2020
@jubalh jubalh requested a review from wstrm December 11, 2020 09:41
@jubalh jubalh self-assigned this Dec 11, 2020
@jubalh
Copy link
Member Author

jubalh commented Dec 11, 2020

This isn't finished yet. Need some opinion here.

The problem, and probably also the reason why I chose to make -f use only the filename in a dir was to keep things simple (for me) :D

We have log_reinit() which is run once we change /log shared on|off. It closes the current log file and uses a new one (toggling with PID attached to filename or not).

So either we need to rewrite log_reinit() which probably means another variable that will hold the information to build the proper filename and will use space all the time. Or we remove log_reinit() altogether and tell users they need to restart the client before /log shared on|off takes effect.

I'm for the latter since its much easier. Otherwise we need to take into account all kind of scenarios like.

@wstrm whats your opinion?

@wstrm
Copy link
Contributor

wstrm commented Dec 11, 2020

Or we remove log_reinit() altogether and tell users they need to restart the client before /log shared on|off takes effect.

As long as we tell the user (in the console) once they've used /log shared on|off that a restart is required I'm fine with it :)

Tell user about this change.
Not needed anymore.
@jubalh
Copy link
Member Author

jubalh commented Dec 11, 2020

@wstrm next question: does it make sense to do logfile rotation if the user provided a logfile with -f? I guess not. So it seems we need a way to detect whether a logfile is supplied anyways. And in this case don't rotate the logs?

@jubalh jubalh marked this pull request as ready for review December 11, 2020 10:34
g_string_append(logfile, "/profanity/logs/");
g_string_append(logfile, log_file);
gchar *log_path = g_path_get_dirname(log_file);
if (!mkdir_recursive(log_path)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, maybe I should move the download directory creation to this file instead then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that would be possible.

@jubalh jubalh added the behaviour-change changes to current behaviour label Dec 11, 2020
@jubalh jubalh merged commit 8806415 into master Dec 11, 2020
@jubalh jubalh deleted the fix/1442-logging branch December 11, 2020 10:52
@mrkiko
Copy link

mrkiko commented Dec 11, 2020

Thanks! Works great!!

As per the logfile rotation - I think it may make sense. removing the logfile with rm would not help infact in e.g.: freeing space, or management.
But restarting the client isn't a problem for me.
thanks a lot for your great work and helping me out. this is a gift.

jubalh added a commit that referenced this pull request Jun 28, 2021
See #1518
It has a good explanation of what happened:

```
Apparently, the _rotate_log_file function tried to extract user-provided
name from currently used mainlogfile and restart logging to the same
place after rotation, but currently this is interpreted as a full path
instead. As I understand, the log rotation is no longer done with
user-provided paths at all so this should be simply skipped altogether
now as passing any non-NULL value is interpreted as user-provided.
Replacing start with NULL appears to fix it for me.
```

In log_msg() we only rotate the log if not user_provided_log.

#1455 changed the
behaviour from user defined filename in the log dir to using full path.
@jubalh jubalh mentioned this pull request Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviour-change changes to current behaviour improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable profanity logging (not chatlogs) alltogeter or choosing where to write them
3 participants