Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

Hello,

On Fri, Nov 11, 2016 at 10:59 AM, Jacob Keller  wrote:
> On Thu, Nov 10, 2016 at 9:36 AM, Karthik Nayak  wrote:
>> On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller  wrote:
>>
>> That does make sense, It would also not error out when we use
>> %(objectname:short=) and
>> not specify the length. Idk, if that's desirable or not. But it does
>> make the code a little more
>> confusing to read at the same time.
>>
>
> I am not sure that would be the case. If you see "objectname:short"
> you trreat this as if they had passed "objectname:short= abbrev>" but if you see "objectname:short=" you die, no?
>

Sorry, my bad.



On Fri, Nov 11, 2016 at 5:02 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>>   else if (!strcmp(arg, "short"))
>> - atom->u.objectname = O_SHORT;
>> - else
>> + atom->u.objectname.option = O_SHORT;
>> + else if (skip_prefix(arg, "short=", )) {
>> + atom->u.objectname.option = O_LENGTH;
>> + if (strtoul_ui(arg, 10, >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;
>> + } else
>>   die(_("unrecognized %%(objectname) argument: %s"), arg);
>>  }
>
> Users who want to use the default-abbrev, i.e. the autoscaling one
> introduced recently, must use "short", not "short=-1", with this
> code (especially with the "must be at least MINIMUM_ABBREV" logic),
> but I do not think it is a problem, so I think this is good.
>

I think I'll leave this as it is. If that's okay

-- 
Regards,
Karthik Nayak

Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length

[Jacob Keller]

On Thu, Nov 10, 2016 at 9:36 AM, Karthik Nayak  wrote:
> On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller  wrote:
>> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
>>> From: Karthik Nayak 
>>>
>>> Add support for %(objectname:short=) 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
>>
>
> That does make sense, It would also not error out when we use
> %(objectname:short=) and
> not specify the length. Idk, if that's desirable or not. But it does
> make the code a little more
> confusing to read at the same time.
>

I am not sure that would be the case. If you see "objectname:short"
you trreat this as if they had passed "objectname:short=" but if you see "objectname:short=" you die, no?

> So since its a small change, I'd be okay going either ways with this.
>
> --
> Regards,
> Karthik Nayak

Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length

[Junio C Hamano]

Karthik Nayak  writes:

>   else if (!strcmp(arg, "short"))
> - atom->u.objectname = O_SHORT;
> - else
> + atom->u.objectname.option = O_SHORT;
> + else if (skip_prefix(arg, "short=", )) {
> + atom->u.objectname.option = O_LENGTH;
> + if (strtoul_ui(arg, 10, >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;
> + } else
>   die(_("unrecognized %%(objectname) argument: %s"), arg);
>  }

Users who want to use the default-abbrev, i.e. the autoscaling one
introduced recently, must use "short", not "short=-1", with this
code (especially with the "must be at least MINIMUM_ABBREV" logic),
but I do not think it is a problem, so I think this is good.


> @@ -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;
> - } 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;

Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller  wrote:
> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
>> From: Karthik Nayak 
>>
>> Add support for %(objectname:short=) 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
>

That does make sense, It would also not error out when we use
%(objectname:short=) and
not specify the length. Idk, if that's desirable or not. But it does
make the code a little more
confusing to read at the same time.

So since its a small change, I'd be okay going either ways with this.

-- 
Regards,
Karthik Nayak

Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length

[Jacob Keller]

On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
> From: Karthik Nayak 
>
> Add support for %(objectname:short=) 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 
> Mentored-by: Matthieu Moy 
> Helped-by: Jacob Keller 
> Signed-off-by: Karthik Nayak 
> ---
>  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=`, 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=", )) {
> +   atom->u.objectname.option = O_LENGTH;
> +   if (strtoul_ui(arg, 10, >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");