| Message ID | 20190905154735.29784-1-newren@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix some git clean issues | expand |
On Thu, Sep 05, 2019 at 08:47:23AM -0700, Elijah Newren wrote: > This patch series fixes a few issues with git-clean: > * Failure to preserve both tracked and untracked files within a nested > Git repository reported a few weeks ago by SZEDER[3]. Wow, I didn't expect a 12 patch series to fix that issue... Thanks. > Elijah Newren (12): > t7300: Add some testcases showing failure to clean specified pathspecs > dir: fix typo in comment > dir: fix off-by-one error in match_pathspec_item > dir: Directories should be checked for matching pathspecs too > dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule > case > dir: If our pathspec might match files under a dir, recurse into it Nit: no capital letters after the '<area>:' prefix. > dir: add commentary explaining match_pathspec_item's return value > git-clean.txt: do not claim we will delete files with -n/--dry-run > clean: disambiguate the definition of -d > clean: avoid removing untracked files in a nested git repository > clean: rewrap overly long line > clean: fix theoretical path corruption > > Documentation/git-clean.txt | 16 +++++----- > builtin/clean.c | 17 ++++++++-- > dir.c | 63 +++++++++++++++++++++++++++---------- > dir.h | 8 +++-- > t/t7300-clean.sh | 44 +++++++++++++++++++++++--- > 5 files changed, 114 insertions(+), 34 deletions(-) > > -- > 2.22.1.11.g45a39ee867 >
On Thu, Sep 5, 2019 at 12:01 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Thu, Sep 05, 2019 at 08:47:23AM -0700, Elijah Newren wrote: > > This patch series fixes a few issues with git-clean: > > > * Failure to preserve both tracked and untracked files within a nested > > Git repository reported a few weeks ago by SZEDER[3]. > > Wow, I didn't expect a 12 patch series to fix that issue... > Thanks. Well, to be fair, only the last three patches were about that issue. The first 9 were about the other issues. It's just that your testcase reminded me of my old series and gave me another nudge to dig it out and see if it helped with your problem. I had to rebase it and look back over it, and then found it didn't help with your problem, but by then I had refamiliarized myself with the code so... > > Elijah Newren (12): > > t7300: Add some testcases showing failure to clean specified pathspecs > > dir: fix typo in comment > > dir: fix off-by-one error in match_pathspec_item > > dir: Directories should be checked for matching pathspecs too > > dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule > > case > > dir: If our pathspec might match files under a dir, recurse into it > > Nit: no capital letters after the '<area>:' prefix. Gah, I should know that any patch series I submitted from a year and a half ago probably made that mistake. I've mostly trained myself out of it now, but I certainly hadn't back then. Thanks for pointing it out; will fix.
NOTE: This series builds on sg/clean-nested-repo-with-ignored, as it (among other things) modifies his testcase from expect_failure to expect_success. Also, Peff is probably the only one who remembers v1 (and even he may have forgotten it): v1 was posted a year and a half ago. This patch series fixes a few issues with git-clean: * Failure to clean when multiple pathspecs are specified, reported both in April 2018[1] and again in May 2019[2]. * Failure to preserve both tracked and untracked files within a nested Git repository reported a few weeks ago by SZEDER[3]. [1] https://public-inbox.org/git/20180405173446.32372-4-newren@gmail.com/ [2] https://public-inbox.org/git/20190531183651.10067-1-rafa.almas@gmail.com/ [3] https://public-inbox.org/git/20190825185918.3909-1-szeder.dev@gmail.com/ I still never got answers to some questions in v1 of my RFC, so after considerable thought I eventually decided to: * Declare the existing documentation to be ambiguous and hard to interpret correctly; modified the documentation to clearly document 'correct behavior' with how different pieces interact. * Overrule four regression tests as having the wrong *expectation*, and modify them to have a correct one. That sounds like a backward compatibility issue BUT: The tests were written to check for issues that were orthogonal to the pieces that mattered in this series and thus couldn't be viewed as actually having an opinion on correct behavior on my issues; rather, they were simply reinforcing existing (buggy) implementation results. * Add a few tests which actually check relevant interactions of parameters and setup, to make this area less ambiguous. (Though one of them was added by SZEDER before my patches, and I should probably add a couple more tests...) Help from reviewers: The biggest area I need help from reviewers is to look at the commit messages for patches 9 and 10, to see if folks agree with my declaration of 'correct behavior' and my changes to the regression tests. If those are good, this series can proceed. If they aren't, and someone else can't provide an alternate easy-to-explain 'correct behavior' that we should implement and which is devoid of ugly edge cases for users, then this patch series may languish for another few years. Other notes: * Patches 1-6 were included in v1 and have almost no changes (just one fix pointed out by Peff). * Patch 6's commit message has some additional RFC-related comments and questions, one of which ties in with Patch 9. * Patch 7 was added as per (old) conversation with Peff. * Patch 9 & 10 are in most need of review (see above); each has lengthy commit messages. * It would be nice if someone knows whether the codepath edited in Patch 12 is dead code. If so, we could change that patch to just drop that if-check block. If it's not dead code, that patch fixes what is probably a rare but ugly bug. Elijah Newren (12): t7300: Add some testcases showing failure to clean specified pathspecs dir: fix typo in comment dir: fix off-by-one error in match_pathspec_item dir: Directories should be checked for matching pathspecs too dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule case dir: If our pathspec might match files under a dir, recurse into it dir: add commentary explaining match_pathspec_item's return value git-clean.txt: do not claim we will delete files with -n/--dry-run clean: disambiguate the definition of -d clean: avoid removing untracked files in a nested git repository clean: rewrap overly long line clean: fix theoretical path corruption Documentation/git-clean.txt | 16 +++++----- builtin/clean.c | 17 ++++++++-- dir.c | 63 +++++++++++++++++++++++++++---------- dir.h | 8 +++-- t/t7300-clean.sh | 44 +++++++++++++++++++++++--- 5 files changed, 114 insertions(+), 34 deletions(-)