[maintainer-tools,RFC,3/3] dim: fix rr_cache_dir discovery

Submitted by Andrzej Hajda on Dec. 14, 2018, 1:38 p.m.

Details

Message ID 20181214133852.19665-4-a.hajda@samsung.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Andrzej Hajda Dec. 14, 2018, 1:38 p.m.
rr_cache_dir function cannot assume REPO/.git is a directory. On the other
side it should be backward compatible - if rr-cache directory/link already
exists it should be returned.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi,

I am not sure of the purpose of rr-cache symbolic link, dim does not use
it (except its creation/removal). So this patch should be verified by
someone who knows better what is going on here.

Regards
Andrzej
---
 dim | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/dim b/dim
index 3afa8b6..b72ebfd 100755
--- a/dim
+++ b/dim
@@ -554,15 +554,6 @@  function check_conflicts # tree
 	true
 }
 
-function rr_cache_dir
-{
-	if [ -d $DIM_PREFIX/drm-tip/.git/ ] ; then
-		echo $DIM_PREFIX/drm-tip/.git/rr-cache
-	else
-		echo $DIM_PREFIX/$DIM_REPO/.git/rr-cache
-	fi
-}
-
 function git_dir
 {
 	local dir=${1:-$PWD}
@@ -574,6 +565,17 @@  function git_dir
 	fi
 }
 
+function rr_cache_dir
+{
+	local dir=$(git_dir $DIM_PREFIX/$DIM_REPO)/rr-cache
+
+	if [ -d $dir ]; then
+		echo $dir
+	else
+		echo $(git_dir $DIM_PREFIX/drm-tip)/rr-cache
+	fi
+}
+
 function pull_rerere_cache
 {
 	cd $DIM_PREFIX/drm-rerere/

Comments

On Fri, Dec 14, 2018 at 02:38:52PM +0100, Andrzej Hajda wrote:
> rr_cache_dir function cannot assume REPO/.git is a directory. On the other
> side it should be backward compatible - if rr-cache directory/link already
> exists it should be returned.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> Hi,
> 
> I am not sure of the purpose of rr-cache symbolic link, dim does not use
> it (except its creation/removal). So this patch should be verified by
> someone who knows better what is going on here.
> 
> Regards
> Andrzej
> ---
>  dim | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/dim b/dim
> index 3afa8b6..b72ebfd 100755
> --- a/dim
> +++ b/dim
> @@ -554,15 +554,6 @@ function check_conflicts # tree
>  	true
>  }
>  
> -function rr_cache_dir
> -{
> -	if [ -d $DIM_PREFIX/drm-tip/.git/ ] ; then
> -		echo $DIM_PREFIX/drm-tip/.git/rr-cache
> -	else
> -		echo $DIM_PREFIX/$DIM_REPO/.git/rr-cache
> -	fi
> -}

I think this breaks it, rr-cache is shared among all worktrees (which is a
big reason for having them).

And yes dim only sets up the symlink, dim rebuild-tip uses the rr-cache
stuff automatically through git merge.
-Daniel

> -
>  function git_dir
>  {
>  	local dir=${1:-$PWD}
> @@ -574,6 +565,17 @@ function git_dir
>  	fi
>  }
>  
> +function rr_cache_dir
> +{
> +	local dir=$(git_dir $DIM_PREFIX/$DIM_REPO)/rr-cache
> +
> +	if [ -d $dir ]; then
> +		echo $dir
> +	else
> +		echo $(git_dir $DIM_PREFIX/drm-tip)/rr-cache
> +	fi
> +}
> +
>  function pull_rerere_cache
>  {
>  	cd $DIM_PREFIX/drm-rerere/
> -- 
> 2.17.1
> 
> _______________________________________________
> dim-tools mailing list
> dim-tools@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools
Hi Daniel,

Thanks for reviewing other two patches.


On 14.12.2018 17:29, Daniel Vetter wrote:
> On Fri, Dec 14, 2018 at 02:38:52PM +0100, Andrzej Hajda wrote:
>> rr_cache_dir function cannot assume REPO/.git is a directory. On the other
>> side it should be backward compatible - if rr-cache directory/link already
>> exists it should be returned.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>> Hi,
>>
>> I am not sure of the purpose of rr-cache symbolic link, dim does not use
>> it (except its creation/removal). So this patch should be verified by
>> someone who knows better what is going on here.
>>
>> Regards
>> Andrzej
>> ---
>>  dim | 20 +++++++++++---------
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/dim b/dim
>> index 3afa8b6..b72ebfd 100755
>> --- a/dim
>> +++ b/dim
>> @@ -554,15 +554,6 @@ function check_conflicts # tree
>>  	true
>>  }
>>  
>> -function rr_cache_dir
>> -{
>> -	if [ -d $DIM_PREFIX/drm-tip/.git/ ] ; then
>> -		echo $DIM_PREFIX/drm-tip/.git/rr-cache
>> -	else
>> -		echo $DIM_PREFIX/$DIM_REPO/.git/rr-cache
>> -	fi
>> -}
> I think this breaks it, rr-cache is shared among all worktrees (which is a
> big reason for having them).


If drm-tip and $DIM_REPO are worktrees (more exactly "linked working
tree" - ie .git is a file), then rr_cache_dir will return invalid path.

As a result update_rerere_cache will fail create symlink, with this kind
of error:

    ln: failed to create symbolic link '/lab/dim/drm-misc-next/.git':
File exists

> And yes dim only sets up the symlink, dim rebuild-tip uses the rr-cache
> stuff automatically through git merge.


I still do not see who and when is using (ie. reading) this link.
rebuild_tip seems to use only $DIM_PREFIX/drm_rerere/rr-cache. Is there
some magic inside git itself, or I am just blind?


Regards

Andrzej



> -Daniel
>
>> -
>>  function git_dir
>>  {
>>  	local dir=${1:-$PWD}
>> @@ -574,6 +565,17 @@ function git_dir
>>  	fi
>>  }
>>  
>> +function rr_cache_dir
>> +{
>> +	local dir=$(git_dir $DIM_PREFIX/$DIM_REPO)/rr-cache
>> +
>> +	if [ -d $dir ]; then
>> +		echo $dir
>> +	else
>> +		echo $(git_dir $DIM_PREFIX/drm-tip)/rr-cache
>> +	fi
>> +}
>> +
>>  function pull_rerere_cache
>>  {
>>  	cd $DIM_PREFIX/drm-rerere/
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> dim-tools mailing list
>> dim-tools@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dim-tools
On Mon, Dec 17, 2018 at 10:54:48AM +0100, Andrzej Hajda wrote:
> Hi Daniel,
> 
> Thanks for reviewing other two patches.
> 
> 
> On 14.12.2018 17:29, Daniel Vetter wrote:
> > On Fri, Dec 14, 2018 at 02:38:52PM +0100, Andrzej Hajda wrote:
> >> rr_cache_dir function cannot assume REPO/.git is a directory. On the other
> >> side it should be backward compatible - if rr-cache directory/link already
> >> exists it should be returned.
> >>
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >> ---
> >> Hi,
> >>
> >> I am not sure of the purpose of rr-cache symbolic link, dim does not use
> >> it (except its creation/removal). So this patch should be verified by
> >> someone who knows better what is going on here.
> >>
> >> Regards
> >> Andrzej
> >> ---
> >>  dim | 20 +++++++++++---------
> >>  1 file changed, 11 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/dim b/dim
> >> index 3afa8b6..b72ebfd 100755
> >> --- a/dim
> >> +++ b/dim
> >> @@ -554,15 +554,6 @@ function check_conflicts # tree
> >>  	true
> >>  }
> >>  
> >> -function rr_cache_dir
> >> -{
> >> -	if [ -d $DIM_PREFIX/drm-tip/.git/ ] ; then
> >> -		echo $DIM_PREFIX/drm-tip/.git/rr-cache
> >> -	else
> >> -		echo $DIM_PREFIX/$DIM_REPO/.git/rr-cache
> >> -	fi
> >> -}
> > I think this breaks it, rr-cache is shared among all worktrees (which is a
> > big reason for having them).
> 
> 
> If drm-tip and $DIM_REPO are worktrees (more exactly "linked working
> tree" - ie .git is a file), then rr_cache_dir will return invalid path.
> 
> As a result update_rerere_cache will fail create symlink, with this kind
> of error:
> 
>     ln: failed to create symbolic link '/lab/dim/drm-misc-next/.git':
> File exists

Ah, now I get it, your $DIM_REPO is also a worktree. Yes that's not
supported with the current code.

But your code doesn't fix it. We do _not_ want $(git_dir)/rr-cache here,
but really the .git/rr-cache of the master repo. The worktrees do not have
their own private rr-cache, but use the one of the master repo.

> > And yes dim only sets up the symlink, dim rebuild-tip uses the rr-cache
> > stuff automatically through git merge.
> 
> 
> I still do not see who and when is using (ie. reading) this link.
> rebuild_tip seems to use only $DIM_PREFIX/drm_rerere/rr-cache. Is there
> some magic inside git itself, or I am just blind?

git merge --rerere-autoupdate uses it internally. We want that drm-tip
uses the rr-cache we store in drm-rerere/rr-cache.

Maybe $(git_dir drm-tip)/../../rr-cache works? Again, the important part
is that we edit the .git/rr-cache in your master repo, not in any of the
worktrees (rr-cache doesn't exist there).
-Daniel

> 
> 
> Regards
> 
> Andrzej
> 
> 
> 
> > -Daniel
> >
> >> -
> >>  function git_dir
> >>  {
> >>  	local dir=${1:-$PWD}
> >> @@ -574,6 +565,17 @@ function git_dir
> >>  	fi
> >>  }
> >>  
> >> +function rr_cache_dir
> >> +{
> >> +	local dir=$(git_dir $DIM_PREFIX/$DIM_REPO)/rr-cache
> >> +
> >> +	if [ -d $dir ]; then
> >> +		echo $dir
> >> +	else
> >> +		echo $(git_dir $DIM_PREFIX/drm-tip)/rr-cache
> >> +	fi
> >> +}
> >> +
> >>  function pull_rerere_cache
> >>  {
> >>  	cd $DIM_PREFIX/drm-rerere/
> >> -- 
> >> 2.17.1
> >>
> >> _______________________________________________
> >> dim-tools mailing list
> >> dim-tools@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dim-tools
> 
>
On 17.12.2018 15:46, Daniel Vetter wrote:
> On Mon, Dec 17, 2018 at 10:54:48AM +0100, Andrzej Hajda wrote:
>> Hi Daniel,
>>
>> Thanks for reviewing other two patches.
>>
>>
>> On 14.12.2018 17:29, Daniel Vetter wrote:
>>> On Fri, Dec 14, 2018 at 02:38:52PM +0100, Andrzej Hajda wrote:
>>>> rr_cache_dir function cannot assume REPO/.git is a directory. On the other
>>>> side it should be backward compatible - if rr-cache directory/link already
>>>> exists it should be returned.
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>> Hi,
>>>>
>>>> I am not sure of the purpose of rr-cache symbolic link, dim does not use
>>>> it (except its creation/removal). So this patch should be verified by
>>>> someone who knows better what is going on here.
>>>>
>>>> Regards
>>>> Andrzej
>>>> ---
>>>>  dim | 20 +++++++++++---------
>>>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/dim b/dim
>>>> index 3afa8b6..b72ebfd 100755
>>>> --- a/dim
>>>> +++ b/dim
>>>> @@ -554,15 +554,6 @@ function check_conflicts # tree
>>>>  	true
>>>>  }
>>>>  
>>>> -function rr_cache_dir
>>>> -{
>>>> -	if [ -d $DIM_PREFIX/drm-tip/.git/ ] ; then
>>>> -		echo $DIM_PREFIX/drm-tip/.git/rr-cache
>>>> -	else
>>>> -		echo $DIM_PREFIX/$DIM_REPO/.git/rr-cache
>>>> -	fi
>>>> -}
>>> I think this breaks it, rr-cache is shared among all worktrees (which is a
>>> big reason for having them).
>>
>> If drm-tip and $DIM_REPO are worktrees (more exactly "linked working
>> tree" - ie .git is a file), then rr_cache_dir will return invalid path.
>>
>> As a result update_rerere_cache will fail create symlink, with this kind
>> of error:
>>
>>     ln: failed to create symbolic link '/lab/dim/drm-misc-next/.git':
>> File exists
> Ah, now I get it, your $DIM_REPO is also a worktree. Yes that's not
> supported with the current code.
>
> But your code doesn't fix it. We do _not_ want $(git_dir)/rr-cache here,
> but really the .git/rr-cache of the master repo. The worktrees do not have
> their own private rr-cache, but use the one of the master repo.
>
>>> And yes dim only sets up the symlink, dim rebuild-tip uses the rr-cache
>>> stuff automatically through git merge.
>>
>> I still do not see who and when is using (ie. reading) this link.
>> rebuild_tip seems to use only $DIM_PREFIX/drm_rerere/rr-cache. Is there
>> some magic inside git itself, or I am just blind?
> git merge --rerere-autoupdate uses it internally. We want that drm-tip
> uses the rr-cache we store in drm-rerere/rr-cache.
>
> Maybe $(git_dir drm-tip)/../../rr-cache works? Again, the important part
> is that we edit the .git/rr-cache in your master repo, not in any of the
> worktrees (rr-cache doesn't exist there).


I think the proper way of finding rr-cache would be:


function rr_cache_dir
{
	echo $(git rev-parse --git-common-dir)/rr-cache
}


If you are OK with it I can prepare patch. In fact since the function is
used only in update_rerere_cache, it could be squashed there:

function update_rerere_cache
{
        echo -n "Updating rerere cache... "
        pull_rerere_cache

        local rr_cache_dir=$(git rev-parse --git-common-dir)/rr-cache

        ...

}


Is this approach OK?


Regards

Andrzej



> -Daniel
>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>
>>> -Daniel
>>>
>>>> -
>>>>  function git_dir
>>>>  {
>>>>  	local dir=${1:-$PWD}
>>>> @@ -574,6 +565,17 @@ function git_dir
>>>>  	fi
>>>>  }
>>>>  
>>>> +function rr_cache_dir
>>>> +{
>>>> +	local dir=$(git_dir $DIM_PREFIX/$DIM_REPO)/rr-cache
>>>> +
>>>> +	if [ -d $dir ]; then
>>>> +		echo $dir
>>>> +	else
>>>> +		echo $(git_dir $DIM_PREFIX/drm-tip)/rr-cache
>>>> +	fi
>>>> +}
>>>> +
>>>>  function pull_rerere_cache
>>>>  {
>>>>  	cd $DIM_PREFIX/drm-rerere/
>>>> -- 
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> dim-tools mailing list
>>>> dim-tools@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dim-tools
>>
On Tue, Dec 18, 2018 at 8:15 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 17.12.2018 15:46, Daniel Vetter wrote:
> > On Mon, Dec 17, 2018 at 10:54:48AM +0100, Andrzej Hajda wrote:
> >> Hi Daniel,
> >>
> >> Thanks for reviewing other two patches.
> >>
> >>
> >> On 14.12.2018 17:29, Daniel Vetter wrote:
> >>> On Fri, Dec 14, 2018 at 02:38:52PM +0100, Andrzej Hajda wrote:
> >>>> rr_cache_dir function cannot assume REPO/.git is a directory. On the other
> >>>> side it should be backward compatible - if rr-cache directory/link already
> >>>> exists it should be returned.
> >>>>
> >>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >>>> ---
> >>>> Hi,
> >>>>
> >>>> I am not sure of the purpose of rr-cache symbolic link, dim does not use
> >>>> it (except its creation/removal). So this patch should be verified by
> >>>> someone who knows better what is going on here.
> >>>>
> >>>> Regards
> >>>> Andrzej
> >>>> ---
> >>>>  dim | 20 +++++++++++---------
> >>>>  1 file changed, 11 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/dim b/dim
> >>>> index 3afa8b6..b72ebfd 100755
> >>>> --- a/dim
> >>>> +++ b/dim
> >>>> @@ -554,15 +554,6 @@ function check_conflicts # tree
> >>>>    true
> >>>>  }
> >>>>
> >>>> -function rr_cache_dir
> >>>> -{
> >>>> -  if [ -d $DIM_PREFIX/drm-tip/.git/ ] ; then
> >>>> -          echo $DIM_PREFIX/drm-tip/.git/rr-cache
> >>>> -  else
> >>>> -          echo $DIM_PREFIX/$DIM_REPO/.git/rr-cache
> >>>> -  fi
> >>>> -}
> >>> I think this breaks it, rr-cache is shared among all worktrees (which is a
> >>> big reason for having them).
> >>
> >> If drm-tip and $DIM_REPO are worktrees (more exactly "linked working
> >> tree" - ie .git is a file), then rr_cache_dir will return invalid path.
> >>
> >> As a result update_rerere_cache will fail create symlink, with this kind
> >> of error:
> >>
> >>     ln: failed to create symbolic link '/lab/dim/drm-misc-next/.git':
> >> File exists
> > Ah, now I get it, your $DIM_REPO is also a worktree. Yes that's not
> > supported with the current code.
> >
> > But your code doesn't fix it. We do _not_ want $(git_dir)/rr-cache here,
> > but really the .git/rr-cache of the master repo. The worktrees do not have
> > their own private rr-cache, but use the one of the master repo.
> >
> >>> And yes dim only sets up the symlink, dim rebuild-tip uses the rr-cache
> >>> stuff automatically through git merge.
> >>
> >> I still do not see who and when is using (ie. reading) this link.
> >> rebuild_tip seems to use only $DIM_PREFIX/drm_rerere/rr-cache. Is there
> >> some magic inside git itself, or I am just blind?
> > git merge --rerere-autoupdate uses it internally. We want that drm-tip
> > uses the rr-cache we store in drm-rerere/rr-cache.
> >
> > Maybe $(git_dir drm-tip)/../../rr-cache works? Again, the important part
> > is that we edit the .git/rr-cache in your master repo, not in any of the
> > worktrees (rr-cache doesn't exist there).
>
>
> I think the proper way of finding rr-cache would be:
>
>
> function rr_cache_dir
> {
>         echo $(git rev-parse --git-common-dir)/rr-cache
> }

Indeed. Also just noticed that rev-parse also has a --git-dir option.
Care to also change git_dir() to use that?

> If you are OK with it I can prepare patch. In fact since the function is
> used only in update_rerere_cache, it could be squashed there:
>
> function update_rerere_cache
> {
>         echo -n "Updating rerere cache... "
>         pull_rerere_cache
>
>         local rr_cache_dir=$(git rev-parse --git-common-dir)/rr-cache
>
>         ...
>
> }

Yeah makes sense.
-Daniel

>
>
> Is this approach OK?
>
>
> Regards
>
> Andrzej
>
>
>
> > -Daniel
> >
> >>
> >> Regards
> >>
> >> Andrzej
> >>
> >>
> >>
> >>> -Daniel
> >>>
> >>>> -
> >>>>  function git_dir
> >>>>  {
> >>>>    local dir=${1:-$PWD}
> >>>> @@ -574,6 +565,17 @@ function git_dir
> >>>>    fi
> >>>>  }
> >>>>
> >>>> +function rr_cache_dir
> >>>> +{
> >>>> +  local dir=$(git_dir $DIM_PREFIX/$DIM_REPO)/rr-cache
> >>>> +
> >>>> +  if [ -d $dir ]; then
> >>>> +          echo $dir
> >>>> +  else
> >>>> +          echo $(git_dir $DIM_PREFIX/drm-tip)/rr-cache
> >>>> +  fi
> >>>> +}
> >>>> +
> >>>>  function pull_rerere_cache
> >>>>  {
> >>>>    cd $DIM_PREFIX/drm-rerere/
> >>>> --
> >>>> 2.17.1
> >>>>
> >>>> _______________________________________________
> >>>> dim-tools mailing list
> >>>> dim-tools@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dim-tools
> >>
>