git-apply does not work in a sub-directory of a Git repository

Hello everyone,

Recently while using git-apply, I observed that if git-apply is used in a sub-directory of a Git repository then it silently dies without doing anything.

Here's what I did

~ $mkdir example
~ $cd example
example $git init
Initialized empty Git repository in /home/mj/example/.git/
example $echo main >main
example $git add main
example $git commit -m 'main'
[master (root-commit) 97aeda3] main
 1 file changed, 1 insertion(+)
 create mode 100644 main
example $git checkout -b feature
Switched to a new branch 'feature'
example $echo modified >main
example $git add main
example $git commit -m 'modified'
[feature 2551fa2] modified
 1 file changed, 1 insertion(+), 1 deletion(-)
example $mkdir outgoing
example $git diff master >outgoing/feature.patch
example $git checkout master
Switched to branch 'master'
example $cd outgoing/
outgoing $git apply feature.patch
outgoing $cd ..
example $cat main
main

As you observed, patch wasn't applied. Is it intended behaviour of git-apply? Usually to apply the patch I have to copy it to top directory and then use git-apply.

I tried out git-am to apply the patch ("git format-patch" was used to make patch) while being in the "outgoing" sub-directory and it worked fine. So why does git-apply show this kind of behaviour?

Thanks, Mehul

On Tue, Mar 22, 2016 at 5:10 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

Think of git-apply as a specialized version of 'patch', which would also error out if there are path issues. (Inside outgoing/ there is no file found at ./main)

git-am is the porcelain command which is what is recommended to users who interact with Git and patches.

On Wed, Mar 23, 2016 at 5:14 AM, Stefan Beller <sbeller@google.com> wrote:

git-am is about patches in mailbox form, not plain patches, isn't it? In that view, it's not a replacement for git-apply.

How about we start deprecating the old behavior?

1) add --no-index to force git-apply ignore .git, --git (or some other
name) to apply patches as if running from topdir, add a config key to
choose default behavior
2) when git-apply is run without --git, --index or --cached from a
subdir and the said config key is not set, start warning and
recommending --no-index
3) wait X years
4)switch default behavior to --git (if run inside a git repo)

Duy Nguyen <pclouds@gmail.com> writes:

I think we do have --no-index (which is why I am largely ignoring the rest of your message as uninformed speculation for now).

See

  http://thread.gmane.org/gmane.comp.version-control.git/288316/focus=288321

I agree it is bad that it silently ignores the path outside the directory. When run with --verbose, we should say "Skipped X that is outside the directory." or something like that, just like we issue notices when we applied with offset, etc.

Junio C Hamano <gitster@pobox.com> writes:

Another thing we may want to do is to loosen (or redo) the logic in builtin/apply.c::use_patch()

        static int use_patch(struct patch *p)
        {
                const char *pathname = p->new_name ? p->new_name : p->old_name;
                int i;

                /* Paths outside are not touched regardless of "--include" */
                if (0 < prefix_length) {
                        int pathlen = strlen(pathname);
                        if (pathlen <= prefix_length ||
                            memcmp(prefix, pathname, prefix_length))
                                return 0;
                }

The include/exclude mechanism does use wildmatch() but does not use the pathspec mechanism (it predates the pathspec machinery that was made reusable in places like this). We should be able to

    $ cd d/e/e/p/d/i/r
    $ git apply --include=:/ ../../../../../../../patch

to lift this limitation. IOW, we can think of the use_patch() to include only the paths in the subdirectory we are in by default, but we can make it allow --include/--exclude command line option to override that default.

That way, the plain-vanilla use would still retain the "when working in subdirectory, we only touch that subdirectory" behaviour, which existing scripts may depend on, but users can loosen the default as necessary.

On Wed, Mar 23, 2016 at 11:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

Interesting. Disabling that comment block seems to work ok. So git-apply works more like git-grep, automatically narrowing to current subdir, rather than full-tree like git-status. git-apply.txt should probably mention about this because (at least to me) it sounds more naturally that if I give a patch, git-apply should apply the whole patch.

We probably should show a warning if everything file is filtered out too because silence usually means "good" from a typical unix command. It could be guarded with advice config key, and should only show if it looks like there are matching paths on worktree, but filtered out. Hmm?

+Brian who also had issues with git-apply.

On Thu, Mar 24, 2016 at 5:49 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Implemented in [04/04] apply: report patch skipping in verbose mode.

I went with a new option instead of changing --include. Making it pathspec can still bite people. And pathspec is not exactly compatible with wildmatch either. This is in

  [03/04] apply: add --whole to apply git patch without prefix filtering

  [02/04] git-apply.txt: mention the behavior inside a subdir

I'm holding this back. Too much heuristics. -- Duy

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-apply.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-apply.txt | 2 ++
 1 file changed, 2 insertions(+)

Back in edf2e37 (git-apply: work from subdirectory. - 2005-11-25), git-apply is made to work from a subdir of a worktree. When applying a git patch this way, only paths in the subdir are patched, the rest is filtered out. To apply without filtering, the user has to move back to toplevel. Add --whole to make it more convenient to do so without moving cwd around.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-apply.txt |  8 +++++++-
 builtin/apply.c             |  4 +++-
 t/t4111-apply-subdir.sh     | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/apply.c | 2 ++
 1 file changed, 2 insertions(+)

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

It might be a "workable" band-aid, but would be an unsatisfying UI if it were the endgame state. You do not say "git grep --whole" (by the way, "whole" is a bad option name, as you cannot tell "100% of *what*" you are referring to--what you are widening is the limit based on the location in the directory structure, so the option name should have some hint about it, e.g. "full-tree" or something) and this command will become an odd-man-out.

I haven't thought things through, but thinking out aloud a few points...

  An existing user/script may be working in a subdirectory of a huge
  working tree and relies on the current behaviour that outside world
  is excluded by default, and may be passing --exclude to further
  limit the extent of damage by applying the patch to a subset of
  paths in the current directory that itself is also huge.  And that
  use case would not be harmed by such a change.

  On the other hand, an existing user/script would not be passing an
  "--include" that names outside the current subdirectory to force
  them to be included, because it is known for the past 10 years not
  to have any effect at all.

So a better alternative may be to conditionally disable the "Paths outside are not touched regardless of --include" logic, i.e. we exclude paths outside by default just as before, but if there is at least one explicit "--include" given, we skip this "return 0".

That way, we do not have to commit to turning --include/--exclude to pathspec (which I agree is a huge change in behaviour that may not be a good idea) and we do not have to add "--full-tree" that is only understood by "apply" but not other commands that operate on the current directory by default.

I agree that the "excluded because the path is outside cwd" should be reported just like we show notices when applying a hunk with offset, and that the "excluded because the path is outside cwd" should be documented.

Junio C Hamano <gitster@pobox.com> writes:

And the necessary change to do so may look like this.  With this:

    $ git show >P
    $ git reset --hard HEAD^
    $ cd t
    $ git apply -v ../P
    $ git apply -v --include=\* ../P

seem to work as expected.

On Thu, Mar 24, 2016 at 11:50 PM, Junio C Hamano <gitster@pobox.com> wrote:

Real-world .gitignore patterns have taught me that even if it does not have any effect, it might still be present in some scripts, waiting for a chance to bite me.

But your suggestion is good and I can't think of any better. We could introduce pathspec as ftiler after "--", but it does not look elegant, and it overlaps with --include/--exclude.

Perhaps we can start to warn people if --include is specified but has no effect for a cycle or two, then we can do as you suggested?

Duy Nguyen <pclouds@gmail.com> writes:

I was imagining that we would allow the magic pathspec syntax used in --include/--exclude command line option parameter. Nobody sane uses glob special characters in their pathnames and those that do deserve whatever breakage that comes to them.

I do not think I'd be against going in that direction.

On Thu, Mar 24, 2016 at 11:50 PM, Junio C Hamano <gitster@pobox.com> wrote:

Suppose I don't like git-apply's default behavior, I make an alias.ap = "apply --include=*". So far so good, but when I want to limit paths back to "subdir" (it does not have to be the same as cwd), how do I do it without typing resorting to typing "git apply" explicitly ? I don't see an option to cancel out --include=*. For "git ap --exclude=* --include=subdir" to have that effect, we need to change

for (i = 0; i < limit_by_name.nr; i++) {

in use_patch() to

for (i = limit_by_name.nr - 1; i >= 0; i--) {

Simple change, but not exactly harmless.

Off topic, but --include/--exclude should be able to deal with relative path like --include=../*.c or --include=./*. I guess nobody has complained about it, so it's not needed.

Duy Nguyen <pclouds@gmail.com> writes:

We used to have one argument when choosing between "limit to cwd" vs "work on full-tree" defaults, i.e. "a full-tree thing can easily be limited to cwd by passing '.' as a pathspec, but limited-to-cwd thing cannot be widened" before we introduced the magic "full-tree" pathspec.

This "limit to cwd" behaviour of "git apply" predates that by several years.

Yes, documentation update is necessary.

We should give an informational message if _anything_ is filtered out, I would say.

On Wed, Mar 23, 2016 at 8:51 PM, Junio C Hamano <gitster@pobox.com> wrote:

--no-index command line flag is there for git-apply but unfortunately not documented.

Also *auto-completion* for "git apply --no-index" doesn't work.

That is

    git apply --no-i<tab><tab>

should be auto completed and give

   git apply --no-index.

Probably following change will remove this problem.