Re: [PATCH 3/6] var: accept multiple variables on the command line
On Wed, Nov 14, 2012 at 09:01:48AM -0800, Jonathan Nieder wrote: > > DESCRIPTION > > --- > > -Prints a git logical variable. > > +Prints one or more git logical variables, separated by newlines. > > + > > +Note that some variables may contain newlines themselves > > Maybe a -z option to NUL-terminate values would be useful some day. Yeah, I thought about that but stopped short. The intended caller in my series is Git.pm, whose command() splits on newlines. Although it is perl...I suspect doing: local $/ = "\0"; my @entries = command(...); would work. For ident variables, we know they don't contain a newline, though. > > --- a/builtin/var.c > > +++ b/builtin/var.c > > @@ -73,8 +73,7 @@ static int show_config(const char *var, const char > > *value, void *cb) > > > > int cmd_var(int argc, const char **argv, const char *prefix) > > { > > - const char *val = NULL; > > - if (argc != 2) > > + if (argc < 2) > > usage(var_usage); > > > > if (strcmp(argv[1], "-l") == 0) { > > What should happen if I pass "-l" followed by other arguments? Good catch. Probably we should just call usage() once we see "-l" and (argc > 2), which matches the previous behavior. I don't see much point in listing specific variables after having listed them all. I was also tempted to convert to parse_options, but I don't think that really buys us anything (we could detect the option in "git var foo -l bar", but since we are not going to do anything useful in such a case, there is not much point). > > + test_tick && > > + echo "A U Thor 1112911993 -0700" >expect && > > Do we need to hardcode the timestamp? Something like > > test_cmp_filtered () { > expect=$1 actual=$2 && > sed -e 's/[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]/TIMESTAMP" \ > <"$actual" >"$actual.filtered" && > test_cmp "$expect" "$actual.filtered" > } No, we don't have to. I was just hoping to keep the tests simple by not doing any parsing trickery. The test_tick keeps it stable, but as you note, it is not robust to reordering. I think it would be sufficient to just put $GIT_COMMITTER_DATE into the expected output. I'll fix both in a re-roll. Thanks. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] var: accept multiple variables on the command line
Jeff King wrote: > This patch lets callers specify multiple variables, and > prints one per line. Yay! [...] > --- a/Documentation/git-var.txt > +++ b/Documentation/git-var.txt > @@ -9,11 +9,16 @@ git-var - Show a git logical variable > SYNOPSIS > > [verse] > -'git var' ( -l | ) > +'git var' ( -l | ... ) > > DESCRIPTION > --- > -Prints a git logical variable. > +Prints one or more git logical variables, separated by newlines. > + > +Note that some variables may contain newlines themselves Maybe a -z option to NUL-terminate values would be useful some day. > --- a/builtin/var.c > +++ b/builtin/var.c > @@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, > void *cb) > > int cmd_var(int argc, const char **argv, const char *prefix) > { > - const char *val = NULL; > - if (argc != 2) > + if (argc < 2) > usage(var_usage); > > if (strcmp(argv[1], "-l") == 0) { What should happen if I pass "-l" followed by other arguments? [...] > --- /dev/null > +++ b/t/t0007-git-var.sh > @@ -0,0 +1,29 @@ > +#!/bin/sh > + > +test_description='basic sanity checks for git var' > +. ./test-lib.sh > + > +test_expect_success 'get GIT_AUTHOR_IDENT' ' > + test_tick && > + echo "A U Thor 1112911993 -0700" >expect && Do we need to hardcode the timestamp? Something like test_cmp_filtered () { expect=$1 actual=$2 && sed -e 's/[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]/TIMESTAMP" \ <"$actual" >"$actual.filtered" && test_cmp "$expect" "$actual.filtered" } ... echo "A U Thor $timestamp" >expect && git var GIT_AUTHOR_IDENT >actual && test_cmp_filtered expect actual should make reordering tests a lot easier, though it has the downside of not being able to catch a weird bug that would make the timestamp out of sync with reality. Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] var: accept multiple variables on the command line
Git-var currently only accepts a single value to print. This is inefficient if the caller is interested in finding multiple values, as they must invoke git-var multiple times. This patch lets callers specify multiple variables, and prints one per line. Signed-off-by: Jeff King --- This will later let us get the "explicit" flag for free. Documentation/git-var.txt | 9 +++-- builtin/var.c | 13 +++-- t/t0007-git-var.sh| 29 + 3 files changed, 43 insertions(+), 8 deletions(-) create mode 100755 t/t0007-git-var.sh diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt index 67edf58..53abba5 100644 --- a/Documentation/git-var.txt +++ b/Documentation/git-var.txt @@ -9,11 +9,16 @@ git-var - Show a git logical variable SYNOPSIS [verse] -'git var' ( -l | ) +'git var' ( -l | ... ) DESCRIPTION --- -Prints a git logical variable. +Prints one or more git logical variables, separated by newlines. + +Note that some variables may contain newlines themselves (e.g., +`GIT_EDITOR`), and it is therefore possible to receive ambiguous output +when requesting multiple variables. This can be mitigated by putting any +such variables at the end of the list. OPTIONS --- diff --git a/builtin/var.c b/builtin/var.c index aedbb53..49b48f5 100644 --- a/builtin/var.c +++ b/builtin/var.c @@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb) int cmd_var(int argc, const char **argv, const char *prefix) { - const char *val = NULL; - if (argc != 2) + if (argc < 2) usage(var_usage); if (strcmp(argv[1], "-l") == 0) { @@ -83,11 +82,13 @@ int cmd_var(int argc, const char **argv, const char *prefix) return 0; } git_config(git_default_config, NULL); - val = read_var(argv[1]); - if (!val) - usage(var_usage); - printf("%s\n", val); + while (*++argv) { + const char *val = read_var(*argv); + if (!val) + usage(var_usage); + printf("%s\n", val); + } return 0; } diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh new file mode 100755 index 000..45a5f66 --- /dev/null +++ b/t/t0007-git-var.sh @@ -0,0 +1,29 @@ +#!/bin/sh + +test_description='basic sanity checks for git var' +. ./test-lib.sh + +test_expect_success 'get GIT_AUTHOR_IDENT' ' + test_tick && + echo "A U Thor 1112911993 -0700" >expect && + git var GIT_AUTHOR_IDENT >actual && + test_cmp expect actual +' + +test_expect_success 'get GIT_COMMITTER_IDENT' ' + test_tick && + echo "C O Mitter 1112912053 -0700" >expect && + git var GIT_COMMITTER_IDENT >actual && + test_cmp expect actual +' + +test_expect_success 'git var can show multiple values' ' + cat >expect <<-\EOF && + A U Thor 1112912053 -0700 + C O Mitter 1112912053 -0700 + EOF + git var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual && + test_cmp expect actual +' + +test_done -- 1.8.0.207.gdf2154c -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html