Re: [PATCH 2/6] pull: make code more similar to the shell script again

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 29, 2016, 22:52 UTC
Message-ID: <xmqqr3976vcy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <9a7cc36eee651fe8be280920587e1f83538caf77.1472137582.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 24 quoted lines

> +static int require_clean_work_tree(const char *action, const char *hint,
> +		int gently)
>  {
>  	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> -	int do_die = 0;
> +	int err = 0;
>  
>  	hold_locked_index(lock_file, 0);
>  	refresh_cache(REFRESH_QUIET);
> @@ -376,20 +377,26 @@ static void die_on_unclean_work_tree(void)
>  	rollback_lock_file(lock_file);
>  
>  	if (has_unstaged_changes()) {
> -		error(_("Cannot pull with rebase: You have unstaged changes."));
> -		do_die = 1;
> +		error(_("Cannot %s: You have unstaged changes."), action);
> ...
>  		if (!autostash)
> -			die_on_unclean_work_tree();
> +			require_clean_work_tree("pull with rebase",
> +				"Please commit or stash them.", 0);
>  
>  		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
>  			hashclr(rebase_fork_point);

Splicing an English/C phrase 'pull with rebase' into a _("localizable %s string") makes the life of i18n team hard.

Can we do this differently?

If you are eventually going to expose this function as public API, I think the right approach would be to enumerate the possible error conditions this function can diagnose and return them to the caller, i.e.

    #define WT_STATUS_DIRTY_WORKTREE 01
    #define WT_STATUS_DIRTY_INDEX    02

    static int require_clean_work_tree(void)
    {
	int status = 0;
	...
        if (has_unstaged_changes())
        	status |= WT_STATUS_DIRTY_WORKTREE;
	if (has_uncommitted_changes())
        	status |= WT_STATUS_DIRTY_INDEX;
	return status;
    }

Then die_on_unclean_work_tree() can be made as a thin-wrapper that calls it and shows the pull-specific error message.