* 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 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: [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: [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: [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: [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: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: [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: 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 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: [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] 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
* 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: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
* 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: [RFC 2/2] grep: use '/' delimiter for paths
From: Junio C Hamano @ 2017-01-20 22:56 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Hajnoczi, git
In-Reply-To: <20170120141954.xyocl6oqoykqmpl5@sigill.intra.peff.net>
Jeff King <peff@peff.net> 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
<tree-ish>:<path>" actually is naming a blob that sits at the <path>
in the <tree-ish>. 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 <commit>:<path>"
and "git show <commit> -- <path>" behaves differently. These are
two completely different features.
^ permalink raw reply
* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Junio C Hamano @ 2017-01-20 23:15 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:
> Your log message for the patch needs to be updated by summarizing
> the above better.
Here is my attempt.
"git show-ref HEAD" used with "--verify" (because the user is
not interested in seeing refs/remotes/origin/HEAD), and used
with "--head" (because the user does not want HEAD to be
filtered out), i.e. "git show-ref --head --verify HEAD", did
not work as expected.
Instead of insisting that the input begins with "refs/", allow
"HEAD" when "--head" is given in the codepath that handles
"--verify", so that all valid full refnames including HEAD are
passed to the same output machinery.
^ permalink raw reply
* Re: [PATCH v2 0/2] grep: make output consistent with revision syntax
From: Junio C Hamano @ 2017-01-20 23:11 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git, Jeff King, Brandon Williams
In-Reply-To: <20170120171126.16269-1-stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes:
> v2:
> * Use obj->type instead of re-parsing name for delimiter
> (Followed Brandon's suggestion but named the variable 'delim' since that
> name is used in other places and 'del' is used for deletion.)
> * Add tests
> * Update Patch 1 commit description with a more relevant example
> * PATCH instead of RFC, now works with all documented git-rev-parse(1) syntax
>
> git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
While I was queuing this series (which I think should become a
single patch in the final version), I was trying to see how it
should be described in the release note (aka an entry in the
periodicall "What's cooking" report). Here is how I explained it.
You may want to borrow some parts of the description, especially the
part that talks about <tree-ish>:<path> being a way to name a blob,
when updating the commit log message.
"git grep", when fed a tree-ish as an input, shows each hit
prefixed with "<tree-ish>:<path>:<lineno>:". As <tree-ish> is
almost always either a commit or a tag that points at a commit,
the early part of the output "<tree-ish>:<path>" can be used as
the name of the blob and given to "git show".
When <tree-ish> is a tree given in the extended SHA-1 syntax
(e.g. "<commit>:", or "<commit>:<dir>"), however, this results
in a string that does not name a blob (e.g. "<commit>::<path>"
or "<commit>:<dir>:<path>"). "git grep" has been taught to be
a bit more intelligent about these cases and omit a colon (in
the former case) or use slash (in the latter case) to produce
"<commit>:<path>" and "<commit>:<dir>/<path>" that can be used
as the name of a blob.
As a release note entry is written in a style different from the
commit log message, you would need to adjust the voice of the last
sentence (i.e. "Teach 'git grep' to ..." to give an order to the
codebase), but otherwise the above would make an understandable
justification for the change suitable in a log message.
^ permalink raw reply
* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Junio C Hamano @ 2017-01-20 23:20 UTC (permalink / raw)
To: Vladimir Panteleev; +Cc: Vladimir Panteleev, git
In-Reply-To: <3b1d2717-dd7f-2add-b935-3ace6063b258@gmail.com>
Vladimir Panteleev <thecybershadow@gmail.com> writes:
> --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?
If two codepaths are called "I don't see a meaningful difference",
then it is really better to share the same code. Today, they may
happen to behave identically. When we need to update the behaviour
of one, we'd be forced to update the other one to match.
IOW, something along this line, perhaps (not even compile tested so
take it with grain of salt).
Thanks.
builtin/show-ref.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 6d4e669002..57491152b7 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -202,7 +202,8 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
while (*pattern) {
struct object_id oid;
- if (starts_with(*pattern, "refs/") &&
+ if (((show_head && !strcmp(*pattern, "HEAD")) ||
+ starts_with(*pattern, "refs/")) &&
!read_ref(*pattern, oid.hash)) {
if (!quiet)
show_one(*pattern, &oid);
^ permalink raw reply related
* Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Stefan Beller @ 2017-01-20 23:22 UTC (permalink / raw)
To: Junio C Hamano, Brandon Williams, Jonathan Nieder
Cc: Jeff King, git@vger.kernel.org
In-Reply-To: <xmqqy3y5wg0l.fsf@gitster.mtv.corp.google.com>
On Fri, Jan 20, 2017 at 1:58 PM, Junio C Hamano <gitster@pobox.com> 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".
I agree. The idea with a tombstone sounds great from a
performance perspective as you do not need to do extra work
in the superproject at all, because any gitlink is detected early
in the discovery.
The big BUT is however the following:
How do current users know if a submodule is e.g. populated?
(From say a third party script). Most likely they use something like
test -e ${sub}/.git
as that just worked. So if we go with the tombstone idea, we
may break things. (i.e. the fictional third party script confirms any
submodule to be there, but it is not)
I do really like the idea though, so maybe we also need to provide
some submodule plumbing that we opine to be the "correct" way
to see the submodules state[1] to make the transition easier for the
script writers?
[1] c.f. submodule states in
https://github.com/gitster/git/commit/e2b51b9df618ceeff7c4ec044e20f5ce9a87241e
>
> 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: [PATCH] show-ref: Allow --head to work with --verify
From: Junio C Hamano @ 2017-01-20 23:29 UTC (permalink / raw)
To: Vladimir Panteleev; +Cc: Vladimir Panteleev, git
In-Reply-To: <xmqqshoduxnj.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> If two codepaths are called "I don't see a meaningful difference",
> then it is really better to share the same code. Today, they may
> happen to behave identically. When we need to update the behaviour
> of one, we'd be forced to update the other one to match.
>
> IOW, something along this line, perhaps (not even compile tested so
> take it with grain of salt).
By the way, I have no strong preference between "read-ref, check
quiet and show-one" and "show-ref", so if you make --verify to
consistently call "show_ref()" for both refs/heads/master and HEAD,
that is also perfectly fine.
I just do not want to see the same feature/codepath to call two
different implementations that happens to do the same thing today.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/2] grep: use '/' delimiter for paths
From: Brandon Williams @ 2017-01-20 23:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Hajnoczi, git, Jeff King
In-Reply-To: <xmqqpojhwf2r.fsf@gitster.mtv.corp.google.com>
On 01/20, Junio C Hamano wrote:
> 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 assume Stefan just grabbed my naive suggestion hence why it checks
equality with a commit. So that's my fault :) Either of these may
not be enough though, since if you do 'git grep malloc v2.9.3^{tree}'
with this change the output prefix is 'v2.9.3^{tree}/' instead of the
correct prefix 'v2.9.3^{tree}:'
>
> 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
--
Brandon Williams
^ permalink raw reply
* Re: Idea: Add a filter option to 'git rebase'
From: Thomas Braun @ 2017-01-20 23:35 UTC (permalink / raw)
To: Philip Oakley, Git List; +Cc: Johannes Schindelin
In-Reply-To: <8AED6D90D2B64AE3A63C6195CA983FE8@PhilipOakley>
Am 20.01.2017 um 23:28 schrieb Philip Oakley:
> 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.
Well you can use an arbitrary shell command as editor, so something like
$ GIT_SEQUENCE_EDITOR="sed -i -re 's/^pick /edit /'" git rebase -i master
will change pick to edit of all commits.
Maybe that can be mentioned in the man page of rebase?
^ permalink raw reply
* Re: [PATCH] show-ref: Allow --head to work with --verify
From: Vladimir Panteleev @ 2017-01-21 0:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo9z1ux7r.fsf@gitster.mtv.corp.google.com>
On 2017-01-20 23:29, Junio C Hamano wrote:
> By the way, I have no strong preference between "read-ref, check
> quiet and show-one" and "show-ref", so if you make --verify to
> consistently call "show_ref()" for both refs/heads/master and HEAD,
> that is also perfectly fine.
This sounds like a good idea, as it will also allow -d and some other
options to work with --verify (whatever use case there might be for
that). Will resubmit with that.
> I just do not want to see the same feature/codepath to call two
> different implementations that happens to do the same thing today.
Understood and agreed.
--
Best regards,
Vladimir
^ permalink raw reply
* [PATCH v2 4/4] show-ref: Inline show_one
From: Vladimir Panteleev @ 2017-01-21 1:08 UTC (permalink / raw)
To: git; +Cc: Vladimir Panteleev
In-Reply-To: <20170121010821.25046-1-git@thecybershadow.net>
As the small function is now only called from a single place, there is
no point in keeping it as a separate function any longer.
Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
builtin/show-ref.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 3cf344d47..ec44164d8 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -17,15 +17,6 @@ static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
static const char **pattern;
static const char *exclude_existing_arg;
-static void show_one(const char *refname, const struct object_id *oid)
-{
- const char *hex = find_unique_abbrev(oid->hash, abbrev);
- if (hash_only)
- printf("%s\n", hex);
- else
- printf("%s %s\n", hex, refname);
-}
-
static int show_ref(const char *refname, const struct object_id *oid,
int flag, void *cbdata)
{
@@ -74,7 +65,11 @@ static int show_ref(const char *refname, const struct object_id *oid,
if (quiet)
return 0;
- show_one(refname, oid);
+ hex = find_unique_abbrev(oid->hash, abbrev);
+ if (hash_only)
+ printf("%s\n", hex);
+ else
+ printf("%s %s\n", hex, refname);
if (!deref_tags)
return 0;
--
2.11.0
^ permalink raw reply related
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