{"thread":{"id":"7559","subject":"[PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()","startedAt":"2007-04-07T14:40:42Z","lastAt":"2007-04-08T00:29:24Z","messageCount":4,"participants":["Junio C Hamano","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"38846","messageId":"7vabxkcknp.fsf@assigned-by-dhcp.cox.net","threadId":"7559","inReplyTo":null,"subject":"[PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-07T14:40:42Z","receivedAt":"2007-04-07T14:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This fixes three buglets in threeway_merge() regarding D/F\nconflict entries.\n\n* After finishing with path D and handling path D/F, some stages\n have D/F conflict entry which are obviously non-NULL. For the\n purpose of determining if the path D/F is missing in the\n ancestor, they should not be taken into account.\n\n* D/F conflict entry is a phony entry and does not record the\n path being processed, so do not pick up the name from there.\n\n* D/F conflict entry is a marker to say \"this stage does _not_\n have the path\", so do not send them to keep_entry().\n\nThere might be more glitches, but I am slowly digging this mess\nthrough, which unfortunately was made even more work since\nmerge-recursive is a built-in now.\n\nSigned-off-by: Junio C Hamano \n---\n unpack-trees.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a0b6769..4039653 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -675,10 +675,10 @@ int threeway_merge(struct cache_entry **stages,\n \tint i;\n \n \tfor (i = 1; i < o->head_idx; i++) {\n-\t\tif (!stages[i])\n+\t\tif (!stages[i] || stages[i] == o->df_conflict_entry)\n \t\t\tany_anc_missing = 1;\n \t\telse {\n-\t\t\tif (!path)\n+\t\t\tif (!path && stages[i] != o->df_conflict_entry)\n \t\t\t\tpath = stages[i]->name;\n \t\t\tno_anc_exists = 0;\n \t\t}\n@@ -790,7 +790,7 @@ int threeway_merge(struct cache_entry **stages,\n \tcount = 0;\n \tif (!head_match || !remote_match) {\n \t\tfor (i = 1; i < o->head_idx; i++) {\n-\t\t\tif (stages[i]) {\n+\t\t\tif (stages[i] && stages[i] != o->df_conflict_entry) {\n \t\t\t\tkeep_entry(stages[i], o);\n \t\t\t\tcount++;\n \t\t\t\tbreak;\n-- \n1.5.1.81.gee969\n"},{"id":"38858","messageId":"Pine.LNX.4.64.0704071456470.27922@iabervon.org","threadId":"7559","inReplyTo":"7vabxkcknp.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-04-07T19:08:06Z","receivedAt":"2007-04-07T19:08:06Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 7 Apr 2007, Junio C Hamano wrote:\n\n> This fixes three buglets in threeway_merge() regarding D/F\n> conflict entries.\n> \n> * After finishing with path D and handling path D/F, some stages\n> have D/F conflict entry which are obviously non-NULL. For the\n> purpose of determining if the path D/F is missing in the\n> ancestor, they should not be taken into account.\n> \n> * D/F conflict entry is a phony entry and does not record the\n> path being processed, so do not pick up the name from there.\n\nThis bit is unnecessary, because the first bit means we treat D/F conflict \nas missing in that conditional, and don't count it as an entry at all, let \nalone one with a useful name.\n\n> * D/F conflict entry is a marker to say \"this stage does _not_\n> have the path\", so do not send them to keep_entry().\n> \n> There might be more glitches, but I am slowly digging this mess\n> through, which unfortunately was made even more work since\n> merge-recursive is a built-in now.\n\nLooks good, although it might be wise to add an \"exists\" function that \nreturns false for df_conflict_entry and for NULL, to make the tests \nclearer, and to get a comment to point out the meaning of \ndf_conflict_entry. Also, I think df_conflict_entry can be static bss in \nunpack-trees and not accessed through o->df_conflict_entry, since it's \nalways the same value (being now universally initialized to a static \npointer to heap...).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"38870","messageId":"7vfy7bbu3a.fsf@assigned-by-dhcp.cox.net","threadId":"7559","inReplyTo":"Pine.LNX.4.64.0704071456470.27922@iabervon.org","subject":"Re: [PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-08T00:14:33Z","receivedAt":"2007-04-08T00:14:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> On Sat, 7 Apr 2007, Junio C Hamano wrote:\n>\n>> This fixes three buglets in threeway_merge() regarding D/F\n>> conflict entries.\n>> \n>> * After finishing with path D and handling path D/F, some stages\n>> have D/F conflict entry which are obviously non-NULL. For the\n>> purpose of determining if the path D/F is missing in the\n>> ancestor, they should not be taken into account.\n>> \n>> * D/F conflict entry is a phony entry and does not record the\n>> path being processed, so do not pick up the name from there.\n>\n> This bit is unnecessary, because the first bit means we treat D/F conflict \n> as missing in that conditional, and don't count it as an entry at all, let \n> alone one with a useful name.\n\nI am not sure about what you mean by \"first bit\", but I added\nthis after noticing there was a case where the path was not\npicked up from index/head/remote correctly because the \"pick up\nthe path from ancestors, while we check for any_anc_missing and\nno_anc_exists condition\" loop at the beginning already set it to\npoint at stages[i]->name (which is an empty string \"\" name of\nthe df-conflict entry).\n\nBut I notice that \"aggressive\" codepath is the only thing that\nwants to use the path (it uses it to check the working tree\nstatus), so maybe we should move that path computation business\ninside the aggressive codepath.\n"},{"id":"38871","messageId":"Pine.LNX.4.64.0704072018321.27922@iabervon.org","threadId":"7559","inReplyTo":"7vfy7bbu3a.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-04-08T00:29:24Z","receivedAt":"2007-04-08T00:29:24Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 7 Apr 2007, Junio C Hamano wrote:\n\n> Daniel Barkalow writes:\n> \n> > On Sat, 7 Apr 2007, Junio C Hamano wrote:\n> >\n> >> This fixes three buglets in threeway_merge() regarding D/F\n> >> conflict entries.\n> >> \n> >> * After finishing with path D and handling path D/F, some stages\n> >> have D/F conflict entry which are obviously non-NULL. For the\n> >> purpose of determining if the path D/F is missing in the\n> >> ancestor, they should not be taken into account.\n> >> \n> >> * D/F conflict entry is a phony entry and does not record the\n> >> path being processed, so do not pick up the name from there.\n> >\n> > This bit is unnecessary, because the first bit means we treat D/F conflict \n> > as missing in that conditional, and don't count it as an entry at all, let \n> > alone one with a useful name.\n> \n> I am not sure about what you mean by \"first bit\"\n\nYour first bullet point. You must have added the check to the path line \nbefore you added the same check to the condition a few lines above, making \nthis check no longer necessary.\n\n if (!stages[i] || stages[i] == o->df_conflict_entry)\n any_anc_missing = 1;\n else {\n\nIf we get here, stages[i] != o->df_conflict_entry...\n\n if (!path && stages[i] != o->df_conflict_entry)\n path = stages[i]->name;\n no_anc_exists = 0;\n }\n\nSo you don't need the second check if you've got the first one, and the \nfirst one makes more sense anyway; for the purposes of this entire \nsection, we want df_conflict_entry to count as missing, and we don't look \nat the paths of missing entries regardless of whether they're specially \nmarked.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}