The -d flag pre-dated git-clean's ability to have paths specified. As
such, the default for git-clean was to only remove untracked files in
the current directory, and -d existed to allow it to recurse into
subdirectories.
The interaction of paths and the -d option appears to not have been
carefully considered, as evidenced by numerous bugs and a dearth of
tests covering such pairings in the testsuite. The definition turns out
to be important, so let's look at some of the various ways one could
interpret the -d option:
A) Without -d, only look in subdirectories which contain tracked
files under them; with -d, also look in subdirectories which
are untracked for files to clean.
B) Without specified paths from the user for us to delete, we need to
have some kind of default, so...without -d, only look in
subdirectories which contain tracked files under them; with -d,
also look in subdirectories which are untracked for files to clean.
The important distinction here is that choice B says that the presence
or absence of '-d' is irrelevant if paths are specified. The logic
behind option B is that if a user explicitly asked us to clean a
specified pathspec, then we should clean anything that matches that
pathspec. Some examples may clarify. Should
git clean -f untracked_dir/file
remove untracked_dir/file or not? It seems crazy not to, but a strict
reading of option A says it shouldn't be removed. How about
git clean -f untracked_dir/file1 tracked_dir/file2
or
git clean -f untracked_dir_1/file1 untracked_dir_2/file2
? Should it remove either or both of these files? Should it require
multiple runs to remove both the files listed? (If this sounds like a
crazy question to even ask, see the commit message of "t7300: Add some
testcases showing failure to clean specified pathspecs" added earlier in
this patch series.) What if -ffd were used instead of -f -- should that
allow these to be removed? Should it take multiple invocations with
-ffd? What if a glob (such as '*tracked*') were used instead of
spelling out the directory names? What if the filenames involved globs,
such as
git clean -f '*.o'
or
git clean -f '*/*.o'
?
The current documentation actually suggests a definition that is
slightly different than choice A, and the implementation prior to this
series provided something radically different than either choices A or
B. (The implementation, though, was clearly just buggy). There may be
other choices as well. However, for almost any given choice of
definition for -d that I can think of, some of the examples above will
appear buggy to the user. The only case that doesn't have negative
surprises is choice B: treat a user-specified path as a request to clean
all untracked files which match that path specification, including
recursing into any untracked directories.
Change the documentation and basic implementation to use this
definition.
There were two regression tests that indirectly depended on the current
implementation, but neither was about subdirectory handling. These two
tests were introduced in commit 5b7570cfb4 ("git-clean: add tests for
relative path", 2008-03-07) which was solely created to add coverage for
the changes in commit fb328947c8e ("git-clean: correct printing relative
path", 2008-03-07). Both tests specified a directory that happened to
have an untracked subdirectory, but both were only checking that the
resulting printout of a file that was removed was shown with a relative
path. Update these tests appropriately.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
It appears that the wrong option got included in the list of what will
cause git-clean to actually take action. Correct the list.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The way match_pathspec_item() handles names and pathspecs with trailing
slash characters, in conjunction with special options like
DO_MATCH_DIRECTORY and DO_MATCH_LEADING_PATHSPEC were non-obvious, and
broken until this patch series. Add a table in a comment explaining the
intent of how these work.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For git clean, if a directory is entirely untracked and the user did not
specify -d (corresponding to DIR_SHOW_IGNORED_TOO), then we usually do
not want to remove that directory and thus do not recurse into it.
However, if the user manually specified specific (or even globbed) paths
somewhere under that directory to remove, then we need to recurse into
the directory to make sure we remove the relevant paths under that
directory as the user requested.
Note that this does not mean that the recursed-into directory will be
added to dir->entries for later removal; as of a few commits earlier in
this series, there is another more strict match check that is run after
returning from a recursed-into directory before deciding to add it to the
list of entries. Therefore, this will only result in files underneath
the given directory which match one of the pathspecs being added to the
entries list.
Two notes of potential interest to future readers:
* If we wanted to only recurse into a directory when it is specifically
matched rather than matched-via-glob (e.g. '*.c'), then we could do
so via making the final non-zero return in match_pathspec_item be
MATCHED_RECURSIVELY instead of MATCHED_RECURSIVELY_LEADING_PATHSPEC.
(Note that the relative order of MATCHED_RECURSIVELY_LEADING_PATHSPEC
and MATCHED_RECURSIVELY are important for such a change.) I was
leaving open that possibility while writing an RFC asking for the
behavior we want, but even though we don't want it, that knowledge
might help you understand the code flow better.
* There is a growing amount of logic in read_directory_recursive() for
deciding whether to recurse into a subdirectory. However, there is a
comment immediately preceding this logic that says to recurse if
instructed by treat_path(). It may be better for the logic in
read_directory_recursive() to ultimately be moved to treat_path() (or
another function it calls, such as treat_directory()), but I have
left that for someone else to tackle in the future.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The specific checks done in match_pathspec_item for the DO_MATCH_SUBMODULE
case are useful for other cases which have nothing to do with submodules.
Rename this constant; a subsequent commit will make use of this change.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Even if a directory doesn't match a pathspec, it is possible, depending
on the precise pathspecs, that some file underneath it might. So we
special case and recurse into the directory for such situations. However,
we previously always added any untracked directory that we recursed into
to the list of untracked paths, regardless of whether the directory
itself matched the pathspec.
For the case of git-clean and a set of pathspecs of "dir/file" and "more",
this caused a problem because we'd end up with dir entries for both of
"dir"
"dir/file"
Then correct_untracked_entries() would try to helpfully prune duplicates
for us by removing "dir/file" since it's under "dir", leaving us with
"dir"
Since the original pathspec only had "dir/file", the only entry left
doesn't match and leaves nothing to be removed. (Note that if only one
pathspec was specified, e.g. only "dir/file", then the common_prefix_len
optimizations in fill_directory would cause us to bypass this problem,
making it appear in simple tests that we could correctly remove manually
specified pathspecs.)
Fix this by actually checking whether the directory we are about to add
to the list of dir entries actually matches the pathspec; only do this
matching check after we have already returned from recursing into the
directory.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For a pathspec like 'foo/bar' comparing against a path named "foo/",
namelen will be 4, and match[namelen] will be 'b'. The correct location
of the directory separator is namelen-1.
However, other callers of match_pathspec_item() such as builtin/grep.c's
submodule_path_match() will compare against a path named "foo" instead of
"foo/". It might be better to change all the callers to be consistent,
as discussed at
https://public-inbox.org/git/xmqq7e6cdnkr.fsf@gitster-ct.c.googlers.com/
and
https://public-inbox.org/git/CABPp-BERWUPCPq-9fVW1LNocqkrfsoF4BPj3gJd9+En43vEkTQ@mail.gmail.com/
but there are many cases to audit, so for now just make sure we handle
both cases with and without a trailing slash.
The reason the code worked despite this sometimes-off-by-one error was
that the subsequent code immediately checked whether the first matchlen
characters matched (which they do) and then bailed and return
MATCHED_RECURSIVELY anyway since wildmatch doesn't have the ability to
check if "name" can be matched as a directory (or prefix) against the
pathspec.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Someone brought me a testcase where multiple git-clean invocations were
required to clean out unwanted files:
mkdir d{1,2}
touch d{1,2}/ut
touch d1/t && git add d1/t
With this setup, the user would need to run
git clean -ffd */ut
twice to delete both ut files.
A little testing showed some interesting variants:
* If only one of those two ut files existed (either one), then only one
clean command would be necessary.
* If both directories had tracked files, then only one git clean would
be necessary to clean both files.
* If both directories had no tracked files then the clean command above
would never clean either of the untracked files despite the pathspec
explicitly calling both of them out.
A bisect showed that the failure to clean out the files started with
commit cf424f5fd8 ("clean: respect pathspecs with "-d", 2014-03-10).
However, that pointed to a separate issue: while the "-d" flag was used
by the original user who showed me this problem, that flag should have
been irrelevant to this problem. Testing again without the "-d" flag
showed that the same buggy behavior exists without using that flag, and
has in fact existed since before cf424f5fd8.
Although these problems at first are perceived to be different (e.g.
never clearing out the requested files vs. taking multiple invocations
to get everything cleared out), they are actually just different
manifestations of the same problem. The case with multiple directories
that have no tracked files is the more general case; solving it will
solve all the others. So, I concentrate on it. Add testcases showing
that multiple untracked files within entirely untracked directories
cannot be cleaned when specifying these files to git clean via
pathspecs.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
'git clean -fd' must not delete an untracked directory if it belongs
to a different Git repository or worktree. Unfortunately, if a
'.gitignore' rule in the outer repository happens to match a file in a
nested repository or worktree, then something goes awry and 'git clean
-fd' does delete the content of the nested repository's worktree
except that ignored file, potentially leading to data loss.
Add a test to 't7300-clean.sh' to demonstrate this breakage.
This issue is a regression introduced in 6b1db43109 (clean: teach
clean -d to preserve ignored paths, 2017-05-23).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
My IEE 'home for life' email service is being withdrawn on 30 Sept 2019.
Replace with my new email domain.
I also have a secondary (backup) 'home for life' through
<philipoakley@dunelm.org.uk>.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
Signed-off-by: Philip Oakley <philipoakley@iee.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Compilation fix.
* cb/xdiff-no-system-includes-in-dot-c:
xdiff: remove duplicate headers from xpatience.c
xdiff: remove duplicate headers from xhistogram.c
xdiff: drop system includes in xutils.c
The internal diff machinery can be made to read out of bounds while
looking for --funcion-context line in a corner case, which has been
corrected.
* jk/xdiff-clamp-funcname-context-index:
xdiff: clamp function context indices in post-image
We have been trying out a few language features outside c89; the
coding guidelines document did not talk about them and instead had
a blanket ban against them.
* jc/post-c89-rules-doc:
CodingGuidelines: spell out post-C89 rules
Code restructuring during 2.20 period broke fetching tags via
"import" based transports.
* fc/fetch-with-import-fix:
fetch: fix regression with transport helpers
fetch: make the code more understandable
fetch: trivial cleanup
t5801 (remote-helpers): add test to fetch tags
t5801 (remote-helpers): cleanup refspec stuff
The commit-graph file is now part of the "files that the runtime
may keep open file descriptors on, all of which would need to be
closed when done with the object store", and the file descriptor to
an existing commit-graph file now is closed before "gc" finalizes a
new instance to replace it.
* ds/close-object-store:
packfile: rename close_all_packs to close_object_store
packfile: close commit-graph in close_all_packs
commit-graph: use raw_object_store when closing
commit-graph: extract write_commit_graph_file()
commit-graph: extract copy_oids_to_commits()
commit-graph: extract count_distinct_commits()
commit-graph: extract fill_oids_from_all_packs()
commit-graph: extract fill_oids_from_commit_hex()
commit-graph: extract fill_oids_from_packs()
commit-graph: create write_commit_graph_context
commit-graph: remove Future Work section
commit-graph: collapse parameters into flags
commit-graph: return with errors during write
commit-graph: fix the_repository reference
"git checkout -p" needs to selectively apply a patch in reverse,
which did not work well.
* pw/add-p-recount:
add -p: fix checkout -p with pathological context
Code clean-up to avoid signed integer overlaps during binary search.
* rs/avoid-overflow-in-midpoint-computation:
cleanup: fix possible overflow errors in binary search, part 2
"git interpret-trailers" always treated '#' as the comment
character, regardless of core.commentChar setting, which has been
corrected.
* jk/trailers-use-config:
interpret-trailers: load default config
"git stash show 23" used to work, but no more after getting
rewritten in C; this regression has been corrected.
* tg/stash-ref-by-index-fix:
stash: fix show referencing stash index
"git rebase --abort" used to leave refs/rewritten/ when concluding
"git rebase -r", which has been corrected.
* pw/rebase-abort-clean-rewritten:
rebase --abort/--quit: cleanup refs/rewritten
sequencer: return errors from sequencer_remove_state()
rebase: warn if state directory cannot be removed
rebase: fix a memory leak
An incorrect list of options was cached after command line
completion failed (e.g. trying to complete a command that requires
a repository outside one), which has been corrected.
* nd/completion-no-cache-failure:
completion: do not cache if --git-completion-helper fails
The code to parse scaled numbers out of configuration files has
been made more robust and also easier to follow.
* rs/config-unit-parsing:
config: simplify parsing of unit factors
config: don't multiply in parse_unit_factor()
config: use unsigned_mult_overflows to check for overflows
The codepath to compute delta islands used to spew progress output
without giving the callers any way to squelch it, which has been
fixed.
* jk/delta-islands-progress-fix:
delta-islands: respect progress flag
Use "Erase in Line" CSI sequence that is already used in the editor
support to clear cruft in the progress output.
* sg/rebase-progress:
progress: use term_clear_line()
rebase: fix garbled progress display with '-x'
pager: add a helper function to clear the last line in the terminal
t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused
t3404: modernize here doc style
"git submodule foreach" did not protect command line options passed
to the command to be run in each submodule correctly, when the
"--recursive" option was in use.
* ms/submodule-foreach-fix:
submodule foreach: fix recursion of options
The configuration variable rebase.rescheduleFailedExec should be
effective only while running an interactive rebase and should not
affect anything when running an non-interactive one, which was not
the case. This has been corrected.
* js/rebase-reschedule-applies-only-to-interactive:
rebase --am: ignore rebase.rescheduleFailedExec
The "git clone" documentation refers to command line options in its
description in the short form; they have been replaced with long
forms to make them more recognisable.
* qn/clone-doc-use-long-form:
docs: git-clone: list short form of options first
docs: git-clone: refer to long form of options
"git rm" to resolve a conflicted path leaked an internal message
"needs merge" before actually removing the path, which was
confusing. This has been corrected.
* jc/denoise-rm-to-resolve:
rm: resolving by removal is not a warning-worthy event
A codepath that reads from GPG for signed object verification read
past the end of allocated buffer, which has been fixed.
* sr/gpg-interface-stop-at-the-end:
gpg-interface: do not scan past the end of buffer
"git clean" silently skipped a path when it cannot lstat() it; now
it gives a warning.
* js/clean-report-too-long-a-path:
clean: show an error message when the path is too long
"git push --atomic" that goes over the transport-helper (namely,
the smart http transport) failed to prevent refs to be pushed when
it can locally tell that one of the ref update will fail without
having to consult the other end, which has been corrected.
* es/local-atomic-push-failure-with-http:
transport-helper: avoid var decl in for () loop control
transport-helper: enforce atomic in push_refs_with_push
Dev support update.
* sg/ci-brew-gcc-workaround:
ci/lib.sh: update a comment about installed P4 and Git-LFS versions
ci: disable Homebrew's auto cleanup
ci: don't update Homebrew
Elijah Newren
SZEDER Gábor
Junio C Hamano
Philip Oakley
Martin Ågren