* [PATCH] Make builtin-tag.c use parse_options.
@ 2007-11-09 13:42 Carlos Rica
2007-11-09 13:57 ` Jakub Narebski
2007-11-10 6:07 ` Junio C Hamano
0 siblings, 2 replies; 11+ messages in thread
From: Carlos Rica @ 2007-11-09 13:42 UTC (permalink / raw)
To: git, Junio C Hamano
Also, this removes those tests ensuring that repeated
-m options don't allocate memory more than once, because now
this is done after parsing options, using the last one
when more are given. The same for -F.
Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
Applied to "next".
Comments welcomed.
builtin-tag.c | 141 ++++++++++++++++++++++++--------------------------------
t/t7004-tag.sh | 8 +---
2 files changed, 61 insertions(+), 88 deletions(-)
diff --git a/builtin-tag.c b/builtin-tag.c
index 66e5a58..5af1950 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -11,9 +11,15 @@
#include "refs.h"
#include "tag.h"
#include "run-command.h"
-
-static const char builtin_tag_usage[] =
- "git-tag [-n [<num>]] -l [<pattern>] | [-a | -s | -u <key-id>] [-f | -d | -v] [-m <msg> | -F <file>] <tagname> [<head>]";
+#include "parse-options.h"
+
+static const char * const git_tag_usage[] = {
+ "git-tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
+ "git-tag -d <tagname>...",
+ "git-tag [-n [<num>]] -l [<pattern>]",
+ "git-tag -v <tagname>...",
+ NULL
+};
static char signingkey[1000];
@@ -308,101 +314,74 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
{
struct strbuf buf;
unsigned char object[20], prev[20];
- int annotate = 0, sign = 0, force = 0, lines = 0, message = 0;
char ref[PATH_MAX];
const char *object_ref, *tag;
- int i;
struct ref_lock *lock;
+ int annotate = 0, sign = 0, force = 0, lines = 0,
+ delete = 0, verify = 0;
+ char *list = NULL, *msg = NULL, *msgfile = NULL, *keyid = NULL;
+ const char *no_pattern = "NO_PATTERN";
+ struct option options[] = {
+ { OPTION_STRING, 'l', NULL, &list, "pattern", "list tag names",
+ PARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },
+ { OPTION_INTEGER, 'n', NULL, &lines, NULL,
+ "print n lines of each tag message",
+ PARSE_OPT_OPTARG, NULL, 1 },
+ OPT_BOOLEAN('d', NULL, &delete, "delete tags"),
+ OPT_BOOLEAN('v', NULL, &verify, "verify tags"),
+
+ OPT_GROUP("Tag creation options"),
+ OPT_BOOLEAN('a', NULL, &annotate,
+ "annotated tag, needs a message"),
+ OPT_STRING('m', NULL, &msg, "msg", "message for the tag"),
+ OPT_STRING('F', NULL, &msgfile, "file", "message in a file"),
+ OPT_BOOLEAN('s', NULL, &sign, "annotated and GPG-signed tag"),
+ OPT_STRING('u', NULL, &keyid, "key-id",
+ "use another key to sign the tag"),
+ OPT_BOOLEAN('f', NULL, &force, "replace the tag if exists"),
+ OPT_END()
+ };
+
git_config(git_tag_config);
- strbuf_init(&buf, 0);
- for (i = 1; i < argc; i++) {
- const char *arg = argv[i];
+ argc = parse_options(argc, argv, options, git_tag_usage, 0);
- if (arg[0] != '-')
- break;
- if (!strcmp(arg, "-a")) {
- annotate = 1;
- continue;
- }
- if (!strcmp(arg, "-s")) {
- annotate = 1;
- sign = 1;
- continue;
- }
- if (!strcmp(arg, "-f")) {
- force = 1;
- continue;
- }
- if (!strcmp(arg, "-n")) {
- if (i + 1 == argc || *argv[i + 1] == '-')
- /* no argument */
- lines = 1;
- else
- lines = isdigit(*argv[++i]) ?
- atoi(argv[i]) : 1;
- continue;
- }
- if (!strcmp(arg, "-m")) {
- annotate = 1;
- i++;
- if (i == argc)
- die("option -m needs an argument.");
- if (message)
- die("only one -F or -m option is allowed.");
- strbuf_addstr(&buf, argv[i]);
- message = 1;
- continue;
- }
- if (!strcmp(arg, "-F")) {
- annotate = 1;
- i++;
- if (i == argc)
- die("option -F needs an argument.");
- if (message)
- die("only one -F or -m option is allowed.");
-
- if (!strcmp(argv[i], "-")) {
+ if (list)
+ return list_tags(list == no_pattern ? NULL : list, lines);
+ if (delete)
+ return for_each_tag_name(argv, delete_tag);
+ if (verify)
+ return for_each_tag_name(argv, verify_tag);
+
+ strbuf_init(&buf, 0);
+ if (msg || msgfile) {
+ if (msg && msgfile)
+ die("only one -F or -m option is allowed.");
+ annotate = 1;
+ if (msg)
+ strbuf_addstr(&buf, msg);
+ else {
+ if (!strcmp(msgfile, "-")) {
if (strbuf_read(&buf, 0, 1024) < 0)
- die("cannot read %s", argv[i]);
+ die("cannot read %s", msgfile);
} else {
- if (strbuf_read_file(&buf, argv[i], 1024) < 0)
+ if (strbuf_read_file(&buf, msgfile, 1024) < 0)
die("could not open or read '%s': %s",
- argv[i], strerror(errno));
+ msgfile, strerror(errno));
}
- message = 1;
- continue;
- }
- if (!strcmp(arg, "-u")) {
- annotate = 1;
- sign = 1;
- i++;
- if (i == argc)
- die("option -u needs an argument.");
- if (strlcpy(signingkey, argv[i], sizeof(signingkey))
- >= sizeof(signingkey))
- die("argument to option -u too long");
- continue;
}
- if (!strcmp(arg, "-l"))
- return list_tags(argv[i + 1], lines);
- if (!strcmp(arg, "-d"))
- return for_each_tag_name(argv + i + 1, delete_tag);
- if (!strcmp(arg, "-v"))
- return for_each_tag_name(argv + i + 1, verify_tag);
- usage(builtin_tag_usage);
}
- if (i == argc) {
+ if (argc == 0) {
if (annotate)
- usage(builtin_tag_usage);
+ usage_with_options(git_tag_usage, options);
return list_tags(NULL, lines);
}
- tag = argv[i++];
+ tag = argv[0];
- object_ref = i < argc ? argv[i] : "HEAD";
- if (i + 1 < argc)
+ object_ref = argc == 2 ? argv[1] : "HEAD";
+ if (argc > 2)
die("too many params");
if (get_sha1(object_ref, object))
@@ -419,7 +398,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die("tag '%s' already exists", tag);
if (annotate)
- create_tag(object, tag, &buf, message, sign, object);
+ create_tag(object, tag, &buf, msg || msgfile, sign, object);
lock = lock_any_ref_for_update(ref, prev, 0);
if (!lock)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0d07bc3..4b09d28 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -339,20 +339,14 @@ test_expect_success \
'
test_expect_success \
- 'trying to create tags giving many -m or -F options should fail' '
+ 'trying to create tags giving both -m or -F options should fail' '
echo "message file 1" >msgfile1 &&
echo "message file 2" >msgfile2 &&
! tag_exists msgtag &&
- ! git-tag -m "message 1" -m "message 2" msgtag &&
- ! tag_exists msgtag &&
- ! git-tag -F msgfile1 -F msgfile2 msgtag &&
- ! tag_exists msgtag &&
! git-tag -m "message 1" -F msgfile1 msgtag &&
! tag_exists msgtag &&
! git-tag -F msgfile1 -m "message 1" msgtag &&
! tag_exists msgtag &&
- ! git-tag -F msgfile1 -m "message 1" -F msgfile2 msgtag &&
- ! tag_exists msgtag &&
! git-tag -m "message 1" -F msgfile1 -m "message 2" msgtag &&
! tag_exists msgtag
'
--
1.5.3.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-09 13:42 [PATCH] Make builtin-tag.c use parse_options Carlos Rica
@ 2007-11-09 13:57 ` Jakub Narebski
2007-11-09 14:31 ` Johannes Schindelin
2007-11-10 6:07 ` Junio C Hamano
1 sibling, 1 reply; 11+ messages in thread
From: Jakub Narebski @ 2007-11-09 13:57 UTC (permalink / raw)
To: git
Carlos Rica wrote:
> + struct option options[] = {
> + { OPTION_STRING, 'l', NULL, &list, "pattern", "list tag names",
> + PARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },
> + OPT_STRING('F', NULL, &msgfile, "file", "message in a file"),
Does it matter that you use OPTION_STRING here and OPT_STRING macro there?
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-09 13:57 ` Jakub Narebski
@ 2007-11-09 14:31 ` Johannes Schindelin
0 siblings, 0 replies; 11+ messages in thread
From: Johannes Schindelin @ 2007-11-09 14:31 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, jasampler
Hi,
[re Cc:ing jasam]
On Fri, 9 Nov 2007, Jakub Narebski wrote:
> Carlos Rica wrote:
>
> > + struct option options[] = {
> > + { OPTION_STRING, 'l', NULL, &list, "pattern", "list tag names",
> > + PARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },
>
> > + OPT_STRING('F', NULL, &msgfile, "file", "message in a file"),
>
> Does it matter that you use OPTION_STRING here and OPT_STRING macro there?
I guess it is because of the PARSE_OPT_OPTARG thing, together with
no_pattern. We need to know if -l was specified, even if no argument was
passed in.
Hth,
Dscho
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-09 13:42 [PATCH] Make builtin-tag.c use parse_options Carlos Rica
2007-11-09 13:57 ` Jakub Narebski
@ 2007-11-10 6:07 ` Junio C Hamano
2007-11-10 9:26 ` Junio C Hamano
` (2 more replies)
1 sibling, 3 replies; 11+ messages in thread
From: Junio C Hamano @ 2007-11-10 6:07 UTC (permalink / raw)
To: Carlos Rica; +Cc: git
Carlos Rica <jasampler@gmail.com> writes:
> Also, this removes those tests ensuring that repeated
> -m options don't allocate memory more than once, because now
> this is done after parsing options, using the last one
> when more are given. The same for -F.
The reason for this change is...? Is this because it is
cumbersome to detect and refuse multiple -m options using the
parseopt API? If so, the API may be what needs to be fixed.
Taking the last one and discarding earlier ones feels to me an
arbitrary choice.
While I freely admit that I do not particularly find the "One -m
introduces one new line, concatenated to form the final
paragraph" handling of multiple -m options done by git-commit
nice nor useful, I suspect that it would make more sense to make
git-tag and git-commit handle multiple -m option consistently,
if you are going to change the existing semantics. Since some
people really seem to like multiple -m handling of git-commit,
the avenue of the least resistance for better consistency would
be to accept and concatenate (with LF in between) multiple -m
options.
With multiple -F, I think erroring out would be the sensible
thing to do, but some people might prefer concatenation. I do
not care either way as long as commit and tag behave
consistently.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-10 6:07 ` Junio C Hamano
@ 2007-11-10 9:26 ` Junio C Hamano
2007-11-10 9:41 ` Junio C Hamano
2007-11-10 12:25 ` Carlos Rica
2007-11-12 13:09 ` Carlos Rica
2 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2007-11-10 9:26 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git, Carlos Rica
Junio C Hamano <gitster@pobox.com> writes:
> While I freely admit that I do not particularly find the "One -m
> introduces one new line, concatenated to form the final
> paragraph" handling of multiple -m options done by git-commit
> nice nor useful, I suspect that it would make more sense to make
> git-tag and git-commit handle multiple -m option consistently,
> if you are going to change the existing semantics. Since some
> people really seem to like multiple -m handling of git-commit,
> the avenue of the least resistance for better consistency would
> be to accept and concatenate (with LF in between) multiple -m
> options.
>
> With multiple -F, I think erroring out would be the sensible
> thing to do, but some people might prefer concatenation. I do
> not care either way as long as commit and tag behave
> consistently.
Alas, this exposes a regression in kh/commit series.
t/t7501-commit.sh | 23 +++++++++++++++++++++++
1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 1b444d4..bf5dd86 100644
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -178,4 +178,27 @@ test_expect_success 'amend commit to fix author' '
diff expected current
'
+
+test_expect_success 'sign off' '
+
+ >positive &&
+ git add positive &&
+ git commit -s -m "thank you" &&
+ actual=$(git cat-file commit HEAD | sed -ne "s/Signed-off-by: //p") &&
+ expected=$(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/") &&
+ test "z$actual" = "z$expected"
+
+'
+
+test_expect_success 'multiple -m' '
+
+ >negative &&
+ git add negative &&
+ git commit -m "one" -m "two" -m "three" &&
+ actual=$(git cat-file commit HEAD | sed -e "1,/^\$/d") &&
+ expected=$(echo one; echo; echo two; echo; echo three) &&
+ test "z$actual" = "z$expected"
+
+'
+
test_done
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-10 9:26 ` Junio C Hamano
@ 2007-11-10 9:41 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2007-11-10 9:41 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git, Carlos Rica
This is an updated patch to the test script...
t/t7501-commit.sh | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 69 insertions(+), 0 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index b151b51..4dc35bd 100644
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -163,4 +163,73 @@ test_expect_success 'partial commit that involves removal (3)' '
'
+author="The Real Author <someguy@his.email.org>"
+test_expect_success 'amend commit to fix author' '
+
+ oldtick=$GIT_AUTHOR_DATE &&
+ test_tick &&
+ git reset --hard &&
+ git cat-file -p HEAD |
+ sed -e "s/author.*/author $author $oldtick/" \
+ -e "s/^\(committer.*> \).*$/\1$GIT_COMMITTER_DATE/" > \
+ expected &&
+ git commit --amend --author="$author" &&
+ git cat-file -p HEAD > current &&
+ diff expected current
+
+'
+
+test_expect_success 'sign off (1)' '
+
+ echo 1 >positive &&
+ git add positive &&
+ git commit -s -m "thank you" &&
+ git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+ (
+ echo thank you
+ echo
+ git var GIT_COMMITTER_IDENT |
+ sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+ ) >expected &&
+ diff -u expected actual
+
+'
+
+test_expect_success 'sign off (2)' '
+
+ echo 2 >positive &&
+ git add positive &&
+ existing="Signed-off-by: Watch This <watchthis@example.com>" &&
+ git commit -s -m "thank you
+
+$existing" &&
+ git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+ (
+ echo thank you
+ echo
+ echo $existing
+ git var GIT_COMMITTER_IDENT |
+ sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+ ) >expected &&
+ diff -u expected actual
+
+'
+
+test_expect_success 'multiple -m' '
+
+ >negative &&
+ git add negative &&
+ git commit -m "one" -m "two" -m "three" &&
+ git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+ (
+ echo one
+ echo
+ echo two
+ echo
+ echo three
+ ) >expected &&
+ diff -u expected actual
+
+'
+
test_done
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-10 6:07 ` Junio C Hamano
2007-11-10 9:26 ` Junio C Hamano
@ 2007-11-10 12:25 ` Carlos Rica
2007-11-10 13:13 ` Pierre Habouzit
2007-11-12 13:09 ` Carlos Rica
2 siblings, 1 reply; 11+ messages in thread
From: Carlos Rica @ 2007-11-10 12:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pierre Habouzit
2007/11/10, Junio C Hamano <gitster@pobox.com>:
> Carlos Rica <jasampler@gmail.com> writes:
>
> > Also, this removes those tests ensuring that repeated
> > -m options don't allocate memory more than once, because now
> > this is done after parsing options, using the last one
> > when more are given. The same for -F.
>
> The reason for this change is...? Is this because it is
> cumbersome to detect and refuse multiple -m options using the
> parseopt API? If so, the API may be what needs to be fixed.
> Taking the last one and discarding earlier ones feels to me an
> arbitrary choice.
>
> While I freely admit that I do not particularly find the "One -m
> introduces one new line, concatenated to form the final
> paragraph" handling of multiple -m options done by git-commit
> nice nor useful, I suspect that it would make more sense to make
> git-tag and git-commit handle multiple -m option consistently,
> if you are going to change the existing semantics. Since some
> people really seem to like multiple -m handling of git-commit,
> the avenue of the least resistance for better consistency would
> be to accept and concatenate (with LF in between) multiple -m
> options.
>
> With multiple -F, I think erroring out would be the sensible
> thing to do, but some people might prefer concatenation. I do
> not care either way as long as commit and tag behave
> consistently.
A solution not needing memory allocation into the option parser
could be setting a callback running over the repeated option
arguments, passing them to the function one per each call.
Then, the user will be able to decide if he wants the arguments
concatenated or only need one of them and prefers erroring out.
Is this already possible with the current parser or the callback
mode only calls using the last option?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-10 12:25 ` Carlos Rica
@ 2007-11-10 13:13 ` Pierre Habouzit
0 siblings, 0 replies; 11+ messages in thread
From: Pierre Habouzit @ 2007-11-10 13:13 UTC (permalink / raw)
To: Carlos Rica; +Cc: Junio C Hamano, git
[-- Attachment #1: Type: text/plain, Size: 2320 bytes --]
On Sat, Nov 10, 2007 at 12:25:44PM +0000, Carlos Rica wrote:
> 2007/11/10, Junio C Hamano <gitster@pobox.com>:
> > Carlos Rica <jasampler@gmail.com> writes:
> A solution not needing memory allocation into the option parser
> could be setting a callback running over the repeated option
> arguments, passing them to the function one per each call.
> Then, the user will be able to decide if he wants the arguments
> concatenated or only need one of them and prefers erroring out.
>
> Is this already possible with the current parser or the callback
> mode only calls using the last option?
Everything is possible, you just have to code it. With a callback
you have in the struct option two places to store "things". The void*
value pointer and the intptr_t defval. _Usually_ the void* is the
pointer to the data that will be _written_ and the defval the data that
will be put into the void* under some circumstances (e.g. when your
option is negated).
For Your case I'd go with some kind of string list pointed into the
void * value, defval has no or little use. You don't really care about
allocating memory in the option parser, I mean, option parsing is done
once at the initialization phase. It's not evil. In pseudo-C here is how
I would write the callback:
int parse_opt_stringlist(const struct option *opt, const char *arg, int unset)
{
string_list **l = opt->value;
string_list_elem *e;
if (unset) { /* negationg option clears the list */
while (*l) {
string_list_elem_free(string_list_pop(l));
}
return 0;
}
e = string_list_elem_new();
e->data = arg;
string_list_push(l, e);
return 0;
}
And you're done, you can do what you want with that list from the caller.
There probably is such a structure in git, if not, it can probably be hacked
in a few lines.
Remember, callbacks give you _full_ control on what you can do in the option
parser, and if you're not happy with Turing complete expressivity, there isn't
anything I can do for you :P Note that if you do write such a generic
callback, it belongs to parse-options.[hc].
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-10 6:07 ` Junio C Hamano
2007-11-10 9:26 ` Junio C Hamano
2007-11-10 12:25 ` Carlos Rica
@ 2007-11-12 13:09 ` Carlos Rica
2007-11-12 14:52 ` Pierre Habouzit
2007-11-12 19:48 ` Kristian Høgsberg
2 siblings, 2 replies; 11+ messages in thread
From: Carlos Rica @ 2007-11-12 13:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kristian Høgsberg, Pierre Habouzit
2007/11/10, Junio C Hamano <gitster@pobox.com>:
> Carlos Rica <jasampler@gmail.com> writes:
>
> > Also, this removes those tests ensuring that repeated
> > -m options don't allocate memory more than once, because now
> > this is done after parsing options, using the last one
> > when more are given. The same for -F.
>
> The reason for this change is...? Is this because it is
> cumbersome to detect and refuse multiple -m options using the
> parseopt API? If so, the API may be what needs to be fixed.
> Taking the last one and discarding earlier ones feels to me an
> arbitrary choice.
You can do many things with repeated options.
Here in git-tag we considered two different ways to manage them:
Concatenating values for the option and/or refusing more than one.
I found that current option-parser can do both from the client
using callbacks, as Pierre shows me, so I think it is the right way to do it.
Pierre, by default, I think that the parser should print an error
when more than one option of the same type is given,
in order to report it to the command-line user,
but make this behaviour optional for the programmer.
Specifically, I thought in this last option:
enum parse_opt_option_flags {
PARSE_OPT_OPTARG = 1,
PARSE_OPT_NOARG = 2,
PARSE_OPT_ALLOWREP = 4
};
> While I freely admit that I do not particularly find the "One -m
> introduces one new line, concatenated to form the final
> paragraph" handling of multiple -m options done by git-commit
> nice nor useful, I suspect that it would make more sense to make
> git-tag and git-commit handle multiple -m option consistently,
> if you are going to change the existing semantics. Since some
> people really seem to like multiple -m handling of git-commit,
> the avenue of the least resistance for better consistency would
> be to accept and concatenate (with LF in between) multiple -m
> options.
>
> With multiple -F, I think erroring out would be the sensible
> thing to do, but some people might prefer concatenation. I do
> not care either way as long as commit and tag behave
> consistently.
Then, Kristian, what are you willing to do in such case?
It seems easier for me to concatenate of -m and -F options, even when
both types are given. I don't know why "people" want multiple -m options,
but I think that mixing -m and -F options could be interesting for them too.
If someone know if this have been discussed and decided already,
please give me the link.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-12 13:09 ` Carlos Rica
@ 2007-11-12 14:52 ` Pierre Habouzit
2007-11-12 19:48 ` Kristian Høgsberg
1 sibling, 0 replies; 11+ messages in thread
From: Pierre Habouzit @ 2007-11-12 14:52 UTC (permalink / raw)
To: Carlos Rica; +Cc: Junio C Hamano, git, Kristian Høgsberg
[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]
On Mon, Nov 12, 2007 at 01:09:37PM +0000, Carlos Rica wrote:
> 2007/11/10, Junio C Hamano <gitster@pobox.com>:
> > Carlos Rica <jasampler@gmail.com> writes:
> >
> > > Also, this removes those tests ensuring that repeated
> > > -m options don't allocate memory more than once, because now
> > > this is done after parsing options, using the last one
> > > when more are given. The same for -F.
> >
> > The reason for this change is...? Is this because it is
> > cumbersome to detect and refuse multiple -m options using the
> > parseopt API? If so, the API may be what needs to be fixed.
> > Taking the last one and discarding earlier ones feels to me an
> > arbitrary choice.
>
> You can do many things with repeated options.
> Here in git-tag we considered two different ways to manage them:
> Concatenating values for the option and/or refusing more than one.
> I found that current option-parser can do both from the client
> using callbacks, as Pierre shows me, so I think it is the right way to do it.
>
> Pierre, by default, I think that the parser should print an error
> when more than one option of the same type is given,
I beg to differ. It makes sense for OPTION_STRING options, but not for
other. Though you cannot always detect that.
Also note that:
(1) repeating options was already silent in many git commands, so it's
not really a regression ;
(2) for many commands it actually make sense to allow repeating (for
_BOOLEAN e.g.). And I'd argue that for OPTION_BIT it also makes
sense as well.
> in order to report it to the command-line user, but make this
> behaviour optional for the programmer. Specifically, I thought in
> this last option:
>
> enum parse_opt_option_flags {
> PARSE_OPT_OPTARG = 1,
> PARSE_OPT_NOARG = 2,
> PARSE_OPT_ALLOWREP = 4
> };
To do that you need to keep a list of the triggered commands to do
that, there is no way to achieve that reliably right now. As taking the
last one and discarding the other is the usual way for option parsers I
never saw this as a big issue.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Make builtin-tag.c use parse_options.
2007-11-12 13:09 ` Carlos Rica
2007-11-12 14:52 ` Pierre Habouzit
@ 2007-11-12 19:48 ` Kristian Høgsberg
1 sibling, 0 replies; 11+ messages in thread
From: Kristian Høgsberg @ 2007-11-12 19:48 UTC (permalink / raw)
To: Carlos Rica; +Cc: Junio C Hamano, git, Pierre Habouzit
On Mon, 2007-11-12 at 14:09 +0100, Carlos Rica wrote:
> 2007/11/10, Junio C Hamano <gitster@pobox.com>:
> > Carlos Rica <jasampler@gmail.com> writes:
...
> Then, Kristian, what are you willing to do in such case?
> It seems easier for me to concatenate of -m and -F options, even when
> both types are given. I don't know why "people" want multiple -m options,
> but I think that mixing -m and -F options could be interesting for them too.
> If someone know if this have been discussed and decided already,
> please give me the link.
I should be pretty easy to just append the contents of multiple fies,
even inter-mingled with -m options. We just do a callback like Johannes
just did for -m in builtin-commit.c for -F and append to the same
strbuf. strbuf_read() already appends, so the callback could look
something like:
static int opt_parse_F(const struct option *opt, const char *arg, int
unset)
{
struct strbuf *buf = opt->value;
if (!strcmp(arg, "-")) {
if (isatty(0))
fprintf(stderr, "(reading log message from
standard input)\n");
if (strbuf_read(&sb, 0, 0) < 0)
die("could not read log from standard input");
} else {
if (strbuf_read_file(&sb, logfile, 0) < 0)
die("could not read log file '%s': %s",
logfile, strerror(errno));
}
}
Shouldn't be too hard :)
Kristian
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2007-11-12 19:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-09 13:42 [PATCH] Make builtin-tag.c use parse_options Carlos Rica
2007-11-09 13:57 ` Jakub Narebski
2007-11-09 14:31 ` Johannes Schindelin
2007-11-10 6:07 ` Junio C Hamano
2007-11-10 9:26 ` Junio C Hamano
2007-11-10 9:41 ` Junio C Hamano
2007-11-10 12:25 ` Carlos Rica
2007-11-10 13:13 ` Pierre Habouzit
2007-11-12 13:09 ` Carlos Rica
2007-11-12 14:52 ` Pierre Habouzit
2007-11-12 19:48 ` Kristian Høgsberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).