{"thread":{"id":"23729","subject":"git-svn commit parents detection algorithm","startedAt":"2010-05-07T13:35:10Z","lastAt":"2010-05-07T13:35:10Z","messageCount":1,"participants":["Dmitry Pavlenko"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"141124","messageId":"201005071735.10322.dmit10@gmail.com","threadId":"23729","inReplyTo":null,"subject":"git-svn commit parents detection algorithm","fromName":"Dmitry Pavlenko","fromEmail":"dmit10@gmail.com","sentAt":"2010-05-07T13:35:10Z","receivedAt":"2010-05-07T13:35:10Z","isPatch":false,"sender":{"key":"dmit10@gmail.com","avatar":null},"body":"Hello.\n\nI have several questions regarding git-svn parents detection algorithm for the \nnewly created branch. It seems that I have found some bugs.\n\nThe most general question is: could you describe a parents detection algorithm \nin the git-svn?\n\n1. When I try to fetch revision created like this:\nsvn cp trunk branches/branch1\nsvn merge http://host/path/branches/branch2 branches/branch1\nsvn commit\n\nsvn:mergeinfo property is set but git-svn tries to do \"svn switch\"'s low level \nanalog if it's possible. svn:mergeinfo property is not received in this case \n(revealed by setting a breakpoint at change_dir_prop in git-svn script) though \nit's present (\"svn propget\" shows this property). But if \"svn switch\" \noperation is not available in the perl svn client library simple \"svn update\" \noperation is used. I didn't check but suspect that then svn:mergeinfo would be \nprocessed and git-svn's find_extra_svn_parents would find addidtional parents. \nBut I thought that parents set shouldn't depend on the svn version (or at \nleast newer versions of svn library that support \"svn switch\" shouldn't make \nbehaviour worse).\n\nMy opinion is that svn:mergeinfo should be checked in this case and merge \nparents be detected and added to the whole parents set.\n\n2. Is it true that \"find_extra_svn_parents\" requires a reference to a list of \nexactly one parent as 4th argument?\n\nIf @$parents is empty in the function \"git rev-list\" and \"git merge-base\" \ncalls in it would fail because of bad syntax. If @$parents has 2 or more \nelements than merge-base call would have unpredictable behaviour.\n\n$merge_base = command_oneline(\n\t\t\t\t\"merge-base\",\n\t\t\t\t@$parents, $merge_tip,\n\t\t\t);\n\t\t\t\nGit merge-base command finds a merge-base between the first commit and any of \nother commits listed as arguments. So if @$parents is (a, b) \"git merge-base\" \nwill find common ancestor between \"a\" and \"b\" or \"a\" and $merge_tip that is \nnonsense. \n\nMore over if the \"find_extra_svn_parents\" call requires exactly one parent and \n\"make_log_entry\" function in git-svn is always called with one-element parents \nlist then finding additional parents in \"find_extra_svk_parents\" method that  is \ncalled before \"find_extra_svn_parents\" would cause fail of finding extra parents \nin  \"find_extra_svn_parents\".\n\nMaybe these lines should be rewritten as \n\n$merge_base = command_oneline(\n\t\t\t\t\"merge-base\",\n\t\t\t\t$merge_tip, @$parents\n\t\t\t);\nThis makes sense in the case of several parents.\n\n3. What's the purpose of \"parents_exclude\" git-svn's function?\n\nIt is called on reference to already found parents list and @commits that are \nfound as \"merge tips\" of svn:mergeinfo property. But note that svn:mergeinfo \nvalue lines are listed *alphabetically*. For example:\n/branches/branch1:4\n/branches/branch2:3-4,8\n/trunk:2,7\ndisregarding their reverse chronological order(!). So the @commits list would \ncontain hashes that corresponding to (branch1, r4), (branch2, r8), (trunk, r7) \nin this \"alphabetical\" order (note: alphabetical order depends on branch \nnaming).\n\nLet us consider another example: we have commits in @commits: a, b, c, d, e, \nf, g (in this order) and $@parents consists of commit \"h\" . And suppose for \nsimplicity that no commit of a, b, c, d, e, f, g is reachable from \"h\" (than \n\"--not h\" will have no sense). Suppose the commits have the following reverse \nchronological order e, c, f, g, a, b, d.\n\nThen do {} while () in the \"parents_exclude\" will have 3 iterations:\n\n\"git rev-list -1 a b c d e f g --not h\" for this case will return simply the \nlatest \ncommit (e). Then inner cycle will set @new to the slice @new = {a, b, c, d} // \nf, g are rejected because they are after \"e\". We add \"e\" to @excluded and \nrepeat\n\n`git rev-list -1 a b c d --not h` will return c as latest from these. \nNew value of @new will become {a, b} and @excluded {e, c}\n\n`git rev-list -1 a b --not h` will return a.\n@new will become empty and @excluded will contain {e, c, a}.\n\nPersonally I don't know why these commits are better than others.\n\nMoreover the order of {e, c, a} is not used later in the \n\"find_extra_svn_parents\" function.\n\nI have two ideas about parents_exclude's purpose:\n1) it lists a set of all commits not reachable from parents;\n2) it lists a set of all commits not reachable from parents in *chronological* \norder.\n\nTo achieve 1) the \"last;\" operator should be removed from \"parents_exclude\" \nfunction.\nTo achieve 2) the \"last;\" operator should be removed from \"parents_exclude\" \nfunction and the function result should be assigned to @merge_tips list in \n\"find_extra_svn_parents\" function (without  \"map { $_ => 1 }\" construction) to \nsave @excluded commits reverse chronological *order*.\n\nAs for me 2) is the most reasonable because then fetched svn repository \nwouldn't depend on branching naming.\n\nIf parents in @excluded shouldn't be reachable from another parents (that is \ntrue) then\n\nmy @cmd = ('rev-list', \"-1\", @commits, \"--not\", @$parents);\n\nshould be replaced into\n\nmy @cmd = ('rev-list', \"-1\", @commits, \"--not\", @$parents, @excluded );\n\nMaybe this will make useless the check done in the doulbe cycle at the end of \n\"find_extra_svn_parents\":\n\n\tfor ( my $i = 0; $i <= $#new_parents; $i++ ) {\n\t\t\tfor ( my $j = 0; $j <= $#new_parents; $j++ ) {\n\n4. The \"parents_exclude\" function pushes \"undef\" value to @merge_tips (in the \ncase of bad $tip_commit) list and then excludes them by \"grep\" and \"next \nunless $merge_tip and $excluded{$merge_tip};\" line. So pushing \"undef\" is \nuseless operation, it shouldn't perplex.\n\n5. In the function \"do_fetch\" the last commit (my $lc = $self->last_commit) is \n*not* put to the parents list for the \"find_extra_svn_parents\" function in the \ncase when \"copyfrom_path parent\" from \"find_parent_branch\" is found (it is \nadded later then) but is put otherwise. Is it done intentionally or by a \nmistake?\n\n6. Could you please tell in a few word \"linearize_history\" function's goals? \nIn all cases I tested the commits list returned by it was the same as its \ninput hash list. I tried to imagine the situation when these lists differ.\n\nLet \ncommit c1 has parent c2\ncommit c2 has parents c3 and c4\ncommit c3 has parent c4\ncommit c4 has parent c5\n\nIn this case \"linearize_history\" for (c1, c2, c3, c4, c5) list would return \n(c1, c2, c3) but I can't reproduce this situation by \"git\" command (I will try \nusing git low-level library later).\nAdditional question: is it true that \"git svn dcommit\" will loose commits c4 \nand c5 in this strange case?\n"}]}