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

[Karthik Nayak <karthik.188@gmail.com>]

On Wed, Dec 14, 2022 at 2:28 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
>

I agree with what you're saying here. I think that's excellent advice,
too. Thanks!

>     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.
>

Furthermore, I think you've put it nice here, I will copy this over and modify
the last statement to:

    It may be beneficial to optionally allow the users to check attributes
    against paths from older commits.

> 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.
>

Yeah, this was my understanding as well, I don't think I tried to change this or
implied the same anywhere.

> >> 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.

Will leave it as it is then.

Thanks both for the review. I think I will push a version 3 now,
mostly changes would be:
1. Documentation
2. Commit message

Also, it has been a while for me on this list, but I did notice this
topic missing from the
`What's cooking in git.git` mail, do I need to do something further?

--
- Karthik