* [PATCH 0/2] Add new "describe" atom
@ 2023-07-05 17:57 Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu
` (2 more replies)
0 siblings, 3 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-05 17:57 UTC (permalink / raw)
To: git; +Cc: Rene Scharfe, Kousik Sanagavarapu
Hi,
This patch series focuses on duplicating the implementation of
%(describe) and its friends from pretty to ref-filter, with the end goal
of making ref-filter do everything that pretty is doing.
PATCH 1/2 - This patch is the duplication of the placeholder from pretty
ref-filter.
PATCH 2/2 - This is an interesting case, where the tests written for the
above duplication are successful but another test below, in
t6300, "Verify sorts with raw:size" fails on linux-sha256 (CI).
Kousik Sanagavarapu (2):
ref-filter: add new "describe" atom
t6300: run describe atom tests on a different repo
Documentation/git-for-each-ref.txt | 19 ++++
ref-filter.c | 144 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 98 ++++++++++++++++++++
3 files changed, 261 insertions(+)
--
2.41.0.237.g2d10a112d6.dirty
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/2] ref-filter: add new "describe" atom
2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu
@ 2023-07-05 17:57 ` Kousik Sanagavarapu
2023-07-06 16:58 ` Junio C Hamano
2023-07-05 17:57 ` [PATCH 2/2] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
2 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-05 17:57 UTC (permalink / raw)
To: git; +Cc: Rene Scharfe, Kousik Sanagavarapu, Christian Couder, Hariom Verma
Duplicate the logic of %(describe) and friends from pretty to
ref-filter. In the future, this change helps in unifying both the
formats as ref-filter will be able to do everything that pretty is doing
and we can have a single interface.
The new atom "describe" and its friends are equivalent to the existing
pretty formats with the same name.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
Documentation/git-for-each-ref.txt | 19 ++++
ref-filter.c | 144 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 85 +++++++++++++++++
3 files changed, 248 insertions(+)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 1e215d4e73..4ac5c3dac4 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -231,6 +231,25 @@ ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the output
ref to the `<committish>` specified in the format.
+describe[:options]:: human-readable name, like
+ link-git:git-describe[1]; empty string for
+ undescribable commits. The `describe` string may be
+ followed by a colon and zero or more comma-separated
+ options. Descriptions can be inconsistent when tags
+ are added or removed at the same time.
++
+** tags=<bool-value>: Instead of only considering annotated tags, consider
+ lightweight tags as well.
+** abbrev=<number>: Instead of using the default number of hexadecimal digits
+ (which will vary according to the number of objects in the
+ repository with a default of 7) of the abbreviated
+ object name, use <number> digits, or as many digits as
+ needed to form a unique object name.
+** match=<pattern>: Only consider tags matching the given `glob(7)` pattern,
+ excluding the "refs/tags/" prefix.
+** exclude=<pattern>: Do not consider tags matching the given `glob(7)`
+ pattern,excluding the "refs/tags/" prefix.
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index e0d03a9f8e..6ec647c81f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -5,6 +5,7 @@
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
+#include "run-command.h"
#include "refs.h"
#include "wildmatch.h"
#include "object-name.h"
@@ -146,6 +147,7 @@ enum atom_type {
ATOM_TAGGERDATE,
ATOM_CREATOR,
ATOM_CREATORDATE,
+ ATOM_DESCRIBE,
ATOM_SUBJECT,
ATOM_BODY,
ATOM_TRAILERS,
@@ -215,6 +217,13 @@ static struct used_atom {
struct email_option {
enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
} email_option;
+ struct {
+ enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,
+ D_MATCH } option;
+ unsigned int tagbool;
+ unsigned int length;
+ char *pattern;
+ } describe;
struct refname_atom refname;
char *head;
} u;
@@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
return 0;
}
+static int parse_describe_option(const char *arg)
+{
+ if (!arg)
+ return D_BARE;
+ else if (starts_with(arg, "tags"))
+ return D_TAGS;
+ else if (starts_with(arg, "abbrev"))
+ return D_ABBREV;
+ else if(starts_with(arg, "exclude"))
+ return D_EXCLUDE;
+ else if (starts_with(arg, "match"))
+ return D_MATCH;
+ return -1;
+}
+
+static int describe_atom_parser(struct ref_format *format UNUSED,
+ struct used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
+ int opt = parse_describe_option(arg);
+
+ switch (opt) {
+ case D_BARE:
+ break;
+ case D_TAGS:
+ /*
+ * It is also possible to just use describe:tags, which
+ * is just treated as describe:tags=1
+ */
+ if (skip_prefix(arg, "tags=", &arg)) {
+ if (strtoul_ui(arg, 10, &atom->u.describe.tagbool))
+ return strbuf_addf_ret(err, -1, _("boolean value "
+ "expected describe:tags=%s"), arg);
+
+ } else {
+ atom->u.describe.tagbool = 1;
+ }
+ break;
+ case D_ABBREV:
+ skip_prefix(arg, "abbrev=", &arg);
+ if (strtoul_ui(arg, 10, &atom->u.describe.length))
+ return strbuf_addf_ret(err, -1, _("positive value "
+ "expected describe:abbrev=%s"), arg);
+ break;
+ case D_EXCLUDE:
+ skip_prefix(arg, "exclude=", &arg);
+ atom->u.describe.pattern = xstrdup(arg);
+ break;
+ case D_MATCH:
+ skip_prefix(arg, "match=", &arg);
+ atom->u.describe.pattern = xstrdup(arg);
+ break;
+ default:
+ return err_bad_arg(err, "describe", arg);
+ break;
+ }
+ atom->u.describe.option = opt;
+ return 0;
+}
+
static int raw_atom_parser(struct ref_format *format UNUSED,
struct used_atom *atom,
const char *arg, struct strbuf *err)
@@ -664,6 +733,7 @@ static struct {
[ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
[ATOM_CREATOR] = { "creator", SOURCE_OBJ },
[ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
+ [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
[ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
@@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
}
}
+static void grab_describe_values(struct atom_value *val, int deref,
+ struct object *obj)
+{
+ struct commit *commit = (struct commit *)obj;
+ int i;
+
+ for (i = 0; i < used_atom_cnt; i++) {
+ struct used_atom *atom = &used_atom[i];
+ const char *name = atom->name;
+ struct atom_value *v = &val[i];
+ int opt;
+
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+
+ if (!!deref != (*name == '*'))
+ continue;
+ if (deref)
+ name++;
+
+ if (!skip_prefix(name, "describe", &name) ||
+ (*name && *name != ':'))
+ continue;
+ if (!*name)
+ name = NULL;
+ else
+ name++;
+
+ opt = parse_describe_option(name);
+ if (opt < 0)
+ continue;
+
+ cmd.git_cmd = 1;
+ strvec_push(&cmd.args, "describe");
+
+ switch(opt) {
+ case D_BARE:
+ break;
+ case D_TAGS:
+ if (atom->u.describe.tagbool)
+ strvec_push(&cmd.args, "--tags");
+ else
+ strvec_push(&cmd.args, "--no-tags");
+ break;
+ case D_ABBREV:
+ strvec_pushf(&cmd.args, "--abbrev=%d",
+ atom->u.describe.length);
+ break;
+ case D_EXCLUDE:
+ strvec_pushf(&cmd.args, "--exclude=%s",
+ atom->u.describe.pattern);
+ break;
+ case D_MATCH:
+ strvec_pushf(&cmd.args, "--match=%s",
+ atom->u.describe.pattern);
+ break;
+ }
+
+ strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+ if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+ error(_("failed to run 'describe'"));
+ v->s = xstrdup("");
+ continue;
+ }
+ strbuf_rtrim(&out);
+ v->s = strbuf_detach(&out, NULL);
+
+ strbuf_release(&err);
+ }
+}
+
/* See grab_values */
static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)
{
@@ -1592,12 +1734,14 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_tag_values(val, deref, obj);
grab_sub_body_contents(val, deref, data);
grab_person("tagger", val, deref, buf);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_COMMIT:
grab_commit_values(val, deref, obj);
grab_sub_body_contents(val, deref, data);
grab_person("author", val, deref, buf);
grab_person("committer", val, deref, buf);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_TREE:
/* grab_tree_values(val, deref, obj, buf, sz); */
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 5c00607608..98ea37d336 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -561,6 +561,91 @@ test_expect_success 'color.ui=always does not override tty check' '
test_cmp expected.bare actual
'
+test_expect_success 'describe atom vs git describe' '
+ test_when_finished "rm -rf describe-repo" &&
+
+ git init describe-repo &&
+ (
+ cd describe-repo &&
+
+ test_commit --no-tag one &&
+ git tag tagone &&
+
+ test_commit --no-tag two &&
+ git tag -a -m "tag two" tagtwo &&
+
+ git for-each-ref refs/tags/ --format="%(objectname)" >obj &&
+ while read hash
+ do
+ if desc=$(git describe $hash)
+ then
+ : >expect-contains-good
+ else
+ : >expect-contains-bad
+ fi &&
+ echo "$hash $desc" || return 1
+ done <obj >expect &&
+ test_path_exists expect-contains-good &&
+ test_path_exists expect-contains-bad &&
+
+ git for-each-ref --format="%(objectname) %(describe)" \
+ refs/tags/ >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+ )
+'
+
+test_expect_success 'describe:tags vs describe --tags' '
+ test_when_finished "git tag -d tagname" &&
+ git tag tagname &&
+ git describe --tags >expect &&
+ git for-each-ref --format="%(describe:tags)" refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '
+ test_when_finished "git tag -d tagname" &&
+
+ # Case 1: We have commits between HEAD and the most
+ # recent tag reachable from it
+ test_commit --no-tag file &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual &&
+
+ # Make sure the hash used is atleast 14 digits long
+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+ test 15 -le $(wc -c <hexpart) &&
+
+ # Case 2: We have a tag at HEAD, describe directly gives
+ # the name of the tag
+ git tag -a -m tagged tagname &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual &&
+ test tagname = $(cat actual)
+'
+
+test_expect_success 'describe:match=... vs describe --match ...' '
+ test_when_finished "git tag -d tag-match" &&
+ git tag -a -m "tag match" tag-match &&
+ git describe --match "*-match" >expect &&
+ git for-each-ref --format="%(describe:match="*-match")" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'describe:exclude:... vs describe --exclude ...' '
+ test_when_finished "git tag -d tag-exclude" &&
+ git tag -a -m "tag exclude" tag-exclude &&
+ git describe --exclude "*-exclude" >expect &&
+ git for-each-ref --format="%(describe:exclude="*-exclude")" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
cat >expected <<\EOF
heads/main
tags/main
--
2.41.0.237.g2d10a112d6.dirty
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 2/2] t6300: run describe atom tests on a different repo
2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu
@ 2023-07-05 17:57 ` Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
2 siblings, 0 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-05 17:57 UTC (permalink / raw)
To: git; +Cc: Rene Scharfe, Kousik Sanagavarapu, Christian Couder, Hariom Verma
The tests for "describe" atom and its friends currently run on the main
repo of t6300, expect for the test for "bare describe", which is run on
"describe-repo".
Things can get messy with the other tests when such changes to a repo
are done (for example, a new commit or a tag is introduced), especially
in t6300 where the tests depend on commits and tags.
An example for this can be seen in [1], where writing the tests the
current way fails the test "Verify sorts with raw:size" on linux-sha256.
This, at first glance, seems totally unrelated.
Digging in a bit deeper, it is apparent that this behavior is because of
the changes in the repo introduced when writing the "describe" tests,
which changes the raw:size of an object. Such a change in raw-size would
have been, however, small if we were dealing with SHA1, but since we are
dealing with SHA256, the change in raw:size is so significant that it
fails the above mentioned test.
So, run all the "describe" atom tests on "describe-repo", which doesn't
interfere with the main repo on which the tests in t6300 are run.
[1]: https://github.com/five-sh/git/actions/runs/5446892074/jobs/9908256427
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
t/t6300-for-each-ref.sh | 101 +++++++++++++++++++++++-----------------
1 file changed, 57 insertions(+), 44 deletions(-)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 98ea37d336..181b04e699 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -561,9 +561,7 @@ test_expect_success 'color.ui=always does not override tty check' '
test_cmp expected.bare actual
'
-test_expect_success 'describe atom vs git describe' '
- test_when_finished "rm -rf describe-repo" &&
-
+test_expect_success 'setup for describe atom tests' '
git init describe-repo &&
(
cd describe-repo &&
@@ -572,9 +570,16 @@ test_expect_success 'describe atom vs git describe' '
git tag tagone &&
test_commit --no-tag two &&
- git tag -a -m "tag two" tagtwo &&
+ git tag -a -m "tag two" tagtwo
+ )
+'
+
+test_expect_success 'describe atom vs git describe' '
+ (
+ cd describe-repo &&
- git for-each-ref refs/tags/ --format="%(objectname)" >obj &&
+ git for-each-ref --format="%(objectname)" \
+ refs/tags/ >obj &&
while read hash
do
if desc=$(git describe $hash)
@@ -596,54 +601,62 @@ test_expect_success 'describe atom vs git describe' '
'
test_expect_success 'describe:tags vs describe --tags' '
- test_when_finished "git tag -d tagname" &&
- git tag tagname &&
- git describe --tags >expect &&
- git for-each-ref --format="%(describe:tags)" refs/heads/ >actual &&
- test_cmp expect actual
+ (
+ cd describe-repo &&
+ git describe --tags >expect &&
+ git for-each-ref --format="%(describe:tags)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+ )
'
test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '
- test_when_finished "git tag -d tagname" &&
-
- # Case 1: We have commits between HEAD and the most
- # recent tag reachable from it
- test_commit --no-tag file &&
- git describe --abbrev=14 >expect &&
- git for-each-ref --format="%(describe:abbrev=14)" \
- refs/heads/ >actual &&
- test_cmp expect actual &&
-
- # Make sure the hash used is atleast 14 digits long
- sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
- test 15 -le $(wc -c <hexpart) &&
-
- # Case 2: We have a tag at HEAD, describe directly gives
- # the name of the tag
- git tag -a -m tagged tagname &&
- git describe --abbrev=14 >expect &&
- git for-each-ref --format="%(describe:abbrev=14)" \
- refs/heads/ >actual &&
- test_cmp expect actual &&
- test tagname = $(cat actual)
+ (
+ cd describe-repo &&
+
+ # Case 1: We have commits between HEAD and the most
+ # recent tag reachable from it
+ test_commit --no-tag file &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual &&
+
+ # Make sure the hash used is atleast 14 digits long
+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+ test 15 -le $(wc -c <hexpart) &&
+
+ # Case 2: We have a tag at HEAD, describe directly gives
+ # the name of the tag
+ git tag -a -m tagged tagname &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual &&
+ test tagname = $(cat actual)
+ )
'
test_expect_success 'describe:match=... vs describe --match ...' '
- test_when_finished "git tag -d tag-match" &&
- git tag -a -m "tag match" tag-match &&
- git describe --match "*-match" >expect &&
- git for-each-ref --format="%(describe:match="*-match")" \
- refs/heads/ >actual &&
- test_cmp expect actual
+ (
+ cd describe-repo &&
+ git tag -a -m "tag match" tag-match &&
+ git describe --match "*-match" >expect &&
+ git for-each-ref --format="%(describe:match="*-match")" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+ )
'
test_expect_success 'describe:exclude:... vs describe --exclude ...' '
- test_when_finished "git tag -d tag-exclude" &&
- git tag -a -m "tag exclude" tag-exclude &&
- git describe --exclude "*-exclude" >expect &&
- git for-each-ref --format="%(describe:exclude="*-exclude")" \
- refs/heads/ >actual &&
- test_cmp expect actual
+ (
+ cd describe-repo &&
+ git tag -a -m "tag exclude" tag-exclude &&
+ git describe --exclude "*-exclude" >expect &&
+ git for-each-ref --format="%(describe:exclude="*-exclude")" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+ )
'
cat >expected <<\EOF
--
2.41.0.237.g2d10a112d6.dirty
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] ref-filter: add new "describe" atom
2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu
@ 2023-07-06 16:58 ` Junio C Hamano
2023-07-09 6:16 ` Kousik Sanagavarapu
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2023-07-06 16:58 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Rene Scharfe, Christian Couder, Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> +describe[:options]:: human-readable name, like
> + link-git:git-describe[1]; empty string for
> + undescribable commits. The `describe` string may be
> + followed by a colon and zero or more comma-separated
> + options. Descriptions can be inconsistent when tags
> + are added or removed at the same time.
> ++
> +** tags=<bool-value>: Instead of only considering annotated tags, consider
> + lightweight tags as well.
> +** abbrev=<number>: Instead of using the default number of hexadecimal digits
> + (which will vary according to the number of objects in the
> + repository with a default of 7) of the abbreviated
> + object name, use <number> digits, or as many digits as
> + needed to form a unique object name.
> +** match=<pattern>: Only consider tags matching the given `glob(7)` pattern,
> + excluding the "refs/tags/" prefix.
> +** exclude=<pattern>: Do not consider tags matching the given `glob(7)`
> + pattern,excluding the "refs/tags/" prefix.
You are missing a SP after the comma in "pattern,excluding" above.
The above description is slightly different from what "git describe
--help" has. If they are described differently on purpose (e.g. you
may have made "%(describe:abbrev=0)" not to show only the closest
tag, unlike "git describe --abbrev=0"), the differences should be
spelled out more explicitly. If the behaviours of the option here
and the corresponding one there are meant to be the same, then
either using exactly the same text, or a much abbreviated
description with a note referring to the option description of "git
describe", would help the readers better. E.g.
abbrev=<number>;; use at least <number> hexadecimal digits; see
the corresponding option in linkgit:git-describe[1] for details.
which would make it clear that no behavioral differences are meant.
This new section becomes a part of an existing "labeled list"
(asciidoctor calls the construct "description list"). Starting the
new heading this patch adds with 'describe[:options]::' makes sense.
It is in line with the existing text.
I however think that the list of options is better done as a nested
description list. Documentation/config/color.txt has an example you
can imitate. See how slots of color.grep.<slot> are described
there.
https://docs.asciidoctor.org/asciidoc/latest/lists/description/
https://asciidoc-py.github.io/userguide.html#_labeled_lists
> diff --git a/ref-filter.c b/ref-filter.c
> index e0d03a9f8e..6ec647c81f 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -5,6 +5,7 @@
> #include "gpg-interface.h"
> #include "hex.h"
> #include "parse-options.h"
> +#include "run-command.h"
> #include "refs.h"
> #include "wildmatch.h"
> #include "object-name.h"
> @@ -146,6 +147,7 @@ enum atom_type {
> ATOM_TAGGERDATE,
> ATOM_CREATOR,
> ATOM_CREATORDATE,
> + ATOM_DESCRIBE,
> ATOM_SUBJECT,
> ATOM_BODY,
> ATOM_TRAILERS,
> @@ -215,6 +217,13 @@ static struct used_atom {
> struct email_option {
> enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
> } email_option;
> + struct {
> + enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,
> + D_MATCH } option;
> + unsigned int tagbool;
> + unsigned int length;
The name "tagbool" sounds strange, as we are not saying
"lengthint".
> + char *pattern;
> + } describe;
I am a bit confused by this structure, actually, as I cannot quite
guess from the data structure alone how you intend to use it. Does
this give a good representation for the piece of data you are trying
to capture?
For example, %(describe:tags=no,abbrev=4) would become a single atom
with 0 in .tagbool and 4 in .length, but what does the .option
member get?
> @@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
> return 0;
> }
>
> +static int parse_describe_option(const char *arg)
> +{
> + if (!arg)
> + return D_BARE;
> + else if (starts_with(arg, "tags"))
> + return D_TAGS;
> + else if (starts_with(arg, "abbrev"))
> + return D_ABBREV;
> + else if(starts_with(arg, "exclude"))
> + return D_EXCLUDE;
> + else if (starts_with(arg, "match"))
> + return D_MATCH;
> + return -1;
> +}
> +
> +static int describe_atom_parser(struct ref_format *format UNUSED,
> + struct used_atom *atom,
> + const char *arg, struct strbuf *err)
> +{
> + int opt = parse_describe_option(arg);
> +
> + switch (opt) {
> + case D_BARE:
> + break;
> + case D_TAGS:
> + /*
> + * It is also possible to just use describe:tags, which
> + * is just treated as describe:tags=1
> + */
> + if (skip_prefix(arg, "tags=", &arg)) {
> + if (strtoul_ui(arg, 10, &atom->u.describe.tagbool))
This is not how you accept a Boolean.
"1", "0", "yes", "no", "true", "false", "on", "off" are all valid
values and you use git_parse_maybe_bool() to parse them.
> + return strbuf_addf_ret(err, -1, _("boolean value "
> + "expected describe:tags=%s"), arg);
> +
> + } else {
> + atom->u.describe.tagbool = 1;
> + }
> + break;
> + case D_ABBREV:
> + skip_prefix(arg, "abbrev=", &arg);
> + if (strtoul_ui(arg, 10, &atom->u.describe.length))
> + return strbuf_addf_ret(err, -1, _("positive value "
> + "expected describe:abbrev=%s"), arg);
> + break;
> + case D_EXCLUDE:
> + skip_prefix(arg, "exclude=", &arg);
> + atom->u.describe.pattern = xstrdup(arg);
> + break;
> + case D_MATCH:
> + skip_prefix(arg, "match=", &arg);
> + atom->u.describe.pattern = xstrdup(arg);
> + break;
> + default:
> + return err_bad_arg(err, "describe", arg);
> + break;
> + }
> + atom->u.describe.option = opt;
> + return 0;
> +}
Even though the documentation patch we saw earlier said "may be
followed by a colon and zero or more comma-separated options", this
seems to expect only and exactly one option. Indeed, if we run the
resulting git like "git for-each-ref --format='%(describe:tags=0,abbrev=4)'"
you will get complaints from this parser.
The implementation needs redesigning as the data structure is not
equipped to handle more than one options given at the same time, as
we saw earlier.
> @@ -664,6 +733,7 @@ static struct {
> [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
> [ATOM_CREATOR] = { "creator", SOURCE_OBJ },
> [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
> + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
> [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
> [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
> [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
> @@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
> }
> }
>
> +static void grab_describe_values(struct atom_value *val, int deref,
> + struct object *obj)
> +{
> + struct commit *commit = (struct commit *)obj;
> + int i;
> +
> + for (i = 0; i < used_atom_cnt; i++) {
> + struct used_atom *atom = &used_atom[i];
> + const char *name = atom->name;
> + struct atom_value *v = &val[i];
> + int opt;
> +
> + struct child_process cmd = CHILD_PROCESS_INIT;
> + struct strbuf out = STRBUF_INIT;
> + struct strbuf err = STRBUF_INIT;
> +
> + if (!!deref != (*name == '*'))
> + continue;
> + if (deref)
> + name++;
> +
> + if (!skip_prefix(name, "describe", &name) ||
> + (*name && *name != ':'))
> + continue;
This looks overly expensive. Why aren't we looking at the atom_type
and see if it is ATOM_DESCRIBE here?
> + switch(opt) {
> + case D_BARE:
> + break;
> + case D_TAGS:
> + if (atom->u.describe.tagbool)
> + strvec_push(&cmd.args, "--tags");
> + else
> + strvec_push(&cmd.args, "--no-tags");
> + break;
> + case D_ABBREV:
> + strvec_pushf(&cmd.args, "--abbrev=%d",
> + atom->u.describe.length);
> + break;
> + case D_EXCLUDE:
> + strvec_pushf(&cmd.args, "--exclude=%s",
> + atom->u.describe.pattern);
> + break;
> + case D_MATCH:
> + strvec_pushf(&cmd.args, "--match=%s",
> + atom->u.describe.pattern);
> + break;
> + }
Again, it is apparent here that the atom takes only one option at most.
I'll stop here.
Thanks.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] ref-filter: add new "describe" atom
2023-07-06 16:58 ` Junio C Hamano
@ 2023-07-09 6:16 ` Kousik Sanagavarapu
0 siblings, 0 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-09 6:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Hariom Verma
On Thu, Jul 06, 2023 at 09:58:09AM -0700, Junio C Hamano wrote:
> Kousik Sanagavarapu <five231003@gmail.com> writes:
>
> > +describe[:options]:: human-readable name, like
> > + link-git:git-describe[1]; empty string for
> > + undescribable commits. The `describe` string may be
> > + followed by a colon and zero or more comma-separated
> > + options. Descriptions can be inconsistent when tags
> > + are added or removed at the same time.
> > ++
> > +** tags=<bool-value>: Instead of only considering annotated tags, consider
> > + lightweight tags as well.
> > +** abbrev=<number>: Instead of using the default number of hexadecimal digits
> > + (which will vary according to the number of objects in the
> > + repository with a default of 7) of the abbreviated
> > + object name, use <number> digits, or as many digits as
> > + needed to form a unique object name.
> > +** match=<pattern>: Only consider tags matching the given `glob(7)` pattern,
> > + excluding the "refs/tags/" prefix.
> > +** exclude=<pattern>: Do not consider tags matching the given `glob(7)`
> > + pattern,excluding the "refs/tags/" prefix.
>
> You are missing a SP after the comma in "pattern,excluding" above.
>
> The above description is slightly different from what "git describe
> --help" has. If they are described differently on purpose (e.g. you
> may have made "%(describe:abbrev=0)" not to show only the closest
> tag, unlike "git describe --abbrev=0"), the differences should be
> spelled out more explicitly. If the behaviours of the option here
> and the corresponding one there are meant to be the same, then
> either using exactly the same text, or a much abbreviated
> description with a note referring to the option description of "git
> describe", would help the readers better. E.g.
>
> abbrev=<number>;; use at least <number> hexadecimal digits; see
> the corresponding option in linkgit:git-describe[1] for details.
>
> which would make it clear that no behavioral differences are meant.
>
> This new section becomes a part of an existing "labeled list"
> (asciidoctor calls the construct "description list"). Starting the
> new heading this patch adds with 'describe[:options]::' makes sense.
> It is in line with the existing text.
>
> I however think that the list of options is better done as a nested
> description list. Documentation/config/color.txt has an example you
> can imitate. See how slots of color.grep.<slot> are described
> there.
>
> https://docs.asciidoctor.org/asciidoc/latest/lists/description/
> https://asciidoc-py.github.io/userguide.html#_labeled_lists
I read through these and looked at the examples (on the git-scm website
where this is used as well). I'll make the changes. Thanks for this.
> > diff --git a/ref-filter.c b/ref-filter.c
> > index e0d03a9f8e..6ec647c81f 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -5,6 +5,7 @@
> > #include "gpg-interface.h"
> > #include "hex.h"
> > #include "parse-options.h"
> > +#include "run-command.h"
> > #include "refs.h"
> > #include "wildmatch.h"
> > #include "object-name.h"
> > @@ -146,6 +147,7 @@ enum atom_type {
> > ATOM_TAGGERDATE,
> > ATOM_CREATOR,
> > ATOM_CREATORDATE,
> > + ATOM_DESCRIBE,
> > ATOM_SUBJECT,
> > ATOM_BODY,
> > ATOM_TRAILERS,
> > @@ -215,6 +217,13 @@ static struct used_atom {
> > struct email_option {
> > enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
> > } email_option;
> > + struct {
> > + enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,
> > + D_MATCH } option;
> > + unsigned int tagbool;
> > + unsigned int length;
>
> The name "tagbool" sounds strange, as we are not saying
> "lengthint".
Yeah, it does sound strange now that you take the example of "lenghtint".
I'm thinking "use_tags" might be good, but I don't know.
> > + char *pattern;
> > + } describe;
>
> I am a bit confused by this structure, actually, as I cannot quite
> guess from the data structure alone how you intend to use it. Does
> this give a good representation for the piece of data you are trying
> to capture?
>
> For example, %(describe:tags=no,abbrev=4) would become a single atom
> with 0 in .tagbool and 4 in .length, but what does the .option
> member get?
That is true. After I read your email, I started thinking of redesigning
it. My initial thought on doing this was to mimic the other atoms'
design in "struct used_atom" but now that I have read your email, it
seems that such a design doesn't work. Maybe I could do something like
how it is handled in pretty while also not losing the design of other
atoms in "struct used_atom".
> > @@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
> > return 0;
> > }
> >
> > +static int parse_describe_option(const char *arg)
> > +{
> > + if (!arg)
> > + return D_BARE;
> > + else if (starts_with(arg, "tags"))
> > + return D_TAGS;
> > + else if (starts_with(arg, "abbrev"))
> > + return D_ABBREV;
> > + else if(starts_with(arg, "exclude"))
> > + return D_EXCLUDE;
> > + else if (starts_with(arg, "match"))
> > + return D_MATCH;
> > + return -1;
> > +}
> > +
> > +static int describe_atom_parser(struct ref_format *format UNUSED,
> > + struct used_atom *atom,
> > + const char *arg, struct strbuf *err)
> > +{
> > + int opt = parse_describe_option(arg);
> > +
> > + switch (opt) {
> > + case D_BARE:
> > + break;
> > + case D_TAGS:
> > + /*
> > + * It is also possible to just use describe:tags, which
> > + * is just treated as describe:tags=1
> > + */
> > + if (skip_prefix(arg, "tags=", &arg)) {
> > + if (strtoul_ui(arg, 10, &atom->u.describe.tagbool))
>
> This is not how you accept a Boolean.
>
> "1", "0", "yes", "no", "true", "false", "on", "off" are all valid
> values and you use git_parse_maybe_bool() to parse them.
Will make this change.
> > + return strbuf_addf_ret(err, -1, _("boolean value "
> > + "expected describe:tags=%s"), arg);
> > +
> > + } else {
> > + atom->u.describe.tagbool = 1;
> > + }
> > + break;
> > + case D_ABBREV:
> > + skip_prefix(arg, "abbrev=", &arg);
> > + if (strtoul_ui(arg, 10, &atom->u.describe.length))
> > + return strbuf_addf_ret(err, -1, _("positive value "
> > + "expected describe:abbrev=%s"), arg);
> > + break;
> > + case D_EXCLUDE:
> > + skip_prefix(arg, "exclude=", &arg);
> > + atom->u.describe.pattern = xstrdup(arg);
> > + break;
> > + case D_MATCH:
> > + skip_prefix(arg, "match=", &arg);
> > + atom->u.describe.pattern = xstrdup(arg);
> > + break;
> > + default:
> > + return err_bad_arg(err, "describe", arg);
> > + break;
> > + }
> > + atom->u.describe.option = opt;
> > + return 0;
> > +}
>
> Even though the documentation patch we saw earlier said "may be
> followed by a colon and zero or more comma-separated options", this
> seems to expect only and exactly one option. Indeed, if we run the
> resulting git like "git for-each-ref --format='%(describe:tags=0,abbrev=4)'"
> you will get complaints from this parser.
>
> The implementation needs redesigning as the data structure is not
> equipped to handle more than one options given at the same time, as
> we saw earlier.
>
> > @@ -664,6 +733,7 @@ static struct {
> > [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
> > [ATOM_CREATOR] = { "creator", SOURCE_OBJ },
> > [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
> > + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
> > [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
> > [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
> > [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
> > @@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
> > }
> > }
> >
> > +static void grab_describe_values(struct atom_value *val, int deref,
> > + struct object *obj)
> > +{
> > + struct commit *commit = (struct commit *)obj;
> > + int i;
> > +
> > + for (i = 0; i < used_atom_cnt; i++) {
> > + struct used_atom *atom = &used_atom[i];
> > + const char *name = atom->name;
> > + struct atom_value *v = &val[i];
> > + int opt;
> > +
> > + struct child_process cmd = CHILD_PROCESS_INIT;
> > + struct strbuf out = STRBUF_INIT;
> > + struct strbuf err = STRBUF_INIT;
> > +
> > + if (!!deref != (*name == '*'))
> > + continue;
> > + if (deref)
> > + name++;
> > +
> > + if (!skip_prefix(name, "describe", &name) ||
> > + (*name && *name != ':'))
> > + continue;
>
> This looks overly expensive. Why aren't we looking at the atom_type
> and see if it is ATOM_DESCRIBE here?
>
> > + switch(opt) {
> > + case D_BARE:
> > + break;
> > + case D_TAGS:
> > + if (atom->u.describe.tagbool)
> > + strvec_push(&cmd.args, "--tags");
> > + else
> > + strvec_push(&cmd.args, "--no-tags");
> > + break;
> > + case D_ABBREV:
> > + strvec_pushf(&cmd.args, "--abbrev=%d",
> > + atom->u.describe.length);
> > + break;
> > + case D_EXCLUDE:
> > + strvec_pushf(&cmd.args, "--exclude=%s",
> > + atom->u.describe.pattern);
> > + break;
> > + case D_MATCH:
> > + strvec_pushf(&cmd.args, "--match=%s",
> > + atom->u.describe.pattern);
> > + break;
> > + }
>
> Again, it is apparent here that the atom takes only one option at most.
Yeah. I'll reroll with the necessary changes so that we handle many
options like the Documentation patch says and also the other things you
pointed out.
Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 0/3] Add new "describe" atom
2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 2/2] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
@ 2023-07-14 19:20 ` Kousik Sanagavarapu
2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu
` (3 more replies)
2 siblings, 4 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-14 19:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu
Hi,
This patch series addresses the previous comments and so now we can do
for example
git for-each-ref --format="%(describe:tags=yes,abbrev=14)"
PATCH 1/3 - This is a new commit which introduces two new functions for
handling multiple options in ref-filter.
There are two ways to do this
- We change the functions in pretty so that they can be used
generally and not only for placeholders.
- We introduce corresponding functions in ref-filter for
handling atoms.
This patch follows the second approach but the first
approach is also good because we don't duplicate the code.
Or maybe there is a much better approach that I don't see.
PATCH 2/3 - Changes are made so that we can handle multiple options and
also the related docs are a nested description list.
PATCH 3/3 - This commit is left unchanged.
Kousik Sanagavarapu (3):
ref-filter: add multiple-option parsing functions
ref-filter: add new "describe" atom
t6300: run describe atom tests on a different repo
Documentation/git-for-each-ref.txt | 23 ++++
ref-filter.c | 206 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 98 ++++++++++++++
3 files changed, 327 insertions(+)
Range-diff against v1:
-: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing
functions
1: 9e3e652659 ! 2: f6f882884c ref-filter: add new "describe" atom
@@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the
output
ref to the `<committish>` specified in the format.
-+describe[:options]:: human-readable name, like
++describe[:options]:: Human-readable name, like
+ link-git:git-describe[1]; empty string for
+ undescribable commits. The `describe` string may be
+ followed by a colon and zero or more
comma-separated
+ options. Descriptions can be inconsistent when tags
+ are added or removed at the same time.
++
-+** tags=<bool-value>: Instead of only considering annotated tags,
consider
-+ lightweight tags as well.
-+** abbrev=<number>: Instead of using the default number of
hexadecimal digits
-+ (which will vary according to the number of objects
in the
-+ repository with a default of 7) of the abbreviated
-+ object name, use <number> digits, or as many digits
as
-+ needed to form a unique object name.
-+** match=<pattern>: Only consider tags matching the given
`glob(7)` pattern,
-+ excluding the "refs/tags/" prefix.
-+** exclude=<pattern>: Do not consider tags matching the given
`glob(7)`
-+ pattern,excluding the "refs/tags/" prefix.
++--
++tags=<bool-value>;; Instead of only considering annotated tags,
consider
++ lightweight tags as well; see the corresponding
option
++ in linkgit:git-describe[1] for details.
++abbrev=<number>;; Use at least <number> hexadecimal digits; see
++ the corresponding option in linkgit:git-describe[1]
++ for details.
++match=<pattern>;; Only consider tags matching the given `glob(7)`
pattern,
++ excluding the "refs/tags/" prefix; see the
corresponding
++ option in linkgit:git-describe[1] for details.
++exclude=<pattern>;; Do not consider tags matching the given
`glob(7)`
++ pattern, excluding the "refs/tags/" prefix; see the
++ corresponding option in linkgit:git-describe[1] for
++ details.
++--
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>::
## ref-filter.c ##
@@
+ #include "alloc.h"
+ #include "environment.h"
+ #include "gettext.h"
++#include "config.h"
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
@@ ref-filter.c: static struct used_atom {
enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
} email_option;
+ struct {
-+ enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,
-+ D_MATCH } option;
-+ unsigned int tagbool;
-+ unsigned int length;
-+ char *pattern;
++ enum { D_BARE, D_TAGS, D_ABBREV,
++ D_EXCLUDE, D_MATCH } option;
++ const char **args;
+ } describe;
struct refname_atom refname;
char *head;
@@ ref-filter.c: static int contents_atom_parser(struct ref_format
*format, struct
return 0;
}
-+static int parse_describe_option(const char *arg)
-+{
-+ if (!arg)
-+ return D_BARE;
-+ else if (starts_with(arg, "tags"))
-+ return D_TAGS;
-+ else if (starts_with(arg, "abbrev"))
-+ return D_ABBREV;
-+ else if(starts_with(arg, "exclude"))
-+ return D_EXCLUDE;
-+ else if (starts_with(arg, "match"))
-+ return D_MATCH;
-+ return -1;
-+}
-+
+static int describe_atom_parser(struct ref_format *format UNUSED,
+ struct used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
-+ int opt = parse_describe_option(arg);
++ const char *describe_opts[] = {
++ "",
++ "tags",
++ "abbrev",
++ "match",
++ "exclude",
++ NULL
++ };
++
++ struct strvec args = STRVEC_INIT;
++ for (;;) {
++ int found = 0;
++ const char *argval;
++ size_t arglen = 0;
++ int optval = 0;
++ int opt;
++
++ if (!arg)
++ break;
++
++ for (opt = D_BARE; !found && describe_opts[opt]; opt++)
{
++ switch(opt) {
++ case D_BARE:
++ /*
++ * Do nothing. This is the bare describe
++ * atom and we already handle this
above.
++ */
++ break;
++ case D_TAGS:
++ if (match_atom_bool_arg(arg,
describe_opts[opt],
++ &arg, &optval))
{
++ if (!optval)
++ strvec_pushf(&args,
"--no-%s",
++
describe_opts[opt]);
++ else
++ strvec_pushf(&args,
"--%s",
++
describe_opts[opt]);
++ found = 1;
++ }
++ break;
++ case D_ABBREV:
++ if (match_atom_arg_value(arg,
describe_opts[opt],
++ &arg, &argval,
&arglen)) {
++ char *endptr;
++ int ret = 0;
+
-+ switch (opt) {
-+ case D_BARE:
-+ break;
-+ case D_TAGS:
-+ /*
-+ * It is also possible to just use describe:tags, which
-+ * is just treated as describe:tags=1
-+ */
-+ if (skip_prefix(arg, "tags=", &arg)) {
-+ if (strtoul_ui(arg, 10,
&atom->u.describe.tagbool))
-+ return strbuf_addf_ret(err, -1,
_("boolean value "
-+ "expected
describe:tags=%s"), arg);
++ if (!arglen)
++ ret = -1;
++ if (strtol(argval, &endptr, 10)
< 0)
++ ret = -1;
++ if (endptr - argval != arglen)
++ ret = -1;
+
-+ } else {
-+ atom->u.describe.tagbool = 1;
++ if (ret)
++ return
strbuf_addf_ret(err, ret,
++
_("positive value expected describe:abbrev=%s"), argval);
++ strvec_pushf(&args, "--%s=%.*s",
++ describe_opts[opt],
++ (int)arglen,
argval);
++ found = 1;
++ }
++ break;
++ case D_MATCH:
++ case D_EXCLUDE:
++ if (match_atom_arg_value(arg,
describe_opts[opt],
++ &arg, &argval,
&arglen)) {
++ if (!arglen)
++ return
strbuf_addf_ret(err, -1,
++ _("value
expected describe:%s="), describe_opts[opt]);
++ strvec_pushf(&args, "--%s=%.*s",
++ describe_opts[opt],
++ (int)arglen,
argval);
++ found = 1;
++ }
++ break;
++ }
+ }
-+ break;
-+ case D_ABBREV:
-+ skip_prefix(arg, "abbrev=", &arg);
-+ if (strtoul_ui(arg, 10, &atom->u.describe.length))
-+ return strbuf_addf_ret(err, -1, _("positive
value "
-+ "expected
describe:abbrev=%s"), arg);
-+ break;
-+ case D_EXCLUDE:
-+ skip_prefix(arg, "exclude=", &arg);
-+ atom->u.describe.pattern = xstrdup(arg);
-+ break;
-+ case D_MATCH:
-+ skip_prefix(arg, "match=", &arg);
-+ atom->u.describe.pattern = xstrdup(arg);
-+ break;
-+ default:
-+ return err_bad_arg(err, "describe", arg);
-+ break;
++ if (!found)
++ break;
+ }
-+ atom->u.describe.option = opt;
++ atom->u.describe.args = strvec_detach(&args);
+ return 0;
+}
+
@@ ref-filter.c: static void append_lines(struct strbuf *out, const
char *buf, unsi
+
+ for (i = 0; i < used_atom_cnt; i++) {
+ struct used_atom *atom = &used_atom[i];
++ enum atom_type type = atom->atom_type;
+ const char *name = atom->name;
+ struct atom_value *v = &val[i];
-+ int opt;
+
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+
++ if (type != ATOM_DESCRIBE)
++ continue;
++
+ if (!!deref != (*name == '*'))
+ continue;
+ if (deref)
@@ ref-filter.c: static void append_lines(struct strbuf *out, const
char *buf, unsi
+ else
+ name++;
+
-+ opt = parse_describe_option(name);
-+ if (opt < 0)
-+ continue;
-+
+ cmd.git_cmd = 1;
+ strvec_push(&cmd.args, "describe");
-+
-+ switch(opt) {
-: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing
functions
1: 9e3e652659 ! 2: f6f882884c ref-filter: add new "describe" atom
@@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the
output
ref to the `<committish>` specified in the format.
-+describe[:options]:: human-readable name, like
++describe[:options]:: Human-readable name, like
+ link-git:git-describe[1]; empty string for
+ undescribable commits. The `describe` string may be
+ followed by a colon and zero or more
comma-separated
+ options. Descriptions can be inconsistent when tags
+ are added or removed at the same time.
++
-+** tags=<bool-value>: Instead of only considering annotated tags,
consider
-+ lightweight tags as well.
-+** abbrev=<number>: Instead of using the default number of
hexadecimal digits
-+ (which will vary according to the number of objects
in the
-+ repository with a default of 7) of the abbreviated
-+ object name, use <number> digits, or as many digits
as
-+ needed to form a unique object name.
-+** match=<pattern>: Only consider tags matching the given
`glob(7)` pattern,
-+ excluding the "refs/tags/" prefix.
-+** exclude=<pattern>: Do not consider tags matching the given
`glob(7)`
-+ pattern,excluding the "refs/tags/" prefix.
++--
++tags=<bool-value>;; Instead of only considering annotated tags,
consider
++ lightweight tags as well; see the corresponding
option
++ in linkgit:git-describe[1] for details.
++abbrev=<number>;; Use at least <number> hexadecimal digits; see
++ the corresponding option in linkgit:git-describe[1]
++ for details.
++match=<pattern>;; Only consider tags matching the given `glob(7)`
pattern,
++ excluding the "refs/tags/" prefix; see the
corresponding
++ option in linkgit:git-describe[1] for details.
++exclude=<pattern>;; Do not consider tags matching the given
`glob(7)`
++ pattern, excluding the "refs/tags/" prefix; see the
++ corresponding option in linkgit:git-describe[1] for
++ details.
++--
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>::
## ref-filter.c ##
@@
+ #include "alloc.h"
+ #include "environment.h"
+ #include "gettext.h"
++#include "config.h"
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
@@ ref-filter.c: static struct used_atom {
enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
} email_option;
+ struct {
-+ enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,
-+ D_MATCH } option;
-+ unsigned int tagbool;
-+ unsigned int length;
-+ char *pattern;
++ enum { D_BARE, D_TAGS, D_ABBREV,
++ D_EXCLUDE, D_MATCH } option;
++ const char **args;
+ } describe;
struct refname_atom refname;
char *head;
@@ ref-filter.c: static int contents_atom_parser(struct ref_format
*format, struct
return 0;
}
-+static int parse_describe_option(const char *arg)
-+{
-+ if (!arg)
-+ return D_BARE;
-+ else if (starts_with(arg, "tags"))
-+ return D_TAGS;
-+ else if (starts_with(arg, "abbrev"))
-+ return D_ABBREV;
-+ else if(starts_with(arg, "exclude"))
-+ return D_EXCLUDE;
-+ else if (starts_with(arg, "match"))
-+ return D_MATCH;
-+ return -1;
-+}
-+
+static int describe_atom_parser(struct ref_format *format UNUSED,
+ struct used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
-+ int opt = parse_describe_option(arg);
++ const char *describe_opts[] = {
++ "",
++ "tags",
++ "abbrev",
++ "match",
++ "exclude",
++ NULL
++ };
++
++ struct strvec args = STRVEC_INIT;
++ for (;;) {
++ int found = 0;
++ const char *argval;
++ size_t arglen = 0;
++ int optval = 0;
++ int opt;
++
++ if (!arg)
++ break;
++
++ for (opt = D_BARE; !found && describe_opts[opt]; opt++)
{
++ switch(opt) {
++ case D_BARE:
++ /*
++ * Do nothing. This is the bare describe
++ * atom and we already handle this
above.
++ */
++ break;
++ case D_TAGS:
++ if (match_atom_bool_arg(arg,
describe_opts[opt],
++ &arg, &optval))
{
++ if (!optval)
++ strvec_pushf(&args,
"--no-%s",
++
describe_opts[opt]);
++ else
++ strvec_pushf(&args,
"--%s",
++
describe_opts[opt]);
++ found = 1;
++ }
++ break;
++ case D_ABBREV:
++ if (match_atom_arg_value(arg,
describe_opts[opt],
++ &arg, &argval,
&arglen)) {
++ char *endptr;
++ int ret = 0;
+
-+ switch (opt) {
-+ case D_BARE:
-+ break;
-+ case D_TAGS:
-+ /*
-+ * It is also possible to just use describe:tags, which
-+ * is just treated as describe:tags=1
-+ */
-+ if (skip_prefix(arg, "tags=", &arg)) {
-+ if (strtoul_ui(arg, 10,
&atom->u.describe.tagbool))
-+ return strbuf_addf_ret(err, -1,
_("boolean value "
-+ "expected
describe:tags=%s"), arg);
++ if (!arglen)
++ ret = -1;
++ if (strtol(argval, &endptr, 10)
< 0)
++ ret = -1;
++ if (endptr - argval != arglen)
++ ret = -1;
+
-+ } else {
-+ atom->u.describe.tagbool = 1;
++ if (ret)
++ return
strbuf_addf_ret(err, ret,
++
_("positive value expected describe:abbrev=%s"), argval);
++ strvec_pushf(&args, "--%s=%.*s",
++ describe_opts[opt],
++ (int)arglen,
argval);
++ found = 1;
++ }
++ break;
++ case D_MATCH:
++ case D_EXCLUDE:
++ if (match_atom_arg_value(arg,
describe_opts[opt],
++ &arg, &argval,
&arglen)) {
++ if (!arglen)
++ return
strbuf_addf_ret(err, -1,
...skipping...
-+ case D_BARE:
-+ break;
-+ case D_TAGS:
-+ if (atom->u.describe.tagbool)
-+ strvec_push(&cmd.args, "--tags");
-+ else
-+ strvec_push(&cmd.args, "--no-tags");
-+ break;
-+ case D_ABBREV:
-+ strvec_pushf(&cmd.args, "--abbrev=%d",
-+ atom->u.describe.length);
-+ break;
-+ case D_EXCLUDE:
-+ strvec_pushf(&cmd.args, "--exclude=%s",
-+ atom->u.describe.pattern);
-+ break;
-+ case D_MATCH:
-+ strvec_pushf(&cmd.args, "--match=%s",
-+ atom->u.describe.pattern);
-+ break;
-+ }
-+
++ strvec_pushv(&cmd.args, atom->u.describe.args);
+ strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+ if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+ error(_("failed to run 'describe'"));
2: 43cd3eef3c = 3: a5122bf5e2 t6300: run describe atom tests on a
different repo
fivlite BR describe4 ~ | Documents | git git checkout master
Switched to branch 'master'
Your branch is up to date with 'upstream/master'.
fivlite BR master ~ | Documents | git git range-diff
9748a6820043..describe4 master..describe5
-: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing
functions
1: 9e3e652659 ! 2: f6f882884c ref-filter: add new "describe" atom
@@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the
output
ref to the `<committish>` specified in the format.
-+describe[:options]:: human-readable name, like
++describe[:options]:: Human-readable name, like
+ link-git:git-describe[1]; empty string for
+ undescribable commits. The `describe` string may be
+ followed by a colon and zero or more
comma-separated
+ options. Descriptions can be inconsistent when tags
+ are added or removed at the same time.
++
-+** tags=<bool-value>: Instead of only considering annotated tags,
consider
-+ lightweight tags as well.
-+** abbrev=<number>: Instead of using the default number of
hexadecimal digits
-+ (which will vary according to the number of objects
in the
-+ repository with a default of 7) of the abbreviated
-+ object name, use <number> digits, or as many digits
as
-+ needed to form a unique object name.
-+** match=<pattern>: Only consider tags matching the given
`glob(7)` pattern,
-+ excluding the "refs/tags/" prefix.
-+** exclude=<pattern>: Do not consider tags matching the given
`glob(7)`
-+ pattern,excluding the "refs/tags/" prefix.
++--
++tags=<bool-value>;; Instead of only considering annotated tags,
consider
++ lightweight tags as well; see the corresponding
option
++ in linkgit:git-describe[1] for details.
++abbrev=<number>;; Use at least <number> hexadecimal digits; see
++ the corresponding option in linkgit:git-describe[1]
++ for details.
++match=<pattern>;; Only consider tags matching the given `glob(7)`
pattern,
++ excluding the "refs/tags/" prefix; see the
corresponding
...skipping...
-+ case D_BARE:
-+ break;
-+ case D_TAGS:
-+ if (atom->u.describe.tagbool)
-+ strvec_push(&cmd.args, "--tags");
-+ else
-+ strvec_push(&cmd.args, "--no-tags");
-+ break;
-+ case D_ABBREV:
-+ strvec_pushf(&cmd.args, "--abbrev=%d",
-+ atom->u.describe.length);
-+ break;
-+ case D_EXCLUDE:
-+ strvec_pushf(&cmd.args, "--exclude=%s",
-+ atom->u.describe.pattern);
-+ break;
-+ case D_MATCH:
-+ strvec_pushf(&cmd.args, "--match=%s",
-+ atom->u.describe.pattern);
-+ break;
-+ }
-+
++ strvec_pushv(&cmd.args, atom->u.describe.args);
+ strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+ if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+ error(_("failed to run 'describe'"));
2: 43cd3eef3c = 3: a5122bf5e2 t6300: run describe atom tests on a
different repo
--
2.41.0.321.g26b82700c0.dirty
^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
@ 2023-07-14 19:20 ` Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu
` (2 subsequent siblings)
3 siblings, 0 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-14 19:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu, Christian Couder, Hariom Verma
The functions
match_placeholder_arg_value()
match_placeholder_bool_arg()
were added in pretty (4f732e0fd7 (pretty: allow %(trailers) options
with explicit value, 2019-01-29)) to parse multiple options in an
argument to --pretty. For example,
git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )"
will output all the trailers matching the key and seperates them by
commas per commit.
Add similar functions,
match_atom_arg_value()
match_atom_bool_arg()
in ref-filter. A particular use of this can be seen in the subsequent
commit where we parse the options given to a new atom "describe".
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index e0d03a9f8e..b170994d9d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -251,6 +251,65 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)
return -1;
}
+static int match_atom_arg_value(const char *to_parse, const char *candidate,
+ const char **end, const char **valuestart,
+ size_t *valuelen)
+{
+ const char *atom;
+
+ if (!(skip_prefix(to_parse, candidate, &atom)))
+ return 0;
+ if (valuestart) {
+ if (*atom == '=') {
+ *valuestart = atom + 1;
+ *valuelen = strcspn(*valuestart, ",\0");
+ atom = *valuestart + *valuelen;
+ } else {
+ if (*atom != ',' && *atom != '\0')
+ return 0;
+ *valuestart = NULL;
+ *valuelen = 0;
+ }
+ }
+ if (*atom == ',') {
+ *end = atom + 1;
+ return 1;
+ }
+ if (*atom == '\0') {
+ *end = atom;
+ return 1;
+ }
+ return 0;
+}
+
+static int match_atom_bool_arg(const char *to_parse, const char *candidate,
+ const char **end, int *val)
+{
+ const char *argval;
+ char *strval;
+ size_t arglen;
+ int v;
+
+ if (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen))
+ return 0;
+
+ if (!argval) {
+ *val = 1;
+ return 1;
+ }
+
+ strval = xstrndup(argval, arglen);
+ v = git_parse_maybe_bool(strval);
+ free(strval);
+
+ if (v == -1)
+ return 0;
+
+ *val = v;
+
+ return 1;
+}
+
static int color_atom_parser(struct ref_format *format, struct used_atom *atom,
const char *color_value, struct strbuf *err)
{
--
2.41.0.321.g26b82700c0.dirty
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 2/3] ref-filter: add new "describe" atom
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu
@ 2023-07-14 19:20 ` Kousik Sanagavarapu
2023-07-14 20:57 ` Junio C Hamano
2023-07-14 19:20 ` [PATCH v2 3/3] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
3 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-14 19:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu, Christian Couder, Hariom Verma
Duplicate the logic of %(describe) and friends from pretty to
ref-filter. In the future, this change helps in unifying both the
formats as ref-filter will be able to do everything that pretty is doing
and we can have a single interface.
The new atom "describe" and its friends are equivalent to the existing
pretty formats with the same name.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
Documentation/git-for-each-ref.txt | 23 +++++
ref-filter.c | 147 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 85 +++++++++++++++++
3 files changed, 255 insertions(+)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 1e215d4e73..2a44119f38 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -231,6 +231,29 @@ ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the output
ref to the `<committish>` specified in the format.
+describe[:options]:: Human-readable name, like
+ link-git:git-describe[1]; empty string for
+ undescribable commits. The `describe` string may be
+ followed by a colon and zero or more comma-separated
+ options. Descriptions can be inconsistent when tags
+ are added or removed at the same time.
++
+--
+tags=<bool-value>;; Instead of only considering annotated tags, consider
+ lightweight tags as well; see the corresponding option
+ in linkgit:git-describe[1] for details.
+abbrev=<number>;; Use at least <number> hexadecimal digits; see
+ the corresponding option in linkgit:git-describe[1]
+ for details.
+match=<pattern>;; Only consider tags matching the given `glob(7)` pattern,
+ excluding the "refs/tags/" prefix; see the corresponding
+ option in linkgit:git-describe[1] for details.
+exclude=<pattern>;; Do not consider tags matching the given `glob(7)`
+ pattern, excluding the "refs/tags/" prefix; see the
+ corresponding option in linkgit:git-describe[1] for
+ details.
+--
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index b170994d9d..fe4830dbea 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2,9 +2,11 @@
#include "alloc.h"
#include "environment.h"
#include "gettext.h"
+#include "config.h"
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
+#include "run-command.h"
#include "refs.h"
#include "wildmatch.h"
#include "object-name.h"
@@ -146,6 +148,7 @@ enum atom_type {
ATOM_TAGGERDATE,
ATOM_CREATOR,
ATOM_CREATORDATE,
+ ATOM_DESCRIBE,
ATOM_SUBJECT,
ATOM_BODY,
ATOM_TRAILERS,
@@ -215,6 +218,11 @@ static struct used_atom {
struct email_option {
enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
} email_option;
+ struct {
+ enum { D_BARE, D_TAGS, D_ABBREV,
+ D_EXCLUDE, D_MATCH } option;
+ const char **args;
+ } describe;
struct refname_atom refname;
char *head;
} u;
@@ -521,6 +529,94 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
return 0;
}
+static int describe_atom_parser(struct ref_format *format UNUSED,
+ struct used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
+ const char *describe_opts[] = {
+ "",
+ "tags",
+ "abbrev",
+ "match",
+ "exclude",
+ NULL
+ };
+
+ struct strvec args = STRVEC_INIT;
+ for (;;) {
+ int found = 0;
+ const char *argval;
+ size_t arglen = 0;
+ int optval = 0;
+ int opt;
+
+ if (!arg)
+ break;
+
+ for (opt = D_BARE; !found && describe_opts[opt]; opt++) {
+ switch(opt) {
+ case D_BARE:
+ /*
+ * Do nothing. This is the bare describe
+ * atom and we already handle this above.
+ */
+ break;
+ case D_TAGS:
+ if (match_atom_bool_arg(arg, describe_opts[opt],
+ &arg, &optval)) {
+ if (!optval)
+ strvec_pushf(&args, "--no-%s",
+ describe_opts[opt]);
+ else
+ strvec_pushf(&args, "--%s",
+ describe_opts[opt]);
+ found = 1;
+ }
+ break;
+ case D_ABBREV:
+ if (match_atom_arg_value(arg, describe_opts[opt],
+ &arg, &argval, &arglen)) {
+ char *endptr;
+ int ret = 0;
+
+ if (!arglen)
+ ret = -1;
+ if (strtol(argval, &endptr, 10) < 0)
+ ret = -1;
+ if (endptr - argval != arglen)
+ ret = -1;
+
+ if (ret)
+ return strbuf_addf_ret(err, ret,
+ _("positive value expected describe:abbrev=%s"), argval);
+ strvec_pushf(&args, "--%s=%.*s",
+ describe_opts[opt],
+ (int)arglen, argval);
+ found = 1;
+ }
+ break;
+ case D_MATCH:
+ case D_EXCLUDE:
+ if (match_atom_arg_value(arg, describe_opts[opt],
+ &arg, &argval, &arglen)) {
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("value expected describe:%s="), describe_opts[opt]);
+ strvec_pushf(&args, "--%s=%.*s",
+ describe_opts[opt],
+ (int)arglen, argval);
+ found = 1;
+ }
+ break;
+ }
+ }
+ if (!found)
+ break;
+ }
+ atom->u.describe.args = strvec_detach(&args);
+ return 0;
+}
+
static int raw_atom_parser(struct ref_format *format UNUSED,
struct used_atom *atom,
const char *arg, struct strbuf *err)
@@ -723,6 +819,7 @@ static struct {
[ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
[ATOM_CREATOR] = { "creator", SOURCE_OBJ },
[ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
+ [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
[ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
@@ -1542,6 +1639,54 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
}
}
+static void grab_describe_values(struct atom_value *val, int deref,
+ struct object *obj)
+{
+ struct commit *commit = (struct commit *)obj;
+ int i;
+
+ for (i = 0; i < used_atom_cnt; i++) {
+ struct used_atom *atom = &used_atom[i];
+ enum atom_type type = atom->atom_type;
+ const char *name = atom->name;
+ struct atom_value *v = &val[i];
+
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+
+ if (type != ATOM_DESCRIBE)
+ continue;
+
+ if (!!deref != (*name == '*'))
+ continue;
+ if (deref)
+ name++;
+
+ if (!skip_prefix(name, "describe", &name) ||
+ (*name && *name != ':'))
+ continue;
+ if (!*name)
+ name = NULL;
+ else
+ name++;
+
+ cmd.git_cmd = 1;
+ strvec_push(&cmd.args, "describe");
+ strvec_pushv(&cmd.args, atom->u.describe.args);
+ strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+ if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+ error(_("failed to run 'describe'"));
+ v->s = xstrdup("");
+ continue;
+ }
+ strbuf_rtrim(&out);
+ v->s = strbuf_detach(&out, NULL);
+
+ strbuf_release(&err);
+ }
+}
+
/* See grab_values */
static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)
{
@@ -1651,12 +1796,14 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_tag_values(val, deref, obj);
grab_sub_body_contents(val, deref, data);
grab_person("tagger", val, deref, buf);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_COMMIT:
grab_commit_values(val, deref, obj);
grab_sub_body_contents(val, deref, data);
grab_person("author", val, deref, buf);
grab_person("committer", val, deref, buf);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_TREE:
/* grab_tree_values(val, deref, obj, buf, sz); */
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 5c00607608..98ea37d336 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -561,6 +561,91 @@ test_expect_success 'color.ui=always does not override tty check' '
test_cmp expected.bare actual
'
+test_expect_success 'describe atom vs git describe' '
+ test_when_finished "rm -rf describe-repo" &&
+
+ git init describe-repo &&
+ (
+ cd describe-repo &&
+
+ test_commit --no-tag one &&
+ git tag tagone &&
+
+ test_commit --no-tag two &&
+ git tag -a -m "tag two" tagtwo &&
+
+ git for-each-ref refs/tags/ --format="%(objectname)" >obj &&
+ while read hash
+ do
+ if desc=$(git describe $hash)
+ then
+ : >expect-contains-good
+ else
+ : >expect-contains-bad
+ fi &&
+ echo "$hash $desc" || return 1
+ done <obj >expect &&
+ test_path_exists expect-contains-good &&
+ test_path_exists expect-contains-bad &&
+
+ git for-each-ref --format="%(objectname) %(describe)" \
+ refs/tags/ >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+ )
+'
+
+test_expect_success 'describe:tags vs describe --tags' '
+ test_when_finished "git tag -d tagname" &&
+ git tag tagname &&
+ git describe --tags >expect &&
+ git for-each-ref --format="%(describe:tags)" refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '
+ test_when_finished "git tag -d tagname" &&
+
+ # Case 1: We have commits between HEAD and the most
+ # recent tag reachable from it
+ test_commit --no-tag file &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual &&
+
+ # Make sure the hash used is atleast 14 digits long
+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+ test 15 -le $(wc -c <hexpart) &&
+
+ # Case 2: We have a tag at HEAD, describe directly gives
+ # the name of the tag
+ git tag -a -m tagged tagname &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual &&
+ test tagname = $(cat actual)
+'
+
+test_expect_success 'describe:match=... vs describe --match ...' '
+ test_when_finished "git tag -d tag-match" &&
+ git tag -a -m "tag match" tag-match &&
+ git describe --match "*-match" >expect &&
+ git for-each-ref --format="%(describe:match="*-match")" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'describe:exclude:... vs describe --exclude ...' '
+ test_when_finished "git tag -d tag-exclude" &&
+ git tag -a -m "tag exclude" tag-exclude &&
+ git describe --exclude "*-exclude" >expect &&
+ git for-each-ref --format="%(describe:exclude="*-exclude")" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
cat >expected <<\EOF
heads/main
tags/main
--
2.41.0.321.g26b82700c0.dirty
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 3/3] t6300: run describe atom tests on a different repo
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu
@ 2023-07-14 19:20 ` Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
3 siblings, 0 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-14 19:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu, Christian Couder, Hariom Verma
The tests for "describe" atom and its friends currently run on the main
repo of t6300, expect for the test for "bare describe", which is run on
"describe-repo".
Things can get messy with the other tests when such changes to a repo
are done (for example, a new commit or a tag is introduced), especially
in t6300 where the tests depend on commits and tags.
An example for this can be seen in [1], where writing the tests the
current way fails the test "Verify sorts with raw:size" on linux-sha256.
This, at first glance, seems totally unrelated.
Digging in a bit deeper, it is apparent that this behavior is because of
the changes in the repo introduced when writing the "describe" tests,
which changes the raw:size of an object. Such a change in raw-size would
have been, however, small if we were dealing with SHA1, but since we are
dealing with SHA256, the change in raw:size is so significant that it
fails the above mentioned test.
So, run all the "describe" atom tests on "describe-repo", which doesn't
interfere with the main repo on which the tests in t6300 are run.
[1]: https://github.com/five-sh/git/actions/runs/5446892074/jobs/9908256427
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
t/t6300-for-each-ref.sh | 101 +++++++++++++++++++++++-----------------
1 file changed, 57 insertions(+), 44 deletions(-)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 98ea37d336..181b04e699 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -561,9 +561,7 @@ test_expect_success 'color.ui=always does not override tty check' '
test_cmp expected.bare actual
'
-test_expect_success 'describe atom vs git describe' '
- test_when_finished "rm -rf describe-repo" &&
-
+test_expect_success 'setup for describe atom tests' '
git init describe-repo &&
(
cd describe-repo &&
@@ -572,9 +570,16 @@ test_expect_success 'describe atom vs git describe' '
git tag tagone &&
test_commit --no-tag two &&
- git tag -a -m "tag two" tagtwo &&
+ git tag -a -m "tag two" tagtwo
+ )
+'
+
+test_expect_success 'describe atom vs git describe' '
+ (
+ cd describe-repo &&
- git for-each-ref refs/tags/ --format="%(objectname)" >obj &&
+ git for-each-ref --format="%(objectname)" \
+ refs/tags/ >obj &&
while read hash
do
if desc=$(git describe $hash)
@@ -596,54 +601,62 @@ test_expect_success 'describe atom vs git describe' '
'
test_expect_success 'describe:tags vs describe --tags' '
- test_when_finished "git tag -d tagname" &&
- git tag tagname &&
- git describe --tags >expect &&
- git for-each-ref --format="%(describe:tags)" refs/heads/ >actual &&
- test_cmp expect actual
+ (
+ cd describe-repo &&
+ git describe --tags >expect &&
+ git for-each-ref --format="%(describe:tags)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+ )
'
test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '
- test_when_finished "git tag -d tagname" &&
-
- # Case 1: We have commits between HEAD and the most
- # recent tag reachable from it
- test_commit --no-tag file &&
- git describe --abbrev=14 >expect &&
- git for-each-ref --format="%(describe:abbrev=14)" \
- refs/heads/ >actual &&
- test_cmp expect actual &&
-
- # Make sure the hash used is atleast 14 digits long
- sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
- test 15 -le $(wc -c <hexpart) &&
-
- # Case 2: We have a tag at HEAD, describe directly gives
- # the name of the tag
- git tag -a -m tagged tagname &&
- git describe --abbrev=14 >expect &&
- git for-each-ref --format="%(describe:abbrev=14)" \
- refs/heads/ >actual &&
- test_cmp expect actual &&
- test tagname = $(cat actual)
+ (
+ cd describe-repo &&
+
+ # Case 1: We have commits between HEAD and the most
+ # recent tag reachable from it
+ test_commit --no-tag file &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual &&
+
+ # Make sure the hash used is atleast 14 digits long
+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+ test 15 -le $(wc -c <hexpart) &&
+
+ # Case 2: We have a tag at HEAD, describe directly gives
+ # the name of the tag
+ git tag -a -m tagged tagname &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/ >actual &&
+ test_cmp expect actual &&
+ test tagname = $(cat actual)
+ )
'
test_expect_success 'describe:match=... vs describe --match ...' '
- test_when_finished "git tag -d tag-match" &&
- git tag -a -m "tag match" tag-match &&
- git describe --match "*-match" >expect &&
- git for-each-ref --format="%(describe:match="*-match")" \
- refs/heads/ >actual &&
- test_cmp expect actual
+ (
+ cd describe-repo &&
+ git tag -a -m "tag match" tag-match &&
+ git describe --match "*-match" >expect &&
+ git for-each-ref --format="%(describe:match="*-match")" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+ )
'
test_expect_success 'describe:exclude:... vs describe --exclude ...' '
- test_when_finished "git tag -d tag-exclude" &&
- git tag -a -m "tag exclude" tag-exclude &&
- git describe --exclude "*-exclude" >expect &&
- git for-each-ref --format="%(describe:exclude="*-exclude")" \
- refs/heads/ >actual &&
- test_cmp expect actual
+ (
+ cd describe-repo &&
+ git tag -a -m "tag exclude" tag-exclude &&
+ git describe --exclude "*-exclude" >expect &&
+ git for-each-ref --format="%(describe:exclude="*-exclude")" \
+ refs/heads/ >actual &&
+ test_cmp expect actual
+ )
'
cat >expected <<\EOF
--
2.41.0.321.g26b82700c0.dirty
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/3] ref-filter: add new "describe" atom
2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu
@ 2023-07-14 20:57 ` Junio C Hamano
2023-07-15 18:24 ` Kousik Sanagavarapu
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2023-07-14 20:57 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> + struct {
> + enum { D_BARE, D_TAGS, D_ABBREV,
> + D_EXCLUDE, D_MATCH } option;
> + const char **args;
> + } describe;
As you parse this into a strvec that has command line options for
the "git describe" invocation, I do not see the point of having the
"enum option" in this struct. The describe->option member seems to
be unused throughout this patch.
In fact, a single "const char **describe_args" should be able to
replace the structure, no?
> +static int describe_atom_parser(struct ref_format *format UNUSED,
> + struct used_atom *atom,
> + const char *arg, struct strbuf *err)
> +{
> + const char *describe_opts[] = {
> + "",
> + "tags",
> + "abbrev",
> + "match",
> + "exclude",
> + NULL
> + };
> +
> + struct strvec args = STRVEC_INIT;
> + for (;;) {
> + int found = 0;
> + const char *argval;
> + size_t arglen = 0;
> + int optval = 0;
> + int opt;
> +
> + if (!arg)
> + break;
> +
> + for (opt = D_BARE; !found && describe_opts[opt]; opt++) {
> + switch(opt) {
> + case D_BARE:
> + /*
> + * Do nothing. This is the bare describe
> + * atom and we already handle this above.
> + */
> + break;
> + case D_TAGS:
> + if (match_atom_bool_arg(arg, describe_opts[opt],
> + &arg, &optval)) {
> + if (!optval)
> + strvec_pushf(&args, "--no-%s",
> + describe_opts[opt]);
> + else
> + strvec_pushf(&args, "--%s",
> + describe_opts[opt]);
> + found = 1;
> + }
As match_atom_bool_arg() and ...
> + break;
> + case D_ABBREV:
> + if (match_atom_arg_value(arg, describe_opts[opt],
> + &arg, &argval, &arglen)) {
> + char *endptr;
> + int ret = 0;
> +
> + if (!arglen)
> + ret = -1;
> + if (strtol(argval, &endptr, 10) < 0)
> + ret = -1;
> + if (endptr - argval != arglen)
> + ret = -1;
> +
> + if (ret)
> + return strbuf_addf_ret(err, ret,
> + _("positive value expected describe:abbrev=%s"), argval);
> + strvec_pushf(&args, "--%s=%.*s",
> + describe_opts[opt],
> + (int)arglen, argval);
> + found = 1;
> + }
... match_atom_arg_value() are both silent when they return false,
we do not see any diagnosis when these two case arms set the "found"
flag. Shouldn't we have a corresponding "else" clause to these "if
(match_atom_blah())" blocks to issue an error message or something?
> + break;
> + case D_MATCH:
> + case D_EXCLUDE:
> + if (match_atom_arg_value(arg, describe_opts[opt],
> + &arg, &argval, &arglen)) {
> + if (!arglen)
> + return strbuf_addf_ret(err, -1,
> + _("value expected describe:%s="), describe_opts[opt]);
> + strvec_pushf(&args, "--%s=%.*s",
> + describe_opts[opt],
> + (int)arglen, argval);
> + found = 1;
> + }
> + break;
> + }
> + }
> + if (!found)
> + break;
> + }
> + atom->u.describe.args = strvec_detach(&args);
> + return 0;
> +}
> +
> static int raw_atom_parser(struct ref_format *format UNUSED,
> struct used_atom *atom,
> const char *arg, struct strbuf *err)
> @@ -723,6 +819,7 @@ static struct {
> [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
> [ATOM_CREATOR] = { "creator", SOURCE_OBJ },
> [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
> + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
> [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
> [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
> [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
> @@ -1542,6 +1639,54 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
> }
> }
>
> +static void grab_describe_values(struct atom_value *val, int deref,
> + struct object *obj)
> +{
> + struct commit *commit = (struct commit *)obj;
> + int i;
> +
> + for (i = 0; i < used_atom_cnt; i++) {
> + struct used_atom *atom = &used_atom[i];
> + enum atom_type type = atom->atom_type;
> + const char *name = atom->name;
> + struct atom_value *v = &val[i];
> +
> + struct child_process cmd = CHILD_PROCESS_INIT;
> + struct strbuf out = STRBUF_INIT;
> + struct strbuf err = STRBUF_INIT;
> +
> + if (type != ATOM_DESCRIBE)
> + continue;
We already have parsed the %(describe:...) and the result is stored
in the used_atom[] array. We iterate over the array, and we just
found that its atom_type member is ATOM_DESCRIBE here (otherwise we
would have moved on to the next array element).
> + if (!!deref != (*name == '*'))
> + continue;
This is trying to avoid %(*describe) answering when the given object
is the tag itself, or %(describe) answering when the given object is
what the tag dereferences to, so having it here makes sense (by the
way, do you add any test for "%(*describe)?").
Now, is the code from here ...
> + if (deref)
> + name++;
> +
> + if (!skip_prefix(name, "describe", &name) ||
> + (*name && *name != ':'))
> + continue;
> + if (!*name)
> + name = NULL;
> + else
> + name++;
... down to here doing anything useful? After all, you already have
all you need to describe the commit in atom->u.describe_args to run
"git describe" with, no? In fact, after computing "name" with the
above code with some complexity, nobody even looks at it.
Perhaps the above was copied from some other grab_* functions; the
reason why they were relevant there needs to be understood, and it
also has to be considered if the same reason to have the code here
applies to this codepath.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/3] ref-filter: add new "describe" atom
2023-07-14 20:57 ` Junio C Hamano
@ 2023-07-15 18:24 ` Kousik Sanagavarapu
2023-07-15 18:56 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-15 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Hariom Verma
On Fri, Jul 14, 2023 at 01:57:40PM -0700, Junio C Hamano wrote:
> Kousik Sanagavarapu <five231003@gmail.com> writes:
>
> > + struct {
> > + enum { D_BARE, D_TAGS, D_ABBREV,
> > + D_EXCLUDE, D_MATCH } option;
> > + const char **args;
> > + } describe;
>
> As you parse this into a strvec that has command line options for
> the "git describe" invocation, I do not see the point of having the
> "enum option" in this struct. The describe->option member seems to
> be unused throughout this patch.
>
> In fact, a single "const char **describe_args" should be able to
> replace the structure, no?
I kept the enum because I thought it could act as an index for the
describe_opts array. Now that I think about it,
diff --git a/ref-filter.c b/ref-filter.c
index fe4830dbea..df7cb39be2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -219,9 +219,7 @@ static struct used_atom {
enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
} email_option;
struct {
- enum { D_BARE, D_TAGS, D_ABBREV,
- D_EXCLUDE, D_MATCH } option;
- const char **args;
+ const char **decsribe_args;
} describe;
struct refname_atom refname;
char *head;
@@ -533,13 +531,16 @@ static int describe_atom_parser(struct ref_format *format UNUSED,
struct used_atom *atom,
const char *arg, struct strbuf *err)
{
- const char *describe_opts[] = {
- "",
- "tags",
- "abbrev",
- "match",
- "exclude",
- NULL
+ struct {
+ char *optname;
+ enum { D_BARE, D_TAGS, D_ABBREV,
+ D_MATCH, D_EXCLUDE } option;
+ } describe_opts[] = {
+ { "", D_BARE },
+ { "tags", D_TAGS },
+ { "abbrev", D_ABBREV },
+ { "match", D_MATCH },
+ { "exclude", D_EXCLUDE }
};
struct strvec args = STRVEC_INIT;
conveys it better or is it too much unnecessary stuff to and should we
just do
struct {
const char **describe_args;
} describe;
leaving the describe_opts array as is and changing the how the switch is
written.
> > +static int describe_atom_parser(struct ref_format *format UNUSED,
> > + struct used_atom *atom,
> > + const char *arg, struct strbuf *err)
> > +{
> > + const char *describe_opts[] = {
> > + "",
> > + "tags",
> > + "abbrev",
> > + "match",
> > + "exclude",
> > + NULL
> > + };
> > +
> > + struct strvec args = STRVEC_INIT;
> > + for (;;) {
> > + int found = 0;
> > + const char *argval;
> > + size_t arglen = 0;
> > + int optval = 0;
> > + int opt;
> > +
> > + if (!arg)
> > + break;
> > +
> > + for (opt = D_BARE; !found && describe_opts[opt]; opt++) {
> > + switch(opt) {
> > + case D_BARE:
> > + /*
> > + * Do nothing. This is the bare describe
> > + * atom and we already handle this above.
> > + */
> > + break;
> > + case D_TAGS:
> > + if (match_atom_bool_arg(arg, describe_opts[opt],
> > + &arg, &optval)) {
> > + if (!optval)
> > + strvec_pushf(&args, "--no-%s",
> > + describe_opts[opt]);
> > + else
> > + strvec_pushf(&args, "--%s",
> > + describe_opts[opt]);
> > + found = 1;
> > + }
>
> As match_atom_bool_arg() and ...
>
> > + break;
> > + case D_ABBREV:
> > + if (match_atom_arg_value(arg, describe_opts[opt],
> > + &arg, &argval, &arglen)) {
> > + char *endptr;
> > + int ret = 0;
> > +
> > + if (!arglen)
> > + ret = -1;
> > + if (strtol(argval, &endptr, 10) < 0)
> > + ret = -1;
> > + if (endptr - argval != arglen)
> > + ret = -1;
> > +
> > + if (ret)
> > + return strbuf_addf_ret(err, ret,
> > + _("positive value expected describe:abbrev=%s"), argval);
> > + strvec_pushf(&args, "--%s=%.*s",
> > + describe_opts[opt],
> > + (int)arglen, argval);
> > + found = 1;
> > + }
>
> ... match_atom_arg_value() are both silent when they return false,
> we do not see any diagnosis when these two case arms set the "found"
> flag. Shouldn't we have a corresponding "else" clause to these "if
> (match_atom_blah())" blocks to issue an error message or something?
Yeah, I'll add this.
> [...]
> Now, is the code from here ...
>
> > + if (deref)
> > + name++;
> > +
> > + if (!skip_prefix(name, "describe", &name) ||
> > + (*name && *name != ':'))
> > + continue;
> > + if (!*name)
> > + name = NULL;
> > + else
> > + name++;
>
> ... down to here doing anything useful? After all, you already have
> all you need to describe the commit in atom->u.describe_args to run
> "git describe" with, no? In fact, after computing "name" with the
> above code with some complexity, nobody even looks at it.
>
> Perhaps the above was copied from some other grab_* functions; the
> reason why they were relevant there needs to be understood, and it
> also has to be considered if the same reason to have the code here
> applies to this codepath.
Sorry you had to read through this. I'll remove these if constructs,
because as you said, they do nothing since we already parse everything
we need and also check for the type and the deref.
There is not test for "%(*describe)", but I'll add one in v3 if you
think it is necessary (if we are doing this, should we also do one for
multiple options?).
Thanks
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/3] ref-filter: add new "describe" atom
2023-07-15 18:24 ` Kousik Sanagavarapu
@ 2023-07-15 18:56 ` Junio C Hamano
0 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2023-07-15 18:56 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> conveys it better or is it too much unnecessary stuff to and should we
> just do
>
> struct {
> const char **describe_args;
> } describe;
>
> leaving the describe_opts array as is and changing the how the switch is
> written.
I think this struct can be replaced with a single
const char **describe_args;
and then
>> > +static int describe_atom_parser(struct ref_format *format UNUSED,
>> > + struct used_atom *atom,
>> > + const char *arg, struct strbuf *err)
>> > +{
>> > + const char *describe_opts[] = {
>> > + "",
>> > + "tags",
>> > + "abbrev",
>> > + "match",
>> > + "exclude",
>> > + NULL
>> > + };
this array can simply go away. Then you can
>> > + struct strvec args = STRVEC_INIT;
>> > + for (;;) {
>> > + int found = 0;
>> > + const char *argval;
>> > + size_t arglen = 0;
>> > + int optval = 0;
>> > + int opt;
>> > +
>> > + if (!arg)
>> > + break;
>> > +
>> > + for (opt = D_BARE; !found && describe_opts[opt]; opt++) {
rewrite this "for" loop plus the "switch" inside to an if/else
if/else cascade:
if (match_atom_bool_arg(arg, "tags", &arg, &optval)) {
... do "tags" thing ...
} else if (match_atom_arg_value(arg, "abbrev", ...)) {
... do "abbrev" thing ...
} else if ...
That way, you do not need any enum anywhere and there is no reason
to have desribe_opts[] array, either.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 0/2] Add new "describe" atom
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
` (2 preceding siblings ...)
2023-07-14 19:20 ` [PATCH v2 3/3] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
@ 2023-07-19 16:15 ` Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
` (3 more replies)
3 siblings, 4 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-19 16:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder, Hariom Verma, Kousik Sanagavarapu
Hi,
Thanks Junio for the review on the previous version of this series. I
have made the changes according to the comments left.
PATCH 1/2 - Left unchanged expect for small changes in the commit
message for more clarity.
PATCH 2/2 - We now parse the arguments in a seperate function
`describe_atom_option_parser()` call this in
`describe_atom_parser()` instead to populate
`atom->u.describe_args`. This splitting of the function
helps err at the right places.
I've also squashed the third commit in the previous version
into this commit.
Kousik Sanagavarapu (2):
ref-filter: add multiple-option parsing functions
ref-filter: add new "describe" atom
Documentation/git-for-each-ref.txt | 23 ++++
ref-filter.c | 189 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 114 +++++++++++++++++
3 files changed, 326 insertions(+)
Range-diff against v2:
1: 50497067a3 ! 1: 08f3be1631 ref-filter: add multiple-option parsing functions
@@ Commit message
match_placeholder_arg_value()
match_placeholder_bool_arg()
- were added in pretty (4f732e0fd7 (pretty: allow %(trailers) options
- with explicit value, 2019-01-29)) to parse multiple options in an
+ were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options
+ with explicit value, 2019-01-29) to parse multiple options in an
argument to --pretty. For example,
git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )"
will output all the trailers matching the key and seperates them by
- commas per commit.
+ a comma followed by a space per commit.
Add similar functions,
match_atom_arg_value()
match_atom_bool_arg()
- in ref-filter. A particular use of this can be seen in the subsequent
- commit where we parse the options given to a new atom "describe".
+ in ref-filter.
+
+ There is no atom yet that can use these functions in ref-filter, but we
+ are going to add a new %(describe) atom in a subsequent commit where we
+ parse options like tags=<bool-value> or match=<pattern> given to it.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
2: f6f882884c ! 2: 742a79113c ref-filter: add new "describe" atom
@@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>::
## ref-filter.c ##
@@
- #include "alloc.h"
+ #include "git-compat-util.h"
#include "environment.h"
#include "gettext.h"
+#include "config.h"
@@ ref-filter.c: enum atom_type {
ATOM_BODY,
ATOM_TRAILERS,
@@ ref-filter.c: static struct used_atom {
- struct email_option {
- enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
- } email_option;
-+ struct {
-+ enum { D_BARE, D_TAGS, D_ABBREV,
-+ D_EXCLUDE, D_MATCH } option;
-+ const char **args;
-+ } describe;
+ enum { S_BARE, S_GRADE, S_SIGNER, S_KEY,
+ S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;
+ } signature;
++ const char **describe_args;
struct refname_atom refname;
char *head;
} u;
@@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct
return 0;
}
++static int describe_atom_option_parser(struct strvec *args, const char **arg,
++ struct strbuf *err)
++{
++ const char *argval;
++ size_t arglen = 0;
++ int optval = 0;
++
++ if (match_atom_bool_arg(*arg, "tags", arg, &optval)) {
++ if (!optval)
++ strvec_push(args, "--no-tags");
++ else
++ strvec_push(args, "--tags");
++ return 1;
++ }
++
++ if (match_atom_arg_value(*arg, "abbrev", arg, &argval, &arglen)) {
++ char *endptr;
++
++ if (!arglen)
++ return strbuf_addf_ret(err, -1,
++ _("argument expected for %s"),
++ "describe:abbrev");
++ if (strtol(argval, &endptr, 10) < 0)
++ return strbuf_addf_ret(err, -1,
++ _("positive value expected %s=%s"),
++ "describe:abbrev", argval);
++ if (endptr - argval != arglen)
++ return strbuf_addf_ret(err, -1,
++ _("cannot fully parse %s=%s"),
++ "describe:abbrev", argval);
++
++ strvec_pushf(args, "--abbrev=%.*s", (int)arglen, argval);
++ return 1;
++ }
++
++ if (match_atom_arg_value(*arg, "match", arg, &argval, &arglen)) {
++ if (!arglen)
++ return strbuf_addf_ret(err, -1,
++ _("value expected %s="),
++ "describe:match");
++
++ strvec_pushf(args, "--match=%.*s", (int)arglen, argval);
++ return 1;
++ }
++
++ if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) {
++ if (!arglen)
++ return strbuf_addf_ret(err, -1,
++ _("value expected %s="),
++ "describe:exclude");
++
++ strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval);
++ return 1;
++ }
++
++ return 0;
++}
++
+static int describe_atom_parser(struct ref_format *format UNUSED,
+ struct used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
-+ const char *describe_opts[] = {
-+ "",
-+ "tags",
-+ "abbrev",
-+ "match",
-+ "exclude",
-+ NULL
-+ };
-+
+ struct strvec args = STRVEC_INIT;
++
+ for (;;) {
+ int found = 0;
-+ const char *argval;
-+ size_t arglen = 0;
-+ int optval = 0;
-+ int opt;
++ const char *bad_arg = NULL;
+
-+ if (!arg)
++ if (!arg || !*arg)
+ break;
+
-+ for (opt = D_BARE; !found && describe_opts[opt]; opt++) {
-+ switch(opt) {
-+ case D_BARE:
-+ /*
-+ * Do nothing. This is the bare describe
-+ * atom and we already handle this above.
-+ */
-+ break;
-+ case D_TAGS:
-+ if (match_atom_bool_arg(arg, describe_opts[opt],
-+ &arg, &optval)) {
-+ if (!optval)
-+ strvec_pushf(&args, "--no-%s",
-+ describe_opts[opt]);
-+ else
-+ strvec_pushf(&args, "--%s",
-+ describe_opts[opt]);
-+ found = 1;
-+ }
-+ break;
-+ case D_ABBREV:
-+ if (match_atom_arg_value(arg, describe_opts[opt],
-+ &arg, &argval, &arglen)) {
-+ char *endptr;
-+ int ret = 0;
-+
-+ if (!arglen)
-+ ret = -1;
-+ if (strtol(argval, &endptr, 10) < 0)
-+ ret = -1;
-+ if (endptr - argval != arglen)
-+ ret = -1;
-+
-+ if (ret)
-+ return strbuf_addf_ret(err, ret,
-+ _("positive value expected describe:abbrev=%s"), argval);
-+ strvec_pushf(&args, "--%s=%.*s",
-+ describe_opts[opt],
-+ (int)arglen, argval);
-+ found = 1;
-+ }
-+ break;
-+ case D_MATCH:
-+ case D_EXCLUDE:
-+ if (match_atom_arg_value(arg, describe_opts[opt],
-+ &arg, &argval, &arglen)) {
-+ if (!arglen)
-+ return strbuf_addf_ret(err, -1,
-+ _("value expected describe:%s="), describe_opts[opt]);
-+ strvec_pushf(&args, "--%s=%.*s",
-+ describe_opts[opt],
-+ (int)arglen, argval);
-+ found = 1;
-+ }
-+ break;
-+ }
-+ }
-+ if (!found)
++ bad_arg = arg;
++ found = describe_atom_option_parser(&args, &arg, err);
++ if (found < 0)
++ return found;
++ if (!found) {
++ if (bad_arg && *bad_arg)
++ return err_bad_arg(err, "describe", bad_arg);
+ break;
++ }
+ }
-+ atom->u.describe.args = strvec_detach(&args);
++ atom->u.describe_args = strvec_detach(&args);
+ return 0;
+}
+
@@ ref-filter.c: static void append_lines(struct strbuf *out, const char *buf, unsi
+
+ if (!!deref != (*name == '*'))
+ continue;
-+ if (deref)
-+ name++;
-+
-+ if (!skip_prefix(name, "describe", &name) ||
-+ (*name && *name != ':'))
-+ continue;
-+ if (!*name)
-+ name = NULL;
-+ else
-+ name++;
+
+ cmd.git_cmd = 1;
+ strvec_push(&cmd.args, "describe");
-+ strvec_pushv(&cmd.args, atom->u.describe.args);
++ strvec_pushv(&cmd.args, atom->u.describe_args);
+ strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+ if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+ error(_("failed to run 'describe'"));
@@ ref-filter.c: static void grab_values(struct atom_value *val, int deref, struct
break;
case OBJ_COMMIT:
grab_commit_values(val, deref, obj);
- grab_sub_body_contents(val, deref, data);
+@@ ref-filter.c: static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_person("author", val, deref, buf);
grab_person("committer", val, deref, buf);
+ grab_signature(val, deref, obj);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_TREE:
@@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override
test_cmp expected.bare actual
'
-+test_expect_success 'describe atom vs git describe' '
-+ test_when_finished "rm -rf describe-repo" &&
-+
++test_expect_success 'setup for describe atom tests' '
+ git init describe-repo &&
+ (
+ cd describe-repo &&
@@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override
+ git tag tagone &&
+
+ test_commit --no-tag two &&
-+ git tag -a -m "tag two" tagtwo &&
++ git tag -a -m "tag two" tagtwo
++ )
++'
+
-+ git for-each-ref refs/tags/ --format="%(objectname)" >obj &&
++test_expect_success 'describe atom vs git describe' '
++ (
++ cd describe-repo &&
++
++ git for-each-ref --format="%(objectname)" \
++ refs/tags/ >obj &&
+ while read hash
+ do
+ if desc=$(git describe $hash)
@@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override
+'
+
+test_expect_success 'describe:tags vs describe --tags' '
-+ test_when_finished "git tag -d tagname" &&
-+ git tag tagname &&
-+ git describe --tags >expect &&
-+ git for-each-ref --format="%(describe:tags)" refs/heads/ >actual &&
-+ test_cmp expect actual
++ (
++ cd describe-repo &&
++ git describe --tags >expect &&
++ git for-each-ref --format="%(describe:tags)" \
++ refs/heads/master >actual &&
++ test_cmp expect actual
++ )
+'
+
+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '
-+ test_when_finished "git tag -d tagname" &&
-+
-+ # Case 1: We have commits between HEAD and the most
-+ # recent tag reachable from it
-+ test_commit --no-tag file &&
-+ git describe --abbrev=14 >expect &&
-+ git for-each-ref --format="%(describe:abbrev=14)" \
-+ refs/heads/ >actual &&
-+ test_cmp expect actual &&
-+
-+ # Make sure the hash used is atleast 14 digits long
-+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
-+ test 15 -le $(wc -c <hexpart) &&
-+
-+ # Case 2: We have a tag at HEAD, describe directly gives
-+ # the name of the tag
-+ git tag -a -m tagged tagname &&
-+ git describe --abbrev=14 >expect &&
-+ git for-each-ref --format="%(describe:abbrev=14)" \
-+ refs/heads/ >actual &&
-+ test_cmp expect actual &&
-+ test tagname = $(cat actual)
++ (
++ cd describe-repo &&
++
++ # Case 1: We have commits between HEAD and the most
++ # recent tag reachable from it
++ test_commit --no-tag file &&
++ git describe --abbrev=14 >expect &&
++ git for-each-ref --format="%(describe:abbrev=14)" \
++ refs/heads/master >actual &&
++ test_cmp expect actual &&
++
++ # Make sure the hash used is atleast 14 digits long
++ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
++ test 15 -le $(wc -c <hexpart) &&
++
++ # Case 2: We have a tag at HEAD, describe directly gives
++ # the name of the tag
++ git tag -a -m tagged tagname &&
++ git describe --abbrev=14 >expect &&
++ git for-each-ref --format="%(describe:abbrev=14)" \
++ refs/heads/master >actual &&
++ test_cmp expect actual &&
++ test tagname = $(cat actual)
++ )
+'
+
+test_expect_success 'describe:match=... vs describe --match ...' '
-+ test_when_finished "git tag -d tag-match" &&
-+ git tag -a -m "tag match" tag-match &&
-+ git describe --match "*-match" >expect &&
-+ git for-each-ref --format="%(describe:match="*-match")" \
-+ refs/heads/ >actual &&
-+ test_cmp expect actual
++ (
++ cd describe-repo &&
++ git tag -a -m "tag foo" tag-foo &&
++ git describe --match "*-foo" >expect &&
++ git for-each-ref --format="%(describe:match="*-foo")" \
++ refs/heads/master >actual &&
++ test_cmp expect actual
++ )
+'
+
+test_expect_success 'describe:exclude:... vs describe --exclude ...' '
-+ test_when_finished "git tag -d tag-exclude" &&
-+ git tag -a -m "tag exclude" tag-exclude &&
-+ git describe --exclude "*-exclude" >expect &&
-+ git for-each-ref --format="%(describe:exclude="*-exclude")" \
-+ refs/heads/ >actual &&
-+ test_cmp expect actual
++ (
++ cd describe-repo &&
++ git tag -a -m "tag bar" tag-bar &&
++ git describe --exclude "*-bar" >expect &&
++ git for-each-ref --format="%(describe:exclude="*-bar")" \
++ refs/heads/master >actual &&
++ test_cmp expect actual
++ )
++'
++
++test_expect_success 'deref with describe atom' '
++ (
++ cd describe-repo &&
++ cat >expect <<-\EOF &&
++
++ tagname
++ tagname
++ tagname
++
++ tagtwo
++ EOF
++ git for-each-ref --format="%(*describe)" >actual &&
++ test_cmp expect actual
++ )
+'
+
cat >expected <<\EOF
3: a5122bf5e2 < -: ---------- t6300: run describe atom tests on a different repo
--
2.41.0.378.g42703afc1f.dirty
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
@ 2023-07-19 16:15 ` Kousik Sanagavarapu
2023-07-19 23:23 ` Junio C Hamano
2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
` (2 subsequent siblings)
3 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-19 16:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder, Hariom Verma, Kousik Sanagavarapu
The functions
match_placeholder_arg_value()
match_placeholder_bool_arg()
were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options
with explicit value, 2019-01-29) to parse multiple options in an
argument to --pretty. For example,
git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )"
will output all the trailers matching the key and seperates them by
a comma followed by a space per commit.
Add similar functions,
match_atom_arg_value()
match_atom_bool_arg()
in ref-filter.
There is no atom yet that can use these functions in ref-filter, but we
are going to add a new %(describe) atom in a subsequent commit where we
parse options like tags=<bool-value> or match=<pattern> given to it.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index 60919f375f..f64437e781 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -255,6 +255,65 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)
return -1;
}
+static int match_atom_arg_value(const char *to_parse, const char *candidate,
+ const char **end, const char **valuestart,
+ size_t *valuelen)
+{
+ const char *atom;
+
+ if (!(skip_prefix(to_parse, candidate, &atom)))
+ return 0;
+ if (valuestart) {
+ if (*atom == '=') {
+ *valuestart = atom + 1;
+ *valuelen = strcspn(*valuestart, ",\0");
+ atom = *valuestart + *valuelen;
+ } else {
+ if (*atom != ',' && *atom != '\0')
+ return 0;
+ *valuestart = NULL;
+ *valuelen = 0;
+ }
+ }
+ if (*atom == ',') {
+ *end = atom + 1;
+ return 1;
+ }
+ if (*atom == '\0') {
+ *end = atom;
+ return 1;
+ }
+ return 0;
+}
+
+static int match_atom_bool_arg(const char *to_parse, const char *candidate,
+ const char **end, int *val)
+{
+ const char *argval;
+ char *strval;
+ size_t arglen;
+ int v;
+
+ if (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen))
+ return 0;
+
+ if (!argval) {
+ *val = 1;
+ return 1;
+ }
+
+ strval = xstrndup(argval, arglen);
+ v = git_parse_maybe_bool(strval);
+ free(strval);
+
+ if (v == -1)
+ return 0;
+
+ *val = v;
+
+ return 1;
+}
+
static int color_atom_parser(struct ref_format *format, struct used_atom *atom,
const char *color_value, struct strbuf *err)
{
--
2.41.0.378.g42703afc1f.dirty
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 2/2] ref-filter: add new "describe" atom
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
@ 2023-07-19 16:15 ` Kousik Sanagavarapu
2023-07-19 22:56 ` Junio C Hamano
2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano
2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu
3 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-19 16:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Christian Couder, Hariom Verma, Kousik Sanagavarapu
Duplicate the logic of %(describe) and friends from pretty to
ref-filter. In the future, this change helps in unifying both the
formats as ref-filter will be able to do everything that pretty is doing
and we can have a single interface.
The new atom "describe" and its friends are equivalent to the existing
pretty formats with the same name.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
Documentation/git-for-each-ref.txt | 23 +++++
ref-filter.c | 130 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 114 +++++++++++++++++++++++++
3 files changed, 267 insertions(+)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2e0318770b..395daf1b22 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -258,6 +258,29 @@ ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the output
ref to the `<committish>` specified in the format.
+describe[:options]:: Human-readable name, like
+ link-git:git-describe[1]; empty string for
+ undescribable commits. The `describe` string may be
+ followed by a colon and zero or more comma-separated
+ options. Descriptions can be inconsistent when tags
+ are added or removed at the same time.
++
+--
+tags=<bool-value>;; Instead of only considering annotated tags, consider
+ lightweight tags as well; see the corresponding option
+ in linkgit:git-describe[1] for details.
+abbrev=<number>;; Use at least <number> hexadecimal digits; see
+ the corresponding option in linkgit:git-describe[1]
+ for details.
+match=<pattern>;; Only consider tags matching the given `glob(7)` pattern,
+ excluding the "refs/tags/" prefix; see the corresponding
+ option in linkgit:git-describe[1] for details.
+exclude=<pattern>;; Do not consider tags matching the given `glob(7)`
+ pattern, excluding the "refs/tags/" prefix; see the
+ corresponding option in linkgit:git-describe[1] for
+ details.
+--
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index f64437e781..43316643be 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1,9 +1,11 @@
#include "git-compat-util.h"
#include "environment.h"
#include "gettext.h"
+#include "config.h"
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
+#include "run-command.h"
#include "refs.h"
#include "wildmatch.h"
#include "object-name.h"
@@ -145,6 +147,7 @@ enum atom_type {
ATOM_TAGGERDATE,
ATOM_CREATOR,
ATOM_CREATORDATE,
+ ATOM_DESCRIBE,
ATOM_SUBJECT,
ATOM_BODY,
ATOM_TRAILERS,
@@ -219,6 +222,7 @@ static struct used_atom {
enum { S_BARE, S_GRADE, S_SIGNER, S_KEY,
S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;
} signature;
+ const char **describe_args;
struct refname_atom refname;
char *head;
} u;
@@ -554,6 +558,91 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
return 0;
}
+static int describe_atom_option_parser(struct strvec *args, const char **arg,
+ struct strbuf *err)
+{
+ const char *argval;
+ size_t arglen = 0;
+ int optval = 0;
+
+ if (match_atom_bool_arg(*arg, "tags", arg, &optval)) {
+ if (!optval)
+ strvec_push(args, "--no-tags");
+ else
+ strvec_push(args, "--tags");
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "abbrev", arg, &argval, &arglen)) {
+ char *endptr;
+
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("argument expected for %s"),
+ "describe:abbrev");
+ if (strtol(argval, &endptr, 10) < 0)
+ return strbuf_addf_ret(err, -1,
+ _("positive value expected %s=%s"),
+ "describe:abbrev", argval);
+ if (endptr - argval != arglen)
+ return strbuf_addf_ret(err, -1,
+ _("cannot fully parse %s=%s"),
+ "describe:abbrev", argval);
+
+ strvec_pushf(args, "--abbrev=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "match", arg, &argval, &arglen)) {
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("value expected %s="),
+ "describe:match");
+
+ strvec_pushf(args, "--match=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) {
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("value expected %s="),
+ "describe:exclude");
+
+ strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ return 0;
+}
+
+static int describe_atom_parser(struct ref_format *format UNUSED,
+ struct used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
+ struct strvec args = STRVEC_INIT;
+
+ for (;;) {
+ int found = 0;
+ const char *bad_arg = NULL;
+
+ if (!arg || !*arg)
+ break;
+
+ bad_arg = arg;
+ found = describe_atom_option_parser(&args, &arg, err);
+ if (found < 0)
+ return found;
+ if (!found) {
+ if (bad_arg && *bad_arg)
+ return err_bad_arg(err, "describe", bad_arg);
+ break;
+ }
+ }
+ atom->u.describe_args = strvec_detach(&args);
+ return 0;
+}
+
static int raw_atom_parser(struct ref_format *format UNUSED,
struct used_atom *atom,
const char *arg, struct strbuf *err)
@@ -756,6 +845,7 @@ static struct {
[ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
[ATOM_CREATOR] = { "creator", SOURCE_OBJ },
[ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
+ [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
[ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
@@ -1662,6 +1752,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
}
}
+static void grab_describe_values(struct atom_value *val, int deref,
+ struct object *obj)
+{
+ struct commit *commit = (struct commit *)obj;
+ int i;
+
+ for (i = 0; i < used_atom_cnt; i++) {
+ struct used_atom *atom = &used_atom[i];
+ enum atom_type type = atom->atom_type;
+ const char *name = atom->name;
+ struct atom_value *v = &val[i];
+
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+
+ if (type != ATOM_DESCRIBE)
+ continue;
+
+ if (!!deref != (*name == '*'))
+ continue;
+
+ cmd.git_cmd = 1;
+ strvec_push(&cmd.args, "describe");
+ strvec_pushv(&cmd.args, atom->u.describe_args);
+ strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+ if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+ error(_("failed to run 'describe'"));
+ v->s = xstrdup("");
+ continue;
+ }
+ strbuf_rtrim(&out);
+ v->s = strbuf_detach(&out, NULL);
+
+ strbuf_release(&err);
+ }
+}
+
/* See grab_values */
static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)
{
@@ -1771,6 +1899,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_tag_values(val, deref, obj);
grab_sub_body_contents(val, deref, data);
grab_person("tagger", val, deref, buf);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_COMMIT:
grab_commit_values(val, deref, obj);
@@ -1778,6 +1907,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_person("author", val, deref, buf);
grab_person("committer", val, deref, buf);
grab_signature(val, deref, obj);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_TREE:
/* grab_tree_values(val, deref, obj, buf, sz); */
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6e6ec852b5..4bbba76874 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -562,6 +562,120 @@ test_expect_success 'color.ui=always does not override tty check' '
test_cmp expected.bare actual
'
+test_expect_success 'setup for describe atom tests' '
+ git init describe-repo &&
+ (
+ cd describe-repo &&
+
+ test_commit --no-tag one &&
+ git tag tagone &&
+
+ test_commit --no-tag two &&
+ git tag -a -m "tag two" tagtwo
+ )
+'
+
+test_expect_success 'describe atom vs git describe' '
+ (
+ cd describe-repo &&
+
+ git for-each-ref --format="%(objectname)" \
+ refs/tags/ >obj &&
+ while read hash
+ do
+ if desc=$(git describe $hash)
+ then
+ : >expect-contains-good
+ else
+ : >expect-contains-bad
+ fi &&
+ echo "$hash $desc" || return 1
+ done <obj >expect &&
+ test_path_exists expect-contains-good &&
+ test_path_exists expect-contains-bad &&
+
+ git for-each-ref --format="%(objectname) %(describe)" \
+ refs/tags/ >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+ )
+'
+
+test_expect_success 'describe:tags vs describe --tags' '
+ (
+ cd describe-repo &&
+ git describe --tags >expect &&
+ git for-each-ref --format="%(describe:tags)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '
+ (
+ cd describe-repo &&
+
+ # Case 1: We have commits between HEAD and the most
+ # recent tag reachable from it
+ test_commit --no-tag file &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual &&
+
+ # Make sure the hash used is atleast 14 digits long
+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+ test 15 -le $(wc -c <hexpart) &&
+
+ # Case 2: We have a tag at HEAD, describe directly gives
+ # the name of the tag
+ git tag -a -m tagged tagname &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual &&
+ test tagname = $(cat actual)
+ )
+'
+
+test_expect_success 'describe:match=... vs describe --match ...' '
+ (
+ cd describe-repo &&
+ git tag -a -m "tag foo" tag-foo &&
+ git describe --match "*-foo" >expect &&
+ git for-each-ref --format="%(describe:match="*-foo")" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'describe:exclude:... vs describe --exclude ...' '
+ (
+ cd describe-repo &&
+ git tag -a -m "tag bar" tag-bar &&
+ git describe --exclude "*-bar" >expect &&
+ git for-each-ref --format="%(describe:exclude="*-bar")" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'deref with describe atom' '
+ (
+ cd describe-repo &&
+ cat >expect <<-\EOF &&
+
+ tagname
+ tagname
+ tagname
+
+ tagtwo
+ EOF
+ git for-each-ref --format="%(*describe)" >actual &&
+ test_cmp expect actual
+ )
+'
+
cat >expected <<\EOF
heads/main
tags/main
--
2.41.0.378.g42703afc1f.dirty
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 2/2] ref-filter: add new "describe" atom
2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
@ 2023-07-19 22:56 ` Junio C Hamano
0 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2023-07-19 22:56 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> Duplicate the logic of %(describe) and friends from pretty to
> ref-filter. In the future, this change helps in unifying both the
> formats as ref-filter will be able to do everything that pretty is doing
> and we can have a single interface.
>
> The new atom "describe" and its friends are equivalent to the existing
> pretty formats with the same name.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Hariom Verma <hariom18599@gmail.com>
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 23 +++++
> ref-filter.c | 130 +++++++++++++++++++++++++++++
> t/t6300-for-each-ref.sh | 114 +++++++++++++++++++++++++
> 3 files changed, 267 insertions(+)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 2e0318770b..395daf1b22 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -258,6 +258,29 @@ ahead-behind:<committish>::
> commits ahead and behind, respectively, when comparing the output
> ref to the `<committish>` specified in the format.
>
> +describe[:options]:: Human-readable name, like
> + link-git:git-describe[1]; empty string for
> + undescribable commits. The `describe` string may be
> + followed by a colon and zero or more comma-separated
> + options.
Why do these new items formatted so differently from the previous
ones? By indenting the lines so deeply you are forcing yourself to
wrap these lines many times. How about imitating the previous entry
for ahead-behind and writing this like so:
describe[:<options>]::
A human-readable name, like linkgit:git-describe[1];
empty string is given for an undescribable commit.
...
By the way, there is a typo "link-git" above that needs to be
corrected.
It is curious that we support "describe:" (i.e. having no options,
but colon is still present). It may not be wrong per se, but it
looks strange. "may be followed by a colon and one or more
comma-separated options" would be more intuitive (I haven't seen the
implementation yet, so if we go that route, the implementation may
also need to be updated).
> .... Descriptions can be inconsistent when tags
> + are added or removed at the same time.
"at the same time" meaning "while the description are being
computed"? I think this was copied from 273c9901 (pretty: document
multiple %(describe) being inconsistent, 2021-02-28) where the
pretty placeholder for "git log" and friends are described, and the
implementation used there go one formatting element at a time,
unlike for-each-ref that can compute a description for a given ref
just once in populate_value() and reuse the same atom number of
times in the format, each instance giving exactly the same value.
So I am not sure if the "can be inconsistent" disclaimer applies
to the %(describe) on this side the same way. Are you sure?
As %(describe) is fairly expensive to compute, if the format string
wants two, e.g. --format="%(refname) %(describe) %(describe)", there
should be some effort to make these two share the same used_atom(),
so that there will be only one "git describe" invocation from
populate_value() that lets get_ref_atom_value() reuse that result
of a single invocation to fill the two placeholder.
> @@ -219,6 +222,7 @@ static struct used_atom {
> enum { S_BARE, S_GRADE, S_SIGNER, S_KEY,
> S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;
> } signature;
> + const char **describe_args;
> struct refname_atom refname;
> char *head;
> } u;
Nice and simple ;-).
> +static int describe_atom_option_parser(struct strvec *args, const char **arg,
> + struct strbuf *err)
> +{
> + const char *argval;
> + size_t arglen = 0;
> + int optval = 0;
> +
> + if (match_atom_bool_arg(*arg, "tags", arg, &optval)) {
> + if (!optval)
> + strvec_push(args, "--no-tags");
> + else
> + strvec_push(args, "--tags");
> + return 1;
> + }
OK. One thing that I hate about the split of this series into two
steps is that [1/2] has to be read without knowing what the expected
use of those two helper functions are. It was especially bad as the
functions lacked any documentation on how they are supposed to be
called.
Now, if we go back to the implementation of match_atom_bool_arg(),
it first called match_atom_arg_value(), which stripped the given key
("tags" in this case) from the argument being parsed, and allowed
'=' (i.e. followed by a val), ',' (i.e. no val, but more "key[=val]"
to follow), or '\0' (i.e. end of the argument string). Anything
else after the matched key meant that the key did not match
(e.g. the arg had "tagsabcd", which should not match "tag"). And it
stored the byte position after '=' if the key was terminated with
'=', or NULL otherwise, to signal where the optional value starts.
match_atom_bool_arg() uses this and correctly translates a key
without the optional [=val] part into "true". So the above is
given, after the caller skips "%(describe:", things like "tags,...",
"tags=no,...", "tags=yes,..." (or replace ",..." with a NUL for the
final option), and chooses between --no-tags and --tags. Sounds
good.
> + ...
> + if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) {
> + if (!arglen)
> + return strbuf_addf_ret(err, -1,
> + _("value expected %s="),
> + "describe:exclude");
> +
> + strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval);
> + return 1;
> + }
I would have expected that these become if/else if/.../else cascade,
i.e.
if (is that "tags"?) {
} else if (is that "abbrev"?) {
...
} else
return 0; /* nothing matched */
return 1;
but I do not mind the above. Each "block" that matches and handles
one key looks more indenendent the way the patch was written, which
may be a good thing.
> + return 0;
> +}
> +
> +static int describe_atom_parser(struct ref_format *format UNUSED,
> + struct used_atom *atom,
> + const char *arg, struct strbuf *err)
> +{
> + struct strvec args = STRVEC_INIT;
OK, parse_ref_fitler_atom() saw "%(describe", possibly followed by a
colon and zero or more comma-separated key[=val], and the location
after ':' (or NULL) is given to arg. Specifically, %(describe) and
%(describe:) both pass NULL in arg.
> + for (;;) {
> + int found = 0;
> + const char *bad_arg = NULL;
> +
> + if (!arg || !*arg)
> + break;
And we stop when there is no more key[=val].
> + bad_arg = arg;
> + found = describe_atom_option_parser(&args, &arg, err);
This one moves arg forward and arranges the next key[=val] to be seen
in the next iteration of this loop. Makes sense.
In the remainder of the code changes, I saw nothing strange.
Quite well made.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
@ 2023-07-19 23:23 ` Junio C Hamano
2023-07-20 5:21 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 38+ messages in thread
From: Junio C Hamano @ 2023-07-19 23:23 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Christian Couder, Hariom Verma, Josh Steadmon,
Siddharth Singh, Glen Choo
Kousik Sanagavarapu <five231003@gmail.com> writes:
> ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
New helper functions that do not have any caller and no
documentation to explain how they are supposed to be called
(i.e. the expectation on the callers---what values they need to feed
as parameters when they call these helpers, and the expectation by
the callers---what they expect to get out of the helpers once they
return) makes it impossible to evaluate if they are any good [*].
Side note. Those of you who are keen to add unit tests to
the system (Cc:ed) , do you think a patch line this one that
adds a new helper function to the system, would benefit from
being able to add a few unit tests for these otherwise
unused helper functions?
The calls to the new functions that the unit test framework
would make should serve as a good piece of interface
documentation, showing what the callers are supposed to pass
and what they expect, I guess.
So whatever framework we choose, it should allow adding a
test or two to this patch easily, without being too
intrusive. Would that be a good and concrete evaluation
criterion?
Anyway, because of that, I had to read [2/2] first and then come
back here to review this one.
The following is my attempt to write down the contract between the
callers and this new helper function---please give something like
that to the final version. The the example below is there just to
illustrate the level of information that would be desired to help
future readers and programmers. Do not take the contents as-written
as truth---I may have (deliberately) mixed in incorrect descriptions
;-).
/*
* The string "to_parse" is expected to be a comma-separated list
* of "key" or "key=val". If your atom allows "key1" and "key2"
* (possibly with their values) as options, make two calls to this
* funtion, passing "key1" in candiate and then passing "key2" in
* candidate.
*
* The function Returns true ONLY when the to_parse string begins
* with the candidate key, possibly followed by its value (valueless
* key-only entries are allowed in the comman-separated list).
* Otherwise, *end, *valuestart and *valuelen are LEFT INTACT and
* the function returns false.
*
* *valuestart will point at the byte after '=' (i.e. the beginning
* of the value), and the number of bytes in the value will be set
* to *valuelen.
* A key-only entry results in *valuestart set to NULL and *valuelen
* set to 0.
* *end will point at the next key[=val] in the comma-separated list
* or NULL when the list ran out.
*/
> +static int match_atom_arg_value(const char *to_parse, const char *candidate,
> + const char **end, const char **valuestart,
> + size_t *valuelen)
> +{
> + const char *atom;
> +
> + if (!(skip_prefix(to_parse, candidate, &atom)))
> + return 0;
> + if (valuestart) {
As far as I saw, no callers pass NULL to valuestart. Getting rid of
this if() statement and always entering its body would clarify what
is going on, I think.
> + if (*atom == '=') {
> + *valuestart = atom + 1;
> + *valuelen = strcspn(*valuestart, ",\0");
> + atom = *valuestart + *valuelen;
> + } else {
> + if (*atom != ',' && *atom != '\0')
> + return 0;
> + *valuestart = NULL;
> + *valuelen = 0;
> + }
> + }
> + if (*atom == ',') {
> + *end = atom + 1;
> + return 1;
> + }
> + if (*atom == '\0') {
> + *end = atom;
> + return 1;
> + }
> + return 0;
> +}
/*
* Write something similar to document the contract between the caller
* and this function here.
*/
> +static int match_atom_bool_arg(const char *to_parse, const char *candidate,
> + const char **end, int *val)
> +{
Thanks.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
2023-07-19 23:23 ` Junio C Hamano
@ 2023-07-20 5:21 ` Junio C Hamano
2023-07-20 16:52 ` Kousik Sanagavarapu
2023-07-20 17:42 ` Glen Choo
2 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2023-07-20 5:21 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Christian Couder, Hariom Verma, Josh Steadmon,
Siddharth Singh, Glen Choo
Junio C Hamano <gitster@pobox.com> writes:
>> +static int match_atom_arg_value(const char *to_parse, const char *candidate,
>> + const char **end, const char **valuestart,
>> + size_t *valuelen)
>> +{
>> + const char *atom;
>> +
>> + if (!(skip_prefix(to_parse, candidate, &atom)))
>> + return 0;
>> + if (valuestart) {
>
> As far as I saw, no callers pass NULL to valuestart. Getting rid of
> this if() statement and always entering its body would clarify what
> is going on, I think.
Specifically, ...
>> + if (*atom == '=') {
>> + *valuestart = atom + 1;
>> + *valuelen = strcspn(*valuestart, ",\0");
>> + atom = *valuestart + *valuelen;
>> + } else {
>> + if (*atom != ',' && *atom != '\0')
>> + return 0;
>> + *valuestart = NULL;
>> + *valuelen = 0;
>> + }
>> + }
>> + if (*atom == ',') {
>> + *end = atom + 1;
>> + return 1;
>> + }
>> + if (*atom == '\0') {
>> + *end = atom;
>> + return 1;
>> + }
>> + return 0;
>> +}
... I think the body of the function would become easier to read if
written like so:
if (!skip_prefix(to_parse, candidate, &atom))
return 0; /* definitely not "candidate" */
if (*atom == '=') {
/* we just saw "candidate=" */
*valuestart = atom + 1;
atom = strchrnul(*valuestart, ',');
*valuelen = atom - *valuestart;
} else if (*atom != ',' && *atom != '\0') {
/* key begins with "candidate" but has more chars */
return 0;
} else {
/* just "candidate" without "=val" */
*valuestart = NULL;
*valuelen = 0;
}
/* atom points at either the ',' or NUL after this key[=val] */
if (*atom == ',')
atom++;
else if (*atom)
BUG("should not happen");
*end = atom;
return 1;
as it is clear that *valuestart, *valuelen, and *end are not touched
when the function returns 0 and they are all filled when the function
returns 1.
Also, avoid passing ",\0" to strcspn(); its effect is exactly the
same as passing ",", and at that point you are better off using
strchnul().
Thanks.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
2023-07-19 23:23 ` Junio C Hamano
2023-07-20 5:21 ` Junio C Hamano
@ 2023-07-20 16:52 ` Kousik Sanagavarapu
2023-07-20 17:59 ` Junio C Hamano
2023-07-20 17:42 ` Glen Choo
2 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-20 16:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Christian Couder, Hariom Verma, Josh Steadmon,
Siddharth Singh, Glen Choo
On Wed, Jul 19, 2023 at 04:23:15PM -0700, Junio C Hamano wrote:
> Kousik Sanagavarapu <five231003@gmail.com> writes:
>
> > ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 59 insertions(+)
>
> New helper functions that do not have any caller and no
> documentation to explain how they are supposed to be called
> (i.e. the expectation on the callers---what values they need to feed
> as parameters when they call these helpers, and the expectation by
> the callers---what they expect to get out of the helpers once they
> return) makes it impossible to evaluate if they are any good [*].
>
> Side note. Those of you who are keen to add unit tests to
> the system (Cc:ed) , do you think a patch line this one that
> adds a new helper function to the system, would benefit from
> being able to add a few unit tests for these otherwise
> unused helper functions?
>
> The calls to the new functions that the unit test framework
> would make should serve as a good piece of interface
> documentation, showing what the callers are supposed to pass
> and what they expect, I guess.
>
> So whatever framework we choose, it should allow adding a
> test or two to this patch easily, without being too
> intrusive. Would that be a good and concrete evaluation
> criterion?
>
> Anyway, because of that, I had to read [2/2] first and then come
> back here to review this one.
>
> The following is my attempt to write down the contract between the
> callers and this new helper function---please give something like
> that to the final version. The the example below is there just to
> illustrate the level of information that would be desired to help
> future readers and programmers. Do not take the contents as-written
> as truth---I may have (deliberately) mixed in incorrect descriptions
> ;-).
I'll spot them---if there are any ;).
>
> /*
> * The string "to_parse" is expected to be a comma-separated list
> * of "key" or "key=val". If your atom allows "key1" and "key2"
> * (possibly with their values) as options, make two calls to this
> * funtion, passing "key1" in candiate and then passing "key2" in
> * candidate.
> *
> * The function Returns true ONLY when the to_parse string begins
> * with the candidate key, possibly followed by its value (valueless
> * key-only entries are allowed in the comman-separated list).
> * Otherwise, *end, *valuestart and *valuelen are LEFT INTACT and
> * the function returns false.
> *
> * *valuestart will point at the byte after '=' (i.e. the beginning
> * of the value), and the number of bytes in the value will be set
> * to *valuelen.
> * A key-only entry results in *valuestart set to NULL and *valuelen
> * set to 0.
> * *end will point at the next key[=val] in the comma-separated list
> * or NULL when the list ran out.
> */
>
> > +static int match_atom_arg_value(const char *to_parse, const char *candidate,
> > + const char **end, const char **valuestart,
> > + size_t *valuelen)
> > +{
> > + const char *atom;
> > +
> > + if (!(skip_prefix(to_parse, candidate, &atom)))
> > + return 0;
> > + if (valuestart) {
>
> As far as I saw, no callers pass NULL to valuestart. Getting rid of
> this if() statement and always entering its body would clarify what
> is going on, I think.
>
> > + if (*atom == '=') {
> > + *valuestart = atom + 1;
> > + *valuelen = strcspn(*valuestart, ",\0");
> > + atom = *valuestart + *valuelen;
> > + } else {
> > + if (*atom != ',' && *atom != '\0')
> > + return 0;
> > + *valuestart = NULL;
> > + *valuelen = 0;
> > + }
> > + }
> > + if (*atom == ',') {
> > + *end = atom + 1;
> > + return 1;
> > + }
> > + if (*atom == '\0') {
> > + *end = atom;
> > + return 1;
> > + }
> > + return 0;
> > +}
>
> /*
> * Write something similar to document the contract between the caller
> * and this function here.
> */
> > +static int match_atom_bool_arg(const char *to_parse, const char *candidate,
> > + const char **end, int *val)
> > +{
I'll make these changes in the re-rolled version. I've also read your
reply to this email with the changes in `match_atom_arg_value()`. I'll
add them too.
Going off in a tangent here---In yesterday's review club which discussed
the %(decorate:<options>) patch[1], Glen suggested the possibility of
having a single kind of a framework (used this word very loosely here)
for parsing these multiple options since we are beginning to see them so
often (might also help new formats which maybe added in the future). The
fact that this wasn't done already says something about its difficulty
as Jacob mentioned yesterday. The difficulty being we don't exactly know
which options to parse as they differ from format to format.
Christian, Hariom and I had a similar discussion about refactoring these
helper functions that are already there in pretty (`match_placeholder_*()`)
so that they can be used here.
One example of this usage of functions from pretty is done when making
ref-filter support trailers.
[1]: https://lore.kernel.org/git/20230715160730.4046-1-andy.koppe@gmail.com/
Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
2023-07-19 23:23 ` Junio C Hamano
2023-07-20 5:21 ` Junio C Hamano
2023-07-20 16:52 ` Kousik Sanagavarapu
@ 2023-07-20 17:42 ` Glen Choo
2023-07-20 20:30 ` Junio C Hamano
2 siblings, 1 reply; 38+ messages in thread
From: Glen Choo @ 2023-07-20 17:42 UTC (permalink / raw)
To: Junio C Hamano, Kousik Sanagavarapu
Cc: git, Christian Couder, Hariom Verma, Josh Steadmon, Siddharth Singh
Junio C Hamano <gitster@pobox.com> writes:
> New helper functions that do not have any caller and no
> documentation to explain how they are supposed to be called
> (i.e. the expectation on the callers---what values they need to feed
> as parameters when they call these helpers, and the expectation by
> the callers---what they expect to get out of the helpers once they
> return) makes it impossible to evaluate if they are any good [*].
Agreed.
> Side note. Those of you who are keen to add unit tests to
> the system (Cc:ed) , do you think a patch line this one that
> adds a new helper function to the system, would benefit from
> being able to add a few unit tests for these otherwise
> unused helper functions?
Absolutely. As a rule, we should strive to test all of our changes as
they are introduced. With our current shell-based testing, this means
that we have to add callers (either via a builtin or test-helper), but
IMO a unit test framework would serve this purpose even better.
> The calls to the new functions that the unit test framework
> would make should serve as a good piece of interface
> documentation, showing what the callers are supposed to pass
> and what they expect, I guess.
Agreed, and as documentation, unit tests can be easier to read, since
they can include only the relevant details.
> So whatever framework we choose, it should allow adding a
> test or two to this patch easily, without being too
> intrusive. Would that be a good and concrete evaluation
> criterion?
Perhaps, but the biggest blocker to adding a unit tests is whether the
source file itself is amenable to being unit tested (e.g. does it depend
on global state? does it compile easily?). Once that is in place, I
can't imagine that there would be a sensible unit test framework that
doesn't make it easy to add tests to a patch like this.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
2023-07-20 16:52 ` Kousik Sanagavarapu
@ 2023-07-20 17:59 ` Junio C Hamano
0 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2023-07-20 17:59 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Christian Couder, Hariom Verma, Josh Steadmon,
Siddharth Singh, Glen Choo
Kousik Sanagavarapu <five231003@gmail.com> writes:
> Going off in a tangent here---In yesterday's review club which discussed
> the %(decorate:<options>) patch[1], Glen suggested the possibility of
> having a single kind of a framework (used this word very loosely here)
> for parsing these multiple options since we are beginning to see them so
> often (might also help new formats which maybe added in the future). The
> fact that this wasn't done already says something about its difficulty
> as Jacob mentioned yesterday. The difficulty being we don't exactly know
> which options to parse as they differ from format to format.
Yes, while writing the sample in-code documentation for the
match_atom_arg_value() function, I found its interface to force the
callers to do "parse if it is 'tags'; else parse if it is 'abbrev';
and so on" hard to use. I wondered if the primitive to parse these
should be modeled after config.c:parse_config_key(), i.e. the helper
function takes the input and returns the split point and lengths of
the components it finds in the return parameter.
As the contract between the caller and the callee is that the caller
passes the beginning of "key1[=val1],key2[=val2],...", the interface
may look like
int parse_cskv(const char *to_parse,
const char **key, size_t *keylen,
const char **val, size_t *vallen,
const char **end);
and would be used like so:
const char *cskv = "key1,key2=val2";
const char *key, *val;
size_t keylen, vallen;
while (parse_cskv(cskv, &key, &keylen, &val, &vallen, &cskv) {
if (!val)
printf("valueless key '%.*s'\n",
(int)keylen, key);
else
printf("key-value pair '%.*s=%.*s'\n",
(int)keylen, key, (int)vallen, val);
}
if (*cskv)
fprintf(stderr, "error - trailing garbage seen '%s'\n",
cskv);
The helper's contract to the caller may look like this:
- It expects the string to be "key" or "key=val" followed by ',' or
'\0' (the latter ends the input, i.e. the last element of comma
separated list). The out variables key, val, keylen, vallen are
used to convey to the caller where key and val are found and how
long they are.
- If it is a valueless "key", the out variable val is set to NULL.
- The out variable end is updated to point at one byte after the
element that has just been parsed.
The need for the caller to check against the list of keys it knows
about in the loop still exists, but the parser may become simpler
that way. I dunno.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
2023-07-20 17:42 ` Glen Choo
@ 2023-07-20 20:30 ` Junio C Hamano
2023-07-21 18:26 ` Glen Choo
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2023-07-20 20:30 UTC (permalink / raw)
To: Glen Choo
Cc: Kousik Sanagavarapu, git, Christian Couder, Hariom Verma,
Josh Steadmon, Siddharth Singh
Glen Choo <chooglen@google.com> writes:
>> So whatever framework we choose, it should allow adding a
>> test or two to this patch easily, without being too
>> intrusive. Would that be a good and concrete evaluation
>> criterion?
>
> Perhaps, but the biggest blocker to adding a unit tests is whether the
> source file itself is amenable to being unit tested (e.g. does it depend
> on global state? does it compile easily?).
Perhaps.
Now would this particular example, the change to ref-filter.c file,
be a reasonable guinea-pig test case for candidate test frameworks
to add tests for these two helper functions? They are pretty-much
plain vanilla string manipulation functions that does not depend too
many things that are specific to Git. They may use helpers we
wrote, i.e. xstrndup(), skip_prefix(), and git_parse_maybe_bool(),
but they shouldn't depend on the program start-up sequence,
discovering repositories, installing at-exit handers, and other
stuff. It was why I wondered if it can be used as a good evaluation
criterion---if a test framework cannot easily add tests while this
patch was being proposed in a non-intrusive way to demonstrate how
these two functions are supposed to work and to protect their
implementations from future breakage, it would not be all that
useful, I would imagine.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/2] Add new "describe" atom
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
@ 2023-07-20 22:52 ` Junio C Hamano
2023-07-20 23:10 ` Junio C Hamano
2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu
3 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2023-07-20 22:52 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> PATCH 1/2 - Left unchanged expect for small changes in the commit
> message for more clarity.
>
> PATCH 2/2 - We now parse the arguments in a seperate function
> `describe_atom_option_parser()` call this in
> `describe_atom_parser()` instead to populate
> `atom->u.describe_args`. This splitting of the function
> helps err at the right places.
This topic may be getting rerolled but from the CI logs,
comparing
* https://github.com/git/git/actions/runs/5603242871 (seen at
77ba682) that passes the tests
* https://github.com/git/git/actions/runs/5605480104 (seen at
29f0316) that breaks linux-gcc (ubuntu-20.04) at t6300 [*]
output from "git shortlog --no-merges 77ba682..29f0316" [*] makes us
suspect that this topic may be the culprit of the recent breakage.
The linux-gcc job is where we force the initial branch name to be
'main' and not 'master', so if your tests assume that the initial &
primary branch name is 'master', that may be something you need to
fix.
Thanks.
[Reference]
* https://github.com/git/git/actions/runs/5605480104/job/15186229680
* git shortlog --no-merges 77ba682..29f0316
Alex Henrie (1):
sequencer: finish parsing the todo list despite an invalid first line
Beat Bolli (1):
trace2: fix a comment
Junio C Hamano (4):
short help: allow multi-line opthelp
remote: simplify "remote add --tags" help text
short help: allow a gap smaller than USAGE_GAP
###
Kousik Sanagavarapu (2):
ref-filter: add multiple-option parsing functions
ref-filter: add new "describe" atom
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/2] Add new "describe" atom
2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano
@ 2023-07-20 23:10 ` Junio C Hamano
2023-07-21 4:17 ` Kousik Sanagavarapu
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2023-07-20 23:10 UTC (permalink / raw)
To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma
Junio C Hamano <gitster@pobox.com> writes:
> The linux-gcc job is where we force the initial branch name to be
> 'main' and not 'master', so if your tests assume that the initial &
> primary branch name is 'master', that may be something you need to
> fix.
Perhaps something along the line of the attached patch?
The primary test repository t6300 uses is aware of the "problem"
where the tester may set GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
to 'main' and hacks it around by using
git branch -M main
as one of the first things it does, to _force_ the primary branch
name always to 'main', whether the tester's environment forces "git"
to start with 'main' or 'master', and existing tests in the script
relies on 'main' being the primary branch.
But your tests are done in a repository newly created with your own
"git init", so depending on the tester's environment, the primary
branch may be 'master' or 'main'. The way your new tests are
written, however, things will fail if "refs/heads/master" is not the
primary branch.
t/t6300-for-each-ref.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh
index 4bbba76874..489f4d9186 100755
--- c/t/t6300-for-each-ref.sh
+++ w/t/t6300-for-each-ref.sh
@@ -563,7 +563,7 @@ test_expect_success 'color.ui=always does not override tty check' '
'
test_expect_success 'setup for describe atom tests' '
- git init describe-repo &&
+ git init -b master describe-repo &&
(
cd describe-repo &&
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/2] Add new "describe" atom
2023-07-20 23:10 ` Junio C Hamano
@ 2023-07-21 4:17 ` Kousik Sanagavarapu
0 siblings, 0 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-21 4:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Hariom Verma
On Thu, Jul 20, 2023 at 04:10:09PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > The linux-gcc job is where we force the initial branch name to be
> > 'main' and not 'master', so if your tests assume that the initial &
> > primary branch name is 'master', that may be something you need to
> > fix.
>
> Perhaps something along the line of the attached patch?
>
> The primary test repository t6300 uses is aware of the "problem"
> where the tester may set GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> to 'main' and hacks it around by using
>
> git branch -M main
>
> as one of the first things it does, to _force_ the primary branch
> name always to 'main', whether the tester's environment forces "git"
> to start with 'main' or 'master', and existing tests in the script
> relies on 'main' being the primary branch.
>
> But your tests are done in a repository newly created with your own
> "git init", so depending on the tester's environment, the primary
> branch may be 'master' or 'main'. The way your new tests are
> written, however, things will fail if "refs/heads/master" is not the
> primary branch.
>
I see. I looked at the trash directory by doing -v -i -d when running
these describe tests and was under the impression that it is always
master.I guess I should have had a bigger view of things.
> t/t6300-for-each-ref.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh
> index 4bbba76874..489f4d9186 100755
> --- c/t/t6300-for-each-ref.sh
> +++ w/t/t6300-for-each-ref.sh
> @@ -563,7 +563,7 @@ test_expect_success 'color.ui=always does not override tty check' '
> '
>
> test_expect_success 'setup for describe atom tests' '
> - git init describe-repo &&
> + git init -b master describe-repo &&
> (
> cd describe-repo &&
I'll add this to the re-rolled version.
Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
2023-07-20 20:30 ` Junio C Hamano
@ 2023-07-21 18:26 ` Glen Choo
0 siblings, 0 replies; 38+ messages in thread
From: Glen Choo @ 2023-07-21 18:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Kousik Sanagavarapu, git, Christian Couder, Hariom Verma,
Josh Steadmon, Siddharth Singh
Junio C Hamano <gitster@pobox.com> writes:
> Glen Choo <chooglen@google.com> writes:
>
>>> So whatever framework we choose, it should allow adding a
>>> test or two to this patch easily, without being too
>>> intrusive. Would that be a good and concrete evaluation
>>> criterion?
>>
>> Perhaps, but the biggest blocker to adding a unit tests is whether the
>> source file itself is amenable to being unit tested (e.g. does it depend
>> on global state? does it compile easily?).
>
> Now would this particular example, the change to ref-filter.c file,
> be a reasonable guinea-pig test case for candidate test frameworks
> to add tests for these two helper functions? They are pretty-much
> plain vanilla string manipulation functions that does not depend too
> many things that are specific to Git.
Ah, yes. This would be close-to-ideal candidate then.
> They may use helpers we
> wrote, i.e. xstrndup(), skip_prefix(), and git_parse_maybe_bool(),
> but they shouldn't depend on the program start-up sequence,
> discovering repositories, installing at-exit handers, and other
> stuff. It was why I wondered if it can be used as a good evaluation
> criterion---if a test framework cannot easily add tests while this
> patch was being proposed in a non-intrusive way to demonstrate how
> these two functions are supposed to work and to protect their
> implementations from future breakage, it would not be all that
> useful, I would imagine.
The current thinking among Googlers is that we won't remove the helpers
from library code. Git will either provide them, e.g. via Calvin's
git-std-lib RFC [1], or we will provide ways for callers to bring their
own implementation (like trace2 or exit, since it doesn't necessarily
make sense to use Git's implementation). So yes, the test framework
should be able to support this sort of compilation pattern. I'm not sure
how much test frameworks differ in this regard, maybe Josh has some
insight here.
[1] https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 0/2] Add new "describe" atom
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
` (2 preceding siblings ...)
2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano
@ 2023-07-23 16:19 ` Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
` (2 more replies)
3 siblings, 3 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-23 16:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh,
Christian Couder, Hariom Verma, Kousik Sanagavarapu
Hi,
Thanks for the review on the previous version.
PATCH 1/2 - Added comments to explain what the helper functions do and
also fixed a nit where config.h should be included
here and not the subsequent commit.
PATCH 2/2 - Changed the formatting on Documentation to follow what was
in the file. Also, the statement
"Descriptions can be insconsistent when tags are added
or removed at the same time"
is not true in ref-filter's case as,
$ time git for-each-ref --format="%(describe)" refs/
real 0m19.936s
user 0m11.488s
sys 0m7.915s
$ time git for-each-ref --format="%(describe) %(describe)" refs/
real 0m19.502s
user 0m11.653s
sys 0m7.623s
I also added a test to check that we err on bad describe
args.
Kousik Sanagavarapu (2):
ref-filter: add multiple-option parsing functions
ref-filter: add new "describe" atom
Documentation/git-for-each-ref.txt | 23 +++
ref-filter.c | 230 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 138 +++++++++++++++++
3 files changed, 391 insertions(+)
Range-diff against v3:
1: 08f3be1631 ! 1: 2914bd58ec ref-filter: add multiple-option parsing functions
@@ Commit message
are going to add a new %(describe) atom in a subsequent commit where we
parse options like tags=<bool-value> or match=<pattern> given to it.
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
## ref-filter.c ##
+@@
+ #include "git-compat-util.h"
+ #include "environment.h"
+ #include "gettext.h"
++#include "config.h"
+ #include "gpg-interface.h"
+ #include "hex.h"
+ #include "parse-options.h"
@@ ref-filter.c: static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)
return -1;
}
++/*
++ * Parse option of name "candidate" in the option string "to_parse" of
++ * the form
++ *
++ * "candidate1[=val1],candidate2[=val2],candidate3[=val3],..."
++ *
++ * The remaining part of "to_parse" is stored in "end" (if we are
++ * parsing the last candidate, then this is NULL) and the value of
++ * the candidate is stored in "valuestart" and its length in "valuelen",
++ * that is the portion after "=". Since it is possible for a "candidate"
++ * to not have a value, in such cases, "valuestart" is set to point to
++ * NULL and "valuelen" to 0.
++ *
++ * The function returns 1 on success. It returns 0 if we don't find
++ * "candidate" in "to_parse" or we find "candidate" but it is followed
++ * by more chars (for example, "candidatefoo"), that is, we don't find
++ * an exact match.
++ *
++ * This function only does the above for one "candidate" at a time. So
++ * it has to be called each time trying to parse a "candidate" in the
++ * option string "to_parse".
++ */
+static int match_atom_arg_value(const char *to_parse, const char *candidate,
+ const char **end, const char **valuestart,
+ size_t *valuelen)
+{
+ const char *atom;
+
-+ if (!(skip_prefix(to_parse, candidate, &atom)))
++ if (!skip_prefix(to_parse, candidate, &atom))
++ return 0; /* definitely not "candidate" */
++
++ if (*atom == '=') {
++ /* we just saw "candidate=" */
++ *valuestart = atom + 1;
++ atom = strchrnul(*valuestart, ',');
++ *valuelen = atom - *valuestart;
++ } else if (*atom != ',' && *atom != '\0') {
++ /* key begins with "candidate" but has more chars */
+ return 0;
-+ if (valuestart) {
-+ if (*atom == '=') {
-+ *valuestart = atom + 1;
-+ *valuelen = strcspn(*valuestart, ",\0");
-+ atom = *valuestart + *valuelen;
-+ } else {
-+ if (*atom != ',' && *atom != '\0')
-+ return 0;
-+ *valuestart = NULL;
-+ *valuelen = 0;
-+ }
-+ }
-+ if (*atom == ',') {
-+ *end = atom + 1;
-+ return 1;
-+ }
-+ if (*atom == '\0') {
-+ *end = atom;
-+ return 1;
++ } else {
++ /* just "candidate" without "=val" */
++ *valuestart = NULL;
++ *valuelen = 0;
+ }
-+ return 0;
++
++ /* atom points at either the ',' or NUL after this key[=val] */
++ if (*atom == ',')
++ atom++;
++ else if (*atom)
++ BUG("Why is *atom not NULL yet?");
++
++ *end = atom;
++ return 1;
+}
+
++/*
++ * Parse boolean option of name "candidate" in the option list "to_parse"
++ * of the form
++ *
++ * "candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],..."
++ *
++ * The remaining part of "to_parse" is stored in "end" (if we are parsing
++ * the last candidate, then this is NULL) and the value (if given) is
++ * parsed and stored in "val", so "val" always points to either 0 or 1.
++ * If the value is not given, then "val" is set to point to 1.
++ *
++ * The boolean value is parsed using "git_parse_maybe_bool()", so the
++ * accepted values are
++ *
++ * to set true - "1", "yes", "true"
++ * to set false - "0", "no", "false"
++ *
++ * This function returns 1 on success. It returns 0 when we don't find
++ * an exact match for "candidate" or when the boolean value given is
++ * not valid.
++ */
+static int match_atom_bool_arg(const char *to_parse, const char *candidate,
+ const char **end, int *val)
+{
2: 742a79113c ! 2: 77a2a56520 ref-filter: add new "describe" atom
@@ Commit message
The new atom "describe" and its friends are equivalent to the existing
pretty formats with the same name.
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
@@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the output
ref to the `<committish>` specified in the format.
-+describe[:options]:: Human-readable name, like
-+ link-git:git-describe[1]; empty string for
-+ undescribable commits. The `describe` string may be
-+ followed by a colon and zero or more comma-separated
-+ options. Descriptions can be inconsistent when tags
-+ are added or removed at the same time.
++describe[:options]::
++ A human-readable name, like linkgit:git-describe[1];
++ empty string for undescribable commits. The `describe` string may
++ be followed by a colon and one or more comma-separated options.
++
+--
-+tags=<bool-value>;; Instead of only considering annotated tags, consider
-+ lightweight tags as well; see the corresponding option
-+ in linkgit:git-describe[1] for details.
-+abbrev=<number>;; Use at least <number> hexadecimal digits; see
-+ the corresponding option in linkgit:git-describe[1]
-+ for details.
-+match=<pattern>;; Only consider tags matching the given `glob(7)` pattern,
-+ excluding the "refs/tags/" prefix; see the corresponding
-+ option in linkgit:git-describe[1] for details.
-+exclude=<pattern>;; Do not consider tags matching the given `glob(7)`
-+ pattern, excluding the "refs/tags/" prefix; see the
-+ corresponding option in linkgit:git-describe[1] for
-+ details.
++tags=<bool-value>;;
++ Instead of only considering annotated tags, consider
++ lightweight tags as well; see the corresponding option in
++ linkgit:git-describe[1] for details.
++abbrev=<number>;;
++ Use at least <number> hexadecimal digits; see the corresponding
++ option in linkgit:git-describe[1] for details.
++match=<pattern>;;
++ Only consider tags matching the given `glob(7)` pattern,
++ excluding the "refs/tags/" prefix; see the corresponding option
++ in linkgit:git-describe[1] for details.
++exclude=<pattern>;;
++ Do not consider tags matching the given `glob(7)` pattern,
++ excluding the "refs/tags/" prefix; see the corresponding option
++ in linkgit:git-describe[1] for details.
+--
+
In addition to the above, for commit and tag objects, the header
@@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>::
## ref-filter.c ##
@@
- #include "git-compat-util.h"
- #include "environment.h"
- #include "gettext.h"
-+#include "config.h"
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
@@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct
+
+ for (;;) {
+ int found = 0;
-+ const char *bad_arg = NULL;
++ const char *bad_arg = arg;
+
+ if (!arg || !*arg)
+ break;
+
-+ bad_arg = arg;
+ found = describe_atom_option_parser(&args, &arg, err);
+ if (found < 0)
+ return found;
-+ if (!found) {
-+ if (bad_arg && *bad_arg)
-+ return err_bad_arg(err, "describe", bad_arg);
-+ break;
-+ }
++ if (!found)
++ return err_bad_arg(err, "describe", bad_arg);
+ }
+ atom->u.describe_args = strvec_detach(&args);
+ return 0;
@@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override
'
+test_expect_success 'setup for describe atom tests' '
-+ git init describe-repo &&
++ git init -b master describe-repo &&
+ (
+ cd describe-repo &&
+
@@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override
+ test_cmp expect actual
+ )
+'
++
++test_expect_success 'err on bad describe atom arg' '
++ (
++ cd describe-repo &&
++
++ # The bad arg is the only arg passed to describe atom
++ cat >expect <<-\EOF &&
++ fatal: unrecognized %(describe) argument: baz
++ EOF
++ ! git for-each-ref --format="%(describe:baz)" \
++ refs/heads/master 2>actual &&
++ test_cmp expect actual &&
++
++ # The bad arg is in the middle of the option string
++ # passed to the describe atom
++ cat >expect <<-\EOF &&
++ fatal: unrecognized %(describe) argument: qux=1,abbrev=14
++ EOF
++ ! git for-each-ref \
++ --format="%(describe:tags,qux=1,abbrev=14)" \
++ ref/heads/master 2>actual &&
++ test_cmp expect actual
++ )
++'
+
cat >expected <<\EOF
heads/main
--
2.41.0.396.g9ab76b0018
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 1/2] ref-filter: add multiple-option parsing functions
2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu
@ 2023-07-23 16:19 ` Kousik Sanagavarapu
2023-07-24 17:29 ` Junio C Hamano
2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu
2 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-23 16:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh,
Christian Couder, Hariom Verma, Kousik Sanagavarapu
The functions
match_placeholder_arg_value()
match_placeholder_bool_arg()
were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options
with explicit value, 2019-01-29) to parse multiple options in an
argument to --pretty. For example,
git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )"
will output all the trailers matching the key and seperates them by
a comma followed by a space per commit.
Add similar functions,
match_atom_arg_value()
match_atom_bool_arg()
in ref-filter.
There is no atom yet that can use these functions in ref-filter, but we
are going to add a new %(describe) atom in a subsequent commit where we
parse options like tags=<bool-value> or match=<pattern> given to it.
Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index 0f3df132b8..8d5f85e0a7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1,6 +1,7 @@
#include "git-compat-util.h"
#include "environment.h"
#include "gettext.h"
+#include "config.h"
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
@@ -255,6 +256,110 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)
return -1;
}
+/*
+ * Parse option of name "candidate" in the option string "to_parse" of
+ * the form
+ *
+ * "candidate1[=val1],candidate2[=val2],candidate3[=val3],..."
+ *
+ * The remaining part of "to_parse" is stored in "end" (if we are
+ * parsing the last candidate, then this is NULL) and the value of
+ * the candidate is stored in "valuestart" and its length in "valuelen",
+ * that is the portion after "=". Since it is possible for a "candidate"
+ * to not have a value, in such cases, "valuestart" is set to point to
+ * NULL and "valuelen" to 0.
+ *
+ * The function returns 1 on success. It returns 0 if we don't find
+ * "candidate" in "to_parse" or we find "candidate" but it is followed
+ * by more chars (for example, "candidatefoo"), that is, we don't find
+ * an exact match.
+ *
+ * This function only does the above for one "candidate" at a time. So
+ * it has to be called each time trying to parse a "candidate" in the
+ * option string "to_parse".
+ */
+static int match_atom_arg_value(const char *to_parse, const char *candidate,
+ const char **end, const char **valuestart,
+ size_t *valuelen)
+{
+ const char *atom;
+
+ if (!skip_prefix(to_parse, candidate, &atom))
+ return 0; /* definitely not "candidate" */
+
+ if (*atom == '=') {
+ /* we just saw "candidate=" */
+ *valuestart = atom + 1;
+ atom = strchrnul(*valuestart, ',');
+ *valuelen = atom - *valuestart;
+ } else if (*atom != ',' && *atom != '\0') {
+ /* key begins with "candidate" but has more chars */
+ return 0;
+ } else {
+ /* just "candidate" without "=val" */
+ *valuestart = NULL;
+ *valuelen = 0;
+ }
+
+ /* atom points at either the ',' or NUL after this key[=val] */
+ if (*atom == ',')
+ atom++;
+ else if (*atom)
+ BUG("Why is *atom not NULL yet?");
+
+ *end = atom;
+ return 1;
+}
+
+/*
+ * Parse boolean option of name "candidate" in the option list "to_parse"
+ * of the form
+ *
+ * "candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],..."
+ *
+ * The remaining part of "to_parse" is stored in "end" (if we are parsing
+ * the last candidate, then this is NULL) and the value (if given) is
+ * parsed and stored in "val", so "val" always points to either 0 or 1.
+ * If the value is not given, then "val" is set to point to 1.
+ *
+ * The boolean value is parsed using "git_parse_maybe_bool()", so the
+ * accepted values are
+ *
+ * to set true - "1", "yes", "true"
+ * to set false - "0", "no", "false"
+ *
+ * This function returns 1 on success. It returns 0 when we don't find
+ * an exact match for "candidate" or when the boolean value given is
+ * not valid.
+ */
+static int match_atom_bool_arg(const char *to_parse, const char *candidate,
+ const char **end, int *val)
+{
+ const char *argval;
+ char *strval;
+ size_t arglen;
+ int v;
+
+ if (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen))
+ return 0;
+
+ if (!argval) {
+ *val = 1;
+ return 1;
+ }
+
+ strval = xstrndup(argval, arglen);
+ v = git_parse_maybe_bool(strval);
+ free(strval);
+
+ if (v == -1)
+ return 0;
+
+ *val = v;
+
+ return 1;
+}
+
static int color_atom_parser(struct ref_format *format, struct used_atom *atom,
const char *color_value, struct strbuf *err)
{
--
2.41.0.396.g9ab76b0018
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v4 2/2] ref-filter: add new "describe" atom
2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
@ 2023-07-23 16:19 ` Kousik Sanagavarapu
2023-07-24 17:21 ` Junio C Hamano
2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu
2 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-23 16:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh,
Christian Couder, Hariom Verma, Kousik Sanagavarapu
Duplicate the logic of %(describe) and friends from pretty to
ref-filter. In the future, this change helps in unifying both the
formats as ref-filter will be able to do everything that pretty is doing
and we can have a single interface.
The new atom "describe" and its friends are equivalent to the existing
pretty formats with the same name.
Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
Documentation/git-for-each-ref.txt | 23 +++++
ref-filter.c | 125 ++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 138 +++++++++++++++++++++++++++++
3 files changed, 286 insertions(+)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index d9588767a9..11b2bc3121 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -264,6 +264,29 @@ ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the output
ref to the `<committish>` specified in the format.
+describe[:options]::
+ A human-readable name, like linkgit:git-describe[1];
+ empty string for undescribable commits. The `describe` string may
+ be followed by a colon and one or more comma-separated options.
++
+--
+tags=<bool-value>;;
+ Instead of only considering annotated tags, consider
+ lightweight tags as well; see the corresponding option in
+ linkgit:git-describe[1] for details.
+abbrev=<number>;;
+ Use at least <number> hexadecimal digits; see the corresponding
+ option in linkgit:git-describe[1] for details.
+match=<pattern>;;
+ Only consider tags matching the given `glob(7)` pattern,
+ excluding the "refs/tags/" prefix; see the corresponding option
+ in linkgit:git-describe[1] for details.
+exclude=<pattern>;;
+ Do not consider tags matching the given `glob(7)` pattern,
+ excluding the "refs/tags/" prefix; see the corresponding option
+ in linkgit:git-describe[1] for details.
+--
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 8d5f85e0a7..df00f1628c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -5,6 +5,7 @@
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
+#include "run-command.h"
#include "refs.h"
#include "wildmatch.h"
#include "object-name.h"
@@ -146,6 +147,7 @@ enum atom_type {
ATOM_TAGGERDATE,
ATOM_CREATOR,
ATOM_CREATORDATE,
+ ATOM_DESCRIBE,
ATOM_SUBJECT,
ATOM_BODY,
ATOM_TRAILERS,
@@ -220,6 +222,7 @@ static struct used_atom {
enum { S_BARE, S_GRADE, S_SIGNER, S_KEY,
S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;
} signature;
+ const char **describe_args;
struct refname_atom refname;
char *head;
} u;
@@ -600,6 +603,87 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
return 0;
}
+static int describe_atom_option_parser(struct strvec *args, const char **arg,
+ struct strbuf *err)
+{
+ const char *argval;
+ size_t arglen = 0;
+ int optval = 0;
+
+ if (match_atom_bool_arg(*arg, "tags", arg, &optval)) {
+ if (!optval)
+ strvec_push(args, "--no-tags");
+ else
+ strvec_push(args, "--tags");
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "abbrev", arg, &argval, &arglen)) {
+ char *endptr;
+
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("argument expected for %s"),
+ "describe:abbrev");
+ if (strtol(argval, &endptr, 10) < 0)
+ return strbuf_addf_ret(err, -1,
+ _("positive value expected %s=%s"),
+ "describe:abbrev", argval);
+ if (endptr - argval != arglen)
+ return strbuf_addf_ret(err, -1,
+ _("cannot fully parse %s=%s"),
+ "describe:abbrev", argval);
+
+ strvec_pushf(args, "--abbrev=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "match", arg, &argval, &arglen)) {
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("value expected %s="),
+ "describe:match");
+
+ strvec_pushf(args, "--match=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) {
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("value expected %s="),
+ "describe:exclude");
+
+ strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ return 0;
+}
+
+static int describe_atom_parser(struct ref_format *format UNUSED,
+ struct used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
+ struct strvec args = STRVEC_INIT;
+
+ for (;;) {
+ int found = 0;
+ const char *bad_arg = arg;
+
+ if (!arg || !*arg)
+ break;
+
+ found = describe_atom_option_parser(&args, &arg, err);
+ if (found < 0)
+ return found;
+ if (!found)
+ return err_bad_arg(err, "describe", bad_arg);
+ }
+ atom->u.describe_args = strvec_detach(&args);
+ return 0;
+}
+
static int raw_atom_parser(struct ref_format *format UNUSED,
struct used_atom *atom,
const char *arg, struct strbuf *err)
@@ -802,6 +886,7 @@ static struct {
[ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
[ATOM_CREATOR] = { "creator", SOURCE_OBJ },
[ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
+ [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
[ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
@@ -1708,6 +1793,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
}
}
+static void grab_describe_values(struct atom_value *val, int deref,
+ struct object *obj)
+{
+ struct commit *commit = (struct commit *)obj;
+ int i;
+
+ for (i = 0; i < used_atom_cnt; i++) {
+ struct used_atom *atom = &used_atom[i];
+ enum atom_type type = atom->atom_type;
+ const char *name = atom->name;
+ struct atom_value *v = &val[i];
+
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+
+ if (type != ATOM_DESCRIBE)
+ continue;
+
+ if (!!deref != (*name == '*'))
+ continue;
+
+ cmd.git_cmd = 1;
+ strvec_push(&cmd.args, "describe");
+ strvec_pushv(&cmd.args, atom->u.describe_args);
+ strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+ if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+ error(_("failed to run 'describe'"));
+ v->s = xstrdup("");
+ continue;
+ }
+ strbuf_rtrim(&out);
+ v->s = strbuf_detach(&out, NULL);
+
+ strbuf_release(&err);
+ }
+}
+
/* See grab_values */
static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)
{
@@ -1817,6 +1940,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_tag_values(val, deref, obj);
grab_sub_body_contents(val, deref, data);
grab_person("tagger", val, deref, buf);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_COMMIT:
grab_commit_values(val, deref, obj);
@@ -1824,6 +1948,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_person("author", val, deref, buf);
grab_person("committer", val, deref, buf);
grab_signature(val, deref, obj);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_TREE:
/* grab_tree_values(val, deref, obj, buf, sz); */
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 910bf1ea94..7116e008f4 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -597,6 +597,144 @@ test_expect_success 'color.ui=always does not override tty check' '
test_cmp expected.bare actual
'
+test_expect_success 'setup for describe atom tests' '
+ git init -b master describe-repo &&
+ (
+ cd describe-repo &&
+
+ test_commit --no-tag one &&
+ git tag tagone &&
+
+ test_commit --no-tag two &&
+ git tag -a -m "tag two" tagtwo
+ )
+'
+
+test_expect_success 'describe atom vs git describe' '
+ (
+ cd describe-repo &&
+
+ git for-each-ref --format="%(objectname)" \
+ refs/tags/ >obj &&
+ while read hash
+ do
+ if desc=$(git describe $hash)
+ then
+ : >expect-contains-good
+ else
+ : >expect-contains-bad
+ fi &&
+ echo "$hash $desc" || return 1
+ done <obj >expect &&
+ test_path_exists expect-contains-good &&
+ test_path_exists expect-contains-bad &&
+
+ git for-each-ref --format="%(objectname) %(describe)" \
+ refs/tags/ >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+ )
+'
+
+test_expect_success 'describe:tags vs describe --tags' '
+ (
+ cd describe-repo &&
+ git describe --tags >expect &&
+ git for-each-ref --format="%(describe:tags)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '
+ (
+ cd describe-repo &&
+
+ # Case 1: We have commits between HEAD and the most
+ # recent tag reachable from it
+ test_commit --no-tag file &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual &&
+
+ # Make sure the hash used is atleast 14 digits long
+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+ test 15 -le $(wc -c <hexpart) &&
+
+ # Case 2: We have a tag at HEAD, describe directly gives
+ # the name of the tag
+ git tag -a -m tagged tagname &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual &&
+ test tagname = $(cat actual)
+ )
+'
+
+test_expect_success 'describe:match=... vs describe --match ...' '
+ (
+ cd describe-repo &&
+ git tag -a -m "tag foo" tag-foo &&
+ git describe --match "*-foo" >expect &&
+ git for-each-ref --format="%(describe:match="*-foo")" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'describe:exclude:... vs describe --exclude ...' '
+ (
+ cd describe-repo &&
+ git tag -a -m "tag bar" tag-bar &&
+ git describe --exclude "*-bar" >expect &&
+ git for-each-ref --format="%(describe:exclude="*-bar")" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'deref with describe atom' '
+ (
+ cd describe-repo &&
+ cat >expect <<-\EOF &&
+
+ tagname
+ tagname
+ tagname
+
+ tagtwo
+ EOF
+ git for-each-ref --format="%(*describe)" >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'err on bad describe atom arg' '
+ (
+ cd describe-repo &&
+
+ # The bad arg is the only arg passed to describe atom
+ cat >expect <<-\EOF &&
+ fatal: unrecognized %(describe) argument: baz
+ EOF
+ ! git for-each-ref --format="%(describe:baz)" \
+ refs/heads/master 2>actual &&
+ test_cmp expect actual &&
+
+ # The bad arg is in the middle of the option string
+ # passed to the describe atom
+ cat >expect <<-\EOF &&
+ fatal: unrecognized %(describe) argument: qux=1,abbrev=14
+ EOF
+ ! git for-each-ref \
+ --format="%(describe:tags,qux=1,abbrev=14)" \
+ ref/heads/master 2>actual &&
+ test_cmp expect actual
+ )
+'
+
cat >expected <<\EOF
heads/main
tags/main
--
2.41.0.396.g9ab76b0018
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v4 2/2] ref-filter: add new "describe" atom
2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
@ 2023-07-24 17:21 ` Junio C Hamano
0 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2023-07-24 17:21 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder,
Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> +test_expect_success 'err on bad describe atom arg' '
> + (
> + cd describe-repo &&
> +
> + # The bad arg is the only arg passed to describe atom
> + cat >expect <<-\EOF &&
> + fatal: unrecognized %(describe) argument: baz
> + EOF
> + ! git for-each-ref --format="%(describe:baz)" \
> + refs/heads/master 2>actual &&
> + test_cmp expect actual &&
Instead of "! git something", use of "test_must_fail git something" is
recommended. The former would pass upon a crashing "git" happily, but
the latter would complain if "git" segfaults.
> + # The bad arg is in the middle of the option string
> + # passed to the describe atom
> + cat >expect <<-\EOF &&
> + fatal: unrecognized %(describe) argument: qux=1,abbrev=14
> + EOF
> + ! git for-each-ref \
> + --format="%(describe:tags,qux=1,abbrev=14)" \
> + ref/heads/master 2>actual &&
Ditto.
> + test_cmp expect actual
> + )
> +'
Other than that, both patches looked good to me. Thanks.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions
2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
@ 2023-07-24 17:29 ` Junio C Hamano
2023-07-24 18:12 ` Kousik Sanagavarapu
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2023-07-24 17:29 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder,
Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> The functions
>
> match_placeholder_arg_value()
> match_placeholder_bool_arg()
>
> were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options
> with explicit value, 2019-01-29) to parse multiple options in an
> argument to --pretty. For example,
>
> git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )"
>
> will output all the trailers matching the key and seperates them by
> a comma followed by a space per commit.
>
> Add similar functions,
>
> match_atom_arg_value()
> match_atom_bool_arg()
>
> in ref-filter.
What are their similarities, and in what way are they different? If
they are similar enough, is it reasonable to allow these two pairs
of helpers to share code (the best case would be we can just call
the existing ones, possibly changing their names to more suitable
ones that fit their now-more-general-purpose nature better)?
> There is no atom yet that can use these functions in ref-filter, but we
> are going to add a new %(describe) atom in a subsequent commit where we
> parse options like tags=<bool-value> or match=<pattern> given to it.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Hariom Verma <hariom18599@gmail.com>
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
> ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
Asking just out of curiousity, all patches from you seem to have
"Mentored-by" naming your mentors, but how deeply involved are they
in each patch you send out? Is it like you first ask them to review
and only after addressing the issues their reviews raise, you are
sending the polished patches to the list? Or are they not deeply
involved in the code but offering suggestions on the design (I am
curious what their reactions were on your design decision to
add the two helper functions)?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions
2023-07-24 17:29 ` Junio C Hamano
@ 2023-07-24 18:12 ` Kousik Sanagavarapu
2023-07-24 20:39 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-24 18:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder,
Hariom Verma
On Mon, Jul 24, 2023 at 10:29:52AM -0700, Junio C Hamano wrote:
> Kousik Sanagavarapu <five231003@gmail.com> writes:
>
> > The functions
> >
> > match_placeholder_arg_value()
> > match_placeholder_bool_arg()
> >
> > were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options
> > with explicit value, 2019-01-29) to parse multiple options in an
> > argument to --pretty. For example,
> >
> > git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )"
> >
> > will output all the trailers matching the key and seperates them by
> > a comma followed by a space per commit.
> >
> > Add similar functions,
> >
> > match_atom_arg_value()
> > match_atom_bool_arg()
> >
> > in ref-filter.
>
> What are their similarities, and in what way are they different? If
> they are similar enough, is it reasonable to allow these two pairs
> of helpers to share code (the best case would be we can just call
> the existing ones, possibly changing their names to more suitable
> ones that fit their now-more-general-purpose nature better)?
What do you mean by "share code"?
They are similar in their functionality, that is parsing the option and
grabbing the value (if the option has a value, otherwise we do what we
did here). The difference is the way we do such a parsing.
In pretty, we directly skip_prefix() the placeholder. So we check for ')'
to see if we have reached the end of "to_parse".
In ref-filter (the current patches), we deal directly with the options
("arg" here), that is we can't do a check for ')' to see if we have
exhausted our option list. So we can't really use the same functions, but
there is the possiblity that we can modify them to be used here too.
So the difference is mainly just how we send "to_parse" and how we want
it parsed.
> > There is no atom yet that can use these functions in ref-filter, but we
> > are going to add a new %(describe) atom in a subsequent commit where we
> > parse options like tags=<bool-value> or match=<pattern> given to it.
> >
> > Helped-by: Junio C Hamano <gitster@pobox.com>
> > Mentored-by: Christian Couder <christian.couder@gmail.com>
> > Mentored-by: Hariom Verma <hariom18599@gmail.com>
> > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> > ---
> > ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 105 insertions(+)
>
> Asking just out of curiousity, all patches from you seem to have
> "Mentored-by" naming your mentors, but how deeply involved are they
> in each patch you send out? Is it like you first ask them to review
> and only after addressing the issues their reviews raise, you are
> sending the polished patches to the list? Or are they not deeply
> involved in the code but offering suggestions on the design
Both actually, the code and the design. I send them the commits which I
push to my fork and they take a look on the code as well as the design
and offer suggestions on how both can be improved or re-did.
> (I am
> curious what their reactions were on your design decision to
> add the two helper functions)?
They suggested doing something similar to what you suggested above but
it is kind of on hold (also because of how we changed the implementation
of "match_atom_arg_value()"). Now that you bring it up, should this
patch be reworked?
Thanks
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions
2023-07-24 18:12 ` Kousik Sanagavarapu
@ 2023-07-24 20:39 ` Junio C Hamano
2023-07-25 19:27 ` Junio C Hamano
0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2023-07-24 20:39 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder,
Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> What do you mean by "share code"?
>
> They are similar in their functionality, that is parsing the option and
> grabbing the value (if the option has a value, otherwise we do what we
> did here). The difference is the way we do such a parsing.
>
> In pretty, we directly skip_prefix() the placeholder. So we check for ')'
> to see if we have reached the end of "to_parse".
>
> In ref-filter (the current patches), we deal directly with the options
> ("arg" here), that is we can't do a check for ')' to see if we have
> exhausted our option list. So we can't really use the same functions, but
> there is the possiblity that we can modify them to be used here too.
That is the kind of "sharing" to reduce repetition I had in mind.
I haven't checked the callers, but another way would be to update
the caller of for-each-ref's side to match the calling convention of
how pretty calls the parser, wouldn't it? After all, they parse the
same "%(token:key=val,key=val,...)" so...?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions
2023-07-24 20:39 ` Junio C Hamano
@ 2023-07-25 19:27 ` Junio C Hamano
0 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2023-07-25 19:27 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder,
Hariom Verma
Junio C Hamano <gitster@pobox.com> writes:
> Kousik Sanagavarapu <five231003@gmail.com> writes:
>
>> What do you mean by "share code"?
>>
>> They are similar in their functionality, that is parsing the option and
>> grabbing the value (if the option has a value, otherwise we do what we
>> did here). The difference is the way we do such a parsing.
>>
>> In pretty, we directly skip_prefix() the placeholder. So we check for ')'
>> to see if we have reached the end of "to_parse".
>>
>> In ref-filter (the current patches), we deal directly with the options
>> ("arg" here), that is we can't do a check for ')' to see if we have
>> exhausted our option list. So we can't really use the same functions, but
>> there is the possiblity that we can modify them to be used here too.
>
> That is the kind of "sharing" to reduce repetition I had in mind.
>
> I haven't checked the callers, but another way would be to update
> the caller of for-each-ref's side to match the calling convention of
> how pretty calls the parser, wouldn't it? After all, they parse the
> same "%(token:key=val,key=val,...)" so...?
Having said all that, let's move things forward by merging this
version. Anybody (it could be you) interested in cleaning up by
unifying these two very similar implementations of the same thing
can do so on top.
Thanks.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v5 0/2] Add new "describe" atom
2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
@ 2023-07-25 20:51 ` Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
` (2 more replies)
2 siblings, 3 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-25 20:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh,
Christian Couder, Hariom Verma, Kousik Sanagavarapu
Hi,
Thanks for the review on the previous version of this series. Here is a
quick re-roll to address the minor changes that you left on the previous
version (apart from the suggestions to PATCH 1/2).
Please queue this instead.
PATCH 1/2 - Left unchanged
PATCH 2/2 - Used the test helper function `test_must_fail` instead of
something like "! git foo" for making a command fail.
Kousik Sanagavarapu (2):
ref-filter: add multiple-option parsing functions
ref-filter: add new "describe" atom
Documentation/git-for-each-ref.txt | 23 +++
ref-filter.c | 230 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 139 +++++++++++++++++
3 files changed, 392 insertions(+)
Range-diff against v4:
1: 2914bd58ec = 1: 2914bd58ec ref-filter: add multiple-option parsing functions
2: 77a2a56520 ! 2: 8127f4399c ref-filter: add new "describe" atom
@@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override
+ cat >expect <<-\EOF &&
+ fatal: unrecognized %(describe) argument: baz
+ EOF
-+ ! git for-each-ref --format="%(describe:baz)" \
++ test_must_fail git for-each-ref \
++ --format="%(describe:baz)" \
+ refs/heads/master 2>actual &&
+ test_cmp expect actual &&
+
@@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override
+ cat >expect <<-\EOF &&
+ fatal: unrecognized %(describe) argument: qux=1,abbrev=14
+ EOF
-+ ! git for-each-ref \
++ test_must_fail git for-each-ref \
+ --format="%(describe:tags,qux=1,abbrev=14)" \
+ ref/heads/master 2>actual &&
+ test_cmp expect actual
--
2.41.0.396.g77a2a56520
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v5 1/2] ref-filter: add multiple-option parsing functions
2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu
@ 2023-07-25 20:51 ` Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-25 21:46 ` [PATCH v5 0/2] Add " Junio C Hamano
2 siblings, 0 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-25 20:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh,
Christian Couder, Hariom Verma, Kousik Sanagavarapu
The functions
match_placeholder_arg_value()
match_placeholder_bool_arg()
were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options
with explicit value, 2019-01-29) to parse multiple options in an
argument to --pretty. For example,
git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )"
will output all the trailers matching the key and seperates them by
a comma followed by a space per commit.
Add similar functions,
match_atom_arg_value()
match_atom_bool_arg()
in ref-filter.
There is no atom yet that can use these functions in ref-filter, but we
are going to add a new %(describe) atom in a subsequent commit where we
parse options like tags=<bool-value> or match=<pattern> given to it.
Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index 0f3df132b8..8d5f85e0a7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1,6 +1,7 @@
#include "git-compat-util.h"
#include "environment.h"
#include "gettext.h"
+#include "config.h"
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
@@ -255,6 +256,110 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)
return -1;
}
+/*
+ * Parse option of name "candidate" in the option string "to_parse" of
+ * the form
+ *
+ * "candidate1[=val1],candidate2[=val2],candidate3[=val3],..."
+ *
+ * The remaining part of "to_parse" is stored in "end" (if we are
+ * parsing the last candidate, then this is NULL) and the value of
+ * the candidate is stored in "valuestart" and its length in "valuelen",
+ * that is the portion after "=". Since it is possible for a "candidate"
+ * to not have a value, in such cases, "valuestart" is set to point to
+ * NULL and "valuelen" to 0.
+ *
+ * The function returns 1 on success. It returns 0 if we don't find
+ * "candidate" in "to_parse" or we find "candidate" but it is followed
+ * by more chars (for example, "candidatefoo"), that is, we don't find
+ * an exact match.
+ *
+ * This function only does the above for one "candidate" at a time. So
+ * it has to be called each time trying to parse a "candidate" in the
+ * option string "to_parse".
+ */
+static int match_atom_arg_value(const char *to_parse, const char *candidate,
+ const char **end, const char **valuestart,
+ size_t *valuelen)
+{
+ const char *atom;
+
+ if (!skip_prefix(to_parse, candidate, &atom))
+ return 0; /* definitely not "candidate" */
+
+ if (*atom == '=') {
+ /* we just saw "candidate=" */
+ *valuestart = atom + 1;
+ atom = strchrnul(*valuestart, ',');
+ *valuelen = atom - *valuestart;
+ } else if (*atom != ',' && *atom != '\0') {
+ /* key begins with "candidate" but has more chars */
+ return 0;
+ } else {
+ /* just "candidate" without "=val" */
+ *valuestart = NULL;
+ *valuelen = 0;
+ }
+
+ /* atom points at either the ',' or NUL after this key[=val] */
+ if (*atom == ',')
+ atom++;
+ else if (*atom)
+ BUG("Why is *atom not NULL yet?");
+
+ *end = atom;
+ return 1;
+}
+
+/*
+ * Parse boolean option of name "candidate" in the option list "to_parse"
+ * of the form
+ *
+ * "candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],..."
+ *
+ * The remaining part of "to_parse" is stored in "end" (if we are parsing
+ * the last candidate, then this is NULL) and the value (if given) is
+ * parsed and stored in "val", so "val" always points to either 0 or 1.
+ * If the value is not given, then "val" is set to point to 1.
+ *
+ * The boolean value is parsed using "git_parse_maybe_bool()", so the
+ * accepted values are
+ *
+ * to set true - "1", "yes", "true"
+ * to set false - "0", "no", "false"
+ *
+ * This function returns 1 on success. It returns 0 when we don't find
+ * an exact match for "candidate" or when the boolean value given is
+ * not valid.
+ */
+static int match_atom_bool_arg(const char *to_parse, const char *candidate,
+ const char **end, int *val)
+{
+ const char *argval;
+ char *strval;
+ size_t arglen;
+ int v;
+
+ if (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen))
+ return 0;
+
+ if (!argval) {
+ *val = 1;
+ return 1;
+ }
+
+ strval = xstrndup(argval, arglen);
+ v = git_parse_maybe_bool(strval);
+ free(strval);
+
+ if (v == -1)
+ return 0;
+
+ *val = v;
+
+ return 1;
+}
+
static int color_atom_parser(struct ref_format *format, struct used_atom *atom,
const char *color_value, struct strbuf *err)
{
--
2.41.0.396.g77a2a56520
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v5 2/2] ref-filter: add new "describe" atom
2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
@ 2023-07-25 20:51 ` Kousik Sanagavarapu
2023-07-25 21:46 ` [PATCH v5 0/2] Add " Junio C Hamano
2 siblings, 0 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-25 20:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh,
Christian Couder, Hariom Verma, Kousik Sanagavarapu
Duplicate the logic of %(describe) and friends from pretty to
ref-filter. In the future, this change helps in unifying both the
formats as ref-filter will be able to do everything that pretty is doing
and we can have a single interface.
The new atom "describe" and its friends are equivalent to the existing
pretty formats with the same name.
Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
Documentation/git-for-each-ref.txt | 23 +++++
ref-filter.c | 125 ++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 139 +++++++++++++++++++++++++++++
3 files changed, 287 insertions(+)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index d9588767a9..11b2bc3121 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -264,6 +264,29 @@ ahead-behind:<committish>::
commits ahead and behind, respectively, when comparing the output
ref to the `<committish>` specified in the format.
+describe[:options]::
+ A human-readable name, like linkgit:git-describe[1];
+ empty string for undescribable commits. The `describe` string may
+ be followed by a colon and one or more comma-separated options.
++
+--
+tags=<bool-value>;;
+ Instead of only considering annotated tags, consider
+ lightweight tags as well; see the corresponding option in
+ linkgit:git-describe[1] for details.
+abbrev=<number>;;
+ Use at least <number> hexadecimal digits; see the corresponding
+ option in linkgit:git-describe[1] for details.
+match=<pattern>;;
+ Only consider tags matching the given `glob(7)` pattern,
+ excluding the "refs/tags/" prefix; see the corresponding option
+ in linkgit:git-describe[1] for details.
+exclude=<pattern>;;
+ Do not consider tags matching the given `glob(7)` pattern,
+ excluding the "refs/tags/" prefix; see the corresponding option
+ in linkgit:git-describe[1] for details.
+--
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 8d5f85e0a7..df00f1628c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -5,6 +5,7 @@
#include "gpg-interface.h"
#include "hex.h"
#include "parse-options.h"
+#include "run-command.h"
#include "refs.h"
#include "wildmatch.h"
#include "object-name.h"
@@ -146,6 +147,7 @@ enum atom_type {
ATOM_TAGGERDATE,
ATOM_CREATOR,
ATOM_CREATORDATE,
+ ATOM_DESCRIBE,
ATOM_SUBJECT,
ATOM_BODY,
ATOM_TRAILERS,
@@ -220,6 +222,7 @@ static struct used_atom {
enum { S_BARE, S_GRADE, S_SIGNER, S_KEY,
S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;
} signature;
+ const char **describe_args;
struct refname_atom refname;
char *head;
} u;
@@ -600,6 +603,87 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
return 0;
}
+static int describe_atom_option_parser(struct strvec *args, const char **arg,
+ struct strbuf *err)
+{
+ const char *argval;
+ size_t arglen = 0;
+ int optval = 0;
+
+ if (match_atom_bool_arg(*arg, "tags", arg, &optval)) {
+ if (!optval)
+ strvec_push(args, "--no-tags");
+ else
+ strvec_push(args, "--tags");
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "abbrev", arg, &argval, &arglen)) {
+ char *endptr;
+
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("argument expected for %s"),
+ "describe:abbrev");
+ if (strtol(argval, &endptr, 10) < 0)
+ return strbuf_addf_ret(err, -1,
+ _("positive value expected %s=%s"),
+ "describe:abbrev", argval);
+ if (endptr - argval != arglen)
+ return strbuf_addf_ret(err, -1,
+ _("cannot fully parse %s=%s"),
+ "describe:abbrev", argval);
+
+ strvec_pushf(args, "--abbrev=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "match", arg, &argval, &arglen)) {
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("value expected %s="),
+ "describe:match");
+
+ strvec_pushf(args, "--match=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) {
+ if (!arglen)
+ return strbuf_addf_ret(err, -1,
+ _("value expected %s="),
+ "describe:exclude");
+
+ strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval);
+ return 1;
+ }
+
+ return 0;
+}
+
+static int describe_atom_parser(struct ref_format *format UNUSED,
+ struct used_atom *atom,
+ const char *arg, struct strbuf *err)
+{
+ struct strvec args = STRVEC_INIT;
+
+ for (;;) {
+ int found = 0;
+ const char *bad_arg = arg;
+
+ if (!arg || !*arg)
+ break;
+
+ found = describe_atom_option_parser(&args, &arg, err);
+ if (found < 0)
+ return found;
+ if (!found)
+ return err_bad_arg(err, "describe", bad_arg);
+ }
+ atom->u.describe_args = strvec_detach(&args);
+ return 0;
+}
+
static int raw_atom_parser(struct ref_format *format UNUSED,
struct used_atom *atom,
const char *arg, struct strbuf *err)
@@ -802,6 +886,7 @@ static struct {
[ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
[ATOM_CREATOR] = { "creator", SOURCE_OBJ },
[ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
+ [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
[ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
[ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
[ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
@@ -1708,6 +1793,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
}
}
+static void grab_describe_values(struct atom_value *val, int deref,
+ struct object *obj)
+{
+ struct commit *commit = (struct commit *)obj;
+ int i;
+
+ for (i = 0; i < used_atom_cnt; i++) {
+ struct used_atom *atom = &used_atom[i];
+ enum atom_type type = atom->atom_type;
+ const char *name = atom->name;
+ struct atom_value *v = &val[i];
+
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+
+ if (type != ATOM_DESCRIBE)
+ continue;
+
+ if (!!deref != (*name == '*'))
+ continue;
+
+ cmd.git_cmd = 1;
+ strvec_push(&cmd.args, "describe");
+ strvec_pushv(&cmd.args, atom->u.describe_args);
+ strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+ if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {
+ error(_("failed to run 'describe'"));
+ v->s = xstrdup("");
+ continue;
+ }
+ strbuf_rtrim(&out);
+ v->s = strbuf_detach(&out, NULL);
+
+ strbuf_release(&err);
+ }
+}
+
/* See grab_values */
static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)
{
@@ -1817,6 +1940,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_tag_values(val, deref, obj);
grab_sub_body_contents(val, deref, data);
grab_person("tagger", val, deref, buf);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_COMMIT:
grab_commit_values(val, deref, obj);
@@ -1824,6 +1948,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s
grab_person("author", val, deref, buf);
grab_person("committer", val, deref, buf);
grab_signature(val, deref, obj);
+ grab_describe_values(val, deref, obj);
break;
case OBJ_TREE:
/* grab_tree_values(val, deref, obj, buf, sz); */
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 910bf1ea94..16082dccb7 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -597,6 +597,145 @@ test_expect_success 'color.ui=always does not override tty check' '
test_cmp expected.bare actual
'
+test_expect_success 'setup for describe atom tests' '
+ git init -b master describe-repo &&
+ (
+ cd describe-repo &&
+
+ test_commit --no-tag one &&
+ git tag tagone &&
+
+ test_commit --no-tag two &&
+ git tag -a -m "tag two" tagtwo
+ )
+'
+
+test_expect_success 'describe atom vs git describe' '
+ (
+ cd describe-repo &&
+
+ git for-each-ref --format="%(objectname)" \
+ refs/tags/ >obj &&
+ while read hash
+ do
+ if desc=$(git describe $hash)
+ then
+ : >expect-contains-good
+ else
+ : >expect-contains-bad
+ fi &&
+ echo "$hash $desc" || return 1
+ done <obj >expect &&
+ test_path_exists expect-contains-good &&
+ test_path_exists expect-contains-bad &&
+
+ git for-each-ref --format="%(objectname) %(describe)" \
+ refs/tags/ >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+ )
+'
+
+test_expect_success 'describe:tags vs describe --tags' '
+ (
+ cd describe-repo &&
+ git describe --tags >expect &&
+ git for-each-ref --format="%(describe:tags)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '
+ (
+ cd describe-repo &&
+
+ # Case 1: We have commits between HEAD and the most
+ # recent tag reachable from it
+ test_commit --no-tag file &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual &&
+
+ # Make sure the hash used is atleast 14 digits long
+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+ test 15 -le $(wc -c <hexpart) &&
+
+ # Case 2: We have a tag at HEAD, describe directly gives
+ # the name of the tag
+ git tag -a -m tagged tagname &&
+ git describe --abbrev=14 >expect &&
+ git for-each-ref --format="%(describe:abbrev=14)" \
+ refs/heads/master >actual &&
+ test_cmp expect actual &&
+ test tagname = $(cat actual)
+ )
+'
+
+test_expect_success 'describe:match=... vs describe --match ...' '
+ (
+ cd describe-repo &&
+ git tag -a -m "tag foo" tag-foo &&
+ git describe --match "*-foo" >expect &&
+ git for-each-ref --format="%(describe:match="*-foo")" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'describe:exclude:... vs describe --exclude ...' '
+ (
+ cd describe-repo &&
+ git tag -a -m "tag bar" tag-bar &&
+ git describe --exclude "*-bar" >expect &&
+ git for-each-ref --format="%(describe:exclude="*-bar")" \
+ refs/heads/master >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'deref with describe atom' '
+ (
+ cd describe-repo &&
+ cat >expect <<-\EOF &&
+
+ tagname
+ tagname
+ tagname
+
+ tagtwo
+ EOF
+ git for-each-ref --format="%(*describe)" >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'err on bad describe atom arg' '
+ (
+ cd describe-repo &&
+
+ # The bad arg is the only arg passed to describe atom
+ cat >expect <<-\EOF &&
+ fatal: unrecognized %(describe) argument: baz
+ EOF
+ test_must_fail git for-each-ref \
+ --format="%(describe:baz)" \
+ refs/heads/master 2>actual &&
+ test_cmp expect actual &&
+
+ # The bad arg is in the middle of the option string
+ # passed to the describe atom
+ cat >expect <<-\EOF &&
+ fatal: unrecognized %(describe) argument: qux=1,abbrev=14
+ EOF
+ test_must_fail git for-each-ref \
+ --format="%(describe:tags,qux=1,abbrev=14)" \
+ ref/heads/master 2>actual &&
+ test_cmp expect actual
+ )
+'
+
cat >expected <<\EOF
heads/main
tags/main
--
2.41.0.396.g77a2a56520
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v5 0/2] Add new "describe" atom
2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
@ 2023-07-25 21:46 ` Junio C Hamano
2 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2023-07-25 21:46 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder,
Hariom Verma
Kousik Sanagavarapu <five231003@gmail.com> writes:
> Thanks for the review on the previous version of this series. Here is a
> quick re-roll to address the minor changes that you left on the previous
> version (apart from the suggestions to PATCH 1/2).
>
> Please queue this instead.
"! -> test_must_fail" has already been corrected locally yesterday
before I pushed the integration results out, so I'd skip this round.
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2023-07-25 21:47 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu
2023-07-06 16:58 ` Junio C Hamano
2023-07-09 6:16 ` Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 2/2] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-14 20:57 ` Junio C Hamano
2023-07-15 18:24 ` Kousik Sanagavarapu
2023-07-15 18:56 ` Junio C Hamano
2023-07-14 19:20 ` [PATCH v2 3/3] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-19 23:23 ` Junio C Hamano
2023-07-20 5:21 ` Junio C Hamano
2023-07-20 16:52 ` Kousik Sanagavarapu
2023-07-20 17:59 ` Junio C Hamano
2023-07-20 17:42 ` Glen Choo
2023-07-20 20:30 ` Junio C Hamano
2023-07-21 18:26 ` Glen Choo
2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-19 22:56 ` Junio C Hamano
2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano
2023-07-20 23:10 ` Junio C Hamano
2023-07-21 4:17 ` Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-24 17:29 ` Junio C Hamano
2023-07-24 18:12 ` Kousik Sanagavarapu
2023-07-24 20:39 ` Junio C Hamano
2023-07-25 19:27 ` Junio C Hamano
2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-24 17:21 ` Junio C Hamano
2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-25 21:46 ` [PATCH v5 0/2] Add " Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.