{"thread":{"id":"29564","subject":"[PATCH] add -e: ignore dirty submodules","startedAt":"2012-02-07T04:05:48Z","lastAt":"2012-02-07T20:43:33Z","messageCount":5,"participants":["Johannes Schindelin","Junio C Hamano","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"184090","messageId":"alpine.DEB.1.00.1202070504340.24259@s15462909.onlinehome-server.info","threadId":"29564","inReplyTo":null,"subject":"[PATCH] add -e: ignore dirty submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2012-02-07T04:05:48Z","receivedAt":"2012-02-07T04:05:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWe cannot add untracked/modified files in submodules anyway.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tThis patch is actually from Oct 23, 2010.\n\n builtin/add.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 1c42900..b79336d 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -280,6 +280,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \n \targc = setup_revisions(argc, argv, &rev, NULL);\n \trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n+\tDIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);\n \tout = open(file, O_CREAT | O_WRONLY, 0644);\n \tif (out < 0)\n \t\tdie (_(\"Could not open '%s' for writing.\"), file);\n-- \n1.7.9.msysgit.0.27.ge92cd\n"},{"id":"184097","messageId":"7vaa4v42fi.fsf@alter.siamese.dyndns.org","threadId":"29564","inReplyTo":"alpine.DEB.1.00.1202070504340.24259@s15462909.onlinehome-server.info","subject":"Re: [PATCH] add -e: ignore dirty submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-07T05:50:41Z","receivedAt":"2012-02-07T05:50:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> We cannot add untracked/modified files in submodules anyway.\n\nI can see the updated code would not break \"git apply\" that will be run on\nthis output, but the above cannot be the whole story. It is unclear to me\nwhat it is trying to achieve (in other words, \"this patch does not break\nthe system\" is not the whole purpose of the patch).\n\nWhen a submodule is updated and is dirty, we would get:\n\n diff --git a/submodule b/submodule\n @@ -1,+1 @@\n -Subproject commit XXXX...\n +Subproject commit YYYY...-dirty\n\nand leaving this diff in the edited patch adds YYYY... for submodule, even\nthough \"-dirty\" suffix is there. So it is not fixing \"the user tries to\nupdate but fails because we do not filter dirty submodules\" bug, or somesuch.\nBesides, showing -dirty may be a good reminder that submodule has further\nchanges on top of what is going to be committed in this case.\n\nWhen a submodule is only dirty, we would see:\n\n diff --git a/submodule b/submodule\n @@ -1,+1 @@\n -Subproject commit XXXX...\n +Subproject commit XXXX...-dirty\n\nand leaving this diff in the edited patch keeps the submodule at XXXX...,\nagain without failing, so it is not fixing \"the user gets unnecessary\nerror message\" bug, or somesuch. In this case, leaving this diff will be\na no-op so it is wasteful and distracting to the user who edits the patch.\n\nIs that what this patch is about? \"For a submodule that is unchanged but\nis dirty, submodule diff whose difference is only the '-dirty' suffix is\ngiven but the user cannot update the index with such a diff anyway, so it\nis a waste of space\", or something like that?\n\nThat is the best guess I arrived at, but I suspect that it cannot be it,\nas that discards the \"-dirty\" clue from the output when the submodule path\ndoes have difference, as we saw in the earlier example. So there must be\nsomething I am missing.\n\nSo I am out of ideas guessing what this patch is trying to achieve. The\ncommit log shouldn't force the readers of the history to _guess_ like the\nabove.\n\n> builtin/add.c | 1 +\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 1c42900..b79336d 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -280,6 +280,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n> \n> \targc = setup_revisions(argc, argv, &rev, NULL);\n> \trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n> +\tDIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);\n> \tout = open(file, O_CREAT | O_WRONLY, 0644);\n> \tif (out < 0)\n> \t\tdie (_(\"Could not open '%s' for writing.\"), file);\n"},{"id":"184105","messageId":"alpine.DEB.1.00.1202070124020.4897@bonsai2","threadId":"29564","inReplyTo":"7vaa4v42fi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] add -e: ignore dirty submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2012-02-07T07:45:36Z","receivedAt":"2012-02-07T07:45:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Dear Junio C Hamano,\n\nOn Mon, 6 Feb 2012, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > We cannot add untracked/modified files in submodules anyway.\n\nThe purpose of \"add -e\" is to stage changes. It does so by presenting a\ndiff which the user can edit (and applying it after recounting the numbers\nin the hunk headers).\n\nIf the diff does not apply, it does not make sense to present it to the\nuser.\n\nOffering to add changes represented by a diff like\n\ndiff --git \n
\n-deadbeef...\n+deadbeef...-dirty\n\ndoes not make sense.\n\nEven a diff like\n\ndiff --git \n
\n-deadbeef...\n-coffeebabe...-dirty\n\nwould be refused by git-apply.\n\nIndeed, due to the design of the submodules, it is impossible to stage\ndirty files in a submodule and a supercommit at the same time. Oh, and this\ndiscussion is not the place to wish for a feature like that, just in case\nyou want to ask me to implement that in order to be allowed to have my puny\nlittle patch applied. (I guess this is the reason why I waited so long\nbefore I dared to submit the patch to the mailing list.)\n\nNow, due to these concerns, even stripping out the -dirty part can lead to\na comically non-sensical diff like\n\ndiff --git \n
\n-deadbeef...\n+deadbeef...\n\nSo keeping the diff generation as-is but preparing the patch by munching\naway the -dirty suffix would not fix the problem.\n\nAlso, it would be wrong to assume that the user asked to get a status\nupdate. git add -e != git status. If the user wanted to know what changes\nare in the worktree, including the worktrees of the submodules, but only\nthose that have been checked out, git status would be the command to call\n(even if it was touted as a git commit --dry-run once upon a time, which\nis kind of wrong, see above, you cannot commit the untracked or dirty\nfiles in a submodule, yet git status shows them).\n\nSo: showing the fact that a submodule has untracked or dirty files in the\npatch that the user wants to edit with git add -e is wrong, wrong, wrong.\n\nThe only submodule-related thing we should ever present to the user who\ncalled git add -e is a diff like\n\ndiff --git \n
\n-deadcad...\n+beda551ed...\n\nbecause that is a stageable change.\n\nAlas, salvation is nigh! Yes, just a little line which asks for the level\n\"dirty\" (which implies \"untracked\", as detailed by diff-options.txt, uhm,\nsorry, I was asked to be precise, Documentation/diff-options.txt) fixes\nthat!\n\nWith this flag, there are no more \"-dirty\" lines in the submodule diffs.\nNone! Only diffs between different submodule commits are shown, and even\nif the worktree of the submodule is dirty, or has untracked files, the\n-dirty suffix is omitted. Just what you need for git-apply to work! Yay!\n\nAs a plus, it makes the diff generation faster because what cannot be\nstaged anyway is not even discovered! Super-yay!\n\nI actually enjoyed writing this text so much, may I respectfully ask to\ninclude it verbatim in the commit message?\n\nThank you very much,\nJohannes Schindelin\n"},{"id":"184108","messageId":"7vk43z2gwc.fsf@alter.siamese.dyndns.org","threadId":"29564","inReplyTo":"alpine.DEB.1.00.1202070124020.4897@bonsai2","subject":"Re: [PATCH] add -e: ignore dirty submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-07T08:21:07Z","receivedAt":"2012-02-07T08:21:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Offering to add changes represented by a diff like\n>\n> diff --git \n>
\n> -deadbeef...\n> +deadbeef...-dirty\n>\n> does not make sense.\n\nNow you are being much clearer. If you had the above from the beginning,\nI wouldn't have had to ask.\n\nSo after all, this is a noise reduction patch, and I think that it is a\ngood change.\n\n> Even a diff like\n>\n> diff --git \n>
\n> -deadbeef...\n> -coffeebabe...-dirty\n>\n> would be refused by git-apply.\n\nThat would be refused, but if you replace '-coffeebabe' with '+coffeebabe',\nwhich is what you would get from \"git diff\" without ignore-dirty-submodule,\nI think it would be accepted by \"git apply\" just fine.\n\nHave you tried it?\n\n> I actually enjoyed writing this text so much, may I respectfully ask to\n> include it verbatim in the commit message?\n\nWith mostly irrelevant mumblings and all? Of course not.\n\nI was asking what you meant to achieve with the patch, because you did not\neven include relevant bits of information.\n\nBut now I think I learned what you meant by this patch, so let me try to\nsee if I understood it correctly by rephrasing it.\n\nLike this?\n\n-- >8 --\nSubject: add -e: do not show difference in a submodule that is merely dirty\nFrom: Johannes Schindelin \nDate: Tue, 7 Feb 2012 05:05:48 +0100 (CET)\n\nWhen the HEAD of the submodule matches what is recorded in the index of\nthe superproject, and it has local changes or untracked files, the patch\noffered by \"git add -e\" for editing shows a diff like this:\n\n diff --git a/submodule b/submodule\n
\n -deadbeef...\n +deadbeef...-dirty\n\nBecause applying such a patch has no effect to the index, this is a\nuseless noise. Generate the patch with IGNORE_DIRTY_SUBMODULES flag to\nprevent such a change from getting reported.\n\nThis patch also loses the \"-dirty\" suffix from the output when the HEAD of\nthe submodule is different from what is in the index of the superproject.\nAs such dirtiness expressed by the suffix does not affect the result of\nthe patch application at all, there is no information lost if we remove\nit. The user could still run \"git status\" before \"git add -e\" if s/he\ncares about the dirtiness.\n\nSigned-off-by: Johannes Schindelin \n---\n\n builtin/add.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 1c42900..b79336d 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -280,6 +280,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \n \targc = setup_revisions(argc, argv, &rev, NULL);\n \trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n+\tDIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);\n \tout = open(file, O_CREAT | O_WRONLY, 0644);\n \tif (out < 0)\n \t\tdie (_(\"Could not open '%s' for writing.\"), file);\n"},{"id":"184165","messageId":"4F318CF5.6050302@web.de","threadId":"29564","inReplyTo":"7vk43z2gwc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] add -e: ignore dirty submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-02-07T20:43:33Z","receivedAt":"2012-02-07T20:43:33Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 07.02.2012 09:21, schrieb Junio C Hamano:\n> So after all, this is a noise reduction patch, and I think that it is a\n> good change.\n\nI agree. While my first thought was that it might make sense to honor\nthe diff.ignoreSubmodules setting here too to produce the same\ninformation \"git status\" gives (so you notice you might have forgotten\nto commit something in the submodule), I now agree that doesn't make\nsense in the context of add -e.\n"}]}