Re: [dim PATCH 2/6] dim: url_to_remote can't normally fail

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 03 Oct 2017, Jani Nikula <jani.nikula@intel.com> wrote:
> On Tue, 03 Oct 2017, Jani Nikula <jani.nikula@intel.com> wrote:
>> Since commit cad37e1910f9 ("dim: auto-add remotes") url_to_remote can't
>> really fail. Same for repo_to_remote when the repo exists. Redirecting
>> their output when the remote isn't there leads to url_to_remote waiting
>> for user input without prompting, giving an appearance of a hang.
>
> Wah. I've been going back and forth with this, but I'm now leaning
> towards reverting cad37e1910f9 ("dim: auto-add remotes") and going back
> to the drawing board with it.
>
> The problem is, url_to_remote, repo_to_remote and friends are low-level
> helpers in the script. That kind of things are better off returning
> true/false status instead of going out of their way to interactively fix
> stuff up. The *caller*, where applicable, should auto-add remotes,
> depending on the case.
>
> I hit the problem first on one machine which doesn't use worktrees. I
> conceded we could make worktrees a requirement. However, I have a strict
> separation between maintainer and developer repos, with no push access
> at all to the remotes from the developer repo. I have no desire to give
> up on that safety measure. However, dim is helpful in the developer repo
> too.
>
> Also, there's been requests to move dim more towards working in the CWD
> instead of cd to predefined repo. I'm not sure it's reasonable to
> require that all of them have all of the remotes available, for no
> particularly good reason. With worktrees the remotes are shared, but if
> you don't use a common tree for all of them, that benefit is lost.

Okay, I admit defeat. I don't have the time or chrystal clear idea how
to fix all this cleanly and nicely, and we can't block on this
indefinitely. One of the problems with tab completion was that
list-upstreams operated in the current dir; I sent a fix on top of this
series. In general, I think we need a better idea on what should work
and operate in CWD. The mixture we have now is not ideal.

So if you would be so kind to review this set so we can move forward,
and let's improve upon this gradually as issues arise.

BR,
Jani.


>
> BR,
> Jani.
>
>
>
>>
>> While at it, change the exit to a return. set -e at the top takes care
>> of aborting.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  dim | 24 ++++++++----------------
>>  1 file changed, 8 insertions(+), 16 deletions(-)
>>
>> diff --git a/dim b/dim
>> index 7832ddca692c..ae8f30b8db83 100755
>> --- a/dim
>> +++ b/dim
>> @@ -282,7 +282,7 @@ function url_to_remote # url [url ...]
>>  		echoerr "Please set it up yourself using:"
>>  		echoerr "    $ git remote add <name> $url"
>>  		echoerr "with a name of your choice."
>> -		exit 1
>> +		return 1
>>  	fi
>>  
>>  	git remote add $remote $url
>> @@ -1749,10 +1749,7 @@ function dim_list_upstreams
>>  {
>>  	local dim_drm_upstream_remote
>>  
>> -	# Handle failures gracefully
>> -	if ! dim_drm_upstream_remote=$(url_to_remote $drm_upstream_git 2>/dev/null); then
>> -		return 0
>> -	fi
>> +	dim_drm_upstream_remote=$(url_to_remote $drm_upstream_git)
>>  
>>  	echo origin/master
>>  	echo $dim_drm_upstream_remote/drm-next
>> @@ -1772,17 +1769,14 @@ function dim_update_branches
>>  
>>  	cd $DIM_PREFIX/$DIM_DRM_INTEL
>>  
>> -	if remote=$(url_to_remote $linux_upstream_git 2>/dev/null); then
>> -		echo -n "Fetching linux (local remote $remote)... "
>> -		git_fetch_helper $remote
>> -		echo "Done."
>> -	fi
>> +	remote=$(url_to_remote $linux_upstream_git)
>> +	echo -n "Fetching linux (local remote $remote)... "
>> +	git_fetch_helper $remote
>> +	echo "Done."
>>  
>>  	for repo in "${!drm_tip_repos[@]}"; do
>>  		url_list=${drm_tip_repos[$repo]}
>> -		if ! remote=$(url_to_remote $url_list 2>/dev/null); then
>> -			continue
>> -		fi
>> +		remote=$(url_to_remote $url_list)
>>  		echo -n "Fetching $repo (local remote $remote)... "
>>  		git_fetch_helper $remote
>>  		echo "Done."
>> @@ -1826,9 +1820,7 @@ function dim_status
>>  
>>  	for branch in $dim_branches ; do
>>  		repo=$(branch_to_repo $branch)
>> -		if ! remote=$(repo_to_remote $repo) ; then
>> -			continue
>> -		fi
>> +		remote=$(repo_to_remote $repo)
>>  
>>  		patches=$(git log --oneline $remote/$branch ^origin/master ^$drm_remote/drm-next ^$drm_remote/drm-fixes | wc -l)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx