uftrace: Add sub-command specific help #1946

ParkSeungHyeok · 2024-08-14T13:13:13Z

uftrace: Add sub-command specific help

i redirect subcommand help to man pages just like git and perf do.

	case -3:
+		if (opts.mode) {
+			execlp("man", "man", "uftrace", argv[1], (char *)NULL);
+			abort();
+		}
		if (opts.use_pager)
			start_pager(setup_pager());
		pr_out(uftrace_usage);
		pr_out(uftrace_help);
		wait_for_pager();
		ret = 0;
		goto cleanup;
	}

Fixed: #1316

Signed-off-by: ParkSeungHyeok tmdgur1324@naver.com

gichoel · 2024-08-14T15:01:49Z

First, thanks for posting the commit that fixes #1316.

However, for a better commit message, you should follow the 50/72 rule[1], and the body of this commit message doesn't seem to do that.

Could you please make this change?

And this is my personal opinion based on existing cefd9be, 0ac2cef PRs, but I think the ** before Fixed is unnecessary :)

[1] https://www.gitkraken.com/learn/git/best-practices/git-commit-message#git-commit-message-structure

honggyukim · 2024-08-14T15:12:55Z

Thanks for the PR @ParkSeungHyeok and thanks for the review @gichoel.
In addition, please remove the diff in the commit message.

yskelg · 2024-08-14T15:34:40Z

uftrace.c

-		pr_out(uftrace_usage);
-		pr_out(uftrace_help);
-		wait_for_pager();
+		if (!(opts.mode)) {


Hello @ParkSeungHyeok, We can follow the convention simply with !opts.mode.

ParkSeungHyeok · 2024-08-14T15:59:51Z

thank you for review @gichoel , @honggyukim, @yskelg
I fixed the commit message to follow the 50/72 rule.
and regarding "the diff" (If i understand correctly), there was in the first commit message, but not now.
lastly i also fixed the code to follow the convention with !opts.mode

yskelg · 2024-08-14T16:11:28Z

Thank you @ParkSeungHyeok, LGTM

honggyukim

Thanks for the update. Your name is currently written as ParkSeungHyeok, but it is better to be changed because I don't think you write your name without having spaces.

Maybe one of the following is better.

Park SeungHyeok
SeungHyeok Park
Seunghyeok Park

You can choose which one to use. Please make sure you update your name in both in the commit author name and Signed-off-by.

uftrace.c

ParkSeungHyeok · 2024-08-15T13:04:42Z

@honggyukim i agree your recommendation. it looks better.
and i changed my name in the commit author name and Signed-off-by.

honggyukim · 2024-08-16T02:07:22Z

Thanks. One more request is to change the error message when man uftrace <command> isn't available in the system. It currently shows as follows.

# ./uftrace record --help
No manual entry for uftrace
No manual entry for record

Maybe we better handle it just like perf as follows.

# perf record --help
No manual entry for perf-record

ParkSeungHyeok · 2024-08-16T14:06:34Z

@honggyukim Okay i agree then how about below code?
i tried to implement that similar to Git and Perf.

+static void show_man_page(char *sub_command)
+{
+	char * page = xmalloc(sizeof("uftrace-"));
+	strcpy(page,"uftrace-");
+	page = strjoin(page,sub_command,"");
+	execlp("man", "man", page, (char *)NULL);
+}

int main(int argc, char *argv[])
{
	...
	case -3:
+		if (opts.mode) {
+			show_man_page(argv[1]);
+			abort();
+		}
		if (opts.use_pager)
			start_pager(setup_pager());
		pr_out(uftrace_usage);
		pr_out(uftrace_help);
		wait_for_pager();
		ret = 0;
		goto cleanup;
	}
	...
}

i not sure if it is okay to make new function
so here's an alternative approach:

int main(int argc, char *argv[])
{
	...
+	char * page = xmalloc(sizeof("uftrace-"));
	...
	case -3:
+		if (opts.mode) {
+			strcpy(page,"uftrace-");
+			page = strjoin(page,argv[1],"");
+			execlp("man", "man", page, (char *)NULL);
+			abort();
+		}
		if (opts.use_pager)
			start_pager(setup_pager());
		pr_out(uftrace_usage);
		pr_out(uftrace_help);
		wait_for_pager();
		ret = 0;
		goto cleanup;
	}
	...
}

namhyung

Thanks for the contribution!

namhyung · 2024-08-17T00:51:37Z

uftrace.c

@@ -1428,6 +1428,10 @@ int main(int argc, char *argv[])
 		ret = 0;
 		goto cleanup;
 	case -3:
+		if (opts.mode) {
+			execlp("man", "man", "uftrace", argv[1], (char *)NULL);
+			abort();


I don't think we need abort() here and maybe it's better to call exit(1) instead. But we used to show the full message for the --help option, so it'd be natural to fall back to the old behavior if no manual pages are available.

abort() here is to show this is unreachable. It doesn't hit the abort() even if No manual entry for ... error message is shown.

But it may be reachable if man command itself is not found in the system, which is very unlikely. Maybe the following is better to cover such case as well.

case -3: if (opts.mode) { execlp("man", "man", "uftrace", argv[1], (char *)NULL); /* fall through if man command itself is not found */ } if (opts.use_pager) start_pager(setup_pager()); pr_out(uftrace_usage); pr_out(uftrace_help); wait_for_pager(); ret = 0; goto cleanup;

But we used to show the full message for the --help option, so it'd be natural to fall back to the old behavior if no manual pages are available.

Once the man command is executed by execlp, then there is no way to go back to the fallback routine. If that is really needed, then we should check whether the proper man page can be found before calling execlp. But I think we don't have to make it too much complicated.

Oh.. ok. But sometimes man might not be installed. I admit it's a rare condition. :)

honggyukim · 2024-08-17T01:18:54Z

Considering all our conversation, we can think about this.

static void show_man_page(char *cmd)
{
        char *cmdstr = NULL;

        if (cmd)
                xasprintf(&cmdstr, "uftrace-%s", cmd);
        else
                cmdstr = xstrdup("uftrace");
        execlp("man", "man", cmdstr, (char *)NULL);
        /* fall through if man command itself is not found */
        free(cmdstr);
}
    ...
int main(int argc, char *argv[])
{
            ...
        switch (parse_options(argc, argv, &opts)) {
            ...
        case -3:
                if (opts.mode)
                        show_man_page(argv[1]);
                if (opts.use_pager)
                        start_pager(setup_pager());
                pr_out(uftrace_usage);
                pr_out(uftrace_help);
                wait_for_pager();
                ret = 0;
                goto cleanup;
        }
        ...
}

ParkSeungHyeok · 2024-08-17T11:54:59Z

thank you for review @namhyung @honggyukim
i reflected your review

but i wonder how to reach below code

        else
                cmdstr = xstrdup("uftrace");

is it for another case later?

honggyukim · 2024-08-17T12:02:32Z

My mistake. if (cmdstr) should be if (cmd).

honggyukim · 2024-08-17T12:44:17Z

You should put show_man_page inside #ifndef UNIT_TEST right above main function to avoid the following unittest error.

/home/runner/work/uftrace/uftrace/uftrace.c:1386:13: error: ‘show_man_page’ defined but not used [-Werror=unused-function]
 1386 | static void show_man_page(char *cmd)
      |             ^~~~~~~~~~~~~
cc1: all warnings being treated as errors

https://github.com/namhyung/uftrace/actions/runs/10432184123/job/28892638730?pr=1946

uftrace.c

honggyukim · 2024-08-17T13:04:36Z

Looks good now. The last request is please read other commit message and make it similar.

I'm typing in my phone so can't tell you the details but please read other commit messages more to learn the convention.

ParkSeungHyeok · 2024-08-17T13:11:18Z

thank you for your detailed review @honggyukim
i missed if (cmdstr) too
i changed that to if(cmd)
and i added __used front of the function to avoid unittest error like apply_default_opts() funciton

but although it is if(cmd), i still wonder how to reach below code

        else
                cmdstr = xstrdup("uftrace");

i mean...
show_man_page function is only called as show_man_page(argv[1])
and when show_man_page(argv[1]) is called, argv[1] always isn't NULL because of if(opts.mode)

and...
i will change commit message after i read other commit message

honggyukim · 2024-08-17T13:26:15Z

thank you for your detailed review @honggyukim

Thanks for your work and effort as well!

but although it is if(cmd), i still wonder how to reach below code
        else cmdstr = xstrdup("uftrace");
i mean...
show_man_page function is only called as show_man_page(argv[1])

That is the caller dependent point of view. When you write a function, you better handle all the possible cases regardless of the input argument.

i will change commit message after i read other commit message

I would write the commit message as follows.

uftrace: Add sub-command specific help

This patch adds sub-command specific help by redirecting the help
message to their man pages of each command just like git and perf do.

The simple usage is as follows.

  $ uftrace [COMMAND] --help

Then you will see the man page of given `[COMMAND]` such as record,
replay and so on.

Fixed: #1316

Signed-off-by: Seunghyeok Park <tmdgur1324@naver.com>

honggyukim · 2024-08-17T13:52:00Z

It's just a nitpick but please begin a sentence in a capital letter unless you have a strong reason for that.

You can think the commit message like sending an email to someone.

This patch adds sub-command specific help by redirecting the help message to their man pages of each command just like git and perf do. The simple usage is as follows. $ uftrace [COMMAND] --help Then you will see the man page of given '[COMMAND]' such as record, replay and so on. Fixed: namhyung#1316 Signed-off-by: Seunghyeok Park <tmdgur1324@naver.com>

ParkSeungHyeok · 2024-08-17T13:55:07Z

thank you @honggyukim
i reflected your review

honggyukim

LGTM! Thanks for your persistence.

ParkSeungHyeok force-pushed the sub-command_help branch 4 times, most recently from 2f1fac3 to 0a198f0 Compare August 14, 2024 13:41

ParkSeungHyeok force-pushed the sub-command_help branch from 0a198f0 to 42477f7 Compare August 14, 2024 15:30

yskelg reviewed Aug 14, 2024

View reviewed changes

ParkSeungHyeok force-pushed the sub-command_help branch from 42477f7 to f738867 Compare August 14, 2024 15:57

honggyukim reviewed Aug 15, 2024

View reviewed changes

uftrace.c Outdated Show resolved Hide resolved

ParkSeungHyeok force-pushed the sub-command_help branch from f738867 to f0572f5 Compare August 15, 2024 13:00

namhyung reviewed Aug 17, 2024

View reviewed changes

ParkSeungHyeok force-pushed the sub-command_help branch from f0572f5 to e263f78 Compare August 17, 2024 11:54

ParkSeungHyeok force-pushed the sub-command_help branch from e263f78 to 328c10a Compare August 17, 2024 12:19

honggyukim reviewed Aug 17, 2024

View reviewed changes

uftrace.c Outdated Show resolved Hide resolved

ParkSeungHyeok force-pushed the sub-command_help branch from 328c10a to c25aca6 Compare August 17, 2024 12:59

ParkSeungHyeok force-pushed the sub-command_help branch from c25aca6 to 4216d6a Compare August 17, 2024 13:49

ParkSeungHyeok force-pushed the sub-command_help branch from 4216d6a to 1848e1a Compare August 17, 2024 13:54

honggyukim approved these changes Aug 17, 2024

View reviewed changes

namhyung merged commit 7e07a43 into namhyung:master Aug 18, 2024
3 checks passed

ParkSeungHyeok deleted the sub-command_help branch September 1, 2024 05:03

honggyukim mentioned this pull request Sep 3, 2024

doc/ko: Add missing options in uftrace-replay.md #1957

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uftrace: Add sub-command specific help #1946

uftrace: Add sub-command specific help #1946

ParkSeungHyeok commented Aug 14, 2024 •

edited

Loading

gichoel commented Aug 14, 2024

honggyukim commented Aug 14, 2024

yskelg Aug 14, 2024

ParkSeungHyeok commented Aug 14, 2024 •

edited

Loading

yskelg commented Aug 14, 2024

honggyukim left a comment •

edited

Loading

ParkSeungHyeok commented Aug 15, 2024

honggyukim commented Aug 16, 2024

ParkSeungHyeok commented Aug 16, 2024 •

edited

Loading

namhyung left a comment

namhyung Aug 17, 2024

honggyukim Aug 17, 2024 •

edited

Loading

honggyukim Aug 17, 2024

namhyung Aug 17, 2024

honggyukim commented Aug 17, 2024 •

edited

Loading

ParkSeungHyeok commented Aug 17, 2024

honggyukim commented Aug 17, 2024

honggyukim commented Aug 17, 2024

honggyukim commented Aug 17, 2024

ParkSeungHyeok commented Aug 17, 2024 •

edited

Loading

honggyukim commented Aug 17, 2024

honggyukim commented Aug 17, 2024 •

edited

Loading

ParkSeungHyeok commented Aug 17, 2024

honggyukim left a comment

uftrace: Add sub-command specific help #1946

uftrace: Add sub-command specific help #1946

Conversation

ParkSeungHyeok commented Aug 14, 2024 • edited Loading

gichoel commented Aug 14, 2024

honggyukim commented Aug 14, 2024

yskelg Aug 14, 2024

Choose a reason for hiding this comment

ParkSeungHyeok commented Aug 14, 2024 • edited Loading

yskelg commented Aug 14, 2024

honggyukim left a comment • edited Loading

Choose a reason for hiding this comment

ParkSeungHyeok commented Aug 15, 2024

honggyukim commented Aug 16, 2024

ParkSeungHyeok commented Aug 16, 2024 • edited Loading

namhyung left a comment

Choose a reason for hiding this comment

namhyung Aug 17, 2024

Choose a reason for hiding this comment

honggyukim Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

honggyukim Aug 17, 2024

Choose a reason for hiding this comment

namhyung Aug 17, 2024

Choose a reason for hiding this comment

honggyukim commented Aug 17, 2024 • edited Loading

ParkSeungHyeok commented Aug 17, 2024

honggyukim commented Aug 17, 2024

honggyukim commented Aug 17, 2024

honggyukim commented Aug 17, 2024

ParkSeungHyeok commented Aug 17, 2024 • edited Loading

honggyukim commented Aug 17, 2024

honggyukim commented Aug 17, 2024 • edited Loading

ParkSeungHyeok commented Aug 17, 2024

honggyukim left a comment

Choose a reason for hiding this comment

ParkSeungHyeok commented Aug 14, 2024 •

edited

Loading

ParkSeungHyeok commented Aug 14, 2024 •

edited

Loading

honggyukim left a comment •

edited

Loading

ParkSeungHyeok commented Aug 16, 2024 •

edited

Loading

honggyukim Aug 17, 2024 •

edited

Loading

honggyukim commented Aug 17, 2024 •

edited

Loading

ParkSeungHyeok commented Aug 17, 2024 •

edited

Loading

honggyukim commented Aug 17, 2024 •

edited

Loading