Skip to content
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

Feat 1757 add signoff option #1758

Merged
merged 13 commits into from
Jul 29, 2023

Conversation

domtac
Copy link
Contributor

@domtac domtac commented Jul 13, 2023

This Pull Request fixes/closes #1757 .

It changes the following:

  • In commit edit via ctrl-s the sign-off feature of git will be toggled

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

Dominik Tacke added 7 commits July 12, 2023 21:56
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
Signed-off-by: Dominik Tacke <tacdom+github@gmail.com>
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
Copy link
Owner

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

I would like to keep the impact on the asyncgit crate minimal. lets keep the add_sign_off as a pub method we expose and lets make the command just add the sign-off part at commit-msg-edit time. this way its not a mode either. the command just triggers the sign-off addition

CHANGELOG.md Outdated Show resolved Hide resolved
src/components/commit.rs Outdated Show resolved Hide resolved
@domtac domtac marked this pull request as draft July 14, 2023 06:12
@domtac
Copy link
Contributor Author

domtac commented Jul 14, 2023

I would like to keep the impact on the asyncgit crate minimal. lets keep the add_sign_off as a pub method we expose and lets make the command just add the sign-off part at commit-msg-edit time. this way its not a mode either. the command just triggers the sign-off addition

Sure thing. Thanks for the remark. I will be off a couple of days. Expect my changes by end of next week latest.
I converted this PR to draft state until I worked in the proposed changes.

Dominik Tacke added 4 commits July 14, 2023 08:16
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
@domtac domtac marked this pull request as ready for review July 25, 2023 19:23
@domtac
Copy link
Contributor Author

domtac commented Jul 25, 2023

I would like to keep the impact on the asyncgit crate minimal. lets keep the add_sign_off as a pub method we expose and lets make the command just add the sign-off part at commit-msg-edit time. this way its not a mode either. the command just triggers the sign-off addition

Sure thing. Thanks for the remark. I will be off a couple of days. Expect my changes by end of next week latest. I converted this PR to draft state until I worked in the proposed changes.

@extrawurst Hi thanks again for the feedback and for the patience until I found time to address your issues.

Please have a look.

CHANGELOG.md Outdated Show resolved Hide resolved
src/components/commit.rs Outdated Show resolved Hide resolved
Dominik Tacke added 2 commits July 25, 2023 22:09
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
Signed-off-by Dominik Tacke <tacdom+github@gmail.com>
@domtac domtac requested a review from extrawurst July 28, 2023 11:08
@extrawurst
Copy link
Owner

LGTM - let's start with a simple version. if you add it twice and forget to clean it its not nice but not a bug right?

@domtac
Copy link
Contributor Author

domtac commented Jul 29, 2023

LGTM - let's start with a simple version. if you add it twice and forget to clean it its not nice but not a bug right?

Aboslutely. I would say that putting the sign-off multiple times is rather an inconvenience than a bug. I'd let it like it is.
Thanks for all your comments and time invested

@extrawurst extrawurst merged commit dba5206 into extrawurst:master Jul 29, 2023
19 checks passed
let mut msg = msg.to_owned();
if let (Some(user), Some(mail)) = (user, mail) {
msg.push_str(&format!(
"\n\nSigned-off-by {user} <{mail}>"

Choose a reason for hiding this comment

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

There's the : missing after the key of the trailer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reported in #2196.

) -> CommandText {
CommandText::new(
format!(
"Sing-off [{}]",

Choose a reason for hiding this comment

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

Typo.

Copy link
Owner

Choose a reason for hiding this comment

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

@matthiasbeyer this PR is already merged, could you open a PR fixing the typos?

IndianBoy42 pushed a commit to IndianBoy42/gitui that referenced this pull request Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add signoffto commits
4 participants