Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given
Quoting Junio C Hamano gits...@pobox.com: @@ -443,10 +443,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix) if (pattern) argv_array_pushf(args, --refs=refs/tags/%s, pattern); } - while (*argv) { - argv_array_push(args, *argv); - argv++; - } + if (argc) What would this code do to 'describe --all --contains'? was my knee-jerk reaction, but the options are all parsed by this code and nothing is delegated to name-rev, so 'if (!argc)' here is truly the lack of any revisions to be described, which is good. Exactly. parse-opts removes all --options from argv as it processes them, barfs at --unknown-options, so all what remains must be treated as a commit-ish. And if nothing is left, well, then there was none given. + while (*argv) { + argv_array_push(args, *argv); + argv++; + } + else + argv_array_push(args, HEAD); By the way, I usually prefer a fatter 'else' clause when everything else is equal, i.e. if (!argc) argv_array_push(args, HEAD); /* default to HEAD */ else { while (*argv) { ... } } because it is easy to miss tiny else-clause while reading code, but it is harder to miss tiny then-clause. In this case, however, the while loop can be replaced with argv_array_pushv() these days, so perhaps if (!argc) argv_array_push(args, HEAD); /* default to HEAD ... */ else argv_array_pushv(args, argv); /* or relay what we got */ or something? Indeed, I didn't notice argv_array_pushv() being added, log tells me it happened quite recently. I suppose with both branches becoming a one-liner the order of them can remain what it was in the patch, this sparing the negation from 'if (!argc)'. v2 comes in a minute. Gábor -- 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] describe --contains: default to HEAD when no commit-ish is given
SZEDER Gábor sze...@ira.uka.de writes: By the way, I usually prefer a fatter 'else' clause when everything else is equal, i.e. if (!argc) argv_array_push(args, HEAD); /* default to HEAD */ else { while (*argv) { ... } } because it is easy to miss tiny else-clause while reading code, but it is harder to miss tiny then-clause. In this case, however, the while loop can be replaced with argv_array_pushv() these days, so perhaps if (!argc) argv_array_push(args, HEAD); /* default to HEAD ... */ else argv_array_pushv(args, argv); /* or relay what we got */ or something? Indeed, I didn't notice argv_array_pushv() being added, log tells me it happened quite recently. I suppose with both branches becoming a one-liner the order of them can remain what it was in the patch, this sparing the negation from 'if (!argc)'. Another reason to favor the way the code is illustrated for educational purposes above is it is easier to see exceptional case first, i.e. if we have nothing then we do this special thing, but otherwise we do the normal thing. But that is a much weaker preference than the preference to fatter else; I could go either way. v2 comes in a minute. Thanks. -- 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] describe --contains: default to HEAD when no commit-ish is given
'git describe --contains' doesn't default to HEAD when no commit is given, and it doesn't produce any output, not even an error: ~/src/git ((v2.5.0))$ ./git describe --contains ~/src/git ((v2.5.0))$ ./git describe --contains HEAD v2.5.0^0 Unlike other 'git describe' options, the '--contains' code path is implemented by calling 'name-rev' with a bunch of options plus all the commit-ishes that were passed to 'git describe'. If no commit-ish was present, then 'name-rev' got invoked with none, which then leads to the behavior illustrated above. Porcelain commands usually default to HEAD when no commit-ish is given, and 'git describe' already does so in all other cases, so it should do so with '--contains' as well. Pass HEAD to 'name-rev' when no commit-ish is given on the command line to make '--contains' behave consistently with other 'git describe' options. 'git describe's short help already indicates that the commit-ish is optional, but the synopsis in the man page doesn't, so update it accordingly as well. Signed-off-by: SZEDER Gábor sze...@ira.uka.de --- Documentation/git-describe.txt | 4 ++-- builtin/describe.c | 11 +++ t/t6120-describe.sh| 8 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt index e045fc73f8..c8f28c8c86 100644 --- a/Documentation/git-describe.txt +++ b/Documentation/git-describe.txt @@ -9,7 +9,7 @@ git-describe - Describe a commit using the most recent tag reachable from it SYNOPSIS [verse] -'git describe' [--all] [--tags] [--contains] [--abbrev=n] commit-ish... +'git describe' [--all] [--tags] [--contains] [--abbrev=n] [commit-ish...] 'git describe' [--all] [--tags] [--contains] [--abbrev=n] --dirty[=mark] DESCRIPTION @@ -27,7 +27,7 @@ see the -a and -s options to linkgit:git-tag[1]. OPTIONS --- commit-ish...:: - Commit-ish object names to describe. + Commit-ish object names to describe. Defaults to HEAD if omitted. --dirty[=mark]:: Describe the working tree. diff --git a/builtin/describe.c b/builtin/describe.c index ce36032b1f..0e31ac5cb9 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -443,10 +443,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix) if (pattern) argv_array_pushf(args, --refs=refs/tags/%s, pattern); } - while (*argv) { - argv_array_push(args, *argv); - argv++; - } + if (argc) + while (*argv) { + argv_array_push(args, *argv); + argv++; + } + else + argv_array_push(args, HEAD); return cmd_name_rev(args.argc, args.argv, prefix); } diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 57d50156bb..bf52a0a1cc 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -115,6 +115,14 @@ check_describe e-3-* --first-parent --tags check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD +test_expect_success 'describe --contains defaults to HEAD without commit-ish' ' + echo A^0 expect + git checkout A + test_when_finished git checkout - + git describe --contains actual + test_cmp expect actual +' + : err.expect check_describe A --all A^0 test_expect_success 'no warning was displayed for A' ' -- 2.5.0.416.g84b07b4 -- 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] describe --contains: default to HEAD when no commit-ish is given
SZEDER Gábor sze...@ira.uka.de writes: 'git describe --contains' doesn't default to HEAD when no commit is given, and it doesn't produce any output, not even an error: ~/src/git ((v2.5.0))$ ./git describe --contains ~/src/git ((v2.5.0))$ ./git describe --contains HEAD v2.5.0^0 Good spotting. I think defaulting to HEAD is a good move. diff --git a/builtin/describe.c b/builtin/describe.c index ce36032b1f..0e31ac5cb9 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -443,10 +443,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix) if (pattern) argv_array_pushf(args, --refs=refs/tags/%s, pattern); } - while (*argv) { - argv_array_push(args, *argv); - argv++; - } + if (argc) What would this code do to 'describe --all --contains'? was my knee-jerk reaction, but the options are all parsed by this code and nothing is delegated to name-rev, so 'if (!argc)' here is truly the lack of any revisions to be described, which is good. + while (*argv) { + argv_array_push(args, *argv); + argv++; + } + else + argv_array_push(args, HEAD); By the way, I usually prefer a fatter 'else' clause when everything else is equal, i.e. if (!argc) argv_array_push(args, HEAD); /* default to HEAD */ else { while (*argv) { ... } } because it is easy to miss tiny else-clause while reading code, but it is harder to miss tiny then-clause. In this case, however, the while loop can be replaced with argv_array_pushv() these days, so perhaps if (!argc) argv_array_push(args, HEAD); /* default to HEAD ... */ else argv_array_pushv(args, argv); /* or relay what we got */ or something? return cmd_name_rev(args.argc, args.argv, prefix); } diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 57d50156bb..bf52a0a1cc 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -115,6 +115,14 @@ check_describe e-3-* --first-parent --tags check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD +test_expect_success 'describe --contains defaults to HEAD without commit-ish' ' + echo A^0 expect + git checkout A + test_when_finished git checkout - + git describe --contains actual + test_cmp expect actual +' : err.expect check_describe A --all A^0 test_expect_success 'no warning was displayed for A' ' -- 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