[PATCH/RFC] blame: respect "core.ignorecase"

[PATCH/RFC] blame: respect "core.ignorecase"

[Ralf Thielow]

If "core.ignorecase" is true, "git blame" fails
when the given path differs to the real path in case
sensitivity.

Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
---
 .gitignore             |  1 +
 Makefile               |  3 +++
 builtin/blame.c        | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test-path-ignorecase.c | 27 +++++++++++++++++++++++
 4 Dateien geändert, 89 Zeilen hinzugefügt(+)
 create mode 100644 test-path-ignorecase.c

diff --git a/.gitignore b/.gitignore
index bb5c91e..65ab9f6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -195,6 +195,7 @@
 /test-sigchain
 /test-subprocess
 /test-svn-fe
+/test-path-ignorecase
 /common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index 6b0c961..dbdd214 100644
--- a/Makefile
+++ b/Makefile
@@ -503,6 +503,7 @@ TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-subprocess
 TEST_PROGRAMS_NEED_X += test-svn-fe
+TEST_PROGRAMS_NEED_X += test-path-ignorecase
 
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
@@ -2562,6 +2563,8 @@ test-parse-options$X: parse-options.o parse-options-cb.o
 
 test-svn-fe$X: vcs-svn/lib.a
 
+test-path-ignorecase$X: builtin/blame.o
+
 .PRECIOUS: $(TEST_OBJS)
 
 test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)
diff --git a/builtin/blame.c b/builtin/blame.c
index 0d50273..895f665 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1937,6 +1937,61 @@ static int has_string_in_work_tree(const char *path)
 	return !lstat(path, &st);
 }
 
+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;
+
+	for (;;) {
+		char c = path[offset++];
+
+		if (is_dir_sep(c) || c == '\0') {
+			DIR *dir;
+
+			if (res.len)
+				dir = opendir(res.buf);
+			else
+				dir = opendir(".");
+
+			if (dir != NULL) {
+				for (;;) {
+					struct dirent *ent = readdir(dir);
+
+					if (ent == NULL )
+						break;
+
+					if (!strcmp(".", ent->d_name) || !strcmp("..", ent->d_name))
+						continue;
+
+					if (!strcmp(p.buf, ent->d_name))
+						break;
+
+					if (!strcasecmp(p.buf, ent->d_name)) {
+						strbuf_release(&p);
+						strbuf_add(&p, ent->d_name, strlen(ent->d_name));
+						break;
+					}
+				}
+				closedir(dir);
+			}
+
+			strbuf_addch(&p, c);
+			strbuf_addbuf(&res, &p);
+			strbuf_release(&p);
+		} else {
+			strbuf_addch(&p, c);
+		}
+
+		if (c == '\0')
+			break;
+	}
+	return res.buf;
+}
+
 static unsigned parse_score(const char *arg)
 {
 	char *end;
@@ -2448,6 +2503,7 @@ parse_done:
 			/* FALLTHROUGH */
 		case 1: /* (1a) */
 			path = add_prefix(prefix, argv[--argc]);
+			path = get_path_ignorecase(path);
 			argv[argc] = NULL;
 			break;
 		default:
@@ -2457,8 +2513,10 @@ parse_done:
 		if (argc < 2)
 			usage_with_options(blame_opt_usage, options);
 		path = add_prefix(prefix, argv[argc - 1]);
+		path = get_path_ignorecase(path);
 		if (argc == 3 && !has_string_in_work_tree(path)) { /* (2b) */
 			path = add_prefix(prefix, argv[1]);
+			path = get_path_ignorecase(path);
 			argv[1] = argv[2];
 		}
 		argv[argc - 1] = "--";
diff --git a/test-path-ignorecase.c b/test-path-ignorecase.c
new file mode 100644
index 0000000..1f17f5c
--- /dev/null
+++ b/test-path-ignorecase.c
@@ -0,0 +1,27 @@
+#include "cache.h"
+
+static const char *usage_msg = "\n"
+		"test-similar-path <path>...\n";
+const char* get_path_ignorecase(const char *path);
+
+int main(int argc, char **argv)
+{
+	char *p;
+	const char *np;
+
+	if (argc != 2) {
+		usage(usage_msg);
+		return 1;
+	}
+
+	argv++;
+	p = *argv;
+
+	ignore_case = 1;
+
+	np = get_path_ignorecase(p);
+
+	printf("%s\n", np);
+
+	return 0;
+}
-- 
1.7.12.1.gfe115d7

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Johannes Sixt]

Am 09.09.2012 19:01, schrieb Ralf Thielow:
> If "core.ignorecase" is true, "git blame" fails
> when the given path differs to the real path in case
> sensitivity.
...
> +				dir = opendir(res.buf);
...
> +					struct dirent *ent = readdir(dir);
...
> +					if (!strcasecmp(p.buf, ent->d_name)) {

I don't think this is how core.ignorecase is intended to work. It does
not trigger case-insensitive lookup in the worktree (this is still done
by the OS), but it does a case-insensitive lookup in the index. That is,
given a worktree entry 'readme', it allows to find the index entry 'README'.

IMO, it is only correct that 'git blame file' operates on 'FILE' if
'FILE' exists in the index, and in only that case, it does not matter
how the 'file' is named in the worktree.

-- Hannes

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> 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.
    

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Junio C Hamano]

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

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

Perhaps like this (again, totally untested).

A few points to note:

 - If the "Nakefile" is a "new file" with substantial contents, the
   result I labelled as "useless" in the previous message _might_
   have been seen as useful by some user; it might be a regression
   in that sense, but then there is fundamentally no way to give
   sensible behaviour to core.ignorecase users.

 - We used to say get_sha1("HEAD"), but that is not a very good
   practice; even though we know DWIM will find the .git/HEAD, make
   it clear that we are not DWIMming by calling resolve_ref().

 builtin/blame.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git i/builtin/blame.c w/builtin/blame.c
index 0e102bf..395dfbc 100644
--- i/builtin/blame.c
+++ w/builtin/blame.c
@@ -2069,6 +2069,19 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+static void verify_working_tree_path(unsigned char *head_sha1, const char *path)
+{
+	unsigned char blob_sha1[20];
+	unsigned mode;
+
+	if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
+		die("no such ref: HEAD");
+	if (get_tree_entry(head_sha1, path, blob_sha1, &mode))
+		die("no such path '%s' in HEAD", path);
+	if (sha1_object_info(blob_sha1, NULL) != OBJ_BLOB)
+		die("path '%s' in HEAD is not a blob", path);
+}
+
 /*
  * Prepare a dummy commit that represents the work tree (or staged) item.
  * Note that annotating work tree item never works in the reverse.
@@ -2087,8 +2100,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	struct cache_entry *ce;
 	unsigned mode;
 
-	if (get_sha1("HEAD", head_sha1))
-		die("No such ref: HEAD");
+	verify_working_tree_path(head_sha1, path);
 
 	time(&now);
 	commit = xcalloc(1, sizeof(*commit));

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Ralf Thielow]

On Sun, Sep 9, 2012 at 9:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ralf Thielow <ralf.thielow@gmail.com> 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

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Jeff King]

On Sun, Sep 09, 2012 at 12:24:58PM -0700, Junio C Hamano wrote:

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

Yes, I think that is the only sensible thing here. The rest of this
email is me essentially me agreeing with you and telling you things you
already know, but I had a slightly different line of reasoning, so I
thought I would share.

As far as the original patch, if you are going to change blame, then it
is only logical to change the whole revision parser so that "git log --
MAKEFILE" works. And I do not think that is a direction we want to go.

core.ignorecase has never been about "make git case-insensitive". Git
represents a case-sensitive tree, and will always do so because of the
sha1 we compute over the tree objects. core.ignorecase is really "make
case-sensitive git work around your case-insensitive filesystem"[1].

If the proposal were instead to add a certain type of pathspec that is
case-insensitive[2], that would make much more sense to me. It is not
violating git's case-sensitivity because it is purely a _query_ issue.
And it is a feature you might use whether or not your filesystem is case
sensitive.

-Peff

[1] I was going to submit a patch to Documentation/config.txt to make
    this more clear, but IMHO the current text is already pretty clear.

[2] I did not keep up with Duy's work on pathspec magic-prefixes (and I
    could not find anything relevant in the code or documentation), but
    it seems like this would be a logical feature to support there.

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If the proposal were instead to add a certain type of pathspec that is
> case-insensitive[2], that would make much more sense to me. It is not
> violating git's case-sensitivity because it is purely a _query_ issue.
> And it is a feature you might use whether or not your filesystem is case
> sensitive.
> ...
> [2] I did not keep up with Duy's work on pathspec magic-prefixes (and I
>     could not find anything relevant in the code or documentation), but
>     it seems like this would be a logical feature to support there.

I think it mostly is in setup.c (look for "Magic pathspec").

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Jeff King]

On Mon, Sep 10, 2012 at 01:30:03PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If the proposal were instead to add a certain type of pathspec that is
> > case-insensitive[2], that would make much more sense to me. It is not
> > violating git's case-sensitivity because it is purely a _query_ issue.
> > And it is a feature you might use whether or not your filesystem is case
> > sensitive.
> > ...
> > [2] I did not keep up with Duy's work on pathspec magic-prefixes (and I
> >     could not find anything relevant in the code or documentation), but
> >     it seems like this would be a logical feature to support there.
> 
> I think it mostly is in setup.c (look for "Magic pathspec").

Thanks, that helped. I got excited when I saw the "icase" in the
comments and thought it might already be implemented. But it looks like
it is still to be done. :)

-Peff

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Sep 10, 2012 at 01:30:03PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > If the proposal were instead to add a certain type of pathspec that is
>> > case-insensitive[2], that would make much more sense to me. It is not
>> > violating git's case-sensitivity because it is purely a _query_ issue.
>> > And it is a feature you might use whether or not your filesystem is case
>> > sensitive.
>> > ...
>> > [2] I did not keep up with Duy's work on pathspec magic-prefixes (and I
>> >     could not find anything relevant in the code or documentation), but
>> >     it seems like this would be a logical feature to support there.
>> 
>> I think it mostly is in setup.c (look for "Magic pathspec").
>
> Thanks, that helped. I got excited when I saw the "icase" in the
> comments and thought it might already be implemented. But it looks like
> it is still to be done. :)

Yeah, some are tongue-in-cheek (e.g. I do not know what "recursive
pathspec" even means), but "noglob" probably is an urgent need from
correctness point of view for people who are writing Porcelain and
want to interact with a history that records funny filenames.
Currently you can "git <cmd> 'foo\*'" to match a path that is
exactly 'foo*' (because it matches) but you also have to hope there
is no other paths that happens to match that pattern.  A script that
grabs paths out of ls-files output and then tries to use them as
pathspec would want to have a way to say "This is literal. Do not
honor globs in it".

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Jeff King]

On Mon, Sep 10, 2012 at 02:38:08PM -0700, Junio C Hamano wrote:

> > Thanks, that helped. I got excited when I saw the "icase" in the
> > comments and thought it might already be implemented. But it looks like
> > it is still to be done. :)
> 
> Yeah, some are tongue-in-cheek (e.g. I do not know what "recursive
> pathspec" even means), but "noglob" probably is an urgent need from
> correctness point of view for people who are writing Porcelain and
> want to interact with a history that records funny filenames.
> Currently you can "git <cmd> 'foo\*'" to match a path that is
> exactly 'foo*' (because it matches) but you also have to hope there
> is no other paths that happens to match that pattern.  A script that
> grabs paths out of ls-files output and then tries to use them as
> pathspec would want to have a way to say "This is literal. Do not
> honor globs in it".

I agree that the automatic globbing is currently a problem (although one
that comes up quite infrequently; I guess people use sane pathnames).
But I would think for that particular use case, you would not want to do
a per-glob prefix for that, but would rather use a command-line switch.
IOW, would you rather do:

  git ls-files |
  while read fn; do
    echo ":(noglob)$fn"
  done |
  xargs git log --stdin --

or:

  git ls-files |
  xargs git log --stdin --no-glob-pathspec --

?

-Peff

Re: [PATCH/RFC] blame: respect "core.ignorecase"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I would think for that particular use case, you would not want to do
> a per-glob prefix for that, but would rather use a command-line switch.

Yes.

Even though it is not listed as possible future semantics, one thing
that we may want to have before all others is ":(negate)", so that
we can say something like:

    git log -- '*.c' ':(negate)compat/"

I do not offhand recall what we decided during the last discussion
on the topic, but IIRC, the concensus was that it will make the code
too complex and general case unusably slow to support 'or', 'and',
and 'not' operators between pathspec elements (e.g. "*.c" or "*.h"
files but not in "compat/" directory, unless ...) in a general way,
and it would be sufficient to support negation by:

 - first grouping pathspec elements into two bins (i.e. normal
   patterns and negative patterns);

 - doing the usual path limiting using only the normal patterns;

 - among the paths that match one of the normal patterns, rejecting
   the ones that match one of the negative patterns.

or something like that.

[PATCH] blame: allow "blame file" in the middle of a conflicted merge

[Junio C Hamano]

"git blame file" has always meant "find the origin of each line of
the file in the history leading to HEAD, oh by the way, blame the
lines that are modified locally to the working tree".

This teaches "git blame" that during a conflicted merge, some
uncommitted changes may have come from the other history that is
being merged.

The verify_working_tree_path() function introduced in the previous
patch to notice a typo in the filename (primarily on case insensitive
filesystems) has been updated to allow a filename that does not exist
in HEAD (i.e. the tip of our history) as long as it exists one of the
commits being merged, so that a "we deleted, the other side modified"
case tracks the history of the file in the history of the other side.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is a follow-up to the previous one that is on 'pu'.  I was
   pleasantly surprised that something generally useful came out of
   a patch to squelch corner case confusion that only happens on a
   case insensitive filesystem (iow, a case I do not particularly
   care deeply about ;-).

 builtin/blame.c                 | 93 ++++++++++++++++++++++++++++++-----------
 t/t8004-blame-with-conflicts.sh |  2 +-
 2 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 83683b8..449880e 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2044,17 +2044,53 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static void verify_working_tree_path(unsigned char *head_sha1, const char *path)
+static void verify_working_tree_path(struct commit *work_tree, const char *path)
 {
-	unsigned char blob_sha1[20];
-	unsigned mode;
+	struct commit_list *parents;
 
-	if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
-		die("no such ref: HEAD");
-	if (get_tree_entry(head_sha1, path, blob_sha1, &mode))
-		die("no such path '%s' in HEAD", path);
-	if (sha1_object_info(blob_sha1, NULL) != OBJ_BLOB)
-		die("path '%s' in HEAD is not a blob", path);
+	for (parents = work_tree->parents; parents; parents = parents->next) {
+		const unsigned char *commit_sha1 = parents->item->object.sha1;
+		unsigned char blob_sha1[20];
+		unsigned mode;
+
+		if (!get_tree_entry(commit_sha1, path, blob_sha1, &mode) &&
+		    sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)
+			return;
+	}
+	die("no such path '%s' in HEAD", path);
+}
+
+static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)
+{
+	struct commit *parent;
+
+	parent = lookup_commit_reference(sha1);
+	if (!parent)
+		die("no such commit %s", sha1_to_hex(sha1));
+	return &commit_list_insert(parent, tail)->next;
+}
+
+static void append_merge_parents(struct commit_list **tail)
+{
+	int merge_head;
+	const char *merge_head_file = git_path("MERGE_HEAD");
+	struct strbuf line = STRBUF_INIT;
+
+	merge_head = open(merge_head_file, O_RDONLY);
+	if (merge_head < 0) {
+		if (errno == ENOENT)
+			return;
+		die("cannot open '%s' for reading", merge_head_file);
+	}
+
+	while (!strbuf_getwholeline_fd(&line, merge_head, '\n')) {
+		unsigned char sha1[20];
+		if (line.len < 40 || get_sha1_hex(line.buf, sha1))
+			die("unknown line in '%s': %s", merge_head_file, line.buf);
+		tail = append_parent(tail, sha1);
+	}
+	close(merge_head);
+	strbuf_release(&line);
 }
 
 /*
@@ -2067,6 +2103,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 {
 	struct commit *commit;
 	struct origin *origin;
+	struct commit_list **parent_tail, *parent;
 	unsigned char head_sha1[20];
 	struct strbuf buf = STRBUF_INIT;
 	const char *ident;
@@ -2074,19 +2111,38 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	int size, len;
 	struct cache_entry *ce;
 	unsigned mode;
-
-	verify_working_tree_path(head_sha1, path);
+	struct strbuf msg = STRBUF_INIT;
 
 	time(&now);
 	commit = xcalloc(1, sizeof(*commit));
-	commit->parents = xcalloc(1, sizeof(*commit->parents));
-	commit->parents->item = lookup_commit_reference(head_sha1);
 	commit->object.parsed = 1;
 	commit->date = now;
 	commit->object.type = OBJ_COMMIT;
+	parent_tail = &commit->parents;
+
+	if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
+		die("no such ref: HEAD");
+
+	parent_tail = append_parent(parent_tail, head_sha1);
+	append_merge_parents(parent_tail);
+	verify_working_tree_path(commit, path);
 
 	origin = make_origin(commit, path);
 
+	ident = fmt_ident("Not Committed Yet", "not.committed.yet", NULL, 0);
+	strbuf_addstr(&msg, "tree 0000000000000000000000000000000000000000\n");
+	for (parent = commit->parents; parent; parent = parent->next)
+		strbuf_addf(&msg, "parent %s\n",
+			    sha1_to_hex(parent->item->object.sha1));
+	strbuf_addf(&msg,
+		    "author %s\n"
+		    "committer %s\n\n"
+		    "Version of %s from %s\n",
+		    ident, ident, path,
+		    (!contents_from ? path :
+		     (!strcmp(contents_from, "-") ? "standard input" : contents_from)));
+	commit->buffer = strbuf_detach(&msg, NULL);
+
 	if (!contents_from || strcmp("-", contents_from)) {
 		struct stat st;
 		const char *read_from;
@@ -2123,7 +2179,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	}
 	else {
 		/* Reading from stdin */
-		contents_from = "standard input";
 		mode = 0;
 		if (strbuf_read(&buf, 0, 0) < 0)
 			die_errno("failed to read from stdin");
@@ -2167,16 +2222,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	 */
 	cache_tree_invalidate_path(active_cache_tree, path);
 
-	commit->buffer = xmalloc(400);
-	ident = fmt_ident("Not Committed Yet", "not.committed.yet", NULL, 0);
-	snprintf(commit->buffer, 400,
-		"tree 0000000000000000000000000000000000000000\n"
-		"parent %s\n"
-		"author %s\n"
-		"committer %s\n\n"
-		"Version of %s from %s\n",
-		sha1_to_hex(head_sha1),
-		ident, ident, path, contents_from ? contents_from : path);
 	return commit;
 }
 
diff --git a/t/t8004-blame-with-conflicts.sh b/t/t8004-blame-with-conflicts.sh
index b4a260a..9c353ab 100755
--- a/t/t8004-blame-with-conflicts.sh
+++ b/t/t8004-blame-with-conflicts.sh
@@ -67,7 +67,7 @@ test_expect_success \
 '
 
 test_expect_success 'blame does not crash with conflicted file in stages 1,3' '
-	test_must_fail git blame file1
+	git blame file1
 '
 
 test_done
-- 
1.7.12.414.g1a62b7a