Re: [PATCH v3 0/2] check-attr: add support to work with revisions

From: Karthik Nayak <karthik.188@gmail.com>
Date: Dec 17, 2022, 14:46 UTC
Message-ID: <CAOLa=ZR2XUX1EPgG0g_UgZo3wjuZLWthJerztqUL4+qCJKb=KA@mail.gmail.com>
In-Reply-To: <221216.86k02r1fcl.gmgdl@evledraar.gmail.com>

On Fri, Dec 16, 2022 at 5:30 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 11 quoted lines

>
>
> On Fri, Dec 16 2022, Karthik Nayak wrote:
>
> > v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/
> > v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t
>
> Could you please set the In-Reply-To header appropriately in the future,
> so that each version of this series isn't in its own disconnected
> thread?
>

I didn't know, will do this from next time!

Show 7 quoted lines

> > This series aims to add a new flag `-r|--revisions` to git-check-attr(1) which
> > allows us to read gitattributes from the specified revision.
>
> I didn't look at the v2, but expected at least the short form to be gone
> here re
> https://lore.kernel.org/git/CAOLa=ZTSzUh2Ma_EMHHWcDunGyKMaUW9BaG=QdegtMqLd+69Wg@mail.gmail.com/;
>

Right, I was open to it, but since there wasn't any confirmation, I didn't go forward with it. Will remove it from the next version.

Show 13 quoted lines

> I'm still more partial to the alternate suggestion I had in
> https://lore.kernel.org/git/221207.86lenja0zi.gmgdl@evledraar.gmail.com/;
> I'm not sure what you meant in your reply at
> https://lore.kernel.org/git/CAOLa=ZQua8TfApCdzoK06_2fkWb4ZCfWewXKOSaXno1fqFSq2A@mail.gmail.com/
> (sorry about not following up at the time) with:
>
>         "when being consistent we need to be fully consistent,
>         i.e. <revision>:<path>, tweaking this slightly to be
>         <revision>:<attr> is worse than breaking consistency."
>
> Yes, it would, but isn't that by definition the case with any
> proposal?
>

I'm not opposing the proposal, rather stating my opinion on it. To go over my reply

I'm only saying that most users of Git are accustomed to the `<revision>:<path>` syntax and now breaking that only in one command to be `<revision>:<attr>` seems a bit odd, from the user experience point of view.

Show 16 quoted lines

> We don't have a way to refer to an attribute (or all attributes for -a)
> for a given revision/path, the task of this series is to invent such a
> syntax.
>
> So we could invent that as this series currently does with:
>
>         git check-attrs --revision <rev> <attr>... <path>...
>
> Or, as I suggested:
>
>         git check-attr [<rev>:]<attr>... -- <path>...
>
> Or whatever. Here I'm not saying that one is better than the other, but
> advocating for one on the basis of consistency doesn't make sense to me,
> this is new syntax.
>

I see what you mean, but I was referring to consistency around how different options are used in other git commands.

Mainly that most commands treat the second section after `<rev>:` to be a path, now adding a new option where the section after `<rev>:` to be an attribute, might be a bit confusing.

Show 6 quoted lines

> I think what you mean is that because the log family uses "<rev>:<path>"
> we should not come up with a syntax that looks anything like
> "<lhs>:<rhs>"., as the "<lhs>" in the mind of some users is going to be
> "<rev>", and the "<rhs>" is "<path>", so it would be confusing to have
> it be "<attr>" here, and have the "<path>..." come after the "--".
>

Exactly.

Show 7 quoted lines

> I'm not convinced by that. From refspecs to e.g. "git log"'s own "-L" we
> have little mini-syntaxes in various places that use this sort of colon
> notation. I find it more elegant than "--revision".
>
> It's fine if you disagree, I'm just trying to understand the basis of
> the disagreement.
>

I don't disagree. I think it's healthy to have this discussion, especially since we're adding a new option and this is the right time. I'm all ears and finally want to get the best solution.

-- 
- Karthik