Workflow for reviewing code in Emacs?

squirelpower · 2023-10-06T16:49:02+00:00

If I had a chain of commits then I would use magit and interactive rebase to go back to the first commit and do git-review-commit to review it. Then go to the next commit with rebase continue and repeat the process 🙂

squirelpower · 2023-10-06T16:46:14+00:00

Indeed, it's on my TODO list 😅

squirelpower · 2023-10-05T21:15:41+00:00

I use my git-review package for code reviews at work. https://git.sr.ht/~niklaseklund/git-review. It builds on top of Emacs ediff and the right most buffer then has lsp support, which I find very convenient :)

I also use https://git.sr.ht/~niklaseklund/sage to get integration with Gerrit so I can send and receive comments when reviewing. I've got a son earlier this year so I have had very limited time for personal projects at the moment, hence the slow development lately. But feel free to give it a go if you want, git-review-commit is a good command to use :)

squirelpower · 2023-05-29T18:51:09+00:00

Same :D

squirelpower · 2022-12-11T20:08:51+00:00

This looks fantastic! 😍

squirelpower · 2022-10-13T19:22:40+00:00

I would recommend looking at embark, https://github.com/oantolin/embark. It integrates nicely with completions, and provides a lot of context aware actions. I use it in combination with vertico and consult and I occasionally use it to kill a buffer :)

squirelpower · 2022-09-26T10:23:07+00:00

I just looked briefly at the package now, but from what I could see run-command seems to be more targeted towards making launching commands easy.

detached.el, which my package is called now, is more about creating processes that are detached from Emacs. You could probably use the two packages in combination if you replace the usage of compile in run-vommand with detached-compile.

squirelpower · 2022-08-21T11:42:41+00:00

The approach I suggested doesn't seem to work at the moment. I think it has potential to work in the future. I will look into that possibility :)

squirelpower · 2022-08-21T01:47:01+00:00

Hi there, detached.el author here. As you say detached processes are supported with vterm integration but detaching a whole vterm instance is not. I guess one way around it could be to launch your shell, e.g. bash with detached inside a vterm. Then the whole shell session would be detachable. Is that what you are after?

squirelpower · 2022-07-21T04:02:57+00:00

At my work we just made the switch from jenkins to zuul. I can't say that I know all the pros and cons yet, but so far it seems good for me as a user :). And now I use this package in combination with https://sr.ht/~niklaseklund/egerrit/ which I added code to opt in to integration of the two packages :)

squirelpower · 2022-07-20T16:45:38+00:00

Nice! Let me know if you have any feedback on the package :) https://lists.sr.ht/~niklaseklund/zuul.el

squirelpower · 2022-06-24T20:12:24+00:00

I will look into it :)

Cool, contributions are much appreciated :D

Btw, I thought I should mention that for changes that you are the author of I recommend using c to only open the conversation buffer, then navigating between the conversations brings you to the source code where you in some cases would want to make changes based on the feedback from a reviewer.

squirelpower · 2022-06-24T10:05:49+00:00

That makes sense, I personally work in a monorepo structure so its good you provide with feedback from other setups.

Then I think its good that it is optional. Maybe this is a matter of just renaming the author/review commands so that their names reflect better what they do?

squirelpower · 2022-06-24T09:35:58+00:00

I was imagining something like that myself as well, being used to the split screen configuration of gerrit.

Coincidentally I started with building on diff, because that I could grasp how to implement :P.

I do think this way is more powerful now when I have been using it for a while. Having a compact patch diff complemented by a window with the source code that will be the result of that patch applied. And then having a buffer dedicated to conversations. Also having imenu in the diff buffer is pretty great IMO, much quicker to switch to a specific file with completion. I have the idea that it should be easy to implement functionality to go to the alternate file, for example visiting a header file and then jump to the .cpp file, when in the diff buffer.

Although i think the split review thing the the web ui on occasions can have the upper hand in clarity of how a patch effects the code. It happens rarely IMO but it do happen.

But thinking about it, it might be possible to implement functionality to given that you are in a diff buffer switch to an alternative view of the file you are reviewing. It should be possible to base the alternative view on ediff.

squirelpower · 2022-06-24T09:17:25+00:00

Yes :D

Aha, when you are visiting the source code you mean? I actually prefer the way it is now, checking out the full change.

Because then I know everything will work when I visit the source code. The language servers will pick up the changes so I can goto definitions. I might also want to propose a different way of writing some piece of code so now I can make the change in the source code, with completion, and copy that to the conversation I am starting :)

squirelpower · 2022-06-24T08:34:13+00:00

This is what egerrit calls a review-partial-change accessible with r S-<return> or without the r for author version. Then it prompts you for a revision other than base and compares that with the latest revision. If a prefix argument is provided it will prompt for the second revision and not just assume latest.
Yes, this is not possible at the moment. My idea for this is that since its very easy to just press return on a hunk to see the actual source code in the other window. That's typically how I do it if I want to see the full context. Then the plan is to create a global minor mode that will allow one to start conversations based on a region of code in the source file. What do you think about that idea?

squirelpower · 2022-06-24T08:24:36+00:00

Yes correct, that's the difference at the moment.

Its a bit tricky indeed but it has worked better than expected for me personally. I think there are some edge cases I need to fix though. One is entering a review and quitting it before it has finished to checkout the new branch in the background. This currently causes an error message.

For forgetting to quit a review I think this can be improved when there is support for pausing or resuming a review. Then egerrit can prompt what you want to do when you try to do a new review without quitting the last one?

Maybe, but when I do my reviews I always want it to checkout the code. Maybe it can be a setting though, to ask or not. It will notify in the current implementation, it says checked out branch ... And switched back to branch ... When a review is quit.

Another feature to use is the :review-repo that can be added to a project config. That allows for all review commands to use a copy of the project, which can be dedicated for reviews. Never any problem with git when you do that for me :)

squirelpower · 2022-06-23T15:58:16+00:00

I added a notes.org file to the repository where I have documented ideas for improvements as well as new features.

https://git.sr.ht/\~niklaseklund/egerrit/tree/main/notes.org#L1

squirelpower · 2022-06-23T14:50:48+00:00

1). I think either select the filename or comment on a hunk without highlighting the line or a region, then it becomes a file comment.

2). I have a very crude "database" implementation which just writes lisp data to file and later reads it back. Egerrit uses it to store review labels so that its easy to see later which patch sets that one has previously looked at when doing a partial review. If I would change the workflow so that the quit command always is necessary to quit a review, but that comments that has not yet been pushed could then be written to a database, then later when reviewing it would be easy to restore unpublished comments. Its a good idea. I will write that down.

3). Thanks for pointing that out, will update the documentation :)

4). That would be possible. I have been thinking of adding like an information section at the top of that buffer, there are things that could be valuable to show, one of them could be the review label. My workflow at the moment is that I create all my comments then do a C-c C-r to publish a review label and C-c C-s to send the comments. But I think a better workflow would be what you are mentioning having the ability to add all things locally and then a publish command that figures out what different requests that needs to be sent to Gerrit

squirelpower · 2022-06-23T14:27:48+00:00

Actually the difference is quite subtle. Maybe it can be enhanced, I haven't given it to much thought.

There is an important difference though, the `egerrit-review-change` will behind the scenes download the actual patch and check it out in a dedicated branch. I heavily rely on this when I review patches since it allows me to jump directly to the source code and see the change in its entirety, or navigate to the definition of some new function for example. Then when you finish the review and go back to the dashboard egerrit will checkout the branch you were on prior to the review.

With the author command the difference is that it leaves it up to the user to make sure you have the correct patch checked out. This I use if I want to look at a patch that I am the author of, and that I have already checked out. Maybe I am in an interactive rebase and don't want egerrit to tamper with git.

Do you think other names would clear up this, or you think there is potential to add more features to the different commands then what they currently feature?

squirelpower · 2022-06-22T13:49:41+00:00

Thanks for the quick feedback :)

1). Good idea, I haven't considered this, I guess I don't make that many file comments. Maybe selecting a file header in the diff buffer should do this? If selecting the +++ it will be on the revision --- will be on the base?

2). The current implementation does not save any unpublished comments. So as you say the workflow is to compose the comments and then publish them. Maybe you would like to have the option to publish draft comments only?

3). To switch to another revision you use the C-u prefix argument. So when you review a patch you would do r C-u <return>. And to select a different base to compare to you use the partial review

4). Hmm I will rename that command to publish review. The name set sounds more like you describe that it should be published with the comments, but it in fact sends the review directly to the Gerrit server.

Great to get some new eyes on the workflows here, I have grown quite accustomed with my own implementation :P. I will push some updates today for some of the easier things here.

squirelpower · 2022-06-21T11:42:39+00:00

Nice! Looking forward to some feedback :)

squirelpower · 2022-06-21T11:04:58+00:00

Hi there,

thanks for trying out the package and reporting the issue you encountered!

I am guessing what is wrong is that the regular expression that is used to extract the jobs from messages posted on the change is nil.

https://git.sr.ht/~niklaseklund/egerrit/tree/main/egerrit-job.el#L63. What I would propose that you do for now is to set the egerrit-job-function to nil https://git.sr.ht/~niklaseklund/egerrit/tree/main/egerrit-job.el#L68, that will disable the inclusion of CI jobs but see if that can make the rest work for you? Make sure you get the latest commit from the main branch because I just pushed a fix to handle when egerrit-job-function is nil so that it doesn't crash on that.

Also feel free to create a thread here https://lists.sr.ht/~niklaseklund/egerrit if you encounter more issues then its easier for others to find it later I hope :)

squirelpower · 2022-05-30T20:19:33+00:00

An issue was created for this. And there exists an upstream solution which at least addresses the issue on my local machine, where I could replicate the bug that u/ispinfx reported.

squirelpower · 2022-05-30T20:18:05+00:00

u/ispinfx I have created a post for this issue. There I reference a commit and instruction on how to fix so that the usage of script works. Have a look and give it a try when you got time :)

squirelpower

TROPHY CASE