':/<oneline prefix>' notation doesn't support full file syntax

':/<oneline prefix>' notation doesn't support full file syntax

[Eric Raible]

Although the rev-parse documentation claims that the
tree-ish:path/to/file syntax works is applicable, this is
not so when using the :/ "oneline prefix" syntax:

% git rev-parse v1.5.0.1-227-g28a4d94
28a4d940443806412effa246ecc7768a21553ec7
% git rev-parse ":/object name"
28a4d940443806412effa246ecc7768a21553ec7

% git rev-parse v1.5.0.1-227-g28a4d94:sha1_name.c
0781477a71ac4d76a1b8783868d6649cae7f8507
% git rev-parse ":/object name":sha1_name.c
:/object name:sha1_name.c
fatal: ambiguous argument ':/object name:sha1_name.c': unknown
revision or path not in the working tree.
Use '--' to separate paths from revisions

A quick look at int sha1_name.c:get_sha1() shows that it doesn't
even try to make this work.  Is this worth fixing?
Or at least documenting?

- Eric

Re: ':/<oneline prefix>' notation doesn't support full file syntax

[Junio C Hamano]

"Eric Raible" <raible@gmail.com> writes:

> % git rev-parse ":/object name":sha1_name.c
> :/object name:sha1_name.c
> fatal: ambiguous argument ':/object name:sha1_name.c': unknown
> revision or path not in the working tree.
> Use '--' to separate paths from revisions
>
> A quick look at int sha1_name.c:get_sha1() shows that it doesn't
> even try to make this work.  Is this worth fixing?

Is there anything to fix?  In that example, you are looking for a commit
that talks about "object name:sha1_name.c" in the comment.

Re: ':/<oneline prefix>' notation doesn't support full file syntax

[Eric Raible]

On Thu, Jul 3, 2008 at 1:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Eric Raible" <raible@gmail.com> writes:
>
> Is there anything to fix?  In that example, you are looking for a commit
> that talks about "object name:sha1_name.c" in the comment.

Yes.  What if I'm looking for specific file (i.e. sha1_name.c) in the commit
described by ":/object name:", just like I can do with 28a4d9404:sha1_name.c?

This is not ambiguous if we first consider the entire string as the prefix.
If that fails we look for a filename after the final ':'.

I'll post a patch in a moment.

- Eric

Re: ':/<oneline prefix>' notation doesn't support full file syntax

[Jeff King]

On Wed, Jul 02, 2008 at 10:42:52PM -0700, Eric Raible wrote:

> Although the rev-parse documentation claims that the
> tree-ish:path/to/file syntax works is applicable, this is
> not so when using the :/ "oneline prefix" syntax:
> 
> % git rev-parse v1.5.0.1-227-g28a4d94
> 28a4d940443806412effa246ecc7768a21553ec7
> % git rev-parse ":/object name"
> 28a4d940443806412effa246ecc7768a21553ec7
> 
> % git rev-parse v1.5.0.1-227-g28a4d94:sha1_name.c
> 0781477a71ac4d76a1b8783868d6649cae7f8507
> % git rev-parse ":/object name":sha1_name.c
> :/object name:sha1_name.c
> fatal: ambiguous argument ':/object name:sha1_name.c': unknown
> revision or path not in the working tree.
> Use '--' to separate paths from revisions
> 
> A quick look at int sha1_name.c:get_sha1() shows that it doesn't
> even try to make this work.  Is this worth fixing?
> Or at least documenting?

IMHO, :/ should stop eating text at the first ':', and allow '\:' for
a literal colon and '\\' for a literal backslash.

I think nobody has really cared up to this point (and I can't say that I
care that much now, but I wouldn't object to such a patch).

-Peff

Re: ':/<oneline prefix>' notation doesn't support full file syntax

[Johannes Schindelin]

Hi,

On Thu, 3 Jul 2008, Eric Raible wrote:

> On Thu, Jul 3, 2008 at 1:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > "Eric Raible" <raible@gmail.com> writes:
> >
> > Is there anything to fix?  In that example, you are looking for a 
> > commit that talks about "object name:sha1_name.c" in the comment.
> 
> Yes.  What if I'm looking for specific file (i.e. sha1_name.c) in the 
> commit described by ":/object name:", just like I can do with 
> 28a4d9404:sha1_name.c?
> 
> This is not ambiguous if we first consider the entire string as the 
> prefix. If that fails we look for a filename after the final ':'.

It is super-expensive, as you have to look through the whole history just 
to find that you do not find anything.

And then, it could be that you do find a commit that starts with that 
string, but what you really wanted it a file, not a commit.

And then, a file name can contain colons.  What to do in that case?

I think your "fix" is not worth it.  ":/<oneline>" is to help you find a 
commit, and it will only ever find the first commit anyway, so you are 
probably better off using

	$ git show $(git log --pretty=format:%H:path/to/file.c \
		--grep=^<oneline>)

to begin with.

Really, the only reason I ever wrote support for ":/blah" is when someone 
less-than-helpful says "In commit 'Bla bla bla' you broke XYZ" and I want 
to
	$ git show :/Bla

Nowadays, however, I would

	$ git log -p --grep=^Bla

so I'd vote to remove the ":/" syntax altogether.  We need not even 
concern ourselves with scripts using that syntax, since the semantics are 
so limited that nobody should use it in scripts anyway.

Ciao,
Dscho

Re: ':/<oneline prefix>' notation doesn't support full file syntax

[Dana How]

On Thu, Jul 3, 2008 at 1:50 AM, Eric Raible <raible@gmail.com> wrote:
> On Thu, Jul 3, 2008 at 1:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> "Eric Raible" <raible@gmail.com> writes:
>>
>> Is there anything to fix?  In that example, you are looking for a commit
>> that talks about "object name:sha1_name.c" in the comment.
>
> Yes.  What if I'm looking for specific file (i.e. sha1_name.c) in the commit
> described by ":/object name:", just like I can do with 28a4d9404:sha1_name.c?
>
> This is not ambiguous if we first consider the entire string as the prefix.
> If that fails we look for a filename after the final ':'.

In part you are proposing this because it is a consistent extension.
But a problem with the current :/string is that it adds 2nd meanings
to both : and / ,
which is not all that consistent to start with.

Last year Junio proposed that :/ be changed to ?
to eliminate the overloading;  thus your proposal becomes:
  ?string:filename
He chose ? because it results in a search backwards through commits.
(You could make that ?string?:filename if you prefer,  where the 2nd ?
 is only needed if you include a filename.)

I was surprised to see Dscho advocating removing this feature altogether.
Others proposed other command sequences which avoided :/ .
If :/ is now going to be extended and thus perhaps more likely to
appear in scripts,
is now the time to change it to ? which has no other special meaning to git?

Thanks,
-- 
Dana L. How danahow@gmail.com +1 650 804 5991 cell

Re: ':/<oneline prefix>' notation doesn't support full file syntax

[Junio C Hamano]

"Dana How" <danahow@gmail.com> writes:

> I was surprised to see Dscho advocating removing this feature altogether.
> Others proposed other command sequences which avoided :/ .
> If :/ is now going to be extended and thus perhaps more likely to
> appear in scripts,
> is now the time to change it to ? which has no other special meaning to git?

There are number of problems with ":/" notation, but my biggest gripe is
that it is only slightly better than "give back a random commit".  You
cannot even tell it to "dig from these branch tips, look for the first one
that talks about this text".

As Dscho mentioned, --grep works much better and instead of saying:

    $ git diff ':/send-email' HEAD

we can say:

    $ git diff \
      $(git log --pretty=format:%H -1 --grep=send-email master next) HEAD

The error behaviour is somewhat different between the two, though.  When
you misspell what to grep, the command substitution will give empty and
you would get an unexpected result.  Being built-in, ':/' syntax can say
"I do not find anything that match" fairly easily, and the command
substitution version has to say something ugly like:

    $ git diff \
        $(
            x=$(git log --pretty=format:%H -1 --grep=send-email master next)
            case "$x" in
            ('') echo 0000000000000000000000000000000000000000 ;;
            (?) echo $x ;; esac
        ) HEAD

to get a similar effect.

But the point is that you can extend it easily with the :path suffix if
you wanted to:

    $ git show \
        $(git log --pretty=format:%H -1 --grep=send-email):git-send-email.perl

You can even alias "log --pretty=format:%H -1" if you wanted to, and use
revision limiter other than --grep, like this:

    (in .git/config)

	[alias]
        	pick = log --pretty=format:%H -1

    $ git diff --stat $(git pick -- Documentation)^
    $ git blame $(git pick pu -- remote.c) remote.c

So in short, ':/' is limited (cannot be suffixed with :path, cannot be
told to dig down from named revs, etc.) but you can do what ':/' cannot do
fairly easily with command substitution.

However, $(git pick --all --grep=something), without suffixed modifiers
such as ~$N and :$path, may still be common enough that it might deserve a
short-hand ':/' (and that is why we have it).

If people do not find that short-hand useful, I am not strongly opposed to
the idea of dropping it.  I personally find the notation not very useful
cute hack anyway ;-).

Re: ':/<oneline prefix>' notation doesn't support full file syntax

[Johannes Schindelin]

Hi,

On Thu, 3 Jul 2008, Junio C Hamano wrote:

> "Dana How" <danahow@gmail.com> writes:
> 
> > I was surprised to see Dscho advocating removing this feature 
> > altogether. Others proposed other command sequences which avoided :/ . 
> > If :/ is now going to be extended and thus perhaps more likely to 
> > appear in scripts, is now the time to change it to ? which has no 
> > other special meaning to git?
> 
> There are number of problems with ":/" notation, but my biggest gripe is 
> that it is only slightly better than "give back a random commit".

Well, it _is_ better than that: it gives you the _newest_ matching commit, 
provided that the people involved in those commits maintained their NTP 
settings correctly.

The _real_ gripe you should have with the notation is what I pointed out 
already: it is _ill_-defined.  It _could_ match more than one commit, but 
matches only _one_.

> As Dscho mentioned, --grep works much better and instead of saying:
> 
>     $ git diff ':/send-email' HEAD
> 
> we can say:
> 
>     $ git diff \
>       $(git log --pretty=format:%H -1 --grep=send-email master next) HEAD
> 
> The error behaviour is somewhat different between the two, though.  
> When you misspell what to grep, the command substitution will give empty 
> and you would get an unexpected result.  Being built-in, ':/' syntax can 
> say "I do not find anything that match" fairly easily, and the command 
> substitution version has to say something ugly like:
> 
>     $ git diff \
>         $(
>             x=$(git log --pretty=format:%H -1 --grep=send-email master next)
>             case "$x" in
>             ('') echo 0000000000000000000000000000000000000000 ;;
>             (?) echo $x ;; esac
>         ) HEAD
> 
> to get a similar effect.

Again, this is the wrong way to think about it.  If you grep for things, 
you can get 0..infty matches, not necessarily 1.

To assume that you get at least one match is already an error.

Letting that funny "case" syntax slip by, I would suggest this command 
line instead:

$ $(git log --pretty=format:'git diff %H..;' --grep=send-email \
	master next)

> But the point is that you can extend it easily with the :path suffix if 
> you wanted to:
> 
>     $ git show \
>         $(git log --pretty=format:%H -1 \
>         --grep=send-email):git-send-email.perl

Again, I would rather suggest pulling the ":<path>" into the format, as I 
did _already_ in another mail, robustifying the whole command.

> So in short, ':/' is limited (cannot be suffixed with :path, cannot be 
> told to dig down from named revs, etc.) but you can do what ':/' cannot 
> do fairly easily with command substitution.
> 
> However, $(git pick --all --grep=something), without suffixed modifiers
> such as ~$N and :$path, may still be common enough that it might deserve a
> short-hand ':/' (and that is why we have it).
> 
> If people do not find that short-hand useful, I am not strongly opposed to
> the idea of dropping it.  I personally find the notation not very useful
> cute hack anyway ;-).

It was a cute hack, and before --grep it was actually useful.

Now it is not any more,
Dscho

Re: ':/<oneline prefix>' notation doesn't support full file syntax

[Johannes Schindelin]

Hi,

On Thu, 3 Jul 2008, Dana How wrote:

> I was surprised to see Dscho advocating removing this feature 
> altogether.

Why is everybody surprised when I admit mistakes?

Granted, --grep did not exist when I wrote :/ but now it does, and there 
is no good reason to keep an ill-defined construct in Git when we have 
something better.

Ciao,
Dscho