{"thread":{"id":"42097","subject":"[PATCH/RFC/GSoC 1/2] count recursion depth","startedAt":"2016-04-21T09:15:20Z","lastAt":"2016-04-22T06:45:46Z","messageCount":10,"participants":["XZS","Johannes Schindelin","Dominik Fischer","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"284025","messageId":"cover.1459946531.git.d.f.fischer@web.de","threadId":"42097","inReplyTo":"xmqqio0b5ash.fsf@gitster.mtv.corp.google.com","subject":"[PATCH/RFC/GSoC 0/2] add a add.patch config variable","fromName":"XZS","fromEmail":"d.f.fischer@web.de","sentAt":"2016-04-21T09:15:20Z","receivedAt":"2016-04-21T09:15:20Z","isPatch":true,"sender":{"key":"d.f.fischer@web.de","avatar":null},"body":"The following patches try to provide an add.patch configuration variable\nwhile keeping out of the way of other commands. To do so, an environment\nvariable counts how often git was recursively invoked. The variable is\nthen only considered in the first recursion.\n\nTo ensure other commands work as expected also when add.patch is set, I\nadded a test exemplifying the case with mergetool. To confirm the same\nfor other commands, I also reran all tests with an activated add.patch,\nall direct invocations of git-add in the tests augmented with\n--no-patch. You can reproduce this by running the following commands\nfrom the root of the git source code repository.\n\nsed -i '/add\\.patch/,/}/ {\n # pretend add.patch to always be active\n s/\\(patch_interactive = \\).*;/\\11;/\n}' builtin/add.c\nfind t -type f -exec sed -i ' # in all tests\n /(use\\|forget\\|hint/! { # exclude help texts\n # replace git add invocations, also when options are passed to git,\n # but not subcommands named add.\n s/git\\( -[^ ]\\+ [^ ]\\+\\| --[^ ]\\+=[^ ]\\+\\)* add/& --no-patch/g\n }\n /patch mode can be activated per option/ {\n # find add.patched test now invalid and deactivate\n s/\\(test_expect_\\)success/\\1failure/\n }\n' '{}' + \n\nI am unsure whether I placed all the code into the correct locations, so\ncomments are much appreciated, as well as opinions about the concept of\na recursion counter in general.\n\nRegards,\nXZS.\n\nXZS (2):\n count recursion depth\n add a add.patch config variable\n\n Documentation/config.txt | 6 ++++++\n Documentation/git-add.txt | 3 +++\n builtin/add.c | 3 +++\n cache.h | 1 +\n contrib/completion/git-completion.bash | 1 +\n git.c | 18 ++++++++++++++++++\n t/t0001-init.sh | 1 +\n t/t0120-recursion-depth.sh | 17 +++++++++++++++++\n t/t3701-add-interactive.sh | 27 +++++++++++++++++++++++++++\n 9 files changed, 77 insertions(+)\n create mode 100755 t/t0120-recursion-depth.sh\n\n-- \n2.8.0\n"},{"id":"284024","messageId":"821b86c5276488425e60c5c559394c919daba282.1459946531.git.d.f.fischer@web.de","threadId":"42097","inReplyTo":"cover.1459946531.git.d.f.fischer@web.de","subject":"[PATCH/RFC/GSoC 1/2] count recursion depth","fromName":"XZS","fromEmail":"d.f.fischer@web.de","sentAt":"2016-04-21T09:15:21Z","receivedAt":"2016-04-21T09:15:21Z","isPatch":true,"sender":{"key":"d.f.fischer@web.de","avatar":null},"body":"Some commands may want to act differently when called transitively by\nother git commands in contrast to when called by the user directly. The\nvariable recursion_depth provides all built-ins with a way to tell these\ncases apart.\n\nScripts can use the underlying environment variable GIT_RECURSION_DEPTH\nto the same purpose.\n\nWrappers around git can intentionally set GIT_RECURSION_DEPTH to ensure\na command acts as if it was called internally.\n\nSigned-off-by: XZS \n---\n cache.h | 1 +\n git.c | 17 +++++++++++++++++\n t/t0001-init.sh | 1 +\n t/t0120-recursion-depth.sh | 17 +++++++++++++++++\n 4 files changed, 36 insertions(+)\n create mode 100755 t/t0120-recursion-depth.sh\n\ndiff --git a/cache.h b/cache.h\nindex 9f09540..105607c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1777,6 +1777,7 @@ struct startup_info {\n \tconst char *prefix;\n };\n extern struct startup_info *startup_info;\n+extern int recursion_depth;\n \n /* merge.c */\n struct commit_list;\ndiff --git a/git.c b/git.c\nindex 968a8a4..0bcc7b4 100644\n--- a/git.c\n+++ b/git.c\n@@ -25,6 +25,7 @@ static const char *env_names[] = {\n };\n static char *orig_env[4];\n static int save_restore_env_balance;\n+int recursion_depth;\n \n static void save_env_before_alias(void)\n {\n@@ -630,12 +631,28 @@ static void restore_sigpipe_to_default(void)\n \tsignal(SIGPIPE, SIG_DFL);\n }\n \n+static int get_recursion_depth(void)\n+{\n+\tconst char *envrec = getenv(\"GIT_RECURSION_DEPTH\");\n+\treturn envrec ? strtol(envrec, NULL, 10) : 0;\n+}\n+\n+static int set_recursion_depth(int depth)\n+{\n+\tchar number[10]; // TODO compute length\n+\tsnprintf(number, sizeof(number), \"%i\", depth);\n+\treturn setenv(\"GIT_RECURSION_DEPTH\", number, 1);\n+}\n+\n int main(int argc, char **av)\n {\n \tconst char **argv = (const char **) av;\n \tconst char *cmd;\n \tint done_help = 0;\n \n+\trecursion_depth = get_recursion_depth();\n+\tset_recursion_depth(recursion_depth + 1);\n+\n \tcmd = git_extract_argv0_path(argv[0]);\n \tif (!cmd)\n \t\tcmd = \"git-help\";\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex a5b9e7a..69e7532 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -93,6 +93,7 @@ test_expect_success 'No extra GIT_* on alias scripts' '\n \t\tsed -n \\\n \t\t\t-e \"/^GIT_PREFIX=/d\" \\\n \t\t\t-e \"/^GIT_TEXTDOMAINDIR=/d\" \\\n+\t\t\t-e \"/^GIT_RECURSION_DEPTH=/d\" \\\n \t\t\t-e \"/^GIT_/s/=.*//p\" |\n \t\tsort\n \tEOF\ndiff --git a/t/t0120-recursion-depth.sh b/t/t0120-recursion-depth.sh\nnew file mode 100755\nindex 0000000..5aeb71a\n--- /dev/null\n+++ b/t/t0120-recursion-depth.sh\n@@ -0,0 +1,17 @@\n+#!/bin/sh\n+\n+test_description='recursion counter'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'recursion counter is 1 on direct run' '\n+\tgit config alias.one \"!echo \\$GIT_RECURSION_DEPTH\" &&\n+\ttest \"$(git one)\" -eq 1\n+'\n+\n+test_expect_success 'recursion counter is greater 1 on transitive run' '\n+\tgit config alias.two \"!git one\" &&\n+\ttest \"$(git two)\" -gt 1\n+'\n+\n+test_done\n-- \n2.8.0\n"},{"id":"284026","messageId":"e6c7aaaf26ec6b5021cc6f183f72b494fdceb330.1459946531.git.d.f.fischer@web.de","threadId":"42097","inReplyTo":"cover.1459946531.git.d.f.fischer@web.de","subject":"[PATCH/RFC/GSoC 2/2] add a add.patch config variable","fromName":"XZS","fromEmail":"d.f.fischer@web.de","sentAt":"2016-04-21T09:15:22Z","receivedAt":"2016-04-21T09:15:22Z","isPatch":true,"sender":{"key":"d.f.fischer@web.de","avatar":null},"body":"Users may like to review their changes when staging by default. It is\nalso a convenient safety feature for beginners nudging them to have a\nsecond look at their changes when composing a commit.\n\nTo this end, the config variable allows to have git-add to always act\nlike -p was passed.\n\nTo not affect other commands that use the add built-in, the variable\nlooses its effect when invoked transitively.\n\nSigned-off-by: XZS \n---\n\nI corrected the errorneous use of -p in the test. Thanks to Christian\nCouder for the notice.\n\n Documentation/config.txt | 6 ++++++\n Documentation/git-add.txt | 3 +++\n builtin/add.c | 3 +++\n contrib/completion/git-completion.bash | 1 +\n git.c | 3 ++-\n t/t3701-add-interactive.sh | 27 +++++++++++++++++++++++++++\n 6 files changed, 42 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex aea6bd1..73f7dfa 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -752,6 +752,12 @@ add.ignore-errors (deprecated)::\n \tas it does not follow the usual naming convention for configuration\n \tvariables.\n \n+add.patch::\n+\tConfigures 'git add' to always interactively choose hunks, hinting the\n+\tuser to review changes before staging. This is equivalent to adding the\n+\t'--patch' option to linkgit:git-add[1] and can be overwritten by\n+\tinvoking git-add with --no-patch.\n+\n alias.*::\n \tCommand aliases for the linkgit:git[1] command wrapper - e.g.\n \tafter defining \"alias.last = cat-file commit HEAD\", the invocation\ndiff --git a/Documentation/git-add.txt b/Documentation/git-add.txt\nindex 6a96a66..cdb6663 100644\n--- a/Documentation/git-add.txt\n+++ b/Documentation/git-add.txt\n@@ -92,6 +92,9 @@ OPTIONS\n This effectively runs `add --interactive`, but bypasses the\n initial command menu and directly jumps to the `patch` subcommand.\n See ``Interactive mode'' for details.\n++\n+The configuration variable `add.patch` can be set to true to make\n+this the default behaviour.\n \n -e::\n --edit::\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 145f06e..3249a55 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -272,6 +272,9 @@ static int add_config(const char *var, const char *value, void *cb)\n \t !strcmp(var, \"add.ignore-errors\")) {\n \t\tignore_add_errors = git_config_bool(var, value);\n \t\treturn 0;\n+\t} else if (!strcmp(var, \"add.patch\") && recursion_depth <= 0) {\n+\t\tpatch_interactive = git_config_bool(var, value);\n+\t\treturn 0;\n \t}\n \treturn git_default_config(var, value, cb);\n }\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex e3918c8..597d20f 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1969,6 +1969,7 @@ _git_config ()\n \tesac\n \t__gitcomp \"\n \t\tadd.ignoreErrors\n+\t\tadd.patch\n \t\tadvice.commitBeforeMerge\n \t\tadvice.detachedHead\n \t\tadvice.implicitIdentity\ndiff --git a/git.c b/git.c\nindex 0bcc7b4..df2fe58 100644\n--- a/git.c\n+++ b/git.c\n@@ -2,6 +2,7 @@\n #include \"exec_cmd.h\"\n #include \"help.h\"\n #include \"run-command.h\"\n+#include \n \n const char git_usage_string[] =\n \t\"git [--version] [--help] [-C ] [-c name=value]\\n\"\n@@ -639,7 +640,7 @@ static int get_recursion_depth(void)\n \n static int set_recursion_depth(int depth)\n {\n-\tchar number[10]; // TODO compute length\n+\tchar number[(int) ceil(log10(pow(2, sizeof(int))))];\n \tsnprintf(number, sizeof(number), \"%i\", depth);\n \treturn setenv(\"GIT_RECURSION_DEPTH\", number, 1);\n }\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex deae948..7ba2817 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -380,4 +380,31 @@ test_expect_success 'patch mode ignores unmerged entries' '\n \ttest_cmp expected diff\n '\n \n+test_expect_success 'patch mode can be activated per option' '\n+\tgit config add.patch true &&\n+\tgit reset --hard &&\n+\techo change >test &&\n+\tyes | git add > output &&\n+\tcat output &&\n+\tgrep \"Stage this hunk \\[y,n,q,a,d,/,e,?\\]?\" output\n+'\n+\n+test_expect_success 'add.patch configuration does not affect transitive add invocations' '\n+\tgit reset --hard &&\n+\tgit checkout -b main >/dev/null 2>&1 &&\n+\tgit branch branch &&\n+\techo change >test &&\n+\tgit add --no-patch test &&\n+\tgit commit -m commit >/dev/null 2>&1 &&\n+\tgit checkout branch >/dev/null 2>&1 &&\n+\techo other >test &&\n+\tgit add --no-patch test &&\n+\tgit commit -m other >/dev/null 2>&1 &&\n+\ttest_must_fail git merge main >/dev/null 2>&1 &&\n+\tgit config merge.tool mybase &&\n+\tgit config mergetool.mybase.cmd \"cat \\\"\\$BASE\\\" >\\\"\\$MERGED\\\"\" &&\n+\tgit config mergetool.mybase.trustExitCode true &&\n+\tgit mergetool 2>&1 1>/dev/null | test_must_fail grep -q \"ignoring unmerged\"\n+'\n+\n test_done\n-- \n2.8.0\n"},{"id":"284038","messageId":"alpine.DEB.2.20.1604211737170.2826@virtualbox","threadId":"42097","inReplyTo":"cover.1459946531.git.d.f.fischer@web.de","subject":"Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-21T15:39:52Z","receivedAt":"2016-04-21T15:39:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Dominik,\n\nOn Thu, 21 Apr 2016, XZS wrote:\n\n> The following patches try to provide an add.patch configuration variable\n> while keeping out of the way of other commands. To do so, an environment\n> variable counts how often git was recursively invoked. The variable is\n> then only considered in the first recursion.\n\nThis dives right into the \"how\". Maybe a good idea would be to start with\na paragraph that explains the \"what\" and the \"why\" first, and only then go\ninto the details how you implemented it, what other options you considered\nand why you preferred your solution?\n\nEven after skimming the patch series, I am still a bit puzzled what issue\nit solves...\n\nCiao,\nJohannes\n"},{"id":"284043","messageId":"34fb607f-1c4c-528a-cb5d-eca95e0e7c68@web.de","threadId":"42097","inReplyTo":"alpine.DEB.2.20.1604211737170.2826@virtualbox","subject":"Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable","fromName":"Dominik Fischer","fromEmail":"d.f.fischer@web.de","sentAt":"2016-04-21T16:30:04Z","receivedAt":"2016-04-21T16:30:04Z","isPatch":true,"sender":{"key":"d.f.fischer@web.de","avatar":null},"body":"Indeed this needs more explanations for everyone who did not read the \nposts before.\n\nI strove to create an add.patch configuration option that did the same \nas always passing the parameter --patch to git-add. Junio C Hamano then \nmade me aware that when set, this option would influence and possibly \ndestroy other commands that internally use git-add. So I implemented the \nrecursion counter, which is now the first of the two commits. With this, \ngit-add is able to only consider the configuration option when run \ndirectly by the user, not affecting any commands building upon it.\n\nI would be interested whether this is a suited method to restrict the \neffect of a configuration option to cases where a command is explicitly \ninvoked by the user.\n\nRegards.\n"},{"id":"284047","messageId":"vpqr3dyud3i.fsf@anie.imag.fr","threadId":"42097","inReplyTo":"34fb607f-1c4c-528a-cb5d-eca95e0e7c68@web.de","subject":"Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-04-21T16:43:29Z","receivedAt":"2016-04-21T16:43:29Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Dominik Fischer writes:\n\n> I strove to create an add.patch configuration option that did the same\n> as always passing the parameter --patch to git-add. Junio C Hamano\n> then made me aware that when set, this option would influence and\n> possibly destroy other commands that internally use git-add. So I\n> implemented the recursion counter, which is now the first of the two\n> commits.\n\nThis does not solve the problem of scripting. People write scripts, and\nwrite git commands in these scripts. \"git add\" is a very good candidate\nto appear in scripts, indeed.\n\nYou can't break people's scripts without a very solid transition plan.\nAnd most of the time, safe transition plans are also painful for\neveryone (remember the push.default change? I'm happy it's done, but\nthat wasn't a lightweight change ...).\n\nIn this case, it's quite clear to me that any transition plan would be\nfar more painful than the possible benefits. Without breaking backward\ncompatibility, I can already set \"alias.p = add --patch\" and use \"git p\"\ninstead of \"git add --patch\". Even better: that's 2 less keystrokes.\n\nA config variable to set an option by default is good when the user\nwants to set it and forget about it. In this case, you clearly can't\n\"forget about it\", as running \"git add\" and \"git add -p\" correspond to\nreally different use-cases.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"284048","messageId":"xmqq4mauucx6.fsf@gitster.mtv.corp.google.com","threadId":"42097","inReplyTo":"vpqr3dyud3i.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-21T16:47:17Z","receivedAt":"2016-04-21T16:47:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> A config variable to set an option by default is good when the user\n> wants to set it and forget about it. In this case, you clearly can't\n> \"forget about it\", as running \"git add\" and \"git add -p\" correspond to\n> really different use-cases.\n\nThat is the most sensible explanation I've heard so far; the\nconfiguration variable add.patch should not exist, period.\n"},{"id":"284055","messageId":"d6747d29-601f-d9ba-2f27-812bf064edff@web.de","threadId":"42097","inReplyTo":"xmqq4mauucx6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable","fromName":"Dominik Fischer","fromEmail":"d.f.fischer@web.de","sentAt":"2016-04-21T17:28:53Z","receivedAt":"2016-04-21T17:28:53Z","isPatch":true,"sender":{"key":"d.f.fischer@web.de","avatar":null},"body":"Matthieu Moy writes:\n > any transition plan would be far more painful than the possible benefits\nI agree, it cannot be expected that every external script sets \nGIT_RECURSION_DEPTH before issuing any command just because of this \nlittle option.\n\nAs an exercise for GSoC, I implemented it nonetheless to get more \nfamiliar with the code base and testing system and receive some \nfeedback. Perhaps the counter can even be useful for some actually \nsignificant use case in the future.\n\nRegards.\n"},{"id":"284097","messageId":"alpine.DEB.2.20.1604220836060.2826@virtualbox","threadId":"42097","inReplyTo":"34fb607f-1c4c-528a-cb5d-eca95e0e7c68@web.de","subject":"Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-22T06:42:32Z","receivedAt":"2016-04-22T06:42:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Dominik,\n\nOn Thu, 21 Apr 2016, Dominik Fischer wrote:\n\n> Indeed this needs more explanations for everyone who did not read the posts\n> before.\n\nSuch as is the case for me. And most future readers of the commit messages\n;-)\n\n> I strove to create an add.patch configuration option that did the same as\n> always passing the parameter --patch to git-add.\n\nAh. So a much more intriguing mail subject would be \"add: optionally imply\n--patch by default\"?\n\n> Junio C Hamano then made me aware that when set, this option would\n> influence and possibly destroy other commands that internally use\n> git-add. So I implemented the recursion counter, which is now the first\n> of the two commits. With this, git-add is able to only consider the\n> configuration option when run directly by the user, not affecting any\n> commands building upon it.\n\nHmm. But what if `git add` was not run by the user, but rather by a\nscript? I am wary that the recursion counter may not really be able to\nanswer the question \"Was this `git add` called by the user *directly*?\".\n\n> I would be interested whether this is a suited method to restrict the\n> effect of a configuration option to cases where a command is explicitly\n> invoked by the user.\n\nAs I said, I do not think this method can do that reliably.\n\nTraditionally, we recommend aliases in such a case. They are just as\nopt-in and also config settings:\n\n\tgit config [--global] alias.ap 'add -p'\n\nThen, `git ap m*.cpp` would work just like you would expect.\n\nCiao,\nJohannes\n"},{"id":"284098","messageId":"alpine.DEB.2.20.1604220843310.2826@virtualbox","threadId":"42097","inReplyTo":"d6747d29-601f-d9ba-2f27-812bf064edff@web.de","subject":"Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-22T06:45:46Z","receivedAt":"2016-04-22T06:45:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Dominik,\n\nOn Thu, 21 Apr 2016, Dominik Fischer wrote:\n\n> Matthieu Moy writes:\n> > any transition plan would be far more painful than the possible benefits\n> I agree, it cannot be expected that every external script sets\n> GIT_RECURSION_DEPTH before issuing any command just because of this little\n> option.\n> \n> As an exercise for GSoC, I implemented it nonetheless to get more familiar\n> with the code base and testing system and receive some feedback. Perhaps the\n> counter can even be useful for some actually significant use case in the\n> future.\n\nMaybe. We come up with tons of patches all the time, and most do not make\nit into the code base, though.\n\nI still think it was a valuable thing to do: you are now much more\nfamiliar with Git's code base. And you now know why the proposed solution\nwon't work. And I guess the alias is what you'll end up using.\n\nCiao,\nJohannes\n"}]}