From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 07 Dec 2022 12:33:22 GMT
Subject: Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions
Message-ID: <CAOLa=ZQua8TfApCdzoK06_2fkWb4ZCfWewXKOSaXno1fqFSq2A@mail.gmail.com>
In-Reply-To: <221207.86lenja0zi.gmgdl@evledraar.gmail.com>

On Wed, Dec 7, 2022 at 12:48 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Wed, Dec 07 2022, Junio C Hamano wrote:
>
> > Karthik Nayak <karthik.188@gmail.com> writes:
> >
> >> Add a new flag `--revision`/`-r` which will allow it work with
> >> revisions. This command will now, instead of checking the files/index,
> >> try and receive the blob for the given attribute file against the
> >> provided revision. The flag overrides checking against the index and
> >> filesystem and also works with bare repositories.
> >
> > As "check-attr" was not invented as a user-facing subcommand but was
> > a hack for debugging, I would have minded this change, but these
> > days people seem to treat it as if it is just one of the proper
> > plumbing commands, the new command line convention bothers me a
> > bit.  No other command uses --<anything> to signal that what comes
> > after it is a rev.
> >
> > But I do not think of a better alternative without making the
> > command line ambiguous, so I'll stop at raising a concern, so that
> > others who may be better at UI can come up with one.
>
> I don't really like it either, but maybe we've backed ourselves into a
> corner here.
>
> But let's look at that. So the command takes:
>
>         git check-attr <attr>... -- <path>...
>
> Or:
>
>         echo "<path>" |
>         git check-attr --stdin <attr>...
>
> So we'd want to specify a <revision>, without making the <attr> or
> <path> ambiguous.
>
> Now, when we map the attributes we go through attr_name_valid(), which
> checks that the attribute names are valid. A commentthere says:
>
>          * Attribute name cannot begin with '-' and must consist of
>          * characters from [-A-Za-z0-9_.].
>
> So can't we instead accept:
>
>         git check-attr [<rev>:]<attr>... -- <path>...
>
> I.e.:
>
>         git check-attr HEAD~:foo -- path
>
> And it wouldn't be ambiguous because attribute names can't contain ":"?
> This would be consistent with e.g. "git show" and "git cat-file", just
> with "<attr>" instead of the "<path>".
>
> This would also mean that we'd support:
>
>         git check-attr HEAD:foo HEAD~:bar HEAD~2:baz
>
> etc., i.e. the ability to support multiple revision/attribute
> pairs. Skimming the currently proposed code there seems to be no good
> reason not to support that (we just need to look up other blobs), and it
> would allow querying those without spawning N processes.

Thanks for this walkthrough, quick question, this wouldn't work with
the `-a` condition, right?
Current patch series tends to work with/without `-a`.

Also, my personal opinion is that 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.

-- 
- Karthik