On Sun, Sep 9, 2012 at 9:24 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Ralf Thielow <ralf.thielow@xxxxxxxxx> writes:
>
>> If "core.ignorecase" is true, "git blame" fails
>> when the given path differs to the real path in case
>> sensitivity.
>
> It is rather hard to respond to this request for comment because you
> are describing the behaviour you perceive as a problem only fuzzily
> (i.e. "fails"---what does it do?) without describing the expectation
> (i.e. what should it do instead?).
>
>> +test-path-ignorecase$X: builtin/blame.o
>
> Yuck. If it is a helper function that deserves its own unit test
> helper executable, shouldn't it live with somewhere more appropriate
> to be shared across commands?
>
>> +const char* get_path_ignorecase(const char *path)
>> +{
>> + struct strbuf res = STRBUF_INIT;
>> + struct strbuf p = STRBUF_INIT;
>> + int offset = 0;
>> +
>> + if (!ignore_case || has_string_in_work_tree(path))
>> + return path;
>
> If the problem you are trying to solve is that the HEAD and the
> history has Makefile but the user said "git blame MAKEFILE"
> (i.e. start traversing from the contents in the working tree), then
> has_string_in_work_tree("MAKEFILE") will return true, and this
> function will tell us to find the contents for "MAKEFILE" not
> "Makefile" in the next revision (i.e. HEAD:MAKEFILE).
>
> As you didn't describe what you perceive as the problem, I do not
> know if the above analysis matters for the case you are trying to
> fix, though.
>
> Step back a bit and write down what problem you are trying to solve,
> including what existing behaviour you should *not* alter.
>
> Suppose we have this history (time flows from bottom to top), and
> our HEAD is at commit F. We may or may not have modifications to
> Makefile in the working tree:
>
> F Add Makefile again
> E Remove Makefile
> D Modify Makefile
> C Remove MAKEFILE, add Makefile with related or unrelated contents
> B Modify MAKEFILE
> A Add MAKEFILE
>
> What should happen to the following?
>
> $ git blame Makefile
> $ git blame MAKEFILE
> $ git blame MakeFILE
>
> $ git blame HEAD -- Makefile
> $ git blame HEAD -- MAKEFILE
> $ git blame HEAD -- MakeFILE
>
> $ git blame E -- Makefile
> $ git blame E -- MAKEFILE
> $ git blame E -- MakeFILE
>
> $ git blame B -- Makefile
> $ git blame B -- MAKEFILE
> $ git blame B -- MakeFILE
>
> Git should see the pathname that was given by the user literally for
> any of the latter 9 cases (i.e. if we are not starting from the
> contents of the working tree) regardless of ignorecase. If the user
> tells us to blame MAKEFILE starting from B, "fixing" the path to the
> version you can read from the working tree that may be similar to
> what you have at commit F (i.e. HEAD) and turn it to a blame for
> Makefile is a wrong thing to do, no?
>
> Another potential problem with the approach your patch takes may be
> that the case insensitive filesystem you are dealing with might be
> not just case insensitive, but also be case destroying. The user
> may say "edit Makefile && git add Makefile", the filesystem may say
> there is MAKEFILE there, and core.ignorecase is designed to handle
> this case well by treating that the user really meant Makefile and
> that it is the filesystem that is lying to us.
>
> Now that we established that the "fixing" of the path we got from
> the user, should _never_ look at the working tree. Also, if such a
> "fixing" ever is useful, it should be done only in the first three
> cases where the user tells us to start blaming from the working
> tree.
>
> So what should happen to the first three cases? When doing
>
> $ git add Makefile
> $ git add MAKEFILE
> $ git add MakeFILE
>
> we infer the case the user really meant by attempting to match the
> path against what is recorded in the index. If there is no matching
> entry, we use what we got from the user, but if there is a matching
> one (and core.ignorecase affects how this matching is done), we fix
> the path by using the path recorded in the index instead. Then we
> will turn the top three cases to "git blame Makefile".
>
> That brings us back to the case where we start blaming from a commit
> (the latter 9 cases). It might make sense to grab the path in the
> tree of the named commit that matches in the core.ignorecase sense
> to the path given by the user. Even though commit F does not have
> MAKEFILE nor MakeFILE, we turn the path given by the user to Makefile
> because that is the only path that the user _could_ have meant in
> the context of the command (similarly, MAKEFILE for a blame starting
> at B). When starting to blame at E that does not have Makefile or
> MAKEFILE, we would use whatever the user threw at us.
>
> I said "might make sense" for the last paragraph for a reason,
> though. In the history A..F above, if the user is aware of the fact
> that the history _is_ case sensitive (and setting core.ignorecase is
> a signal that she does---it is an admission that the filesystem she
> happens to have her working tree is incapable of interacting with
> such a history natively and needs a bit of help from Git) and that
> some commit have Makefile and others have MAKEFILE, it will look
> inconsistent if she asked "git blame D -- MAKEFILE" and gets the
> result for "git blame D -- Makefile" instead.
>
> Having said all that, I am not sure if the "fixing" is really the
> right approach to begin with. Contrast these two:
>
> $ git blame MakeFILE
> $ git blame HEAD -- MakeFILE
>
> The latter, regardless of core.ignorecase, should fail, with "No
> such path MakeFILE in HEAD". The former is merely an extension to
> the latter, in the sense that the main traversal is exactly the same
> as the latter, but on top, local modifications are blamed to the
> working tree.
>
> If we were to do anything, I would think the most sane thing to do
> is a smaller patch to fix fake_working_tree_commit() where it calls
> lstat() and _should_ die with "Cannot lstat MakeFILE" on a sane
> filesystem. It does not currently make sure the path exists in the
> HEAD exactly as given by the user (i.e. without core.ignorecase
> matching), and die when it is not found.
>
> And that can be done regardless of core.ignorecase. Currently on a
> case sensitive filesystem without core.ignorecase, this will give a
> useless result:
>
> $ git rm Nakefile || :;
> $ git commit --allow-empty -m 'Made sure there is no Nakefile'
> $ >Nakefile
> $ git blame -- Nakefile
> 00000000 (Not Committed Yet 2012-09-09 12:21:42 -0700 1)
>
> and such a change to verify that the path exists in HEAD will give
> us "No such path Nakefile in HEAD" in such a case.
>
> It is a behaviour change, but I think it is a good change,
> regardless of the "What I have is Makefile, but my filesystem lies
> to us saying yes when I ask if MAKEFILE exists" issue.
>
Thanks for your long answer! I've learned a lot out of it.
Just to answer your question, you were right, the problem
I wanted to solve is the following:
echo "foo" > README && git add README && git commit -m "add README"
git config core.ignorecase false
git add README; # OK
git add readme; # 'fatal: pathspec 'readme' did not match any files' OK
git blame README; # has a result, OK
git blame readme; # 'fatal: cannot stat path 'readme': No such file or
directory' OK
git config core.ignorecase true
git add readme; # now there's no error here; OK
git blame readme; # still an error << NOK
git blame HEAD -- readme; # 'fatal: no such path readme in HEAD' << NOK
Thanks
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html