{"thread":{"id":"56694","subject":"[BUG] git add -A --dry-run adds files to .git directory","startedAt":"2021-10-12T14:56:06Z","lastAt":"2021-10-12T20:37:25Z","messageCount":5,"participants":["git.mexon@spamgourmet.com","René Scharfe","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"438566","messageId":"a2c0deed-fff3-6c1c-633f-af9dffea1e02@exon.name","threadId":"56694","inReplyTo":null,"subject":"[BUG] git add -A --dry-run adds files to .git directory","fromName":"","fromEmail":"git.mexon@spamgourmet.com","sentAt":"2021-10-12T14:55:59Z","receivedAt":"2021-10-12T14:56:06Z","isPatch":false,"sender":{"key":"git.mexon@spamgourmet.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\nmat@charly:~$ git init repo\nmat@charly:~$ cat /dev/random | head -c 100000000 > repo/randomfile\nmat@charly:~$ git -C repo add -A -n\nmat@charly:~$ du -hs repo/.git\n\nWhat did you expect to happen? (Expected behavior)\n\n  76K    repo/.git\n\nWhat happened instead? (Actual behavior)\n\n  97M    repo/.git\n\nWhat's different between what you expected and what actually happened?\n\nEven though I specified -n on the command line to make a dry run, it \nstill added the large file as an object.\n\nAnything else you want to add:\n\nThis is counter-intuitive behaviour.  When the documentation says \"Don't \nactually add the file(s)\", it's reasonable to expect that this command \nwill leave the repository state as it found it.  If there's a good \nreason why these files should be added to the .git directory, that \nbehaviour should be made clear in the documentation, probably including \nthe reason why that counter-intuitive behaviour is necessary.\n\nThis bit me due to a script that was using git add -A -n to detect if a \nrepository had been locally modified.  While I do have alternative \n(actually better) ways to get this information, it wasn't an \nunreasonable command to use in the first place, and it caused a real \nissue in a production system.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.30.1 (Apple Git-130)\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Darwin 20.6.0 Darwin Kernel Version 20.6.0: Mon Aug 30 06:12:21 \nPDT 2021; root:xnu-7195.141.6~3/RELEASE_X86_64 x86_64\ncompiler info: clang: 13.0.0 (clang-1300.0.29.3)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n\n"},{"id":"438585","messageId":"0131d21f-dabd-3da5-34bd-a570e990f9e0@web.de","threadId":"56694","inReplyTo":"a2c0deed-fff3-6c1c-633f-af9dffea1e02@exon.name","subject":"[PATCH] add: don't write objects with --dry-run","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-12T19:15:50Z","receivedAt":"2021-10-12T19:15:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When the option --dry-run/-n is given, \"git add\" doesn't change the\nindex, but still writes out new object files.  Only hash the latter\nwithout writing instead to make the run as dry as possible.\n\nUse this opportunity to also make the hash_flags variable unsigned,\nto match the index_path() parameter it is used as.\n\nReported-by: git.mexon@spamgourmet.com\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nAm I missing something?  Do we sometimes rely on the written objects\nwithin the \"git add --dry-run\" command?\n\n read-cache.c          | 2 +-\n t/t2200-add-update.sh | 3 +++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex a78b88a41b..7fcc948077 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -738,7 +738,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \tint intent_only = flags & ADD_CACHE_INTENT;\n \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n \t\t\t  (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n-\tint hash_flags = HASH_WRITE_OBJECT;\n+\tunsigned hash_flags = pretend ? 0 : HASH_WRITE_OBJECT;\n \tstruct object_id oid;\n\n \tif (flags & ADD_CACHE_RENORMALIZE)\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex 45ca35d60a..94c4cb0672 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -129,12 +129,15 @@ test_expect_success 'add -n -u should not add but just report' '\n \t\techo \"remove '\\''top'\\''\"\n \t) >expect &&\n \tbefore=$(git ls-files -s check top) &&\n+\tgit count-objects -v >objects_before &&\n \techo changed >>check &&\n \trm -f top &&\n \tgit add -n -u >actual &&\n \tafter=$(git ls-files -s check top) &&\n+\tgit count-objects -v >objects_after &&\n\n \ttest \"$before\" = \"$after\" &&\n+\ttest_cmp objects_before objects_after &&\n \ttest_cmp expect actual\n\n '\n--\n2.33.0\n\n"},{"id":"438587","messageId":"xmqqr1cq2e5o.fsf@gitster.g","threadId":"56694","inReplyTo":"0131d21f-dabd-3da5-34bd-a570e990f9e0@web.de","subject":"Re: [PATCH] add: don't write objects with --dry-run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-12T20:15:15Z","receivedAt":"2021-10-12T20:15:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> When the option --dry-run/-n is given, \"git add\" doesn't change the\n> index, but still writes out new object files.  Only hash the latter\n> without writing instead to make the run as dry as possible.\n>\n> Use this opportunity to also make the hash_flags variable unsigned,\n> to match the index_path() parameter it is used as.\n>\n> Reported-by: git.mexon@spamgourmet.com\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Am I missing something?  Do we sometimes rely on the written objects\n> within the \"git add --dry-run\" command?\n\nGood question.  I do not think of anything offhand, but this obvious\n\"omission\" makes me suspect that we may be forgetting something.\n\nThanks.\n\n\n>  read-cache.c          | 2 +-\n>  t/t2200-add-update.sh | 3 +++\n>  2 files changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index a78b88a41b..7fcc948077 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -738,7 +738,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>  \tint intent_only = flags & ADD_CACHE_INTENT;\n>  \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n>  \t\t\t  (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n> -\tint hash_flags = HASH_WRITE_OBJECT;\n> +\tunsigned hash_flags = pretend ? 0 : HASH_WRITE_OBJECT;\n>  \tstruct object_id oid;\n>\n>  \tif (flags & ADD_CACHE_RENORMALIZE)\n> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\n> index 45ca35d60a..94c4cb0672 100755\n> --- a/t/t2200-add-update.sh\n> +++ b/t/t2200-add-update.sh\n> @@ -129,12 +129,15 @@ test_expect_success 'add -n -u should not add but just report' '\n>  \t\techo \"remove '\\''top'\\''\"\n>  \t) >expect &&\n>  \tbefore=$(git ls-files -s check top) &&\n> +\tgit count-objects -v >objects_before &&\n>  \techo changed >>check &&\n>  \trm -f top &&\n>  \tgit add -n -u >actual &&\n>  \tafter=$(git ls-files -s check top) &&\n> +\tgit count-objects -v >objects_after &&\n>\n>  \ttest \"$before\" = \"$after\" &&\n> +\ttest_cmp objects_before objects_after &&\n>  \ttest_cmp expect actual\n>\n>  '\n> --\n> 2.33.0\n"},{"id":"438588","messageId":"871r4qj8mw.fsf@evledraar.gmail.com","threadId":"56694","inReplyTo":"0131d21f-dabd-3da5-34bd-a570e990f9e0@web.de","subject":"Re: [PATCH] add: don't write objects with --dry-run","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-12T20:17:06Z","receivedAt":"2021-10-12T20:22:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 12 2021, René Scharfe wrote:\n\n> When the option --dry-run/-n is given, \"git add\" doesn't change the\n> index, but still writes out new object files.  Only hash the latter\n> without writing instead to make the run as dry as possible.\n>\n> Use this opportunity to also make the hash_flags variable unsigned,\n> to match the index_path() parameter it is used as.\n>\n> Reported-by: git.mexon@spamgourmet.com\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Am I missing something?  Do we sometimes rely on the written objects\n> within the \"git add --dry-run\" command?\n\nProbably not, here's a semi-related patch of mine that never got\nintegrated. E.g. you'll probably find that even if you're not writing\nobjects we're still doing things like zlib compression here too (or not,\nI haven't looked):\nhttps://lore.kernel.org/git/20190520222932.22843-1-avarab@gmail.com/\n\nI think the \"git fetch --dry-run\" command behaves like this too,\ni.e. doesn't update refs, but fetches and writes objects.\n\nFor the patch I hacked up I think it's easy to argue that it shouldn't\ndo compression etc.\n\nFor this sort of thing and \"fetch\" I'm not so sure. Do we really know\nthat there aren't people who rely on this for say the performance of\nseeing what an operation would do, and then not pay as much for the\n\"real one\" that updates the index/refs/etc. later? Is that subsequent\n\"fetch\" cheaper because of the --dry-run?\n\nMaybe not, but it seems like something to look into.\n"},{"id":"438590","messageId":"xmqqfst62d4u.fsf@gitster.g","threadId":"56694","inReplyTo":"871r4qj8mw.fsf@evledraar.gmail.com","subject":"Re: [PATCH] add: don't write objects with --dry-run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-12T20:37:21Z","receivedAt":"2021-10-12T20:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I think the \"git fetch --dry-run\" command behaves like this too,\n> i.e. doesn't update refs, but fetches and writes objects.\n>\n> For the patch I hacked up I think it's easy to argue that it shouldn't\n> do compression etc.\n>\n> For this sort of thing and \"fetch\" I'm not so sure. Do we really know\n> that there aren't people who rely on this for say the performance of\n> seeing what an operation would do, and then not pay as much for the\n> \"real one\" that updates the index/refs/etc. later? Is that subsequent\n> \"fetch\" cheaper because of the --dry-run?\n\nThe answer to the last one is an easy \"yes\".  Trying to gauge the\ntime it would take for a real fetch with \"--dry-run\" is a losing\nbattle, I would think, as the pre-fetching would make the \"real\" one\ncheaper, so from that point of view, I think we can ignore those who\ntime \"--dry-run\" and try to figure out anything meaningful.\n\nThis in any case is an interesting area, as the definition of\ncorrectness of what \"dry-run\" does can be quite fuzzy.  As long as\nit does not change the index, \"git add --dry-run\", even if it writes\nobjects or detects filesystem corruption by noticing I/O error while\ncompressing the data taken from the working tree files, is still\ncorrect and the patch in question is not technically a bugfix (it is\na performance thing).  \"git fetch --dry-run\" would fall into the\nsame category, so would \"git hash-object\" without \"-w\".\n\nAll can use performance enhancement without breaking existing users,\nI would think.\n\nThanks.\n"}]}