Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length
From: Jacob Keller <hidden>
Date: 2016-11-08 23:28:21
On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak [off-list ref] wrote:
From: Karthik Nayak <redacted> Add support for %(objectname:short=<length>) which would print the abbreviated unique objectname of given length. When no length is specified, the length is 'DEFAULT_ABBREV'. The minimum length is 'MINIMUM_ABBREV'. The length may be exceeded to ensure that the provided object name is unique.
Ok this makes sense. It may be annoying that the length might go beyond the size that we wanted, but I think it's better than printing a non-unique short abbreviation. I have one suggested change, which is to drop O_LENGTH and have O_SHORT store the length always, setting it to DEFAULT_ABBREV when no length provided. This allows you to drop some code. I don't think it's actually worth a re-roll by itself since the current code is correct. Thanks, Jake
quoted hunk ↗ jump to hunk
Add tests and documentation for the same. Mentored-by: Christian Couder [off-list ref] Mentored-by: Matthieu Moy [off-list ref] Helped-by: Jacob Keller [off-list ref] Signed-off-by: Karthik Nayak <redacted> --- Documentation/git-for-each-ref.txt | 4 ++++ ref-filter.c | 25 +++++++++++++++++++------ t/t6300-for-each-ref.sh | 10 ++++++++++ 3 files changed, 33 insertions(+), 6 deletions(-)diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index b7b8560..92184c4 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt@@ -107,6 +107,10 @@ objectsize:: objectname:: The object name (aka SHA-1). For a non-ambiguous abbreviation of the object name append `:short`. + For an abbreviation of the object name with desired length append + `:short=<length>`, where the minimum length is MINIMUM_ABBREV. The + length may be exceeded to ensure unique object names. + upstream:: The name of a local ref which can be considered ``upstream''diff --git a/ref-filter.c b/ref-filter.c index 44481c3..fe4ea2b 100644 --- a/ref-filter.c +++ b/ref-filter.c@@ -55,7 +55,10 @@ static struct used_atom { const char *if_equals, *not_equals; } if_then_else; - enum { O_FULL, O_SHORT } objectname; + struct { + enum { O_FULL, O_LENGTH, O_SHORT } option; + unsigned int length; + } objectname; } u; } *used_atom; static int used_atom_cnt, need_tagged, need_symref;@@ -118,10 +121,17 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg) static void objectname_atom_parser(struct used_atom *atom, const char *arg) { if (!arg) - atom->u.objectname = O_FULL; + atom->u.objectname.option = O_FULL; else if (!strcmp(arg, "short")) - atom->u.objectname = O_SHORT; - else + atom->u.objectname.option = O_SHORT; + else if (skip_prefix(arg, "short=", &arg)) { + atom->u.objectname.option = O_LENGTH; + if (strtoul_ui(arg, 10, &atom->u.objectname.length) || + atom->u.objectname.length == 0) + die(_("positive value expected objectname:short=%s"), arg); + if (atom->u.objectname.length < MINIMUM_ABBREV) + atom->u.objectname.length = MINIMUM_ABBREV;
One way to reduce some code is to set O_SHORT and O_LENGTH as the same (either O_SHORT or O_LENGTH) and when no length is found simply set it to the DEFAULT_ABBREV.
quoted hunk ↗ jump to hunk
+ } else die(_("unrecognized %%(objectname) argument: %s"), arg); }@@ -591,12 +601,15 @@ static int grab_objectname(const char *name, const unsigned char *sha1, struct atom_value *v, struct used_atom *atom) { if (starts_with(name, "objectname")) { - if (atom->u.objectname == O_SHORT) { + if (atom->u.objectname.option == O_SHORT) { v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV)); return 1;
That would allow dropping an entire section here. I don't think this is worth a re-roll by itself, and I think either approach is probably ok.
quoted hunk ↗ jump to hunk
- } else if (atom->u.objectname == O_FULL) { + } else if (atom->u.objectname.option == O_FULL) { v->s = xstrdup(sha1_to_hex(sha1)); return 1; + } else if (atom->u.objectname.option == O_LENGTH) { + v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length)); + return 1; } else die("BUG: unknown %%(objectname) option"); }diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 19a2823..2be0a3f 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh@@ -60,6 +60,8 @@ test_atom head objecttype commit test_atom head objectsize 171 test_atom head objectname $(git rev-parse refs/heads/master) test_atom head objectname:short $(git rev-parse --short refs/heads/master) +test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master) +test_atom head objectname:short=10 $(git rev-parse --short=10 refs/heads/master) test_atom head tree $(git rev-parse refs/heads/master^{tree}) test_atom head parent '' test_atom head numparent 0@@ -99,6 +101,8 @@ test_atom tag objecttype tag test_atom tag objectsize 154 test_atom tag objectname $(git rev-parse refs/tags/testtag) test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag) +test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master) +test_atom head objectname:short=10 $(git rev-parse --short=10 refs/heads/master) test_atom tag tree '' test_atom tag parent '' test_atom tag numparent ''@@ -164,6 +168,12 @@ test_expect_success 'Check invalid format specifiers are errors' ' test_must_fail git for-each-ref --format="%(authordate:INVALID)" refs/heads ' +test_expect_success 'arguments to %(objectname:short=) must be positive integers' ' + test_must_fail git for-each-ref --format="%(objectname:short=0)" && + test_must_fail git for-each-ref --format="%(objectname:short=-1)" && + test_must_fail git for-each-ref --format="%(objectname:short=foo)" +' + test_date () { f=$1 && committer_date=$2 && --2.10.2