Skip to content

Refactor git storage and command #22775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 6, 2023

No description provided.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 6, 2023
args []string
parentContext context.Context
desc string
extraDesc string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the default description, so we can remove most descriptions when invoking git command.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then SetDescription will be removed, right?

Copy link
Member Author

@lunny lunny Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still will be kept but in most places, we don't need that function.

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetDescription still will be kept but in most places, we don't need that function.

Then I still can not get the point why SetDescription and SetExtraDescription should both exist.

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After grepping the calls to SetDescription and SetExtraDescription. If I understand correctly, the SetDescription is mainly for making the description of git command with long arguments to be concise and add repoPath to the description.

But SetDescription and SetExtraDescription really seem similar and may be misused later ....

(just a discussion, no more thoughts from my side)

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thought about an interesting idea:

cmd.SetDescription("totally new description: %s, %d", filename, line)
cmd.SetDescription("${cmd}, and extra description: %s, %d", filename, line)

The ${cmd} could be replaced by the command and arguments.

Then only one function is enough. And one command always have one description (no extra). And no need to call Sprintf separately when setting the description.

ps: I know the trick ${cmd} is arguable, so not sure whether it is good enough, just FYI and for fun.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 6, 2023
@lunny
Copy link
Member Author

lunny commented Jun 7, 2023

replaced by #25071

@lunny lunny closed this Jun 7, 2023
@lunny lunny deleted the lunny/command_runner branch June 7, 2023 04:10
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants