{"thread":{"id":"41663","subject":"[PATCH] pull: drop confusing prefix parameter of die_on_unclean_work_tree()","startedAt":"2016-03-10T13:24:26Z","lastAt":"2016-03-14T17:11:13Z","messageCount":6,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280556","messageId":"974d0bfed38e8aa410e97e05022bc5dbbd78d915.1457615785.git.johannes.schindelin@gmx.de","threadId":"41663","inReplyTo":null,"subject":"[PATCH] pull: drop confusing prefix parameter of die_on_unclean_work_tree()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-03-10T13:24:26Z","receivedAt":"2016-03-10T13:24:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In cmd_pull(), when verifying that there are no changes preventing a\nrebasing pull, we diligently pass the prefix parameter to the\ndie_on_unclean_work_tree() function which in turn diligently passes it\nto the has_unstaged_changes() and has_uncommitted_changes() functions.\n\nThe casual reader might now be curious (as this developer was) whether\nthat means that calling `git pull --rebase` in a subdirectory will\nignore unstaged changes in other parts of the working directory. And be\npuzzled that `git pull --rebase` (correctly) complains about those\nchanges outside of the current directory.\n\nThe puzzle is easily resolved: while we take pains to pass around the\nprefix and even pass it to init_revisions(), the fact that no paths are\npassed to init_revisions() ensures that the prefix is simply ignored.\n\nThat, combined with the fact that we will *always* want a *full* working\ndirectory check before running a rebasing pull, is reason enough to\nsimply do away with the actual prefix parameter and to pass NULL\ninstead, as if we were running this from the top-level working directory\nanyway.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/pull.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..4ed46b3 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -308,12 +308,12 @@ static enum rebase_type config_get_rebase(void)\n /**\n * Returns 1 if there are unstaged changes, 0 otherwise.\n */\n-static int has_unstaged_changes(const char *prefix)\n+static int has_unstaged_changes()\n {\n \tstruct rev_info rev_info;\n \tint result;\n \n-\tinit_revisions(&rev_info, prefix);\n+\tinit_revisions(&rev_info, NULL);\n \tDIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n \tDIFF_OPT_SET(&rev_info.diffopt, QUICK);\n \tdiff_setup_done(&rev_info.diffopt);\n@@ -324,7 +324,7 @@ static int has_unstaged_changes(const char *prefix)\n /**\n * Returns 1 if there are uncommitted changes, 0 otherwise.\n */\n-static int has_uncommitted_changes(const char *prefix)\n+static int has_uncommitted_changes()\n {\n \tstruct rev_info rev_info;\n \tint result;\n@@ -332,7 +332,7 @@ static int has_uncommitted_changes(const char *prefix)\n \tif (is_cache_unborn())\n \t\treturn 0;\n \n-\tinit_revisions(&rev_info, prefix);\n+\tinit_revisions(&rev_info, NULL);\n \tDIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n \tDIFF_OPT_SET(&rev_info.diffopt, QUICK);\n \tadd_head_to_pending(&rev_info);\n@@ -345,7 +345,7 @@ static int has_uncommitted_changes(const char *prefix)\n * If the work tree has unstaged or uncommitted changes, dies with the\n * appropriate message.\n */\n-static void die_on_unclean_work_tree(const char *prefix)\n+static void die_on_unclean_work_tree()\n {\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));\n \tint do_die = 0;\n@@ -355,12 +355,12 @@ static void die_on_unclean_work_tree(const char *prefix)\n \tupdate_index_if_able(&the_index, lock_file);\n \trollback_lock_file(lock_file);\n \n-\tif (has_unstaged_changes(prefix)) {\n+\tif (has_unstaged_changes()) {\n \t\terror(_(\"Cannot pull with rebase: You have unstaged changes.\"));\n \t\tdo_die = 1;\n \t}\n \n-\tif (has_uncommitted_changes(prefix)) {\n+\tif (has_uncommitted_changes()) {\n \t\tif (do_die)\n \t\t\terror(_(\"Additionally, your index contains uncommitted changes.\"));\n \t\telse\n@@ -842,7 +842,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n \t\tif (!autostash)\n-\t\t\tdie_on_unclean_work_tree(prefix);\n+\t\t\tdie_on_unclean_work_tree();\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n \t\t\thashclr(rebase_fork_point);\n-- \n2.7.2.windows.1.8.g47d64e6.dirty\n"},{"id":"280562","messageId":"xmqqr3fidxs3.fsf@gitster.mtv.corp.google.com","threadId":"41663","inReplyTo":"974d0bfed38e8aa410e97e05022bc5dbbd78d915.1457615785.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] pull: drop confusing prefix parameter of die_on_unclean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-10T17:54:04Z","receivedAt":"2016-03-10T17:54:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> In cmd_pull(), when verifying that there are no changes preventing a\n> rebasing pull, we diligently pass the prefix parameter to the\n> die_on_unclean_work_tree() function which in turn diligently passes it\n> to the has_unstaged_changes() and has_uncommitted_changes() functions.\n>\n> The casual reader might now be curious (as this developer was) whether\n> that means that calling `git pull --rebase` in a subdirectory will\n> ignore unstaged changes in other parts of the working directory. And be\n> puzzled that `git pull --rebase` (correctly) complains about those\n> changes outside of the current directory.\n\nI think this is quite subjective, as I tend to take the presence of\n\"prefix\" to mean \"the callee assumes that the caller has gone up to\nthe root level already\", and take the absense of use of \"prefix\" in\nthe callee to mean \"the callee is working on the whole tree\", and\ndiscarding the parameter is robbing that clue from that point of\nview.\n\nSo I am mildly opposed to most parts of this change, including not\nspelling out (void) as the list of parameters for a function that\ndoes not take any.\n\nI do think not passing \"prefix\" to init_revisions() would be the\nright thing. In fact, that prefix is copied to rev, but the current\nend result is correct _only_ because the pathspec limit given by\nthat \"prefix\" parameter to init_revisions() is not automatically\ncopied to rev_info.diffopt, and the code is very misleading.\n"},{"id":"280563","messageId":"xmqqk2ladx36.fsf@gitster.mtv.corp.google.com","threadId":"41663","inReplyTo":"xmqqr3fidxs3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pull: drop confusing prefix parameter of die_on_unclean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-10T18:09:01Z","receivedAt":"2016-03-10T18:09:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think this is quite subjective, as I tend to take the presence of\n> \"prefix\" to mean \"the callee assumes that the caller has gone up to\n> the root level already\", and take the absense of use of \"prefix\" in\n> the callee to mean \"the callee is working on the whole tree\", and\n> discarding the parameter is robbing that clue from that point of\n> view.\n>\n> So I am mildly opposed to most parts of this change, including not\n> spelling out (void) as the list of parameters for a function that\n> does not take any.\n>\n> I do think not passing \"prefix\" to init_revisions() would be the\n> right thing. In fact, that prefix is copied to rev, but the current\n> end result is correct _only_ because the pathspec limit given by\n> that \"prefix\" parameter to init_revisions() is not automatically\n> copied to rev_info.diffopt, and the code is very misleading.\n\nAnother reason why it is more sensible to keep the prefix available,\nbut not use it to limit the extent of diff, to has_*_changes()\nfunctions is that it would be easier for us to change our mind later\nto allow the users to ask for more detailed output. Instead of\n\"Cannot pull with rebase: You have unstaged changes, period.\", you\nmay be asked to list which paths are dirty in such a case, and in\norder to present the list relative to the directory where the user\nstarted the command, you would need \"prefix\" available to the code\nthat calls into diff machinery somehow.\n"},{"id":"280707","messageId":"alpine.DEB.2.20.1603141410590.4690@virtualbox","threadId":"41663","inReplyTo":"xmqqk2ladx36.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pull: drop confusing prefix parameter of die_on_unclean_work_tree()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-03-14T13:42:32Z","receivedAt":"2016-03-14T13:42:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 10 Mar 2016, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > I think this is quite subjective, as I tend to take the presence of\n> > \"prefix\" to mean \"the callee assumes that the caller has gone up to\n> > the root level already\", and take the absense of use of \"prefix\" in\n> > the callee to mean \"the callee is working on the whole tree\", and\n> > discarding the parameter is robbing that clue from that point of\n> > view.\n> >\n> > So I am mildly opposed to most parts of this change, including not\n> > spelling out (void) as the list of parameters for a function that\n> > does not take any.\n> >\n> > I do think not passing \"prefix\" to init_revisions() would be the\n> > right thing. In fact, that prefix is copied to rev, but the current\n> > end result is correct _only_ because the pathspec limit given by\n> > that \"prefix\" parameter to init_revisions() is not automatically\n> > copied to rev_info.diffopt, and the code is very misleading.\n> \n> Another reason why it is more sensible to keep the prefix available,\n> but not use it to limit the extent of diff, to has_*_changes()\n> functions is that it would be easier for us to change our mind later\n> to allow the users to ask for more detailed output. Instead of\n> \"Cannot pull with rebase: You have unstaged changes, period.\", you\n> may be asked to list which paths are dirty in such a case, and in\n> order to present the list relative to the directory where the user\n> started the command, you would need \"prefix\" available to the code\n> that calls into diff machinery somehow.\n\nLet me summarize.\n\nFirst, you argue that the prefix is a documented way to say: \"This\nfunction needs to be called from the top-level directory\".\n\nNevermind that the parameter reads \"prefix\" instead of\n\"this_parameter_means_you_have_to_call_this_from_the_top_level_directory\".\n\nAnd then you say that it is not a bug to pretend to use said \"prefix\"\nvalue by passing it to init_revisions() only to ignore it presently due to\nsome chain of side effects.\n\nIn short: there is no bug, even if the code is really confusing, so much\nso that this developer got highly confused.\n\nAnd after that, you continue by stating that we need to keep the \"prefix\"\nparameter because we *might*, *eventually* fix the bug (that is not a\nbug at all?!?!?!)?\n\nNow I am even more confused than before.\n\nCiao,\nDscho\n\nP.S.: The idea that a rebasing pull might, at some stage in the future,\nwant to require only part of the working directory to be clean, this idea\nalso makes little sense to me.\n"},{"id":"280718","messageId":"xmqqfuvtav5k.fsf@gitster.mtv.corp.google.com","threadId":"41663","inReplyTo":"alpine.DEB.2.20.1603141410590.4690@virtualbox","subject":"Re: [PATCH] pull: drop confusing prefix parameter of die_on_unclean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-14T16:20:23Z","receivedAt":"2016-03-14T16:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Another reason why it is more sensible to keep the prefix available,\n>> but not use it to limit the extent of diff, to has_*_changes()\n>> functions is that it would be easier for us to change our mind later\n>> to allow the users to ask for more detailed output. Instead of\n>> \"Cannot pull with rebase: You have unstaged changes, period.\", you\n>> may be asked to list which paths are dirty in such a case, and in\n>> order to present the list relative to the directory where the user\n>> started the command, you would need \"prefix\" available to the code\n>> that calls into diff machinery somehow.\n>\n> Let me summarize.\n>\n> First, you argue that the prefix is a documented way to say: \"This\n> function needs to be called from the top-level directory\".\n\nIt tells the reader of this function that the caller has already\nmoved to the top-level and as a633fca0 (Call setup_git_directory()\nmuch earlier, 2006-07-28) says, gives you the \"prefix\" (i.e.\npathname of $PWD relative to the project root level). When you read\nthe function, hence, you know \"prefix\" can be used to tell where\noriginally the command was run from, if you want to learn it.\n\nIn other words, I didn't mean to argue anything like what you are\nsaying. Either I phrased poorly, or you misread it.\n\nFor the purpose of letting \"git pull --rebase\" make sure that there\nis no difference between the index and the working tree, we'd want a\ntree-wide diff, so we do not want to limit the diff with pathspec.\nNot passing \"prefix\" to init_revisions() _is_ the right thing.\nAgain, I think I said that in the message you are responding to, and\neither I phrased poorly, or you misread it.\n\nIn any case, that call to init_revisions() should have imitated what\nindex_differs_from() and do_diff_cache() in diff-lib.c do, i.e.\npass NULL as the prefix to say \"I do not want any path/subdirectory\nlimitation\".\n\n> P.S.: The idea that a rebasing pull might, at some stage in the future,\n> want to require only part of the working directory to be clean, this idea\n> also makes little sense to me.\n\nAnd there is no such idea. Again, perhaps I phrased the whole thing\npoorly or you misread it under the assumption that whenever I disagree\nwith what you say, it is more likely what I say is incorrect.\n\nI was talking about this sequence:\n\n $ edit rerere.c\n ... time passes ...\n $ cd Documentation/\n $ git pull --rebase\n error: Cannot pull with rebase: You have unstaged changes.\n ... oh, where do I have changes?\n $ git status -suno\n M ../rerere.c\n\nwhich you may want to optimize the human-end-user experience by\nallowing 'pull --rebase' to say \"You have unstaged changes in\n../rerere.c\" or something along that line [*1*] so that the user\ndoes not have to give a separate \"git status\" there.\n\nAnd for that, the code that gives the error message needs to be able\nto access the \"prefix\" (i.e. pathname of $PWD relative to the\nproject root level) to make that path relative in the output.\n\n\n[Footnote]\n\n*1* By \"something along that line\" I mean that you'd need to work\nout the details of what to do when there are too many of them, for\nexample you wouldn't list everything but indicate that the list is\ntruncated in some way.\n"},{"id":"280719","messageId":"xmqqbn6hassu.fsf@gitster.mtv.corp.google.com","threadId":"41663","inReplyTo":"xmqqfuvtav5k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pull: drop confusing prefix parameter of die_on_unclean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-14T17:11:13Z","receivedAt":"2016-03-14T17:11:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> For the purpose of letting \"git pull --rebase\" make sure that there\n> is no difference between the index and the working tree, we'd want a\n> tree-wide diff, so we do not want to limit the diff with pathspec.\n> Not passing \"prefix\" to init_revisions() _is_ the right thing.\n\nOh, sorry, I think it is me who uses confusing phrasing, not your\nreading skills. What I meant by the above was:\n\n - We do want tree-wide diff.\n\n - We do not want to limit to a subtree.\n\n - Passing prefix to init_revisions() mean \"if we have pathspec,\n they are relative to this prefix\".\n\n - That may hint to the reader that the diff we are going to run may\n be limited to some subset to the tree, which is not what we are\n doing.\n\n - Hence, we should pass NULL to init_revisions() to make it\n explicit that we are not limiting.\n\n - And the part of your patch to pass NULL to init_revisions() is\n correct.\n\nBut the function itself should take prefix, even if it is not going\nto use it to call init_revisions(), because it makes it possible to\ndo a better reporting later.\n"}]}