Proposal

Greetings,

I hope it is not yet too late to jump on the Summer of Code bandwagon.

I would appreciate comments on my application [1] and my microproject contribution, which will follow this mail as a reply.

My proposal mostly stems from what was noted under "convert scripts to builtins" and "git rebase improvements" in the ideas page. Both list no mentor, so please let me know if you know anyone who should be mentioned in CC.

Regards,
XZS.

[1]: https://docs.google.com/document/d/1-BV-s5VUGTvBlcVDeo6tVqQO5D1hqeQDqaf37iYuIfU/edit?usp=sharing

2.7.4

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.

---
The list of microproject suggestions for the Summer of Code 2016 proposed to "add configuration options for some commonly used command-line options", so I added a configuraion variable for an option I use all the time.

 Documentation/config.txt               | 5 +++++
 Documentation/git-add.txt              | 3 +++
 builtin/add.c                          | 3 +++
 contrib/completion/git-completion.bash | 1 +
 t/t3701-add-interactive.sh             | 9 +++++++++
 5 files changed, 21 insertions(+)

index 2cd6bdd..614c599 100644
@@ -752,6 +752,11 @@ 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].
+
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.
 	after defining "alias.last = cat-file commit HEAD", the invocation

index 6a96a66..cdb6663 100644
@@ -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::

index 145f06e..04f8b5e 100644
@@ -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")) {
+		patch_interactive = git_config_bool(var, value);
+		return 0;
 	}
 	return git_default_config(var, value, cb);
 }

index e3918c8..597d20f 100644
@@ -1969,6 +1969,7 @@ _git_config ()
 	esac
 	__gitcomp "
 		add.ignoreErrors
+		add.patch
 		advice.commitBeforeMerge
 		advice.detachedHead
 		advice.implicitIdentity

index deae948..25e4b2e 100755
@@ -380,4 +380,13 @@ 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 &&
+  echo y | git add -p > output &&
+  cat output &&
+  grep "Stage this hunk \[y,n,q,a,d,/,e,?\]?" output
+'
+
 test_done

2.7.4

Now with such a configuration in her ~/.gitconfig, how would she ever run the normal "git add", which perhaps is invoked by one of the scripts she regularly uses? E.g. "git mergetool"?

Am 24.03.2016 um 22:20 schrieb Junio C Hamano:

As the configuration variable can be overwritten by a command line option, I am tempted to amend this by replacing all occurrences of "git add" in other scripts with "git add --no-patch" to ensure the expected behavior.

But this would introduce changes into a vast number of points in the code. Apart from that, I suspect other options may have the same reason not to be available as config variables. It would be much better if git-add could ignore the variable when called internally, only taking it into account when used as an entrypoint.

Is there already a mechanism in place to determine if git was called by git? If not, I could implement one through an environment variable that counts up on each git invocation, essentially providing a git recursion counter.

Don't even think about it--you won't know all scripts you have to fix like that, as a lot more invocations of "git add" appear in end user's scripts that we have never seen.

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

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.

---
 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

index 9f09540..105607c 100644
@@ -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;

index 968a8a4..0bcc7b4 100644
@@ -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";

index a5b9e7a..69e7532 100755
@@ -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

new file mode 100755
index 0000000..5aeb71a
@@ -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

Hi Dominik,

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

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.

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/

That is the most sensible explanation I've heard so far; the configuration variable add.patch should not exist, period.

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.

Hi Dominik,

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

Hi Dominik,

Such as is the case for me. And most future readers of the commit messages ;-)

Ah. So a much more intriguing mail subject would be "add: optionally imply --patch by default"?

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*?".

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

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.

---

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(-)

index aea6bd1..73f7dfa 100644
@@ -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

index 6a96a66..cdb6663 100644
@@ -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::

index 145f06e..3249a55 100644
@@ -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);
 }

index e3918c8..597d20f 100644
@@ -1969,6 +1969,7 @@ _git_config ()
 	esac
 	__gitcomp "
 		add.ignoreErrors
+		add.patch
 		advice.commitBeforeMerge
 		advice.detachedHead
 		advice.implicitIdentity

index 0bcc7b4..df2fe58 100644
@@ -2,6 +2,7 @@
 #include "exec_cmd.h"
 #include "help.h"
 #include "run-command.h"
+#include <math.h>
 
 const char git_usage_string[] =
 	"git [--version] [--help] [-C <path>] [-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);
 }

index deae948..7ba2817 100755
@@ -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