{"thread":{"id":"16712","subject":"[patch] Fix a corner case in git update-index --index-info","startedAt":"2008-12-13T13:03:06Z","lastAt":"2008-12-15T10:52:25Z","messageCount":3,"participants":["Thomas Jarosch","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"97791","messageId":"200812131403.08740.thomas.jarosch@intra2net.com","threadId":"16712","inReplyTo":null,"subject":"[patch] Fix a corner case in git update-index --index-info","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2008-12-13T13:03:06Z","receivedAt":"2008-12-13T13:03:06Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"Fix a corner case in git update-index --index-info:\nIf there are no input lines, it won't create an empty index.\n\nHere's a short test for this:\necho -n \"\" |GIT_INDEX_FILE=index.new git update-index --index-info\n-> The index \"index.new\" won't get created\n\nIt failed for me while I was using\ngit filter-branch as described in the man page:\n\n    git filter-branch --index-filter \\\n            ´git ls-files -s | sed \"s-\\t-&newsubdir/-\" |\n                    GIT_INDEX_FILE=$GIT_INDEX_FILE.new \\\n                            git update-index --index-info &&\n            mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE´ HEAD\n\nThe conversion aborted because the first commit was empty.\n(created by cvs2svn)\n\nSigned-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>\n\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 65d5775..998a48e 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -299,6 +299,7 @@ static void read_index_info(int line_termination)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf uq = STRBUF_INIT;\n+\tint found_something = 0;\n \n \twhile (strbuf_getline(&buf, stdin, line_termination) != EOF) {\n \t\tchar *ptr, *tab;\n@@ -308,6 +309,8 @@ static void read_index_info(int line_termination)\n \t\tunsigned long ul;\n \t\tint stage;\n \n+\t\tfound_something = 1;\n+\n \t\t/* This reads lines formatted in one of three formats:\n \t\t *\n \t\t * (1) mode         SP sha1          TAB path\n@@ -383,6 +386,11 @@ static void read_index_info(int line_termination)\n \tbad_line:\n \t\tdie(\"malformed index info %s\", buf.buf);\n \t}\n+\n+\t/* Force creation of empty index - needed by git filter-branch */\n+\tif (!found_something)\n+\t\tactive_cache_changed = 1;\n+\n \tstrbuf_release(&buf);\n \tstrbuf_release(&uq);\n }\n"},{"id":"97808","messageId":"7viqpn6fhz.fsf@gitster.siamese.dyndns.org","threadId":"16712","inReplyTo":"200812131403.08740.thomas.jarosch@intra2net.com","subject":"Re: [patch] Fix a corner case in git update-index --index-info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-13T19:29:12Z","receivedAt":"2008-12-13T19:29:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Jarosch <thomas.jarosch@intra2net.com> writes:\n\n> Fix a corner case in git update-index --index-info:\n> If there are no input lines, it won't create an empty index.\n>\n> Here's a short test for this:\n> echo -n \"\" |GIT_INDEX_FILE=index.new git update-index --index-info\n> -> The index \"index.new\" won't get created\n>\n> It failed for me while I was using\n> git filter-branch as described in the man page:\n>\n>     git filter-branch --index-filter \\\n>             ´git ls-files -s | sed \"s-\\t-&newsubdir/-\" |\n>                     GIT_INDEX_FILE=$GIT_INDEX_FILE.new \\\n>                             git update-index --index-info &&\n>             mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE´ HEAD\n>\n> The conversion aborted because the first commit was empty.\n> (created by cvs2svn)\n\nIf you are doing a filter-branch and the commits near the beginning of the\nhistory did not have any path you are interested in, I do not think you\nwould want to even create corresponding commits for them that record an\nempty tree to begin with, so I do not necessarily agree with the above\ncommand line.  The mv would fail due to absense of index.new file, and you\ncan take it as a sign that you can skip that commit.\n\nOutside the context of your command line above, I am slightly more\nsympathetic than neutral to the argument that \"update-index --index-info\"\n(and \"update-index --stdin\", which I suspect would have the same issue,\nbut I did not check) should create an output file if one did not exist.\n\nYou should note however that such a change would rob from you a way to\ndetect that you did not feed anything to the command by checking the lack\nof the output.  Such a change would break people's existing scripts that\nrelied on the existing behaviour; one example is that the above \"The mv\nwould fail...and you can\" would be made impossible.\n\n> @@ -308,6 +309,8 @@ static void read_index_info(int line_termination)\n>  \t\tunsigned long ul;\n>  \t\tint stage;\n>  \n> +\t\tfound_something = 1;\n> +\n>  \t\t/* This reads lines formatted in one of three formats:\n>  \t\t *\n>  \t\t * (1) mode         SP sha1          TAB path\n> @@ -383,6 +386,11 @@ static void read_index_info(int line_termination)\n>  \tbad_line:\n>  \t\tdie(\"malformed index info %s\", buf.buf);\n>  \t}\n> +\n> +\t/* Force creation of empty index - needed by git filter-branch */\n\nAs I already mentioned, I do not agree with this \"needed by\" at all.\n\n> +\tif (!found_something)\n> +\t\tactive_cache_changed = 1;\n> +\n>  \tstrbuf_release(&buf);\n>  \tstrbuf_release(&uq);\n>  }\n\nI think this implementation is conceptually wrong, even if we assume it is\nthe right thing to always create a new file.  The --index-info mode may\nwell be fed with the same information as it already records, in which case\nactive_cache_changed shouldn't be toggled, and if it is fed something\ndifferent from what is recorded, active_cache_changed should be marked as\nchanged, and that decision should be left to the add_cache_entry() that is\ncalled from add_cacheinfo().  What you did is to make it _always_ write\nthe new index out, even if we started with an existing index, and there\nwas no change, or even if we started with missing index, and there was no\ninput.  You only wanted the latter but you did both.\n\nIf that is what you want to do, you can get rid of found_something logic\naltogether and flip active_cache_changed unconditionally to do the same\nthing, but it feels wrong to write the index out when nothing changed.\n\nSince I said I am slightly more sympathetic than neutral, here is a patch\nthat forces creation of an empty index file without making it write out\nthe same thing if the index already existed.\n\nBut again, this would break people who have been relying on the existing\nbehaviour that no resulting file, when GIT_INDEX_FILE points at a\nnonexistent file, signals no operation.\n\nI think it is a bad idea to do this in -rc period, even if we were to\nchange the semantics.\n\n builtin-update-index.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git c/builtin-update-index.c w/builtin-update-index.c\nindex 65d5775..9abc0b2 100644\n--- c/builtin-update-index.c\n+++ w/builtin-update-index.c\n@@ -566,6 +566,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tchar set_executable_bit = 0;\n \tunsigned int refresh_flags = 0;\n \tint lock_error = 0;\n+\tint was_unborn;\n \tstruct lock_file *lock_file;\n \n \tgit_config(git_default_config, NULL);\n@@ -580,6 +581,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tentries = read_cache();\n \tif (entries < 0)\n \t\tdie(\"cache corrupted\");\n+\twas_unborn = is_cache_unborn();\n \n \tfor (i = 1 ; i < argc; i++) {\n \t\tconst char *path = argv[i];\n@@ -677,7 +679,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tdie(\"--index-info must be at the end\");\n \t\t\t\tallow_add = allow_replace = allow_remove = 1;\n \t\t\t\tread_index_info(line_termination);\n-\t\t\t\tbreak;\n+\t\t\t\tgoto finish;\n \t\t\t}\n \t\t\tif (!strcmp(path, \"--unresolve\")) {\n \t\t\t\thas_errors = do_unresolve(argc - i, argv + i,\n@@ -738,7 +740,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t}\n \n  finish:\n-\tif (active_cache_changed) {\n+\tif (active_cache_changed || was_unborn) {\n \t\tif (newfd < 0) {\n \t\t\tif (refresh_flags & REFRESH_QUIET)\n \t\t\t\texit(128);\n"},{"id":"97949","messageId":"200812151152.59451.thomas.jarosch@intra2net.com","threadId":"16712","inReplyTo":"7viqpn6fhz.fsf@gitster.siamese.dyndns.org","subject":"Re: [patch] Fix a corner case in git update-index --index-info","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2008-12-15T10:52:25Z","receivedAt":"2008-12-15T10:52:25Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"On Saturday, 13. December 2008 20:29:12 you wrote:\n> If you are doing a filter-branch and the commits near the beginning of the\n> history did not have any path you are interested in, I do not think you\n> would want to even create corresponding commits for them that record an\n> empty tree to begin with, so I do not necessarily agree with the above\n> command line.  The mv would fail due to absense of index.new file, and you\n> can take it as a sign that you can skip that commit.\n\nTrue. I killed the empty commit later using rebase -i. Great tool :-)\n\n> Outside the context of your command line above, I am slightly more\n> sympathetic than neutral to the argument that \"update-index --index-info\"\n> (and \"update-index --stdin\", which I suspect would have the same issue,\n> but I did not check) should create an output file if one did not exist.\n>\n> You should note however that such a change would rob from you a way to\n> detect that you did not feed anything to the command by checking the lack\n> of the output.  Such a change would break people's existing scripts that\n> relied on the existing behaviour; one example is that the above \"The mv\n> would fail...and you can\" would be made impossible.\n\nThat is also true. OTOH there would be no way to create an empty tree,\nf.e. if you do positive filtering like --subdirectory-filter\njust with multiple subdirs:\n\ngit filter-branch --tag-name-filter cat --index-filter \\\n    'git ls-files -s |grep -P \"\\t(DIR1|DIR2)\" \\\n    |GIT_INDEX_FILE=$GIT_INDEX_FILE.new git update-index --index-info &&\n    mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE' -- --all\n\nLater on I removed all empty commits in a second run.\n\n> > +\tif (!found_something)\n> > +\t\tactive_cache_changed = 1;\n> > +\n> >  \tstrbuf_release(&buf);\n> >  \tstrbuf_release(&uq);\n> >  }\n>\n> I think this implementation is conceptually wrong, even if we assume it is\n> the right thing to always create a new file.  The --index-info mode may\n> well be fed with the same information as it already records, in which case\n> active_cache_changed shouldn't be toggled, and if it is fed something\n> different from what is recorded, active_cache_changed should be marked as\n> changed, and that decision should be left to the add_cache_entry() that is\n> called from add_cacheinfo().  What you did is to make it _always_ write\n> the new index out, even if we started with an existing index, and there\n> was no change, or even if we started with missing index, and there was no\n> input.  You only wanted the latter but you did both.\n\nThe idea was to toggle the active_cache_changed variable only if we didn't get \na single line of input from stdin. If you feed back the same index information\nf.e. via \"git ls-tree -s\", the active_cache_changed=1 code shouldn't be \nexecuted. Though I didn't explicitly test this case, so I guess you are right.\n\n> But again, this would break people who have been relying on the existing\n> behaviour that no resulting file, when GIT_INDEX_FILE points at a\n> nonexistent file, signals no operation.\n\nSee my remark about \"positive list\" filtering above.\n\n> I think it is a bad idea to do this in -rc period, even if we were to\n> change the semantics.\n\nYes, this is something one doesn't want in a -rc :-)\n\nThanks for your implementation.\n\nbtw: I sent a small documentation update to the list and forgot to add you\nto the CC: list. The subject line was\n\n\"[patch] documentation: Explain how to free up space after filter-branch\"\n\nCheers,\nThomas\n"}]}