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

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I've got a couple of comments below about the details of the
> implementation but the basic idea seems reasonable.
>
> On 09/12/2022 21:03, Karthik Nayak wrote:
>> Git check-attr currently doesn't check the git worktree,
>
> Normally worktree refers to the directory on disk where the
> repository's working copy is checked out. Here you seem to mean
> something else.

Strictly speaking, what you just said is "working tree".  The term
"worktree" in Git's context yet means something slightly different.
You can arrange to have multiple working trees attached to a single
repository, and each of these is called a "worktree" attached to the
repository.

In any case, thanks for pointing out that the original's wording is
wrong.  It is natural to read it to claim that we do not check the
.gitattributes files that are checked out in the working trees,
which is utterly incorrect.

>> it either
>> checks the index or the files directly.

> This means we cannot check the
> attributes for a file against a certain revision.

Whenever one is tempted to say "This means", one should realize that
one does not have absolute confidence in whatever written before it,
in other words, without additional explanation, one suspects that
what one wanted to say would not be understood.

A good piece of advice for such a person is to try rewriting WITHOUT
anything before (and including) "This means".  And I think this is a
good example to which the advice applies well.

    There is no way with "git check-attr" to apply attributes from
    .gitattributes files recorded in the same treeish to paths in a
    treeish object.

Our usual preference is to (1) start by describing the current state
and (2) propose what can be done by deviating from it, in that
order, so one might write it like so:

    The contents of the .gitattributes files may evolve over time,
    but "git check-attr" always checks attributes against them in
    the working tree and/or in the index.  It may be beneficial to
    optionally allow the version of .gitattributes found in the same
    commit when checking the attributes for paths in an older commit.

By the way, applying the attributes from the working tree is by
design and it should stay to be the default.  People are almost
always working near the tip of the history, and working tree files
are by definition ahead of any committed version---it is a feature
that users can correct attribute definitions in their working tree
files and then apply them to paths in the committed version.

>> 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.
>
> The system, global and the attributes in .git/info/attributes from the
> filesystem are still used. It would be useful to document that and
> explain in the commit message why that is useful when using -r.
>
> -r is documented as accepting a revision but actually accepts any
>  tree. That means a user can pass "-r HEAD:subdirectory" and all the
>  attributes will be looked up as if subdirectory was the root
>  directory of the repository which might be confusing. It would be
>  helpful to know if passing a tree rather than a revision is
>  useful. If it isn't then you could use lookup_commit_reference() to
> ensure the user passes a revision.

Unless you use ancestry relationships in any way [*], you do not
want to require commits when an operation only requires trees.  In
this case, taking tree-ish and documenting it as such is the right
thing to do.

[Footnote]

* A good example that makes sense to limit to commit-ishes is when
  merging two histories (without requiring the user to supply the
  merge-base). You'd need to compute the merge-bases, so you require
  two committishes and it is not enough to take two trees.