From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 22 Dec 2022 14:36:38 GMT
Subject: Re: [PATCH v4 2/2] attr: add flag `--revision` to work with revisions
Message-ID: <1d40c799-e98a-2f44-9edd-795ade2b189f@dunelm.org.uk>
In-Reply-To: <CAPig+cTHJzHsUum0RnyCcqxj1qhxJR9EcKRjoShCznQ_oXYHOA@mail.gmail.com>

Hi Eric

On 21/12/2022 21:13, Eric Sunshine wrote:
> On Wed, Dec 21, 2022 at 3:58 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 21/12/2022 13:47, Karthik Nayak wrote:
>>> Since we use a tree-ish object, the user can pass "--revision
>>> HEAD:subdirectory" and all the attributes will be looked up as if
>>> subdirectory was the root directory of the repository.
>>
>> We should be clear in the documentation and option help that --revision
>> takes a tree-ish (i.e. --revision=<tree-ish>). Maybe calling the option
>> --tree would be clearer.
> 
> If that's the case, shouldn't this be called --source (or some
> variation thereof) for consistency[1] with git-restore?

Yes, that's a good suggestion, sorry I'd forgotten that you'd mentioned 
that before.

> [1]: https://lore.kernel.org/git/CAPig+cT_xz3m-3kkL_Scf0opNXkikU94kSF46VO8KHkNMWbGzg@mail.gmail.com/
> 
>>> +attr_check_revision () {
>>> +     path="$1" expect="$2" revision="$3" git_opts="$4" &&
>>
>> Is that line valid posix shell? I know it works with local (which is not
>> in posix) but is it allowed for global variables?
> 
> This is fine as-is; it's POSIX.

Thanks for clarifying that

Best Wishes

Phillip