Skip to content

Document that Unspecified options should initialize to -1 #238

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PhilipOakley
Copy link

@PhilipOakley PhilipOakley commented May 29, 2019

Many of Git's command-line options with optional arguments are handled internally as enum values, and most of them are initialized with the value -1.

This patch documents the -1 default for unspecified cli options as highlighted by Junio in a recent mail thread https://public-inbox.org/git/[email protected]/.

This resolves #237, suggested by #201 .

@dscho
Copy link
Member

dscho commented May 29, 2019

@PhilipOakley I see you branched off of Git for Windows' master, but GitGitGadget PRs can only target git.git's branches. Maybe you want to do this locally, then force-push?

git rebase -i --onto aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6 da5a923050c541b9761dfbe32e5748e1af0ae02d

@dscho
Copy link
Member

dscho commented May 29, 2019

Also, keep in mind that the PR description is used as cover letter; You might want to rephrase it a little, to motivate the readers of the Git mailing list to review your patch 😸

@dscho
Copy link
Member

dscho commented May 29, 2019

git rebase -i --onto aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6 da5a923050c541b9761dfbe32e5748e1af0ae02d

Or even better:

git rebase -i --onto v2.21.0 HEAD~1

This documents previous folk wisdom as discussed in
https://public-inbox.org/git/[email protected]/.

Raised recently in gitgitgadget#201
and git-for-windows#237.

Signed-off-by: Philip Oakley <[email protected]>
@PhilipOakley
Copy link
Author

various done-done things done. Hopefully complete.

Not sure if/how the PR message is displayed/shown/edited (relative to the commit message)

@PhilipOakley
Copy link
Author

I updated the initial comment, should that be the PR message!

@dscho
Copy link
Member

dscho commented May 30, 2019

I updated the initial comment, should that be the PR message!

Yes, indeed. I see that you never got the welcome message of GitGitGadget because you were allowed to use it before GitGitGadget was taught to greet new users. Here is an example: #229 (comment)

Reading your initial comment, I wonder whether you would want to "de-GitHub-ify" it a bit more:

This PR documents the -1 default for unspecified cli options as highlighted by Junio in a recent mail thread.

This resolves #237, suggested by #201 .

This cover letter talks about a PR (instead of a "patch series", but it would reach the mailing list as the latter rather than the former), it does not link to the "recent mail thread", and it does not really introduce the patch series. As a reader, I would be more compelled to review the patches if it read a bit more like: "Many of Git's command-line options with optional arguments are handled internally as enum values, and most of them are initialized with the value -1. Let's make this a rule that we recommend for any new contribution."

@PhilipOakley PhilipOakley changed the title Unspecified is 1 Document that Unspecified options should initialize to -1 May 30, 2019
@PhilipOakley
Copy link
Author

updated the PR message and title (GitHub had lost the - sign).

The commit message itself is quite short.

@dscho
Copy link
Member

dscho commented May 30, 2019

Looks good to me!

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.

CodingGuidelines: recommend to use -1 for unspecified values of command-line options of config settings
2 participants