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

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

On Sat, Dec 17, 2022 at 12:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > ... to optionally allow the users
> > to check attributes against paths from older commits.
>
> The above makes it sounds as if attributes are taken from places
> that are unrelated to the "older commits" and the point of the
> change allows "paths in an older commit" to be inspected.  I do not
> think that is what is going on, though.  Isn't the point of the patch
> to take attributes definitions from arbitrary tree-ish?
>
> Also, "older commits" is misleading.  You may be chasing a bug in
> older codebase and you have a checkout of an old commit, but you
> know you have corrected the attributes definition since that old
> version.  In such a case, you may want to take the attributes from
> the latest release and apply to the currently checked out working
> tree or commit that is older.  That is checking attributes taken
> from newer commit.
>
>         ... to check attributes taken from a commit other than HEAD
>         against paths.
>
> or something, perhaps?
>

I agree with your wording, it's much better. I'll stick to it.

> > Add a new flag `--revision`/`-r` which will allow users to check the
> > attributes against a tree-ish revision.
>
> "tree-ish revision" sounds a bit strange.  We used to use "revision"
> very loosely to mean an "object name", but we weaned ourselves off
> of such a loose terminology over time.  These days, the word
> "revision" only refers to a commit (or commit-ish).
>
> I would understand "... against a tree-ish."  If you feared that
> "tree-ish" is not widely understood (which is a valid concern), then
> "... against a commit (actually any tree-ish would do)" is probably
> what I would write.
>

Thanks for explaining, I somehow keep associating revision to be the universal
set, which consists of tree, commit. I'll use your wording though.

>
> > Since we use a tree-ish object, the user can pass "-r HEAD:subdirectory"
> > and all the attributes will be looked up as if subdirectory was the root
> > directory of the repository.
>
> Is this meant to explain a feature, or a misfeature?  In other
> words, when would this be useful?  I would omit this paragraph if I
> were writing it.

It's a misfeature to be honest, I think it was called out in the
previous version
of the series. I'm happy to drop it, because I initially didn't include it.

https://lore.kernel.org/git/674caf56-940b-8130-4a5e-ea8dc4783e81@dunelm.org.uk/

>
> > We cannot use the `<rev>:<path>` syntax like the one used in `git show`
> > because any non-flag parameter before `--` is treated as an attribute
> > and any parameter after `--` is treated as a pathname.
>
> I do not see what this one wants to say.  <rev>:<path> is an
> established way to name an object that is sitting at the <path> in
> the tree-ish whose name is <rev>.  The user can use "-r
> <rev>:<path>" and if the <path> in <rev> is a tree, then all the
> attributes will be looked up as if <path> were the root.  So the
> users can use the <rev>:<path> syntax, cannot they?
>

Yes ofcourse, this one merely is stating why we cannot use `<rev>:<path>`
directly (i.e. without the --revision flag).

I'll correct it to:

    We cannot simply use the `<rev>:<path>` syntax without the `--revision`
    flag, similar to how it is used in `git show` because any non-flag
    parameter before `--` is treated as an attribute and any parameter after
    `--` is treated as a pathname.

> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > Co-authored-by: toon@iotcl.com
>
> Co-authoring is fine, but as one of the copyright holder, the other
> author needs to sign it off, too.

Can I simply add this, or does Toon need to provide his approval on this list?

-- 
- Karthik