git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions

From
Karthik Nayak <karthik.188@gmail.com>
Date
Dec 7, 2022, 12:33 UTC
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:

Show 64 quoted lines
>
>
> 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
Previous: Ævar Arnfjörð BjarmasonNext: Philip Oakley
Message 13 of 17 in “check-attr: add support to work with revisions”
  1. 0/2 check-attr: add support to work with revisionsKarthik Nayak, Dec 6, 2022
  2. 1/2 t0003: move setup for `--all` into new blockKarthik Nayak, Dec 6, 2022
  3. 2/2 attr: add flag `-r|--revisions` to work with revisionsKarthik Nayak, Dec 6, 2022
  4. Ævar Arnfjörð BjarmasonDec 6, 2022
  5. Karthik NayakDec 6, 2022
  6. Junio C HamanoDec 7, 2022
  7. Eric SunshineDec 7, 2022
  8. Karthik NayakDec 7, 2022
  9. Karthik NayakDec 7, 2022
  10. Ævar Arnfjörð BjarmasonDec 7, 2022
  11. Karthik NayakDec 7, 2022
  12. Ævar Arnfjörð BjarmasonDec 7, 2022
  13. Karthik NayakDec 7, 2022
  14. Philip OakleyDec 6, 2022
  15. Karthik NayakDec 6, 2022
  16. Taylor BlauDec 7, 2022
  17. brian m. carlsonDec 7, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.