* Re: [PATCH 3/3] Convert all fnmatch() calls to wildmatch()
From: Nguyen Thai Ngoc Duy @ 2012-12-20 12:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8v8tanyn.fsf@alter.siamese.dyndns.org>
On Thu, Dec 20, 2012 at 1:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -627,7 +628,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
>> return entry_interesting;
>>
>> if (item->use_wildcard) {
>> - if (!fnmatch(match + baselen, entry->path, 0))
>> + if (!wildmatch(match + baselen, entry->path, 0))
>> return entry_interesting;
>
> This and the change to dir.c obviously have interactions with
> 8c6abbc (pathspec: apply "*.c" optimization from exclude,
> 2012-11-24).
>
> I've already alluded to it in my response to 2/3, I guess.
Conflict of plans. I did not expect myself to work on replacing
fnmatch soon and git_fnmatch is an intermediate step to collect more
optimizations and gradually replace fnmatch. Eventually git_fnmatch()
would become a wrapper of wildmatch, all the optimizations are pushed
down there. If we replace fnmatch->wildmatch first then there's little
reason for the existence of git_fnmatch(). Maybe I should merge this
with the fnmatch elimination into a single series.
--
Duy
^ permalink raw reply
* Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
From: Joachim Schmitz @ 2012-12-20 8:20 UTC (permalink / raw)
To: git
In-Reply-To: <7v623x6xvv.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> Use mktemp to create the /dev/null placeholder for p4merge.
>> This keeps it out of the current directory.
>>
>> Reported-by: Jeremy Morton <admin@game-point.net>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> I consider this a final finishing touch on a new 1.8.1 feature,
>> so hopefully we can get this in before 1.8.1.
>
> Does everybody have mktemp(1), which is not even in POSIX.1?
HP-NonStop doesn't have it, unless you're on the latest release, which added
GNU coreutils.
Bye, Jojo
^ permalink raw reply
* Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
From: David Aguilar @ 2012-12-20 8:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v623x6xvv.fsf@alter.siamese.dyndns.org>
On Wed, Dec 19, 2012 at 10:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> Use mktemp to create the /dev/null placeholder for p4merge.
>> This keeps it out of the current directory.
>>
>> Reported-by: Jeremy Morton <admin@game-point.net>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> I consider this a final finishing touch on a new 1.8.1 feature,
>> so hopefully we can get this in before 1.8.1.
>
> Does everybody have mktemp(1), which is not even in POSIX.1?
>
> I'm a bit hesitant to apply this to the upcoming release without
> cooking it in 'next' for sufficiently long time to give it a chance
> to be tried by wider audience.
True. I only tried Linux and Mac OS X, so I would not be
surprised to find some exotic UNIX that does not have mktemp.
I meant to write "is this portable?" in the section after
the double-dash; saying that it's polish for a fix
is only true if it's portable.
The git-unpack thing looks interesting...
is the SHA-1 in your example the special SHA-1 for an
empty tree or blob?
The reason I ask is because in this code path we are
comparing an unknown blob, basically a blob that does
not exist in one of the trees, so I'm not sure if an
'unpack' command would help for this case because we
would not have a blob SHA-1 to unpack.
As far as portability goes, the "UNIX" list
for p4merge is here:
http://www.perforce.com/downloads/complete_list
I do not have AIX, Solaris, or FreeBSD to test,
so I agree that this can wait.
Does msysgit have mktemp?
p4merge is available on Windows too so I would
need to check there too unless someone happens
to know off the top of their heads.
My other thought was to write a simple shell
function that checks TMPDIR itself, and defaults
to creating some file under /tmp when it is undefined.
I can pursue this option if you think it's a safer choice.
--
David
^ permalink raw reply
* Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
From: Junio C Hamano @ 2012-12-20 7:07 UTC (permalink / raw)
To: David Aguilar; +Cc: git
In-Reply-To: <7v623x6xvv.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> David Aguilar <davvid@gmail.com> writes:
>
>> Use mktemp to create the /dev/null placeholder for p4merge.
>> This keeps it out of the current directory.
>>
>> Reported-by: Jeremy Morton <admin@game-point.net>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> I consider this a final finishing touch on a new 1.8.1 feature,
>> so hopefully we can get this in before 1.8.1.
>
> Does everybody have mktemp(1), which is not even in POSIX.1?
>
> I'm a bit hesitant to apply this to the upcoming release without
> cooking it in 'next' for sufficiently long time to give it a chance
> to be tried by wider audience.
One approach may be to do something like this as a preparation step
(current callers of unpack-file may want to do their temporary work
in TMPDIR as well), and then use
git unpack-file -t e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
to create your empty temporary file.
Documentation/git-unpack-file.txt | 10 +++++++---
builtin/unpack-file.c | 28 +++++++++++++++++++++-------
2 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-unpack-file.txt b/Documentation/git-unpack-file.txt
index e9f148a..56af328 100644
--- a/Documentation/git-unpack-file.txt
+++ b/Documentation/git-unpack-file.txt
@@ -10,16 +10,20 @@ git-unpack-file - Creates a temporary file with a blob's contents
SYNOPSIS
--------
[verse]
-'git unpack-file' <blob>
+'git unpack-file' [-t] <blob>
DESCRIPTION
-----------
Creates a file holding the contents of the blob specified by sha1. It
-returns the name of the temporary file in the following format:
- .merge_file_XXXXX
+returns the name of the temporary file (by default `.merge_file_XXXXX`).
+
OPTIONS
-------
+-t::
+ The temporary file is created in `$TMPDIR` directory,
+ instead of the current directory.
+
<blob>::
Must be a blob id
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 1920029..de1f845 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -1,8 +1,9 @@
#include "builtin.h"
-static char *create_temp_file(unsigned char *sha1)
+static char *create_temp_file(unsigned char *sha1, int in_tempdir)
{
- static char path[50];
+ static char path[1024];
+ static const char template[] = ".merge_file_XXXXXX";
void *buf;
enum object_type type;
unsigned long size;
@@ -12,8 +13,12 @@ static char *create_temp_file(unsigned char *sha1)
if (!buf || type != OBJ_BLOB)
die("unable to read blob object %s", sha1_to_hex(sha1));
- strcpy(path, ".merge_file_XXXXXX");
- fd = xmkstemp(path);
+ if (in_tempdir) {
+ fd = git_mkstemp(path, sizeof(path) - 1, template);
+ } else {
+ strcpy(path, template);
+ fd = xmkstemp(path);
+ }
if (write_in_full(fd, buf, size) != size)
die_errno("unable to write temp-file");
close(fd);
@@ -23,14 +28,23 @@ static char *create_temp_file(unsigned char *sha1)
int cmd_unpack_file(int argc, const char **argv, const char *prefix)
{
unsigned char sha1[20];
+ int in_tempdir = 0;
+
+ if (argc < 2 || 3 < argc || !strcmp(argv[1], "-h"))
+ usage("git unpack-file [-t] <sha1>");
+ if (argc == 3) {
+ if (strcmp(argv[1], "-t"))
+ usage("git unpack-file [-t] <sha1>");
+ in_tempdir = 1;
+ argc--;
+ argv++;
+ }
- if (argc != 2 || !strcmp(argv[1], "-h"))
- usage("git unpack-file <sha1>");
if (get_sha1(argv[1], sha1))
die("Not a valid object name %s", argv[1]);
git_config(git_default_config, NULL);
- puts(create_temp_file(sha1));
+ puts(create_temp_file(sha1, in_tempdir));
return 0;
}
^ permalink raw reply related
* Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
From: Junio C Hamano @ 2012-12-20 6:28 UTC (permalink / raw)
To: David Aguilar; +Cc: git
In-Reply-To: <1355978754-7041-1-git-send-email-davvid@gmail.com>
David Aguilar <davvid@gmail.com> writes:
> Use mktemp to create the /dev/null placeholder for p4merge.
> This keeps it out of the current directory.
>
> Reported-by: Jeremy Morton <admin@game-point.net>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> I consider this a final finishing touch on a new 1.8.1 feature,
> so hopefully we can get this in before 1.8.1.
Does everybody have mktemp(1), which is not even in POSIX.1?
I'm a bit hesitant to apply this to the upcoming release without
cooking it in 'next' for sufficiently long time to give it a chance
to be tried by wider audience.
> mergetools/p4merge | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/mergetools/p4merge b/mergetools/p4merge
> index 295361a..090fa9b 100644
> --- a/mergetools/p4merge
> +++ b/mergetools/p4merge
> @@ -4,13 +4,13 @@ diff_cmd () {
> rm_remote=
> if test "/dev/null" = "$LOCAL"
> then
> - LOCAL="./p4merge-dev-null.LOCAL.$$"
> + LOCAL="$(create_empty_file)"
> >"$LOCAL"
> rm_local=true
> fi
> if test "/dev/null" = "$REMOTE"
> then
> - REMOTE="./p4merge-dev-null.REMOTE.$$"
> + REMOTE="$(create_empty_file)"
> >"$REMOTE"
> rm_remote=true
> fi
> @@ -33,3 +33,7 @@ merge_cmd () {
> "$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
> check_unchanged
> }
> +
> +create_empty_file () {
> + mktemp -t git-difftool-p4merge-empty-file.XXXXXX
> +}
^ permalink raw reply
* Re: [PATCH] add core.pathspecGlob config option
From: Junio C Hamano @ 2012-12-20 5:26 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyen Thai Ngoc Duy, git
In-Reply-To: <20121220035543.GA14965@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Dec 19, 2012 at 07:51:16PM -0800, Junio C Hamano wrote:
>
>> > ++ if (limit_pathspec_to_literal())
>> > ++ item->nowildcard_len = item->len;
>> > ++ else {
>> > ++ item->nowildcard_len = simple_length(path);
>> > ++ if (item->nowildcard_len < item->len) {
>> > ++ pathspec->has_wildcard = 1;
>> > ++ if (path[item->nowildcard_len] == '*' &&
>> > ++ no_wildcard(path + item->nowildcard_len + 1))
>> > ++ item->flags |= PATHSPEC_ONESTAR;
>> > ++ }
>> > + }
>>
>> Hmph. I thought that returning the length without any "stop at glob
>> special" trick from simple_length() would be a simpler resolution.
>>
>> That is what is queued at the tip of 'pu', anyway.
>
> I don't think we can make a change in simple_length. It gets used not
> only for pathspecs, but also for parsing exclude patterns, which I do
> not think should be affected by this option.
Ouch, yeah, you're right.
^ permalink raw reply
* Re: [RFC] test: Old shells and physical paths
From: David Aguilar @ 2012-12-20 5:01 UTC (permalink / raw)
To: David Michael; +Cc: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <CAEvUa7=_iyXxaaRs3WtxZOy5PNnncG-iMAUNkCMLJ19ZtReqaw@mail.gmail.com>
On Wed, Dec 19, 2012 at 6:28 PM, David Michael <fedora.dm0@gmail.com> wrote:
> Hi,
>
> On Thu, Dec 20, 2012 at 12:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Is "here is a nickel, get a better shell" an option?
>
> It is, somewhat. There is a pre-built port of GNU bash 2.03 for the
> platform, but I was trying to see how far things could go with the
> OS's supported shell before having to bring in unsupported
> dependencies. Unfortunately, I do not believe the OS fully conforms
> to POSIX.1-2001 yet, so that means no "-P" or "-L" without going
> rogue.
>
> I'll carry test fixes for this platform locally.
Do you know if the differences are relegated to "cd",
or do other common commands such as awk, grep, sed, mktemp, expr,
etc. have similar issues?
I wonder if it'd be helpful to have a low-numbered test that checks
the basics needed by the scripted Porcelains and test suite.
It would give us an easy way to answer these questions, and could
be a good way to document (in code) the capabilities we expect.
--
David
^ permalink raw reply
* [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
From: David Aguilar @ 2012-12-20 4:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Use mktemp to create the /dev/null placeholder for p4merge.
This keeps it out of the current directory.
Reported-by: Jeremy Morton <admin@game-point.net>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
I consider this a final finishing touch on a new 1.8.1 feature,
so hopefully we can get this in before 1.8.1.
mergetools/p4merge | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 295361a..090fa9b 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -4,13 +4,13 @@ diff_cmd () {
rm_remote=
if test "/dev/null" = "$LOCAL"
then
- LOCAL="./p4merge-dev-null.LOCAL.$$"
+ LOCAL="$(create_empty_file)"
>"$LOCAL"
rm_local=true
fi
if test "/dev/null" = "$REMOTE"
then
- REMOTE="./p4merge-dev-null.REMOTE.$$"
+ REMOTE="$(create_empty_file)"
>"$REMOTE"
rm_remote=true
fi
@@ -33,3 +33,7 @@ merge_cmd () {
"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
check_unchanged
}
+
+create_empty_file () {
+ mktemp -t git-difftool-p4merge-empty-file.XXXXXX
+}
--
1.8.1.rc2.6.g18499ba
^ permalink raw reply related
* Re: [PATCH] mergetools/p4merge: Handle "/dev/null"
From: David Aguilar @ 2012-12-20 4:41 UTC (permalink / raw)
To: Jeremy Morton; +Cc: Junio C Hamano, git
In-Reply-To: <508B9F89.7050909@game-point.net>
On Sat, Oct 27, 2012 at 1:47 AM, Jeremy Morton <admin@game-point.net> wrote:
> Sorry to be replying to this so late; I hadn't noticed the post until now!
>
> I've tried putting that code in my p4merge script and yes it does indeed
> work fine. However, it puts a temporary file in the working directory which
> I'm not sure is a good idea? If we look at this patch which actually solved
> pretty much the same problem, but when merging and, during a merge conflict,
> a file was created in both branches:
> https://github.com/git/git/commit/ec245ba
>
> ... it is creating a temp file in a proper temp dir, rather than in the
> working dir. I think that would be the proper solution here. However, I
> really want to get this fixed so I'd be happy for this band-aid fix of the
> p4merge script to be checked in until we could get a patch more like the
> aforementioned one, at a later date, to create empty files in a proper temp
> dir and pass them as $LOCAL and $REMOTE. :-)
I had the same thoughts when I wrote it, but I figured that following
the existing pattern used by mergetool for $REMOTE and $LOCAL when
they do exist was simpler as the first step.
I have a patch that fixes this by using mktemp that I will send shortly.
It only does it for the /dev/null file since the existing behavior for
files that do exist is fine.
--
David
^ permalink raw reply
* Re: [PATCH] add core.pathspecGlob config option
From: Jeff King @ 2012-12-20 4:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, git
In-Reply-To: <20121220040602.GA15172@sigill.intra.peff.net>
On Wed, Dec 19, 2012 at 11:06:02PM -0500, Jeff King wrote:
> > I don't think we can make a change in simple_length. It gets used not
> > only for pathspecs, but also for parsing exclude patterns, which I do
> > not think should be affected by this option.
>
> Our test suite wouldn't catch such a misfeature, of course, because the
> feature is not turned on by default. But I found it instructive to run
> all of the tests with GIT_LITERAL_PATHSPECS on. There are failures, of
> course, but by inspecting each failure you can see that it is an
> intended effect of the patch (i.e., each tries to use a wildcard
> pathspec, which no longer works).
>
> When you suggested changing common_prefix, I ran such a test both with
> and without the change[1] and confirmed that it did not change the set
> of failure sites. I did not try it, but I suspect running such a test
> with the tip of pu would reveal new failures in the .gitignore tests.
I just tried it, and indeed, running the test suite with this patch:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 256f1c6..1c43593 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -102,6 +102,9 @@ export EDITOR
export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
export EDITOR
+GIT_LITERAL_PATHSPECS=1
+export GIT_LITERAL_PATHSPECS
+
# Add libc MALLOC and MALLOC_PERTURB test
# only if we are not executing the test with valgrind
if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
produces many more failures on "pu" than it does on jk/pathspec-literal.
-Peff
^ permalink raw reply related
* Re: [PATCH] add core.pathspecGlob config option
From: Jeff King @ 2012-12-20 4:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, git
In-Reply-To: <20121220035543.GA14965@sigill.intra.peff.net>
On Wed, Dec 19, 2012 at 10:55:43PM -0500, Jeff King wrote:
> On Wed, Dec 19, 2012 at 07:51:16PM -0800, Junio C Hamano wrote:
>
> > > ++ if (limit_pathspec_to_literal())
> > > ++ item->nowildcard_len = item->len;
> > > ++ else {
> > > ++ item->nowildcard_len = simple_length(path);
> > > ++ if (item->nowildcard_len < item->len) {
> > > ++ pathspec->has_wildcard = 1;
> > > ++ if (path[item->nowildcard_len] == '*' &&
> > > ++ no_wildcard(path + item->nowildcard_len + 1))
> > > ++ item->flags |= PATHSPEC_ONESTAR;
> > > ++ }
> > > + }
> >
> > Hmph. I thought that returning the length without any "stop at glob
> > special" trick from simple_length() would be a simpler resolution.
> >
> > That is what is queued at the tip of 'pu', anyway.
>
> I don't think we can make a change in simple_length. It gets used not
> only for pathspecs, but also for parsing exclude patterns, which I do
> not think should be affected by this option.
Our test suite wouldn't catch such a misfeature, of course, because the
feature is not turned on by default. But I found it instructive to run
all of the tests with GIT_LITERAL_PATHSPECS on. There are failures, of
course, but by inspecting each failure you can see that it is an
intended effect of the patch (i.e., each tries to use a wildcard
pathspec, which no longer works).
When you suggested changing common_prefix, I ran such a test both with
and without the change[1] and confirmed that it did not change the set
of failure sites. I did not try it, but I suspect running such a test
with the tip of pu would reveal new failures in the .gitignore tests.
-Peff
[1] This is in addition to reading and reasoning about the code, of
course. I would not consider this a very robust form of testing,
though a test failure which cannot be easily explained would be a
good starting point for investigation.
^ permalink raw reply
* Re: [PATCH] add core.pathspecGlob config option
From: Jeff King @ 2012-12-20 3:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, git
In-Reply-To: <7vehil7557.fsf@alter.siamese.dyndns.org>
On Wed, Dec 19, 2012 at 07:51:16PM -0800, Junio C Hamano wrote:
> > ++ if (limit_pathspec_to_literal())
> > ++ item->nowildcard_len = item->len;
> > ++ else {
> > ++ item->nowildcard_len = simple_length(path);
> > ++ if (item->nowildcard_len < item->len) {
> > ++ pathspec->has_wildcard = 1;
> > ++ if (path[item->nowildcard_len] == '*' &&
> > ++ no_wildcard(path + item->nowildcard_len + 1))
> > ++ item->flags |= PATHSPEC_ONESTAR;
> > ++ }
> > + }
>
> Hmph. I thought that returning the length without any "stop at glob
> special" trick from simple_length() would be a simpler resolution.
>
> That is what is queued at the tip of 'pu', anyway.
I don't think we can make a change in simple_length. It gets used not
only for pathspecs, but also for parsing exclude patterns, which I do
not think should be affected by this option.
-Peff
^ permalink raw reply
* Re: [PATCH] add core.pathspecGlob config option
From: Junio C Hamano @ 2012-12-20 3:51 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyen Thai Ngoc Duy, git
In-Reply-To: <20121220031327.GB9917@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Dec 20, 2012 at 08:28:57AM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> > So I think this is a nice, simple approach for sites that want it, and
>> > noglob magic can come later (and will not be any harder to implement as
>> > a result of this patch).
>>
>> Any chance to make use of nd/pathspec-wildcard? It changes the same
>> code path in match_one. If you base on top of nd/pathspec-wildcard,
>> all you have to do is assign nowildcard_len to len (i.e. no wildcard
>> part).
>
> I'd rather keep it separate for now. One, just because they really are
> independent topics, and two, because I am actually back-porting it for
> GitHub (we are fairly conservative about upgrading our backend git
> versions, as most of the interesting stuff happens on the client side; I
> cherry-pick critical patches with no regard to the release cycle).
>
> And the resolution is pretty trivial, too. It looks like this:
>
> diff --cc dir.c
> index 5c0e5f6,03ff36b..81cb439
> --- a/dir.c
> +++ b/dir.c
> @@@ -1456,14 -1433,10 +1460,18 @@@ int init_pathspec(struct pathspec *path
>
> item->match = path;
> item->len = strlen(path);
> - item->nowildcard_len = simple_length(path);
> - item->use_wildcard = !limit_pathspec_to_literal() &&
> - !no_wildcard(path);
> - if (item->use_wildcard)
> - pathspec->has_wildcard = 1;
> + item->flags = 0;
> - if (item->nowildcard_len < item->len) {
> - pathspec->has_wildcard = 1;
> - if (path[item->nowildcard_len] == '*' &&
> - no_wildcard(path + item->nowildcard_len + 1))
> - item->flags |= PATHSPEC_ONESTAR;
> ++ if (limit_pathspec_to_literal())
> ++ item->nowildcard_len = item->len;
> ++ else {
> ++ item->nowildcard_len = simple_length(path);
> ++ if (item->nowildcard_len < item->len) {
> ++ pathspec->has_wildcard = 1;
> ++ if (path[item->nowildcard_len] == '*' &&
> ++ no_wildcard(path + item->nowildcard_len + 1))
> ++ item->flags |= PATHSPEC_ONESTAR;
> ++ }
> + }
> }
>
> qsort(pathspec->items, pathspec->nr,
Hmph. I thought that returning the length without any "stop at glob
special" trick from simple_length() would be a simpler resolution.
That is what is queued at the tip of 'pu', anyway.
^ permalink raw reply
* Re: [PATCH] add core.pathspecGlob config option
From: Jeff King @ 2012-12-20 3:13 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8BB3=3ZHD5Ua9M-0+98JVigHBBuo07gBSgEwanvB0zBSA@mail.gmail.com>
On Thu, Dec 20, 2012 at 08:28:57AM +0700, Nguyen Thai Ngoc Duy wrote:
> > So I think this is a nice, simple approach for sites that want it, and
> > noglob magic can come later (and will not be any harder to implement as
> > a result of this patch).
>
> Any chance to make use of nd/pathspec-wildcard? It changes the same
> code path in match_one. If you base on top of nd/pathspec-wildcard,
> all you have to do is assign nowildcard_len to len (i.e. no wildcard
> part).
I'd rather keep it separate for now. One, just because they really are
independent topics, and two, because I am actually back-porting it for
GitHub (we are fairly conservative about upgrading our backend git
versions, as most of the interesting stuff happens on the client side; I
cherry-pick critical patches with no regard to the release cycle).
And the resolution is pretty trivial, too. It looks like this:
diff --cc dir.c
index 5c0e5f6,03ff36b..81cb439
--- a/dir.c
+++ b/dir.c
@@@ -1456,14 -1433,10 +1460,18 @@@ int init_pathspec(struct pathspec *path
item->match = path;
item->len = strlen(path);
- item->nowildcard_len = simple_length(path);
- item->use_wildcard = !limit_pathspec_to_literal() &&
- !no_wildcard(path);
- if (item->use_wildcard)
- pathspec->has_wildcard = 1;
+ item->flags = 0;
- if (item->nowildcard_len < item->len) {
- pathspec->has_wildcard = 1;
- if (path[item->nowildcard_len] == '*' &&
- no_wildcard(path + item->nowildcard_len + 1))
- item->flags |= PATHSPEC_ONESTAR;
++ if (limit_pathspec_to_literal())
++ item->nowildcard_len = item->len;
++ else {
++ item->nowildcard_len = simple_length(path);
++ if (item->nowildcard_len < item->len) {
++ pathspec->has_wildcard = 1;
++ if (path[item->nowildcard_len] == '*' &&
++ no_wildcard(path + item->nowildcard_len + 1))
++ item->flags |= PATHSPEC_ONESTAR;
++ }
+ }
}
qsort(pathspec->items, pathspec->nr,
Not re-indenting the conditional would make the diff more readable, but
I think the resulting code is simpler to read if all of the wildcard
stuff is inside the "else" block.
-Peff
^ permalink raw reply
* Re: sys/param.h
From: Junio C Hamano @ 2012-12-20 3:03 UTC (permalink / raw)
To: Mark Levedahl; +Cc: git
In-Reply-To: <50D27CC6.3000203@gmail.com>
Thanks.
^ permalink raw reply
* Re: sys/param.h
From: Mark Levedahl @ 2012-12-20 2:49 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, git
In-Reply-To: <CABPQNSZQk6hHm-dWqFFQf0HF34Mvbjc8-mgzCr=G0zbBKiYUvA@mail.gmail.com>
On 12/19/2012 02:59 AM, Erik Faye-Lund wrote:
> On Tue, Dec 18, 2012 at 6:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>
>>>> Junio C Hamano wrote:
>>>>> It could turn out that we may be able to get rid of sys/param.h
>>>>> altogether, but one step at a time. Inputs from people on minority
>>>>> platforms are very much appreciated---does your platform build fine
>>>>> when the inclusion of the file is removed from git-compat-util.h?
cygwin is fine with that removed.
Mark
^ permalink raw reply
* Re: [RFC] test: Old shells and physical paths
From: David Michael @ 2012-12-20 2:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <7vmwx97f0o.fsf@alter.siamese.dyndns.org>
Hi,
On Thu, Dec 20, 2012 at 12:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Is "here is a nickel, get a better shell" an option?
It is, somewhat. There is a pre-built port of GNU bash 2.03 for the
platform, but I was trying to see how far things could go with the
OS's supported shell before having to bring in unsupported
dependencies. Unfortunately, I do not believe the OS fully conforms
to POSIX.1-2001 yet, so that means no "-P" or "-L" without going
rogue.
I'll carry test fixes for this platform locally.
Thanks.
David
^ permalink raw reply
* Re: [PATCH 2/3] wildmatch: support "no FNM_PATHNAME" mode
From: Nguyen Thai Ngoc Duy @ 2012-12-20 1:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vlicu9cpk.fsf@alter.siamese.dyndns.org>
On Thu, Dec 20, 2012 at 12:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> When that happens, we should want to retain the same "do not bother
> to descend into subdirectories that will never match" optimization
> for a pattern like "Doc*tion/**/*.txt". Because of FNM_PATHNAME, we
> can tell if a subdirectory is worth descending into by looking at
> the not-so-simple prefix "Doc*tion/"; "Documentation" will match,
> "Doc" will not (because '*' won't match '/').
>
> Which tells me that integrating this _well_ into the rest of the
> system is not just a matter of replacing fnmatch() with wildmatch().
Yeah, we open a door for more opportunities and a lot more headache.
> I also expect that wildmatch() would be much slower than fnmatch()
> especially when doing its "**" magic, but I personally do not think
> it will be a showstopper.
A potential showstopper is the lack of multibyte support. I don't know
if people want that though.
> If the user asks for a more powerful but
> expensive operation, we are saving time for the user by doing a more
> powerful thing (reducing the need to postprocess the results) and
> can afford to spend extra cycles.
In some case we may be able to spend fewer cycles by preprocessing
patterns first.
> As long as simpler patterns fnmatch() groks (namely, '?', '*', and
> '[class]' wildcards only) are not slowed down by replacing it with
> wildmatch(), that is, of course.
I'm concerned about performance vs fnmatch too. I'll probably write a
small program to exercise both and measure it with glibc.
--
Duy
^ permalink raw reply
* Re: [PATCH] add core.pathspecGlob config option
From: Nguyen Thai Ngoc Duy @ 2012-12-20 1:28 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20121219203449.GA10001@sigill.intra.peff.net>
On Thu, Dec 20, 2012 at 3:34 AM, Jeff King <peff@peff.net> wrote:
> Part of me thinks this is just gross, because ":(noglob)" is the right
> solution. But after spending a few hours trying it this morning, there
> is a ton of refactoring required to make it work correctly everywhere
> (although we could die() if we see it in places that are not using
> init_pathspec, so at least we could say "not supported yet" and not just
> silently ignore it).
Yep, I'm still half way to converting everything to the new pathspec
code. I'm not there yet. And things like this probably better goes
with a config key or command line option, appending :(noglob) to every
pathspec item is not pleasant. :(icase) may go the same way.
> So I think this is a nice, simple approach for sites that want it, and
> noglob magic can come later (and will not be any harder to implement as
> a result of this patch).
Any chance to make use of nd/pathspec-wildcard? It changes the same
code path in match_one. If you base on top of nd/pathspec-wildcard,
all you have to do is assign nowildcard_len to len (i.e. no wildcard
part).
--
Duy
^ permalink raw reply
* Re: [RFC] test: Old shells and physical paths
From: Junio C Hamano @ 2012-12-20 0:17 UTC (permalink / raw)
To: David Michael; +Cc: git@vger.kernel.org
In-Reply-To: <CAEvUa7=sOPF9xwfGuBXv0CBZhT+79+8z3tm9ar_cz3q--kfqRQ@mail.gmail.com>
David Michael <fedora.dm0@gmail.com> writes:
> In working on a port, I have to tolerate an ancient shell. The "cd"
> and "pwd" commands don't understand the "-P" flag for physical paths,
> as some tests use. The biggest offender is "cd -P" causing a failure
> in t/test-lib.sh (since 1bd9c64), which is sourced by every test
> script.
Is "here is a nickel, get a better shell" an option? Running tests
is one thing, but I'd be worried more about scripted Porcelains
broken by a non-POSIX shell if I were you.
> Would it be acceptable to instead force the platform's shell option
> (if it exists) to always use physical paths for the tests and drop the
> "-P" flags?
As a patch to the source files in my tree? Not likely, even though
I cannot say for sure without looking at how the change would look
like.
^ permalink raw reply
* Re: [PATCH] git-completion.bash: add support for path completion
From: Manlio Perillo @ 2012-12-19 23:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vy5gt7j3c.fsf@alter.siamese.dyndns.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 19/12/2012 23:49, Junio C Hamano ha scritto:
> Manlio Perillo <manlio.perillo@gmail.com> writes:
>
>>> git mv COPYING README X
>>
>> Assuming X is a new untracked directory, do you think it is an usability
>> problem if an user try to do:
>>
>> git mv COPYING README <TAB>
>>
>> and X does not appear in the completion list?
>
> It is hard to say. Will it show "Documentation/" in the list? Will
> it show "git.c" in the list?
>
Currently all cached files will be showed.
> Your "git mv git.<TAB>" completing to "git mv git.c" would be an
> improvement compared to the stock "less git.<TAB>" that offers
> "git.c" and "git.o" as choices. For things like "mv" and "rm", this
> may not make too much of a difference, "git add <TAB>" would be a
> vast improvement from the stock one. The users will notice that the
> completion knows what it is doing, and will come to depend on it.
>
Yes, this is precisely the reason why I'm implementing it ;-).
I also use Mercurial (I discovered Git just a few weeks ago, after
reading Pro Git), and Mercurial do have path completion (completion list
does not stop at directory boundary, however).
When I started to use Git, one of the first thing I noticed was the lack
of path completion for git add.
> [...]
> In the ideal world (read: I am *not* saying "you should implement it
> this way, or we won't look at your patch"), I think you would want
> to show tracked files (because it may be the third path that the
> user wants to move with the command, not the destination directory)
> and any directory on the filesystem (it could be the third path that
> is being moved if it has tracked files, or it could be the final
> destination directory argument).
What about this?
* show all and only cached files for the first argument
* show all cached + untracked directories and files for all other
arguments
This should solve most of the problem, and will still not show ignored
files. And, most important, it is very easy to implement.
The only issue is that "git ls-files -o" does not show empty
directories, and "git ls-files --directory -o" will add a trailing slash.
Thanks Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlDSUr4ACgkQscQJ24LbaURf0gCeJVZviwfKHgHNZ8vYBjnjwP8+
WF4AnAn3/KPJciJg9r387qIzCajx4j0s
=/10k
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v3] git-completion.bash: add support for path completion
From: Manlio Perillo @ 2012-12-19 23:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor, Felipe Contreras
In-Reply-To: <7vzk1995mx.fsf@alter.siamese.dyndns.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 19/12/2012 20:57, Junio C Hamano ha scritto:
> [...]
I just found a serious bug with "git commit" path completion.
When doing the first commit on an empty repository, completion will
cause an error:
$git commit -m init <TAB>fatal: ambiguous argument 'HEAD': unknown
revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
The problem is that for a newly created repository there is no HEAD.
If HEAD does not exists, code must use ls-files instead of diff-index
to get the completion list.
Another change is to always call git commands with stderr redirected to
/dev/null.
By the way, this is also a bug of current bash completion code:
$ git show does-not-exists:<TAB>fatal: Not a valid object name
does-not-exists
I will write a patch (based on master) to fix this.
> [...]
Regards Manlio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlDSUR0ACgkQscQJ24LbaUSkRwCfVKk9JhSD4sKDFm4heAkVbN0o
KAAAn3paTXyUiFlfY/UBpnkwiARegLsE
=7Q5s
-----END PGP SIGNATURE-----
^ permalink raw reply
* [RFC] test: Old shells and physical paths
From: David Michael @ 2012-12-19 23:22 UTC (permalink / raw)
To: git@vger.kernel.org
Hi,
In working on a port, I have to tolerate an ancient shell. The "cd"
and "pwd" commands don't understand the "-P" flag for physical paths,
as some tests use. The biggest offender is "cd -P" causing a failure
in t/test-lib.sh (since 1bd9c64), which is sourced by every test
script.
This shell does understand the difference between physical and logical
paths, but the only way to choose which is with a shell option. The
shell option is named "logical" which is not portable; GNU bash uses
the opposite, "physical".
Does anyone have any suggestions for how to handle this?
Would it be acceptable to instead force the platform's shell option
(if it exists) to always use physical paths for the tests and drop the
"-P" flags?
If not, is it worth maintaining compatibility wrappers for this one
obscure platform?
Thanks.
David
^ permalink raw reply
* Re: [PATCH v2] Add directory pattern matching to attributes
From: Junio C Hamano @ 2012-12-19 22:58 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git
In-Reply-To: <201212192334.51085.avila.jn@gmail.com>
"Jean-Noël AVILA" <avila.jn@gmail.com> writes:
> Le mercredi 19 décembre 2012 22:44:59, vous avez écrit :
>> "Jean-Noël AVILA" <avila.jn@gmail.com> writes:
>> > This patch was not reviewed when I submitted it for the second time.
>>
>> Did you miss this?
>>
>
> Grml, I did. Sorry for the noise.
That's OK. Your previous one, with the update suggested in the
thread, has already been queued in 'next' and will be cooking
there throughout the pre-release feature freeze.
^ permalink raw reply
* Re: [PATCH] git-completion.bash: add support for path completion
From: Junio C Hamano @ 2012-12-19 22:49 UTC (permalink / raw)
To: Manlio Perillo; +Cc: git
In-Reply-To: <50D23960.4070108@gmail.com>
Manlio Perillo <manlio.perillo@gmail.com> writes:
>> git mv COPYING README X
>
> Assuming X is a new untracked directory, do you think it is an usability
> problem if an user try to do:
>
> git mv COPYING README <TAB>
>
> and X does not appear in the completion list?
It is hard to say. Will it show "Documentation/" in the list? Will
it show "git.c" in the list?
Your "git mv git.<TAB>" completing to "git mv git.c" would be an
improvement compared to the stock "less git.<TAB>" that offers
"git.c" and "git.o" as choices. For things like "mv" and "rm", this
may not make too much of a difference, "git add <TAB>" would be a
vast improvement from the stock one. The users will notice that the
completion knows what it is doing, and will come to depend on it.
But at that point, if "git mv COPYING README <TAB>" offers only
directories that contain tracked files, the users may get irritated,
because it is likely that the destination directory was created
immediately before the user started typing "git mv". You will hear
"Surely I created X, it is there, why aren't you showing it to me?"
The updated completion knows what it is doing everywhere else, and
it sets the user-expectation at that level. Uneven cleverness will
stand out like a sore thumb and hurts the user perception, which is
arguably unfair, but nothing in life is fair X-<.
I think over-showing the choices is much better than hiding some
choices, if we cannot do a perfect completion in some cases. "You
should know that I won't be moving these files in Y, as I already
marked it to be ignored in the .gitignore file!" is less grave a
gripe than "You know I created X just a minute ago, and it is there,
why aren't you showing it to me?" because you can simply say "but Y
is there as a directory." admitting that you are less clever than
the user expects you to be, but at least you are giving the choice
to the user of not picking it.
In the ideal world (read: I am *not* saying "you should implement it
this way, or we won't look at your patch"), I think you would want
to show tracked files (because it may be the third path that the
user wants to move with the command, not the destination directory)
and any directory on the filesystem (it could be the third path that
is being moved if it has tracked files, or it could be the final
destination directory argument).
^ 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