* Re: [RFC 2/2] grep: use '/' delimiter for paths
From: Stefan Hajnoczi @ 2017-01-20 14:17 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, gitster
In-Reply-To: <20170119182934.GH10641@google.com>
[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]
On Thu, Jan 19, 2017 at 10:29:34AM -0800, Brandon Williams wrote:
> On 01/19, Stefan Hajnoczi wrote:
> > If the tree contains a sub-directory then git-grep(1) output contains a
> > colon character instead of a path separator:
> >
> > $ git grep malloc v2.9.3:t
> > v2.9.3:t:test-lib.sh: setup_malloc_check () {
> > $ git show v2.9.3:t:test-lib.sh
> > fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
> >
> > This patch attempts to use the correct delimiter:
> >
> > $ git grep malloc v2.9.3:t
> > v2.9.3:t/test-lib.sh: setup_malloc_check () {
> > $ git show v2.9.3:t/test-lib.sh
> > (success)
> >
> > This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
> > it as a path because it contains colons.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > builtin/grep.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 3643d8a..06f8b47 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
> >
> > /* Add a delimiter if there isn't one already */
> > if (name[len - 1] != '/' && name[len - 1] != ':') {
> > - strbuf_addch(&base, ':');
> > + /* rev: or rev:path/ */
> > + strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
>
> As Jeff mentioned it may be better to base which character gets appended
> by checking the obj->type field like this maybe:
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 69dab5dc5..9dfe11dc7 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -495,7 +495,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
> /* Add a delimiter if there isn't one already */
> if (name[len - 1] != '/' && name[len - 1] != ':') {
> /* rev: or rev:path/ */
> - strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
> + char del = obj->type == OBJ_COMMIT ? ':' : '/';
> + strbuf_addch(&base, del);
Nice, that also solves the false positive with @{1979-02-26 18:30:00}.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [RFC 0/2] grep: make output consistent with revision syntax
From: Stefan Hajnoczi @ 2017-01-20 14:18 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20170119165958.xtotlvdta7udqllb@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]
On Thu, Jan 19, 2017 at 11:59:59AM -0500, Jeff King wrote:
> On Thu, Jan 19, 2017 at 03:03:45PM +0000, Stefan Hajnoczi wrote:
>
> > git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
> >
> > This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
> > and expect "git show rev:path/to/file.c" to work. See the individual patches
> > for examples of command-lines that produce invalid output.
>
> I think this is a good goal.
>
> I couldn't immediately think of any cases where your patches would
> misbehave, but my initial thought was that the "/" versus ":"
> distinction is about whether the initial object is a tree or a commit.
>
> You do still have to special case the root tree (so "v2.9.3:" does not
> get any delimiter). I think "ends in a colon" is actually a reasonable
> way of determining that.
>
> > This series is an incomplete attempt at solving the issue. I'm not familiar
> > enough with the git codebase to propose a better solution. Perhaps someone is
> > interested in a proper fix?
>
> Are there cases you know that aren't covered by your patches?
From Patch 2/2:
This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
it as a path because it contains colons.
If we use obj->type instead of re-parsing the name then that problem is
solved.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [RFC 2/2] grep: use '/' delimiter for paths
From: Jeff King @ 2017-01-20 14:19 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Junio C Hamano, git
In-Reply-To: <20170120141212.GC17499@stefanha-x1.localdomain>
On Fri, Jan 20, 2017 at 02:12:12PM +0000, Stefan Hajnoczi wrote:
> I find <rev>:<path> vs <rev> -- <path> confusing:
>
> | <rev>:<path> | <rev> -- <path>
> ----------+----------------------+---------------------
> git grep | OK | OK
> ----------+----------------------+---------------------
> git show | OK | <path> ignored
> ----------+----------------------+---------------------
> git log | no output | OK
> ----------+----------------------+---------------------
>
> Neither syntax always does what I expect. If git show <rev> -- <path>
> honored <path> then I could use that syntax consistently.
>
> Sorry for going on a tangent. Does it seem reasonable to handle <path>
> in git-show(1) as a UI convenience?
It's not ignored; just as with git-log, it's a pathspec to limit the
diff. E.g.:
$ git show --name-status v2.9.3
...
M Documentation/RelNotes/2.9.3.txt
M Documentation/git.txt
M GIT-VERSION-GEN
$ git show --name-status v2.9.3 -- Documentation
M Documentation/RelNotes/2.9.3.txt
M Documentation/git.txt
That's typically less useful than it is with log (where limiting the
diff also kicks in history simplification and omits some commits
entirely). But it does do something.
-Peff
^ permalink raw reply
* Re: [PATCH] log: new option decorate reflog of remote refs
From: Jeff King @ 2017-01-20 14:30 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8A_LkRMZYfoJuYEUok4r7Tw0VuMwVkG_Kr1o1hFcAUWNw@mail.gmail.com>
On Fri, Jan 20, 2017 at 05:55:21PM +0700, Duy Nguyen wrote:
> > We already have very flexible ref-selectors like --remotes, --branches,
> > etc. The generalization of this would perhaps be something like:
> >
> > git log --decorate-reflog --remotes --branches
> >
> > where "--decorate-reflog" applies to the next ref selector and then is
> > reset, the same way --exclude is. And it includes those refs _only_ for
> > decoration, not for traversal. So you could do:
> >
> > git log --decorate-reflog --remotes --remotes
> >
> > if you wanted to see use those as traversal roots, too (if this is
> > common, it might even merit another option for "decorate and show").
> >
> > That's just off the top of my head, so maybe there are issues. I was
> > just surprised to see the "-remote" part in your option name.
>
> 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
^ permalink raw reply
* Re: [RFC 0/2] grep: make output consistent with revision syntax
From: Jeff King @ 2017-01-20 14:32 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git, gitster
In-Reply-To: <20170120141832.GE17499@stefanha-x1.localdomain>
On Fri, Jan 20, 2017 at 02:18:32PM +0000, Stefan Hajnoczi wrote:
> > Are there cases you know that aren't covered by your patches?
>
> From Patch 2/2:
>
> This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
> it as a path because it contains colons.
>
> If we use obj->type instead of re-parsing the name then that problem is
> solved.
Ah, right. I somehow totally missed that, and blindly assumed your colon
search was only at the end. But of course that wouldn't work at all (it
would miss "v2.9.3:t").
So yes, definitely it should be checking the object type.
-Peff
^ permalink raw reply
* Re: [RFC] stash --continue
From: Marc Branchaud @ 2017-01-20 15:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stephan Beyer, git
In-Reply-To: <alpine.DEB.2.20.1701192220320.3469@virtualbox>
On 2017-01-19 04:30 PM, Johannes Schindelin wrote:
>
> At this point I will stop commenting on this issue, as I have said all
> that I wanted to say about it, at least once. If I failed to get my points
> across so far, I simply won't be understood.
Yes, we're obviously looking at this from completely different perspectives.
Stephan (or whoever) if you decide to do this work, I will be content
with whichever way you choose to go.
M.
^ permalink raw reply
* Re: [PATCH/TOY] Shortcuts to quickly refer to a commit name with keyboard
From: Johannes Schindelin @ 2017-01-20 15:21 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8C0SFsTNTB=R8zLLvnqkPofP0VQWPUR9pguT-n2Y+Tp1w@mail.gmail.com>
Hi Duy,
On Fri, 20 Jan 2017, Duy Nguyen wrote:
> On Fri, Jan 20, 2017 at 5:46 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Why not introduce a flag to "git log" that shows a keyboard-friendly name
> > similar to what `git name-rev` would have said, except that the name would
> > be generated using the name(s) specified on the command-line?
> >
> > Example:
> >
> > git log 8923d2d0 upstream/pu
> >
> > commit 8923d2d00192ceb1107078484cccf537cb51c1b5 (8923d2d0)
> > ...
> > commit 9f500d6cf5eaa49391d6deca85fc864e5bd23415 (8923d2d0^)
> > ...
> > commit f79c24a291a58845b08cfec7573e22cc153693e1 (8923d2d0~2)
> > ...
> > commit c921c5bb63baaa16dc760de9549da55c8c89dc9c (upstream/pu)
> > ...
> > commit 16793ba6b6333ba0cdee1adb53d979c3fbdb17bc (upstream/pu^)
> > ...
> >
> > Granted, this is still a little more cumbersome to type than @h1, but
> > then, you can skip those round-robin games as well as the possibly
> > backwards-incompatible extension of the rev syntax.
>
> I mentioned name-rev a few paragraphs down.
Okay, then.
> No, I want the sweet and short @h1 (or something like that). name-rev
> does not qualify.
I am afraid that you have to go back to the drawing board, then, to come
up with a backwards-compatible syntax that is as equally short and sweet
as @h1.
And you will probably also have to come up with a strategy for
implementing "transient refs", because that is exactly what you are doing
here. After all, you need to ensure that @h1 still works correctly (i.e.
without scary warnings) when the corresponding ref has been removed and an
aggressive garbage collection was completed.
And there are other concerns to be considered, not only the
backwards-compatibility. Certainly the current design will need to be
overhauled.
Just from a cursory brain-storming on my side (I even left out the
hand-waving concerns):
- this feature requires write-access. From a user's point of view, this
should be read-only, or at least also work without write access
- the term "commit mark" is already used for something very different in
fast-import
- the idea that running "git log --mark-commits master" twice will
generate *different* marks is surprising (and not in a good way)
- introducing a feature to save keyboard strokes, and then requiring the
user to type ` --mark-commits`, i.e. 15 key presses, is, uhm, funny
- the fact that these marks are per-worktree makes it a lot less useful
than if they were per-repository
- related: there is no way to re-use these marks in different
repositories, e.g. when helping other developers, say, in a chat room
- the fact that the first mark in every repository will be @a1 opens the
door very wide for using that mark accidentally in the wrong repository.
At least if I write 8923d in the git-gui repository, it will tell me
that there is no such revision. Using @a1, it would succeed, and it
would take me a *long* time to even realize that I am in the wrong
directory!
And then there are the implementation issues:
- it uses symlinks. I cannot let you do that, Dave.
- using sscanf() to get the value of the first byte of a buffer? What's
wrong with `*buf*`?
- "asdfghjk"? That is *very* limiting. The opposite of diverse. Just ask
the French, for starters
- latest_fd is never closed, nor flushed
It sure complicates the implementation side as well as the overall design
a heck of a lot to insist on not using existing rev syntax.
> I don't feel comfortable typing 8923d2d0 without looking at the
> keyboard, and that's a lot of movement on the keyboard.
I mentioned 8923d2d0 only as an example to illustrate the difference to
name-rev.
And when you talk about movement on the keyboard, you *must* realize that
you impose your favorite keyboard layout here. On the German keyboard, for
example, the `@` sign is typed via AltGr+q. That is very cumbersome right
there.
And when you talk about speed advantages of keyboard vs mouse, it seems
that there are studies that show that keyboard is faster than mouse. For
keyboard shortcuts. Not for typing. I actually found no study comparing
the speed of typing with the speed of copy/paste, although I am sure that
they exist, and that they show pretty clearly that copy/paste is faster,
and more accurate, than typing.
But back to minimizing movement on the keyboard.
You would usually type `git log upstream/pu`, tab-completing
`upstream/pu`, right? And then you would want to refer to one particular
entry in that output, say, `upstream/pu~50^2`, right?
Right:
> upstream/pu is a bit better, but still very long (at least for me). Yes
> TAB-ing does help, but not enough. Then you'll get the dreadful "^2~1^3"
> dance.
Yes, exactly. You would use tab-completion there, too, most of the way.
Although I do not find that "^2^^3" [*1*] dreadful, I can see that some
users find it cumbersome. And then it is a little bit over the top to try
to optimize away that "^2^^3".
However, I am starting to feel like you try to use the wrong approach,
bending Git's user interface out of shape just to be able force a workflow
that avoids copy-paste or GUIs, or for that matter, advanced ref notation
such as "upstream/pu^{/worktree.remove}".
Don't get me wrong, I would love to have faster method to go between log
and prompt. But to be honest, what costs me much more of my time is
figuring out which commit and branch any given mail talks about when
reviewing, or addressing reviews. Or for that matter, finding the mail
thread discussing the patch series that the "What's cooking" mail talks
about. I guess I'd spend more time on accelerating that than on avoiding
copy/paste from `git log`'s output.
Ciao,
Johannes
Footnote *1*: Do we really have octopus merges in Git? I do not think so.
*clicketyclick*. Urgh. You are correct. I found a whopping 45 in my
repository, and I certainly did not introduce any. On a sunny note, the
last Octopus seems to be v1.7.10-rc0~9^2~9(Merge branches
zj/decimal-width, zj/term-columns and jc/diff-stat-scaler, 2012-02-24),
i.e. a couple of years old.
^ permalink raw reply
* Re: [RFC] stash --continue
From: Johannes Schindelin @ 2017-01-20 15:27 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Stephan Beyer, git
In-Reply-To: <1fba12e3-78f0-6b2f-31ca-8d888744b9aa@xiplink.com>
Hi Marc,
On Fri, 20 Jan 2017, Marc Branchaud wrote:
> On 2017-01-19 04:30 PM, Johannes Schindelin wrote:
> >
> > At this point I will stop commenting on this issue, as I have said all
> > that I wanted to say about it, at least once. If I failed to get my
> > points across so far, I simply won't be understood.
>
> Yes, we're obviously looking at this from completely different
> perspectives.
Yes, but you claim to argue from the users' perspective, while I actually
work with enough users to be really certain that I described their mental
model of what an operation is very accurately.
> Stephan (or whoever) if you decide to do this work, I will be content
> with whichever way you choose to go.
So you only wanted to argue and not actually do anything? Tsk, tsk...
:-)
Ciao,
Johannes
^ permalink raw reply
* Re: fatal: bad revision 'git rm -r --ignore-unmatch -- folder'
From: jean-christophe manciot @ 2017-01-20 15:24 UTC (permalink / raw)
To: git
In-Reply-To: <CAKcFC3ZYwrVfEZ2Xua1kpQVeOMKY-wM=oce9JQhz4Tnookf8Dg@mail.gmail.com>
I've finally found the correct command after some significant research:
git filter-branch --tag-name-filter cat --index-filter "git rm -r
--cached --ignore-unmatch ${file_path}/${file_name}" --prune-empty
--force -- --all
On Fri, Jan 20, 2017 at 4:23 PM, jean-christophe manciot
<actionmystique@gmail.com> wrote:
> I've finally found the correct command after some significant research:
> git filter-branch --tag-name-filter cat --index-filter "git rm -r --cached
> --ignore-unmatch ${file_path}/${file_name}" --prune-empty --force -- --all
>
> On Thu, Jan 19, 2017 at 9:42 AM, jean-christophe manciot
> <actionmystique@gmail.com> wrote:
>>
>> Also some context information:
>> Ubuntu 16.10 4.8
>> git 2.11.0
>>
>> On Thu, Jan 19, 2017 at 9:32 AM, jean-christophe manciot
>> <actionmystique@gmail.com> wrote:
>> > In case you were wondering whether these files were tracked or not:
>> >
>> > git-Games# git ls-files Ubuntu/16.04
>> > Ubuntu/16.04/residualvm_0.3.0~git-1_amd64.deb
>> > Ubuntu/16.04/scummvm-data_1.8.0_all.deb
>> > Ubuntu/16.04/scummvm-data_1.9.0_all.deb
>> > Ubuntu/16.04/scummvm-dbgsym_1.8.2~git20160821.bad86050_amd64.deb
>> > Ubuntu/16.04/scummvm-dbgsym_1.9.0_amd64.deb
>> > Ubuntu/16.04/scummvm_1.8.0_amd64.deb
>> > Ubuntu/16.04/scummvm_1.9.0_amd64.deb
>> >
>> > On Tue, Jan 17, 2017 at 4:30 PM, jean-christophe manciot
>> > <actionmystique@gmail.com> wrote:
>> >> Hi there,
>> >>
>> >> I'm trying to purge a complete folder and its files from the
>> >> repository history with:
>> >>
>> >> git-Games# git filter-branch 'git rm -r --ignore-unmatch --
>> >> Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD
>> >> fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/'
>> >>
>> >> git does not find the folder although it's there:
>> >>
>> >> git-Games# ll Ubuntu/16.04/
>> >> total 150316
>> >> drwxr-x--- 2 actionmystique actionmystique 4096 Nov 19 13:40 ./
>> >> drwxr-x--- 4 actionmystique actionmystique 4096 Oct 30 14:02 ../
>> >> -rwxr-x--- 1 actionmystique actionmystique 2190394 May 9 2016
>> >> residualvm_0.3.0~git-1_amd64.deb*
>> >> ...
>> >> -rw-r--r-- 1 actionmystique actionmystique 67382492 Oct 13 09:15
>> >> scummvm-dbgsym_1.9.0_amd64.deb
>> >>
>> >> What is going on?
>> >>
>> >> --
>> >> Jean-Christophe
>> >
>> >
>> >
>> > --
>> > Jean-Christophe
>>
>>
>>
>> --
>> Jean-Christophe
>
>
>
>
> --
> Jean-Christophe
--
Jean-Christophe
^ permalink raw reply
* Re: [PATCH/TOY] Shortcuts to quickly refer to a commit name with keyboard
From: Jeff King @ 2017-01-20 16:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20170120102249.15572-1-pclouds@gmail.com>
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
^ permalink raw reply
* [PATCH] show-ref: Allow --head to work with --verify
From: Vladimir Panteleev @ 2017-01-20 15:50 UTC (permalink / raw)
To: git; +Cc: Vladimir Panteleev
Previously, when --verify was specified, --head was being ignored, and
"show-ref --verify --head HEAD" would always print "fatal: 'HEAD' -
not a valid ref". As such, when using show-ref to look up any
ref (including HEAD) precisely by name, one would have to special-case
looking up the HEAD ref.
This patch adds --head support to show-ref's --verify logic, by
explicitly checking if the "HEAD" ref is specified when --head is
present.
Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
builtin/show-ref.c | 2 ++
t/t1403-show-ref.sh | 8 ++++++++
2 files changed, 10 insertions(+)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 6d4e66900..ee5078604 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -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
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 7e10bcfe3..de64ebfb7 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -164,4 +164,12 @@ test_expect_success 'show-ref --heads, --tags, --head, pattern' '
test_cmp expect actual
'
+test_expect_success 'show-ref --verify --head' '
+ {
+ echo $(git rev-parse HEAD) HEAD
+ } >expect &&
+ git show-ref --verify --head HEAD >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.11.0
^ permalink raw reply related
* [PATCH v2 1/2] grep: only add delimiter if there isn't one already
From: Stefan Hajnoczi @ 2017-01-20 17:11 UTC (permalink / raw)
To: git; +Cc: Jeff King, gitster, Brandon Williams, Stefan Hajnoczi
In-Reply-To: <20170120171126.16269-1-stefanha@redhat.com>
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, ':');
+ }
}
init_tree_desc(&tree, data, size);
hit = grep_tree(opt, pathspec, &tree, &base, base.len,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index de2405c..e804a3f 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1435,4 +1435,25 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '
test_cmp expected actual
'
+cat >expected <<EOF
+HEAD:t/a/v:vvv
+HEAD:t/v:vvv
+EOF
+
+test_expect_success 'grep outputs valid <rev>:<path> for HEAD:t/' '
+ git grep vvv HEAD:t/ >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<EOF
+HEAD:t/a/v:vvv
+HEAD:t/v:vvv
+HEAD:v:vvv
+EOF
+
+test_expect_success 'grep outputs valid <rev>:<path> for HEAD:' '
+ git grep vvv HEAD: >actual &&
+ test_cmp expected actual
+'
+
test_done
--
2.9.3
^ permalink raw reply related
* [PATCH v2 2/2] grep: use '/' delimiter for paths
From: Stefan Hajnoczi @ 2017-01-20 17:11 UTC (permalink / raw)
To: git; +Cc: Jeff King, gitster, Brandon Williams, Stefan Hajnoczi
In-Reply-To: <20170120171126.16269-1-stefanha@redhat.com>
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 ? ':' : '/';
+ 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
+'
+
cat >expected <<EOF
HEAD:t/a/v:vvv
HEAD:t/v:vvv
--
2.9.3
^ permalink raw reply related
* [PATCH v2 0/2] grep: make output consistent with revision syntax
From: Stefan Hajnoczi @ 2017-01-20 17:11 UTC (permalink / raw)
To: git; +Cc: Jeff King, gitster, Brandon Williams, Stefan Hajnoczi
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.
This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
and expect "git show rev:path/to/file.c" to work. See the individual patches
for examples of command-lines that produce invalid output.
Stefan Hajnoczi (2):
grep: only add delimiter if there isn't one already
grep: use '/' delimiter for paths
builtin/grep.c | 8 +++++++-
t/t7810-grep.sh | 26 ++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
--
2.9.3
^ 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
* 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: [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: [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: 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: [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: 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: 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: [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: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 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
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