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

Flag options in describe.h as being optional #4587

Merged
merged 1 commit into from Apr 6, 2018
Merged

Conversation

rcjsuen
Copy link
Contributor

@rcjsuen rcjsuen commented Mar 20, 2018

Both git_describe_options and git_describe_formatting_options are sanitized before they are actually used. They should be noted as being optional in the documentation.

libgit2/src/describe.c

Lines 641 to 654 in 5585e35

static int normalize_options(
git_describe_options *dst,
const git_describe_options *src)
{
git_describe_options default_options = GIT_DESCRIBE_OPTIONS_INIT;
if (!src) src = &default_options;
*dst = *src;
if (dst->max_candidates_tags > GIT_DESCRIBE_DEFAULT_MAX_CANDIDATES_TAGS)
dst->max_candidates_tags = GIT_DESCRIBE_DEFAULT_MAX_CANDIDATES_TAGS;
return 0;
}

libgit2/src/describe.c

Lines 767 to 778 in 5585e35

static int normalize_format_options(
git_describe_format_options *dst,
const git_describe_format_options *src)
{
if (!src) {
git_describe_init_format_options(dst, GIT_DESCRIBE_FORMAT_OPTIONS_VERSION);
return 0;
}
memcpy(dst, src, sizeof(git_describe_format_options));
return 0;
}

@tiennou
Copy link
Contributor

tiennou commented Mar 20, 2018

I think our "canonical" phrasing is "Can be NULL". Maybe something more to the point, like "Can be NULL, see git_(describe_)?_init_options for the default."

@rcjsuen
Copy link
Contributor Author

rcjsuen commented Mar 21, 2018

@tiennou Thank you for the review. Which would you prefer?

  1. the lookup options, can be NULL

  2. the lookup options, can be NULL, see git_describe_init_options for the default options

  3. the lookup options, optional and can be NULL

  4. the lookup options, optional and can be NULL, see git_describe_init_options for the default options

@tiennou
Copy link
Contributor

tiennou commented Mar 21, 2018

Oooh a vote 😉. I say 2 !

@ethomson
Copy link
Member

We should say that it may be NULL (ie, that we permit it), not that it can be NULL.

We probably shouldn't point people to git_describe_init_options, since it doesn't have additional documentation about what it's doing. It feels like our documentation should be self-contained and shouldn't point people to reading the source... I think that if we think that we want to provide more visibility to what the defaults are then we should figure out a way to surface that information in the documentation itself.

Personally, I think keeping it succinct - like "the lookup options (or null for defaults)" is adequate. But I also realize that we probably don't have any strict commonality amongst the various things that take options, so ... 🤷‍♂️

@rcjsuen
Copy link
Contributor Author

rcjsuen commented Mar 26, 2018

In the past, I've just appended the optional word (3a13365 and 93e2c74). I've also done what @ethomson suggested (21d4a37) before. 🤷‍♂️

@pks-t
Copy link
Member

pks-t commented Mar 27, 2018

The most common style we have in our code base is either "Structure with options to influence whatever or NULL for defaults." or "Options for whatever, or NULL for {defaults,default options}". I'd say "Options for whatever, or NULL for defaults" is the best way to go. It's short, concise, already used and one knows what to expect

The git_describe_options in git_describe_commit and
git_describe_workdir and the git_describe_format_options in
git_describe_format are optional and can be NULL. State this in the
documentation to make people's lives easier when calling these
functions.

Signed-off-by: Remy Suen <remy.suen@gmail.com>
@rcjsuen
Copy link
Contributor Author

rcjsuen commented Apr 5, 2018

Is the new version (db90e95) I made okay?

@pks-t pks-t merged commit a57f42a into libgit2:master Apr 6, 2018
@pks-t
Copy link
Member

pks-t commented Apr 6, 2018

It is. Thanks for this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants