| Message ID | 20181214133852.19665-4-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dim: fix git directory evaluation | expand |
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 > >> >
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/
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(-)