dinitctl: show the service path in the "status" command output by mobin-2008 · Pull Request #550 · davmac314/dinit

mobin-2008 · 2026-05-31T12:38:16Z

Use the existing functionality to show the service path of currently loaded service.

Fixes #508

davmac314 · 2026-06-01T10:36:15Z

Someone was already working on this (#511). It does look like that effort stalled, but could you confirm with them please before we proceed with this new PR, out of politeness.

It would be good if you could communicate what you are intending to work on before you start doing it, that will help avoid this sort of situation.

mobin-2008 · 2026-06-01T22:43:09Z

Someone was already working on this (#511). It does look like that effort stalled, but could you confirm with them please before we proceed with this new PR, out of politeness.

I asked him and I'm waiting for his response.

It would be good if you could communicate what you are intending to work on before you start doing it, that will help avoid this sort of situation.

Yes. I should did more than assigning issue (#508) to myself.

davmac314 · 2026-06-02T01:09:04Z

Let's give it 3 days to wait for an answer. In the meantime, can you please close this PR and re-open after that time (I assume you have properly self-reviewed, if not please do that before re-opening).

mobin-2008 · 2026-06-10T20:49:30Z

I forgot to reopen this PR before force pushing my branch :(
I need to open another PR.
@davmac314 Is that OK?

davmac314 · 2026-06-10T22:06:49Z

Create a new branch from your current one (dinit_show_service_dir) as a backup, do a hard reset on dinit_show_service_dir to the commit 4a646c595af27fe98f0c381fe2ea13feea78484f, force-push, re-open the PR, then do a hard reset to the backup branch.

davmac314 · 2026-06-11T08:30:59Z

As per the contribution guide, you should specify what testing has been done when opening a PR. Can you do that now please?

Can you also confirm that you're otherwise following the process, and have self-reviewed?

Use the existing functionality to show the service path of currently loaded service. Addresses davmac314#508

mobin-2008 · 2026-06-11T09:55:48Z

As per the contribution guide, you should specify what testing has been done when opening a PR. Can you do that now please?

Changes tested against these cases:

$ src/dinitctl status kanshi
Service: kanshi (/home/mobin/.config/dinit.d/kanshi)
    State: STARTED
    Activation: start due to dependent(s)
    Process ID: 19268

$ src/dinitctl status boot
Service: boot (/run/turnstiled/1000/srv.1229/boot)
    State: STARTED
    Activation: explicitly started

# src/dinitctl status logger@kmsg
Service: logger@kmsg (/etc/dinit.d/logger)
    State: STARTED
    Activation: start due to dependent(s)
    Process ID: 606

Can you also confirm that you're otherwise following the process, and have self-reviewed?

Yes.

davmac314 · 2026-06-11T10:23:40Z

Can you also confirm that you're otherwise following the process, and have self-reviewed?

Yes.

It seems like you just pushed changes after opening the PR - that's not the process, and we've talked about that before! - and yet you are now saying, just a few minutes later, that you are following the process.

Please read the CONTRIBUTING and PR-PROCESS guides, in full. This is the last time I will allow you grace for this..

davmac314 · 2026-06-11T10:31:57Z


-    // Issue STATUS request
    {
+        // Prepare to show the service path in the output later down


"Service path" isn't a thing. You're talking about a service description file path. Don't make up terminology please.

Two questions:

"Prepare to show" is not clear. What is this code actually doing?

Why do you feel the output "later down" is significant enough that it needs to be mentioned here?

"Service path" isn't a thing. You're talking about a service description file path. Don't make up terminology please.

OK. Will be fixed.

Two questions:

* "Prepare to show" is not clear. What is this code actually doing?

It's preparing a variable (service_path) to be used for showing the service path in the output later down. The code shows how this preparation works.

* Why do you feel the output "later down" is significant enough that it needs to be mentioned here?

Because it feels weird to me that there is a random get_service_description_dir before issuing STATUS. This is here to mention it's going to be used for showing the service path in the output later down the code and you need to keep looking for its usage.

It's preparing a variable (service_path) to be used for showing the service path in the output later down.

What does "preparing" mean in this case? (Also, can you explain it without referring to a variable? Variables are everywhere and I can see the variables in the code).

The code shows how this preparation works.

It shouldn't be necessary to read the code to understand what a comment means. The comment is supposed to explain the code, not the other way around.

What is the specific purpose of the comment here? Eg Is it explaining something that you think is difficult to understand, or is it just summarising what the following section of code does to help navigating the code, or something else?

This is here to mention it's going to be used for showing the service path in the output later down the code and you need to keep looking for its usage

Isn't that normal though? There are heaps of places where some value is determined so that it can be used later - that's basically every variable assignment! - so does it really need to be explained in the comment? Do you see any other comments that do that?

Because it feels weird to me that there is a random get_service_description_dir before issuing STATUS

The comment doesn't explain why get_service_description_dir is called at all, though.

Sorry, that's a bit of an overload of questions. Can we focus on these for now:

What does "preparing" mean in this case?

("Preparing a variable" means nothing to me. What do you mean exactly?)

and

What is the specific purpose of the comment here? Eg Is it explaining something that you think is difficult to understand, or is it just summarising what the following section of code does to help navigating the code, or something else?

("Preparing a variable" means nothing to me. What do you mean exactly?)

The original comment was this:

// Get the service description directory to show the service path in the output later down

I thought it is a bad idea to repeat what the code does in this comment, so I replaced it with "prepare" which is also bad, because everything up to the issuing the STATUS request is just preparation.

What is the specific purpose of the comment here? Eg Is it explaining something that you think is difficult to understand, or is it just summarising what the following section of code does to help navigating the code, or something else?

To be faithful to the style of this function, I thought it needs a comment explaining the section below. I mean it does

just summarising what the following section of code does to help navigating the code

but now I think it doesn't need to a comment explaining it, I just misunderstood the style.

I thought it is a bad idea to repeat what the code does in this comment

It is fine to explain what a block of code does (at a slightly higher level than the code itself) with a comment.

If the comment is summarising the section of code though, then it should focus on what this section of code does, not non-specific things such as that it is "preparing" something "to be output later down". Those words (quoted) don't give any useful information to someone reading the comment.

but now I think it doesn't need to a comment explaining it

It is fine and good to have a comment here, that summarises what the code is doing. That is why I asked: what does this code actually do? (I know what it does - but I want you to explain it, in a way that would be useful as a comment).

So again: what does this code actually do?

davmac314 · 2026-06-11T10:34:08Z

        }

-        cout << "Service: " << service_name << "\n"
+        cout << "Service: " << service_name << " (" << service_path << ")" << "\n"


Don't output it here. Output it with an appropriate heading (maybe 'File:') as with other status elements.

Feature design should really be discussed before working on it - that's also part of the documented process.

I think adding File is good.

davmac314 · 2026-06-11T10:38:48Z

Please answer questions above and then I will complete the review.

mobin-2008 · 2026-06-16T15:46:11Z

        return 1;
    }

-    // Issue STATUS request


I think the purpose of indentation here is to split the function into two sections:

Code preparing for issuing the STATUS request (e.g. getting the service handle)

Issuing the STATUS and generate the output by processing the response from the daemon and with help of things defined in outer indentation.

If that is correct I think anything that doesn't have anything to do with issuing request and processing it, should be defined in the outer layer, right?

I think you mean the scoping (via { and '}')? Indentation by itself does nothing.

It's not really all that important here. It was initially used to limit the scope of some temporary variables but it doesn't really matter, the scope got too long anyway.

mobin-2008 · 2026-06-16T15:56:01Z

that's not the process, and we've talked about that before! - and yet you are now saying, just a few minutes later, that you are following the process.

Feature design should really be discussed before working on it - that's also part of the documented process.

Now I see how half-baked this PR is, I try my best to fix it. I answered your questions and also asked a question for better understanding the current design. If the feedback is still negative, I would mark this PR as draft until it gets to the point of being up to the standard.

davmac314 · 2026-06-16T21:58:52Z

Now I see how half-baked this PR is, I try my best to fix it. I answered your questions

There's still a question pending - see above.

Once that's resolved I'll complete the review. Thanks.

mobin-2008 added A-Importance: Normal Enhancement/New Feature Improving things or introduce new feature C-dinitctl Things about dinitctl labels May 31, 2026

mobin-2008 closed this Jun 2, 2026

mobin-2008 mentioned this pull request Jun 4, 2026

Let dinitctl print service file path #511

Closed

mobin-2008 reopened this Jun 10, 2026

mobin-2008 force-pushed the dinit_show_service_dir branch from 4a646c5 to d03c4d5 Compare June 10, 2026 22:12

dinitctl: show the service path in the "status" command output

c4be545

Use the existing functionality to show the service path of currently loaded service. Addresses davmac314#508

mobin-2008 force-pushed the dinit_show_service_dir branch from d03c4d5 to c4be545 Compare June 11, 2026 09:55

davmac314 reviewed Jun 11, 2026

View reviewed changes

mobin-2008 commented Jun 16, 2026

View reviewed changes

Uh oh!

Conversation

mobin-2008 commented May 31, 2026

Uh oh!

davmac314 commented Jun 1, 2026

Uh oh!

mobin-2008 commented Jun 1, 2026

Uh oh!

davmac314 commented Jun 2, 2026

Uh oh!

mobin-2008 commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davmac314 commented Jun 10, 2026

Uh oh!

davmac314 commented Jun 11, 2026

Uh oh!

mobin-2008 commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davmac314 commented Jun 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mobin-2008 Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davmac314 commented Jun 11, 2026

Uh oh!

mobin-2008 Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mobin-2008 commented Jun 16, 2026

Uh oh!

davmac314 commented Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mobin-2008 commented Jun 10, 2026 •

edited

Loading

mobin-2008 commented Jun 11, 2026 •

edited

Loading

mobin-2008 Jun 16, 2026 •

edited

Loading

mobin-2008 Jun 16, 2026 •

edited

Loading