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.
--
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