{"thread":{"id":"24725","subject":"[PATCH 0/2] Fix windows portability issues in en/d-f-conflict-fix series","startedAt":"2010-08-13T02:09:10Z","lastAt":"2010-08-18T23:13:40Z","messageCount":9,"participants":["Elijah Newren","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"147986","messageId":"1281665352-10533-1-git-send-email-newren@gmail.com","threadId":"24725","inReplyTo":null,"subject":"[PATCH 0/2] Fix windows portability issues in en/d-f-conflict-fix series","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-13T02:09:10Z","receivedAt":"2010-08-13T02:09:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This fixes two issues in next for Windows reported by Hannes, both due\nto my en/d-f-conflict-fix patch series:\n\nOn Thu, Aug 12, 2010 at 3:23 AM, Johannes Sixt wrote:\n> Today's next produces this warning:\n>\n> merge-recursive.c: In function 'process_df_entry':\n> merge-recursive.c:1246: warning: unused variable 'o_sha'\n>\n> (line number may be different) because o_sha is only used inside assert().\n> I don't know how you would like this fixed.\n\n\n>> * en/d-f-conflict-fix (2010-07-27) 7 commits\n>> (merged to 'next' on 2010-08-03 at 7f78604)\n>> + t/t6035-merge-dir-to-symlink.sh: Remove TODO on passing test\n>> + fast-import: Improve robustness when D->F changes provided in wrong order\n>> + fast-export: Fix output order of D/F changes\n>> + merge_recursive: Fix renames across paths below D/F conflicts\n>> + merge-recursive: Fix D/F conflicts\n>> + Add a rename + D/F conflict testcase\n>> + Add additional testcases for D/F conflicts\n>\n> The new tests in t/t3509-cherry-pick-merge-df.sh and t9350-fast-export.sh\n> need SYMLINKS prerequisite.\n\nElijah Newren (2):\n merge-recursive: Workaround unused variable warning\n Mark tests that use symlinks as needing SYMLINKS prerequisite\n\n merge-recursive.c | 1 +\n t/t3509-cherry-pick-merge-df.sh | 6 ++++++\n t/t9350-fast-export.sh | 2 +-\n 3 files changed, 8 insertions(+), 1 deletions(-)\n\n-- \n1.7.2.1.119.gca9fe.dirty\n"},{"id":"147987","messageId":"1281665352-10533-2-git-send-email-newren@gmail.com","threadId":"24725","inReplyTo":"1281665352-10533-1-git-send-email-newren@gmail.com","subject":"[PATCH 1/2] merge-recursive: Workaround unused variable warning","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-13T02:09:11Z","receivedAt":"2010-08-13T02:09:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 9678c1d..7e32498 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1214,6 +1214,7 @@ static int process_df_entry(struct merge_options *o,\n \t/* We currently only handle D->F cases */\n \tassert((!o_sha && a_sha && !b_sha) ||\n \t (!o_sha && !a_sha && b_sha));\n+\t(void)o_sha;\n \n \tentry->processed = 1;\n \n-- \n1.7.2.1.119.gca9fe.dirty\n"},{"id":"147988","messageId":"1281665352-10533-3-git-send-email-newren@gmail.com","threadId":"24725","inReplyTo":"1281665352-10533-1-git-send-email-newren@gmail.com","subject":"[PATCH 2/2] Mark tests that use symlinks as needing SYMLINKS prerequisite","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-13T02:09:12Z","receivedAt":"2010-08-13T02:09:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"\nSigned-off-by: Elijah Newren \n---\n t/t3509-cherry-pick-merge-df.sh | 6 ++++++\n t/t9350-fast-export.sh | 2 +-\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t3509-cherry-pick-merge-df.sh b/t/t3509-cherry-pick-merge-df.sh\nindex 6e7ef84..93ad20d 100755\n--- a/t/t3509-cherry-pick-merge-df.sh\n+++ b/t/t3509-cherry-pick-merge-df.sh\n@@ -3,6 +3,12 @@\n test_description='Test cherry-pick with directory/file conflicts'\n . ./test-lib.sh\n \n+if ! test_have_prereq SYMLINKS\n+then\n+\tskip_all=\"symbolic links not supported - skipping tests\"\n+\ttest_done\n+fi\n+\n test_expect_success 'Setup rename across paths each below D/F conflicts' '\n \tmkdir a &&\n \t>a/f &&\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 1ee1461..27aea5c 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -376,7 +376,7 @@ test_expect_success 'tree_tag-obj' 'git fast-export tree_tag-obj'\n test_expect_success 'tag-obj_tag' 'git fast-export tag-obj_tag'\n test_expect_success 'tag-obj_tag-obj' 'git fast-export tag-obj_tag-obj'\n \n-test_expect_success 'directory becomes symlink' '\n+test_expect_success SYMLINKS 'directory becomes symlink' '\n \tgit init dirtosymlink &&\n \tgit init result &&\n \t(\n-- \n1.7.2.1.119.gca9fe.dirty\n"},{"id":"148326","messageId":"AANLkTikLXy4gPttmX=wzcDaFAfZO=OEk4PEDKh3sCSOF@mail.gmail.com","threadId":"24725","inReplyTo":"1281665352-10533-2-git-send-email-newren@gmail.com","subject":"Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-18T00:00:57Z","receivedAt":"2010-08-18T00:00:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Thu, Aug 12, 2010 at 8:09 PM, Elijah Newren wrote:\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 9678c1d..7e32498 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1214,6 +1214,7 @@ static int process_df_entry(struct merge_options *o,\n>        /* We currently only handle D->F cases */\n>        assert((!o_sha && a_sha && !b_sha) ||\n>               (!o_sha && !a_sha && b_sha));\n> +       (void)o_sha;\n>\n>        entry->processed = 1;\n\nIt appears that this patch did not get included, though 2/2 from this\nseries did. I'm not sure whether that was intentional or accidental.\nIf it was intentional, would a different method of fixing warnings\nwhen NDEBUG is defined be preferred? (Maybe changing the\n\"assert(foo)\" into \"if (!foo) die...\" instead?)\n\nIf you'd rather just leave it as is, that's fine too. I just wanted\nto make sure it was addressed to Johannes' satisfaction, since he\nbrought it up as an issue for the Windows build caused by my previous\npatches.\n\nThanks,\nElijah\n"},{"id":"148328","messageId":"20100818001032.GA7694@burratino","threadId":"24725","inReplyTo":"AANLkTikLXy4gPttmX=wzcDaFAfZO=OEk4PEDKh3sCSOF@mail.gmail.com","subject":"Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-18T00:10:32Z","receivedAt":"2010-08-18T00:10:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Elijah Newren wrote:\n> On Thu, Aug 12, 2010 at 8:09 PM, Elijah Newren wrote:\n\n>> +++ b/merge-recursive.c\n>> @@ -1214,6 +1214,7 @@ static int process_df_entry(struct merge_options *o,\n>>        /* We currently only handle D->F cases */\n>>        assert((!o_sha && a_sha && !b_sha) ||\n>>               (!o_sha && !a_sha && b_sha));\n>> +       (void)o_sha;\n[...]\n> would a different method of fixing warnings\n> when NDEBUG is defined be preferred? (Maybe changing the\n> \"assert(foo)\" into \"if (!foo) die...\" instead?)\n\nYes, that sounds like a good idea. The user would probably benefit\nfrom knowing what happened.\n"},{"id":"148402","messageId":"7veidvagz8.fsf@alter.siamese.dyndns.org","threadId":"24725","inReplyTo":"20100818001032.GA7694@burratino","subject":"Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-18T18:55:07Z","receivedAt":"2010-08-18T18:55:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Elijah Newren wrote:\n>> On Thu, Aug 12, 2010 at 8:09 PM, Elijah Newren wrote:\n>\n>>> +++ b/merge-recursive.c\n>>> @@ -1214,6 +1214,7 @@ static int process_df_entry(struct merge_options *o,\n>>>        /* We currently only handle D->F cases */\n>>>        assert((!o_sha && a_sha && !b_sha) ||\n>>>               (!o_sha && !a_sha && b_sha));\n>>> +       (void)o_sha;\n> [...]\n>> would a different method of fixing warnings\n>> when NDEBUG is defined be preferred? (Maybe changing the\n>> \"assert(foo)\" into \"if (!foo) die...\" instead?)\n>\n> Yes, that sounds like a good idea. The user would probably benefit\n> from knowing what happened.\n\nI'd agree. This assert() is not about protecting new callers from making\nobvious programming error by passing wrong parameters, but about Elijah\nnot being confident enough that the changes made to process_entry() with\nthis series really covers all the cases so that only D/F cases are left\nunprocessed.\n\nAnother possibility is to move this assert() out of process_df_entry() and\nhave it on the calling side. Perhaps something like the attached.\n\nBTW, it is not so obvious if (!o_sha && !!a_sha != !!b_sha) is equivalent\nto \"we are handling a D-F case\". Can you explain why?\n\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex b0f055e..7bab728 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1208,9 +1208,8 @@ static int process_df_entry(struct merge_options *o,\n \tconst char *conf;\n \tstruct stat st;\n \n-\t/* We currently only handle D->F cases */\n-\tassert((!o_sha && a_sha && !b_sha) ||\n-\t (!o_sha && !a_sha && b_sha));\n+\tif (! ((!o_sha && a_sha && !b_sha) || (!o_sha && !a_sha && b_sha)))\n+\t\treturn 1; /* we don't handle non D-F cases */\n \n \tentry->processed = 1;\n \n@@ -1321,6 +1320,12 @@ int merge_trees(struct merge_options *o,\n \t\t\t\t&& !process_df_entry(o, path, e))\n \t\t\t\tclean = 0;\n \t\t}\n+\t\tfor (i = 0; i < entries->nr; i++) {\n+\t\t\tstruct stage_data *e = entries->items[i].util;\n+\t\t\tif (!e->processed)\n+\t\t\t\tdie(\"Unprocessed path??? %s\", \n+\t\t\t\t entries->items[i].string);\n+\t\t}\n \n \t\tstring_list_clear(re_merge, 0);\n \t\tstring_list_clear(re_head, 0);\n"},{"id":"148415","messageId":"AANLkTik9mho7Pm5m=3dEbtMrH1r81Lc480i_z2_aHCxs@mail.gmail.com","threadId":"24725","inReplyTo":"7veidvagz8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-18T22:02:04Z","receivedAt":"2010-08-18T22:02:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Wed, Aug 18, 2010 at 12:55 PM, Junio C Hamano wrote:\n\n>> Yes, that sounds like a good idea.  The user would probably benefit\n>> from knowing what happened.\n>\n> I'd agree.  This assert() is not about protecting new callers from making\n> obvious programming error by passing wrong parameters, but about Elijah\n> not being confident enough that the changes made to process_entry() with\n> this series really covers all the cases so that only D/F cases are left\n> unprocessed.\n\nActually, it is pretty clear right now that only D/F cases are left\nunprocessed, and in particular D->F cases. This is because\nprocess_entry() starts with unconditionally setting \"entry->processed\n= 1\" and only unsets it in the one 'if' block where we know that\n(!o_sha && !!a_sha != !!b_sha &&\nstring_list_has_string(&o->current_directory_set, path)).\n\nSo, I'd say it is more about programming errors, in particular ones\nwhere people modify the code to make process_entry() leave more cases\nunprocessed than what is currently possible without also making the\nnecessary modifications to process_df_entry().\n\n> Another possibility is to move this assert() out of process_df_entry() and\n> have it on the calling side.  Perhaps something like the attached.\n>\n> BTW, it is not so obvious if (!o_sha && !!a_sha != !!b_sha) is equivalent\n> to \"we are handling a D-F case\".  Can you explain why?\n\nIt's not equivalent. Things are slightly confusing, because !\ncan mean either (a) there is nothing at the given path, or (b) there\nis a directory at the given path. The only way to tell which of the\ntwo it means is to look up the path in o->current_directory_set.\n\nA directory/file conflict (\"D-F\" in my shorthand) implies !!a_sha !=\n!!b_sha (but not vice versa).\n\nA directory/file conflict where the relevant path was a file in the\nmerge-base (\"F->D\" in my shorthand) implies (o_sha && !!a_sha !=\n!!b_sha). This case is handled just fine by process_entry() (if the\nfile was unchanged, the correct resolution is to delete it, allowing\npaths beneath the directory of the same name to be handled by later\nprocess_entry() calls -- although this silently relies on the order of\nentries from get_unmerged() to be such that things do operate in this\norder. That seems to be correct for the cases I've seen).\n\nA directory/file conflict where the path was a directory in the\nmerge-base (\"D->F\" in my shorthand) implies (!o_sha && !!a_sha !=\n!!b_sha). This is the case the process_df_entry needs to be invoked\nto handle. That function was explicitly written explicitly for that\none case, hence the assert. The assert might be triggered, for\nexample, if get_unmerged() were changed to return entries in a\ndifferent order and someone decides to make the F->D case be\nunprocessed by process_entry() as well (and forgets to update\nprocess_df_entry).\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index b0f055e..7bab728 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1208,9 +1208,8 @@ static int process_df_entry(struct merge_options *o,\n>        const char *conf;\n>        struct stat st;\n>\n> -       /* We currently only handle D->F cases */\n> -       assert((!o_sha && a_sha && !b_sha) ||\n> -              (!o_sha && !a_sha && b_sha));\n> +       if (! ((!o_sha && a_sha && !b_sha) || (!o_sha && !a_sha && b_sha)))\n> +               return 1; /* we don't handle non D-F cases */\n>\n>        entry->processed = 1;\n>\n> @@ -1321,6 +1320,12 @@ int merge_trees(struct merge_options *o,\n>                                && !process_df_entry(o, path, e))\n>                                clean = 0;\n>                }\n> +               for (i = 0; i < entries->nr; i++) {\n> +                       struct stage_data *e = entries->items[i].util;\n> +                       if (!e->processed)\n> +                               die(\"Unprocessed path??? %s\",\n> +                                   entries->items[i].string);\n> +               }\n>\n>                string_list_clear(re_merge, 0);\n>                string_list_clear(re_head, 0);\n>\n\nOther than possible wording of the comment (\"we only handle\ndirectory/file conflicts where the path was not a directory in the\nmerge-base\"? \"we don't currently handle any other cases\"? something\nelse?), the patch looks good to me.\n\nElijah\n"},{"id":"148416","messageId":"7vk4nn8soz.fsf@alter.siamese.dyndns.org","threadId":"24725","inReplyTo":"AANLkTik9mho7Pm5m=3dEbtMrH1r81Lc480i_z2_aHCxs@mail.gmail.com","subject":"Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-18T22:25:00Z","receivedAt":"2010-08-18T22:25:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> So, I'd say it is more about programming errors, in particular ones\n> where people modify the code to make process_entry() leave more cases\n> unprocessed than what is currently possible without also making the\n> necessary modifications to process_df_entry().\n\nYeah. But they do not need to touch process_df_entry().\n\nI actually was hoping that my weatherbaloon patch will illustrate that a\nnew special case these people may make to process_entry() to leave other\ncases unprocessed do _NOT_ have to be handled by process_df_entry().\n\nThe \"if\" statement in process_df_entry() would check if the entry is\nsomething the function is ready to resolve, and otherwise punts. A new\nexception they add to process_entry() can introduce a separate phase (just\nlike process_df_entry() is not done in parallel with other kinds of\nentries inside the process_entry() but as a separate post-processing\nphase) between the loop that calls process_df_entry() and the loop that\nchecks if there is a remaining entry. And it probably should, as such a\nnew exception may not have anything to do with \"df\", and adding such a\nlogic to process_df_entry() would be wrong ;-).\n"},{"id":"148422","messageId":"AANLkTikVWd60Pe3-X2xj4mWhtiTf9VgydYJ7HKxsp+dd@mail.gmail.com","threadId":"24725","inReplyTo":"7vk4nn8soz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2010-08-18T23:13:40Z","receivedAt":"2010-08-18T23:13:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 18, 2010 at 4:25 PM, Junio C Hamano wrote:\n> I actually was hoping that my weatherbaloon patch will illustrate that a\n> new special case these people may make to process_entry() to leave other\n> cases unprocessed do _NOT_ have to be handled by process_df_entry().\n>\n> The \"if\" statement in process_df_entry() would check if the entry is\n> something the function is ready to resolve, and otherwise punts.  A new\n> exception they add to process_entry() can introduce a separate phase (just\n> like process_df_entry() is not done in parallel with other kinds of\n> entries inside the process_entry() but as a separate post-processing\n> phase) between the loop that calls process_df_entry() and the loop that\n> checks if there is a remaining entry.  And it probably should, as such a\n> new exception may not have anything to do with \"df\", and adding such a\n> logic to process_df_entry() would be wrong ;-).\n\nThat makes sense and sounds like a good idea to me. I think we should\ngo with your patch, modulo possibly modifying the comment's wording.\n"}]}