* [RFC] Instruct git-completion.bash that we are in test mode
From: Jean-Noël AVILA @ 2013-01-21 22:30 UTC (permalink / raw)
To: git
In test mode, git completion should only propose core commands.
Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
---
I reworked the patch so that the test argument is only evaluated
when sourcing the file and there is no environment clutter.
At least, "it works for me".
contrib/completion/git-completion.bash | 8 +++++++-
t/t9902-completion.sh | 2 +-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 14dd5e7..ac9fa65 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,10 +531,16 @@ __git_complete_strategy ()
return 1
}
+if [ "x$1" != "xTEST" ]; then
+ __git_cmdlist () { git help -a|egrep '^ [a-zA-Z0-9]'; }
+else
+ __git_cmdlist () { git help -a| egrep -m 1 -B1000 PATH | egrep '^ [a-zA-Z0-9]'; }
+fi
+
__git_list_all_commands ()
{
local i IFS=" "$'\n'
- for i in $(git help -a|egrep '^ [a-zA-Z0-9]')
+ for i in $(__git_cmdlist)
do
case $i in
*--*) : helper pattern;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..51463b2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -13,7 +13,7 @@ complete ()
return 0
}
-. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" TEST
# We don't need this function to actually join words or do anything special.
# Also, it's cleaner to avoid touching bash's internal completion variables.
--
1.8.1.1.271.g02f55e6
^ permalink raw reply related
* Re: [PATCH 0/2] Hiding some refs in ls-remote
From: Jeff King @ 2013-01-21 22:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, git, spearce, mfick
In-Reply-To: <7v38xxnfv3.fsf@alter.siamese.dyndns.org>
On Sat, Jan 19, 2013 at 11:16:00AM -0800, Junio C Hamano wrote:
> For example, if you mirror-clone from either of my repositories from
> GitHub:
>
> git clone --mirror git://github.com/git/git/
> git clone --mirror git://github.com/gitster/git/
>
> you will see stuff that does not belong to the project; objects that
> are only reachable from refs/pull/* are not something I approved to
> be placed in my repository; I as the project owner do not want to
> give these objects to a cloner as if they are part of my project.
>
> The server side can hide refs/pull/ and refs/pull-request-tags/ so
> that clones (and ls-remote) will not see clutter, and nobody gets
> hurt.
I think it is unfortunately not so simple as that. You do not want them
to be part of your "clone --mirror", but some people do (because they
will inspect them when evaluating the pull request). And that decision
is, I think, in the eye of the cloner, not the eye of the repo owner. I
think in your case it is a little harder to see, in that you do not care
about the PR mechanism for git.git at all, but let us think for a minute
about a project that actually uses PRs.
For an ordinary developer, they would be content to fetch the branch
tips and tags (and that is what we do by default with "git clone). They
do not need to fetch all of refs/pull. If they learn out-of-band that PR
123 exists, they can in theory ask for refs/pull/123/head. That works
even with your proposal, because it is just about dropping the
advertisement, not the availability of refs.
But what about entities which really do want all of refs/pull? I can
think of two good reasons to want them:
1. You really do want a mirror, because you are creating a backup or
alternate distribution channel. IOW, you are using "git clone
--mirror", but it is missing those refs.
2. You are a developer who is sometimes disconnected. You want to
fetch all of the PRs, and then examine them at your leisure.
Without advertising, how do they ask for the wildcard of `refs/pull/*`?
They're stuck massaging some out-of-band data into a set of distinct
fetch refspecs.
I don't know much about what's in Gerrit's refs/changes, but I imagine
one could make a similar argument that their value is in the eye of the
client. And later you give this example:
> Another example. If you run ls-remote against the above two
> repositories, you will notice that the latter has tons more
> branches. The former are to publish only the primary integration
> branches, while the latter are to show individual topics.
>
> I wish I didn't have to do this if I could.
>
> We have periodical "What's cooking" postings that let interested
> parties learn topics out-of-band. If I can hide refs/heads/??/*
> from normal users' clones while actually keeping individual topics
> there at the same place, I do not have to push into two places.
Most people do not want to see those heads. But some people (like me)
do, and it is a great convenience to be able to fetch them all with a
wildcard refspec, which cannot work if they are not advertised. I would
have to parse what's cooking and fetch them each individually. That's
not a technologically insurmountable problem, but it means git is being
a lot less helpful than it could be.
So I think in these cases of "extra refs", some clients would want them,
and some would not. And only they can know which camp they are in, not
the server side. Which is why the current system works pretty well: we
advertise everything and let the client decide what it wants. Clone very
sanely fetches only refs/heads/* (and associated tags), and you can put
whatever extra junk you want elsewhere in the hierarchy. A mirrored
clone will fetch it, but to me that is the point of --mirror. And if you
want some arbitrary subset, then you can implement that, too, by the
use of fetch refspecs[1].
The downside, of course, is that the ref advertisement is bigger than
many clients will want. But dealing with that is a separate issue from
"these refs should never be shown", as solutions for one may not work
from the other (e.g., if we delayed ref advertisement until the client
had spoken, the client could tell us what refspecs they are interested
in).
For your topic branch example, why don't you push to refs/topics of the
main git repository? Then normal cloners wouldn't see it, but anybody
interested could add the appropriate fetch refspec.
-Peff
[1] It may be that refspecs are not as expressive as we would like,
but that is a client side problem we can deal with. For instance,
you cannot currently say "give me everything except refs/foo/*".
^ permalink raw reply
* Re: [PATCH 0/2] Hiding some refs in ls-remote
From: Jeff King @ 2013-01-21 23:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, spearce, mfick
In-Reply-To: <7vip6rjyn3.fsf@alter.siamese.dyndns.org>
On Sun, Jan 20, 2013 at 02:08:32PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Jeff King <peff@peff.net> writes:
> >
> >>> [uploadPack]
> >>> hiderefs = refs/changes
> >>
> >> Would you want to do the same thing on receive-pack? It could benefit
> >> from the same reduction in network cost (although it tends to be invoked
> >> less frequently than upload-pack).
> > ...
> > As I said, I view this primarily as solving the cluttering issue.
> > The use of hiderefs to hide these refs that point at objects I do
> > not consider belong to my repository from the pushers equally makes
> > sense as such, I think.
>
> Now that raises one question. Should this be transfer.hiderefs that
> governs both upload-pack and receive-pack? I tend to think the
> answer is yes.
Yes, that is what I was getting at (and it should have individual
hiderefs for each side that override the transfer.*, in case somebody
wants to make the distinction).
> It may even make sense to have "git push" honor it, excluding them
> from "git push --mirror" (or "git push --all" if some of the
> branches are hidden); I haven't thought things through, though.
That is harder, as that is something that happens on the client. How
does the client learn about the transfer.hiderefs setting on the remote?
It has to be out-of-band, at which point calling it by the same name has
little benefit.
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] Hiding some refs in ls-remote
From: Jeff King @ 2013-01-21 23:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, spearce, mfick
In-Reply-To: <7vpq0zn2za.fsf@alter.siamese.dyndns.org>
On Sun, Jan 20, 2013 at 10:06:33AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> [uploadPack]
> >> hiderefs = refs/changes
> >
> > Would you want to do the same thing on receive-pack? It could benefit
> > from the same reduction in network cost (although it tends to be invoked
> > less frequently than upload-pack).
>
> Do *I* want to do? Not when there already is a patch exists; I'd
> rather take existing and tested patch ;-)
The patch we have is below, but I do not think you want it, as it is
missing several things:
- docs and tests
- handling multiple values
- anything but raw prefix matching (you even have to put in the "/"
yourself).
It was not my patch originally, and I never bothered to clean and
upstream it because I didn't think it was something anybody else would
be interested in. I'm happy to do so, but if you are working in the area
anyway, it would make sense to be part of your series.
-Peff
---
diff --git b/builtin/receive-pack.c a/builtin/receive-pack.c
index 0afb8b2..b22670c 100644
--- b/builtin/receive-pack.c
+++ a/builtin/receive-pack.c
@@ -39,6 +39,7 @@ static void *head_name_to_free;
static int auto_gc = 1;
static const char *head_name;
static void *head_name_to_free;
+static const char *hide_refs;
static int sent_capabilities;
static enum deny_action parse_deny_action(const char *var, const char *value)
@@ -113,11 +114,19 @@ static void show_ref(const char *path, const unsigned char *sha1)
return 0;
}
+ if (strcmp(var, "receive.hiderefs") == 0) {
+ git_config_string(&hide_refs, var, value);
+ return 0;
+ }
+
return git_default_config(var, value, cb);
}
static void show_ref(const char *path, const unsigned char *sha1)
{
+ if (hide_refs && strncmp(path, hide_refs, strlen(hide_refs)) == 0)
+ return 0;
+
if (sent_capabilities)
packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
else
^ permalink raw reply related
* Re: [PATCH v3 08/31] parse_pathspec: add PATHSPEC_EMPTY_MATCH_ALL
From: Martin von Zweigbergk @ 2013-01-21 23:12 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, Matthieu Moy
In-Reply-To: <1358080539-17436-9-git-send-email-pclouds@gmail.com>
Hi,
I was tempted to ask this before, and the recent thread regarding "add
-u/A" [1] convinced me to.
On Sun, Jan 13, 2013 at 4:35 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> We have two ways of dealing with empty pathspec:
>
> 1. limit it to current prefix
> 2. match the entire working directory
>
> Some commands go with #1, some with #2. get_pathspec() and
> parse_pathspec() only supports #1. Make it support #2 too via
> PATHSPEC_EMPTY_MATCH_ALL flag.
If #2 is indeed the direction we want to go, then maybe we should make
that the default behavior from parse_pathspec()? I.e. rename the flag
"PATHSPEC_EMPTY_MATCH_PREFIX" (or something). Makes sense?
Btw, Matthieu was asking where we use #1. If you do invert the name
and meaning of the flag, then the answer to that question should be
(mostly?) obvious from a re-roll of your series (i.e. all the places
where PATHSPEC_EMPTY_MATCH_PREFIX is used).
Martin
[1] http://thread.gmane.org/gmane.comp.version-control.git/213988/focus=214113
^ permalink raw reply
* Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content
From: Jeff King @ 2013-01-21 23:15 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1358768433-26096-1-git-send-email-pclouds@gmail.com>
On Mon, Jan 21, 2013 at 06:40:33PM +0700, Nguyen Thai Ngoc Duy wrote:
> + /*
> + * If it has the same content that we are going to overwrite,
> + * there's no point in complaining. We still overwrite it in
> + * the end though.
> + */
> + if (ce &&
> + S_ISREG(st->st_mode) && S_ISREG(ce->ce_mode) &&
> + (!trust_executable_bit ||
> + (0100 & (ce->ce_mode ^ st->st_mode)) == 0) &&
> + st->st_size < SAME_CONTENT_SIZE_LIMIT &&
> + sha1_object_info(ce->sha1, &ce_size) == OBJ_BLOB &&
> + ce_size == st->st_size) {
> + void *buffer = NULL;
> + unsigned long size;
> + enum object_type type;
> + struct strbuf sb = STRBUF_INIT;
> + int matched =
> + strbuf_read_file(&sb, ce->name, ce_size) == ce_size &&
> + (buffer = read_sha1_file(ce->sha1, &type, &size)) != NULL &&
> + type == OBJ_BLOB &&
> + size == ce_size &&
> + !memcmp(buffer, sb.buf, size);
> + free(buffer);
> + strbuf_release(&sb);
> + if (matched)
> + return 0;
> + }
Can you elaborate on when this code is triggered?
In the general case, shouldn't we already know the sha1 of what's on
disk in the index, and be able to just compare the hashes? And if we
don't, because the entry is start-dirty, should we be updating it
(possibly earlier, so we do not even get into the "need to write" code
path) instead of doing this ad-hoc byte comparison?
Confused...
-Peff
^ permalink raw reply
* Re: [RFC] git rm -u
From: Junio C Hamano @ 2013-01-21 23:18 UTC (permalink / raw)
To: Matthieu Moy
Cc: Piotr Krukowiecki, Jonathan Nieder, Eric James Michael Ritz,
Git Mailing List, Tomas Carnecky
In-Reply-To: <vpqhamapal1.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> But v1.5.2.5~1 (git-add -u paths... now works from subdirectory,
>> 2007-08-16) changed the semantics to limit the operation to the
>> working tree.
>
> Not really. It fixed "git add -u path", not plain "git add -u". A quick
> test checking out and compiling v1.5.2.5~1^ shows that "git add -u ."
> from a subdirectory was adding everything from the root.
>
> My interpretation is that v1.5.2.5~1 fixed an actual bug, without
> thinking about what would happen when "git add -u" was called without
> path, so the behavior is "what happens to be the most natural to
> implement".
I guess at this point it does not matter that much if that was an
unintended consequence of a buggy fix, or a new behaviour by design.
We initially were tree-wide but later limited the operation to the
current directory.
I think your "Check 'git diff' then run 'git add -u'" example may be
a good enough argument that it is a good idea to restore the
originally intended "tree-wide" behaviour in any case.
^ permalink raw reply
* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Junio C Hamano @ 2013-01-21 23:32 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git
In-Reply-To: <201301212330.10824.jn.avila@free.fr>
"Jean-Noël AVILA" <jn.avila@free.fr> writes:
> At least, "it works for me".
I suspect that your approach will still not fix the case in which
you build a branch with a new command git-check-ignore, and then
check out another branch that does not yet have that command without
first running "make clean".
Does the following really pass with your patch?
git checkout origin/next
make
git checkout origin/maint
git apply your_patch.mbox
make
cd t && sh ./t9902-completion.sh
> + __git_cmdlist () { git help -a| egrep -m 1 -B1000 PATH | egrep '^ [a-zA-Z0-9]'; }
'egrep' is not even in POSIX in the first place but grep -E ought to
be a replacement for it, so I'll let it pass, but "-m1 -B1000"?
Please stay within portable options.
git help -a |
sed -n -e '/^ [a-z]/p' -e '/^git commands available from elsewhere/q'/'
might be a good enough substitute, I think, if we were to take your
approach, but I suspect it needs a lot more to limit the output in
the test mode.
^ permalink raw reply
* Re: [PATCH 0/2] Hiding some refs in ls-remote
From: Junio C Hamano @ 2013-01-21 23:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, spearce, mfick
In-Reply-To: <20130121230108.GB17156@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> It may even make sense to have "git push" honor it, excluding them
>> from "git push --mirror" (or "git push --all" if some of the
>> branches are hidden); I haven't thought things through, though.
>
> That is harder, as that is something that happens on the client. How
> does the client learn about the transfer.hiderefs setting on the remote?
No, I was talking about running "git push" from a repository that
has the transfer.hiderefs set, emulating a fetch from a client by
pushing out in the reverse direction.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Jeff King @ 2013-01-21 23:40 UTC (permalink / raw)
To: Chris Rorvick
Cc: Junio C Hamano, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <CAEUsAPZr+bNNA-pqrQbBGvku4T3h58Ub66mK2zLeHqghEKw5Aw@mail.gmail.com>
On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:
> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King <peff@peff.net> wrote:
> > However, if instead of the rule being
> > "blobs on the remote side cannot be replaced", if it becomes "the old
> > value on the remote side must be referenced by what we replace it with",
> > that _is_ something we can calculate reliably on the sending side.
>
> Interesting. I would have thought knowing reachability implied having
> the old object in the sending repository.
No, because if you do not have it, then you know it is not reachable
from your refs (or your repository is corrupted). If you do have it, it
_might_ be reachable. For commits, checking is cheap (merge-base) and we
already do it. For trees and blobs, it is much more expensive, as you
have to walk the whole object graph. While it might be "more correct"
in some sense to say "it's OK to replace a tree with a commit that
points to it", in practice I doubt anyone cares, so you can probably
just punt on those ones and say "no, it's not a fast forward".
> > And
> > that is logically an extension of the fast-forward rule, which is why I
> > suggested placing it with ref_newer (but the latter should probably be
> > extended to not suggest merging if we _know_ it is a non-commit object).
>
> Sounds great, especially if it is not dependent on the sender actually
> having the old object. Until this is implemented, though, I don't
> understand what was wrong with doing the checks in the
> is_forwardable() helper function (of course after fixing the
> regression/bug.)
I don't think it is wrong per se; I just think that the check would go
more naturally where we are checking whether the object does indeed
fast-forward. Because is_forwardable in some cases must say "I don't
know; I don't have the object to check its type, so maybe it is
forwardable, and maybe it is not". Whereas when we do the actual
reachability check, we can say definitely "this is not reachable because
I don't have it, or this is not reachable because it is a commit and I
checked, or this might be reachable but I don't care to check because it
has a funny type".
I think looking at it as the latter makes it more obvious how to handle
the "maybe" situation (e.g., the bug in is_forwardable was hard to see).
Anyway, I do not care that much where it goes. To me, the important
thing is the error message. I do think the error "already exists" is a
reasonable one for refs/tags (we do not allow non-force pushes of
existing tags), but not necessarily for other cases, like trying to push
a blob over a blob. The problem there is not "already exists" but rather
"a blob is not something that can fast-forward". Using the existing
REJECT_NONFASTFORWARD is insufficient (because later code will recommend
pull-then-push, which is wrong). So I'd be in favor of creating a new
error status for it.
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] Hiding some refs in ls-remote
From: Jeff King @ 2013-01-21 23:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, spearce, mfick
In-Reply-To: <7v38xuf6w5.fsf@alter.siamese.dyndns.org>
On Mon, Jan 21, 2013 at 03:33:46PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> It may even make sense to have "git push" honor it, excluding them
> >> from "git push --mirror" (or "git push --all" if some of the
> >> branches are hidden); I haven't thought things through, though.
> >
> > That is harder, as that is something that happens on the client. How
> > does the client learn about the transfer.hiderefs setting on the remote?
>
> No, I was talking about running "git push" from a repository that
> has the transfer.hiderefs set, emulating a fetch from a client by
> pushing out in the reverse direction.
Ah. That seems a bit more questionable to me, as you are assuming that
the face that the repository shows to network clients is the same as it
would show when it is the client itself. That wouldn't be true, for
example, when pushing to a backup repository which would expect to get
everything.
Of course the same problem comes from a backup repository which wants to
fetch from you. Which again comes down to the fact that I think this
ref-hiding is really in the eye of the receiver, not the sender.
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-21 23:53 UTC (permalink / raw)
To: Jeff King
Cc: Chris Rorvick, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <20130121234002.GE17156@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... The problem there is not "already exists" but rather
> "a blob is not something that can fast-forward". Using the existing
> REJECT_NONFASTFORWARD is insufficient (because later code will recommend
> pull-then-push, which is wrong). So I'd be in favor of creating a new
> error status for it.
Very well said.
Please make it so ;-) or should I?
^ permalink raw reply
* Re: [PATCH v3 1/6] Change old system name 'GIT' to 'Git'
From: Junio C Hamano @ 2013-01-22 0:39 UTC (permalink / raw)
To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <1335904329.632749.1358795780375.JavaMail.ngmail@webmail20.arcor-online.net>
Thomas Ackermann <th.acker@arcor.de> writes:
> Git changed its 'official' system name from 'GIT' to 'Git' in v1.6.5.3
> (as can be seen in the corresponding release note where 'GIT' was
> changed to 'Git' in the header line). So change every occurrence
> of 'GIT" in the documention to 'Git'.
>
> Signed-off-by: Thomas Ackermann <th.acker@arcor.de>
> ---
This is more about "stop using poor-man's small caps".
I think it misses "GIT - the stupid content tracker" in README, but
probably it is OK (it is not an end-user facing documentation).
I noticed that these two places still use poor-man's small caps
after this patch.
* Documentation/SubmittingPatches:
that are being emailed around. Although core GIT is a lot
* Documentation/git-credential.txt:
TYPICAL USE OF GIT CREDENTIAL
With these two updated, I think it is a sensible change and the
patch is timed reasonably well. It applies to 'maint', and other
topics in flight do not seem to add new uses of GIT.
Not commenting on other 5 patches in the series yet, but if they
interact with other topics in flight, they may have to be separated
out. We'll see.
Thanks.
^ permalink raw reply
* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Jeff King @ 2013-01-22 0:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7v7gn6f6ya.fsf@alter.siamese.dyndns.org>
On Mon, Jan 21, 2013 at 03:32:29PM -0800, Junio C Hamano wrote:
> "Jean-Noël AVILA" <jn.avila@free.fr> writes:
>
> > At least, "it works for me".
>
> I suspect that your approach will still not fix the case in which
> you build a branch with a new command git-check-ignore, and then
> check out another branch that does not yet have that command without
> first running "make clean".
>
> Does the following really pass with your patch?
>
> git checkout origin/next
> make
> git checkout origin/maint
> git apply your_patch.mbox
> make
> cd t && sh ./t9902-completion.sh
I really hate to suggest this, but should it be more like:
if test -z "$FAKE_COMMAND_LIST"; then
__git_cmdlist() {
git help -a | egrep '^ [a-zA-Z0-9]'
}
else
__git_cmdlist() {
printf '%s' "$FAKE_COMMAND_LIST"
}
fi
That gives us a nice predictable starting point for actually testing the
completion code. The downside is that it doesn't let us test that we
remain compatible with the output of "help -a". But we could potentially
add a single, more liberal test (without $FAKE_COMMAND_LIST, but ready
to expect extra output) that checks that.
> > + __git_cmdlist () { git help -a| egrep -m 1 -B1000 PATH | egrep '^ [a-zA-Z0-9]'; }
>
> 'egrep' is not even in POSIX in the first place but grep -E ought to
> be a replacement for it, so I'll let it pass, but "-m1 -B1000"?
> Please stay within portable options.
If I recall correctly, egrep is actually more portable than "grep -E"
(and it is already in use, so I think we are OK). I agree on the rest,
though. :)
-Peff
^ permalink raw reply
* Re: [PATCH v3 2/6] Change 'git' to 'Git' whenever the whole system is referred to #1
From: Junio C Hamano @ 2013-01-22 0:41 UTC (permalink / raw)
To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <1430594044.632790.1358795873467.JavaMail.ngmail@webmail20.arcor-online.net>
Thomas Ackermann <th.acker@arcor.de> writes:
> Signed-off-by: Thomas Ackermann <th.acker@arcor.de>
> ---
>
Forgot --stat?
It helps to check the integrity of patch application and also helps
anticipating possible interaction with other topics in flight.
Please don't omit it.
^ permalink raw reply
* Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content
From: Duy Nguyen @ 2013-01-22 0:59 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20130121231515.GD17156@sigill.intra.peff.net>
On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
> Can you elaborate on when this code is triggered?
>
> In the general case, shouldn't we already know the sha1 of what's on
> disk in the index, and be able to just compare the hashes? And if we
> don't, because the entry is start-dirty, should we be updating it
> (possibly earlier, so we do not even get into the "need to write" code
> path) instead of doing this ad-hoc byte comparison?
>
> Confused...
git reset HEAD~10
# blah that was a mistake, undo it
git checkout HEAD@{1}
I hit it a few times, probably not with the exact recipe above though.
--
Duy
^ permalink raw reply
* Re: [RFC/PATCH] add: warn when -u or -A is used without filepattern
From: Duy Nguyen @ 2013-01-22 1:10 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, gitster, Jonathan Nieder, Eric James Michael Ritz,
Tomas Carnecky
In-Reply-To: <1358769611-3625-1-git-send-email-Matthieu.Moy@imag.fr>
On Mon, Jan 21, 2013 at 7:00 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> Most git commands that can be used with our without a filepattern are
> tree-wide by default, the filepattern being used to restrict their scope.
> A few exceptions are: 'git grep', 'git clean', 'git add -u' and 'git add -A'.
>
> The inconsistancy of 'git add -u' and 'git add -A' are particularly
> problematic since other 'git add' subcommands (namely 'git add -p' and
> 'git add -e') are tree-wide by default.
>
> Flipping the default now is unacceptable, so this patch starts training
> users to type explicitely 'git add -u|-A :/' or 'git add -u|-A .', to prepare
> for the next steps:
>
> * forbid 'git add -u|-A' without filepattern (like 'git add' without
> option)
>
> * much later, maybe, re-allow 'git add -u|-A' without filepattern, with a
> tree-wide scope.
What about 'grep' and 'clean'? I think at least 'clean' should go
tree-wide default too. I don't mind grep go the same way either but I
think people voiced preference in current behavior..
--
Duy
^ permalink raw reply
* Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content
From: Junio C Hamano @ 2013-01-22 1:45 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, git
In-Reply-To: <CACsJy8AFKXYkHbUf4aqBpCg+v06oFsvHq_zxQFE4BOCzTDAqtg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
>> Can you elaborate on when this code is triggered?
>>
>> In the general case, shouldn't we already know the sha1 of what's on
>> disk in the index, and be able to just compare the hashes? And if we
>> don't, because the entry is start-dirty, should we be updating it
>> (possibly earlier, so we do not even get into the "need to write" code
>> path) instead of doing this ad-hoc byte comparison?
If the index knows what is in the working tree, I think I agree.
>> Confused...
>
> git reset HEAD~10
> # blah that was a mistake, undo it
> git checkout HEAD@{1}
>
> I hit it a few times, probably not with the exact recipe above though.
I've seen myself doing "git reset HEAD~10" by mistake (I wanted "--hard"),
but the recovery is to do "git reset --hard @{1}" and then come back
with "git reset --hard HEAD~10", so it hasn't been a real problem
for me.
A case similar to this is already covered by a special case of
two-tree read-tree where the index already matches the tree we are
checking out even though it is different from HEAD; in other words,
if you did this:
git init
date >file
git add file; git commit -m 'has a file'
git checkout -b side
git rm file; git commit -m 'does not have the file'
git checkout master file
: now index has the file from 'master' and worktree is clean
git checkout master
you shouldn't get any complaint, I think.
If you did "git rm --cached file" to lose it from the index,
immediately after "git checkout master file", then we wouldn't know
what we may be losing. If the file in the working tree happens to
match the index and the HEAD after checking out the other branch
('master' in this case), it is _not_ losing information, so we might
be able to treat it as an extension of the existing special case.
I haven't thought things through to see if in a more general case
that this codepath triggers we might be losing a local changes that
we should be carrying forward while checking out a new branch,
though.
^ permalink raw reply
* Re: [RFC/PATCH] add: warn when -u or -A is used without filepattern
From: Junio C Hamano @ 2013-01-22 1:51 UTC (permalink / raw)
To: Duy Nguyen
Cc: Matthieu Moy, git, Jonathan Nieder, Eric James Michael Ritz,
Tomas Carnecky
In-Reply-To: <CACsJy8B1=3gMfGUf3kyea9TyZmr1J7dbM1_+huMNrep24hwuiQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> What about 'grep' and 'clean'? I think at least 'clean' should go
> tree-wide default too. I don't mind grep go the same way either but I
> think people voiced preference in current behavior..
I think the major argument for "git grep" to be the way it is is
because people expect it to be extension of running "grep -r" in the
same directory ("git grep" excludes untracked ones so you do not
have to suffer from --exclude=.git and such unpleasantries).
Shouldn't "git clean" be an extension of running "rm -r" in the same
directory ("git clean" does not lose tracked ones but otherwise it
is a recursive removal)?
^ permalink raw reply
* Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content
From: Duy Nguyen @ 2013-01-22 1:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vwqv6c7oe.fsf@alter.siamese.dyndns.org>
On Tue, Jan 22, 2013 at 8:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
>>> Can you elaborate on when this code is triggered?
>>>
>>> In the general case, shouldn't we already know the sha1 of what's on
>>> disk in the index, and be able to just compare the hashes? And if we
>>> don't, because the entry is start-dirty, should we be updating it
>>> (possibly earlier, so we do not even get into the "need to write" code
>>> path) instead of doing this ad-hoc byte comparison?
>
> If the index knows what is in the working tree, I think I agree.
>
>>> Confused...
>>
>> git reset HEAD~10
>> # blah that was a mistake, undo it
>> git checkout HEAD@{1}
>>
>> I hit it a few times, probably not with the exact recipe above though.
>
> I've seen myself doing "git reset HEAD~10" by mistake (I wanted "--hard"),
> but the recovery is to do "git reset --hard @{1}" and then come back
> with "git reset --hard HEAD~10", so it hasn't been a real problem
> for me.
Except when you already make some changes elsewhere and you don't want --hard.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/2] Update :/abc ambiguity check
From: Duy Nguyen @ 2013-01-22 2:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vsj5ufia6.fsf@alter.siamese.dyndns.org>
On Tue, Jan 22, 2013 at 2:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> :/abc may mean two things:
>>
>> - as a revision, it means the revision that has "abc" in commit
>> message.
>>
>> - as a pathpec, it means "abc" from root.
>>
>> Currently we see ":/abc" as a rev (most of the time), but never see it
>> as a pathspec even if "abc" exists and "git log :/abc" will gladly
>> take ":/abc" as rev even it's ambiguous. This patch makes it:
>>
>> - ambiguous when "abc" exists on worktree
>> - a rev if abc does not exist on worktree
>> - a path if abc is not found in any commits (although better use
>
> The "any commits" above sounds very scary. Are you really going to
> check against all the commits?
If I remember correctly :/ will search through commit chains until it
finds a commit that matches. So :/non-existent-string definitely
searches through all commits.
>> "--" to avoid ambiguation because searching through commit DAG is
>> expensive)
>>
>> A plus from this patch is, because ":/" never matches anything as a
>> rev, it is never considered a valid rev and because root directory
>> always exists, ":/" is always unambiguously seen as a pathspec.
>
> That is the primary plus in practice, I think, and it is a big one.
>
> When naming a directory that belongs to a different subdirectory
> hierarchy, typing ":/that/directory/name" is not any easier than
> having your shell help you complete "../../that/directory/name"; I
> suspect nobody uses the relative-to-root notation to name anything
> but the root in real life.
As I noted in the patch comment, I do copy/paste repo-absolute paths
from a diff quite often (just skip the "a" and "b" prefix). Sometimes
I hope "git diff" has an option to produce relative paths..
--
Duy
^ permalink raw reply
* Re: [PATCH v3 08/31] parse_pathspec: add PATHSPEC_EMPTY_MATCH_ALL
From: Duy Nguyen @ 2013-01-22 2:46 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git, Junio C Hamano, Matthieu Moy
In-Reply-To: <CANiSa6gfTxOWzMg7V19PntDBhU4kW6qpa+K2XjnWgzNRXDKRSw@mail.gmail.com>
On Tue, Jan 22, 2013 at 6:12 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> Hi,
>
> I was tempted to ask this before, and the recent thread regarding "add
> -u/A" [1] convinced me to.
>
> On Sun, Jan 13, 2013 at 4:35 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> We have two ways of dealing with empty pathspec:
>>
>> 1. limit it to current prefix
>> 2. match the entire working directory
>>
>> Some commands go with #1, some with #2. get_pathspec() and
>> parse_pathspec() only supports #1. Make it support #2 too via
>> PATHSPEC_EMPTY_MATCH_ALL flag.
>
> If #2 is indeed the direction we want to go, then maybe we should make
> that the default behavior from parse_pathspec()? I.e. rename the flag
> "PATHSPEC_EMPTY_MATCH_PREFIX" (or something). Makes sense?
No problem with me. Will do unless someone objects.
> Btw, Matthieu was asking where we use #1. If you do invert the name
> and meaning of the flag, then the answer to that question should be
> (mostly?) obvious from a re-roll of your series (i.e. all the places
> where PATHSPEC_EMPTY_MATCH_PREFIX is used).
>
> Martin
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/213988/focus=214113
--
Duy
^ permalink raw reply
* Re: [PATCH 1/2] Update :/abc ambiguity check
From: Junio C Hamano @ 2013-01-22 4:10 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8B1uxbJvP+0ZEx3br9_Qr9ZX7num8bcgd5sFS7XnvGNpw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
>>> ... take ":/abc" as rev even it's ambiguous. This patch makes it:
>>>
>>> - ambiguous when "abc" exists on worktree
>>> - a rev if abc does not exist on worktree
>>> - a path if abc is not found in any commits (although better use
>>
>> The "any commits" above sounds very scary. Are you really going to
>> check against all the commits?
>
> If I remember correctly :/ will search through commit chains until it
> finds a commit that matches. So :/non-existent-string definitely
> searches through all commits.
That is the real work the user asked us to do, so it is not a wasted
latency. The description looked as if you were doing extra work
only for disambiguation, which triggered my "Huh?" meter.
^ permalink raw reply
* Re: Lockless Refs? (Was [PATCH] refs: do not use cached refs in repack_without_ref)
From: Drew Northup @ 2013-01-22 4:31 UTC (permalink / raw)
To: Jeff King
Cc: Martin Fick, Michael Haggerty, Shawn Pearce, git, Junio C Hamano
In-Reply-To: <20130105161215.GA24900@sigill.intra.peff.net>
On Sat, Jan 5, 2013 at 11:12 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 31, 2012 at 03:30:53AM -0700, Martin Fick wrote:
>
>> The general approach is to setup a transaction and either
>> commit or abort it. A transaction can be setup by renaming
>> an appropriately setup directory to the "ref.lock" name. If
>> the rename succeeds, the transaction is begun. Any actor can
>> abort the transaction (up until it is committed) by simply
>> deleting the "ref.lock" directory, so it is not at risk of
>> going stale.
>
> Deleting a directory is not atomic, as you first have to remove the
> contents, putting it into a potentially inconsistent state. I'll assume
> you deal with that later...
I know I'm a bit late to the dance here, but FWIW the apparent atomicy
(odd conjugation there) of directory deletion depends largely upon the
filesystem and VFS api in use. It is not unheard of that a delete
operation actually consist of moving the reference to the item's own
allocation marker into a "trashcan" to be cleaned up after later.
In other words, I'd not advise planning on directory deletes always
being atomic nor always not being atomic.
--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Junio C Hamano @ 2013-01-22 4:31 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <20130122003954.GA23297@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I really hate to suggest this, but should it be more like:
>
> if test -z "$FAKE_COMMAND_LIST"; then
> __git_cmdlist() {
> git help -a | egrep '^ [a-zA-Z0-9]'
> }
> else
> __git_cmdlist() {
> printf '%s' "$FAKE_COMMAND_LIST"
> }
> fi
>
> That gives us a nice predictable starting point for actually testing the
> completion code. The downside is that it doesn't let us test that we
> remain compatible with the output of "help -a".
Yeah, I think this is simpler and more to the point for the test in
t9902. If we really want to test something that is the same as, or
at least any closer than this approach (or my "help --standard"), to
what the real users use, the test has to become inherently flaky, so
I think we should go for the simplicity of this patch shows.
^ 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