Skip to content

Add commit --allow-empty-message option and fix empty message parsing in process_commit_log_data #482

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 5, 2020
Merged

Add commit --allow-empty-message option and fix empty message parsing in process_commit_log_data #482

merged 1 commit into from
Sep 5, 2020

Conversation

kpd200081
Copy link
Contributor

@kpd200081 kpd200081 commented Aug 11, 2020 •
edited
Loading

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

  • Ensure all commits include DCO sign-off.
  • Ensure that your contributions pass unit testing.
  • Ensure that your contributions contain documentation if applicable.

Description

If the commit doesn't contain a message, parsing the output of "git log"
results in an error, which occures because the end of the commit message
can't be detected.

fixes #481

@kpd200081
Copy link
Contributor Author

@rvodden @perlun @tarcinil, please can somebody review this pull request?

PS: build passed https://travis-ci.org/github/ruby-git/ruby-git/builds/716900598, I'm not sure why the status is not updating

@jcouball
Copy link
Member

I re-ran the build and the status was updated. Not sure why it wasn't updating before.

@jcouball
Copy link
Member

@kpd200081 can you please add a failing test that this change fixes?

You should probable create your own test fixture instead of changing existing fixtures. If you don't have time to figure this out, I can help out -- just let me know. Here is what I told someone else:

Reusing the default test fixture (test/files/working/dot_git) breaks tests that don't have anything to do with the change you are making. To me this is tech debt that is piling up with each change like this one. The next PR might have to correct all the same tests plus the new ones introduced in this PR. Additionally, I think merge conflicts will be more of a problem if everyone is trying to change the same test fixture for their tests.

You can see an example of using a different fixture in tests/units/test_diff_non_default_encoding.rb for using the fixture tests/files/encoding. It would probably be good to create a new git working copy setup just for your test instead of copying an existing one that has a lot of setup for other things.

@jcouball jcouball added this to the 1.8.0 milestone Aug 17, 2020
@jcouball jcouball added the Bug label Aug 17, 2020
@kpd200081
Copy link
Contributor Author

@jcouball, I'm not sure if a test is needed for this case.

Anyway, I'm not familiar with Ruby tests, so if you think it's necessary it would be nice if you could help with that.

@kpd200081 kpd200081 force-pushed the log-empty-message branch 2 times, most recently from 3bbf1e9 to 7340bc5 Compare August 29, 2020 10:27
@kpd200081
Copy link
Contributor Author

kpd200081 commented Aug 29, 2020 •
edited
Loading

@jcouball I added a test that falling without this fix, but I had to change the commit method a bit, take a look please

As we can see in travis it fail because run on published version of gem (I don't know why, locally I have same behavior)
Oh, now travis passed, I don't have idea what changed (maybe github got lost in my pushes...)

@kpd200081 kpd200081 force-pushed the log-empty-message branch 2 times, most recently from 6267880 to e725384 Compare August 30, 2020 08:05
jcouball
jcouball reviewed Aug 30, 2020
View reviewed changes
tests/units/test_log_empty_message.rb Outdated
set_file_paths
end

def test_repo_with_empty_commit
Copy link
Member

Choose a reason for hiding this comment

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

I think this test name should be changed to test_repo_with_empty_commit_message since empty commit is something different.

For this functionality, I would think to do two tests:
1.
a test that does not have a commit message and does not use the :allow_empty_message option
2. a test that does not have a commit message and DOES use the :allow_empty_messages option

Since the :allow_empty and :allow_empty_message options are so similar, I found the use of the :allow_empty option in this particular test confusing. I'll leave it up to you if you want to do anything about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this test name should be changed to test_repo_with_empty_commit_message since empty commit is something different.

oh, year, I did misspell here

Since the :allow_empty and :allow_empty_message options are so similar, I found the use of the :allow_empty option in this particular test confusing. I'll leave it up to you if you want to do anything about this.

:allow_empty just allow commit without changes, so it was added to both commits

Copy link
Contributor Author

@kpd200081 kpd200081 Aug 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

a test that does not have a commit message and does not use the :allow_empty_message option

we can't commit without this option, this results in an error in git and test failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, as test without :allow_empty_message can't work because of git internal error, can we resolve this?

@kpd200081 kpd200081 force-pushed the log-empty-message branch 4 times, most recently from daa5445 to b79eac0 Compare August 31, 2020 06:51
@jcouball jcouball changed the title Fix detecting of commit message in Git::Lib::process_commit_log_data Add commit --allow-empty-message option and fix empty message parsing in Git::Lib#process_commit_log_data Aug 31, 2020
@jcouball jcouball changed the title Add commit --allow-empty-message option and fix empty message parsing in Git::Lib#process_commit_log_data Add commit --allow-empty-message option and fix empty message parsing in process_commit_log_data Aug 31, 2020
jcouball
jcouball approved these changes Aug 31, 2020
View reviewed changes
Copy link
Member

@jcouball jcouball left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for the contribution.

@kpd200081
Add commit --allow-empty-message option and fix empty message parsing…
d2ce751 
… in process_commit_log_data

If the commit doesn't contain a message, parsing the output of "git log"
results in an error, which occures because the end of the commit message
can't be detected.
Additionaly was added option to commit without message.

Signed-off-by: Pavel Kuznetsov <kpd200081@yandex.ru>
@kpd200081
Copy link
Contributor Author

kpd200081 commented Aug 31, 2020 •
edited
Loading

@jcouball I fix commit's author and rebase it to remove merge commit, now I think nothing block from accepting PR.

@jcouball jcouball merged commit 896e31d into ruby-git:master Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouball jcouball jcouball approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.

Incorrect log if repo contains commits with empty message
2 participants
@kpd200081 @jcouball