Watch20

Star402

Fork

You've already forked forgejo-cli

Code Issues 44 Pull requests 8 Releases 11 Packages 2 Wiki Activity

feat: use git's `core.editor` if available #385

Merged

Cyborusmerged 4 commits from florian-obernberger/forgejo-cli:main into main 2026-03-25 03:48:34 +01:00

Conversation 8 Commits 4 Files changed 3 +42 -17

florian-obernberger commented 2026-03-24 09:41:21 +01:00

Contributor

Copy link

Changes Made

This implements the feature described in #382:

Check if git core.editor is available
If available parse out flags and use
If not fall back to old approach of using $EDITOR with special case support

Code of Conduct

I agree to act in accordance with the CoC & AI Agreement.
This contribution was not generated by an LLM, even in part.

Changes Made This implements the feature described in #382: - Check if `git core.editor` is available - If available parse out flags and use - If not fall back to old approach of using `$EDITOR` with special case support ### Code of Conduct - [x] I agree to act in accordance with the CoC & AI Agreement. - [x] This contribution was not generated by an LLM, even in part.

florian-obernberger added 3 commits 2026-03-24 09:41:22 +01:00

feat(cli): get editor from git-config... bb891695d1

Get editor from git-config, which intern falls back to the $EDITOR
variable, if not available.

This also led to having to deal with arguments, as the git-config
`core.editor` can contain arguments (eg. `code --wait`).

If no arguments are present it defaults to the old behavior of providing
the `--wait` flag for certain known editors.

Merge remote-tracking branch 'refs/remotes/origin/main' 09c1cfc90e

feat: simplify command extraction and bubble error state

All checks were successful

ci/woodpecker/pr/check Pipeline was successful

Details

6c75691dcd

Cyborus requested changes 2026-03-24 17:31:43 +01:00 Dismissed

Cyborus left a comment

Copy link

Where does this fall-back to $EDITOR? It doesn't access that var anywhere.

src/main.rs Outdated

Show resolved Hide resolved

| | | | @ -176,0 +180,4 @@ | | | | | | | | | | | | let editor = PathBuf::from(args.remove(0)); | | | | | | let flags = if args.is_empty() { | | | | | | get_default_editor_flags(&editor).unwrap_or(args) |

Cyborus commented 2026-03-24 17:25:12 +01:00

Member

Copy link

args is already known to be empty at this point, so .unwrap_or(args) is equivalent to .unwrap_or_default(). Since this is the only place get_default_editor_flags is called, it seems unnecessary for it to return Option at all. Reverting to the original get_editor_flags and returning an empty array (or, Vec now) by default seems like a simpler way to do this.

florian-obernberger commented 2026-03-24 21:38:47 +01:00

Author

Contributor

Copy link

Thanks for pointing it out, I'll also update that :)

florian-obernberger commented 2026-03-24 21:41:15 +01:00

Author

Contributor

Copy link

The reason I used a vec btw is due to shlex returning a vec and I assumed it is more efficient to stick with that type instead of converting back to an array. I am aware that creating a vec has a bit more overhead tho, so I do not have an opinion on which is better.

Cyborus marked this conversation as resolved

florian-obernberger commented 2026-03-24 21:38:20 +01:00

Author

Contributor

Copy link

Huh; you are right, I apparently forgot to put that logic back in, I apologize ^^"

I'll add it back in now :)

Huh; you are right, I apparently forgot to put that logic back in, I apologize ^^" I'll add it back in now :)

florian-obernberger added 1 commit 2026-03-24 22:18:26 +01:00

rework: fix logic errors and add fallback to $EDITOR...

All checks were successful

ci/woodpecker/pr/check Pipeline was successful

Details

32489c186c

- Reworked some logic errors
- Added fallback to $EDITOR where previously erroneously assumed that git2 would fall back automatically

Ref:[#385 (comment)](https://codeberg.org/forgejo-contrib/forgejo-cli/pulls/385#issuecomment-12030597)

florian-obernberger commented 2026-03-24 22:19:51 +01:00

Author

Contributor

Copy link

So, I fixed the logic bugs and also added the $EDITOR fallback. I also remembered why I didn't in the first place: I had assumed that similarly to git itself the git2 library falls back to the environment variable on its own - it does not.

Sorry for the extra hassle, hopefully all or at least most kinks are ironed out now :)

Cyborus requested changes 2026-03-24 22:53:52 +01:00 Dismissed

src/main.rs

Show resolved Hide resolved

| | | | @ -176,0 +191,4 @@ | | | | | | let flags = if args.is_empty() { | | | | | | get_default_editor_flags(&editor) | | | | | | } else { | | | | | | args |

Cyborus commented 2026-03-24 22:51:25 +01:00

Member

Copy link

RE: what you said in your reply to the other review:

get_default_editor_flags could still return &'static [&'static str], and this line could be changed to &args which would turn it also into a slice. No extra allocation needed! Turning a Vec into a slice &[T] (not an array [T; N]!) is free.

RE: what you said in your reply to the other review: get_default_editor_flags could still return &'static [&'static str], and this line could be changed to &args which would turn it also into a slice. No extra allocation needed! Turning a Vec into a slice &[T] (not an array [T; N]!) is free.

florian-obernberger commented 2026-03-25 01:28:29 +01:00

Author

Contributor

Copy link

Okay, maybe I am misunderstanding, but &args simply resolves to a &Vec<String>, not a &[&str]. And since the vector has type String and not str I'd still have to map all Strings to &strs—which may not be as much overhead as I think it could be. But I couldn't find a simple lets say type coersion from Vec<String> to &[&str].

Here's a SC from my editor where I tried experimenting to get the desired str slice

Okay, maybe I am misunderstanding, but &args simply resolves to a &Vec\<String\>, not a &[&str]. And since the vector has type String and not str I'd still have to map all Strings to &strs—which may not be as much overhead as I think it could be. But I couldn't find a simple lets say type coersion from Vec\<String\> to &[&str]. Here's a SC from my editor where I tried experimenting to get the desired str slice

[image.png](https://codeberg.org/attachments/f6ed71aa-36ca-4e1b-b1b6-0839cd24886b "Click to see "image.png" in a new tab")

22 KiB

Cyborus commented 2026-03-25 01:32:02 +01:00

Member

Copy link

Ah, shoot. Strings and &strs again... nevermind then, it's good as-is!

❤️ 1

florian-obernberger commented 2026-03-25 01:33:37 +01:00

Author

Contributor

Copy link

Yeah, that's what I struggled with as well 🙃

florian-obernberger marked this conversation as resolved

florian-obernberger commented 2026-03-25 01:43:04 +01:00

Author

Contributor

Copy link

One more thing that I forgot to mention/ask: shlex is specifically for parsing posix commands; is there a high chance that windows users will use tools where commands dont follow posix rules in regards to command/argument parsing? Personally I'd say its rather unlikely as I cannot think of a single command rn that on WIndows that doesnt follow it (taskkill /f is still valid posix command I think, just unconventional)

Cyborus approved these changes 2026-03-25 01:44:55 +01:00

Cyborus commented 2026-03-25 01:49:32 +01:00

Member

Copy link

@florian-obernberger wrote in #385 (comment):

is there a high chance that windows users will use tools where commands dont follow posix rules in regards to command/argument parsing?

Unlikely? For much the same reasoning, most windows commands still technically fit it. If something ever comes up where it doesn't, it can be fixed then.

@florian-obernberger wrote in https://codeberg.org/forgejo-contrib/forgejo-cli/pulls/385#issuecomment-12058188: > is there a high chance that windows users will use tools where commands dont follow posix rules in regards to command/argument parsing? Unlikely? For much the same reasoning, most windows commands still technically fit it. If something ever comes up where it doesn't, it can be fixed then.

👍 1

florian-obernberger commented 2026-03-25 01:50:51 +01:00

Author

Contributor

Copy link

Perfect, just wanted to make I don't forget to mention it :)

Cyborus merged commit 234c38fa41 into main 2026-03-25 03:48:34 +01:00

Cyborus referenced this pull request from a commit 2026-03-25 03:48:36 +01:00 Merge pull request 'feat: use git's core.editor if available' (#385) from florian-obernberger/forgejo-cli:main into main

stalecontext referenced this pull request from stalecontext/forgejo-cli-plus 2026-03-31 14:08:19 +02:00 Upstream sync 2026-03-31 #48

stalecontext referenced this pull request from stalecontext/forgejo-cli-plus 2026-03-31 14:29:33 +02:00 Upstream sync 2026-03-31 #48

stalecontext referenced this pull request from a commit 2026-03-31 15:38:05 +02:00 Upstream sync 2026-03-31 (#48)

Cyborus added this to the v0.5.0 milestone 2026-04-16 05:05:00 +02:00

Reviewers

No reviewers

Cyborus

Labels

Clear labels[ Kind/Breaking Breaking change that won't be backward compatible

](#)[ Kind/Bug Something is not working

](#)[ Kind/Design Discussion about UI/UX design

](#)[ Kind/Documentation Documentation changes

](#)[ Kind/Enhancement Improve existing functionality

](#)[ Kind/Feature New functionality

](#)[ Kind/Security This is security issue

](#)[ Kind/Testing Issue or pull request related to testing

](#)[ Kind/Upstream This is an issue with upstream software (Forgejo) that is probably not our fault

](#)

[ Priority

Critical The priority is critical

](#)[ Priority

High The priority is high

](#)[ Priority

Low The priority is low

](#)[ Priority

Medium The priority is medium

](#)

[ Reviewed

Confirmed Issue has been confirmed

](#)[ Reviewed

Duplicate This issue or pull request already exists

](#)[ Reviewed

Invalid Invalid issue

](#)[ Reviewed

Won't Fix This issue won't be fixed

](#)

[ Status

Abandoned Somebody has started to work on this but abandoned work

](#)[ Status

Blocked Something is blocking this issue or pull request

](#)[ Status

Need More Info Feedback is required to reproduce issue or to continue work

](#)

[ Suspicious

](#)

No labels Kind/Breaking Kind/Bug Kind/Design Kind/Documentation Kind/Enhancement Kind/Feature Kind/Security Kind/Testing Kind/Upstream [ Priority

Critical ](/forgejo-contrib/forgejo-cli/pulls?labels=173012) [ Priority

High ](/forgejo-contrib/forgejo-cli/pulls?labels=173013) [ Priority

Low ](/forgejo-contrib/forgejo-cli/pulls?labels=173015) [ Priority

Medium ](/forgejo-contrib/forgejo-cli/pulls?labels=173014) [ Reviewed

Confirmed ](/forgejo-contrib/forgejo-cli/pulls?labels=173007) [ Reviewed

Duplicate ](/forgejo-contrib/forgejo-cli/pulls?labels=173005) [ Reviewed

Invalid ](/forgejo-contrib/forgejo-cli/pulls?labels=173006) [ Reviewed

Won't Fix ](/forgejo-contrib/forgejo-cli/pulls?labels=173008) [ Status

Abandoned ](/forgejo-contrib/forgejo-cli/pulls?labels=173011) [ Status

Blocked ](/forgejo-contrib/forgejo-cli/pulls?labels=173010) [ Status

Need More Info ](/forgejo-contrib/forgejo-cli/pulls?labels=173009) Suspicious

Milestone

Clear milestone

No items

No milestone v0.5.0

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo-contrib/forgejo-cli!385

WritePreview

Loading…

Add table

CancelOK

Add a link

Url Description Hint: With a URL in your clipboard, you can paste directly into the editor to create a link.

CancelOK

CancelSave

Reference in a new issue

Repository

forgejo-contrib/forgejo-cli

Title

Body

Create issue

No description provided.

Delete branch "florian-obernberger/forgejo-cli:main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?

No Yes

feat: use git's core.editor if available #385

feat: use git's core.editor if available #385

Changes Made

Code of Conduct

feat: use git's `core.editor` if available #385