* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Vladimir Panteleev @ 2017-01-20 22:55 UTC (permalink / raw)
To: Junio C Hamano, Vladimir Panteleev; +Cc: git
In-Reply-To: <xmqqa8aly2o4.fsf@gitster.mtv.corp.google.com>
On 2017-01-20 19:03, Junio C Hamano wrote:
> and viewed in the wider context, I notice that quiet is not honored
> in the added code. I think that is easily fixable by replacing this
> hunk with something like:
--quiet will still work correctly with the current patch, because
show_ref already checks quiet. Granted, the original --verify code used
show_one and not show_ref; however, I don't see a meaningful difference
between calling show_ref and show_one for HEAD, other than a bit of
overhead, so adding a new function may not be worthwhile. I will still
add tests for this; however, in light of this, would you still like me
to perform the change you requested?
--
Best regards,
Vladimir
^ permalink raw reply
* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Junio C Hamano @ 2017-01-20 22:31 UTC (permalink / raw)
To: Vladimir Panteleev; +Cc: git
In-Reply-To: <xmqqefzxwew9.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> My use case was the following series of steps:
>> ... long and readable if a bit too verbose description ...
> Your log message for the patch needs to be updated by summarizing
> the above better.
That raises the number of things to improve in the patch to 3 (so
far):
* the log message clarification.
* the code change I mentioned already to hook into the existing
"only read the fully specified ref" part, sharing the actual
action (i.e. read_ref() to read it, honor quiet and show it by
calling show_one()), instead of adding another clause that does
it differently (i.e. you didn't do read_ref/quiet?/show_one)
* add a test to make sure that the command also works sensibly when
--quiet is used.
With all of the above, I think the change makes sense.
^ permalink raw reply
* Idea: Add a filter option to 'git rebase'
From: Philip Oakley @ 2017-01-20 22:28 UTC (permalink / raw)
To: Git List; +Cc: Johannes Schindelin
A recent question on stackoverflow
http://stackoverflow.com/questions/41753252/drop-commits-by-commit-message-in-git-rebase
sought to remove automatically commits that could be identified by relevant
words in the commit message.
I had thought that the ubiquitous `git filter-branch` should be able to do
this sort of thing. I was wrong. (It was pointed out to me that...) The man
page notes that removing a commit via filter-branch does not remove the
changes from following commits and directs readers to using `git rebase(1)`.
However the rebase command does not have any filter option to allow the
automatic population of its TODO list with the appropriate
pick/edit/drop/etc. values.
It does feel as if a --filter style option would be a useful addition to
rebase to complement the filter-branch options once the current conversion
from script to code is complete.
Is this something that should be put in the 'worth considering' pile?
--
Philip
^ permalink raw reply
* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Junio C Hamano @ 2017-01-20 22:22 UTC (permalink / raw)
To: Vladimir Panteleev; +Cc: git
In-Reply-To: <1bf9a446-0b00-f27a-4625-0bc8c25356fe@gmail.com>
Vladimir Panteleev <thecybershadow@gmail.com> writes:
> Hi Junio,
>
> On 2017-01-20 19:03, Junio C Hamano wrote:
>> Having said all that, using --verify on HEAD does not make much
>> sense, because if HEAD is missing in .git/, I do not think Git
>> considers that directory as a Git repository to begin with. So from
>> that point of view, I am not sure what value this change adds to the
>> system, even though the change is almost correct (modulo the "quiet"
>> thing).
>
> My use case was the following series of steps:
>
> Q1: How do I resolve a full ref name to a commit SHA1?
> A1: Use show-ref <full-ref-name>.
>
> Q2: How to make git show-ref also work when HEAD is specified as the
> reference?
> A2: Add --head.
>
> Q3: How do I make git show-ref only look for the exact full ref name
> specified, instead of doing a pattern/substring search, and thus
> output at most one result?
> A3: Add --verify.
>
> However, A2 and A3 are incompatible. Thus, there doesn't seem to be a
> way to e.g. make a simple alias which looks up a ref only by its full
> ref name, where said ref might or might not be HEAD. The obvious
> workaround is to check if the ref is HEAD in the rev-parse caller,
> however it seemed more logical to fix it in git instead.
>
> The documentation for show-ref also makes no mention that --head is
> ignored if --verify is specified, and the combination was not covered
> by any tests, therefore this seemed to me as a simple omission in
> --verify's logic.
>
> There is also rev-parse, which also has a --verify switch, however it
> does something else, and I don't see a way to ask rev-parse to only
> consider full refs.
Your log message for the patch needs to be updated by summarizing
the above better. I couldn't read the motivation behind the change
fully from the original (even though I guessed it correctly).
^ permalink raw reply
* Re: [PATCH v2 1/2] grep: only add delimiter if there isn't one already
From: Junio C Hamano @ 2017-01-20 22:19 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git, Jeff King, Brandon Williams
In-Reply-To: <20170120171126.16269-2-stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes:
> git-grep(1) output does not follow git's own syntax:
>
> $ 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 avoids emitting the unnecessary ':' delimiter if the name
> already ends with ':' or '/':
>
> $ git grep malloc v2.9.3:
> v2.9.3:t/test-lib.sh: setup_malloc_check () {
> $ git show v2.9.3:t/test-lib.sh
> (succeeds)
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> builtin/grep.c | 6 +++++-
> t/t7810-grep.sh | 21 +++++++++++++++++++++
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8887b6a..90a4f3d 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -491,7 +491,11 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
> strbuf_init(&base, PATH_MAX + len + 1);
> if (len) {
> strbuf_add(&base, name, len);
> - strbuf_addch(&base, ':');
> +
> + /* Add a delimiter if there isn't one already */
> + if (name[len - 1] != '/' && name[len - 1] != ':') {
> + strbuf_addch(&base, ':');
> + }
I agree with peff and Brandon that checking treeness is the right
way to make the decision, and you seem to have done that in the
updated 2/2, but why doesn't it apply here as well?
> }
> init_tree_desc(&tree, data, size);
> hit = grep_tree(opt, pathspec, &tree, &base, base.len,
^ permalink raw reply
* Re: [PATCH v2 2/2] grep: use '/' delimiter for paths
From: Junio C Hamano @ 2017-01-20 22:18 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git, Jeff King, Brandon Williams
In-Reply-To: <20170120171126.16269-3-stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> 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'
>
> 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)
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> builtin/grep.c | 4 +++-
> t/t7810-grep.sh | 5 +++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 90a4f3d..7a7aab9 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -494,7 +494,9 @@ 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/ */
> + char delim = obj->type == OBJ_COMMIT ? ':' : '/';
Why check the equality with commit, rather than un-equality with
tree? Wouldn't you want to treat $commit:path and $tag:path the
same way?
I also think the two patches should be squashed together, as it is
only after this patch that the code does the "right thing" by
changing the delimiter depending on obj->type.
> + strbuf_addch(&base, delim);
> }
> }
> init_tree_desc(&tree, data, size);
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index e804a3f..8a58d5e 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1445,6 +1445,11 @@ test_expect_success 'grep outputs valid <rev>:<path> for HEAD:t/' '
> test_cmp expected actual
> '
>
> +test_expect_success 'grep outputs valid <rev>:<path> for HEAD:t' '
> + git grep vvv HEAD:t >actual &&
> + test_cmp expected actual
> +'
> +
Perhaps you want an annotated tag object refs/tags/v1.0 that points
at the commit at the HEAD, and then run the same grep on v1.0:t, in
addition to the above test.
> cat >expected <<EOF
> HEAD:t/a/v:vvv
> HEAD:t/v:vvv
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Jeff King @ 2017-01-20 22:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqqy3y5wg0l.fsf@gitster.mtv.corp.google.com>
On Fri, Jan 20, 2017 at 01:58:02PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Jeff King <peff@peff.net> writes:
> >
> >>> And in my current understanding of submodules the check in
> >>> .gitmodules ought to be enough, too.
> >>
> >> Yeah, that probably makes sense. You can have a gitlink without a
> >> .gitmodules file, but I don't quite know what that would mean in terms
> >> of submodules (I guess it's not a submodule but "something else").
> >
> > That may be a lot better than reading the index unconditionally, but
> > I'd rather not to see "git rev-parse" read ".gitmodules" at all. It
> > would discourage scripted use of Git for no good reason.
>
> Thinking about this more, I suspect that
>
> cd sub && git anything
>
> when the index of the top-level thinks "sub" must be a submodule and
> the user is not interested in "sub" (hence it hasn't gone through
> "git submodule init" or "update") should get the same error as you
> would get if you did
>
> cd /var/tmp/ && git anything
>
> when none of /, /var, /var/tmp/ is controlled by any Git repository.
> I.e. "fatal: Not a git repository".
Not sure if our emails just crossed, but yes, I agree completely.
> Perhaps we can update two things and make it cheap.
>
> - checking out the top-level working tree without populating the
> working tree of a submodule learns to do a bit more than just
> creating an empty directory. Instead, it creates the third kind
> of ".git" (we currently support two kinds of ".git", one that is
> a repository itself, and another that is points at a repository),
> that tells us that there is not (yet) a repository there.
>
> - the "discovering the root of the working tree" logic learns to
> notice the third kind of ".git" and stop with "Not a git
> repository".
Yeah, I thought about suggesting something like that earlier. It's
slightly more efficient than the "find the root and then complain" thing
Stefan and I were talking about, but I'd worry it comes with weird
corner cases. E.g., are there tools (or other parts of git) that care
about it literally being an empty directory? E.g., would parts of
"checkout" need to know that it's OK to blow it away?
^ permalink raw reply
* Re: [PATCH] log: new option decorate reflog of remote refs
From: Jacob Keller @ 2017-01-20 22:00 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <20170120143031.p2mux5uxxzniovkx@sigill.intra.peff.net>
On Fri, Jan 20, 2017 at 6:30 AM, Jeff King <peff@peff.net> wrote:
>> Imposing order between options could cause confusion, I think, if you
>> remove --decorate-reflog leaving --remotes on by accident, now you get
>> --remotes with a new meaning. We could go with something like
>> --decodate-reflog=remote, but that clashes with the number of reflog
>> entries and we may need a separator, like --decorate-reflog=remote,3.
>> Or we could add something to --decorate= in addition to
>> short|full|auto|no. Something like --decorate=full,reflog or
>> --decorate=full,reflog=remote,entries=3 if I want 3 reflog entries.
>
> I agree that making option-order important is potentially confusing. But
> it does already exist with --exclude. It's necessary to specify some
> sets of refs (e.g., all of A, except for those that match B, and then
> all of C, including those that match B).
>
> Having --decorate-reflog=remote would be similarly constrained. You
> couldn't do "decorate all remotes except for these ones". For that
> matter, I'm not sure how you would do "decorate just the refs from
> origin".
>
> I'll grant that those are going to be a lot less common than just "all
> the remotes" (or all the tags, or whatever). I'd just hate to see us
> revisiting this in a year to generalize it, and being stuck with
> historical baggage.
>
>> My hesitant to go that far is because I suspect decorating reflog
>> won't be helpful for non-remotes. But I'm willing to make more changes
>> if it opens door to master.
>
> Forgetting reflogs for a moment, I'd actually find it useful to just
> decorate tags and local branches, but not remotes. But right now there
> isn't any way to select which refs are worthy of decoration (reflog or
> not).
>
> That's why I'm thinking so much about a general ref-selection system. I
> agree the "--exclude=... --remotes" thing is complicated, but it's also
> the ref-selection system we _already_ have, which to me is a slight
> point in its favor.
>
> -Peff
I agree that the interaction between --exclude and --remotes/etc is
confusing, but I think it's reasonable enough because we already
support it, so it makes sense to extend it with this. I also think its
better to extend here than it is to hard-code it. We could provide a
single short-option that does the longer variant if it's that common.
Thanks,
Jake
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Junio C Hamano @ 2017-01-20 21:58 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqq37gdxvbx.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>>> And in my current understanding of submodules the check in
>>> .gitmodules ought to be enough, too.
>>
>> Yeah, that probably makes sense. You can have a gitlink without a
>> .gitmodules file, but I don't quite know what that would mean in terms
>> of submodules (I guess it's not a submodule but "something else").
>
> That may be a lot better than reading the index unconditionally, but
> I'd rather not to see "git rev-parse" read ".gitmodules" at all. It
> would discourage scripted use of Git for no good reason.
Thinking about this more, I suspect that
cd sub && git anything
when the index of the top-level thinks "sub" must be a submodule and
the user is not interested in "sub" (hence it hasn't gone through
"git submodule init" or "update") should get the same error as you
would get if you did
cd /var/tmp/ && git anything
when none of /, /var, /var/tmp/ is controlled by any Git repository.
I.e. "fatal: Not a git repository".
Perhaps we can update two things and make it cheap.
- checking out the top-level working tree without populating the
working tree of a submodule learns to do a bit more than just
creating an empty directory. Instead, it creates the third kind
of ".git" (we currently support two kinds of ".git", one that is
a repository itself, and another that is points at a repository),
that tells us that there is not (yet) a repository there.
- the "discovering the root of the working tree" logic learns to
notice the third kind of ".git" and stop with "Not a git
repository".
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Jeff King @ 2017-01-20 21:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqq37gdxvbx.fsf@gitster.mtv.corp.google.com>
On Fri, Jan 20, 2017 at 01:41:54PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> And in my current understanding of submodules the check in
> >> .gitmodules ought to be enough, too.
> >
> > Yeah, that probably makes sense. You can have a gitlink without a
> > .gitmodules file, but I don't quite know what that would mean in terms
> > of submodules (I guess it's not a submodule but "something else").
>
> That may be a lot better than reading the index unconditionally, but
> I'd rather not to see "git rev-parse" read ".gitmodules" at all. It
> would discourage scripted use of Git for no good reason.
Why is that? Just because it makes rev-parse seem more bloated?
I think Stefan's putting it into git.c is confusing the issue a bit.
This is fundamentally about figuring out which git repository we're in,
and that procedure is the right place to put the check.
IOW, when we call setup_git_repository() we are already walking up the
tree and looking at .git/HEAD, .git/config, etc to see if we are in a
valid git repository. It doesn't seem unreasonable to me to make this
part of that check. I.e.:
- if we we walked up from the working tree (so we have a non-NULL
prefix); and
- if there is a .gitmodules file; and
- if the .gitmodules file shows that we were inside what _should_ have
been a submodule; then
- complain and do not accept the outer repository as a valid repo.
That adds only an extra failed open() for people who do not use
submodules, and an extra config-file parse for people who do. And then
only when they are not in the top-level of the working tree (so scripts,
etc that cd_to_toplevel wouldn't pay per-invocation).
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Junio C Hamano @ 2017-01-20 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <20170120200041.hefg44stddqe344z@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> And in my current understanding of submodules the check in
>> .gitmodules ought to be enough, too.
>
> Yeah, that probably makes sense. You can have a gitlink without a
> .gitmodules file, but I don't quite know what that would mean in terms
> of submodules (I guess it's not a submodule but "something else").
That may be a lot better than reading the index unconditionally, but
I'd rather not to see "git rev-parse" read ".gitmodules" at all. It
would discourage scripted use of Git for no good reason.
^ permalink raw reply
* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Vladimir Panteleev @ 2017-01-20 20:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1bf9a446-0b00-f27a-4625-0bc8c25356fe@gmail.com>
And to clarify:
On 2017-01-20 20:26, Vladimir Panteleev wrote:
> On 2017-01-20 19:03, Junio C Hamano wrote:
>> Having said all that, using --verify on HEAD does not make much
>> sense, because if HEAD is missing in .git/, I do not think Git
>> considers that directory as a Git repository to begin with.
The behavior of --verify I am interested in is not to check that the ref
exists, but to get its SHA1 while avoiding the pattern search. This
avoids accidentally matching refs via substring (e.g. "git show-ref
--head HEAD" will print HEAD, but also e.g. refs/remotes/origin/HEAD),
and for performance reasons (looking up a ref by exact name can be MUCH
faster than matching all refs by pattern search, e.g. in one of my
projects where I use git as an object store, --verify makes a difference
of 21 milliseconds vs. over 5 minutes).
--
Best regards,
Vladimir
^ permalink raw reply
* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Vladimir Panteleev @ 2017-01-20 20:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa8aly2o4.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On 2017-01-20 19:03, Junio C Hamano wrote:
> Having said all that, using --verify on HEAD does not make much
> sense, because if HEAD is missing in .git/, I do not think Git
> considers that directory as a Git repository to begin with. So from
> that point of view, I am not sure what value this change adds to the
> system, even though the change is almost correct (modulo the "quiet"
> thing).
My use case was the following series of steps:
Q1: How do I resolve a full ref name to a commit SHA1?
A1: Use show-ref <full-ref-name>.
Q2: How to make git show-ref also work when HEAD is specified as the
reference?
A2: Add --head.
Q3: How do I make git show-ref only look for the exact full ref name
specified, instead of doing a pattern/substring search, and thus output
at most one result?
A3: Add --verify.
However, A2 and A3 are incompatible. Thus, there doesn't seem to be a
way to e.g. make a simple alias which looks up a ref only by its full
ref name, where said ref might or might not be HEAD. The obvious
workaround is to check if the ref is HEAD in the rev-parse caller,
however it seemed more logical to fix it in git instead.
The documentation for show-ref also makes no mention that --head is
ignored if --verify is specified, and the combination was not covered by
any tests, therefore this seemed to me as a simple omission in
--verify's logic.
There is also rev-parse, which also has a --verify switch, however it
does something else, and I don't see a way to ask rev-parse to only
consider full refs.
--
Best regards,
Vladimir
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Stefan Beller @ 2017-01-20 20:07 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20170120200041.hefg44stddqe344z@sigill.intra.peff.net>
On Fri, Jan 20, 2017 at 12:00 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 20, 2017 at 11:53:01AM -0800, Stefan Beller wrote:
>
>> > One alternative would be to make the check cheaper. Could we reliably
>> > tell from the submodule.foo.* block in the config that path "foo" is a
>> > submodule? I think that would work after "submodule init" but not right
>> > after "git clone". So the index really is the source of truth there.
>>
>> Well we can check if there is a .gitmodules file that has a
>> submodule.*.path equal to the last part of $CWD, no need to look
>> at the git config.
>>
>> And that would also work right after git clone (in an
>> unpopulated/uninitialized submodule as I call it).
>>
>> And in my current understanding of submodules the check in
>> .gitmodules ought to be enough, too.
>
> Yeah, that probably makes sense. You can have a gitlink without a
> .gitmodules file, but I don't quite know what that would mean in terms
> of submodules (I guess it's not a submodule but "something else").
yeah, I agree it could be git series[1] at work, or as you said
"something else", and we have no idea what to do.
I think this could actually be implemented top-down, because the
check is cheap as we're beginning with lstat(.gitmodules), and no further
pursue checking this corner case in case the .gitmodules is not found.
I'll see if I can make a patch that passes the test suite.
[1] https://github.com/git-series/git-series/blob/master/INTERNALS.md
>
> -Peff
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Jeff King @ 2017-01-20 20:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kYKY=hDVjUx7AkeWZ=3V8Fy2hqQMFPZcoxT4NvXTFgG=Q@mail.gmail.com>
On Fri, Jan 20, 2017 at 11:53:01AM -0800, Stefan Beller wrote:
> > One alternative would be to make the check cheaper. Could we reliably
> > tell from the submodule.foo.* block in the config that path "foo" is a
> > submodule? I think that would work after "submodule init" but not right
> > after "git clone". So the index really is the source of truth there.
>
> Well we can check if there is a .gitmodules file that has a
> submodule.*.path equal to the last part of $CWD, no need to look
> at the git config.
>
> And that would also work right after git clone (in an
> unpopulated/uninitialized submodule as I call it).
>
> And in my current understanding of submodules the check in
> .gitmodules ought to be enough, too.
Yeah, that probably makes sense. You can have a gitlink without a
.gitmodules file, but I don't quite know what that would mean in terms
of submodules (I guess it's not a submodule but "something else").
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Stefan Beller @ 2017-01-20 19:53 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20170120194224.vikzovupwqx53x2c@sigill.intra.peff.net>
On Fri, Jan 20, 2017 at 11:42 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 20, 2017 at 11:33:45AM -0800, Stefan Beller wrote:
>
>> > I'd rather see it in the commands themselves. Especially given the
>> > "ideal" in your status example, which requires command-specific
>> > knowledge.
>>
>> So you rather want to go bottom up, i.e. add it to each command individually
>> for which it makes sense, instead of rather first having a catch-it-all like
>> this and then we can have a flag similar to RUN_SETUP, e.g.
>> ALLOW_IN_UNPOP_SUBMODULE, which allows commands to
>> take over the responsibility to act responsibly in this case?
>
> Yes. I know it's "less safe" in the sense that commands have to make an
> effort to detect the situation, but I feel like only they'll know what
> the sensible behavior is. And they can also do the check at a time when
> they would be reading the index anyway.
>
>> status may be the first command for going that route; I wonder if we'd
>> want to add this feature unconditionally or only in the porcelain case.
>> (In plumbing you're supposed to know what you're doing... so there is
>> no need as well as our promise to not change it)
>
> Yeah. The reason that it would be so painful to load the index
> for every rev-parse is not just that it probably doesn't otherwise need
> the index, but that scripts may make a _ton_ of rev-parse (or other
> plumbing) calls.
>
> One alternative would be to make the check cheaper. Could we reliably
> tell from the submodule.foo.* block in the config that path "foo" is a
> submodule? I think that would work after "submodule init" but not right
> after "git clone". So the index really is the source of truth there.
Well we can check if there is a .gitmodules file that has a
submodule.*.path equal to the last part of $CWD, no need to look
at the git config.
And that would also work right after git clone (in an
unpopulated/uninitialized submodule as I call it).
And in my current understanding of submodules the check in
.gitmodules ought to be enough, too.
>
> I guess there could be an index extension "these are the gitlinks I
> contain" and in theory we could read just that extension. I dunno.
>
> -Peff
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Jeff King @ 2017-01-20 19:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kaJQefSDkV-LKxRCTtSepsNsX7U+AZqy3Z_YCd1xsmTxQ@mail.gmail.com>
On Fri, Jan 20, 2017 at 11:33:45AM -0800, Stefan Beller wrote:
> > I'd rather see it in the commands themselves. Especially given the
> > "ideal" in your status example, which requires command-specific
> > knowledge.
>
> So you rather want to go bottom up, i.e. add it to each command individually
> for which it makes sense, instead of rather first having a catch-it-all like
> this and then we can have a flag similar to RUN_SETUP, e.g.
> ALLOW_IN_UNPOP_SUBMODULE, which allows commands to
> take over the responsibility to act responsibly in this case?
Yes. I know it's "less safe" in the sense that commands have to make an
effort to detect the situation, but I feel like only they'll know what
the sensible behavior is. And they can also do the check at a time when
they would be reading the index anyway.
> status may be the first command for going that route; I wonder if we'd
> want to add this feature unconditionally or only in the porcelain case.
> (In plumbing you're supposed to know what you're doing... so there is
> no need as well as our promise to not change it)
Yeah. The reason that it would be so painful to load the index
for every rev-parse is not just that it probably doesn't otherwise need
the index, but that scripts may make a _ton_ of rev-parse (or other
plumbing) calls.
One alternative would be to make the check cheaper. Could we reliably
tell from the submodule.foo.* block in the config that path "foo" is a
submodule? I think that would work after "submodule init" but not right
after "git clone". So the index really is the source of truth there.
I guess there could be an index extension "these are the gitlinks I
contain" and in theory we could read just that extension. I dunno.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Stefan Beller @ 2017-01-20 19:33 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20170120191728.l3ne5tt5pwbmafjh@sigill.intra.peff.net>
On Fri, Jan 20, 2017 at 11:17 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 19, 2017 at 11:30:23AM -0800, Stefan Beller wrote:
>
>> Now let's ask the same question for "git -C sub status ." (which is a
>> command that is only reading and not writing to the repository)
>>
>> 1) If the submodule is populated, the user clearly intended to know
>> more about the submodules status
>> 2) It is unclear if the user wanted to learn about the submodules state
>> (So ideally: "The submodule 'sub' is not initialized. To init ...")
>> or the status check should be applied to the superproject instead.
>>
>> Avoid the confusion in 2) as well and just error out for now. Later on
>> we may want to add another flag to git.c to allow commands to be run
>> inside unpopulated submodules and each command reacts appropriately.
>
> I like the general idea of catching commands in unpopulated submodules,
> but I'm somewhat uncomfortable with putting an unconditional check into
> git.c, for two reasons:
>
> 1. Reading the index can be expensive. You would not want "git
> rev-parse" to incur this cost.
Well, I would want rev-parse to not be run in the wrong repo.
(intended to rev-parse something in the submodule, but got results for
the superproject).
Talking about rev-parse, I was about to propose an extension in reply to
"[PATCH] git-prompt.sh: add submodule indicator" that rev-parse could
learn a flag similar to --show-toplevel, named:
--show-superproject-if-any or
--indicate-if-in-submodule-possibly
which would help out there.
>
> 2. How does this interact with commands which do interact with the
> index? Don't they expect to find the_index unpopulated?
That is another sloppiness in this RFC patch, as I haven't nailed down
the corner cases yet.
>
> (I notice that it's effectively tied to RUN_SETUP, which is good.
> But that also means that many commands, like "diff", won't get the
> benefit. Not to mention non-builtins).
>
> I'd rather see it in the commands themselves. Especially given the
> "ideal" in your status example, which requires command-specific
> knowledge.
So you rather want to go bottom up, i.e. add it to each command individually
for which it makes sense, instead of rather first having a catch-it-all like
this and then we can have a flag similar to RUN_SETUP, e.g.
ALLOW_IN_UNPOP_SUBMODULE, which allows commands to
take over the responsibility to act responsibly in this case?
status may be the first command for going that route; I wonder if we'd
want to add this feature unconditionally or only in the porcelain case.
(In plumbing you're supposed to know what you're doing... so there is
no need as well as our promise to not change it)
Thanks,
Stefan
>
> -Peff
^ permalink raw reply
* Re: [PATCH/TOY] Shortcuts to quickly refer to a commit name with keyboard
From: Jeff King @ 2017-01-20 19:25 UTC (permalink / raw)
To: Jacob Keller; +Cc: Nguyễn Thái Ngọc Duy, Git mailing list
In-Reply-To: <CA+P7+xoMTX5n_h+5MytZwVqKjqa0wdNKCeDtH29A_+WSfr6gTQ@mail.gmail.com>
On Fri, Jan 20, 2017 at 11:16:15AM -0800, Jacob Keller wrote:
> > I recently taught urxvt to recognize sha1s and grab them via keyboard
> > hints, and I'm finding it quite useful. Here's what it looks like if
> > you're interested:
> >
> > http://peff.net/git-hints.gif
> >
> > The hints technique is taken from pentadactyl (which I also use), but
> > the urxvt port is mine. I'm happy to share the code.
> [...]
>
> I would be interested in the code for this.. I'm curious if I can
> adapt it to my use of tmux.
See below. You can drop it into ~/.urxvt/ext/urlmatch and put something
like this into your Xresources:
URxvt.keysym.C-b: urlmatch:list
URxvt.keysym.C-semicolon: urlmatch:hint
I think the interesting bits if you're adapting it to another system
will be the regexes in find_commits(), and the key-at-a-time state
machine in handle_select_key().
Note that there's a sort-of bug in find_commits(). If you have:
1234abcd..5678abcd
it finds three matches: one for each sha1, and one for the whole range.
But the range hint is covered up by the first sha1's hint.
And probably other bugs, too. I've only been using it for about a week,
and I did a bunch of cleanups this morning (after foolishly offering to
let other people see it ;) ).
-- >8 --
#!/usr/bin/perl
use warnings qw(all FATAL);
use strict;
sub on_action {
my ($self, $action) = @_;
($action, my @args) = split /:/, $action;
if ($action eq 'list') {
$self->matchlist(@args);
} elsif ($action eq 'hint') {
$self->open_hints(@args);
}
();
}
sub d { print STDERR "debug: " . join(' ', @_) . "\n"; }
sub handle_select_key {
my ($self, $event, $keysym, $octets) = @_;
my $input = $self->{input};
my $escape;
if (48 <= $keysym && $keysym <= 57) {
$input->{number} .= ($keysym - 48);
} elsif (97 <= $keysym && $keysym <= 122) {
$input->{text} .= chr($keysym);
} elsif ($self->XKeysymToString($keysym) eq 'Return') {
$input->{number} .= '$';
} elsif ($keysym == 59) {
$input->{action} = 'yank';
return 1;
} else {
$escape = 1;
}
if (!$escape) {
my $min = 0;
my $max = scalar(@{$input->{array}});
my $nr_re = qr/^$input->{number}/;
my $text_re = qr/$input->{text}/;
my @matches;
for (my $i = $min; $i < $max; $i++) {
my $item = $input->{array}->[$i];
if ($i =~ /$nr_re/ && $item->{text} =~ /$text_re/) {
push @matches, $i;
} elsif ($input->{unmatch}) {
my $item = $input->{array}->[$i];
$input->{unmatch}->($self, $item);
}
}
if (@matches > 1) {
# not enough data yet
return 1;
}
if (@matches == 1) {
my $item = $input->{array}->[$matches[0]];
my $action = $input->{action} ||
$item->{action} ||
'activate';
if ($action eq 'activate') {
$self->exec_async(qw(webview), $item->{text});
} elsif ($action eq 'yank') {
$self->selection($item->{text});
$self->selection_grab(urxvt::CurrentTime);
}
}
}
delete $self->{input};
$self->disable('key_press');
1;
}
sub enable_select {
my $self = shift;
$self->{input} = {text => '', number => '', @_};
$self->enable(key_press => \&handle_select_key);
}
sub matchlist {
my $self = shift;
my @items = $self->find_matches(@_)
or return;
my $max_width = 0;
for (my $i = 0; $i < @items; $i++) {
my $item = $items[$i];
$item->{number} = $i;
$item->{menu} = "$i. $item->{text}";
my $width = $self->strwidth($item->{menu});
$max_width = $width if $width > $max_width;
}
my $rend = urxvt::SET_COLOR(urxvt::OVERLAY_RSTYLE, 0, 3);
my $o = $self->overlay(0, 0, $max_width, scalar(@items), $rend, 0);
for (my $i = 0; $i < @items; $i++) {
$o->set(0, $i, $items[$i]->{menu});
}
$self->enable_select(
array => \@items,
overlay => $o,
unmatch => sub {
my ($self, $item) = @_;
my $o = $self->{input}->{overlay};
$o->set(0, $item->{number}, ' ' x length($item->{menu}));
},
);
}
sub open_hints {
my $self = shift;
my @items = $self->find_matches(@_)
or return;
for (my $i = 0; $i < @items; $i++) {
my $item = $items[$i];
$item->{overlay} = $self->overlay($item->{col}, $item->{row},
length($i), 1,
urxvt::OVERLAY_RSTYLE, 0);
$item->{overlay}->set(0, 0, $i);
}
$self->enable_select(
array => \@items,
unmatch => sub {
my ($self, $item) = @_;
$item->{overlay} = undef;
},
);
}
my $URLCHAR = qr![-a-zA-Z:0-9./\#%+~_?=&()@;,]!;
my $URLEND = qr![a-zA-Z0-9/]!;
#my $URLLINE = qr!$URLCHAR{60,}$URLEND\n|$URLCHAR*$URLEND!;
my $URL = qr!$URLCHAR*$URLEND!;
sub find_urls {
local $_ = shift;
my @urls;
while (m!(http|https|ftp|file)://$URL!g) {
push @urls, [$-[0], $&];
}
while (m!^ *([a-zA-Z0-9]$URL\.(?:com|net|org)/$URL)!mg) {
push @urls, [$-[0], "http://$&"];
}
while (m!(?:^|(?<=\s))(www|ftp)\.$URL!g) {
push @urls, [$-[0], ($1 eq 'www' ? 'http://' : 'ftp://') . $&];
}
return map { {
offset => $_->[0],
text => $_->[1],
} } @urls;
}
sub find_commits {
local $_ = shift;
my @items;
while (m/(?:^|[^0-9a-f])
([0-9a-f]{7,40})
(?![0-9a-f])/gx) {
push @items, { offset => $-[1], text => $1, action => 'yank' };
}
while (m/(?:^|\s)
([0-9a-f]{7,} \.{2,3} [0-9a-f]{7,})
(?:$|\s)/gx) {
push @items, { offset => $-[1], text => $1, action => 'yank' };
}
return @items;
}
sub find_matches {
my $self = shift;
my @ret;
my $row = 0;
while ($row < $self->nrow) {
my $line = $self->line($row);
for my $item (find_line_matches($line->t)) {
@$item{qw(row col)} = $line->coord_of($item->{offset});
push @ret, $item;
}
$row = 1 + $line->end;
}
return @ret;
}
sub find_line_matches {
my $buf = shift;
return map {
if ($_ eq 'url') { find_urls($buf) }
elsif ($_ eq 'commit') { find_commits($buf) }
else { d("unknown type: $_"); () }
} @_ ? @_ : qw(url commit);
}
^ permalink raw reply
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Jeff King @ 2017-01-20 19:17 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170119193023.26837-1-sbeller@google.com>
On Thu, Jan 19, 2017 at 11:30:23AM -0800, Stefan Beller wrote:
> Now let's ask the same question for "git -C sub status ." (which is a
> command that is only reading and not writing to the repository)
>
> 1) If the submodule is populated, the user clearly intended to know
> more about the submodules status
> 2) It is unclear if the user wanted to learn about the submodules state
> (So ideally: "The submodule 'sub' is not initialized. To init ...")
> or the status check should be applied to the superproject instead.
>
> Avoid the confusion in 2) as well and just error out for now. Later on
> we may want to add another flag to git.c to allow commands to be run
> inside unpopulated submodules and each command reacts appropriately.
I like the general idea of catching commands in unpopulated submodules,
but I'm somewhat uncomfortable with putting an unconditional check into
git.c, for two reasons:
1. Reading the index can be expensive. You would not want "git
rev-parse" to incur this cost.
2. How does this interact with commands which do interact with the
index? Don't they expect to find the_index unpopulated?
(I notice that it's effectively tied to RUN_SETUP, which is good.
But that also means that many commands, like "diff", won't get the
benefit. Not to mention non-builtins).
I'd rather see it in the commands themselves. Especially given the
"ideal" in your status example, which requires command-specific
knowledge.
-Peff
^ permalink raw reply
* Re: [PATCH/TOY] Shortcuts to quickly refer to a commit name with keyboard
From: Jacob Keller @ 2017-01-20 19:16 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, Git mailing list
In-Reply-To: <20170120160942.srqf4y5w5r6feidw@sigill.intra.peff.net>
On Fri, Jan 20, 2017 at 8:09 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 20, 2017 at 05:22:49PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> OK This patch is horrible. Though the idea is cool and I've found it
>> very useful. So here it is. Perhaps the idea may be revised a bit
>> that's more suitable for more than one user.
>>
>> The problem is old, SHA-1 name is not keyboard-friendly, even in
>> abbreviated form. And recent change has made abbrev form longer,
>> harder to type. Most of the time I just go with copy/paste with the
>> mouse, which I don't like. name-rev helps a bit, but it's still long
>> to type (especially with all the ^ and ~ that requires holding shift
>> down).
>
> Not really a comment on your patch itself, but I think a lot of people
> solve this at a higher level, either in their terminal or via a tool
> like tmux.
>
> I recently taught urxvt to recognize sha1s and grab them via keyboard
> hints, and I'm finding it quite useful. Here's what it looks like if
> you're interested:
>
> http://peff.net/git-hints.gif
>
> The hints technique is taken from pentadactyl (which I also use), but
> the urxvt port is mine. I'm happy to share the code.
>
> Which isn't to say solving it inside Git is wrong, but I've found it
> really convenient for two reasons:
>
> 1. It works whenever you see a sha1, not just in git commands (so
> emails, inside commit messages, etc).
>
> 2. It doesn't take any screen space until you're ready to select.
>
> The big downside is that it's scraping the screen, so you're guessing at
> what is a sha1. False positives are a little annoying, but usually not
> that big a deal because you're already looking at what you want to
> select, and the hint pops up right there.
>
> -Peff
I would be interested in the code for this.. I'm curious if I can
adapt it to my use of tmux.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Junio C Hamano @ 2017-01-20 19:03 UTC (permalink / raw)
To: Vladimir Panteleev; +Cc: git
In-Reply-To: <20170120155015.4360-1-git@thecybershadow.net>
Vladimir Panteleev <git@thecybershadow.net> writes:
> This patch adds --head support to show-ref's --verify logic, by
> explicitly checking if the "HEAD" ref is specified when --head is
> present.
> @@ -207,6 +207,8 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> if (!quiet)
> show_one(*pattern, &oid);
> }
> + else if (show_head && !strcmp(*pattern, "HEAD"))
> + head_ref(show_ref, NULL);
> else if (!quiet)
> die("'%s' - not a valid ref", *pattern);
> else
The context around here look like this:
while (*pattern) {
struct object_id oid;
if (starts_with(*pattern, "refs/") &&
!read_ref(*pattern, oid.hash)) {
if (!quiet)
show_one(*pattern, &oid);
}
else if (!quiet)
die("'%s' - not a valid ref", *pattern);
else
return 1;
pattern++;
}
and viewed in the wider context, I notice that quiet is not honored
in the added code. I think that is easily fixable by replacing this
hunk with something like:
- if (starts_with(*pattern, "refs/") &&
+ if (to_show_ref(*pattern) &&
and then another hunk that implements to_show_ref() helper function,
perhaps like
static int to_show_ref(const char *pattern)
{
if (starts_with(pattern, "refs/"))
return 1;
if (show_head && !strcmp(pattern, "HEAD"))
return 1;
return 0;
}
or something.
Having said all that, using --verify on HEAD does not make much
sense, because if HEAD is missing in .git/, I do not think Git
considers that directory as a Git repository to begin with. So from
that point of view, I am not sure what value this change adds to the
system, even though the change is almost correct (modulo the "quiet"
thing).
^ permalink raw reply
* Re: [PATCH/TOY] Shortcuts to quickly refer to a commit name with keyboard
From: Junio C Hamano @ 2017-01-20 18:27 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170120160942.srqf4y5w5r6feidw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Not really a comment on your patch itself, but I think a lot of people
> solve this at a higher level, either in their terminal or via a tool
> like tmux.
>
> I recently taught urxvt to recognize sha1s and grab them via keyboard
> hints, and I'm finding it quite useful. Here's what it looks like if
> you're interested:
>
> http://peff.net/git-hints.gif
Nice. I would have called the "give me the string that is already
on the screen" solution solving at a lower level, not higher, but I
think I agree with the general direction. I always work in "screen"
and grab a string I see displayed by going into its "copy" mode,
which lets me jump to where the string appears by searching, and I
think that is a solution in the same class.
> 2. It doesn't take any screen space until you're ready to select.
Yup, personally I find this quite important (as I am often on a box
with a smaller screen).
I dream an integration with the command line completion we have. I
do not offhand see what the mecanism to tell what object names were
shown on the display recently to the completion mechanism should be,
but if we can solve that small detail, the result would be wonderful
;-)
^ permalink raw reply
* Re: [RFC 1/2] grep: only add delimiter if there isn't one already
From: Junio C Hamano @ 2017-01-20 18:16 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git
In-Reply-To: <20170120135612.GB17499@stefanha-x1.localdomain>
Stefan Hajnoczi <stefanha@redhat.com> writes:
> v2.9.3::Makefile may convey that the user originally provided v2.9.3:
> but is that actually useful information?
You are either asking a wrong question, or asking a wrong person
(i.e. Git) the question. The real question is why the user added a
colon at the end, when "v2.9.3" alone would have sufficed. What did
the user want to get out of giving an extra colon like "v2.9.3:"?
One answer I can think of is that it is a degenerate case of [2/2],
i.e. if "v2.9.3:t" were an appropriate way to look in the subtree
"t" of "v2.9.3", "v2.9.3:" would be the appropriate way to look in
the whole tree of "v2.9.3".
I understand, from your response to my comment in the thread for
[2/2], that the reason why "v2.9.3:t" was used in the example was
because you felt uncomfortable with using pathspec.
That's superstition.
My only piece of advice to folks who feel that way is to learn Git
more and get comfortable. You can do neat things like
$ git grep -e pattern rev -- t ':!t/helper/'
that you cannot do with "rev:t", for example ;-)
All examples we saw so far are the ones that the user used the colon
syntax when it is more natural to express the command without it. I
am hesitant to see the behaviour of the command changed to appease
such suboptimal use cases if it requires us to discard a bit of
information, when we haven't established it is OK to lose.
There may be a case
(1) where the colon syntax is the most natural thing to use, AND
script reading the output that used that syntax is forced to do
unnecessary work because "git grep" parrots the colon
literally, instread of being more intelligent about it
(i.e. omitting or substituting with slash when the input is a
tree object, not a tree-ish, as discussed in the thread).
(2) where the colon syntax is the most natural thing, AND script
reading the output WANTS to see the distinction in the input
(remember, "git grep" can take more than one input tree).
We haven't seen either of the above two in the discussion, so the
discussion so far is not sufficient to support the change being
proposed in this RFC, which requires that it is safe to assume that
(1) is the only case where the input is a raw tree (or the input
contains a colon) and (2) will never happen.
So I am still mildly negative on the whole thing.
^ permalink raw reply
* Re: merge maintaining history
From: Junio C Hamano @ 2017-01-20 17:33 UTC (permalink / raw)
To: Jakub Narębski; +Cc: David J. Bakeman, Jacob Keller, Git mailing list
In-Reply-To: <38ca43cb-2fc7-0448-352f-7d9413f815c5@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
> Then you would have the above history in repositories that fetched
> refs/replace/*, and the one below if replacement info is absent:
>
> original A<-B<-C<-D<-E<-F<-----------M
> \ /
> first branch b<-c<-d<-e /
> /
> new repo e*<-f->g->h
>
> But as Junio said it is highly unlikely that you are in this situation.
I do not think I said it is highly unlikely. I just said that I
didn't read in what David wrote that he did some unusual things, so
I based my conclusion on that assumption. People who bring problems
here sometimes forget to tell crucial details, and that missing
piece of information often changes how the situation should be
handled.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox