# [PATCH/RFC/GSoC 1/2] count recursion depth 10 messages from 2016-04-21 to 2016-04-22. Participants: XZS, Johannes Schindelin, Dominik Fischer, Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/42097 ## XZS, 2016-04-21 09:15 Subject: [PATCH/RFC/GSoC 0/2] add a add.patch config variable Message-ID: URL: https://gitlist.dev/e/cover.1459946531.git.d.f.fischer%40web.de In-Reply-To: ``` The following patches try to provide an add.patch configuration variable while keeping out of the way of other commands. To do so, an environment variable counts how often git was recursively invoked. The variable is then only considered in the first recursion. To ensure other commands work as expected also when add.patch is set, I added a test exemplifying the case with mergetool. To confirm the same for other commands, I also reran all tests with an activated add.patch, all direct invocations of git-add in the tests augmented with --no-patch. You can reproduce this by running the following commands from the root of the git source code repository. sed -i '/add\.patch/,/}/ { # pretend add.patch to always be active s/\(patch_interactive = \).*;/\11;/ }' builtin/add.c find t -type f -exec sed -i ' # in all tests /(use\|forget\|hint/! { # exclude help texts # replace git add invocations, also when options are passed to git, # but not subcommands named add. s/git\( -[^ ]\+ [^ ]\+\| --[^ ]\+=[^ ]\+\)* add/& --no-patch/g } /patch mode can be activated per option/ { # find add.patched test now invalid and deactivate s/\(test_expect_\)success/\1failure/ } ' '{}' + I am unsure whether I placed all the code into the correct locations, so comments are much appreciated, as well as opinions about the concept of a recursion counter in general. Regards, XZS. XZS (2): count recursion depth add a add.patch config variable Documentation/config.txt | 6 ++++++ Documentation/git-add.txt | 3 +++ builtin/add.c | 3 +++ cache.h | 1 + contrib/completion/git-completion.bash | 1 + git.c | 18 ++++++++++++++++++ t/t0001-init.sh | 1 + t/t0120-recursion-depth.sh | 17 +++++++++++++++++ t/t3701-add-interactive.sh | 27 +++++++++++++++++++++++++++ 9 files changed, 77 insertions(+) create mode 100755 t/t0120-recursion-depth.sh -- 2.8.0 ``` ## XZS, 2016-04-21 09:15 Subject: [PATCH/RFC/GSoC 1/2] count recursion depth Message-ID: <821b86c5276488425e60c5c559394c919daba282.1459946531.git.d.f.fischer@web.de> URL: https://gitlist.dev/e/821b86c5276488425e60c5c559394c919daba282.1459946531.git.d.f.fischer%40web.de In-Reply-To: ``` Some commands may want to act differently when called transitively by other git commands in contrast to when called by the user directly. The variable recursion_depth provides all built-ins with a way to tell these cases apart. Scripts can use the underlying environment variable GIT_RECURSION_DEPTH to the same purpose. Wrappers around git can intentionally set GIT_RECURSION_DEPTH to ensure a command acts as if it was called internally. Signed-off-by: XZS --- cache.h | 1 + git.c | 17 +++++++++++++++++ t/t0001-init.sh | 1 + t/t0120-recursion-depth.sh | 17 +++++++++++++++++ 4 files changed, 36 insertions(+) create mode 100755 t/t0120-recursion-depth.sh diff --git a/cache.h b/cache.h index 9f09540..105607c 100644 --- a/cache.h +++ b/cache.h @@ -1777,6 +1777,7 @@ struct startup_info { const char *prefix; }; extern struct startup_info *startup_info; +extern int recursion_depth; /* merge.c */ struct commit_list; diff --git a/git.c b/git.c index 968a8a4..0bcc7b4 100644 --- a/git.c +++ b/git.c @@ -25,6 +25,7 @@ static const char *env_names[] = { }; static char *orig_env[4]; static int save_restore_env_balance; +int recursion_depth; static void save_env_before_alias(void) { @@ -630,12 +631,28 @@ static void restore_sigpipe_to_default(void) signal(SIGPIPE, SIG_DFL); } +static int get_recursion_depth(void) +{ + const char *envrec = getenv("GIT_RECURSION_DEPTH"); + return envrec ? strtol(envrec, NULL, 10) : 0; +} + +static int set_recursion_depth(int depth) +{ + char number[10]; // TODO compute length + snprintf(number, sizeof(number), "%i", depth); + return setenv("GIT_RECURSION_DEPTH", number, 1); +} + int main(int argc, char **av) { const char **argv = (const char **) av; const char *cmd; int done_help = 0; + recursion_depth = get_recursion_depth(); + set_recursion_depth(recursion_depth + 1); + cmd = git_extract_argv0_path(argv[0]); if (!cmd) cmd = "git-help"; diff --git a/t/t0001-init.sh b/t/t0001-init.sh index a5b9e7a..69e7532 100755 --- a/t/t0001-init.sh +++ b/t/t0001-init.sh @@ -93,6 +93,7 @@ test_expect_success 'No extra GIT_* on alias scripts' ' sed -n \ -e "/^GIT_PREFIX=/d" \ -e "/^GIT_TEXTDOMAINDIR=/d" \ + -e "/^GIT_RECURSION_DEPTH=/d" \ -e "/^GIT_/s/=.*//p" | sort EOF diff --git a/t/t0120-recursion-depth.sh b/t/t0120-recursion-depth.sh new file mode 100755 index 0000000..5aeb71a --- /dev/null +++ b/t/t0120-recursion-depth.sh @@ -0,0 +1,17 @@ +#!/bin/sh + +test_description='recursion counter' + +. ./test-lib.sh + +test_expect_success 'recursion counter is 1 on direct run' ' + git config alias.one "!echo \$GIT_RECURSION_DEPTH" && + test "$(git one)" -eq 1 +' + +test_expect_success 'recursion counter is greater 1 on transitive run' ' + git config alias.two "!git one" && + test "$(git two)" -gt 1 +' + +test_done -- 2.8.0 ``` ## XZS, 2016-04-21 09:15 Subject: [PATCH/RFC/GSoC 2/2] add a add.patch config variable Message-ID: URL: https://gitlist.dev/e/e6c7aaaf26ec6b5021cc6f183f72b494fdceb330.1459946531.git.d.f.fischer%40web.de In-Reply-To: ``` Users may like to review their changes when staging by default. It is also a convenient safety feature for beginners nudging them to have a second look at their changes when composing a commit. To this end, the config variable allows to have git-add to always act like -p was passed. To not affect other commands that use the add built-in, the variable looses its effect when invoked transitively. Signed-off-by: XZS --- I corrected the errorneous use of -p in the test. Thanks to Christian Couder for the notice. Documentation/config.txt | 6 ++++++ Documentation/git-add.txt | 3 +++ builtin/add.c | 3 +++ contrib/completion/git-completion.bash | 1 + git.c | 3 ++- t/t3701-add-interactive.sh | 27 +++++++++++++++++++++++++++ 6 files changed, 42 insertions(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index aea6bd1..73f7dfa 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -752,6 +752,12 @@ add.ignore-errors (deprecated):: as it does not follow the usual naming convention for configuration variables. +add.patch:: + Configures 'git add' to always interactively choose hunks, hinting the + user to review changes before staging. This is equivalent to adding the + '--patch' option to linkgit:git-add[1] and can be overwritten by + invoking git-add with --no-patch. + alias.*:: Command aliases for the linkgit:git[1] command wrapper - e.g. after defining "alias.last = cat-file commit HEAD", the invocation diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 6a96a66..cdb6663 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -92,6 +92,9 @@ OPTIONS This effectively runs `add --interactive`, but bypasses the initial command menu and directly jumps to the `patch` subcommand. See ``Interactive mode'' for details. ++ +The configuration variable `add.patch` can be set to true to make +this the default behaviour. -e:: --edit:: diff --git a/builtin/add.c b/builtin/add.c index 145f06e..3249a55 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -272,6 +272,9 @@ static int add_config(const char *var, const char *value, void *cb) !strcmp(var, "add.ignore-errors")) { ignore_add_errors = git_config_bool(var, value); return 0; + } else if (!strcmp(var, "add.patch") && recursion_depth <= 0) { + patch_interactive = git_config_bool(var, value); + return 0; } return git_default_config(var, value, cb); } diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index e3918c8..597d20f 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1969,6 +1969,7 @@ _git_config () esac __gitcomp " add.ignoreErrors + add.patch advice.commitBeforeMerge advice.detachedHead advice.implicitIdentity diff --git a/git.c b/git.c index 0bcc7b4..df2fe58 100644 --- a/git.c +++ b/git.c @@ -2,6 +2,7 @@ #include "exec_cmd.h" #include "help.h" #include "run-command.h" +#include const char git_usage_string[] = "git [--version] [--help] [-C ] [-c name=value]\n" @@ -639,7 +640,7 @@ static int get_recursion_depth(void) static int set_recursion_depth(int depth) { - char number[10]; // TODO compute length + char number[(int) ceil(log10(pow(2, sizeof(int))))]; snprintf(number, sizeof(number), "%i", depth); return setenv("GIT_RECURSION_DEPTH", number, 1); } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index deae948..7ba2817 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -380,4 +380,31 @@ test_expect_success 'patch mode ignores unmerged entries' ' test_cmp expected diff ' +test_expect_success 'patch mode can be activated per option' ' + git config add.patch true && + git reset --hard && + echo change >test && + yes | git add > output && + cat output && + grep "Stage this hunk \[y,n,q,a,d,/,e,?\]?" output +' + +test_expect_success 'add.patch configuration does not affect transitive add invocations' ' + git reset --hard && + git checkout -b main >/dev/null 2>&1 && + git branch branch && + echo change >test && + git add --no-patch test && + git commit -m commit >/dev/null 2>&1 && + git checkout branch >/dev/null 2>&1 && + echo other >test && + git add --no-patch test && + git commit -m other >/dev/null 2>&1 && + test_must_fail git merge main >/dev/null 2>&1 && + git config merge.tool mybase && + git config mergetool.mybase.cmd "cat \"\$BASE\" >\"\$MERGED\"" && + git config mergetool.mybase.trustExitCode true && + git mergetool 2>&1 1>/dev/null | test_must_fail grep -q "ignoring unmerged" +' + test_done -- 2.8.0 ``` ## Johannes Schindelin, 2016-04-21 15:39 Subject: Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604211737170.2826%40virtualbox In-Reply-To: ``` Hi Dominik, On Thu, 21 Apr 2016, XZS wrote: > The following patches try to provide an add.patch configuration variable > while keeping out of the way of other commands. To do so, an environment > variable counts how often git was recursively invoked. The variable is > then only considered in the first recursion. This dives right into the "how". Maybe a good idea would be to start with a paragraph that explains the "what" and the "why" first, and only then go into the details how you implemented it, what other options you considered and why you preferred your solution? Even after skimming the patch series, I am still a bit puzzled what issue it solves... Ciao, Johannes ``` ## Dominik Fischer, 2016-04-21 16:30 Subject: Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable Message-ID: <34fb607f-1c4c-528a-cb5d-eca95e0e7c68@web.de> URL: https://gitlist.dev/e/34fb607f-1c4c-528a-cb5d-eca95e0e7c68%40web.de In-Reply-To: ``` Indeed this needs more explanations for everyone who did not read the posts before. I strove to create an add.patch configuration option that did the same as always passing the parameter --patch to git-add. Junio C Hamano then made me aware that when set, this option would influence and possibly destroy other commands that internally use git-add. So I implemented the recursion counter, which is now the first of the two commits. With this, git-add is able to only consider the configuration option when run directly by the user, not affecting any commands building upon it. I would be interested whether this is a suited method to restrict the effect of a configuration option to cases where a command is explicitly invoked by the user. Regards. ``` ## Matthieu Moy, 2016-04-21 16:43 Subject: Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable Message-ID: URL: https://gitlist.dev/e/vpqr3dyud3i.fsf%40anie.imag.fr In-Reply-To: <34fb607f-1c4c-528a-cb5d-eca95e0e7c68@web.de> ``` Dominik Fischer writes: > I strove to create an add.patch configuration option that did the same > as always passing the parameter --patch to git-add. Junio C Hamano > then made me aware that when set, this option would influence and > possibly destroy other commands that internally use git-add. So I > implemented the recursion counter, which is now the first of the two > commits. This does not solve the problem of scripting. People write scripts, and write git commands in these scripts. "git add" is a very good candidate to appear in scripts, indeed. You can't break people's scripts without a very solid transition plan. And most of the time, safe transition plans are also painful for everyone (remember the push.default change? I'm happy it's done, but that wasn't a lightweight change ...). In this case, it's quite clear to me that any transition plan would be far more painful than the possible benefits. Without breaking backward compatibility, I can already set "alias.p = add --patch" and use "git p" instead of "git add --patch". Even better: that's 2 less keystrokes. A config variable to set an option by default is good when the user wants to set it and forget about it. In this case, you clearly can't "forget about it", as running "git add" and "git add -p" correspond to really different use-cases. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Junio C Hamano, 2016-04-21 16:47 Subject: Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable Message-ID: URL: https://gitlist.dev/e/xmqq4mauucx6.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Matthieu Moy writes: > A config variable to set an option by default is good when the user > wants to set it and forget about it. In this case, you clearly can't > "forget about it", as running "git add" and "git add -p" correspond to > really different use-cases. That is the most sensible explanation I've heard so far; the configuration variable add.patch should not exist, period. ``` ## Dominik Fischer, 2016-04-21 17:28 Subject: Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable Message-ID: URL: https://gitlist.dev/e/d6747d29-601f-d9ba-2f27-812bf064edff%40web.de In-Reply-To: ``` Matthieu Moy writes: > any transition plan would be far more painful than the possible benefits I agree, it cannot be expected that every external script sets GIT_RECURSION_DEPTH before issuing any command just because of this little option. As an exercise for GSoC, I implemented it nonetheless to get more familiar with the code base and testing system and receive some feedback. Perhaps the counter can even be useful for some actually significant use case in the future. Regards. ``` ## Johannes Schindelin, 2016-04-22 06:42 Subject: Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604220836060.2826%40virtualbox In-Reply-To: <34fb607f-1c4c-528a-cb5d-eca95e0e7c68@web.de> ``` Hi Dominik, On Thu, 21 Apr 2016, Dominik Fischer wrote: > Indeed this needs more explanations for everyone who did not read the posts > before. Such as is the case for me. And most future readers of the commit messages ;-) > I strove to create an add.patch configuration option that did the same as > always passing the parameter --patch to git-add. Ah. So a much more intriguing mail subject would be "add: optionally imply --patch by default"? > Junio C Hamano then made me aware that when set, this option would > influence and possibly destroy other commands that internally use > git-add. So I implemented the recursion counter, which is now the first > of the two commits. With this, git-add is able to only consider the > configuration option when run directly by the user, not affecting any > commands building upon it. Hmm. But what if `git add` was not run by the user, but rather by a script? I am wary that the recursion counter may not really be able to answer the question "Was this `git add` called by the user *directly*?". > I would be interested whether this is a suited method to restrict the > effect of a configuration option to cases where a command is explicitly > invoked by the user. As I said, I do not think this method can do that reliably. Traditionally, we recommend aliases in such a case. They are just as opt-in and also config settings: git config [--global] alias.ap 'add -p' Then, `git ap m*.cpp` would work just like you would expect. Ciao, Johannes ``` ## Johannes Schindelin, 2016-04-22 06:45 Subject: Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1604220843310.2826%40virtualbox In-Reply-To: ``` Hi Dominik, On Thu, 21 Apr 2016, Dominik Fischer wrote: > Matthieu Moy writes: > > any transition plan would be far more painful than the possible benefits > I agree, it cannot be expected that every external script sets > GIT_RECURSION_DEPTH before issuing any command just because of this little > option. > > As an exercise for GSoC, I implemented it nonetheless to get more familiar > with the code base and testing system and receive some feedback. Perhaps the > counter can even be useful for some actually significant use case in the > future. Maybe. We come up with tons of patches all the time, and most do not make it into the code base, though. I still think it was a valuable thing to do: you are now much more familiar with Git's code base. And you now know why the proposed solution won't work. And I guess the alias is what you'll end up using. Ciao, Johannes ```