Re: [PATCH 2/6] pull: make code more similar to the shell script again
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 9, 2016, 10:49 UTC
- Message-ID
- <alpine.DEB.2.20.1609091244500.129229@virtualbox>
- In-Reply-To
- <xmqqr3976vcy.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 29 Aug 2016, Junio C Hamano wrote:
Show 53 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.
>
> 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.Hrm. After thinking about this for over a week, I think that this is the wrong approach.
To introduce new wrapper functions just for the sake of being able to provide possibly dozens of different error messages seems quite a bit wrong.
I agree, however, that it may be a better idea to add a "gently" flag to require_clean_work_tree() that lets it print out a (localizable) error message and return -1 instead of die()ing.
The result would be that a failed `git pull --rebase` would then print out *two* lines: one explaining that there are unstaged changes, and one that explains that the pull did not even start due to an unclean work tree.
That solution would scale better, and I may get a chance to make those changes and send out another iteration of this patch series before October.
Ciao, Johannes