Re: [PATCH v2 04/16] ref-filter: modify "%(objectname:short)" to take length
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Mar 18, 2016, 18:38 UTC
- Message-ID
- <CAOLa=ZQCWk46cMy0A-wpquVtCUXL4Eyo_uVLqxQfnpqo9r1OhA@mail.gmail.com>
- In-Reply-To
- <CA+P7+xofBUBOWLMdYi7V7Np00V0e7kcBgDDgjT-g4dnk2ag22g@mail.gmail.com>
Hello
On Wed, Mar 16, 2016 at 3:41 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
Show 9 quoted lines
> On Tue, Mar 15, 2016 at 9:47 AM, Karthik Nayak <karthik.188@gmail.com> wrote: >> Add support for %(objectname:short=<length>) which would print the >> abbreviated unique objectname of given length. When no length is >> specified 7 is used. The minimum length is 'MINIMUM_ABBREV'. >> > > Isn't the default abbreviation value used here, not hard coded to 7? > Thus user can configure this? >
DEFAULT_ABBREV is set to 7. But I think it makes for sense to state DEFAULT_ABBREV rather than mention 7 without any base information.
> Description should also mention that length may be exceeded if it is > not long enough to be a unique identifier. >
Will add that in.
>> Add tests and documentation for the same. >> > > Also I didn't see any update to the documentation here..
I totally missed that out, will add it in
Show 45 quoted lines
>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> ref-filter.c | 25 +++++++++++++++++++------
>> t/t6300-for-each-ref.sh | 10 ++++++++++
>> 2 files changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 857a8b5..17f781d 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.contents.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;
>
> Should this error instead of accepting and upgrading it to the minimum?
>I prefer that it doesn't error out so that someone who's not aware of the absolute value of MINIMUM_ABBREV would still be able to give a value which is < MINIMUM_ABRREV and still expect results.
Show 15 quoted lines
>> + } 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));
>
> Yes, here we are using DEFAULT_ABBREV, so you should mention that
> instead of using 7 in the commit message.
>Will do.
Show 49 quoted lines
>> return 1;
>> - } 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.7.3
>>Thanks for your suggestions.
-- Regards, Karthik Nayak