[v4,0/2] Add new "describe" atom

Message

Kousik Sanagavarapu July 23, 2023, 4:19 p.m. UTC

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 <[e-mail]>
         Mentored-by: Christian Couder <[e-mail]>
         Mentored-by: Hariom Verma <[e-mail]>
         Signed-off-by: Kousik Sanagavarapu <[e-mail]>
     
      ## 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 <[e-mail]>
         Mentored-by: Christian Couder <[e-mail]>
         Mentored-by: Hariom Verma <[e-mail]>
         Signed-off-by: Kousik Sanagavarapu <[e-mail]>
    @@ 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