threads / patch / 36119

patch[GSoC] parse-options: Add OPT_SET_INT_NONEG.

Subject: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.

## tl;dr

6 messages between Mar 11, 2014 and Mar 12, 2014. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Yuxuan Shui· Mar 11, 2014, 10:50 UTC · lore
Reference: http://git.github.io/SoC-2014-Microprojects.html
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
---
 builtin/fetch.c        |  5 ++---
 builtin/merge.c        |  5 ++---
 builtin/notes.c        | 10 ++++------
 builtin/pack-objects.c | 15 ++++++---------
 builtin/update-index.c | 20 ++++++++------------
 parse-options.h        |  4 ++++
 6 files changed, 26 insertions(+), 33 deletions(-)
Show changes to 6 files +26 −33

builtin/fetch.c, builtin/merge.c, builtin/notes.c, builtin/pack-objects.c, builtin/update-index.c, parse-options.h

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..37c2a9d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -97,9 +97,8 @@ static struct option builtin_fetch_options[] = {
 	OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
 	OPT_STRING(0, "depth", &depth, N_("depth"),
 		   N_("deepen history of shallow clone")),
-	{ OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
-		   N_("convert to a complete repository"),
-		   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
+	OPT_SET_INT_NONEG(0, "unshallow", &unshallow,
+		   N_("convert to a complete repository"), 1),
 	{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, N_("dir"),
 		   N_("prepend this to submodule path output"), PARSE_OPT_HIDDEN },
 	{ OPTION_STRING, 0, "recurse-submodules-default",
diff --git a/builtin/merge.c b/builtin/merge.c
index f0cf120..cf9a157 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -203,9 +203,8 @@ static struct option builtin_merge_options[] = {
 	OPT_BOOL('e', "edit", &option_edit,
 		N_("edit message before committing")),
 	OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
-	{ OPTION_SET_INT, 0, "ff-only", &fast_forward, NULL,
-		N_("abort if fast-forward is not possible"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, FF_ONLY },
+	OPT_SET_INT_NONEG(0, "ff-only", &fast_forward,
+		N_("abort if fast-forward is not possible"), FF_ONLY),
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_BOOL(0, "verify-signatures", &verify_signatures,
 		N_("Verify that the named commit has a valid GPG signature")),
diff --git a/builtin/notes.c b/builtin/notes.c
index 2b24d05..02a554d 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -739,13 +739,11 @@ static int merge(int argc, const char **argv, const char *prefix)
 			   N_("resolve notes conflicts using the given strategy "
 			      "(manual/ours/theirs/union/cat_sort_uniq)")),
 		OPT_GROUP(N_("Committing unmerged notes")),
-		{ OPTION_SET_INT, 0, "commit", &do_commit, NULL,
-			N_("finalize notes merge by committing unmerged notes"),
-			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},
+		OPT_SET_INT_NONEG(0, "commit", &do_commit,
+			N_("finalize notes merge by committing unmerged notes"), 1),
 		OPT_GROUP(N_("Aborting notes merge resolution")),
-		{ OPTION_SET_INT, 0, "abort", &do_abort, NULL,
-			N_("abort notes merge"),
-			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},
+		OPT_SET_INT_NONEG(0, "abort", &do_abort,
+			N_("abort notes merge"), 1),
 		OPT_END()
 	};
 
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..2e2b12a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2579,15 +2579,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			 N_("do not create an empty pack output")),
 		OPT_BOOL(0, "revs", &use_internal_rev_list,
 			 N_("read revision arguments from standard input")),
-		{ OPTION_SET_INT, 0, "unpacked", &rev_list_unpacked, NULL,
-		  N_("limit the objects to those that are not yet packed"),
-		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
-		{ OPTION_SET_INT, 0, "all", &rev_list_all, NULL,
-		  N_("include objects reachable from any reference"),
-		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
-		{ OPTION_SET_INT, 0, "reflog", &rev_list_reflog, NULL,
-		  N_("include objects referred by reflog entries"),
-		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
+		OPT_SET_INT_NONEG(0, "unpacked", &rev_list_unpacked,
+		  N_("limit the objects to those that are not yet packed"), 1),
+		OPT_SET_INT_NONEG(0, "all", &rev_list_all,
+		  N_("include objects reachable from any reference"), 1),
+		OPT_SET_INT_NONEG(0, "reflog", &rev_list_reflog,
+		  N_("include objects referred by reflog entries"), 1),
 		OPT_BOOL(0, "stdout", &pack_to_stdout,
 			 N_("output pack to stdout")),
 		OPT_BOOL(0, "include-tag", &include_tag,
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d12ad95..807e853 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -749,18 +749,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			N_("override the executable bit of the listed files"),
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 			chmod_callback},
-		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
-			N_("mark files as \"not changing\""),
-			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
-		{OPTION_SET_INT, 0, "no-assume-unchanged", &mark_valid_only, NULL,
-			N_("clear assumed-unchanged bit"),
-			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
-		{OPTION_SET_INT, 0, "skip-worktree", &mark_skip_worktree_only, NULL,
-			N_("mark files as \"index-only\""),
-			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
-		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
-			N_("clear skip-worktree bit"),
-			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		OPT_SET_INT_NONEG(0, "assume-unchanged", &mark_valid_only,
+			N_("mark files as \"not changing\""), MARK_FLAG),
+		OPT_SET_INT_NONEG(0, "no-assume-unchanged", &mark_valid_only,
+			N_("clear assumed-unchanged bit"), UNMARK_FLAG),
+		OPT_SET_INT_NONEG(0, "skip-worktree", &mark_skip_worktree_only,
+			N_("mark files as \"index-only\""), MARK_FLAG),
+		OPT_SET_INT_NONEG(0, "no-skip-worktree", &mark_skip_worktree_only,
+			N_("clear skip-worktree bit"), UNMARK_FLAG),
 		OPT_SET_INT(0, "info-only", &info_only,
 			N_("add to index only; do not add content to object database"), 1),
 		OPT_SET_INT(0, "force-remove", &force_remove,
diff --git a/parse-options.h b/parse-options.h
index d670cb9..7d20cf9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -125,6 +125,10 @@ struct option {
 				      (h), PARSE_OPT_NOARG }
 #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (i) }
+#define OPT_SET_INT_NONEG(s, l, v, h, i)  \
+				      { OPTION_SET_INT, (s), (l), (v), NULL, \
+				      (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, \
+				      NULL, (i) }
 #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
 #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}

-- 
1.9.0
Duy Nguyen· Mar 12, 2014, 10:47 UTC · re: Yuxuan Shui · lore

Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.

By convention, no full stop in the subject line. The subject should summarize your changes and "add ..NONEG" is just one part of it. The other is "convert to use ...NONEG". So I suggest "parse-options: convert to use new macro OPT_SET_INT_NONEG()" or something like that.

You should also explain in the message body (before Signed-off-by:) why this is a good thing to do. My guess is better readability and harder to make mistakes in the future when you have to declare new options with noneg.

On Tue, Mar 11, 2014 at 5:50 PM, Yuxuan Shui <yshuiv7@gmail.com> wrote:
> Reference: http://git.github.io/SoC-2014-Microprojects.html

I think this project is actually two: one is convert current {OPTION_SET_INT, ... _NONEG} to the new macro, which is truly a micro project. The other is to find OPT_...(..) that should have NONEG but does not. This one may need more time because you need to check what those options do and if it makes sense to have --no- form.

I think we can focus on the {OPTION_..., _NONEG} conversion, which should be enough get you familiar with git community.

Show 15 quoted lines
> diff --git a/parse-options.h b/parse-options.h
> index d670cb9..7d20cf9 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -125,6 +125,10 @@ struct option {
>                                       (h), PARSE_OPT_NOARG }
>  #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
>                                       (h), PARSE_OPT_NOARG, NULL, (i) }
> +#define OPT_SET_INT_NONEG(s, l, v, h, i)  \
> +                                     { OPTION_SET_INT, (s), (l), (v), NULL, \
> +                                     (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, \
> +                                     NULL, (i) }
>  #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
>  #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
>                                       (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}

To avoid the proliferation of similar macros in future, I think we should make a macro that takes any flags, e.g.

#define OPT_SET_INT_X(s, l, v, h, i, flags) {  ....., PARSE_OPT_NOARG
| PARSE_OPT_ ## flags, NULL, (i) }

and we can use it for NONEG like "OPT_SET_INT_X(...., NONEG)". We could even redefine OPT_SET_INT() to use OPT_SET_INT_X() to reduce duplication.

While we're at NONEG, I see that builtin/grep.c has this construct "{ OPTION_INTEGER...NONEG}" and builtin/read-tree.c has "{ OPTION_STRING..NONEG}". It would be great if you could look at them and see if NONEG is really needed there, or simpler forms OPT_INTEGER(...) and OPT_STRING(...) are enough.

You might need to read parse-options.c to understand these options. Documentation/technical/api-parse-options.txt should give you a good overview.

You could also think if we could transform "{ OPTION_CALLBACK.... }" to OPT_CALLBACK(...). But if you do and decide to do it, please make it a separate patch (one patch deals with one thing).

That remaining of your patch looks good.
-- 
Duy
Yuxuan Shui· Mar 12, 2014, 17:25 UTC · re: Duy Nguyen · lore

Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.

On Wed, Mar 12, 2014 at 6:47 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 9 quoted lines
> By convention, no full stop in the subject line. The subject should
> summarize your changes and "add ..NONEG" is just one part of it. The
> other is "convert to use ...NONEG". So I suggest "parse-options:
> convert to use new macro OPT_SET_INT_NONEG()" or something like that.
>
> You should also explain in the message body (before Signed-off-by:)
> why this is a good thing to do. My guess is better readability and
> harder to make mistakes in the future when you have to declare new
> options with noneg.
Thanks for pointing that out, I'll do as you suggested.
Show 9 quoted lines
>
> On Tue, Mar 11, 2014 at 5:50 PM, Yuxuan Shui <yshuiv7@gmail.com> wrote:
>> Reference: http://git.github.io/SoC-2014-Microprojects.html
>
> I think this project is actually two: one is convert current
> {OPTION_SET_INT, ... _NONEG} to the new macro, which is truly a micro
> project. The other is to find OPT_...(..) that should have NONEG but
> does not. This one may need more time because you need to check what
> those options do and if it makes sense to have --no- form.

Hmm, this microproject has been striked from the ideas page, maybe I should switch to another one...

Show 29 quoted lines
>
> I think we can focus on the {OPTION_..., _NONEG} conversion, which
> should be enough get you familiar with git community.
>
>> diff --git a/parse-options.h b/parse-options.h
>> index d670cb9..7d20cf9 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -125,6 +125,10 @@ struct option {
>>                                       (h), PARSE_OPT_NOARG }
>>  #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
>>                                       (h), PARSE_OPT_NOARG, NULL, (i) }
>> +#define OPT_SET_INT_NONEG(s, l, v, h, i)  \
>> +                                     { OPTION_SET_INT, (s), (l), (v), NULL, \
>> +                                     (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, \
>> +                                     NULL, (i) }
>>  #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
>>  #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
>>                                       (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
>
> To avoid the proliferation of similar macros in future, I think we
> should make a macro that takes any flags, e.g.
>
> #define OPT_SET_INT_X(s, l, v, h, i, flags) {  ....., PARSE_OPT_NOARG
> | PARSE_OPT_ ## flags, NULL, (i) }
>
> and we can use it for NONEG like "OPT_SET_INT_X(...., NONEG)". We
> could even redefine OPT_SET_INT() to use OPT_SET_INT_X() to reduce
> duplication.
I could do that, but it seems only the NONEG flag is used in the code.
Show 6 quoted lines
>
> While we're at NONEG, I see that builtin/grep.c has this construct "{
> OPTION_INTEGER...NONEG}" and builtin/read-tree.c has "{
> OPTION_STRING..NONEG}". It would be great if you could look at them
> and see if NONEG is really needed there, or simpler forms
> OPT_INTEGER(...) and OPT_STRING(...) are enough.

I've grep'd through the source code, and most of the manually filled option structures don't seems to have a pattern. And I think writing a overly generalized macro won't help much.

Show 10 quoted lines
>
> You might need to read parse-options.c to understand these options.
> Documentation/technical/api-parse-options.txt should give you a good
> overview.
>
> You could also think if we could transform "{ OPTION_CALLBACK.... }"
> to OPT_CALLBACK(...). But if you do and decide to do it, please make
> it a separate patch (one patch deals with one thing).
>
> That remaining of your patch looks good.
Thanks for reviewing my patch.
> --
> Duy
-- 
Regards
Yuxuan Shui
Junio C Hamano· Mar 12, 2014, 18:30 UTC · re: Duy Nguyen · lore

Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.

Duy Nguyen <pclouds@gmail.com> writes:
Show 9 quoted lines
> By convention, no full stop in the subject line. The subject should
> summarize your changes and "add ..NONEG" is just one part of it. The
> other is "convert to use ...NONEG". So I suggest "parse-options:
> convert to use new macro OPT_SET_INT_NONEG()" or something like that.
>
> You should also explain in the message body (before Signed-off-by:)
> why this is a good thing to do. My guess is better readability and
> harder to make mistakes in the future when you have to declare new
> options with noneg.

As I said elsewhere, I am not sure if doubling the number of OPT_<type> macros as your micro suggestion proposes is the right solution to the problem.

What are we trying to solve?
Yuxuan Shui· Mar 12, 2014, 18:33 UTC · re: Junio C Hamano · lore

Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.

Hi,
On Thu, Mar 13, 2014 at 2:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> By convention, no full stop in the subject line. The subject should
>> summarize your changes and "add ..NONEG" is just one part of it. The
>> other is "convert to use ...NONEG". So I suggest "parse-options:
>> convert to use new macro OPT_SET_INT_NONEG()" or something like that.
>>
>> You should also explain in the message body (before Signed-off-by:)
>> why this is a good thing to do. My guess is better readability and
>> harder to make mistakes in the future when you have to declare new
>> options with noneg.
>
> As I said elsewhere, I am not sure if doubling the number of
> OPT_<type> macros as your micro suggestion proposes is the right
> solution to the problem.
>
> What are we trying to solve?
OK, I'll switch to another micro project.
-- 
Regards
Yuxuan Shui
Junio C Hamano· Mar 12, 2014, 19:02 UTC · re: Yuxuan Shui · lore

Re: [PATCH][GSoC] parse-options: Add OPT_SET_INT_NONEG.

Yuxuan Shui <yshuiv7@gmail.com> writes:
Show 20 quoted lines
> On Thu, Mar 13, 2014 at 2:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> By convention, no full stop in the subject line. The subject should
>>> summarize your changes and "add ..NONEG" is just one part of it. The
>>> other is "convert to use ...NONEG". So I suggest "parse-options:
>>> convert to use new macro OPT_SET_INT_NONEG()" or something like that.
>>>
>>> You should also explain in the message body (before Signed-off-by:)
>>> why this is a good thing to do. My guess is better readability and
>>> harder to make mistakes in the future when you have to declare new
>>> options with noneg.
>>
>> As I said elsewhere, I am not sure if doubling the number of
>> OPT_<type> macros as your micro suggestion proposes is the right
>> solution to the problem.
>>
>> What are we trying to solve?
>
> OK, I'll switch to another micro project.

That is fine, but note that it was not an objection but was a pure question. I was asking you to explain why this is a good change.

← back to recent threads