{"thread":{"id":"30668","subject":"[PATCH] Remove perl dependency from git-submodule.sh","startedAt":"2012-05-31T08:48:46Z","lastAt":"2012-05-31T19:26:08Z","messageCount":9,"participants":["Fredrik Gustafsson","Ævar Arnfjörð Bjarmason","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"192548","messageId":"1338454126-30441-1-git-send-email-iveqy@iveqy.com","threadId":"30668","inReplyTo":null,"subject":"[PATCH] Remove perl dependency from git-submodule.sh","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2012-05-31T08:48:46Z","receivedAt":"2012-05-31T08:48:46Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"Rewrote a perl section in sh.\n\nThe code may be a bit slower (doing grep on strings instead of using\nperl-lists).\n\nSigned-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\n---\n git-submodule.sh |   35 ++++++++++++++++++-----------------\n 1 files changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex f46862f..d3dfb24 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -73,24 +73,25 @@ resolve_relative_url ()\n #\n module_list()\n {\n+\tunmerged=\n+\tnull_sha1=0000000000000000000000000000000000000000\n \tgit ls-files --error-unmatch --stage -- \"$@\" |\n-\tperl -e '\n-\tmy %unmerged = ();\n-\tmy ($null_sha1) = (\"0\" x 40);\n-\twhile (<STDIN>) {\n-\t\tchomp;\n-\t\tmy ($mode, $sha1, $stage, $path) =\n-\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n-\t\tnext unless $mode eq \"160000\";\n-\t\tif ($stage ne \"0\") {\n-\t\t\tif (!$unmerged{$path}++) {\n-\t\t\t\tprint \"$mode $null_sha1 U\\t$path\\n\";\n-\t\t\t}\n-\t\t\tnext;\n-\t\t}\n-\t\tprint \"$_\\n\";\n-\t}\n-\t'\n+\twhile read mode sha1 stage path\n+\tdo\n+\t\tif test $mode -eq 160000\n+\t\tthen\n+\t\t\tif test $stage -ne 0\n+\t\t\tthen\n+\t\t\t\tif test -z \"$(echo $unmerged | grep \"|$path|\")\"\n+\t\t\t\t\tthen\n+\t\t\t\t\techo \"$mode $null_sha1 U\\t$path\"\n+\t\t\t\tfi\n+\t\t\t\tunmerged=\"$unmerged|$path|\"\n+\t\t\telse\n+\t\t\t\techo \"$mode $sha1 $stage\\t$path\"\n+\t\t\tfi\n+\t\tfi\n+\tdone\n }\n \n #\n-- \n1.7.5.1180.g4bfe7.dirty\n"},{"id":"192549","messageId":"CACBZZX6fV5PpgV6GMhDMYgXeeP_QYK9jnP15tMn9Y-a-O6Pn3A@mail.gmail.com","threadId":"30668","inReplyTo":"1338454126-30441-1-git-send-email-iveqy@iveqy.com","subject":"Re: [PATCH] Remove perl dependency from git-submodule.sh","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-05-31T09:07:22Z","receivedAt":"2012-05-31T09:07:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, May 31, 2012 at 10:48 AM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:\n> Rewrote a perl section in sh.\n>\n> The code may be a bit slower (doing grep on strings instead of using\n> perl-lists).\n\nMay be? Did you test it, if so what's the difference?\n"},{"id":"192550","messageId":"4FC73788.6070805@viscovery.net","threadId":"30668","inReplyTo":"1338454126-30441-1-git-send-email-iveqy@iveqy.com","subject":"Re: [PATCH] Remove perl dependency from git-submodule.sh","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-05-31T09:19:04Z","receivedAt":"2012-05-31T09:19:04Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/31/2012 10:48, schrieb Fredrik Gustafsson:\n> Rewrote a perl section in sh.\n\n> The code may be a bit slower (doing grep on strings instead of using\n> perl-lists).\n\n\"A lot\" would be more correct on Windows :-) But it can be avoided, I think.\n\n>  module_list()\n>  {\n> +\tunmerged=\n> +\tnull_sha1=0000000000000000000000000000000000000000\n>  \tgit ls-files --error-unmatch --stage -- \"$@\" |\n> -\tperl -e '\n> -\tmy %unmerged = ();\n> -\tmy ($null_sha1) = (\"0\" x 40);\n> -\twhile (<STDIN>) {\n> -\t\tchomp;\n> -\t\tmy ($mode, $sha1, $stage, $path) =\n> -\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n> -\t\tnext unless $mode eq \"160000\";\n> -\t\tif ($stage ne \"0\") {\n> -\t\t\tif (!$unmerged{$path}++) {\n> -\t\t\t\tprint \"$mode $null_sha1 U\\t$path\\n\";\n> -\t\t\t}\n> -\t\t\tnext;\n> -\t\t}\n> -\t\tprint \"$_\\n\";\n> -\t}\n> -\t'\n> +\twhile read mode sha1 stage path\n\nBe prepared for backslashes in the path name:\n\n\twhile read -r mode sha1 stage path\n\n> +\tdo\n> +\t\tif test $mode -eq 160000\n\n$mode is not a number, but a string: test \"$mode\" = 160000\n\n> +\t\tthen\n> +\t\t\tif test $stage -ne 0\n\nThat $stage looks like a number is of no importance, either.\n\n> +\t\t\tthen\n> +\t\t\t\tif test -z \"$(echo $unmerged | grep \"|$path|\")\"\n> +\t\t\t\t\tthen\n> +\t\t\t\t\techo \"$mode $null_sha1 U\\t$path\"\n> +\t\t\t\tfi\n> +\t\t\t\tunmerged=\"$unmerged|$path|\"\n\nIIUC, the purpose of $unmerged and this check is to avoid that an unmerged\npath is dumped for each stage that is listed by ls-files. Therefore it\nshould be sufficient to just check that the current path is different from\nthe last path.\n\n> +\t\t\telse\n> +\t\t\t\techo \"$mode $sha1 $stage\\t$path\"\n> +\t\t\tfi\n> +\t\tfi\n> +\tdone\n>  }\n>  \n>  #\n\n-- Hannes\n"},{"id":"192554","messageId":"20120531103713.GA30500@paksenarrion.iveqy.com","threadId":"30668","inReplyTo":"CACBZZX6fV5PpgV6GMhDMYgXeeP_QYK9jnP15tMn9Y-a-O6Pn3A@mail.gmail.com","subject":"Re: [PATCH] Remove perl dependency from git-submodule.sh","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2012-05-31T10:37:13Z","receivedAt":"2012-05-31T10:37:13Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Thu, May 31, 2012 at 11:07:22AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> On Thu, May 31, 2012 at 10:48 AM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:\n> > Rewrote a perl section in sh.\n> >\n> > The code may be a bit slower (doing grep on strings instead of using\n> > perl-lists).\n> \n> May be? Did you test it, if so what's the difference?\n\nI did not test it. I think that it can be easily judged by someone that\nknows how perl implements its arrays. With an array implementation with\na hashmap-solution, it should be slower, of course depending on the\nnumber of submodules.\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"192555","messageId":"20120531104036.GB30500@paksenarrion.iveqy.com","threadId":"30668","inReplyTo":"4FC73788.6070805@viscovery.net","subject":"Re: [PATCH] Remove perl dependency from git-submodule.sh","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2012-05-31T10:40:36Z","receivedAt":"2012-05-31T10:40:36Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Thu, May 31, 2012 at 11:19:04AM +0200, Johannes Sixt wrote:\n> Am 5/31/2012 10:48, schrieb Fredrik Gustafsson:\n> > Rewrote a perl section in sh.\n> \n> > The code may be a bit slower (doing grep on strings instead of using\n> > perl-lists).\n> \n> \"A lot\" would be more correct on Windows :-) But it can be avoided, I think.\n> \n> >  module_list()\n> >  {\n> > +\tunmerged=\n> > +\tnull_sha1=0000000000000000000000000000000000000000\n> >  \tgit ls-files --error-unmatch --stage -- \"$@\" |\n> > -\tperl -e '\n> > -\tmy %unmerged = ();\n> > -\tmy ($null_sha1) = (\"0\" x 40);\n> > -\twhile (<STDIN>) {\n> > -\t\tchomp;\n> > -\t\tmy ($mode, $sha1, $stage, $path) =\n> > -\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n> > -\t\tnext unless $mode eq \"160000\";\n> > -\t\tif ($stage ne \"0\") {\n> > -\t\t\tif (!$unmerged{$path}++) {\n> > -\t\t\t\tprint \"$mode $null_sha1 U\\t$path\\n\";\n> > -\t\t\t}\n> > -\t\t\tnext;\n> > -\t\t}\n> > -\t\tprint \"$_\\n\";\n> > -\t}\n> > -\t'\n> > +\twhile read mode sha1 stage path\n> \n> Be prepared for backslashes in the path name:\n> \n> \twhile read -r mode sha1 stage path\n\nWe are not using -r on any place in git-submodule.sh. Maybe we should? I\ncan provide a patch if needed.\n\n> \n> > +\tdo\n> > +\t\tif test $mode -eq 160000\n> \n> $mode is not a number, but a string: test \"$mode\" = 160000\n\nokay, fixed in next iteration.\n\n> \n> > +\t\tthen\n> > +\t\t\tif test $stage -ne 0\n> \n> That $stage looks like a number is of no importance, either.\n\nActually I don't know what stage does and if it's important here. This\npart is just to mimic the perl code. Should it be removed?\n\n> \n> > +\t\t\tthen\n> > +\t\t\t\tif test -z \"$(echo $unmerged | grep \"|$path|\")\"\n> > +\t\t\t\t\tthen\n> > +\t\t\t\t\techo \"$mode $null_sha1 U\\t$path\"\n> > +\t\t\t\tfi\n> > +\t\t\t\tunmerged=\"$unmerged|$path|\"\n> \n> IIUC, the purpose of $unmerged and this check is to avoid that an unmerged\n> path is dumped for each stage that is listed by ls-files. Therefore it\n> should be sufficient to just check that the current path is different from\n> the last path.\n\nThat requires that submodules always is in the same order, right? That\nwould not work for:\nsub1\nsub2\nsub1\n\ncase. But is that order really a possibility or is it always\nsub1\nsub1\nsub2\n\n?\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"192561","messageId":"4FC75520.2090601@viscovery.net","threadId":"30668","inReplyTo":"20120531104036.GB30500@paksenarrion.iveqy.com","subject":"Re: [PATCH] Remove perl dependency from git-submodule.sh","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-05-31T11:25:20Z","receivedAt":"2012-05-31T11:25:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/31/2012 12:40, schrieb Fredrik Gustafsson:\n> On Thu, May 31, 2012 at 11:19:04AM +0200, Johannes Sixt wrote:\n>> Be prepared for backslashes in the path name:\n>>\n>> \twhile read -r mode sha1 stage path\n> \n> We are not using -r on any place in git-submodule.sh. Maybe we should? I\n> can provide a patch if needed.\n\nI can imagine that this would fix a bug or two with paths that contain\nto-be-quoted characters.\n\n>>> +\tdo\n>>> +\t\tif test $mode -eq 160000\n>>\n>> $mode is not a number, but a string: test \"$mode\" = 160000\n> \n> okay, fixed in next iteration.\n> \n>>\n>>> +\t\tthen\n>>> +\t\t\tif test $stage -ne 0\n>>\n>> That $stage looks like a number is of no importance, either.\n> \n> Actually I don't know what stage does and if it's important here. This\n> part is just to mimic the perl code. Should it be removed?\n\nNo; you should 'test \"$stage\" != 0'.\n\n>>> +\t\t\tthen\n>>> +\t\t\t\tif test -z \"$(echo $unmerged | grep \"|$path|\")\"\n>>> +\t\t\t\t\tthen\n>>> +\t\t\t\t\techo \"$mode $null_sha1 U\\t$path\"\n>>> +\t\t\t\tfi\n>>> +\t\t\t\tunmerged=\"$unmerged|$path|\"\n>>\n>> IIUC, the purpose of $unmerged and this check is to avoid that an unmerged\n>> path is dumped for each stage that is listed by ls-files. Therefore it\n>> should be sufficient to just check that the current path is different from\n>> the last path.\n> \n> That requires that submodules always is in the same order, right?\n\nls-files guarantees a suitable order: different stages of the same\nsubmodule path appear on consecutive lines.\n\n-- Hannes\n"},{"id":"192604","messageId":"7vpq9k6y16.fsf@alter.siamese.dyndns.org","threadId":"30668","inReplyTo":"20120531104036.GB30500@paksenarrion.iveqy.com","subject":"Re: [PATCH] Remove perl dependency from git-submodule.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-31T17:49:41Z","receivedAt":"2012-05-31T17:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\n\n> On Thu, May 31, 2012 at 11:19:04AM +0200, Johannes Sixt wrote:\n>> Am 5/31/2012 10:48, schrieb Fredrik Gustafsson:\n>> > Rewrote a perl section in sh.\n>> \n>> > The code may be a bit slower (doing grep on strings instead of using\n>> > perl-lists).\n>> \n>> \"A lot\" would be more correct on Windows :-) But it can be avoided, I think.\n>> \n>> >  module_list()\n>> >  {\n>> > +\tunmerged=\n>> > +\tnull_sha1=0000000000000000000000000000000000000000\n>> >  \tgit ls-files --error-unmatch --stage -- \"$@\" |\n>> > -\tperl -e '\n>> > -\tmy %unmerged = ();\n>> > -\tmy ($null_sha1) = (\"0\" x 40);\n>> > -\twhile (<STDIN>) {\n>> > -\t\tchomp;\n>> > -\t\tmy ($mode, $sha1, $stage, $path) =\n>> > -\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n>> > -\t\tnext unless $mode eq \"160000\";\n>> > -\t\tif ($stage ne \"0\") {\n>> > -\t\t\tif (!$unmerged{$path}++) {\n>> > -\t\t\t\tprint \"$mode $null_sha1 U\\t$path\\n\";\n>> > -\t\t\t}\n>> > -\t\t\tnext;\n>> > -\t\t}\n>> > -\t\tprint \"$_\\n\";\n>> > -\t}\n>> > -\t'\n>> > +\twhile read mode sha1 stage path\n>> \n>> Be prepared for backslashes in the path name:\n>> \n>> \twhile read -r mode sha1 stage path\n>\n> We are not using -r on any place in git-submodule.sh. Maybe we should? I\n> can provide a patch if needed.\n\nI've already written off \"git-submodule.sh\" script as unfriendly to\npathnames with funny characters in them.  Using \"read -r\" may work\naround backslashes in the path, but you won't be able to sensibly\nhandle LFs in filenames, for example.  In other words, we could just\ntell users \"don't use funny pathnames\"---and from that point of\nview, rewriting the Perl scriptlet with a pure shell implementation\nthat does not fork other little tools is sensible, especially if it\nresults in better performance, more readable code, etc.  \n\nHaving said that, in the longer term, I think the right direction to\ngo is the opposite.  It would be better to make \"git-submodule.sh\"\nwork better with paths with funny characters in them, and one\nobvious approach is to read \"ls-files -z\" output with something\ncapable of parsing NUL-terminated records, e.g. a Perl scriptlet.\nAdding a new shell loop like this patch only adds one place that\nneeds to be fixed later when that happens, so I am not sure I like\nthis patch.\n"},{"id":"192613","messageId":"20120531184841.GA32131@paksenarrion.iveqy.com","threadId":"30668","inReplyTo":"7vpq9k6y16.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Remove perl dependency from git-submodule.sh","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2012-05-31T18:48:41Z","receivedAt":"2012-05-31T18:48:41Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Thu, May 31, 2012 at 10:49:41AM -0700, Junio C Hamano wrote:\n> Having said that, in the longer term, I think the right direction to\n> go is the opposite.  It would be better to make \"git-submodule.sh\"\n> work better with paths with funny characters in them, and one\n> obvious approach is to read \"ls-files -z\" output with something\n> capable of parsing NUL-terminated records, e.g. a Perl scriptlet.\n> Adding a new shell loop like this patch only adds one place that\n> needs to be fixed later when that happens, so I am not sure I like\n> this patch.\n\nIs perl really a dependency that git wants? Today only a few bit (often\nnon critical) are in perl. I thought the way was to get rid of those and\nreplace them with c? I'm very critical to dependencies when they are not needed.\n\nI don't think forking for text-parsing when not needed is a good idea\neither. Apart from the runtime issues, it makes the code harder to read.\n\nWith that said I do agree that funny path names should be supported and\nmaybe the correct solution is to make more use of perl and less of sh.\nMixing those, and doing it in the same file, I don't think is a good\nidea.\n\nIs the right direction to run a shellscript that invokes a\nperl-scriptlet for textparsing?\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"192615","messageId":"7v7gvs6tkf.fsf@alter.siamese.dyndns.org","threadId":"30668","inReplyTo":"20120531184841.GA32131@paksenarrion.iveqy.com","subject":"Re: [PATCH] Remove perl dependency from git-submodule.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-31T19:26:08Z","receivedAt":"2012-05-31T19:26:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\n\n> On Thu, May 31, 2012 at 10:49:41AM -0700, Junio C Hamano wrote:\n>> Having said that, in the longer term, I think the right direction to\n>> go is the opposite.  It would be better to make \"git-submodule.sh\"\n>> work better with paths with funny characters in them, and one\n>> obvious approach is to read \"ls-files -z\" output with something\n>> capable of parsing NUL-terminated records, e.g. a Perl scriptlet.\n>> Adding a new shell loop like this patch only adds one place that\n>> needs to be fixed later when that happens, so I am not sure I like\n>> this patch.\n>\n> Is perl really a dependency that git wants?\n\nIt depends on your definition of \"want\"; I'd say \"if alternative is\nto lose things like functionality, performance, etc., we would\nrather live with it.\"\n\nIt is one of the more widely available scripting languages whose\nscripts are more portable across platforms (sadly, we ought to be\nable to use sed and awk which are more available but we have seen\nportability issues with them); if we want to step outside of what\ncan be done with POSIX shell scripts (e.g. handling NUL terminated\nstream) but are not ready to rewrite everything in C, I would say it\nis the least evil among others.  So the short answer is yes.\n\n> Today only a few bit (often\n> non critical) are in perl. I thought the way was to get rid of those and\n> replace them with c?\n\nBut your patch does not help us bring ourselves any closer to\nreplace anything with C at all.\n\n> I'm very critical to dependencies when they are not needed.\n\nThe key-phrase is \"when they are not needed\".  \n\nIf the patch were to replace it with awk, sed or shell *without*\nlosing functionality, performance, readability, portability &\nmaintainability, it would be giving us one less dependency without\nlosing anything.  It may not be replacing everything with C, but at\nleast it would not be going backwards.\n\nOn the other hand, if the patch were to replace Perl scriptlet with\nruby or python, that would be adding unnecessary extra dependencies,\nas we do not have anything written in them in the core.  To such a\npatch, we can confidently say \"It adds unnecessary dependency\nwithout value\" and reject it.\n"}]}