Re: [PATCH v1 2/2] log: add option to choose which refs to decorate

From: Rafael Ascensão <rafa.almas@gmail.com>
Date: Nov 10, 2017, 13:38 UTC
Message-ID: <89e7f8e0-8b0d-fde0-5e28-31173213a26e@gmail.com>
In-Reply-To: <xmqqbmkfhrf3.fsf@gitster.mtv.corp.google.com>

On 07/11/17 00:18, Junio C Hamano wrote:

Show 10 quoted lines

> Jacob Keller <jacob.keller@gmail.com> writes:
> 
> I would have to say that the describe's one is wrong if it does not
> match what for_each_glob_ref() does for the log family of commands'
> "--branches=<pattern>" etc.  describe.c::get_name() uses positive
> and negative patterns, just like log-tree.c::add_ref_decoration()
> would with the patch we are discussing, so perhaps the items in
> these lists should get the same "normalize" treatment the patch 1/2
> of this series brings in to make things consistent?
>

I agree that describe should receive the "normalize" treatment. However, and following the same reasoning, why should describe users adopt the rules imposed by --glob? I could argue they're also used to the way it works now.

That being said, the suggestion I mentioned earlier would allow to keep both current behaviors consistent at the expense of the extra call to refs.c::ref_exists().

+if (!has_glob_specials(pattern) && !ref_exists(normalized_pattern->buf)) {
+        /* Append implied '/' '*' if not present. */
+        strbuf_complete(normalized_pattern, '/');
+        /* No need to check for '*', there is none. */
+        strbuf_addch(normalized_pattern, '*');
+}

But I don't have enough expertise to decide if this consistency is worth the extra call to refs.c::ref_exists() or if there are other side-effects I am not considering.

>> That being said, if we think the extra glob would not cause
>> problems and generally do what people mean... I guess consistent
>> with --glob would be good... But it's definitely not what I'd
>> expect at first glance.

My position is that consistency is good, but the "first glance expectation" is definitely something important we should take into consideration.