{"thread":{"id":"38287","subject":"[PATCH v3] doc: core.ignoreStat update, and clarify the --assume-unchanged effect","startedAt":"2015-01-05T22:22:53Z","lastAt":"2015-01-10T18:54:54Z","messageCount":5,"participants":["Philip Oakley","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"254309","messageId":"1420496573-3260-1-git-send-email-philipoakley@iee.org","threadId":"38287","inReplyTo":null,"subject":"[PATCH v3] doc: core.ignoreStat update, and clarify the --assume-unchanged effect","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-01-05T22:22:53Z","receivedAt":"2015-01-05T22:22:53Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"The assume-unchanged bit, and consequently core.ignoreStat, can be\nmisunderstood. Be assertive about the expectation that file changes should\nnotified to Git.\n\nOverhaul the general wording thus:\n 1. direct description of what is ignored given first.\n 2. example instruction of the user manual action required.\n 3. use sideways indirection for assume-unchanged and update-index\n references.\n 4. add a 'normally' to give leeway for the change detection.\n\nSigned-off-by: Philip Oakley \n---\n\nThis is the corrected patch which now applies on top of next and has been\nchecked on AsciiDoc. (was $gmane/261974/focus=262022)\n\nI have included the previous 'after three-dashes' comment and included\nthem in the commit message. I'm happy for it to be tweaked as appropriate.\n\n---\n Documentation/config.txt | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 52eeadd..fe179d0 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -375,15 +375,18 @@ This is useful for excluding servers inside a firewall from\n proxy use, while defaulting to a common proxy for external domains.\n \n core.ignoreStat::\n-\tIf true, commands which modify both the working tree and the index\n-\twill mark the updated paths with the \"assume unchanged\" bit in the\n-\tindex. These marked files are then expected to stay unchanged in the\n-\tworking tree. If you change them you should mark their update manually.\n-\tGit will normally not detect the file changes by lstat() calls.\n-\tThis is useful on systems where those calls are very slow, such as\n-\tcifs/Microsoft Windows.\n-\tSee linkgit:git-update-index[1].\n-\tFalse by default.\n+\tIf true, Git will avoid using lstat() calls to detect if files have\n+\tchanged. Git will set the \"assume-unchanged\" bit for those tracked files\n+\twhich it has updated identically in both the index and working tree.\n++\n+When files are modified outside of Git, the user will need to stage\n+the modified files explicitly (e.g. see 'Examples' section in\n+linkgit:git-update-index[1]).\n+Git will not normally detect changes to those files.\n++\n+This is useful on systems where lstat() calls are very slow, such as\n+CIFS/Microsoft Windows.\n+False by default.\n \n core.preferSymlinkRefs::\n \tInstead of the default \"symref\" format for HEAD\n-- \n1.9.5.msysgit.0\n"},{"id":"254407","messageId":"xmqqbnmajwd9.fsf@gitster.dls.corp.google.com","threadId":"38287","inReplyTo":"1420496573-3260-1-git-send-email-philipoakley@iee.org","subject":"Re: [PATCH v3] doc: core.ignoreStat update, and clarify the --assume-unchanged effect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-07T18:09:38Z","receivedAt":"2015-01-07T18:09:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philip Oakley writes:\n\n> The assume-unchanged bit, and consequently core.ignoreStat, can be\n> misunderstood. Be assertive about the expectation that file changes should\n> notified to Git.\n>\n> Overhaul the general wording thus:\n> 1. direct description of what is ignored given first.\n> 2. example instruction of the user manual action required.\n> 3. use sideways indirection for assume-unchanged and update-index\n> references.\n> 4. add a 'normally' to give leeway for the change detection.\n>\n> Signed-off-by: Philip Oakley \n> ---\n>\n> This is the corrected patch which now applies on top of next and has been\n> checked on AsciiDoc. (was $gmane/261974/focus=262022)\n>\n> I have included the previous 'after three-dashes' comment and included\n> them in the commit message. I'm happy for it to be tweaked as appropriate.\n\nThanks.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 52eeadd..fe179d0 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -375,15 +375,18 @@ This is useful for excluding servers inside a firewall from\n> proxy use, while defaulting to a common proxy for external domains.\n> \n> core.ignoreStat::\n> +\tIf true, Git will avoid using lstat() calls to detect if files have\n> +\tchanged. Git will set the \"assume-unchanged\" bit for those tracked files\n> +\twhich it has updated identically in both the index and working tree.\n\nI wonder if this is better stated in two seemingly independent\nsentences (like your version), or \"... if files have changed by\nsetting the assume-unchanged bit ....\" to clarify where the setting\nof the bits to these files come into the big picture, but it is\nminor. Either way, I think it is a lot easier to understand than\nwhat we have in 'master'.\n\n> ++\n> +When files are modified outside of Git, the user will need to stage\n> +the modified files explicitly (e.g. see 'Examples' section in\n> +linkgit:git-update-index[1]).\n> +Git will not normally detect changes to those files.\n> ++\n> +This is useful on systems where lstat() calls are very slow, such as\n> +CIFS/Microsoft Windows.\n> +False by default.\n\nI think you are trying to make the result more readable by using\nseparate paragraphs for separate conceptual points, but then isn't\nit wrong to have \"False by default\" as part of stating which\nplatforms are intended targets? I wonder if we want to have that\nlast line as its own paragraph instead.\n\nThanks.\n\n> \n> core.preferSymlinkRefs::\n> \tInstead of the default \"symref\" format for HEAD\n"},{"id":"254504","messageId":"1ED88DC1DCC2463782B4F4167DC85D5D@PhilipOakley","threadId":"38287","inReplyTo":"xmqqbnmajwd9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] doc: core.ignoreStat update, and clarify the --assume-unchanged effect","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-01-09T08:44:10Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n> Philip Oakley writes:\n>\n>> The assume-unchanged bit, and consequently core.ignoreStat, can be\n>> misunderstood. Be assertive about the expectation that file changes \n>> should\n>> notified to Git.\n>>\n>> Overhaul the general wording thus:\n>> 1. direct description of what is ignored given first.\n>> 2. example instruction of the user manual action required.\n>> 3. use sideways indirection for assume-unchanged and update-index\n>> references.\n>> 4. add a 'normally' to give leeway for the change detection.\n>>\n>> Signed-off-by: Philip Oakley \n>> ---\n>>\n>> This is the corrected patch which now applies on top of next and has \n>> been\n>> checked on AsciiDoc. (was $gmane/261974/focus=262022)\n>>\n>> I have included the previous 'after three-dashes' comment and \n>> included\n>> them in the commit message. I'm happy for it to be tweaked as \n>> appropriate.\n>\n> Thanks.\n>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index 52eeadd..fe179d0 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -375,15 +375,18 @@ This is useful for excluding servers inside a \n>> firewall from\n>> proxy use, while defaulting to a common proxy for external domains.\n>>\n>> core.ignoreStat::\n>> + If true, Git will avoid using lstat() calls to detect if files have\n>> + changed. Git will set the \"assume-unchanged\" bit for those tracked \n>> files\n>> + which it has updated identically in both the index and working \n>> tree.\n>\n> I wonder if this is better stated in two seemingly independent\n> sentences (like your version), or \"... if files have changed by\n> setting the assume-unchanged bit ....\" to clarify where the setting\n> of the bits to these files come into the big picture, but it is\n> minor. Either way, I think it is a lot easier to understand than\n> what we have in 'master'.\n\nI had considered a number of different wordings, and wanted to keep the \ntricky parts separate to ease cognition.\n\nOn a separte note, this patch was a development from the problem noticed \nby Sérgio of the different actions of 'git commit .'and 'git commit -a' \nwhen --assume-unchanged was used. Did you have any thoughts regarding \nDuy's patch (05 December 2014 10:56) to his code given in $gmane/260865.\n\nI wasn't sure if it had just been missed, or if there was some other \nissue?\n\n\n>> ++\n>> +When files are modified outside of Git, the user will need to stage\n>> +the modified files explicitly (e.g. see 'Examples' section in\n>> +linkgit:git-update-index[1]).\n>> +Git will not normally detect changes to those files.\n>> ++\n>> +This is useful on systems where lstat() calls are very slow, such as\n>> +CIFS/Microsoft Windows.\n>> +False by default.\n>\n> I think you are trying to make the result more readable by using\n> separate paragraphs for separate conceptual points, but then isn't\n> it wrong to have \"False by default\" as part of stating which\n> platforms are intended targets? I wonder if we want to have that\n> last line as its own paragraph instead.\n\nI was happy with it being a simple separate sentence.\n\n>\n> Thanks.\n>\n>>\n>> core.preferSymlinkRefs::\n>> Instead of the default \"symref\" format for HEAD\n> --\nPhilip \n"},{"id":"254510","messageId":"xmqqbnm7da76.fsf@gitster.dls.corp.google.com","threadId":"38287","inReplyTo":"1ED88DC1DCC2463782B4F4167DC85D5D@PhilipOakley","subject":"Re: [PATCH v3] doc: core.ignoreStat update, and clarify the --assume-unchanged effect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-09T19:29:33Z","receivedAt":"2015-01-09T19:29:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n>>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>>> index 52eeadd..fe179d0 100644\n>>> --- a/Documentation/config.txt\n>>> +++ b/Documentation/config.txt\n>>> @@ -375,15 +375,18 @@ This is useful for excluding servers inside a\n>>> firewall from\n>>> proxy use, while defaulting to a common proxy for external domains.\n>>>\n>>> core.ignoreStat::\n>>> + If true, Git will avoid using lstat() calls to detect if files have\n>>> + changed. Git will set the \"assume-unchanged\" bit for those\n>>> tracked files\n>>> + which it has updated identically in both the index and working\n>>> tree.\n>>\n>> I wonder if this is better stated in two seemingly independent\n>> sentences (like your version), or \"... if files have changed by\n>> setting the assume-unchanged bit ....\" to clarify where the setting\n>> of the bits to these files come into the big picture, but it is\n>> minor. Either way, I think it is a lot easier to understand than\n>> what we have in 'master'.\n>\n> I had considered a number of different wordings, and wanted to keep\n> the tricky parts separate to ease cognition.\n\nHmph, but wouldn't the result get more confusing, by stating two\n\"tricky\" things in separate sentences without giving any clue to\nguess how these two trickies are related? That is why I suggested\nto say the same two things in a way that clarifies that \"avoid using\nlstat(2)\" is the effect and \"setting the assume-unchanged bit\" is\nthe underlying implementation detail to cause that effect, i.e.\n\n\tIf true, Git will avoid using lstat(2) calls to detect if\n\tfiles have changed by setting \"assume-unchanged\" bit for\n\tthese tracked paths that Git has updated identically in both\n\tthe index and in the working tree.\n\n> On a separte note, this patch was a development from the problem\n> noticed by Sérgio of the different actions of 'git commit .'and 'git\n> commit -a' when --assume-unchanged was used. Did you have any thoughts\n> regarding Duy's patch (05 December 2014 10:56) to his code given in\n> $gmane/260865.\n>\n> I wasn't sure if it had just been missed, or if there was some other\n> issue?\n\nI thought the reason why we are discussing this documentation\nclean-up (specifically, clarifying that assume-unchanged is a\npromise the user makes not to modify the paths marked as such and is\nnot about telling Git to ignore changes to tracked paths), was\nbecause we agreed that such a change is a wrong thing to do.\n\nIt is wrong for at least two reasons.\n\n - The \"I promise not to modify them, so please omit lstat(2)\n assuming that I keep that promise\" is a performance thing---we\n shouldn't add more code to cater to people who do not keep that\n promise.\n\n - Adding one more case of \"Git will hide changes to tracked paths\n that you promised not to change\" gives more chance to confuse\n users into an incorrect understanding of what assume-unchanged\n bit is about. By not applying $gmane/260865, we keep one more\n way for the users to notice that the bit is *not* a mechanism\n to hide changes to tracked paths.\n\n\n>>> +When files are modified outside of Git, the user will need to stage\n>>> +the modified files explicitly (e.g. see 'Examples' section in\n>>> +linkgit:git-update-index[1]).\n>>> +Git will not normally detect changes to those files.\n>>> ++\n>>> +This is useful on systems where lstat() calls are very slow, such as\n>>> +CIFS/Microsoft Windows.\n>>> +False by default.\n>>\n>> I think you are trying to make the result more readable by using\n>> separate paragraphs for separate conceptual points, but then isn't\n>> it wrong to have \"False by default\" as part of stating which\n>> platforms are intended targets? I wonder if we want to have that\n>> last line as its own paragraph instead.\n>\n> I was happy with it being a simple separate sentence.\n\nI am also _for_ a separate sentence. But when a set of three\nparagraphs, i.e.\n\n\tA, something about A, and things about A.\n\n B, something about B, and things about B.\n\n C, something about C, and things about C.\n\nand you want to say something X that is not specific to A or B or C,\nwould you add that X at the end of C's paragraph, resulting in:\n\n\tA, something about A, and things about A.\n\n B, something about B, and things about B.\n\n C, something about C, and things about C. X that applies to\n all of A, B and C.\n\nor would it be more clear to see:\n\n\tA, something about A, and things about A.\n\n B, something about B, and things about B.\n\n C, something about C, and things about C.\n\n X that applies to all of A, B and C.\n\nwas the question. I think a simple separate sentence should not be\npart of the same \"In what situations this is useful\" paragraph.\n"},{"id":"254539","messageId":"5861D798BD564D6EBE0D55C529C8ECF8@PhilipOakley","threadId":"38287","inReplyTo":"xmqqbnm7da76.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] doc: core.ignoreStat update, and clarify the --assume-unchanged effect","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-01-10T18:54:54Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n> \"Philip Oakley\" writes:\n>\n>>>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>>>> index 52eeadd..fe179d0 100644\n>>>> --- a/Documentation/config.txt\n>>>> +++ b/Documentation/config.txt\n>>>> @@ -375,15 +375,18 @@ This is useful for excluding servers inside a\n>>>> firewall from\n>>>> proxy use, while defaulting to a common proxy for external\n>>>> domains.\n>>>>\n>>>> core.ignoreStat::\n>>>> + If true, Git will avoid using lstat() calls to detect if files\n>>>> have\n>>>> + changed. Git will set the \"assume-unchanged\" bit for those\n>>>> tracked files\n>>>> + which it has updated identically in both the index and working\n>>>> tree.\n>>>\n>>> I wonder if this is better stated in two seemingly independent\n>>> sentences (like your version), or \"... if files have changed by\n>>> setting the assume-unchanged bit ....\" to clarify where the setting\n>>> of the bits to these files come into the big picture, but it is\n>>> minor. Either way, I think it is a lot easier to understand than\n>>> what we have in 'master'.\n>>\n>> I had considered a number of different wordings, and wanted to keep\n>> the tricky parts separate to ease cognition.\n>\n> Hmph, but wouldn't the result get more confusing, by stating two\n> \"tricky\" things in separate sentences without giving any clue to\n> guess how these two trickies are related? That is why I suggested\n> to say the same two things in a way that clarifies that \"avoid using\n> lstat(2)\" is the effect and \"setting the assume-unchanged bit\" is\n> the underlying implementation detail to cause that effect, i.e.\n>\n> If true, Git will avoid using lstat(2) calls to detect if\n> files have changed by setting \"assume-unchanged\" bit for\n> these tracked paths that Git has updated identically in both\n> the index and in the working tree.\n\nI actually found that awkward to understand that because of both its\nlength (as a single sentence), and who does what when being mixed\ntogether. Surely it's a 'separation of concerns' process. The splitting\nthe sentence halves the readability score (among other things).\n\n>\n>> On a separte note, this patch was a development from the problem\n>> noticed by Sérgio of the different actions of 'git commit .'and 'git\n>> commit -a' when --assume-unchanged was used. Did you have any\n>> thoughts\n>> regarding Duy's patch (05 December 2014 10:56) to his code given in\n>> $gmane/260865.\n>>\n>> I wasn't sure if it had just been missed, or if there was some other\n>> issue?\n>\n> I thought the reason why we are discussing this documentation\n> clean-up (specifically, clarifying that assume-unchanged is a\n> promise the user makes not to modify the paths marked as such and is\n> not about telling Git to ignore changes to tracked paths), was\n> because we agreed that such a change is a wrong thing to do.\n\nWhile that is true (the documentation gave false suggestions), the\ncode should be trying to match our hoped for optimization so that the\nwhole tree doesn't need to be lstat'd\n\n>\n> It is wrong for at least two reasons.\n>\n> - The \"I promise not to modify them, so please omit lstat(2)\n> assuming that I keep that promise\" is a performance thing---we\n> shouldn't add more code to cater to people who do not keep that\n> promise.\n\nWe should add code that keeps our \"promise\" that [where reasonable] we\nwill optimise out lstats when there are large numbers of files to be\nchecked; Surely?\n\n>\n> - Adding one more case of \"Git will hide changes to tracked paths\n> that you promised not to change\" gives more chance to confuse\n> users into an incorrect understanding of what assume-unchanged\n> bit is about. By not applying $gmane/260865, we keep one more\n> way for the users to notice that the bit is *not* a mechanism\n> to hide changes to tracked paths.\n\nSurely, correcting the documentation is that route.\nI don't think we should be retaining the spikes on the steering wheel as\na bad driver reminder. There will be other corner cases for bad drivers\nto spin off at.\n\nUltimately the reason for inclusion would be that on large repos both\ncommit -a and commit . should be equally fast, which would need to be\nsaid in the commit message.\n\nAs a side point, a possible follow on tidy-up patch would be to have a\nmacro to cover the common (ce->ce_flags & (CE_VALID | CE_SKIP_WORKTREE)\nidiom (many places, with different constructs) in the same vein as\nce_skip_worktree()\n\n>\n>>>> +When files are modified outside of Git, the user will need to\n>>>> stage\n>>>> +the modified files explicitly (e.g. see 'Examples' section in\n>>>> +linkgit:git-update-index[1]).\n>>>> +Git will not normally detect changes to those files.\n>>>> ++\n>>>> +This is useful on systems where lstat() calls are very slow, such\n>>>> as\n>>>> +CIFS/Microsoft Windows.\n>>>> +False by default.\n>>>\n>>> I think you are trying to make the result more readable by using\n>>> separate paragraphs for separate conceptual points, but then isn't\n>>> it wrong to have \"False by default\" as part of stating which\n>>> platforms are intended targets? I wonder if we want to have that\n>>> last line as its own paragraph instead.\n>>\n>> I was happy with it being a simple separate sentence.\n>\n> I am also _for_ a separate sentence. But when a set of three\n> paragraphs, i.e.\n>\n> A, something about A, and things about A.\n>\n> B, something about B, and things about B.\n>\n> C, something about C, and things about C.\n>\n> and you want to say something X that is not specific to A or B or C,\n> would you add that X at the end of C's paragraph, resulting in:\n>\n> A, something about A, and things about A.\n>\n> B, something about B, and things about B.\n>\n> C, something about C, and things about C. X that applies to\n> all of A, B and C.\n>\n> or would it be more clear to see:\n>\n> A, something about A, and things about A.\n>\n> B, something about B, and things about B.\n>\n> C, something about C, and things about C.\n>\n> X that applies to all of A, B and C.\n>\n> was the question. I think a simple separate sentence should not be\n> part of the same \"In what situations this is useful\" paragraph.\n\nIn this case my split was about making sure that B was a distinct item\nwithin the flow, rather than being a single ABC paragraph, where the\nmiddle item gets lost in the noise [1]. People do remember the first and\nlast item (sentence), so the default is easy to see.\n\n--\nPhilip\n\n[1] Losing the middle item(s) in a list is a common human error, and is\ncommonly cited in human factors studies e.g. \"Human Error\", Reason.J,\n0521314194, cites Three Mile Island maintenance errors from poor\ndocumentation among others.\n"}]}