Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository

[John Cai <johncai86@gmail.com>]

Hi Junio,

On 14 Mar 2023, at 16:37, Junio C Hamano wrote:

> John Cai <johncai86@gmail.com> writes:
>
>>> Just for illustration, here is one way to do so.
>>>
>>> The implementation goes in the opposite direction from the more
>>> recent trend, which is why I am not making it an official patch, but
>>
>> Could you explain why this goes against the "more recent trend" for my
>> understanding?
>
> The illustration uses a global state.
>
> The recent trend is to reduce reliance on global states and use the
> repository object and others that hold such state through the
> callchain.

Ah I see--in that case, what would be a good object to put this state into? Mabe
repo_settings?

>
> But a new global variable that holds the fallback tree-ish object name
> was a so convenient way to illustrate the core of the idea, without
> having to change many callchains.
>
>>> with this you can do things like:
>>>
>>>   $ git --attr-source=e83c5163 check-attr whitespace cache.h
>>>   cache.h: whitespace: unspecified
>>>   $ git --attr-source=e2f6331a142^ check-attr whitespace cache.h
>>>   cache.h: whitespace: set
>>>   $ git --attr-source=HEAD check-attr whitespace cache.h
>>>   cache.h: whitespace: indent,trail,space
>>
>> I like the idea of an option that is global. For git-check-attr however, we
>
> I guess I shouldn't have used check-attr to avoid confusion.
>
> The point is that the internal mechanisms introduced by 47cfc9bd
> (attr: add flag `--source` to work with tree-ish, 2023-01-14), which
> taught check-attr the --source option and is reused by this
> illustration patch, was a good idea, but its UI was a mistake.  We
> do not need per-command --source option the commit adds if we did
> the global option from day one.  Yes, I think we can deprecate the
> "--source" option from there, if we all prefer the global option
> avenue.  I _think_ git_all_attrs() needs to be told about the
> default attr-source trick (which I didn't touch in my illustration
> patch) before it happens, though.

Good point. I think to have a global flag would be a better user experience.

>
> If you used the mechanism in the illustration patch I gave you, and
> adjusted the test part of your patch to match (i.e. "diff" does not
> learn "--attr-source" option, but "git --attr-source=... diff" is
> how you make it read attributes from a tree-ish), would the result
> work well, or do we need more work to make it usable?  How well do
> other commands (e.g. "git show") work in the same test repository
> you created in your version?

I think it works pretty smoothly. I adjusted my tests to work with this git
flag, and I also tested git show and it works well.

In fact, your proposal would serve the needs of my patch independently of any of
the diff code, which is nice.

>
> Thanks.

Thanks,
John