{"thread":{"id":"41524","subject":"[PATCH/RFC] git-commit: add a commit.verbose config variable","startedAt":"2016-02-25T21:27:00Z","lastAt":"2016-02-26T17:32:55Z","messageCount":7,"participants":["Pranit Bauva","Matthieu Moy","Stefan Beller","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279387","messageId":"vpqmvqo1nx7.fsf@anie.imag.fr","threadId":"41524","inReplyTo":"56CFBF19.6040004@zoho.com","subject":"Re: [PATCH/RFC] git-commit: add a commit.verbose config variable","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-02-25T21:27:00Z","receivedAt":"2016-02-25T21:27:00Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Pranit Bauva writes:\n\n> From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001\n> From: Pranit Bauva \n> Date: Fri, 26 Feb 2016 07:14:18 +0530\n> Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable\n\nThese should not appear in the body of your message, but they should be\nthe actual headers of your email. Try sending such message to yourself,\nand compare with what you see on the list.\n\nUsing \"git send-email\" normally does the right thing. You may want to\nlook at https://submitgit.herokuapp.com/ too.\n\n> The variable `verbose` is changed instead of `s.verbose` as the method\n> run_status() updates the `s.verbose` with the value of `verbose`. So in\n> this way the change is reflected in both of them.\n\nThe commit message should not try to rephrase what the patch aleady\nsays.\n\n> This is a patch for the microproject of GSOC 2016. I have done the change\n> under careful consideration of where to place the line. I have to yet write\n> the tests for this.\n\nIf you know you haven't finished, you may use WIP (work in progress)\ninstead of RFC in the title.\n\n> +commit.verbose::\n> +\tA boolean to specify whether to always include the verbose option\n\nBoolean is usually written with a capital letter.\n\n> +\twith git-config.\n\nDid you mean \"git commit\"?\n\n> +\tof the commit message. If this option is used always, it can\n\n\"If this option is used always\" does not sound right. I'd write \"To\nactivate this option permanently, ...\"\n\n> +\tbe set in the git-config with the boolean variable `commit.verbose`.\n\n\"the git-config\" is not proper English. You mean \"a configuration file\".\nI'd write \"the configuration variable `commit.verbose` can be set to\ntrue\".\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1644,6 +1644,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n> \ts.colopts = 0;\n> \n> +\tgit_config_get_bool(\"commit.verbose\", &verbose);\n\nDoesn't this override any value that --verbose or --no-verbose may have\nset before?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"279390","messageId":"CAGZ79kaAv6yaDPU5roRCAniiq3+HqLXhyNEr7Bd4PwQTY_30QQ@mail.gmail.com","threadId":"41524","inReplyTo":"vpqmvqo1nx7.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC] git-commit: add a commit.verbose config variable","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-25T21:58:12Z","receivedAt":"2016-02-25T21:58:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 25, 2016 at 1:27 PM, Matthieu Moy\n wrote:\n>\n>> +commit.verbose::\n>> + A boolean to specify whether to always include the verbose option\n>\n> Boolean is usually written with a capital letter.\n\nI disagree here, (\"grep -riI boolean\" suggestes to only capitalize boolean\nat the beginning of a sentence.)\n"},{"id":"279385","messageId":"56CFBF19.6040004@zoho.com","threadId":"41524","inReplyTo":null,"subject":"[PATCH/RFC] git-commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@zoho.com","sentAt":"2016-02-26T02:57:29Z","receivedAt":"2016-02-26T02:57:29Z","isPatch":true,"sender":{"key":"pranit.bauva@zoho.com","avatar":null},"body":">From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001\nFrom: Pranit Bauva \nDate: Fri, 26 Feb 2016 07:14:18 +0530\nSubject: [PATCH/RFC] git-commit: add a commit.verbose config variable\n\nSince many people always run the command with this option, and would\nprefer not to use the argument again and again but instead specify it in\nsome config file.\n\nThe variable `verbose` is changed instead of `s.verbose` as the method\nrun_status() updates the `s.verbose` with the value of `verbose`. So in\nthis way the change is reflected in both of them.\n\nSigned-off-by: Pranit Bauva \n---\n\nNotes:\n This is a patch for the microproject of GSOC 2016. I have done the change\n under careful consideration of where to place the line. I have to yet write\n the tests for this. I have explored the config API and I am currently going\n through the tests part. I have run the test locally by manually checking.\n I currently learning about the test suite. I will update this patch\n with some tests in some time.\n\n Documentation/config.txt | 5 +++++\n Documentation/git-commit.txt | 3 ++-\n builtin/commit.c | 2 ++\n 3 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 01cca0a..f7e9c09 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1110,6 +1110,11 @@ commit.template::\n \t\"`~/`\" is expanded to the value of `$HOME` and \"`~user/`\" to the\n \tspecified user's home directory.\n \n+commit.verbose::\n+\tA boolean to specify whether to always include the verbose option\n+\twith git-config.\n+\tSee linkgit:git-commit[1]\n+\n credential.helper::\n \tSpecify an external helper to be called when a username or\n \tpassword credential is needed; the helper may consult external\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 9ec6b3c..2a72437 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].\n \twhat changes the commit has.\n \tNote that this diff output doesn't have its\n \tlines prefixed with '#'. This diff will not be a part\n-\tof the commit message.\n+\tof the commit message. If this option is used always, it can\n+\tbe set in the git-config with the boolean variable `commit.verbose`.\n +\n If specified twice, show in addition the unified diff between\n what would be committed and the worktree files, i.e. the unstaged\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b3bd2d4..68080fe 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1644,6 +1644,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n \ts.colopts = 0;\n \n+\tgit_config_get_bool(\"commit.verbose\", &verbose);\n+\n \tif (get_sha1(\"HEAD\", sha1))\n \t\tcurrent_head = NULL;\n \telse {\n-- \n2.1.4\n"},{"id":"279481","messageId":"CAPig+cQE6ytRKFjqRRLrPHCYqJuf52NKvy8sZs8rX3t5_kDRVg@mail.gmail.com","threadId":"41524","inReplyTo":"56CFBF19.6040004@zoho.com","subject":"Re: [PATCH/RFC] git-commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-26T03:00:51Z","receivedAt":"2016-02-26T03:00:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the submission. Review comments below...\n\nOn Thu, Feb 25, 2016 at 9:57 PM, Pranit Bauva wrote:\n> From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001\n> From: Pranit Bauva \n> Date: Fri, 26 Feb 2016 07:14:18 +0530\n> Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable\n\nDrop these four lines. The first is only meaningful in your\nrepository, and the rest are picked up automatically by git-am from\nthe email envelope.\n\n> Since many people always run the command with this option, and would\n> prefer not to use the argument again and again but instead specify it in\n> some config file.\n\nThis seems like a sentence fragment. I suppose you meant it as a\ncontinuation of the patch subject? Better would be to write a full\nsentence instead so that the reader doesn't have to guess at its\nmeaning.\n\n> The variable `verbose` is changed instead of `s.verbose` as the method\n> run_status() updates the `s.verbose` with the value of `verbose`. So in\n> this way the change is reflected in both of them.\n\nTalking about this in the commit message misleads the reader into\nthinking that there is some potential oddity going on where a careful\ndecision needs to be made about which variable to set, when that's not\nin fact the case. The 'verbose' member of wt_status is just one\nconsumer of the \"verbose\" flag, not the sole consumer. Another\nconsumer is found in builtin/commit.c:cmd_commit():\n\n if (verbose ||\n cleanup_mode == CLEANUP_SCISSORS)\n wt_status_truncate_message_at_cut_line(&sb);\n\nSo, it would not be correct for the configuration ever to set only\nwt_status::verbose.\n\nConsequently, it would be better to drop this paragraph altogether\nfrom the commit message, so as to avoid confusing readers.\n\n> Signed-off-by: Pranit Bauva \n> ---\n>\n> Notes:\n> This is a patch for the microproject of GSOC 2016. I have done the change\n> under careful consideration of where to place the line. I have to yet write\n> the tests for this. I have explored the config API and I am currently going\n> through the tests part. I have run the test locally by manually checking.\n> I currently learning about the test suite. I will update this patch\n> with some tests in some time.\n\nSome tests to consider:\n\n* commit.verbose unset\n* commit.verbose=true\n* commit.verbose=false\n* --verbose overrides commit.verbose\n* --no-verbose overrides commit.verbose\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 01cca0a..f7e9c09 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1110,6 +1110,11 @@ commit.template::\n> \"`~/`\" is expanded to the value of `$HOME` and \"`~user/`\" to the\n> specified user's home directory.\n>\n> +commit.verbose::\n> + A boolean to specify whether to always include the verbose option\n\nIt's nice to see a documentation update included in the patch.\n\n> + with git-config.\n\nI guess you meant \"git-commit\".\n\n> + See linkgit:git-commit[1]\n\nNit: It wouldn't hurt to fold this line into the line above.\n\n> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> @@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].\n> what changes the commit has.\n> Note that this diff output doesn't have its\n> lines prefixed with '#'. This diff will not be a part\n> - of the commit message.\n> + of the commit message. If this option is used always, it can\n> + be set in the git-config with the boolean variable `commit.verbose`.\n\nYou could probably replace this entire added sentence with the simpler:\n\n Also see the `commit.verbose` configuration variable.\n\n> If specified twice, show in addition the unified diff between\n> what would be committed and the worktree files, i.e. the unstaged\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index b3bd2d4..68080fe 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1644,6 +1644,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> status_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n> s.colopts = 0;\n>\n> + git_config_get_bool(\"commit.verbose\", &verbose);\n\nI haven't fully digested builtin/commit.c, but the placement of this\nnew code seems suspect. My expectation would have been to see\ngit_commit_config() updated to recognize the new \"commit.verbose\"\nvariable. Am I missing something?\n\n> if (get_sha1(\"HEAD\", sha1))\n> current_head = NULL;\n> else {\n> --\n> 2.1.4\n"},{"id":"279487","messageId":"56D05295.6020102@zoho.com","threadId":"41524","inReplyTo":"CAPig+cQE6ytRKFjqRRLrPHCYqJuf52NKvy8sZs8rX3t5_kDRVg@mail.gmail.com","subject":"Re: [PATCH/RFC] git-commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@zoho.com","sentAt":"2016-02-26T13:26:45Z","receivedAt":"2016-02-26T13:26:45Z","isPatch":true,"sender":{"key":"pranit.bauva@zoho.com","avatar":null},"body":"Matthieu Moy:\n> Using \"git send-email\" normally does the right thing. You may want to\n> look at https://submitgit.herokuapp.com/ too.\ngit send-email does not work as my institute proxy does not allow\nIMAP/SMTP connections. But I will take care of this from hence forward.\n\n> The commit message should not try to rephrase what the patch aleady\n> says.\n\nI am dropping this statement off because of one another reason which\nEric Sunshine gave.\n\n> If you know you haven't finished, you may use WIP (work in progress)\n> instead of RFC in the title.\n\nI wasn't familiar with this tag. I will keep it in mind. And this is not\nincluded in Documentation/SubmittingPatches , so I will send a patch to\ninclude WIP tag.\n\n> \"the git-config\" is not proper English. You mean \"a configuration file\".\n> I'd write \"the configuration variable `commit.verbose` can be set to\n> true\".\n\nI actually was facing problem in phrasing it. Thanks for your\nsuggestion. I will update this.\n\n>> +\twith git-config.\n>\n> Did you mean \"git commit\"?\nSorry for my carelessness. I will update this.\n\n\n> Doesn't this override any value that --verbose or --no-verbose may >\n have set before?\nYes, this was the problem. I have fixed it now. But there is a glitch.\nSee below.\n\nEric Sunshine:\n> On Thu, Feb 25, 2016 at 9:57 PM, Pranit Bauva wrote:\n>> From c273a02fc9cab9305cedf6e37422e257a1cc3b1e Mon Sep 17 00:00:00 2001\n>> From: Pranit Bauva \n>> Date: Fri, 26 Feb 2016 07:14:18 +0530\n>> Subject: [PATCH/RFC] git-commit: add a commit.verbose config variable\n\n>From hence forth I will take that into consideration.\n\n> Talking about this in the commit message misleads the reader into\n> thinking that there is some potential oddity going on where a careful\n> decision needs to be made about which variable to set, when that's not\n> in fact the case. The 'verbose' member of wt_status is just one\n> consumer of the \"verbose\" flag, not the sole consumer. Another\n> consumer is found in builtin/commit.c:cmd_commit():\n> \n> if (verbose ||\n> cleanup_mode == CLEANUP_SCISSORS)\n> wt_status_truncate_message_at_cut_line(&sb);\n> \n> So, it would not be correct for the configuration ever to set only\n> wt_status::verbose.\n> \n> Consequently, it would be better to drop this paragraph altogether\n> from the commit message, so as to avoid confusing readers.\n\nI guessed this parent-child behavior and I wanted the commit to sound\nlike that but now that I read it again, I can understand that it might\nconfuse readers who aren't familiar with the code base.\n\n> Some tests to consider:\n> \n> * commit.verbose unset\n\nThis was working perfectly fine!\n\n> * commit.verbose=true\n\nThis was working perfectly fine!\n\n> * commit.verbose=false\n\nThis was working perfectly fine!\n\n> * --verbose overrides commit.verbose\n\nThis was showing errors. So I now included an if statement and then\nplaced the whole code after the method parse_and_validate_options()\notherwise it would just ignore the behavior. Now even this is working\nperfectly fine.\n\n> * --no-verbose overrides commit.verbose\n\nThis is a problematic one as currently `git-commit` does not have such a\nbehavior. I tried printing value of `verbose` in both the cases ie. with\n`git commit` and `git commit --no-verbose` and in both of them it\nprinted the value 0. So currently, the program won't understand the\n\"overriding\" nature. I have an idea for implementing this. We can keep\nthe default nature to point out to 0 and `--no-verbose` to point to -1.\nBut I guess this problem would have been faced/implemented in other part\nof the code. I will have to look in different parts of code and see how\nthis has been done in those so as to maintain the practices followed\nwith git. I am currently not quite familiar with `parse-options.c` so I\nwill read about it. But you could help by pointing out specific commits\nor email threads which are related to `--no-verbose` option in other git\ncommands to speed up the process.\n\n> I haven't fully digested builtin/commit.c, but the placement of this\n> new code seems suspect. My expectation would have been to see\n> git_commit_config() updated to recognize the new \"commit.verbose\"\n> variable. Am I missing something?\n\nI too had a lot of confusing regarding this. But it seems to me that the\nmethod git_commit_config() shows a \"different\" behavior and I don't\nthink such \"complicated\" behavior is required for reading the boolean\nvariable `commit.verbose`.\n"},{"id":"279535","messageId":"xmqqio1bl8a7.fsf@gitster.mtv.corp.google.com","threadId":"41524","inReplyTo":"56D05295.6020102@zoho.com","subject":"Re: [PATCH/RFC] git-commit: add a commit.verbose config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-26T16:56:48Z","receivedAt":"2016-02-26T16:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n>> If you know you haven't finished, you may use WIP (work in progress)\n>> instead of RFC in the title.\n>\n> I wasn't familiar with this tag. I will keep it in mind. And this is not\n> included in Documentation/SubmittingPatches , so I will send a patch to\n> include WIP tag.\n\nPlease don't. It is OK to say WIP or RFC or some people even say\n[not a patch] there; we do not need to enumerate all of them, and\nyour original RFC was just fine.\n"},{"id":"279542","messageId":"vpqoab3s7g8.fsf@anie.imag.fr","threadId":"41524","inReplyTo":"xmqqio1bl8a7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC] git-commit: add a commit.verbose config variable","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-02-26T17:32:55Z","receivedAt":"2016-02-26T17:32:55Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Pranit Bauva writes:\n>\n>>> If you know you haven't finished, you may use WIP (work in progress)\n>>> instead of RFC in the title.\n>>\n>> I wasn't familiar with this tag. I will keep it in mind. And this is not\n>> included in Documentation/SubmittingPatches , so I will send a patch to\n>> include WIP tag.\n>\n> Please don't. It is OK to say WIP or RFC or some people even say\n> [not a patch] there; we do not need to enumerate all of them, and\n> your original RFC was just fine.\n\nIn case it was unclear: my \"you may\" here was really meant to be \"you\nmay\", not \"you should\" ;-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}