| Message ID | 20240129113527.607022-5-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 693e807311b7c47168785dd32ff063b479ff2d8b |
| Headers | show |
| Series | for-each-ref: print all refs on empty string pattern | expand |
Hi Karthik On 29/01/2024 11:35, Karthik Nayak wrote: > When the user uses an empty string pattern (""), we don't match any refs > in git-for-each-ref(1). This is because in git-for-each-ref(1), we use > path based matching and an empty string doesn't match any path. > > In this commit we change this behavior by making empty string pattern > match all references. This is done by introducing a new flag > `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly > introduced `refs_for_each_all_refs()` function to iterate over all the > refs in the repository. It actually iterates over all the refs in the current worktree, not all the refs in the repository. I have to say that I find it extremely unintuitive that "" behaves differently to not giving a pattern. I wonder if we can find a better UI here - perhaps a command line option to include pseudorefs? > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > Documentation/git-for-each-ref.txt | 3 ++- > builtin/for-each-ref.c | 21 +++++++++++++++++- > ref-filter.c | 13 ++++++++++-- > ref-filter.h | 4 +++- > t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++ > 5 files changed, 70 insertions(+), 5 deletions(-) > > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > index be9543f684..b1cb482bf5 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -32,7 +32,8 @@ OPTIONS > If one or more patterns are given, only refs are shown that > match against at least one pattern, either using fnmatch(3) or > literally, in the latter case matching completely or from the > - beginning up to a slash. > + beginning up to a slash. If an empty string is provided all refs > + are printed, including HEAD and pseudorefs. I think it would be helpful to clarify that it is all the refs for the current worktree that are printed, not all the refs in the repository - we still don't have a way to iterate over the per-worktree refs from other worktrees Best Wishes Phillip
On Mon, Feb 05, 2024 at 06:48:52PM +0000, Phillip Wood wrote: > Hi Karthik > > On 29/01/2024 11:35, Karthik Nayak wrote: > > When the user uses an empty string pattern (""), we don't match any refs > > in git-for-each-ref(1). This is because in git-for-each-ref(1), we use > > path based matching and an empty string doesn't match any path. > > > > In this commit we change this behavior by making empty string pattern > > match all references. This is done by introducing a new flag > > `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly > > introduced `refs_for_each_all_refs()` function to iterate over all the > > refs in the repository. > > It actually iterates over all the refs in the current worktree, not all the > refs in the repository. I have to say that I find it extremely unintuitive > that "" behaves differently to not giving a pattern. I wonder if we can find > a better UI here - perhaps a command line option to include pseudorefs? I tend to agree, and have argued for a flag in another thread, too [1]. Something like `--show-all-refs` feels a lot more intuitive to me and also doesn't have the potential to break preexisting scripts which pass the empty prefix (which would have returned the empty set of refs, but now returns all refs). [1]: https://lore.kernel.org/git/ZZWCXFghtql4i4YE@tanuki/ Patrick
Hello, On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Karthik > > On 29/01/2024 11:35, Karthik Nayak wrote: > > When the user uses an empty string pattern (""), we don't match any refs > > in git-for-each-ref(1). This is because in git-for-each-ref(1), we use > > path based matching and an empty string doesn't match any path. > > > > In this commit we change this behavior by making empty string pattern > > match all references. This is done by introducing a new flag > > `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly > > introduced `refs_for_each_all_refs()` function to iterate over all the > > refs in the repository. > > It actually iterates over all the refs in the current worktree, not all > the refs in the repository. I have to say that I find it extremely > unintuitive that "" behaves differently to not giving a pattern. I > wonder if we can find a better UI here - perhaps a command line option > to include pseudorefs? > As Patrick mentioned, this was discussed a while ago and we decided to move forward with the `git for-each-ref ""` syntax. > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > > --- > > Documentation/git-for-each-ref.txt | 3 ++- > > builtin/for-each-ref.c | 21 +++++++++++++++++- > > ref-filter.c | 13 ++++++++++-- > > ref-filter.h | 4 +++- > > t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++ > > 5 files changed, 70 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > > index be9543f684..b1cb482bf5 100644 > > --- a/Documentation/git-for-each-ref.txt > > +++ b/Documentation/git-for-each-ref.txt > > @@ -32,7 +32,8 @@ OPTIONS > > If one or more patterns are given, only refs are shown that > > match against at least one pattern, either using fnmatch(3) or > > literally, in the latter case matching completely or from the > > - beginning up to a slash. > > + beginning up to a slash. If an empty string is provided all refs > > + are printed, including HEAD and pseudorefs. > > I think it would be helpful to clarify that it is all the refs for the > current worktree that are printed, not all the refs in the repository - > we still don't have a way to iterate over the per-worktree refs from > other worktrees > I agree, based on if we keep the current commits or remove them, I'll send in a new patch or amend the current series. Thanks!
Hi Patrick On 06/02/2024 05:33, Patrick Steinhardt wrote: > On Mon, Feb 05, 2024 at 06:48:52PM +0000, Phillip Wood wrote: >> Hi Karthik >> >> On 29/01/2024 11:35, Karthik Nayak wrote: >>> When the user uses an empty string pattern (""), we don't match any refs >>> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use >>> path based matching and an empty string doesn't match any path. >>> >>> In this commit we change this behavior by making empty string pattern >>> match all references. This is done by introducing a new flag >>> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly >>> introduced `refs_for_each_all_refs()` function to iterate over all the >>> refs in the repository. >> >> It actually iterates over all the refs in the current worktree, not all the >> refs in the repository. I have to say that I find it extremely unintuitive >> that "" behaves differently to not giving a pattern. I wonder if we can find >> a better UI here - perhaps a command line option to include pseudorefs? > > I tend to agree, and have argued for a flag in another thread, too [1]. Thanks for that, I'd missed that discussion > Something like `--show-all-refs` feels a lot more intuitive to me and > also doesn't have the potential to break preexisting scripts which pass > the empty prefix (which would have returned the empty set of refs, but > now returns all refs). Yes, and as you point out in that other thread flag would allow the pseuderefs to be filtered and allow us to show the refs for all worktrees as well in the future. Best Wishes Phillip > [1]: https://lore.kernel.org/git/ZZWCXFghtql4i4YE@tanuki/ > > Patrick
Hi Karthik On 06/02/2024 08:52, Karthik Nayak wrote: > On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote: >> On 29/01/2024 11:35, Karthik Nayak wrote: >>> When the user uses an empty string pattern (""), we don't match any refs >>> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use >>> path based matching and an empty string doesn't match any path. >>> >>> In this commit we change this behavior by making empty string pattern >>> match all references. This is done by introducing a new flag >>> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly >>> introduced `refs_for_each_all_refs()` function to iterate over all the >>> refs in the repository. >> >> It actually iterates over all the refs in the current worktree, not all >> the refs in the repository. I have to say that I find it extremely >> unintuitive that "" behaves differently to not giving a pattern. I >> wonder if we can find a better UI here - perhaps a command line option >> to include pseudorefs? >> > > As Patrick mentioned, this was discussed a while ago and we decided to > move forward with the `git for-each-ref ""` syntax. Thanks I'd missed that discussion. I see that at the end of that discussion Junio was concerned that the proposed "" did not account for "refs/worktrees/$worktree/*" [1] - how has that been resolved? Best Wishes Phillip [1] https://lore.kernel.org/git/xmqq1qawr6p4.fsf@gitster.g/ >>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> >>> --- >>> Documentation/git-for-each-ref.txt | 3 ++- >>> builtin/for-each-ref.c | 21 +++++++++++++++++- >>> ref-filter.c | 13 ++++++++++-- >>> ref-filter.h | 4 +++- >>> t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++ >>> 5 files changed, 70 insertions(+), 5 deletions(-) >>> >>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt >>> index be9543f684..b1cb482bf5 100644 >>> --- a/Documentation/git-for-each-ref.txt >>> +++ b/Documentation/git-for-each-ref.txt >>> @@ -32,7 +32,8 @@ OPTIONS >>> If one or more patterns are given, only refs are shown that >>> match against at least one pattern, either using fnmatch(3) or >>> literally, in the latter case matching completely or from the >>> - beginning up to a slash. >>> + beginning up to a slash. If an empty string is provided all refs >>> + are printed, including HEAD and pseudorefs. >> >> I think it would be helpful to clarify that it is all the refs for the >> current worktree that are printed, not all the refs in the repository - >> we still don't have a way to iterate over the per-worktree refs from >> other worktrees >> > > I agree, based on if we keep the current commits or remove them, I'll > send in a new patch or amend the current series. > > Thanks!
Hello, On Tue, Feb 6, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Karthik > > On 06/02/2024 08:52, Karthik Nayak wrote: > > On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > >> On 29/01/2024 11:35, Karthik Nayak wrote: > >>> When the user uses an empty string pattern (""), we don't match any refs > >>> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use > >>> path based matching and an empty string doesn't match any path. > >>> > >>> In this commit we change this behavior by making empty string pattern > >>> match all references. This is done by introducing a new flag > >>> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly > >>> introduced `refs_for_each_all_refs()` function to iterate over all the > >>> refs in the repository. > >> > >> It actually iterates over all the refs in the current worktree, not all > >> the refs in the repository. I have to say that I find it extremely > >> unintuitive that "" behaves differently to not giving a pattern. I > >> wonder if we can find a better UI here - perhaps a command line option > >> to include pseudorefs? > >> > > > > As Patrick mentioned, this was discussed a while ago and we decided to > > move forward with the `git for-each-ref ""` syntax. > > Thanks I'd missed that discussion. I see that at the end of that > discussion Junio was concerned that the proposed "" did not account for > "refs/worktrees/$worktree/*" [1] - how has that been resolved? > The current implementation (merged to next) prints all the refs from the current worktree and doesn't support printing from other worktrees. $ git worktree add ../wt-files @~10 Preparing worktree (detached HEAD 559d5138bc) HEAD is now at 559d5138bc Merge branch 'jc/make-libpath-template' into next $ cd ../wt-files/ direnv: unloading $ git for-each-ref "" | grep -v "refs" 559d5138bc8b81354fd1bc3421ace614076e66de commit HEAD 559d5138bc8b81354fd1bc3421ace614076e66de commit ORIG_HEAD $ git rebase -i @~3 Stopped at be65f9ef88... t/Makefile: get UNIT_TESTS list from C sources You can amend the commit now, with git commit --amend '-S' Once you are satisfied with your changes, run git rebase --continue $ git for-each-ref "" | grep -v "refs" be65f9ef88ff741454dcf10a0af6e384d0bf26cf commit HEAD 43f081b9c7dfb9c23e547ffee1778af0f30c5c4e commit ORIG_HEAD be65f9ef88ff741454dcf10a0af6e384d0bf26cf commit REBASE_HEAD
Phillip Wood <phillip.wood123@gmail.com> writes: > Thanks I'd missed that discussion. I see that at the end of that > discussion Junio was concerned that the proposed "" did not account > for "refs/worktrees/$worktree/*" [1] - how has that been resolved? Ah, that is an excellent point. If we plan to never allow showing refs/worktrees/ hierarchy, then the "there is a default pattern, refs/, that gets used when there is no user-specified patterns" model would be sufficient to allow showing things that are directly beneath $GIT_DIR and are out of refs/ hierarchy, but that does not explain why refs/worktrees/ is omitted. I'll envision a design for a longer term later, but an easy way out would be to add --include-worktree-refs option for that, and at that point, adding --include-root-refs option for things outside the refs/ hierarchy may become a lot more natural solution. In the longer term, I suspect that we would want something similar to the negative refspec magic (e.g., "git log ':!Documentation/'" that shows things outside the named hierarchy) exposed to the API[*], so that we can say $ git for-each-ref --format=... \ refs/ !refs/heads/ !refs/tags/ !refs/remotes/ to show things under refs/ excluding the commonly used hierarchies, and at that point, the current behaviour for "no limit" case can again become explainable as having "refs/ !refs/worktrees/" as the default. It still does not explain why "git for-each-ref refs/" omits the refs/worktrees/ hierchy, unless the default limit pattern rule were something like "unless you give a positive limit pattern rule, then we use 'refs/' by default, and unless you give a negative limit pattern rule, then we use '!refs/worktrees/' by default". It then gives an easy explanation on the traditional behaviour, with "" used for "including stuff outside refs/", and is more flexible. The use of dashed-options to include hierachies that are by default excluded (e.g. "--include-root-refs" and "--include-worktree-refs") feels limiting, but should be sufficient for our needs, both current (i.e. I want to see HEAD and FETCH_HEAD) and in the immediate future (i.e. I want to see worktree refs from that worktree), and I can buy that as a good alternative solution, at least in the shorter term. I still worry that it may make introducing the negative ref patterns harder, though. How does --include-worktree-refs=another to include the worktree refs from another worktree in refs/worktrees/another interact with a negative pattern that was given from the command line that overlaps with it? Whatever interaction rules we define, can we easily explain it in the documentation? Just like "an empty string tells Git to include everything" is a perfectly reasonable approach if we plan to never allow refs/worktrees/ hierarchy, "dashed-options for selected hierarchies" is a perfectly reasonable approach if we plan to never allow negative limit patterns, I suspect. We should stop complexity at some point, and the decision to never support negative limit patterns might be the place to draw that line. I dunno. [Footnote] * Such an exclusion mechanism already exists and are used to hide certain refs from being seen over the network by "git fetch" and friends. I do not think it is plugged to the machinery used by for-each-ref and friends, but it smells like a reasonably easy thing to do.
Junio C Hamano <gitster@pobox.com> writes: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> Thanks I'd missed that discussion. I see that at the end of that >> discussion Junio was concerned that the proposed "" did not account >> for "refs/worktrees/$worktree/*" [1] - how has that been resolved? > > Ah, that is an excellent point. > ... > The use of dashed-options to include hierachies that are by default > excluded (e.g. "--include-root-refs" and "--include-worktree-refs") > feels limiting, but should be sufficient for our needs, both current > (i.e. I want to see HEAD and FETCH_HEAD) and in the immediate future > (i.e. I want to see worktree refs from that worktree), and I can buy > that as a good alternative solution, at least in the shorter term. > > I still worry that it may make introducing the negative ref patterns > harder, though. How does --include-worktree-refs=another to include > the worktree refs from another worktree in refs/worktrees/another > interact with a negative pattern that was given from the command > line that overlaps with it? Whatever interaction rules we define, > can we easily explain it in the documentation? > > Just like "an empty string tells Git to include everything" is a > perfectly reasonable approach if we plan to never allow > refs/worktrees/ hierarchy, "dashed-options for selected hierarchies" > is a perfectly reasonable approach if we plan to never allow > negative limit patterns, I suspect. We should stop complexity at > some point, and the decision to never support negative limit > patterns might be the place to draw that line. I dunno. For now, let's block the kn/for-all-refs topic until we figure out the UI issue. Which means this (and the reftable integration that started to depend on it) will not be in the upcoming release. FWIW, I am leaning towards "a set of narrowly targetted command line options (like "--include-root-refs")" approach over "a empty string defeats the built-in default of 'refs/' limit". Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Phillip Wood <phillip.wood123@gmail.com> writes: >> >>> Thanks I'd missed that discussion. I see that at the end of that >>> discussion Junio was concerned that the proposed "" did not account >>> for "refs/worktrees/$worktree/*" [1] - how has that been resolved? >> >> Ah, that is an excellent point. >> ... >> The use of dashed-options to include hierachies that are by default >> excluded (e.g. "--include-root-refs" and "--include-worktree-refs") >> feels limiting, but should be sufficient for our needs, both current >> (i.e. I want to see HEAD and FETCH_HEAD) and in the immediate future >> (i.e. I want to see worktree refs from that worktree), and I can buy >> that as a good alternative solution, at least in the shorter term. >> >> I still worry that it may make introducing the negative ref patterns >> harder, though. How does --include-worktree-refs=another to include >> the worktree refs from another worktree in refs/worktrees/another >> interact with a negative pattern that was given from the command >> line that overlaps with it? Whatever interaction rules we define, >> can we easily explain it in the documentation? >> >> Just like "an empty string tells Git to include everything" is a >> perfectly reasonable approach if we plan to never allow >> refs/worktrees/ hierarchy, "dashed-options for selected hierarchies" >> is a perfectly reasonable approach if we plan to never allow >> negative limit patterns, I suspect. We should stop complexity at >> some point, and the decision to never support negative limit >> patterns might be the place to draw that line. I dunno. > > For now, let's block the kn/for-all-refs topic until we figure out > the UI issue. Which means this (and the reftable integration that > started to depend on it) will not be in the upcoming release. > I think it makes sense to remove the kn/for-all-refs topic for now. I also think that the reftable integration branch can go forward though, since the difference between v2 and v3 of that series was simply that v3 was rebased on top of kn/for-all-refs. So we can continue using v2. > FWIW, I am leaning towards "a set of narrowly targetted command line > options (like "--include-root-refs")" approach over "a empty string > defeats the built-in default of 'refs/' limit". I think this was what the earlier discussion in the RFC series was too, but Phillip definitely helped consolidate the point. I'll send a new version of this patch series with `--include-root-refs` option and we can discuss on top of that. Thanks all!
Karthik Nayak <karthik.188@gmail.com> writes: > I think this was what the earlier discussion in the RFC series was too, > but Phillip definitely helped consolidate the point. > > I'll send a new version of this patch series with `--include-root-refs` > option and we can discuss on top of that. Thanks. By the way, I am not married to the "root refs" name, but - we do not want to say "all refs", as I expect refs from other worktrees are not included, and possibly the option when combined with explicit patterns, like refs/tags/, may further be used to limit the output; - we do not want to say "pseudo refs", as I expect we would want to show HEAD that is (unfortunately) classified outside "pseudoref" class.
On Tue, Feb 06, 2024 at 02:10:41PM -0800, Karthik Nayak wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Junio C Hamano <gitster@pobox.com> writes: > >> Phillip Wood <phillip.wood123@gmail.com> writes: [snip] > > For now, let's block the kn/for-all-refs topic until we figure out > > the UI issue. Which means this (and the reftable integration that > > started to depend on it) will not be in the upcoming release. > > > > I think it makes sense to remove the kn/for-all-refs topic for now. > > I also think that the reftable integration branch can go forward though, > since the difference between v2 and v3 of that series was simply that > v3 was rebased on top of kn/for-all-refs. So we can continue using v2. I've sent a rebased v4 that evicted the kn/for-all-refs dependency. This also allowed me to adapt to some fixes for the reftable library which have been merged down to `master` now so that the corresponding tests are now with `test_expect_success`. Patrick
On Tue, Feb 6, 2024 at 11:16 PM Junio C Hamano <gitster@pobox.com> wrote: > > Karthik Nayak <karthik.188@gmail.com> writes: > > > I think this was what the earlier discussion in the RFC series was too, > > but Phillip definitely helped consolidate the point. > > > > I'll send a new version of this patch series with `--include-root-refs` > > option and we can discuss on top of that. > > Thanks. > > By the way, I am not married to the "root refs" name, but > > - we do not want to say "all refs", as I expect refs from other > worktrees are not included, and possibly the option when > combined with explicit patterns, like refs/tags/, may further be > used to limit the output; > > - we do not want to say "pseudo refs", as I expect we would want to > show HEAD that is (unfortunately) classified outside "pseudoref" > class. > I'm thinking "--all-ref-types" might be a good alternative. Mostly because, "--include-root-refs" seems very specific to the files backend. Also, we don't include other refs which are not HEAD | pseudorefs, but in the $GIT_DIR.
Karthik Nayak <karthik.188@gmail.com> writes: > I'm thinking "--all-ref-types" might be a good alternative. Mostly because, > "--include-root-refs" seems very specific to the files backend. Also, we don't > include other refs which are not HEAD | pseudorefs, but in the $GIT_DIR. I strongly disagree wiht the "files backend specific" part of the comment. No matter what backend you would use, refs and pseudorefs have the full refname, which may look like "HEAD", "FETCH_HEAD", "refs/heads/maint", etc., and you can easily see these full refnames form a tree structure, with "HEAD", "FETCH_HEAD", "refs/" at the root level. I do not understand your "we don't include other refs", either. There may be "things" that are ignored by your implementation of "for-each-ref ''" even with the files backend in $GIT_DIR directory. They are not refs, because the refs are by definition inside "refs/" hierarchy, unless they are ones that are specifically included from outside the hierarchy ("pseudorefs" is one class of specific exception, "HEAD" is another).
Patrick Steinhardt <ps@pks.im> writes: >> I also think that the reftable integration branch can go forward though, >> since the difference between v2 and v3 of that series was simply that >> v3 was rebased on top of kn/for-all-refs. So we can continue using v2. > > I've sent a rebased v4 that evicted the kn/for-all-refs dependency. This > also allowed me to adapt to some fixes for the reftable library which > have been merged down to `master` now so that the corresponding tests > are now with `test_expect_success`. Thanks, will take a look.
On Wed, Feb 7, 2024 at 5:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > Karthik Nayak <karthik.188@gmail.com> writes: > > > I'm thinking "--all-ref-types" might be a good alternative. Mostly because, > > "--include-root-refs" seems very specific to the files backend. Also, we don't > > include other refs which are not HEAD | pseudorefs, but in the $GIT_DIR. > > I strongly disagree wiht the "files backend specific" part of the > comment. No matter what backend you would use, refs and pseudorefs > have the full refname, which may look like "HEAD", "FETCH_HEAD", > "refs/heads/maint", etc., and you can easily see these full refnames > form a tree structure, with "HEAD", "FETCH_HEAD", "refs/" at the > root level. I conceded to this point, I was thinking "root" here refers to $GIT_DIR and this structuring comes from the files backend. But I see the flaw there that irrelevant of the backend, there is a tree hierarchy built up and for refs without prefixes, it can be considered as "root". > I do not understand your "we don't include other refs", either. > There may be "things" that are ignored by your implementation of > "for-each-ref ''" even with the files backend in $GIT_DIR directory. > They are not refs, because the refs are by definition inside "refs/" > hierarchy, unless they are ones that are specifically included from > outside the hierarchy ("pseudorefs" is one class of specific > exception, "HEAD" is another). This is a bit of a grey area, what I mean is that we do allow users to create non "refs/" prefixed refs: $ git update-ref foo @~1 $ cat .git/foo 2b52187cd2930931c6d34436371f470bb26eef4f What I mean to say is that, by saying "--include-root-refs" it seems to imply that any such refs should be included too, but this simply is not the case.
Karthik Nayak <karthik.188@gmail.com> writes: > This is a bit of a grey area, what I mean is that we do allow users to create > non "refs/" prefixed refs: > > $ git update-ref foo @~1 > > $ cat .git/foo > 2b52187cd2930931c6d34436371f470bb26eef4f > > What I mean to say is that, by saying "--include-root-refs" it seems to imply > that any such refs should be included too, but this simply is not the case. But isn't that a quality of implementation issue? I'd consider it a bug once we have and enforce the definition of what pseudorefs are.
On Wed, Feb 7, 2024 at 5:46 PM Junio C Hamano <gitster@pobox.com> wrote: > > Karthik Nayak <karthik.188@gmail.com> writes: > > > This is a bit of a grey area, what I mean is that we do allow users to create > > non "refs/" prefixed refs: > > > > $ git update-ref foo @~1 > > > > $ cat .git/foo > > 2b52187cd2930931c6d34436371f470bb26eef4f > > > > What I mean to say is that, by saying "--include-root-refs" it seems to imply > > that any such refs should be included too, but this simply is not the case. > > But isn't that a quality of implementation issue? I'd consider it a > bug once we have and enforce the definition of what pseudorefs are. Yeah, that makes sense. I'll use "--include-root-refs" then. Thanks
On Wed, Feb 07, 2024 at 06:02:34PM +0100, Karthik Nayak wrote: > On Wed, Feb 7, 2024 at 5:46 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > Karthik Nayak <karthik.188@gmail.com> writes: > > > > > This is a bit of a grey area, what I mean is that we do allow users to create > > > non "refs/" prefixed refs: > > > > > > $ git update-ref foo @~1 > > > > > > $ cat .git/foo > > > 2b52187cd2930931c6d34436371f470bb26eef4f > > > > > > What I mean to say is that, by saying "--include-root-refs" it seems to imply > > > that any such refs should be included too, but this simply is not the case. > > > > But isn't that a quality of implementation issue? I'd consider it a > > bug once we have and enforce the definition of what pseudorefs are. > > Yeah, that makes sense. I'll use "--include-root-refs" then. I'd still argue that this is too specific to the files backend. The fact that we end up only adding root refs to the files backend to me is an implementation-specific limitation and not a feature. What I really want is to ask the backend to give me all refs that it knows of for the current worktree. The "reftable" backend has this snippet in its iterator: ``` /* * The files backend only lists references contained in * "refs/". We emulate the same behaviour here and thus skip * all references that don't start with this prefix. */ if (!starts_with(iter->ref.refname, "refs/")) continue; ``` If we add the new `--include-root-refs` flag then we would have to extend it to query whether the ref either starts with "refs/" or whether it might look like a pseudo-ref: ``` if (!starts_with(iter->ref.refname, "refs/") && !(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname))) continue; ``` The problem I have is that it still wouldn't end up surfacing all refs which exist in the ref backend while being computationally more expensive. So the original usecase I had in mind when pitching this topic isn't actually addressed. I know that in theory, the reftable backend shouldn't contain refs other than "refs/" or pseudo-refs anyway. But regardless of that, I think that formulating this in the form of "root refs" is too restrictive and too much focussed on the "files" backend. I wouldn't be surprised if there are ways to have git-update-ref(1) or other tools write refs with unexpected names, and not being able to learn about those is a limitation. Putting this in the context of "Give me all refs which you know of for the current worktree" is a lot simpler. It would still be equivalent to "--include-root-refs" for the "files" backend, because the files backend doesn't have a way to properly track all refs it has ever created. And for the "reftable" backend it's as simple as this: ``` if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) && !starts_with(iter->ref.refname, "refs/")) ``` I'm not sure about better terminology though. Something like `--include-all-worktree-refs` could easily lead to confusion because it might make the user think that they also want to list refs from other worktrees. The best I can come up with is `--include-non-refs` -- pseudo refs aren't real refs, and neither are refs which don't start with "refs/". Patrick
Hi Juino On 06/02/2024 22:16, Junio C Hamano wrote: > - we do not want to say "pseudo refs", as I expect we would want to > show HEAD that is (unfortunately) classified outside "pseudoref" > class. This is a bit of a tangent but I've been wondering what the practical benefit of distinguishing between "HEAD" and "pseudoref" is. I don't know the history so there may be a good reason but not classifying "HEAD" as a "pseudoref" seems to make discussions like this more complicated than they would be if "HEAD" were a "pseudoref". Would it be beneficial to loosen the definition of "psuedoref" to remove the restriction that they may not be symbolic refs or have a reflog? Best Wishes Phillip
Patrick Steinhardt <ps@pks.im> writes: > ``` > if (!starts_with(iter->ref.refname, "refs/") && > !(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname))) > continue; > ``` > > The problem I have is that it still wouldn't end up surfacing all refs > which exist in the ref backend while being computationally more > expensive. So the original usecase I had in mind when pitching this > topic isn't actually addressed. The reftable format, as a database format, may be capable of having "refs/heads/master" and "refs/heads/master/1" at the same time, but to be used as a ref backend for Git, it must refrain from surfacing both at the same time. I think it is the same deal that it should only allow "refs/*", "HEAD", and so called pseudorefs to be stored. So INCLUDE_ROOT_REFS should be sufficient as long as the "ref creation and update" side is not letting random cruft (e.g., "config") in. Isn't that sufficient? > I know that in theory, the reftable backend shouldn't contain refs other > than "refs/" or pseudo-refs anyway. But regardless of that, I think that > formulating this in the form of "root refs" is too restrictive and too > much focussed on the "files" backend. It is not "focused on". The ref namespace of Git is tree-shaped, period. The shape may have originated from its first ref backend implementation's limitation, but as we gain other backends, we are not planning to lift such limitations, are we? So we may still say "when there is a master branch, you cannot have master/1 branch (due to D/F conflict)", even if there is no notion of directory or file in a backend implementation backed by a databasy file format. "HEAD" and "CHERRY_PICK_HEAD", unlike "refs/tags/v1.0", are at the "root level", not only when they are stored in a files backend, but always when they are presented to end-users, who can tell that they are not inside "refs/".
Phillip Wood <phillip.wood123@gmail.com> writes: > This is a bit of a tangent but I've been wondering what the practical > benefit of distinguishing between "HEAD" and "pseudoref" is. I hate that distinction too. The practical difference they gave me was that we historically never made FOO_HEAD to be a symref, and we do not want them to be in the future, but for HEAD, it is perfectly natural to be a symref.
On Thu, Feb 08, 2024 at 09:04:54AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > ``` > > if (!starts_with(iter->ref.refname, "refs/") && > > !(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname))) > > continue; > > ``` > > > > The problem I have is that it still wouldn't end up surfacing all refs > > which exist in the ref backend while being computationally more > > expensive. So the original usecase I had in mind when pitching this > > topic isn't actually addressed. > > The reftable format, as a database format, may be capable of having > "refs/heads/master" and "refs/heads/master/1" at the same time, but > to be used as a ref backend for Git, it must refrain from surfacing > both at the same time. I think it is the same deal that it should > only allow "refs/*", "HEAD", and so called pseudorefs to be stored. > So INCLUDE_ROOT_REFS should be sufficient as long as the "ref > creation and update" side is not letting random cruft (e.g., > "config") in. Isn't that sufficient? That's a different problem from the one I have right now. Let's take the following sequence of commands: $ git init repo Initialized empty Git repository in /tmp/repo/.git/ $ git -C repo commit --allow-empty --message message [main (root-commit) aa5eec4] message $ git -C repo update-ref ref/head/foo HEAD $ ls repo/.git/ref/head/foo repo/.git/ref/head/foo Now the fact that you can create "ref/head/foo" is a bug that needs to be fixed, no arguing there. The problem is that rectifying this problem with the "files" backend is easy -- you look into the repo, notice that there's a weird directory, and then "rm -rf" it. But how do you learn about this ref existing with the "reftable" backend in the first place? You can't without looking at the binary format -- there doesn't exist a single command that would allow you to list all refs unfiltered. But that is very much required in order to learn about misbehaviour and fix it. As I said -- this is a bug, and I agree that it shouldn't happen. But bugs happen, and especially with the new reftable format I expect them to happen. What I look for in this context is to create the tools to fix problems like this, but `--include-root-refs` doesn't. A flag that unconditionally returns all refs, regardless of whether they have a bad name or not, does address the issue. Think of it of more of a debugging tool. Spelled out like that it brings me a different idea: maybe I'm just trying to address this in the wrong tool. I plan to introduce ref backend specific fsck checks, so that could be a better place to warn about such refs with bad names. Like this we don't erode the tree-shaped nature by somehow accepting them in some tools, and we make clear that this is indeed something that shouldn't happen. > > I know that in theory, the reftable backend shouldn't contain refs other > > than "refs/" or pseudo-refs anyway. But regardless of that, I think that > > formulating this in the form of "root refs" is too restrictive and too > > much focussed on the "files" backend. > > It is not "focused on". The ref namespace of Git is tree-shaped, > period. The shape may have originated from its first ref backend > implementation's limitation, but as we gain other backends, we are > not planning to lift such limitations, are we? So we may still say > "when there is a master branch, you cannot have master/1 branch (due > to D/F conflict)", even if there is no notion of directory or file > in a backend implementation backed by a databasy file format. "HEAD" > and "CHERRY_PICK_HEAD", unlike "refs/tags/v1.0", are at the "root > level", not only when they are stored in a files backend, but always > when they are presented to end-users, who can tell that they are not > inside "refs/". I agree, and I do not intend to change this. Patrick
Patrick Steinhardt <ps@pks.im> writes: > That's a different problem from the one I have right now. Let's take the > following sequence of commands: > > $ git init repo > Initialized empty Git repository in /tmp/repo/.git/ > $ git -C repo commit --allow-empty --message message > [main (root-commit) aa5eec4] message > $ git -C repo update-ref ref/head/foo HEAD > $ ls repo/.git/ref/head/foo > repo/.git/ref/head/foo > > Now the fact that you can create "ref/head/foo" is a bug that needs to > be fixed, no arguing there. The problem is that rectifying this problem > with the "files" backend is easy -- you look into the repo, notice that > there's a weird directory, and then "rm -rf" it. OK. > But how do you learn about this ref existing with the "reftable" backend > in the first place? You can't without looking at the binary format -- > there doesn't exist a single command that would allow you to list all > refs unfiltered. But that is very much required in order to learn about > misbehaviour and fix it. I think I have been saying that it is perfectly OK if reftable backend, being newer and backed by more experience using Git, rejected any attempt to create anything under "ref/" (to avoid confusion to those who are reading from sidelines, it should allow creating "refs/mytool/" for third-party tools to store their own pointers). > As I said -- this is a bug, and I agree that it shouldn't happen. But > bugs happen, and especially with the new reftable format I expect them > to happen. What I look for in this context is to create the tools to fix > problems like this, but `--include-root-refs` doesn't. A flag that > unconditionally returns all refs, regardless of whether they have a bad > name or not, does address the issue. Think of it of more of a debugging > tool. OK, "--include-all-refs" would be fine. And without bugs there should not be a difference. Where does "all refs in this worktree" you mentioned fit in this picture? Should a bogus "ref/foo/bar" be considered to be worktree specific, or is it an incorrect attempt to create a ref that is specific to 'foo' worktree that is not the current one and be filtered out?
On Thu, Feb 08, 2024 at 09:53:24AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: [snip] > > As I said -- this is a bug, and I agree that it shouldn't happen. But > > bugs happen, and especially with the new reftable format I expect them > > to happen. What I look for in this context is to create the tools to fix > > problems like this, but `--include-root-refs` doesn't. A flag that > > unconditionally returns all refs, regardless of whether they have a bad > > name or not, does address the issue. Think of it of more of a debugging > > tool. > > OK, "--include-all-refs" would be fine. And without bugs there > should not be a difference. > > Where does "all refs in this worktree" you mentioned fit in this > picture? Should a bogus "ref/foo/bar" be considered to be worktree > specific, or is it an incorrect attempt to create a ref that is > specific to 'foo' worktree that is not the current one and be > filtered out? Good question indeed. The reason I said "all refs in this worktree" is mostly that I don't think we should also be returning refs from other worktrees. I consider their refdbs to be separate refdbs, even though it is possible to access them via the special "worktrees/$wt/refs" syntax. So I would say that such an option should return all refs of the current worktree's refdb, plus the common worktree refs. Another important question here is how the "files" backend should behave with such a flag. Should it try to read all loose files as refs and return those which just happen to look like one? That feels really wrong to me, as we would now have to special case those namespaces where we know that it's not a proper ref, like "worktrees/", "rebase-apply/" and others. The alternative would be to make it behave like `--include-root-refs`, where we do a best effort and live with the fact that the "files" backend cannot enumerate all refs it has created. This would mean that behaviour between the "files" and "reftable" backend is diverging though because the latter can trivially figure out all refs it contains. The question is whether we are fine with that or not. Depending on the answer, I think we can go one of two ways: - Accept the diverging behaviour and add `--include-all-refs`. The "files" backend does a best effort and only includes root refs, the "reftable" backend lists all refs. - Double down on the fact that refs must either be pseudo refs or start with "refs/" and treat any ref that doesn't fit this bill as corrupted. The consequence here would be that we instead go with `--include-root-refs` that can be implemented the same for both backends. In addition, we add checks to git-fsck(1) to surface and flag refs with bogus names for the "reftable" backend. While I seem to have convinced you that `--include-all-refs` might not be a bad idea after all, you have convinced me by now that the second option would be preferable. I'd be okay with either of these options as both of them address the issue at hand. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Depending on the answer, I think we can go one of two ways: > > - Accept the diverging behaviour and add `--include-all-refs`. The > "files" backend does a best effort and only includes root refs, the > "reftable" backend lists all refs. > > - Double down on the fact that refs must either be pseudo refs or > start with "refs/" and treat any ref that doesn't fit this bill as > corrupted. The consequence here would be that we instead go with > `--include-root-refs` that can be implemented the same for both > backends. In addition, we add checks to git-fsck(1) to surface and > flag refs with bogus names for the "reftable" backend. > > While I seem to have convinced you that `--include-all-refs` might not > be a bad idea after all, you have convinced me by now that the second > option would be preferable. I'd be okay with either of these options as > both of them address the issue at hand. For now my tentative preference is the latter. If ref/head/foo is an end-user mistake with one backend, it is cleaner if it is considered a mistake with other backends. Doesn't our ref enumeration/iteration API have "include broken ones as well" bit? I wonder if this issue becomes easier to solve by (re|ab)using that bit.
Junio C Hamano <gitster@pobox.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >> Depending on the answer, I think we can go one of two ways: >> >> - Accept the diverging behaviour and add `--include-all-refs`. The >> "files" backend does a best effort and only includes root refs, the >> "reftable" backend lists all refs. >> >> - Double down on the fact that refs must either be pseudo refs or >> start with "refs/" and treat any ref that doesn't fit this bill as >> corrupted. The consequence here would be that we instead go with >> `--include-root-refs` that can be implemented the same for both >> backends. In addition, we add checks to git-fsck(1) to surface and >> flag refs with bogus names for the "reftable" backend. >> >> While I seem to have convinced you that `--include-all-refs` might not >> be a bad idea after all, you have convinced me by now that the second >> option would be preferable. I'd be okay with either of these options as >> both of them address the issue at hand. > > For now my tentative preference is the latter. If ref/head/foo is > an end-user mistake with one backend, it is cleaner if it is > considered a mistake with other backends. > > Doesn't our ref enumeration/iteration API have "include broken ones > as well" bit? I wonder if this issue becomes easier to solve by > (re|ab)using that bit. I'll then go ahead with point 2 then. I'll modify my patch series for now to fit in and will follow up "checks to git-fsck(1) to surface and flag refs with bogus names for the "reftable" backend" in a follow up series. - Karthik
On Fri, Feb 09, 2024 at 06:27:39PM +0000, Karthik Nayak wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Patrick Steinhardt <ps@pks.im> writes: > >> Depending on the answer, I think we can go one of two ways: > >> > >> - Accept the diverging behaviour and add `--include-all-refs`. The > >> "files" backend does a best effort and only includes root refs, the > >> "reftable" backend lists all refs. > >> > >> - Double down on the fact that refs must either be pseudo refs or > >> start with "refs/" and treat any ref that doesn't fit this bill as > >> corrupted. The consequence here would be that we instead go with > >> `--include-root-refs` that can be implemented the same for both > >> backends. In addition, we add checks to git-fsck(1) to surface and > >> flag refs with bogus names for the "reftable" backend. > >> > >> While I seem to have convinced you that `--include-all-refs` might not > >> be a bad idea after all, you have convinced me by now that the second > >> option would be preferable. I'd be okay with either of these options as > >> both of them address the issue at hand. > > > > For now my tentative preference is the latter. If ref/head/foo is > > an end-user mistake with one backend, it is cleaner if it is > > considered a mistake with other backends. > > > > Doesn't our ref enumeration/iteration API have "include broken ones > > as well" bit? I wonder if this issue becomes easier to solve by > > (re|ab)using that bit. > > I'll then go ahead with point 2 then. > > I'll modify my patch series for now to fit in and will follow up "checks > to git-fsck(1) to surface and flag refs with bogus names for the > "reftable" backend" in a follow up series. Thanks. Note that the fsck checks are also proposed as one of the GSoC projects where you're listed as a mentor. Might be worth it to hold back until we know whether any student wants to work on it. Patrick
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index be9543f684..b1cb482bf5 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -32,7 +32,8 @@ OPTIONS If one or more patterns are given, only refs are shown that match against at least one pattern, either using fnmatch(3) or literally, in the latter case matching completely or from the - beginning up to a slash. + beginning up to a slash. If an empty string is provided all refs + are printed, including HEAD and pseudorefs. --stdin:: If `--stdin` is supplied, then the list of patterns is read from diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 3885a9c28e..5aa879e8be 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -25,6 +25,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) struct ref_format format = REF_FORMAT_INIT; int from_stdin = 0; struct strvec vec = STRVEC_INIT; + unsigned int flags = FILTER_REFS_ALL; struct option opts[] = { OPT_BIT('s', "shell", &format.quote_style, @@ -93,11 +94,29 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) /* vec.v is NULL-terminated, just like 'argv'. */ filter.name_patterns = vec.v; } else { + size_t i; + filter.name_patterns = argv; + + /* + * Search for any empty string pattern, if it exists then we + * print all refs without any filtering. + */ + i = 0; + while (argv[i]) { + if (!argv[i][0]) { + flags = FILTER_REFS_NO_FILTER; + /* doing this removes any pattern from being matched */ + filter.name_patterns[0] = NULL; + break; + } + + i++; + } } filter.match_as_path = 1; - filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format); + filter_and_format_refs(&filter, flags, sorting, &format); ref_filter_clear(&filter); ref_sorting_release(sorting); diff --git a/ref-filter.c b/ref-filter.c index 35b989e1df..6dac133b87 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2622,6 +2622,11 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter, each_ref_fn cb, void *cb_data) { + if (filter->kind & FILTER_REFS_NO_FILTER) { + return refs_for_each_all_refs( + get_main_ref_store(the_repository), cb, cb_data); + } + if (!filter->match_as_path) { /* * in this case, the patterns are applied after @@ -2775,8 +2780,12 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct /* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */ kind = filter_ref_kind(filter, refname); - if (!(kind & filter->kind)) + if (filter->kind & FILTER_REFS_NO_FILTER) { + if (kind == FILTER_REFS_DETACHED_HEAD) + kind = FILTER_REFS_OTHERS; + } else if (!(kind & filter->kind)) { return NULL; + } if (!filter_pattern_match(filter, refname)) return NULL; @@ -3041,7 +3050,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref ret = for_each_fullref_in("refs/remotes/", fn, cb_data); else if (filter->kind == FILTER_REFS_TAGS) ret = for_each_fullref_in("refs/tags/", fn, cb_data); - else if (filter->kind & FILTER_REFS_ALL) + else if (filter->kind & FILTER_REFS_ALL || filter->kind & FILTER_REFS_NO_FILTER) ret = for_each_fullref_in_pattern(filter, fn, cb_data); if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD)) head_ref(fn, cb_data); diff --git a/ref-filter.h b/ref-filter.h index 07cd6f6da3..1eab325ce0 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -22,7 +22,9 @@ #define FILTER_REFS_ALL (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \ FILTER_REFS_REMOTES | FILTER_REFS_OTHERS) #define FILTER_REFS_DETACHED_HEAD 0x0020 -#define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD) +#define FILTER_REFS_NO_FILTER 0x0040 +#define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD | \ + FILTER_REFS_NO_FILTER) struct atom_value; struct ref_sorting; diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index 82f3d1ea0f..3922326cab 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -31,6 +31,40 @@ test_expect_success 'setup some history and refs' ' git update-ref refs/odd/spot main ' +cat >expect <<-\EOF + HEAD + ORIG_HEAD + refs/heads/main + refs/heads/side + refs/odd/spot + refs/tags/annotated-tag + refs/tags/doubly-annotated-tag + refs/tags/doubly-signed-tag + refs/tags/four + refs/tags/one + refs/tags/signed-tag + refs/tags/three + refs/tags/two +EOF + +test_expect_success 'empty pattern prints pseudorefs' ' + git update-ref ORIG_HEAD main && + git for-each-ref --format="%(refname)" "" >actual && + test_cmp expect actual +' + +test_expect_success 'empty pattern with other patterns' ' + git update-ref ORIG_HEAD main && + git for-each-ref --format="%(refname)" "" "refs/" >actual && + test_cmp expect actual +' + +test_expect_success 'empty pattern towards the end' ' + git update-ref ORIG_HEAD main && + git for-each-ref --format="%(refname)" "refs/" "" >actual && + test_cmp expect actual +' + test_expect_success 'filtering with --points-at' ' cat >expect <<-\EOF && refs/heads/main
When the user uses an empty string pattern (""), we don't match any refs in git-for-each-ref(1). This is because in git-for-each-ref(1), we use path based matching and an empty string doesn't match any path. In this commit we change this behavior by making empty string pattern match all references. This is done by introducing a new flag `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly introduced `refs_for_each_all_refs()` function to iterate over all the refs in the repository. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- Documentation/git-for-each-ref.txt | 3 ++- builtin/for-each-ref.c | 21 +++++++++++++++++- ref-filter.c | 13 ++++++++++-- ref-filter.h | 4 +++- t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 5 deletions(-)