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

[Outreachy] config support for allowEmpty in commit #1902

Closed
wants to merge 1 commit into from
Closed

[Outreachy] config support for allowEmpty in commit #1902

wants to merge 1 commit into from

Conversation

tanushree27
Copy link

This commit is for issue #1854. Added support for allowEmpty commit attribute in configuration.

@tanushree27 tanushree27 changed the title config support for allowEmpty in commit [Outreachy] config support for allowEmpty in commit Oct 27, 2018
Documentation/config.txt Outdated Show resolved Hide resolved
Copy link

@PhilipOakley PhilipOakley left a comment

Choose a reason for hiding this comment

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

I guess this will be going upstream as a patch once it has had a first review.

It is worth getting started on the patch submission process ('format-patch' and 'send-email'), first to yourself, and then to the main Git List, so that they also see the 'open' discussions and it let's them feel that they helped ;-)

Documentation/config.txt Outdated Show resolved Hide resolved
Documentation/git-commit.txt Outdated Show resolved Hide resolved
@dscho
Copy link
Member

dscho commented Oct 31, 2018

It is worth getting started on the patch submission process ('format-patch' and 'send-email'), first to yourself, and then to the main Git List, so that they also see the 'open' discussions and it let's them feel that they helped ;-)

We tried to make that easier via this document: https://github.com/git-for-windows/git/blob/master/CONTRIBUTING.md

builtin/commit.c Outdated Show resolved Hide resolved
Documentation/config.txt Show resolved Hide resolved
Documentation/config.txt Outdated Show resolved Hide resolved
builtin/commit.c Outdated Show resolved Hide resolved
builtin/commit.c Outdated Show resolved Hide resolved
Documentation/config.txt Outdated Show resolved Hide resolved
Documentation/git-commit.txt Show resolved Hide resolved
Documentation/git-commit.txt Show resolved Hide resolved
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Almost there!

test_must_fail git -c commit.allowEmpty=$i commit --allow-empty -m 'test'
"
done

This comment was marked as off-topic.

This comment was marked as off-topic.

Documentation/git-commit.txt Show resolved Hide resolved
when we cherrypick an existing commit it doesn't change anything and
therefore it fails prompting to reset (skip commit) or commit using
--allow-empty attribute and then continue.

Add commit.allowEmpty configuration variable as a convenience to skip
this process.

Add tests to check the behavior introduced by this commit.

This closes #1854

Signed-off-by: tanushree27 <[email protected]>
@dscho
Copy link
Member

dscho commented Nov 30, 2018

For the record, this patch has been contributed to the Git mailing list and can be viewed here: https://public-inbox.org/git/[email protected]/#r

@tanushree27 I think what ended up as consensus was that the original reporter had an XY problem (i.e. they thought they needed X but they really needed Y): instead of allowing empty commits by default, what they need is to skip now-empty commits in git cherry-pick <commit-range>.

While this would be another fun project, it cannot reuse any of your work in this here PR, unfortunately.

So maybe close this PR?

@tanushree27
Copy link
Author

Closed.
Yes, unfortunately we cannot reuse this. But I did learn to add a custom config. :)

@dscho
Copy link
Member

dscho commented Nov 30, 2018

I did learn to add a custom config.

Yep! And you learned to find your way around Git's code base. All good things.

@chucklu
Copy link

chucklu commented Feb 27, 2019

@tanushree27 Sorry, I mistook the options I wanted in description of #1854

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.

5 participants