kernel: Declare a useless function #203

alevkoy · 2022-12-08T23:54:14Z

Add a poorly declared thread function for demo purposes.

Signed-off-by: Abe Levkoy [email protected]

Signed-off-by: Anas Nashif <[email protected]>

Add a poorly declared thread function for demo purposes. Signed-off-by: Abe Levkoy <[email protected]>

alevkoy · 2022-12-08T23:59:09Z

This is a proof-of-concept PR to try out using fixup commits during reviews and autosquash at the end of the review. Context: zephyrproject-rtos/zephyr#52731

People may review this, and I'll make changes in the form of pushing fixup commits. Once people like the whole thing, I'll autosquash and push again. Hopefully, it will be obvious to the reviewers that I didn't change anything else, but it will have been easier for them to review intermediate changes than it would be if I updated a whole stack of commits and force pushed.

Adding another commit to make the squash a little more interesting.

keith-zephyr · 2022-12-09T00:11:34Z

include/zephyr/kernel.h

@@ -5832,6 +5832,14 @@ int k_thread_runtime_stats_get(k_tid_t thread,
 */
 int k_thread_runtime_stats_all_get(k_thread_runtime_stats_t *stats);

+/** 


Please fix the trailing white space.

Fixed in 86c2994.

keith-zephyr · 2022-12-09T00:13:15Z

include/zephyr/kernel.h

+ * @param thread ID of thread
+ * @return 0 on success, negative error code on failure
+ */
+void z_describe_thread(k_tid_t thread, const char *description);


How about creating a symmetrical API. z_thread_set_friendly_name and z_thread_get_friendly_name.

Fixed in fixup commits.

alevkoy · 2022-12-09T00:17:34Z

@galak Some time ago, the process improvement WG discussed creating a test PR to try out autosquash during code reviews. IDK if you want to personally give this a try, but can you add someone who might be interested to this review?

stephanosio · 2022-12-09T05:53:52Z

kernel/thread.c

+
+void z_describe_thread(k_tid_t thread, const char *description)
+{
+    int description_size = 1000; /* Max size of a description */


(dummy review) description_size could be 128.

Fixed in 46c6b6a.

Fix formatting, add a getter API Signed-off-by: Abe Levkoy <[email protected]>

Adjust buffer size, fix formatting, add getter API This commit message is now wrong and will need to be updated as well. Signed-off-by: Abe Levkoy <[email protected]>

alevkoy

Responded to review comments in fixup commits, i.e. commits that will be squashed with previous commits before merging.

I started out using git commit --fixup in anticipation of using git rebase --autosquash later. This would have produced commit messages of the form fixup! <previous headline>. However, I like to use that feature locally for my own purposes, and keeping track of the fixup commits I wanted to upload vs the ones I wanted to squash locally before anyone saw them became confusing.

So I used a slightly different commit message format here, and I'll manually rearrange the commits the way that --autosquash would have when I finally squash. That's one lesson learned so far.

alevkoy · 2022-12-09T21:50:20Z

include/zephyr/kernel.h

@@ -5832,6 +5832,14 @@ int k_thread_runtime_stats_get(k_tid_t thread,
 */
 int k_thread_runtime_stats_all_get(k_thread_runtime_stats_t *stats);

+/** 


Fixed in 86c2994.

alevkoy · 2022-12-09T21:50:29Z

include/zephyr/kernel.h

+ * @param thread ID of thread
+ * @return 0 on success, negative error code on failure
+ */
+void z_describe_thread(k_tid_t thread, const char *description);


Fixed in fixup commits.

alevkoy · 2022-12-09T21:51:00Z

kernel/thread.c

+
+void z_describe_thread(k_tid_t thread, const char *description)
+{
+    int description_size = 1000; /* Max size of a description */


Fixed in 46c6b6a.

alevkoy · 2022-12-13T23:20:37Z

@stephanosio @keith-zephyr Looking at the PR as it currently exists, how is the re-review experience?

carlescufi · 2022-12-27T11:43:24Z

@alevkoy this is the wrong repo. This repo is only for testing CI and GitHub actions.
Please post a draft PR to https://github.com/zephyrproject-rtos/zephyr

alevkoy · 2022-12-27T17:08:35Z

@carlescufi I don't intend to submit this. I just want to demo a review workflow based on fixups.

carlescufi · 2022-12-27T17:33:22Z

@carlescufi I don't intend to submit this. I just want to demo a review workflow based on fixups.

Oh apologies, I realize this now.

alevkoy · 2023-01-04T23:22:05Z

Happy new year! Anyone want to take a look at my fixup commits?

stephanosio · 2023-01-05T03:28:06Z

Happy new year! Anyone want to take a look at my fixup commits?

Could you explain how the auto-squash part is supposed to work?

It might also be helpful if you could join the Process WG meeting next week and demonstrate how the auto-squash part is supposed to work.

alevkoy · 2023-01-05T17:41:40Z

Each "FIXUP" commit refers to a previous commit in the PR. The fixup commits contain changes in response to comments. My hope is that this will make it easier for the people who originally requested the changes, e.g. you, to review the newly changed code.

So please review the new commits and let me know how the experience as a reviewer is.

Could you explain how the auto-squash part is supposed to work?

Once you like how the code looks, I'll perform a rebase in which I squash the fixup commits onto their corresponding previous commits. I'll then push again. This will be a force push, because I'm rewriting existing commits instead of just appending. But there should be essentially no code changes, and reviewers should be able to quickly verify that using the "compare" button in Github.

I will then need to request review again, because the commits changed, but it should be a formality, because the reviewers already looked at all the code. You'll just be double checking that the commits are organized in a way that you like.

If you approve, then I'll be able to submit as normal. However, I will probably not actually submit this PR, because, as noted in the commit message, the actual code change is useless. It will have served its purpose to experiment with this review process.

It might also be helpful if you could join the Process WG meeting next week and demonstrate how the auto-squash part is supposed to work.

I'll do that.

mbolivar-nordic · 2023-01-06T00:42:53Z

@alevkoy if you're going to push for this, then I would say we should at the very least be following the commit shortlog formats expected by git rebase --autosquash instead of this hand-rolled format.

That said I am kind of skeptical about this but I am also happy to add this to the process WG agenda for next week. Please make sure there's a corresponding issue filed with the 'Process' label in the main zephyr git repository and send me a ping on Discord or email with a link to that issue by Tuesday mid-day US pacific time and I'll make sure it's on the agenda. Thanks!

alevkoy · 2023-01-06T17:15:57Z

@alevkoy if you're going to push for this, then I would say we should at the very least be following the commit shortlog formats expected by git rebase --autosquash instead of this hand-rolled format.

I started out doing that, but I found that it interfered with my own behind-the-scenes use of --autosquash during my development process. I could be convinced to switch back and make do.

Please make sure there's a corresponding issue filed with the 'Process' label in the main zephyr git repository and send me a ping on Discord or email with a link to that issue by Tuesday mid-day US pacific time and I'll make sure it's on the agenda. Thanks!

Done. I'll follow up on Discord.

sonarqubecloud · 2024-01-07T01:31:43Z

Quality Gate failed

Failed conditions

C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

stephanosio and others added 5 commits November 27, 2022 13:16

[TEST] Adjust path for zephyr-testing

a690857

[TEST] Print runner information

a5a189f

[TEST] Use test-runner

6a7b2fc

ci: track new bugs in a project

c6ca717

Signed-off-by: Anas Nashif <[email protected]>

kernel: Declare a useless function

64a7af5

Add a poorly declared thread function for demo purposes. Signed-off-by: Abe Levkoy <[email protected]>

thread: Implement z_describe_thread

d7bd9f1

Adding another commit to make the squash a little more interesting.

keith-zephyr suggested changes Dec 9, 2022

View reviewed changes

stephanosio requested review from galak, mbolivar-nordic, nashif, yperess, teburd, stephanosio and carlescufi December 9, 2022 05:47

stephanosio reviewed Dec 9, 2022

View reviewed changes

Abe Levkoy added 2 commits December 9, 2022 14:40

FIXUP: kernel: Declare a useless function

86c2994

Fix formatting, add a getter API Signed-off-by: Abe Levkoy <[email protected]>

FIXUP: thread: Implement z_describe_thread

46c6b6a

Adjust buffer size, fix formatting, add getter API This commit message is now wrong and will need to be updated as well. Signed-off-by: Abe Levkoy <[email protected]>

alevkoy commented Dec 9, 2022

View reviewed changes

alevkoy mentioned this pull request Dec 15, 2022

docs: Add Committer Expectations document zephyrproject-rtos/zephyr#52731

Merged

stephanosio force-pushed the main branch 15 times, most recently from a8ae4ac to 8ada639 Compare December 16, 2023 15:33

stephanosio force-pushed the main branch 2 times, most recently from ff9facc to 231c7a2 Compare December 18, 2023 17:06

nashif force-pushed the main branch 5 times, most recently from 52fb4c3 to 1990754 Compare December 22, 2023 13:49

stephanosio force-pushed the main branch from 59163a6 to ed8578d Compare January 4, 2024 01:18

nashif force-pushed the main branch 5 times, most recently from d7cc31b to cd9ff80 Compare January 8, 2024 16:01

alevkoy closed this Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: Declare a useless function #203

kernel: Declare a useless function #203

alevkoy commented Dec 8, 2022

alevkoy commented Dec 8, 2022

keith-zephyr Dec 9, 2022

alevkoy Dec 9, 2022

keith-zephyr Dec 9, 2022

alevkoy Dec 9, 2022

alevkoy commented Dec 9, 2022

stephanosio Dec 9, 2022

alevkoy Dec 9, 2022

alevkoy left a comment

alevkoy Dec 9, 2022

alevkoy Dec 9, 2022

alevkoy Dec 9, 2022

alevkoy commented Dec 13, 2022

carlescufi commented Dec 27, 2022

alevkoy commented Dec 27, 2022

carlescufi commented Dec 27, 2022

alevkoy commented Jan 4, 2023

stephanosio commented Jan 5, 2023

alevkoy commented Jan 5, 2023

mbolivar-nordic commented Jan 6, 2023

alevkoy commented Jan 6, 2023

sonarqubecloud bot commented Jan 7, 2024

kernel: Declare a useless function #203

kernel: Declare a useless function #203

Conversation

alevkoy commented Dec 8, 2022

alevkoy commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alevkoy commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alevkoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alevkoy commented Dec 13, 2022

carlescufi commented Dec 27, 2022

alevkoy commented Dec 27, 2022

carlescufi commented Dec 27, 2022

alevkoy commented Jan 4, 2023

stephanosio commented Jan 5, 2023

alevkoy commented Jan 5, 2023

mbolivar-nordic commented Jan 6, 2023

alevkoy commented Jan 6, 2023

sonarqubecloud bot commented Jan 7, 2024

Quality Gate failed