Re: [PATCH] pull: drop confusing prefix parameter of die_on_unclean_work_tree()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2016, 18:09 UTC
Message-ID: <xmqqk2ladx36.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqr3fidxs3.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> I think this is quite subjective, as I tend to take the presence of
> "prefix" to mean "the callee assumes that the caller has gone up to
> the root level already", and take the absense of use of "prefix" in
> the callee to mean "the callee is working on the whole tree", and
> discarding the parameter is robbing that clue from that point of
> view.
>
> So I am mildly opposed to most parts of this change, including not
> spelling out (void) as the list of parameters for a function that
> does not take any.
>
> I do think not passing "prefix" to init_revisions() would be the
> right thing.  In fact, that prefix is copied to rev, but the current
> end result is correct _only_ because the pathspec limit given by
> that "prefix" parameter to init_revisions() is not automatically
> copied to rev_info.diffopt, and the code is very misleading.

Another reason why it is more sensible to keep the prefix available, but not use it to limit the extent of diff, to has_*_changes() functions is that it would be easier for us to change our mind later to allow the users to ask for more detailed output. Instead of "Cannot pull with rebase: You have unstaged changes, period.", you may be asked to list which paths are dirty in such a case, and in order to present the list relative to the directory where the user started the command, you would need "prefix" available to the code that calls into diff machinery somehow.