[GSoC] Proposal

[GSoC] Proposal

[XZS]

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

[PATCH/GSoC] add a add.patch config variable

[XZS]

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.

Signed-off-by: XZS <d.f.fischer@web.de>
---
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(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..614c599 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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
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..04f8b5e 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")) {
+		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/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index deae948..25e4b2e 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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

Re: [PATCH/GSoC] add a add.patch config variable

[Junio C Hamano]

XZS <d.f.fischer@web.de> writes:

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

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

Re: [PATCH/GSoC] add a add.patch config variable

[Dominik Fischer]

Am 24.03.2016 um 22:20 schrieb Junio C Hamano:
> XZS <d.f.fischer@web.de> writes:
>> 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.
>
> 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"?

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.

Re: [PATCH/GSoC] add a add.patch config variable

[Junio C Hamano]

Dominik Fischer <d.f.fischer@web.de> writes:

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

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.

Re: [PATCH/GSoC] add a add.patch config variable

[Christian Couder]

On Thu, Mar 24, 2016 at 10:09 PM, XZS <d.f.fischer@web.de> wrote:
>
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index deae948..25e4b2e 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -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 &&

Why are you using "-p" if you want to test that the config option works?

> +  cat output &&
> +  grep "Stage this hunk \[y,n,q,a,d,/,e,?\]?" output
> +'
> +
>  test_done

[PATCH/RFC/GSoC 0/2] add a add.patch config variable

[XZS]

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

[PATCH/RFC/GSoC 1/2] count recursion depth

[XZS]

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 <d.f.fischer@web.de>
---
 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

Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable

[Johannes Schindelin]

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

Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable

[Dominik Fischer]

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.

Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable

[Matthieu Moy]

Dominik Fischer <d.f.fischer@web.de> 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/

Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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.

Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable

[Dominik Fischer]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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.

Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable

[Johannes Schindelin]

Hi Dominik,

On Thu, 21 Apr 2016, Dominik Fischer wrote:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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

Re: [PATCH/RFC/GSoC 0/2] add a add.patch config variable

[Johannes Schindelin]

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

[PATCH/RFC/GSoC 2/2] add a add.patch config variable

[XZS]

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 <d.f.fischer@web.de>
---

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 <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);
 }
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