* [PATCH] blame: Allow to blame paths freshly added to the index @ 2016-07-15 2:42 Mike Hommey 2016-07-15 10:45 ` Johannes Schindelin 2016-07-15 20:58 ` [PATCH] " Junio C Hamano 0 siblings, 2 replies; 15+ messages in thread From: Mike Hommey @ 2016-07-15 2:42 UTC (permalink / raw) To: git; +Cc: gitster When blaming files, changes in the work tree are taken into account and displayed as being "Not Committed Yet". However, when blaming a file that is not known to the current HEAD, git blame fails with `no such path 'foo' in HEAD`, even when the file was git add'ed. This would seem uninteresting with the plain `git blame` case, which it is, but it becomes useful when using copy detection, and the new file was created from pieces already in HEAD, moved or copied from other files. --- builtin/blame.c | 4 +++- t/t8003-blame-corner-cases.sh | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index 1e214bd..0858b18 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path) sha1_object_info(blob_sha1, NULL) == OBJ_BLOB) return; } - die("no such path '%s' in HEAD", path); + + if (cache_name_pos(path, strlen(path)) < 0) + die("no such path '%s' in HEAD", path); } static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1) diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh index a9b266f..a0a09e2 100755 --- a/t/t8003-blame-corner-cases.sh +++ b/t/t8003-blame-corner-cases.sh @@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' ' ' +test_expect_success 'blame wholesale copy and more in the index' ' + + { + echo ABC + echo DEF + echo XXXX + echo YYYY + echo GHIJK + } >horse && + git add horse && + git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current && + { + echo mouse-Initial + echo mouse-Second + echo cow-Fifth + echo horse-Not + echo mouse-Third + } >expected && + test_cmp expected current && + git rm -f horse + +' + test_expect_success 'blame path that used to be a directory' ' mkdir path && echo A A A A A >path/file && -- 2.9.1.276.geea30e8 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] blame: Allow to blame paths freshly added to the index 2016-07-15 2:42 [PATCH] blame: Allow to blame paths freshly added to the index Mike Hommey @ 2016-07-15 10:45 ` Johannes Schindelin 2016-07-15 12:32 ` Mike Hommey 2016-07-15 12:55 ` [PATCH v2] " Mike Hommey 2016-07-15 20:58 ` [PATCH] " Junio C Hamano 1 sibling, 2 replies; 15+ messages in thread From: Johannes Schindelin @ 2016-07-15 10:45 UTC (permalink / raw) To: Mike Hommey; +Cc: git, gitster Hi Mike, On Fri, 15 Jul 2016, Mike Hommey wrote: > When blaming files, changes in the work tree are taken into account > and displayed as being "Not Committed Yet". > > However, when blaming a file that is not known to the current HEAD, > git blame fails with `no such path 'foo' in HEAD`, even when the file > was git add'ed. > > This would seem uninteresting with the plain `git blame` case, which > it is, but it becomes useful when using copy detection, and the new file > was created from pieces already in HEAD, moved or copied from other > files. > --- Well explained. Please add your sign-off. > static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1) > diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh > index a9b266f..a0a09e2 100755 > --- a/t/t8003-blame-corner-cases.sh > +++ b/t/t8003-blame-corner-cases.sh > @@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' ' > > ' > > +test_expect_success 'blame wholesale copy and more in the index' ' > + > + { > + echo ABC > + echo DEF > + echo XXXX > + echo YYYY > + echo GHIJK > + } >horse && A more common way to do this in our test scripts is by using here documents. However, in this case I would suggest test_write_lines ABC DEF XXXX YYYY GHIJK >horse instead. The equivalent applies to the 'expected' file below: > + git add horse && > + git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current && > + { > + echo mouse-Initial > + echo mouse-Second > + echo cow-Fifth > + echo horse-Not > + echo mouse-Third > + } >expected && > + test_cmp expected current && > + git rm -f horse Should this not be a test_when_finished "git rm -f horse" at the beginning? Otherwise it looks really good to me. Ciao, Dscho ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] blame: Allow to blame paths freshly added to the index 2016-07-15 10:45 ` Johannes Schindelin @ 2016-07-15 12:32 ` Mike Hommey 2016-07-15 12:37 ` Jeff King 2016-07-15 12:55 ` [PATCH v2] " Mike Hommey 1 sibling, 1 reply; 15+ messages in thread From: Mike Hommey @ 2016-07-15 12:32 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, gitster On Fri, Jul 15, 2016 at 12:45:15PM +0200, Johannes Schindelin wrote: > Hi Mike, > > On Fri, 15 Jul 2016, Mike Hommey wrote: > > > When blaming files, changes in the work tree are taken into account > > and displayed as being "Not Committed Yet". > > > > However, when blaming a file that is not known to the current HEAD, > > git blame fails with `no such path 'foo' in HEAD`, even when the file > > was git add'ed. > > > > This would seem uninteresting with the plain `git blame` case, which > > it is, but it becomes useful when using copy detection, and the new file > > was created from pieces already in HEAD, moved or copied from other > > files. > > --- > > Well explained. > > Please add your sign-off. Facepalm, forgot to sign-off again. > > static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1) > > diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh > > index a9b266f..a0a09e2 100755 > > --- a/t/t8003-blame-corner-cases.sh > > +++ b/t/t8003-blame-corner-cases.sh > > @@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' ' > > > > ' > > > > +test_expect_success 'blame wholesale copy and more in the index' ' > > + > > + { > > + echo ABC > > + echo DEF > > + echo XXXX > > + echo YYYY > > + echo GHIJK > > + } >horse && > > A more common way to do this in our test scripts is by using here > documents. However, in this case I would suggest > > test_write_lines ABC DEF XXXX YYYY GHIJK >horse I merely copied the pattern used in other places in the same test file. Using test_write_lines or something else (what are "here documents"?) would break consistency. I can also change the other similar blocks at the same time, though, whichever you prefer. > instead. The equivalent applies to the 'expected' file below: > > > + git add horse && > > + git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current && > > + { > > + echo mouse-Initial > > + echo mouse-Second > > + echo cow-Fifth > > + echo horse-Not > > + echo mouse-Third > > + } >expected && > > + test_cmp expected current && > > + git rm -f horse > > Should this not be a > > test_when_finished "git rm -f horse" > > at the beginning? Indeed. Thanks Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] blame: Allow to blame paths freshly added to the index 2016-07-15 12:32 ` Mike Hommey @ 2016-07-15 12:37 ` Jeff King 2016-07-15 12:42 ` Mike Hommey 0 siblings, 1 reply; 15+ messages in thread From: Jeff King @ 2016-07-15 12:37 UTC (permalink / raw) To: Mike Hommey; +Cc: Johannes Schindelin, git, gitster On Fri, Jul 15, 2016 at 09:32:45PM +0900, Mike Hommey wrote: > > > +test_expect_success 'blame wholesale copy and more in the index' ' > > > + > > > + { > > > + echo ABC > > > + echo DEF > > > + echo XXXX > > > + echo YYYY > > > + echo GHIJK > > > + } >horse && > > > > A more common way to do this in our test scripts is by using here > > documents. However, in this case I would suggest > > > > test_write_lines ABC DEF XXXX YYYY GHIJK >horse > > I merely copied the pattern used in other places in the same test file. > Using test_write_lines or something else (what are "here documents"?) > would break consistency. I can also change the other similar blocks at > the same time, though, whichever you prefer. A here document is this: cat <<-\EOF ABC DEF XXXX YYYY GHIJK EOF The "<<" starts the here-doc. The "-" tells the shell to strip leading tabs (so you can keep it indented with the rest of the code. The "\" tells the shell not to interpolate (not a big deal here, but great for more complicated input). The "EOF" tells it where to stop. Matching surrounding style is always reasonable, though I do think this particular file is a bit of an oddball. Most of our scripts use here documents. Either is OK in this case, IMHO. Personally I do not find test_write_lines particularly readable, but I guess some people do, which is why it exists. -Peff ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] blame: Allow to blame paths freshly added to the index 2016-07-15 12:37 ` Jeff King @ 2016-07-15 12:42 ` Mike Hommey 0 siblings, 0 replies; 15+ messages in thread From: Mike Hommey @ 2016-07-15 12:42 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Schindelin, git, gitster On Fri, Jul 15, 2016 at 08:37:59AM -0400, Jeff King wrote: > On Fri, Jul 15, 2016 at 09:32:45PM +0900, Mike Hommey wrote: > > > > > +test_expect_success 'blame wholesale copy and more in the index' ' > > > > + > > > > + { > > > > + echo ABC > > > > + echo DEF > > > > + echo XXXX > > > > + echo YYYY > > > > + echo GHIJK > > > > + } >horse && > > > > > > A more common way to do this in our test scripts is by using here > > > documents. However, in this case I would suggest > > > > > > test_write_lines ABC DEF XXXX YYYY GHIJK >horse > > > > I merely copied the pattern used in other places in the same test file. > > Using test_write_lines or something else (what are "here documents"?) > > would break consistency. I can also change the other similar blocks at > > the same time, though, whichever you prefer. > > A here document is this: > > cat <<-\EOF > ABC > DEF > XXXX > YYYY > GHIJK > EOF > > The "<<" starts the here-doc. The "-" tells the shell to strip leading > tabs (so you can keep it indented with the rest of the code. The "\" > tells the shell not to interpolate (not a big deal here, but great for > more complicated input). The "EOF" tells it where to stop. Oh, so that's what they are called! I've used them for 20 years without knowing :) TIL. Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] blame: Allow to blame paths freshly added to the index 2016-07-15 10:45 ` Johannes Schindelin 2016-07-15 12:32 ` Mike Hommey @ 2016-07-15 12:55 ` Mike Hommey 2016-07-15 15:28 ` Johannes Schindelin 1 sibling, 1 reply; 15+ messages in thread From: Mike Hommey @ 2016-07-15 12:55 UTC (permalink / raw) To: git; +Cc: gitster, Johannes.Schindelin When blaming files, changes in the work tree are taken into account and displayed as being "Not Committed Yet". However, when blaming a file that is not known to the current HEAD, git blame fails with `no such path 'foo' in HEAD`, even when the file was git add'ed. This would seem uninteresting with the plain `git blame` case, which it is, but it becomes useful when using copy detection, and the new file was created from pieces already in HEAD, moved or copied from other files. Signed-off-by: Mike Hommey <mh@glandium.org> --- builtin/blame.c | 4 ++- t/t8003-blame-corner-cases.sh | 57 ++++++++++++++++++++++++++++++------------- 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index 1e214bd..0858b18 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path) sha1_object_info(blob_sha1, NULL) == OBJ_BLOB) return; } - die("no such path '%s' in HEAD", path); + + if (cache_name_pos(path, strlen(path)) < 0) + die("no such path '%s' in HEAD", path); } static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1) diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh index a9b266f..2812d7c 100755 --- a/t/t8003-blame-corner-cases.sh +++ b/t/t8003-blame-corner-cases.sh @@ -41,12 +41,12 @@ test_expect_success setup ' test_tick && GIT_AUTHOR_NAME=Fourth git commit -m Fourth && - { - echo ABC - echo DEF - echo XXXX - echo GHIJK - } >cow && + cat >cow <<-\EOF && + ABC + DEF + XXXX + GHIJK + EOF git add cow && test_tick && GIT_AUTHOR_NAME=Fifth git commit -m Fifth @@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' ' test_expect_success 'blame wholesale copy' ' git blame -f -C -C1 HEAD^ -- cow | sed -e "$pick_fc" >current && - { - echo mouse-Initial - echo mouse-Second - echo mouse-Third - } >expected && + cat >expected <<-\EOF && + mouse-Initial + mouse-Second + mouse-Third + EOF test_cmp expected current ' @@ -127,12 +127,35 @@ test_expect_success 'blame wholesale copy' ' test_expect_success 'blame wholesale copy and more' ' git blame -f -C -C1 HEAD -- cow | sed -e "$pick_fc" >current && - { - echo mouse-Initial - echo mouse-Second - echo cow-Fifth - echo mouse-Third - } >expected && + cat >expected <<-\EOF && + mouse-Initial + mouse-Second + cow-Fifth + mouse-Third + EOF + test_cmp expected current + +' + +test_expect_success 'blame wholesale copy and more in the index' ' + + cat >horse <<-\EOF && + ABC + DEF + XXXX + YYYY + GHIJK + EOF + git add horse && + test_when_finished "git rm -f horse" && + git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current && + cat >expected <<-\EOF && + mouse-Initial + mouse-Second + cow-Fifth + horse-Not + mouse-Third + EOF test_cmp expected current ' -- 2.9.1.276.geea30e8 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] blame: Allow to blame paths freshly added to the index 2016-07-15 12:55 ` [PATCH v2] " Mike Hommey @ 2016-07-15 15:28 ` Johannes Schindelin 0 siblings, 0 replies; 15+ messages in thread From: Johannes Schindelin @ 2016-07-15 15:28 UTC (permalink / raw) To: Mike Hommey; +Cc: git, gitster Hi Mike, On Fri, 15 Jul 2016, Mike Hommey wrote: > diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh > index a9b266f..2812d7c 100755 > --- a/t/t8003-blame-corner-cases.sh > +++ b/t/t8003-blame-corner-cases.sh > @@ -41,12 +41,12 @@ test_expect_success setup ' > test_tick && > GIT_AUTHOR_NAME=Fourth git commit -m Fourth && > > - { > - echo ABC > - echo DEF > - echo XXXX > - echo GHIJK > - } >cow && > + cat >cow <<-\EOF && > + ABC > + DEF > + XXXX > + GHIJK > + EOF > git add cow && Sorry, I did not realize that there was precedent for this awkward paradigm in the same test script. I like that you fix them, but I would prefer that to be done in a separate patch (does not even need to be the same patch series). Apart from that (i.e. apart from touching unrelated parts of the test script), the patch looks good to me. Thanks, Dscho ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] blame: Allow to blame paths freshly added to the index 2016-07-15 2:42 [PATCH] blame: Allow to blame paths freshly added to the index Mike Hommey 2016-07-15 10:45 ` Johannes Schindelin @ 2016-07-15 20:58 ` Junio C Hamano 2016-07-15 21:14 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2016-07-15 20:58 UTC (permalink / raw) To: Mike Hommey; +Cc: git Mike Hommey <mh@glandium.org> writes: > When blaming files, changes in the work tree are taken into account > and displayed as being "Not Committed Yet". > > However, when blaming a file that is not known to the current HEAD, > git blame fails with `no such path 'foo' in HEAD`, even when the file > was git add'ed. > > This would seem uninteresting with the plain `git blame` case, which > it is, but it becomes useful when using copy detection, and the new file > was created from pieces already in HEAD, moved or copied from other > files. I suspect that this would be useful without copy detection. If you "git mv fileA fileB" (optionally followed by "edit fileB"), fileB would not be in HEAD but you should be able to trace the lineage of the lines in it back through the renaming event, and this change also allows that use case, no? > --- Missing sign-off? > builtin/blame.c | 4 +++- > t/t8003-blame-corner-cases.sh | 23 +++++++++++++++++++++++ > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/builtin/blame.c b/builtin/blame.c > index 1e214bd..0858b18 100644 > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path) > sha1_object_info(blob_sha1, NULL) == OBJ_BLOB) > return; > } > - die("no such path '%s' in HEAD", path); > + > + if (cache_name_pos(path, strlen(path)) < 0) > + die("no such path '%s' in HEAD", path); > } This is a surprisingly small change to bring a big difference at the usage level. I am sort-of surprised that the "fake working tree commit" mechanism was already prepared to handle this, even though I am responsible for the introduction of it at 1cfe7733 (git-blame: no rev means start from the working tree file., 2007-01-30). Having said that, do we act differently when the index is unmerged at path in any way? When path exists but is unmerged, you get negative index from cache_name_pos(), and IIUC, "blaming of working tree file" does not care if the index is unmerged. It creates the fake working tree file commit, pretends HEAD is its parent, and digs the lineage from there. I suspect that the above change needs to be updated further if the user wants to run "blame path" during a conflicted renaming merge, i.e. 0. Before two histories diverged, there was old_path. 1. Our side updated contents of that file and kept it at old_path. 2. Their side updated contents of that file and moved it to path. 3. "git merge" notcied the rename, created three stages at "path", with the result of conflicted content-level merge in the working tree at "path". 4. The user edits "path" and resolves the conflict, but wants to double check before running "git add path". 5. "git blame path" Perhaps something like this should suffice: pos = cache_name_pos(path, strlen(path)); if (0 <= pos) ; /* ok */ else if (!strcmp(active_cache[-1 - pos]->name), path) ; /* ok -- just unmerged */ else die("no such path in HEAD"); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] blame: Allow to blame paths freshly added to the index 2016-07-15 20:58 ` [PATCH] " Junio C Hamano @ 2016-07-15 21:14 ` Junio C Hamano 2016-07-15 23:16 ` Mike Hommey 2016-07-15 23:23 ` [PATCH v3 1/2] " Mike Hommey 0 siblings, 2 replies; 15+ messages in thread From: Junio C Hamano @ 2016-07-15 21:14 UTC (permalink / raw) To: Mike Hommey; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > I suspect that the above change needs to be updated further if the > user wants to run "blame path" during a conflicted renaming merge, > i.e. > > 0. Before two histories diverged, there was old_path. > 1. Our side updated contents of that file and kept it at old_path. > 2. Their side updated contents of that file and moved it to path. > 3. "git merge" notcied the rename, created three stages at "path", > with the result of conflicted content-level merge in the working > tree at "path". > 4. The user edits "path" and resolves the conflict, but wants to > double check before running "git add path". > 5. "git blame path" > > Perhaps something like this should suffice: > > pos = cache_name_pos(path, strlen(path)); > if (0 <= pos) > ; /* ok */ > else if (!strcmp(active_cache[-1 - pos]->name), path) > ; /* ok -- just unmerged */ > else > die("no such path in HEAD"); I do not think the "conflicted renaming merge" example would not be a problem in practice iff "git merge" was used, because the fake working tree commit would look at both our tree and their tree, and find "path" in theirs inside the loop above this "die". But the user can be in the same conflicted rename situation with "git am -3" or cherry-pick, and in these cases there won't be extra parent commits for the fake work tree commit, hence the conclusion does not change. Thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] blame: Allow to blame paths freshly added to the index 2016-07-15 21:14 ` Junio C Hamano @ 2016-07-15 23:16 ` Mike Hommey 2016-07-18 18:49 ` Junio C Hamano 2016-07-15 23:23 ` [PATCH v3 1/2] " Mike Hommey 1 sibling, 1 reply; 15+ messages in thread From: Mike Hommey @ 2016-07-15 23:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git > I suspect that this would be useful without copy detection. If you > "git mv fileA fileB" (optionally followed by "edit fileB"), fileB > would not be in HEAD but you should be able to trace the lineage of > the lines in it back through the renaming event, and this change > also allows that use case, no? It should, but that'd be copy/move detection, wouldn't it? :) > > I suspect that the above change needs to be updated further if the > > user wants to run "blame path" during a conflicted renaming merge, > > i.e. > > > > 0. Before two histories diverged, there was old_path. > > 1. Our side updated contents of that file and kept it at old_path. > > 2. Their side updated contents of that file and moved it to path. > > 3. "git merge" notcied the rename, created three stages at "path", > > with the result of conflicted content-level merge in the working > > tree at "path". > > 4. The user edits "path" and resolves the conflict, but wants to > > double check before running "git add path". > > 5. "git blame path" > > > > Perhaps something like this should suffice: > > > > pos = cache_name_pos(path, strlen(path)); > > if (0 <= pos) > > ; /* ok */ > > else if (!strcmp(active_cache[-1 - pos]->name), path) > > ; /* ok -- just unmerged */ > > else > > die("no such path in HEAD"); > > I do not think the "conflicted renaming merge" example would not be > a problem in practice iff "git merge" was used, because the fake > working tree commit would look at both our tree and their tree, and > find "path" in theirs inside the loop above this "die". More than that, it seems that's a case that already works without the patch (it doesn't complain that "no such path in HEAD", although it only identifies the "theirs" part of the merge conflict when blaming the file straight out of the merge failure, but that'd be a separate issue. > But the user can be in the same conflicted rename situation with > "git am -3" or cherry-pick, and in these cases there won't be extra > parent commits for the fake work tree commit, hence the conclusion > does not change. Indeed, with cherry-pick, the "no such path in HEAD" error is happening with the patch. Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] blame: Allow to blame paths freshly added to the index 2016-07-15 23:16 ` Mike Hommey @ 2016-07-18 18:49 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2016-07-18 18:49 UTC (permalink / raw) To: Mike Hommey; +Cc: git Mike Hommey <mh@glandium.org> writes: >> I suspect that this would be useful without copy detection. If you >> "git mv fileA fileB" (optionally followed by "edit fileB"), fileB >> would not be in HEAD but you should be able to trace the lineage of >> the lines in it back through the renaming event, and this change >> also allows that use case, no? > > It should, but that'd be copy/move detection, wouldn't it? :) Actually, in the context of "git blame", there is no extra "detection" needed for following a whole file rename. >> But the user can be in the same conflicted rename situation with >> "git am -3" or cherry-pick, and in these cases there won't be extra >> parent commits for the fake work tree commit, hence the conclusion >> does not change. > > Indeed, with cherry-pick, the "no such path in HEAD" error is happening > with the patch. OK. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/2] blame: Allow to blame paths freshly added to the index 2016-07-15 21:14 ` Junio C Hamano 2016-07-15 23:16 ` Mike Hommey @ 2016-07-15 23:23 ` Mike Hommey 2016-07-15 23:23 ` [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents Mike Hommey 1 sibling, 1 reply; 15+ messages in thread From: Mike Hommey @ 2016-07-15 23:23 UTC (permalink / raw) To: git; +Cc: gitster, Johannes.Schindelin When blaming files, changes in the work tree are taken into account and displayed as being "Not Committed Yet". However, when blaming a file that is not known to the current HEAD, git blame fails with `no such path 'foo' in HEAD`, even when the file was git add'ed. This would seem uninteresting with the plain `git blame` case, which it is, but it becomes useful when using copy detection, and the new file was created from pieces already in HEAD, moved or copied from other files. Signed-off-by: Mike Hommey <mh@glandium.org> --- builtin/blame.c | 10 +++++++++- t/t8003-blame-corner-cases.sh | 45 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index 1e214bd..f297a7f 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2230,6 +2230,7 @@ static int git_blame_config(const char *var, const char *value, void *cb) static void verify_working_tree_path(struct commit *work_tree, const char *path) { struct commit_list *parents; + int pos; for (parents = work_tree->parents; parents; parents = parents->next) { const unsigned char *commit_sha1 = parents->item->object.oid.hash; @@ -2240,7 +2241,14 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path) sha1_object_info(blob_sha1, NULL) == OBJ_BLOB) return; } - die("no such path '%s' in HEAD", path); + + pos = cache_name_pos(path, strlen(path)); + if (pos >= 0) + ; /* path is in the index */ + else if (!strcmp(active_cache[-1 - pos]->name, path)) + ; /* path is in the index, unmerged */ + else + die("no such path '%s' in HEAD", path); } static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1) diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh index a9b266f..eab2e28 100755 --- a/t/t8003-blame-corner-cases.sh +++ b/t/t8003-blame-corner-cases.sh @@ -137,6 +137,51 @@ test_expect_success 'blame wholesale copy and more' ' ' +test_expect_success 'blame wholesale copy and more in the index' ' + + cat >horse <<-\EOF && + ABC + DEF + XXXX + YYYY + GHIJK + EOF + git add horse && + test_when_finished "git rm -f horse" && + git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current && + cat >expected <<-\EOF && + mouse-Initial + mouse-Second + cow-Fifth + horse-Not + mouse-Third + EOF + test_cmp expected current + +' + +test_expect_success 'blame during cherry-pick with file rename conflict' ' + + test_when_finished "git reset --hard && git checkout master" && + git checkout HEAD~3 && + echo MOUSE >> mouse && + git mv mouse rodent && + git add rodent && + GIT_AUTHOR_NAME=Rodent git commit -m "rodent" && + git checkout --detach master && + (git cherry-pick HEAD@{1} || test $? -eq 1) && + git show HEAD@{1}:rodent > rodent && + git add rodent && + git blame -f -C -C1 rodent | sed -e "$pick_fc" >current && + cat current && + cat >expected <<-\EOF && + mouse-Initial + mouse-Second + rodent-Not + EOF + test_cmp expected current +' + test_expect_success 'blame path that used to be a directory' ' mkdir path && echo A A A A A >path/file && -- 2.9.1.277.gdbc86c9.dirty ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents 2016-07-15 23:23 ` [PATCH v3 1/2] " Mike Hommey @ 2016-07-15 23:23 ` Mike Hommey 2016-07-16 13:05 ` Johannes Schindelin 2016-07-18 18:52 ` Junio C Hamano 0 siblings, 2 replies; 15+ messages in thread From: Mike Hommey @ 2016-07-15 23:23 UTC (permalink / raw) To: git; +Cc: gitster, Johannes.Schindelin Somehow, this test was using: { echo A echo B } > file block to feed file contents. This changes those to the form most common in git test scripts: cat >file <<-\EOF A B EOF Signed-off-by: Mike Hommey <mh@glandium.org> --- t/t8003-blame-corner-cases.sh | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh index eab2e28..e48370d 100755 --- a/t/t8003-blame-corner-cases.sh +++ b/t/t8003-blame-corner-cases.sh @@ -41,12 +41,12 @@ test_expect_success setup ' test_tick && GIT_AUTHOR_NAME=Fourth git commit -m Fourth && - { - echo ABC - echo DEF - echo XXXX - echo GHIJK - } >cow && + cat >cow <<-\EOF && + ABC + DEF + XXXX + GHIJK + EOF git add cow && test_tick && GIT_AUTHOR_NAME=Fifth git commit -m Fifth @@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' ' test_expect_success 'blame wholesale copy' ' git blame -f -C -C1 HEAD^ -- cow | sed -e "$pick_fc" >current && - { - echo mouse-Initial - echo mouse-Second - echo mouse-Third - } >expected && + cat >expected <<-\EOF && + mouse-Initial + mouse-Second + mouse-Third + EOF test_cmp expected current ' @@ -127,12 +127,12 @@ test_expect_success 'blame wholesale copy' ' test_expect_success 'blame wholesale copy and more' ' git blame -f -C -C1 HEAD -- cow | sed -e "$pick_fc" >current && - { - echo mouse-Initial - echo mouse-Second - echo cow-Fifth - echo mouse-Third - } >expected && + cat >expected <<-\EOF && + mouse-Initial + mouse-Second + cow-Fifth + mouse-Third + EOF test_cmp expected current ' -- 2.9.1.277.gdbc86c9.dirty ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents 2016-07-15 23:23 ` [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents Mike Hommey @ 2016-07-16 13:05 ` Johannes Schindelin 2016-07-18 18:52 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Johannes Schindelin @ 2016-07-16 13:05 UTC (permalink / raw) To: Mike Hommey; +Cc: git, gitster Hi Mike, On Sat, 16 Jul 2016, Mike Hommey wrote: > Somehow, this test was using: > > { > echo A > echo B > } > file > > block to feed file contents. This changes those to the form most common > in git test scripts: > > cat >file <<-\EOF > A > B > EOF > > Signed-off-by: Mike Hommey <mh@glandium.org> I reviewed both patches and like them. Thanks, Dscho ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents 2016-07-15 23:23 ` [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents Mike Hommey 2016-07-16 13:05 ` Johannes Schindelin @ 2016-07-18 18:52 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2016-07-18 18:52 UTC (permalink / raw) To: Mike Hommey; +Cc: git, Johannes.Schindelin Mike Hommey <mh@glandium.org> writes: > Somehow, this test was using: > > { > echo A > echo B > } > file > > block to feed file contents. This changes those to the form most common > in git test scripts: > > cat >file <<-\EOF > A > B > EOF > > Signed-off-by: Mike Hommey <mh@glandium.org> > --- It is not so strong a preference to ask you to re-roll, but for future reference, I'd prefer to see this preparatory clean-up early in the series, followed by the primary thing, IOW, I would have liked more if the two patches were swapped. Thanks. Will queue. > t/t8003-blame-corner-cases.sh | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh > index eab2e28..e48370d 100755 > --- a/t/t8003-blame-corner-cases.sh > +++ b/t/t8003-blame-corner-cases.sh > @@ -41,12 +41,12 @@ test_expect_success setup ' > test_tick && > GIT_AUTHOR_NAME=Fourth git commit -m Fourth && > > - { > - echo ABC > - echo DEF > - echo XXXX > - echo GHIJK > - } >cow && > + cat >cow <<-\EOF && > + ABC > + DEF > + XXXX > + GHIJK > + EOF > git add cow && > test_tick && > GIT_AUTHOR_NAME=Fifth git commit -m Fifth > @@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' ' > test_expect_success 'blame wholesale copy' ' > > git blame -f -C -C1 HEAD^ -- cow | sed -e "$pick_fc" >current && > - { > - echo mouse-Initial > - echo mouse-Second > - echo mouse-Third > - } >expected && > + cat >expected <<-\EOF && > + mouse-Initial > + mouse-Second > + mouse-Third > + EOF > test_cmp expected current > > ' > @@ -127,12 +127,12 @@ test_expect_success 'blame wholesale copy' ' > test_expect_success 'blame wholesale copy and more' ' > > git blame -f -C -C1 HEAD -- cow | sed -e "$pick_fc" >current && > - { > - echo mouse-Initial > - echo mouse-Second > - echo cow-Fifth > - echo mouse-Third > - } >expected && > + cat >expected <<-\EOF && > + mouse-Initial > + mouse-Second > + cow-Fifth > + mouse-Third > + EOF > test_cmp expected current > > ' ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-07-18 18:52 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-15 2:42 [PATCH] blame: Allow to blame paths freshly added to the index Mike Hommey 2016-07-15 10:45 ` Johannes Schindelin 2016-07-15 12:32 ` Mike Hommey 2016-07-15 12:37 ` Jeff King 2016-07-15 12:42 ` Mike Hommey 2016-07-15 12:55 ` [PATCH v2] " Mike Hommey 2016-07-15 15:28 ` Johannes Schindelin 2016-07-15 20:58 ` [PATCH] " Junio C Hamano 2016-07-15 21:14 ` Junio C Hamano 2016-07-15 23:16 ` Mike Hommey 2016-07-18 18:49 ` Junio C Hamano 2016-07-15 23:23 ` [PATCH v3 1/2] " Mike Hommey 2016-07-15 23:23 ` [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents Mike Hommey 2016-07-16 13:05 ` Johannes Schindelin 2016-07-18 18:52 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.