[RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

Junio C Hamano
Stefan Beller <[hidden email]> writes:

> And we want to have both "label=A B C" and attr:label=A B C" or *just* the
> attr query?

I think the choice at this point is between supporting just "label=A
B C" or supporting just "attr:eol=crlf text=auto !diff".

I think "attr:label=A" is merely a degenerated case of the latter.

> We should not allow the user to add arbitrary attributes (i.e. labels).

Hmph, why not?

> Instead each of the "attr for labeling purposes" needs to follow a clear scheme
> that allows us to add attributes later on that are outside of that scheme.

That was my initial reaction when I saw Duy's "attr:crlf=auto" (by
the way, there is no such setting; crlf should be one of TRUE, UNSET
or set to string "input") idea.  But I do not think of a good
argument to justify that arbitrary attributes are not allowed.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

Stefan Beller-4
On Mon, May 16, 2016 at 2:50 PM, Junio C Hamano <[hidden email]> wrote:

> Stefan Beller <[hidden email]> writes:
>
>> And we want to have both "label=A B C" and attr:label=A B C" or *just* the
>> attr query?
>
> I think the choice at this point is between supporting just "label=A
> B C" or supporting just "attr:eol=crlf text=auto !diff".
>
> I think "attr:label=A" is merely a degenerated case of the latter.
>
>> We should not allow the user to add arbitrary attributes (i.e. labels).

Because we cannot extend our attributes any further from that?

Consider a user starts using attr <foo> for their labeling purposes.
Later (in 2 years) we decide that <foo> is an attribute we want to
use to mark files as foo-ish. so we add meaning to that attribute
(just like eol.crlf does an arbitrary thing, foo would do another arbitrary
thing).

Then the user picks up the new version of Git and expects <foo> to
still be a "I use it for labeling purposes only" thing. They would not
expect to all files labeled as <foo> to start behaving <foo-ish> ?

> Hmph, why not?

We need a namespace for which
* we can guarantee that it is for labeling purposes only (even in the future)
* is obvious to the user to be a labeling name space

Starting with "label" offers both?

>
>> Instead each of the "attr for labeling purposes" needs to follow a clear scheme
>> that allows us to add attributes later on that are outside of that scheme.
>
> That was my initial reaction when I saw Duy's "attr:crlf=auto" (by
> the way, there is no such setting; crlf should be one of TRUE, UNSET
> or set to string "input") idea.  But I do not think of a good
> argument to justify that arbitrary attributes are not allowed.

I agree that querying for attr:eol or attr:diff is a good idea.

I do however want to point out that all labels for "labeling purposes"
MUST be a clear from the beginning, otherwise we'll have the maintenance
problem down the road?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

Junio C Hamano
Stefan Beller <[hidden email]> writes:

>> Hmph, why not?
>
> We need a namespace for which
> * we can guarantee that it is for labeling purposes only (even in the future)
> * is obvious to the user to be a labeling name space
>
> Starting with "label" offers both?

Ah, of course.  I thought that you were trying to limit ":(attr:<attribute>)"
magic only to attributes that begin with "label-", which is where my
"why not?" comes from.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

Stefan Beller-4
On Mon, May 16, 2016 at 3:02 PM, Junio C Hamano <[hidden email]> wrote:

> Stefan Beller <[hidden email]> writes:
>
>>> Hmph, why not?
>>
>> We need a namespace for which
>> * we can guarantee that it is for labeling purposes only (even in the future)
>> * is obvious to the user to be a labeling name space
>>
>> Starting with "label" offers both?
>
> Ah, of course.  I thought that you were trying to limit ":(attr:<attribute>)"
> magic only to attributes that begin with "label-", which is where my
> "why not?" comes from.

And going by the logic you presented before, we would
need to error out for the given pathspec ":(<string>)" if

* either the string is not well known (e.g. diif, eol )
* or is outside of the labeling namespace.

So we don't want to see users complaining about
"bug attr:foo worked as a label, now it is a feature; you broke my code"

We would need to ignore data from .gitattributes as it may be crafted from
a newer version of Git, but the command line argument still needs to die
for unknown arguments?

So asking for :(foo) would yield a

    fatal: attr 'foo' is not known to Git, nor is it in the labeling name space

I guess what I am asking is if there is a nice way to query "do we know
this attribute?"

Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]

Junio C Hamano
Stefan Beller <[hidden email]> writes:

>> Ah, of course.  I thought that you were trying to limit ":(attr:<attribute>)"
>> magic only to attributes that begin with "label-", which is where my
>> "why not?" comes from.
>
> And going by the logic you presented before, we would
> need to error out for the given pathspec ":(<string>)" if
>
> * either the string is not well known (e.g. diif, eol )
> * or is outside of the labeling namespace.

I do not think that follows _my_ line of thought.  What is "well
known"?  Doesn't that change over time?

If we are to do the "attribute match", there is no useful
enforcement point.  An arbitrary version of Git cannot differentiate
a random cruft users will write in their .gitattributes from a
legitimate attribute that will be introduced in the future, so both
"data in .gitattributes" and "pathspec magic that referes to attribute"
cannot be limited by us.

So if we are going to take the arbitrary ":(attr:<attribute>)"
route, "label-" prefix would be limitation on the "core Git" side
and does not limit what end-user does.  We will promise that we
won't use names that begin with the prefix ourselves and leave them
up to the projects.  If the end user uses an attribute "foo", which
does not begin with "label-", the end user is risking to be broken
by future versions of Git.

If you want to compile an authoritative list of attributes used by
core Git and maintain it forever, you are welcome to add warning,
but I wouldn't bother if I were doing this series, at least at the
beginning.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
12