Re: [RFC 2/2] grep: use '/' delimiter for paths
Stefan Hajnoczi writes:
> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
>
> $ git grep malloc v2.9.3:t
> v2.9.3:t:test-lib.sh: setup_malloc_check () {
> $ git show v2.9.3:t:test-lib.sh
> fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
I am slightly less negative on this compared to 1/2, but not by a
big margin. The user wanted to feed a subtree to the command,
instead of doing the more natural
$ git grep -e malloc v2.9.3 -- t
So again, "contains a colon character" is not coming from what Git
does, but the user gave Git "a colon character" when she didn't have
to.
Having said that, if we wanted to start ignoring what the end-user
said in the initial input like 1/2 and 2/2 does (i.e. "this specific
tree object" as an input), I think the approach to solve for 1/2 and
2/2 should be the same. I think we should decide to do a slash
instead of a colon, not because v2.9.3: has colon at the end and
v2.9.3:t has colon in it, but because both of these are both bare
tree objects. The patches presented does not seem to base their
decision on the actual object type but on the textual input, which
seems wrong.
Re: [RFC 2/2] grep: use '/' delimiter for paths
On 01/19, Stefan Hajnoczi wrote:
> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
>
> $ git grep malloc v2.9.3:t
> v2.9.3:t:test-lib.sh: setup_malloc_check () {
> $ git show v2.9.3:t:test-lib.sh
> fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
>
> This patch attempts to use the correct delimiter:
>
> $ git grep malloc v2.9.3:t
> v2.9.3:t/test-lib.sh: setup_malloc_check () {
> $ git show v2.9.3:t/test-lib.sh
> (success)
>
> This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
> it as a path because it contains colons.
>
> Signed-off-by: Stefan Hajnoczi
> ---
> builtin/grep.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 3643d8a..06f8b47 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct
> pathspec *pathspec,
>
> /* Add a delimiter if there isn't one already */
> if (name[len - 1] != '/' && name[len - 1] != ':') {
> - strbuf_addch(&base, ':');
> + /* rev: or rev:path/ */
> + strbuf_addch(&base, strchr(name, ':') ? '/' :
> ':');
As Jeff mentioned it may be better to base which character gets appended
by checking the obj->type field like this maybe:
diff --git a/builtin/grep.c b/builtin/grep.c
index 69dab5dc5..9dfe11dc7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -495,7 +495,8 @@ static int grep_object(struct grep_opt *opt, const struct
pathspec *pathspec,
/* Add a delimiter if there isn't one already */
if (name[len - 1] != '/' && name[len - 1] != ':') {
/* rev: or rev:path/ */
- strbuf_addch(&base, strchr(name, ':') ? '/' :
':');
+ char del = obj->type == OBJ_COMMIT ? ':' : '/';
+ strbuf_addch(&base, del);
}
}
init_tree_desc(&tree, data, size);
--
Brandon Williams
Re: [RFC 2/2] grep: use '/' delimiter for paths
On Thu, Jan 19, 2017 at 10:54:02AM -0800, Junio C Hamano wrote:
> Stefan Hajnoczi writes:
>
> > If the tree contains a sub-directory then git-grep(1) output contains a
> > colon character instead of a path separator:
> >
> > $ git grep malloc v2.9.3:t
> > v2.9.3:t:test-lib.sh: setup_malloc_check () {
> > $ git show v2.9.3:t:test-lib.sh
> > fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
>
> I am slightly less negative on this compared to 1/2, but not by a
> big margin. The user wanted to feed a subtree to the command,
> instead of doing the more natural
>
> $ git grep -e malloc v2.9.3 -- t
I find : vs -- confusing:
| : | --
--+--+-
git grep | OK | OK
--+--+-
git show | OK | ignored
--+--+-
git log | no output| OK
--+--+-
Neither syntax always does what I expect. If git show --
honored then I could use that syntax consistently.
Sorry for going on a tangent. Does it seem reasonable to handle
in git-show(1) as a UI convenience?
> So again, "contains a colon character" is not coming from what Git
> does, but the user gave Git "a colon character" when she didn't have
> to.
>
> Having said that, if we wanted to start ignoring what the end-user
> said in the initial input like 1/2 and 2/2 does (i.e. "this specific
> tree object" as an input), I think the approach to solve for 1/2 and
> 2/2 should be the same. I think we should decide to do a slash
> instead of a colon, not because v2.9.3: has colon at the end and
> v2.9.3:t has colon in it, but because both of these are both bare
> tree objects. The patches presented does not seem to base their
> decision on the actual object type but on the textual input, which
> seems wrong.
Yes, reparsing the name is ugly and I hoped to get feedback with this
RFC. Thanks for the quick review!
signature.asc
Description: PGP signature
Re: [RFC 2/2] grep: use '/' delimiter for paths
On Thu, Jan 19, 2017 at 10:29:34AM -0800, Brandon Williams wrote:
> On 01/19, Stefan Hajnoczi wrote:
> > If the tree contains a sub-directory then git-grep(1) output contains a
> > colon character instead of a path separator:
> >
> > $ git grep malloc v2.9.3:t
> > v2.9.3:t:test-lib.sh: setup_malloc_check () {
> > $ git show v2.9.3:t:test-lib.sh
> > fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
> >
> > This patch attempts to use the correct delimiter:
> >
> > $ git grep malloc v2.9.3:t
> > v2.9.3:t/test-lib.sh: setup_malloc_check () {
> > $ git show v2.9.3:t/test-lib.sh
> > (success)
> >
> > This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
> > it as a path because it contains colons.
> >
> > Signed-off-by: Stefan Hajnoczi
> > ---
> > builtin/grep.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 3643d8a..06f8b47 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const
> > struct pathspec *pathspec,
> >
> > /* Add a delimiter if there isn't one already */
> > if (name[len - 1] != '/' && name[len - 1] != ':') {
> > - strbuf_addch(&base, ':');
> > + /* rev: or rev:path/ */
> > + strbuf_addch(&base, strchr(name, ':') ? '/' :
> > ':');
>
> As Jeff mentioned it may be better to base which character gets appended
> by checking the obj->type field like this maybe:
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 69dab5dc5..9dfe11dc7 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -495,7 +495,8 @@ static int grep_object(struct grep_opt *opt, const struct
> pathspec *pathspec,
> /* Add a delimiter if there isn't one already */
> if (name[len - 1] != '/' && name[len - 1] != ':') {
> /* rev: or rev:path/ */
> - strbuf_addch(&base, strchr(name, ':') ? '/' :
> ':');
> + char del = obj->type == OBJ_COMMIT ? ':' : '/';
> + strbuf_addch(&base, del);
Nice, that also solves the false positive with @{1979-02-26 18:30:00}.
Stefan
signature.asc
Description: PGP signature
Re: [RFC 2/2] grep: use '/' delimiter for paths
On Fri, Jan 20, 2017 at 02:12:12PM +, Stefan Hajnoczi wrote: > I find : vs -- confusing: > > | : | -- > --+--+- > git grep | OK | OK > --+--+- > git show | OK | ignored > --+--+- > git log | no output| OK > --+--+- > > Neither syntax always does what I expect. If git show -- > honored then I could use that syntax consistently. > > Sorry for going on a tangent. Does it seem reasonable to handle > in git-show(1) as a UI convenience? It's not ignored; just as with git-log, it's a pathspec to limit the diff. E.g.: $ git show --name-status v2.9.3 ... M Documentation/RelNotes/2.9.3.txt M Documentation/git.txt M GIT-VERSION-GEN $ git show --name-status v2.9.3 -- Documentation M Documentation/RelNotes/2.9.3.txt M Documentation/git.txt That's typically less useful than it is with log (where limiting the diff also kicks in history simplification and omits some commits entirely). But it does do something. -Peff
Re: [RFC 2/2] grep: use '/' delimiter for paths
Jeff King writes: > It's not ignored; just as with git-log, it's a pathspec to limit the > diff. E.g.: > > $ git show --name-status v2.9.3 > ... > M Documentation/RelNotes/2.9.3.txt > M Documentation/git.txt > M GIT-VERSION-GEN > > $ git show --name-status v2.9.3 -- Documentation > M Documentation/RelNotes/2.9.3.txt > M Documentation/git.txt > > That's typically less useful than it is with log (where limiting the > diff also kicks in history simplification and omits some commits > entirely). But it does do something. I think Stefan is missing the fact that the argument to "git show :" actually is naming a blob that sits at the in the . In other words, "show" is acting as a glorified "git -p cat-file blob", in that use. The use of "git show" you are demonstrating is still about showing the commit object, whose behaviour is defined to show the log message and the diff relative to its sole parent, limited to the paths that match the pathspec. It is perfectly fine and desirable that "git show :" and "git show -- " behaves differently. These are two completely different features.
Re: [RFC 2/2] grep: use '/' delimiter for paths
On Fri, Jan 20, 2017 at 02:56:26PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > It's not ignored; just as with git-log, it's a pathspec to limit the > > diff. E.g.: > > > > $ git show --name-status v2.9.3 > > ... > > M Documentation/RelNotes/2.9.3.txt > > M Documentation/git.txt > > M GIT-VERSION-GEN > > > > $ git show --name-status v2.9.3 -- Documentation > > M Documentation/RelNotes/2.9.3.txt > > M Documentation/git.txt > > > > That's typically less useful than it is with log (where limiting the > > diff also kicks in history simplification and omits some commits > > entirely). But it does do something. > > I think Stefan is missing the fact that the argument to "git show > :" actually is naming a blob that sits at the > in the . In other words, "show" is acting as a glorified > "git -p cat-file blob", in that use. > > The use of "git show" you are demonstrating is still about showing > the commit object, whose behaviour is defined to show the log > message and the diff relative to its sole parent, limited to the > paths that match the pathspec. > > It is perfectly fine and desirable that "git show :" > and "git show -- " behaves differently. These are > two completely different features. Thanks for explaining guys. It all makes logical sense. I just hope I remember the distinctions in that table for everyday git use. Stefan signature.asc Description: PGP signature
Re: [RFC 2/2] grep: use '/' delimiter for paths
On Tue, Jan 24, 2017 at 1:54 AM Stefan Hajnoczi wrote: > > The use of "git show" you are demonstrating is still about showing > > the commit object, whose behaviour is defined to show the log > > message and the diff relative to its sole parent, limited to the > > paths that match the pathspec. > > > > It is perfectly fine and desirable that "git show :" > > and "git show -- " behaves differently. These are > > two completely different features. > > Thanks for explaining guys. It all makes logical sense. I just hope I > remember the distinctions in that table for everyday git use. Familiar itch; previous discussion here: https://public-inbox.org/git/1377528372-31206-1-git-send-email-ho...@cisco.com/ Phil
