# "git add -i" with path gives "Argument list too long" 5 messages from 2010-03-21 to 2010-03-22. Participants: Wincent Colaiuta, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/23128 ## Wincent Colaiuta, 2010-03-21 20:52 Subject: "git add -i" with path gives "Argument list too long" Message-ID: <45CD9D1B-6774-46F2-A444-654B1DEF7F69@wincent.com> URL: https://gitlist.dev/e/45CD9D1B-6774-46F2-A444-654B1DEF7F69%40wincent.com ``` Back in January I reported a wart in "git add -i" and discussion kind of petered out and I forgot about the bug until today when I ran into it again. Now I have a few more cycles to burn I have the time to sort it out. Just to revise what was discussed back in January: - my original report: http://lists-archives.org/git/708569-git-add-i-with-path-gives-argument-list-too-long.html - quick and easy fix posted by Jeff King: http://lists-archives.org/git/708591-git-add-i-with-path-gives-argument-list-too-long.html - notes on inconsistencies between "git add" and "git add -i" by Jeff king: http://lists-archives.org/git/708698-git-add-i-with-path-gives-argument-list-too-long.html To summarize and expand a bit upon what Jeff noted in that last post: - given _tracked_ file "foo.c" and _untracked_ file "bar.c" - "git add '*.c'" ignores the tracked file and stages the untracked file - "git add -i '*.c'" ignores the untracked file and operates on the tracked file So, I gather that we can agree that this behavior isn't right. Either we treat the arguments as pathspecs or we don't, but it doesn't seem right to do one thing for tracked files and another for untracked ones, and it seems especially wrong for the differential treatment to apply in one direction with "git add" and in the other with "git add -i". So, as a first step I'd like to make some test cases to serve as a specification for what we want the desired behavior to be. I saw in one of the "what's cooking messages" in February that there was some changes cooking in the "cp/add-u-pathspec" topic, so I am not entirely clear on if there is consensus about the desired behaviour. Before I run ahead and start writing test, do we have consensus? (Will have to worry about the initial cause of my bug report -- the "argument list too long" error -- later on.) Cheers, Wincent ``` ## Jeff King, 2010-03-22 00:39 Subject: Re: "git add -i" with path gives "Argument list too long" Message-ID: <20100322003915.GA3212@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100322003915.GA3212%40coredump.intra.peff.net In-Reply-To: <45CD9D1B-6774-46F2-A444-654B1DEF7F69@wincent.com> ``` On Sun, Mar 21, 2010 at 09:52:56PM +0100, Wincent Colaiuta wrote: > Back in January I reported a wart in "git add -i" and discussion kind > of petered out and I forgot about the bug until today when I ran into > it again. Now I have a few more cycles to burn I have the time to sort > it out. Thanks for following up. > To summarize and expand a bit upon what Jeff noted in that last post: > > - given _tracked_ file "foo.c" and _untracked_ file "bar.c" > - "git add '*.c'" ignores the tracked file and stages the untracked file > - "git add -i '*.c'" ignores the untracked file and operates on the > tracked file > [...] > So, as a first step I'd like to make some test cases to serve as a > specification for what we want the desired behavior to be. I saw in I'm pretty sure the behavior that we want eventually is for both the diff family (including diff-files), and "git add" to learn about the globbing pathspecs that ls-files uses. That will make "git add" and "git add -i" consistent, and once diff-files understands it, we can drop the call to ls-files in add--interactive, which will solve your "argument list too long" problem. And yes, I am being intentionally vague about the behavior beyond saying "do what ls-files does". Personally I am not too familiar with the globbing pathspecs, so I am not qualified to give an exhaustive list of their behavior. Junio could probably say more, or you will have to read the code. -Peff ``` ## Junio C Hamano, 2010-03-22 01:23 Subject: Re: "git add -i" with path gives "Argument list too long" Message-ID: <7vwrx56tet.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwrx56tet.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100322003915.GA3212@coredump.intra.peff.net> ``` Jeff King writes: > their behavior. Junio could probably say more, or you will have to read > the code. Or read what I already said here a few times ;-) I generally do not want to repeat myself. There are two semantics of pathspecs: (1) exact match, or leading path. e.g. git ls-files Makefile Documentation/ (2) exact match, leading path, or fnmatch(3). git ls-files Makefile Documentation/ '*.txt' The former is used by the diff family, and the latter by pretty much everything else. In very old days, the former was the only kind we supported, and originally, ls-files didn't even take any pathspecs. The 5be4efb ([PATCH] Make "git-ls-files" work in subdirectories, 2005-08-21) taught ls-files to take pathspec that can glob, but diff family never got updated to match that. In order to operate on set of paths, you would need to (a) enumerate your paths, and (b) filter that enumeration efficiently with pathspecs. If you are iterating over the index (e.g. "ls-files", "diff-files", "grep"), there is nothing tricky in the enumeration step. We have a flat array of names in the index and you just walk active_cache[] from 0 to active_nr. If on the other hand you are walking in an inherently hierarchical namespace (e.g. "ls-tree", "diff-tree", "grep --no-index") with non empty set of pathspecs, you need to take advantage of the filtering behaviour while enumerating the paths---otherwise your performance will suck. "Leading path" semantics is easier to understand; if a tree entry you are looking at is "contrib", and the pathspecs you have are "Makefile" and "Documentation/", then there is no way for anything underneath it (e.g. "contrib/README") would survive the filtering process, so you can skip the entry without even opening the sub-tree object. Linus's argument was "teaching globs to pathspec code would suck in performance" and he is right in general. Because diff-tree is inherently about walking the two tree objects in parallel, it does not extend its pathspec semantics to globbing (i.e. if the user asked for '*.txt', you have to open _all_ the tree objects down to the leaf level to see if they contain any file whose name ends with .txt), and other family members of diff (namely, diff-files and "diff" without --cached nor any tree-ish argument) match this behaviour for consistency, even though theoretically "diff-files" could easily do globbing, as it walks the flat index namespace. But I think it is Ok to sacrifice the optimization and descend into any and all subtrees/directories to see if a path that might match the pattern exists when the user asks for '*.txt', as long as (and this is a _very_ important point) an update to pathspec logic on the diff side does not break the optimization unnecessarily. E.g. git diff v1.0.0 v1.2.0 -- Makefile 'Documentation/*.txt' should still skip opening tree object for 'contrib/' (because anything underneath contrib/ would never match either pathspecs given), but can and should descend into Documentation. And it should _not_ skip 'howto' subdirectory in Documentation/ directory, as it could find a match with '*.txt' in that subdirectory. To prepare for this, later reimplementations of pathspec matching logic (the one used by "git grep") can compute hints meant to be used by the path enumeration step, as I explained earlier, enumeration needs to take advantage of what filtering would do to paths that it will find. By the way I threw this "pathspec unification" to the list of possible GSoC ideas, but I suspect it might be a bit too much work to do this properly for a summer student (and also we might not want to trust this important part of the system to a summer student). Other things that probably needs to be thought through (and I haven't) that may be related to this codepath is how to handle case-insensitive filesystems. I think we currently do not match paths that we obtain from the filesystem case insensitively with the given pathspecs (we probably shouldn't go case insensitive when we are walking the index or the tree objects, on the other hand). ``` ## Jeff King, 2010-03-22 01:41 Subject: Re: "git add -i" with path gives "Argument list too long" Message-ID: <20100322014100.GA7717@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100322014100.GA7717%40coredump.intra.peff.net In-Reply-To: <7vwrx56tet.fsf@alter.siamese.dyndns.org> ``` On Sun, Mar 21, 2010 at 06:23:38PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > their behavior. Junio could probably say more, or you will have to read > > the code. > > Or read what I already said here a few times ;-) I generally do not want > to repeat myself. Well, yeah. :) What I meant was: > There are two semantics of pathspecs: > > (1) exact match, or leading path. e.g. > > git ls-files Makefile Documentation/ > > (2) exact match, leading path, or fnmatch(3). > > git ls-files Makefile Documentation/ '*.txt' Is type (2) exactly fnmatch? Or are there subtle corner cases that should be tested? For the implementation it shouldn't matter, as hopefully the same code paths will be used, and "just like ls-files" should be specification enough. But for testing, it might be nice to enumerate some of the more subtle cases. -Peff ``` ## Junio C Hamano, 2010-03-22 01:55 Subject: Re: "git add -i" with path gives "Argument list too long" Message-ID: <7veijd3ysc.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7veijd3ysc.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100322014100.GA7717@coredump.intra.peff.net> ``` Jeff King writes: > Is type (2) exactly fnmatch? It calls fnmatch(3) but without FNM_PATHNAME nor FNM_PERIOD, which makes me sometimes wonder if it was a historical mistake and go to "git log" to check, but it is more-or-less deliberate choice, as illustrated by the 'arch/i386/*.[ch]' example in 5be4efb ([PATCH] Make "git-ls-files" work in subdirectories, 2005-08-21). ```