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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 30, 2016, 17:56 UTC
Message-ID: <alpine.DEB.2.20.1608301953340.129229@virtualbox>
In-Reply-To: <xmqqr3976vcy.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Mon, 29 Aug 2016, Junio C Hamano wrote:

Show 29 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +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.

Hrm.

> Can we do this differently?

Sure, but not at this stage. Because...

Show 21 quoted lines

> 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.

This sounds like a good plan, if involved. At this stage, I am really unwilling to introduce such extensive changes, for fear of introducing regressions.

I will keep it in mind and make those changes, once Git for Windows v2.10.0 is out.

Ciao, Dscho