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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2016, 16:20 UTC
Message-ID: <xmqqfuvtav5k.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1603141410590.4690@virtualbox>

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

Show 14 quoted lines

>> 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.
>
> Let me summarize.
>
> First, you argue that the prefix is a documented way to say: "This
> function needs to be called from the top-level directory".

It tells the reader of this function that the caller has already moved to the top-level and as a633fca0 (Call setup_git_directory() much earlier, 2006-07-28) says, gives you the "prefix" (i.e. pathname of $PWD relative to the project root level). When you read the function, hence, you know "prefix" can be used to tell where originally the command was run from, if you want to learn it.

In other words, I didn't mean to argue anything like what you are saying. Either I phrased poorly, or you misread it.

For the purpose of letting "git pull --rebase" make sure that there is no difference between the index and the working tree, we'd want a tree-wide diff, so we do not want to limit the diff with pathspec. Not passing "prefix" to init_revisions() _is_ the right thing. Again, I think I said that in the message you are responding to, and either I phrased poorly, or you misread it.

In any case, that call to init_revisions() should have imitated what index_differs_from() and do_diff_cache() in diff-lib.c do, i.e. pass NULL as the prefix to say "I do not want any path/subdirectory limitation".

> P.S.: The idea that a rebasing pull might, at some stage in the future,
> want to require only part of the working directory to be clean, this idea
> also makes little sense to me.

And there is no such idea. Again, perhaps I phrased the whole thing poorly or you misread it under the assumption that whenever I disagree with what you say, it is more likely what I say is incorrect.

I was talking about this sequence:

    $ edit rerere.c
    ... time passes ...
    $ cd Documentation/
    $ git pull --rebase
    error: Cannot pull with rebase: You have unstaged changes.
    ... oh, where do I have changes?
    $ git status -suno
     M ../rerere.c

which you may want to optimize the human-end-user experience by allowing 'pull --rebase' to say "You have unstaged changes in ../rerere.c" or something along that line [*1*] so that the user does not have to give a separate "git status" there.

And for that, the code that gives the error message needs to be able to access the "prefix" (i.e. pathname of $PWD relative to the project root level) to make that path relative in the output.

[Footnote]

*1* By "something along that line" I mean that you'd need to work out the details of what to do when there are too many of them, for example you wouldn't list everything but indicate that the list is truncated in some way.