RFC patch, 2 partscount recursion depth

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

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

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

Hi Dominik,

On Thu, 21 Apr 2016, XZS wrote:

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.

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

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

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

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.

Hi Dominik,

On Thu, 21 Apr 2016, Dominik Fischer wrote:

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,

On Thu, 21 Apr 2016, Dominik Fischer wrote:

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