-
-
Notifications
You must be signed in to change notification settings - Fork 607
Feat 1757 add signoff option #1758
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
Conversation
Signed-off-by Dominik Tacke <[email protected]>
Signed-off-by: Dominik Tacke <[email protected]>
Signed-off-by Dominik Tacke <[email protected]>
Signed-off-by Dominik Tacke <[email protected]>
Signed-off-by Dominik Tacke <[email protected]>
There was a problem hiding this 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
Sure thing. Thanks for the remark. I will be off a couple of days. Expect my changes by end of next week latest. |
Signed-off-by Dominik Tacke <[email protected]>
Signed-off-by Dominik Tacke <[email protected]>
Signed-off-by Dominik Tacke <[email protected]>
@extrawurst Hi thanks again for the feedback and for the patience until I found time to address your issues. Please have a look. |
Signed-off-by Dominik Tacke <[email protected]>
Signed-off-by Dominik Tacke <[email protected]>
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. |
let mut msg = msg.to_owned(); | ||
if let (Some(user), Some(mail)) = (user, mail) { | ||
msg.push_str(&format!( | ||
"\n\nSigned-off-by {user} <{mail}>" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 [{}]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo.
There was a problem hiding this comment.
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?
This Pull Request fixes/closes #1757 .
It changes the following:
I followed the checklist:
make check
without errors