On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>
> 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
> Add tests and documentation for the same.
>
> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
> Mentored-by: Matthieu Moy <matthieu.moy@xxxxxxxxxxxxxxx>
> Helped-by: Jacob Keller <jacob.keller@xxxxxxxxx>
> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> ---
> 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.
> + } 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.
> - } 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
>