{"thread":{"id":"50731","subject":"[PATCH 1/3] sequencer: break some long lines","startedAt":"2019-03-13T18:26:37Z","lastAt":"2019-03-21T16:43:52Z","messageCount":10,"participants":["Phillip Wood","Johannes Schindelin","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"371381","messageId":"20190313182615.7351-1-phillip.wood123@gmail.com","threadId":"50731","inReplyTo":null,"subject":"[PATCH 1/3] sequencer: break some long lines","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-13T18:26:13Z","receivedAt":"2019-03-13T18:26:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nreformat save_opts() to remove excessively long lines.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 35 ++++++++++++++++++++++-------------\n 1 file changed, 22 insertions(+), 13 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0db410d590..5e19b22f8f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2700,36 +2700,45 @@ static int save_opts(struct replay_opts *opts)\n \tint res = 0;\n \n \tif (opts->no_commit)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.no-commit\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.no-commit\", \"true\");\n \tif (opts->edit)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.edit\", \"true\");\n \tif (opts->signoff)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.signoff\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.signoff\", \"true\");\n \tif (opts->record_origin)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.record-origin\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.record-origin\", \"true\");\n \tif (opts->allow_ff)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.allow-ff\", \"true\");\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.allow-ff\", \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.mainline\", buf.buf);\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.mainline\", buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\n \tif (opts->strategy)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.strategy\", opts->strategy);\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.strategy\", opts->strategy);\n \tif (opts->gpg_sign)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.gpg-sign\", opts->gpg_sign);\n \tif (opts->xopts) {\n \t\tint i;\n \t\tfor (i = 0; i < opts->xopts_nr; i++)\n \t\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n-\t\t\t\t\t\t\t\"options.strategy-option\",\n-\t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n+\t\t\t\t\t\"options.strategy-option\",\n+\t\t\t\t\topts->xopts[i], \"^$\", 0);\n \t}\n \tif (opts->allow_rerere_auto)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.allow-rerere-auto\",\n-\t\t\t\t\t\t     opts->allow_rerere_auto == RERERE_AUTOUPDATE ?\n-\t\t\t\t\t\t     \"true\" : \"false\");\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\"options.allow-rerere-auto\",\n+\t\t\t\topts->allow_rerere_auto == RERERE_AUTOUPDATE ?\n+\t\t\t\t\"true\" : \"false\");\n \treturn res;\n }\n \n-- \n2.21.0\n\n"},{"id":"371382","messageId":"20190313182615.7351-2-phillip.wood123@gmail.com","threadId":"50731","inReplyTo":"20190313182615.7351-1-phillip.wood123@gmail.com","subject":"[PATCH 2/3] cherry-pick: demonstrate option amnesia","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-13T18:26:14Z","receivedAt":"2019-03-13T18:26:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen cherry-pick stops for a conflict resolution it forgets\n--allow-empty --allow-empty-message and --keep-redundant-commits.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t3507-cherry-pick-conflict.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 0db166152a..79e994cffa 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -25,6 +25,11 @@ test_expect_success setup '\n \ttest_commit base foo b &&\n \ttest_commit picked foo c &&\n \ttest_commit --signoff picked-signed foo d &&\n+\tgit checkout -b topic initial &&\n+\ttest_commit redundant-pick foo c redundant &&\n+\tgit commit --allow-empty --allow-empty-message &&\n+\tgit tag empty &&\n+\tgit checkout master &&\n \tgit config advice.detachedhead false\n \n '\n@@ -405,4 +410,23 @@ test_expect_success 'cherry-pick preserves sparse-checkout' '\n \ttest_i18ngrep ! \"Changes not staged for commit:\" actual\n '\n \n+test_expect_failure 'cherry-pick --continue remembers --keep-redundant-commits' '\n+\ttest_when_finished \"git cherry-pick --abort || :\" &&\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --keep-redundant-commits picked redundant &&\n+\techo c >foo &&\n+\tgit add foo &&\n+\tgit cherry-pick --continue\n+'\n+\n+test_expect_failure 'cherry-pick --continue remembers --allow-empty and --allow-empty-message' '\n+\ttest_when_finished \"git cherry-pick --abort || :\" &&\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --allow-empty --allow-empty-message \\\n+\t\t\t\t       picked empty &&\n+\techo c >foo &&\n+\tgit add foo &&\n+\tgit cherry-pick --continue\n+'\n+\n test_done\n-- \n2.21.0\n\n"},{"id":"371383","messageId":"20190313182615.7351-3-phillip.wood123@gmail.com","threadId":"50731","inReplyTo":"20190313182615.7351-1-phillip.wood123@gmail.com","subject":"[PATCH 3/3] cherry-pick --continue: remember options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-13T18:26:15Z","receivedAt":"2019-03-13T18:26:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nRemember --allow-empty, --allow-empty-message and\n--keep-redundant-commits when cherry-pick stops for a conflict\nresolution.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                     | 18 ++++++++++++++++++\n t/t3507-cherry-pick-conflict.sh |  4 ++--\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5e19b22f8f..1ad51aa498 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2307,6 +2307,15 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \t\topts->no_commit = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.edit\"))\n \t\topts->edit = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.allow-empty\"))\n+\t\topts->allow_empty =\n+\t\t\tgit_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.allow-empty-message\"))\n+\t\topts->allow_empty_message =\n+\t\t\tgit_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.keep-redundant-commits\"))\n+\t\topts->keep_redundant_commits =\n+\t\t\tgit_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.signoff\"))\n \t\topts->signoff = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.record-origin\"))\n@@ -2705,6 +2714,15 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->edit)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.edit\", \"true\");\n+\tif (opts->allow_empty)\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\t\"options.allow-empty\", \"true\");\n+\tif (opts->allow_empty_message)\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\"options.allow-empty-message\", \"true\");\n+\tif (opts->keep_redundant_commits)\n+\t\tres |= git_config_set_in_file_gently(opts_file,\n+\t\t\t\t\"options.keep-redundant-commits\", \"true\");\n \tif (opts->signoff)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.signoff\", \"true\");\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 79e994cffa..1ef8e9d534 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -410,7 +410,7 @@ test_expect_success 'cherry-pick preserves sparse-checkout' '\n \ttest_i18ngrep ! \"Changes not staged for commit:\" actual\n '\n \n-test_expect_failure 'cherry-pick --continue remembers --keep-redundant-commits' '\n+test_expect_success 'cherry-pick --continue remembers --keep-redundant-commits' '\n \ttest_when_finished \"git cherry-pick --abort || :\" &&\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick --keep-redundant-commits picked redundant &&\n@@ -419,7 +419,7 @@ test_expect_failure 'cherry-pick --continue remembers --keep-redundant-commits'\n \tgit cherry-pick --continue\n '\n \n-test_expect_failure 'cherry-pick --continue remembers --allow-empty and --allow-empty-message' '\n+test_expect_success 'cherry-pick --continue remembers --allow-empty and --allow-empty-message' '\n \ttest_when_finished \"git cherry-pick --abort || :\" &&\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick --allow-empty --allow-empty-message \\\n-- \n2.21.0\n\n"},{"id":"371412","messageId":"nycvar.QRO.7.76.6.1903132344350.41@tvgsbejvaqbjf.bet","threadId":"50731","inReplyTo":"20190313182615.7351-3-phillip.wood123@gmail.com","subject":"Re: [PATCH 3/3] cherry-pick --continue: remember options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-13T22:45:41Z","receivedAt":"2019-03-13T22:45:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 13 Mar 2019, Phillip Wood wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> Remember --allow-empty, --allow-empty-message and\n> --keep-redundant-commits when cherry-pick stops for a conflict\n> resolution.\n> \n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThis whole patch series makes sense to me.\n\nAnd it is especially nice that you make it easy to verify that there is a\nbug in the first place, by separating the concern of demonstrating it from\nthe concern to fix it.\n\nThanks,\nDscho\n\n> ---\n>  sequencer.c                     | 18 ++++++++++++++++++\n>  t/t3507-cherry-pick-conflict.sh |  4 ++--\n>  2 files changed, 20 insertions(+), 2 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 5e19b22f8f..1ad51aa498 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2307,6 +2307,15 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n>  \t\topts->no_commit = git_config_bool_or_int(key, value, &error_flag);\n>  \telse if (!strcmp(key, \"options.edit\"))\n>  \t\topts->edit = git_config_bool_or_int(key, value, &error_flag);\n> +\telse if (!strcmp(key, \"options.allow-empty\"))\n> +\t\topts->allow_empty =\n> +\t\t\tgit_config_bool_or_int(key, value, &error_flag);\n> +\telse if (!strcmp(key, \"options.allow-empty-message\"))\n> +\t\topts->allow_empty_message =\n> +\t\t\tgit_config_bool_or_int(key, value, &error_flag);\n> +\telse if (!strcmp(key, \"options.keep-redundant-commits\"))\n> +\t\topts->keep_redundant_commits =\n> +\t\t\tgit_config_bool_or_int(key, value, &error_flag);\n>  \telse if (!strcmp(key, \"options.signoff\"))\n>  \t\topts->signoff = git_config_bool_or_int(key, value, &error_flag);\n>  \telse if (!strcmp(key, \"options.record-origin\"))\n> @@ -2705,6 +2714,15 @@ static int save_opts(struct replay_opts *opts)\n>  \tif (opts->edit)\n>  \t\tres |= git_config_set_in_file_gently(opts_file,\n>  \t\t\t\t\t\"options.edit\", \"true\");\n> +\tif (opts->allow_empty)\n> +\t\tres |= git_config_set_in_file_gently(opts_file,\n> +\t\t\t\t\t\"options.allow-empty\", \"true\");\n> +\tif (opts->allow_empty_message)\n> +\t\tres |= git_config_set_in_file_gently(opts_file,\n> +\t\t\t\t\"options.allow-empty-message\", \"true\");\n> +\tif (opts->keep_redundant_commits)\n> +\t\tres |= git_config_set_in_file_gently(opts_file,\n> +\t\t\t\t\"options.keep-redundant-commits\", \"true\");\n>  \tif (opts->signoff)\n>  \t\tres |= git_config_set_in_file_gently(opts_file,\n>  \t\t\t\t\t\"options.signoff\", \"true\");\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 79e994cffa..1ef8e9d534 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -410,7 +410,7 @@ test_expect_success 'cherry-pick preserves sparse-checkout' '\n>  \ttest_i18ngrep ! \"Changes not staged for commit:\" actual\n>  '\n>  \n> -test_expect_failure 'cherry-pick --continue remembers --keep-redundant-commits' '\n> +test_expect_success 'cherry-pick --continue remembers --keep-redundant-commits' '\n>  \ttest_when_finished \"git cherry-pick --abort || :\" &&\n>  \tpristine_detach initial &&\n>  \ttest_must_fail git cherry-pick --keep-redundant-commits picked redundant &&\n> @@ -419,7 +419,7 @@ test_expect_failure 'cherry-pick --continue remembers --keep-redundant-commits'\n>  \tgit cherry-pick --continue\n>  '\n>  \n> -test_expect_failure 'cherry-pick --continue remembers --allow-empty and --allow-empty-message' '\n> +test_expect_success 'cherry-pick --continue remembers --allow-empty and --allow-empty-message' '\n>  \ttest_when_finished \"git cherry-pick --abort || :\" &&\n>  \tpristine_detach initial &&\n>  \ttest_must_fail git cherry-pick --allow-empty --allow-empty-message \\\n> -- \n> 2.21.0\n> \n> \n"},{"id":"371457","messageId":"xmqqimwm9hh5.fsf@gitster-ct.c.googlers.com","threadId":"50731","inReplyTo":"nycvar.QRO.7.76.6.1903132344350.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/3] cherry-pick --continue: remember options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T05:01:58Z","receivedAt":"2019-03-14T05:02:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> Remember --allow-empty, --allow-empty-message and\n>> --keep-redundant-commits when cherry-pick stops for a conflict\n>> resolution.\n>> \n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> This whole patch series makes sense to me.\n\nYes, the changes look sensible (provided if it is the sensible goal\nto make --continue use the same set of options, that is).\n\n> And it is especially nice that you make it easy to verify that there is a\n> bug in the first place, by separating the concern of demonstrating it from\n> the concern to fix it.\n\nFor a multi-patch series, breaking out the verification test out of\nthe fixing patch is OK.  It is a different story for a simple change\nthat could be made in a single step to artificially separate the\ntest into a separate step.  An early \"test_expect_failure\" would not\nstop the test when applied to a different codebase as a standalone\n\"does the breakage exist in this unrelated version?\" even if the\n\"test only\" patch might appear easier to apply (as opposed to\napplying only the t/ part of the patch that expects success and\nseeing it actually fail).\n\n"},{"id":"371499","messageId":"nycvar.QRO.7.76.6.1903141432000.41@tvgsbejvaqbjf.bet","threadId":"50731","inReplyTo":"xmqqimwm9hh5.fsf@gitster-ct.c.googlers.com","subject":"separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-14T14:09:23Z","receivedAt":"2019-03-14T14:09:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 14 Mar 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > And it is especially nice that you make it easy to verify that there\n> > is a bug in the first place, by separating the concern of\n> > demonstrating it from the concern to fix it.\n> \n> For a multi-patch series, breaking out the verification test out of the\n> fixing patch is OK.  It is a different story for a simple change that\n> could be made in a single step to artificially separate the test into a\n> separate step.  An early \"test_expect_failure\" would not stop the test\n> when applied to a different codebase as a standalone \"does the breakage\n> exist in this unrelated version?\" even if the \"test only\" patch might\n> appear easier to apply (as opposed to applying only the t/ part of the\n> patch that expects success and seeing it actually fail).\n\nThis keeps coming up, and I feel that in the past, I have only managed to\nupset you, not to convince you that my point of view has at least *some*\nmerit.\n\nSo please let me try again, this time with a cooler head.\n\nAs you know, I find it rather cumbersome to review code changes on the Git\nmailing list. Unlike others, I do not suffer from issues revolving around\nHTML parts and/or mailer issues, and I do try to just work with the\nlimitations of the medium. But sometimes the diff just does not give\nenough context. Or I really would need to look at it with `--color-moved`\nor `--color-words` or `--patience` or `-w`. Or yet other advanced diff\noptions.\n\nSo I would like to have the code changes locally, in a branch in my\nworktree.\n\nAnd at least for me, code review often does not end there. I need to dive\ninto the commit history to see e.g. why a line that a patch wants to\nremoved was added in the first place, i.e. read the commit message and see\nhow the surrounding code changed.\n\nSo I would like to have not only some re-applied changes locally, but\nreally a clone of the branch that the contributor themselves had locally.\n\nAnd as my understanding of code review is first and foremost to ensure the\ncorrectness of the code (which is why I want to leave the more tedious\nparts of code review that do not really require a human to review, or even\nfix, to automation), in some cases I really need to run the code (or the\nregression test) locally, e.g. to see whether it is a Windows-specific\nissue, or whether it affects this or that branch.\n\nThe necessity for this kind of thing was demonstrated rather well by\nHannes Sixt recently, where he ran a regression test that I added as part\nof https://github.com/gitgitgadget/git/pull/96 (mail thread here:\nhttps://public-inbox.org/git/pull.96.git.gitgitgadget@gmail.com/), and\nfigured out that this regression does not even exist in git.git's `master`\nbranch. Therefore it don't need no fixing there, either.\n\nSo now that we established how important it is to go back to the local Git\nworktree for many in-depth reviews, let's see how easy/convenient that is\nright now.\n\nIn our currently established process, you have to have mutt with your own\ncustom scripts. Kidding! But it is undeniable that without serious\nscripting (that nobody seems to be able to do in a way that can be shared\nbetween reviewers), it is super hard to do anything but a pure diff-based\nreview (which would not have caught the problem described above).\n\nSo how can we make this easier? How can we make it less painful to review\ncode changes beyond the bare minimum diff-based review which, let's face\nit, encourages \"white space review\", i.e. focusing on problems that can be\nseen from the diff, but that are really of little consequence when it\ncomes to the health of our code base?\n\nWhat it boils down to: it needs to be easier to pull down patches and to\nrecreate local branches for them.\n\nIt is in this context that I find it much better to separate patches that\nadd regression test cases from patches that fix the regressions. How often\ndid I encounter problems to apply the diff from public-inbox? I cannot\ncount the number that happened/happens to me. And usually the diff does\nnot apply, many more times in the part that touches libgit.a code than in\nthe part that touches the test suite.\n\nAnd how many times did `git am -3` not help me because I lacked the\nprerequisite objects? Again, I cannot count that number, it\nhappend/happens *so* often.\n\nBut the regression test patches usually apply cleanly, or if they don't,\nchances are that I have the prerequisite objects, because we rarely change\nregression tests locally, we only add to them. And even if I do not have\nthose prerequisite objects, those patches usually *add* code, so it is\neasier to resolve the problems.\n\nNote: I sometimes want the regression test locally not so much to verify\nthat it demonstrates a current bug, but sometimes to look at the code\nflow, to see which parts of the code are executed. That is why I like to\nhave that part locally much more often than I need the actual fix locally.\n\nSo yes, the easier I can make that process, the better.\n\nSide note: for some time, our team tried to track all patches (whether\nthey made their way into a branch at https://github.com/gitster/git or\nnot) in the form of branches in an internal fork. Stolee drove that\nproject forward, and it worked for the most part. This might be something\nwe would actually want in a public fork, maybe even with a simple web app\nto find the branch corresponding to a given Message-ID, or some such. But\nI digress.\n\nIn any case, before we get better tooling to work around these issues, I\nstill think it makes a ton of sense to encourage proper separation of\nconcerns: to keep patches that introduce regression tests demonstrating a\nbreakage separate from patches that fix the breakage. It would certainly\nhelp me (e.g. when staring at a range diff).\n\nIt certainly would make it easier for me to justify the time I spend on\nreviews, because I could review more patch series in the same amount of\ntime.\n\nSo hopefully we can find some middle ground between your endeavor to keep\nthe contributions compact, and my desire to separate concerns better (even\nat the expense of number of patches; I think commits are cheap, cheaper\nthan reviewers' time at least, for sure).\n\nCiao,\nDscho\n"},{"id":"371501","messageId":"CACsJy8DCZLNNa9zFj04kPx=f1S_5VJqqP_qTK6QYJ0fFmusymg@mail.gmail.com","threadId":"50731","inReplyTo":"nycvar.QRO.7.76.6.1903141432000.41@tvgsbejvaqbjf.bet","subject":"Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-14T14:30:53Z","receivedAt":"2019-03-14T14:31:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 14, 2019 at 9:10 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> In any case, before we get better tooling to work around these issues, I\n> still think it makes a ton of sense to encourage proper separation of\n> concerns: to keep patches that introduce regression tests demonstrating a\n> breakage separate from patches that fix the breakage. It would certainly\n> help me (e.g. when staring at a range diff).\n\nThen perhaps improve the tools now because these separate patches\nenter 'master' and stay in the history forever. One of the problems I\nhave with separating tests from the actual code change is the\ndescription of the problem often stays on the test commit, which I\ncan't see in git-log if I'm searching for the code change. And no\nsometimes I can't just look at the parent commit if I filter code by\npath (and with --full-diff)\n-- \nDuy\n"},{"id":"371755","messageId":"xmqqbm2980wk.fsf@gitster-ct.c.googlers.com","threadId":"50731","inReplyTo":"CACsJy8DCZLNNa9zFj04kPx=f1S_5VJqqP_qTK6QYJ0fFmusymg@mail.gmail.com","subject":"Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T00:46:35Z","receivedAt":"2019-03-18T00:46:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Thu, Mar 14, 2019 at 9:10 PM Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n>> In any case, before we get better tooling to work around these issues, I\n>> still think it makes a ton of sense to encourage proper separation of\n>> concerns: to keep patches that introduce regression tests demonstrating a\n>> breakage separate from patches that fix the breakage. It would certainly\n>> help me (e.g. when staring at a range diff).\n>\n> Then perhaps improve the tools now because these separate patches\n> enter 'master' and stay in the history forever. One of the problems I\n> have with separating tests from the actual code change is the\n> description of the problem often stays on the test commit, which I\n> can't see in git-log if I'm searching for the code change. And no\n> sometimes I can't just look at the parent commit if I filter code by\n> path (and with --full-diff)\n\nI do not think the phrase \"separation of concerns\" is applied\ncorrectly here.\n\nThe concern of a simple single-patch is to fix a bug---the test that\nshows what bug was fixed and protects the code by ensuring that a\nreintroduction of the bug gets noticed is an integral part of it.\nIt does not make much sense to artificially split it into two.\n\nIt's like arguing for adding declarations for new functions and\nglobal variables in *.h files in a step before a separate step adds\ntheir implementations in *.c files, claiming that the first step\ndesigns the interface (which is sufficient for writing the client\ncode) and the second one gives an implementation of the interface\n(which can be replaced later), and they address two separate\nconcerns.\n\nAnd then you would find that there are some compilers that warn\nagainst unimplemented functions and undefined variables.  The\n\"solution\" would be to enclose the whole thing that was added in the\nfirst \"*.h only patch\" inside \"#if 0/#endif\" to hide it from the\ncompiler ;-) That in fact looks quite similar to how \"test only\npatch\" marks the new tests as expecting failure in the beginning.\nNeither is truly useful when applied to a context different from\nwhere it is originally developed for.\n"},{"id":"371793","messageId":"xmqqtvg0zjva.fsf@gitster-ct.c.googlers.com","threadId":"50731","inReplyTo":"CACsJy8DCZLNNa9zFj04kPx=f1S_5VJqqP_qTK6QYJ0fFmusymg@mail.gmail.com","subject":"Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T08:07:05Z","receivedAt":"2019-03-18T08:07:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> .... One of the problems I\n> have with separating tests from the actual code change is the\n> description of the problem often stays on the test commit, which I\n> can't see in git-log if I'm searching for the code change.\n\nIn the message you are reponding to, Dscho made it sound as if I am\nreviewing only from my MUA, but most of my reviews are done after\nthe patches are tentatively applied (it is a separate issue if the\nresult is found worth keeping and merged to 'pu'), so our workflows\nare not so different.  It is not like \"must have them separate\" is\nthe need shared among those who prefer to review in-tree.  I do not\nwant a logically single patch split into two.\n\nAnd I find your \"find the other half\" of a pair of patch that is\nartificially split into two a real problem for me, too.  If you\nsplit a single patch into two, depending on which half you find\nfirst, finding the other half is either trivial (as you can just\nfollow the parent pointer to $THAT_THING^) or hard (you'd need to\nhave something that binds everything together, like 'pu', and grep\nfor \"git log ..pu\" trying to find what its most relevant child is).\n"},{"id":"372143","messageId":"nycvar.QRO.7.76.6.1903211548200.41@tvgsbejvaqbjf.bet","threadId":"50731","inReplyTo":"xmqqbm2980wk.fsf@gitster-ct.c.googlers.com","subject":"Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-21T16:43:25Z","receivedAt":"2019-03-21T16:43:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 18 Mar 2019, Junio C Hamano wrote:\n\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n> > On Thu, Mar 14, 2019 at 9:10 PM Johannes Schindelin\n> > <Johannes.Schindelin@gmx.de> wrote:\n> >> In any case, before we get better tooling to work around these issues, I\n> >> still think it makes a ton of sense to encourage proper separation of\n> >> concerns: to keep patches that introduce regression tests demonstrating a\n> >> breakage separate from patches that fix the breakage. It would certainly\n> >> help me (e.g. when staring at a range diff).\n> >\n> > Then perhaps improve the tools now because these separate patches\n> > enter 'master' and stay in the history forever. One of the problems I\n> > have with separating tests from the actual code change is the\n> > description of the problem often stays on the test commit, which I\n> > can't see in git-log if I'm searching for the code change. And no\n> > sometimes I can't just look at the parent commit if I filter code by\n> > path (and with --full-diff)\n>\n> I do not think the phrase \"separation of concerns\" is applied\n> correctly here.\n\nYou might currently still think so, so let me give you material to\nre-think that stance:\n\n- If you truly care about contributors of all stripes, you also care about\n  contributors who report bugs, contributors who turn bug reports into\n  reproducible regression test cases, contributors who verify regressions,\n  etc.\n\n  And if you really want to give your respect to, say, the contributors\n  who provide such regression test cases, you will accept them as full\n  commit authors.\n\n  And that fact alone makes it a separate concern, notwithstanding that in\n  my case, the contributor who wrote the regression test case and the\n  contributor who fixed the regression happened to be the same person: me.\n  That fact simply does not invalidate that there are different concerns at\n  play.\n\n  You probably even know people who are good at one of those concerns, but\n  not at the other.\n\n- If it true what you say, that a bug fix should be in the same commit as\n  the regression test case designed to prevent the bug from being\n  reintroduced, then both should live in the same file, because it is\n  really the same concern.\n\n  But test code lives in a separate area of the source code, is not even\n  \"shipped with the product\", *because* it is a separate concern.\n\n- There is an entire industry built on books and training advertising\n  Test Driven Development (TDD). If you really believe what you said, that\n  regression tests are really the same concern as fixing the regressions,\n  then you claim that all of them are wrong. Which would be a, how can I\n  say it, erm, bold statement indeed.\n\n- You did brush aside the example I gave: Hannes Sixt applied a patch\n  that demonstrated the breakage, or more correctly: that *should* have\n  demonstrated a breakage.\n\n  Let's not brush aside this example. Let's not ignore it just like that.\n\n  This commit addressed a very specific concern: it verified that there is\n  a breakage. Or actually, that in this case, there was *no* breakage.\n\n  By not separating those concerns, as you would have me do it, I would\n  have made it harder to demonstrate that this bug fix was not even necessary\n  in git.git's `master`. I would have made it harder to bisect *where* in\n  Git for Windows' patch thicket the regression was introduced.\n\n  You might now say that it should have been easy to separate the concern\n  after the fact, to extract it from a commit that adds both fix and\n  regression test case.\n\n  But by that token, you should squash all contributions into a single, big,\n  honking commit that do way too much \"because it is easy to extract what\n  you need\". I agree, that sounds ridiculous.\n\n  And even saying that it would be easy to extract the test case from the\n  commit already admits that there is a good chunk that you can extract\n  *that can live on its own*, i.e. is a separate concern.\n\n- We introduced, specifically, the shell function `test_expect_failure` to\n  allow demonstrating breakages. If we really thought that that has no\n  value, if there should not be any commit that introduces a regression\n  test case without fixing any bug at the same time, if that was not a\n  concern, then we should get rid of that function. But experience tells us,\n  of course, that we need to keep it.\n\n- There is *semantic* meaning in `test_expect_failure`. Depriving the\n  commit history from the commits that introduce such test cases\n  demonstrating bugs, and from commits that fix those bugs, makes it\n  impossible to perform otherwise simple queries along the lines \"how many\n  regressions did <insert-name-here> fix in September 2017?\".\n\n  I know you are a fan of software archaeology as much as I am (maybe even\n  a bit more), so hopefully you find at least this argument convincing.\n\n> The concern of a simple single-patch is to fix a bug---the test that\n> shows what bug was fixed and protects the code by ensuring that a\n> reintroduction of the bug gets noticed is an integral part of it.\n\nBy this reasoning, it should not be possible (or allowed) to write and\ncontribute a patch that merely demonstrates that something is wrong with\nGit, but does not fix it.\n\nThat position is untenable.\n\n> It does not make much sense to artificially split it into two.\n\nLet's not brush aside the above-mentioned example, okay?\n\nLet's consider how much easier it was for Hannes (and for that matter, how\nmuch easier it is for *me*) to grab a patch that demonstrates a breakage,\napply it, and the report back whether I can confirm the breakage or not.\n\nIt would be *even* easier, of course, if I could just fetch a branch and\ncherry-pick, but that is not possible in a project that is centered around\na mailing list. So that's just not an option here.\n\nBut back to the point: there is a *lot* of value in having an easy way to\nreproduce breakages. Conflating this value with the value of a bug fix\ndoes a disservice to everybody involved.\n\nThere was nothing artificial about splitting this patch into two.\n\nEven worse: that commit was never artificially split to begin with! What\nyou assumed was incorrect: those two commits were always two commits. I\ncame up with the regression test case first, to convince myself that there\nwas something that needed fixing (much in the same way as Hannes wanted to\nconvince himself that there was something that needed fixing, and the fact\nthat that commit was separate from the fix made it very easy for Hannes to\npoint out that there was actually nothing in git.git yet that needed\nfixing).\n\nAnd if you are still averse to the idea of separating the concern of\ndemonstrating a breakage from the concern to fix it, I have really,\nreally, really bad news for you: a couple of my branches that I intend to\ncontribute as patch series, once they are finalized, *already* have those\ncommits that demonstrate breakages, *without* commits to fix them!\n\nI am sorry if that made you shudder ;-)\n\nYou even saw at least one of them:\nhttps://public-inbox.org/git/cab7bb36eb85dbe38ad95ee02b083f11f0820e24.1533421100.git.gitgitgadget@gmail.com/\n\nBut here is a silver lining for you: There is a real advantage of having\nthis regression test case in a separate patch *for you* (and other\nreviewers): That regression test case will most likely not change in\nsubsequent iterations of the patch series, while the fix will most likely\nlook *very* different. And by having the regression test case in a\ndifferent patch than the regression fix, you will have a very easy time\nverifying that it is unchanged from before, that you do not have to\nre-review it.\n\n(Although you might have to re-review it if you do not remember that patch\nfrom the beginning of August, last year.)\n\n> It's like arguing for adding declarations for new functions and\n> global variables in *.h files in a step before a separate step adds\n> their implementations in *.c files, claiming that the first step\n> designs the interface (which is sufficient for writing the client\n> code) and the second one gives an implementation of the interface\n> (which can be replaced later), and they address two separate\n> concerns.\n\nNo, that is an apples-to-oranges comparison because the declarations\nthemselves cannot really be used for anything, while the regression test\ncases demonstrating a breakage *can* be used for:\n\n- validating the claim that there is a breakage,\n\n- bisecting,\n\n- debugging,\n\n- implementing a fix,\n\n- verifying the claim that a given Git version has this breakage, or\n  disproving it.\n\n> And then you would find that there are some compilers that warn\n> against unimplemented functions and undefined variables.\n\nIndeed. And I often find myself between a rock and a hard place when I\nwant to wrap my patch series into nice, reviewable, logically separate\npatches, and I cannot, because we specifically discourage file-local\n(`static`) functions without users. Although we have not the faintest\nproblem with *global* functions without users (which makes no sense,\neither disallow both file-local *and* global functions without users, or\nallow both):\n\nIn b0fa1a3f9974 (test-lib-functions.sh: add generate_zero_bytes function,\n2019-02-09), a function was introduced that was then consumed in\n24b451e77c7d (t5318: replace use of /dev/zero with generate_zero_bytes,\n2019-02-09).\n\nIn f1f5de442faf (revision: add mark_tree_uninteresting_sparse,\n2019-01-16), we introduced a function that was consumed in the following\ncommit 4f6d26b16703 (list-objects: consume sparse tree walk, 2019-01-16).\n\nIn 47edb649973c (hex: introduce functions to print arbitrary hashes,\n2018-11-14), the hash_to_hex*() functions were added, to be used only in\nsubsequent commits.\n\nAnd the list goes on.\n\nSo in this instance, your argument is that we have to please the compiler,\neven if it does not make any logical sense for humans.\n\nAfter all, a patch series has a temporal order. It has a \"story arc\", so\nto say, or in musical terms, it is a \"slur\". The compiler does not know\nthat a function will be used, or a regression test will be marked as\nfixed, in a future patch. The human reviewer would. But we actively make\nit harder on the human reviewer, by combining, say, the (rather large)\nimplementation of a linear assignment problem solver into the same commit\nas its only user (also large), just to please the compiler.\n\nOr at least we would have made it harder on the human reviewer if we had\nnot used a *trick* to *fool* the compiler into not complaining: the\nimplementation was put into the separate file file linear-assignment.c,\n*even* if it has *but a single user*!\n\nAnd since you actually committed this in that form, you already proved\nthat you agree with me. You also did not find it better to smoosh separate\nconcerns into the same commit. You had no objection to apply a patch that\nhad no value whatsoever on its own, that only realizes its value together\nwith a subsequent commit actually introducing a caller to that\nfunctionality. Which is actually worse than a commit doing nothing else\nthan introducing a single regression test case that demonstrates a bug,\nwhich does have value on its own.\n\n> The \"solution\" would be to enclose the whole thing that was added in the\n> first \"*.h only patch\" inside \"#if 0/#endif\" to hide it from the\n> compiler ;-)\n\nYes, if we really wanted to cater to the compiler, rather than human\nreaders, then we would indeed do that.\n\nPlus, let's not forget about the fact that this is an apples to oranges\ncomparison. More on that below.\n\n> That in fact looks quite similar to how \"test only patch\" marks the new\n> tests as expecting failure in the beginning. Neither is truly useful\n> when applied to a context different from where it is originally\n> developed for.\n\nIf you really want to hear what the equivalent to your `.h only commit`\nwould be, here you go. It would look something like:\n\n-- snip --\ndiff --git t/...\n@@ ... @@\n '\n\n+test_expect_failure 'demonstrate a horrible bug' '\n+\tfalse # will be implemented later\n+'\n+\n test_done\n-- snap --\n\n*That* is the equivalent to your example of introducing just the function\nsignatures, leaving the actual implementation to a later commit.\n\nYou will note that this looks very different from what I am advertising,\nwhere the actual implementation that does demonstrate a horrible bug (and\nwill hopefully gain a second life after the bug is fixed by keeping the\nbug fixed) is added at the same time as the `test_expect_failure`.\n\nAnd if you really, really, really needed another argument in favor of\naccepting regression test-only patches as standalone commits: take the\nunfortunate example of\nhttps://public-inbox.org/git/CA+h-Bnuf6u=hkPBcxhMm06FbfkS+jtrozu+inqqmUY1cNkXrWQ@mail.gmail.com/\n\nIt might not have mattered in this case had you stopped discouraging\ncontributions of patches that merely add test cases demonstrating\nbreakages. But it could not have helped, for sure.\n\nIf we keep discouraging such patches (and note that I specifically\n*encouraged* such a patch instead [*1*]), if we keep insisting that a\nregression test case on its own is of no value, we will get even less of\nthose contributions.\n\nAnd we will be repeating the same type of story again, where a bug was\nreported, no patch was contributed (let alone applied) to demonstrate the\nbug, the bug will be fixed, only to be broken *just in time* for the next\nofficial Git version.\n\nThat is not what I want.\n\nAnd that is why I keep considering it a bad practice to discourage\nseparating commits adding regression tests from the corresponding commits\nthat fix the regressions.\n\nAs I said, it might not have mattered in this instance. Or in the next\none. Or even in many cases.\n\nBut encouraging that separation of concerns, acknowledging that there is a\nreal, measurable value not only in knowing about a bug, but in addition\nhaving an automated way to reproduce the bug (and to verify a bug fix),\n*this* will get us more of those contributions rather than less.\n\nPhew. What a long mail. I am glad you made it this far.\n\nLet me end by saying that laying out all of the above, I do not intend to\nannoy you or anything. I simply care about this project, and I want it to\nrun as smoothly as possible. And I really think that the separation of\nconcerns that we are talking about is one of the ways to make it run\nsmoother. To make it easier on contributors. To make it easier on\nreviewers. To make it easier on developers validating bugs.\n\nCiao,\nDscho\n\nFootnote *1*:\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1901091501320.41@tvgsbejvaqbjf.bet/\n"}]}