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
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
