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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2016, 17:54 UTC
Message-ID: <xmqqr3fidxs3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <974d0bfed38e8aa410e97e05022bc5dbbd78d915.1457615785.git.johannes.schindelin@gmx.de>

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

Show 10 quoted lines

> In cmd_pull(), when verifying that there are no changes preventing a
> rebasing pull, we diligently pass the prefix parameter to the
> die_on_unclean_work_tree() function which in turn diligently passes it
> to the has_unstaged_changes() and has_uncommitted_changes() functions.
>
> The casual reader might now be curious (as this developer was) whether
> that means that calling `git pull --rebase` in a subdirectory will
> ignore unstaged changes in other parts of the working directory. And be
> puzzled that `git pull --rebase` (correctly) complains about those
> changes outside of the current directory.

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.