* [PATCH] builtin/config.c: Fix a sparse warning
From: Ramsay Jones @ 2012-10-28 21:05 UTC (permalink / raw)
To: Jeff King; +Cc: GIT Mailing-list
Sparse issues an "Using plain integer as NULL pointer" warning while
checking a 'struct strbuf_list' initializer expression. The initial
field of the struct has pointer type, but the initializer expression
is given as '{0}'. In order to suppress the warning, we simply replace
the initializer with '{NULL}'.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Jeff,
I try to catch these types of warnings while the patches are still
in the pu branch. I don't know how I missed this one, but I don't
remember your 'jk/config-ignore-duplicates' branch being in pu.
Sorry about that.
ATB,
Ramsay Jones
builtin/config.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/config.c b/builtin/config.c
index f881053..e796af4 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -165,7 +165,7 @@ static int collect_config(const char *key_, const char *value_, void *cb)
static int get_value(const char *key_, const char *regex_)
{
int ret = CONFIG_GENERIC_ERROR;
- struct strbuf_list values = {0};
+ struct strbuf_list values = {NULL};
int i;
if (use_key_regexp) {
--
1.8.0
^ permalink raw reply related
* Re: [PATCHv3 0/2] Teach --recursive to submodule sync
From: Jens Lehmann @ 2012-10-28 21:02 UTC (permalink / raw)
To: Phil Hord; +Cc: git, phil.hord, Jeff King
In-Reply-To: <1351280683-8402-1-git-send-email-hordp@cisco.com>
Am 26.10.2012 21:44, schrieb Phil Hord:
> [PATCHv3 1/2] Teach --recursive to submodule sync
>
> Now with less noise and no redundant flags passed to the recursive call.
>
>
> [PATCHv3 2/2] Add tests for submodule sync --recursive
>
> The test remains unchanged.
Both are looking good.
Acked-By: Jens Lehmann <Jens.Lehmann@web.de>
^ permalink raw reply
* Re: [PATCH v2] git-submodule add: Add -r/--record option.
From: Jens Lehmann @ 2012-10-28 20:48 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git, Nahor, Phil Hord, Shawn O. Pearce
In-Reply-To: <20121025005307.GE801@odin.tremily.us>
Am 25.10.2012 02:53, schrieb W. Trevor King:
> On Wed, Oct 24, 2012 at 09:15:32PM +0200, Jens Lehmann wrote:
>> I still fail to see what adding that functionality to the submodule
>> command buys us (unless we also add code which really uses the branch
>> setting). What's wrong with doing a simple:
>>
>> git config -f .gitmodules submodule.<path>.branch <record_branch>
>>
>> on the command line when you want to use the branch setting for your
>> own purposes? You could easily wrap that into a helper script, no?
>
> Sure. But why maintain my own helper script if I can edit
> git-submodules.sh? It seems like a number of people are using this
> config option, and they generally store the same name in it that they
> use to create the submodule. This way I can save them time too.
But people are already using the "branch" setting in *different* ways:
Am 23.10.2012 22:55, schrieb W. Trevor King:
> As Phil pointed out, doing anything with this variable is ambiguous:
>
> On Mon, Oct 22, 2012 at 06:03:53PM -0400, Phil Hord wrote:
>> Some projects now use the 'branch' config value to record the tracking
>> branch for the submodule. Some ascribe different meaning to the
>> configuration if the value is given vs. undefined. For example, see
>> the Gerrit submodule-subscription mechanism. This change will cause
>> those workflows to behave differently than they do now.
I don't have a problem with the amount or complexity of the code being
added, But by adding that option we may be giving the impression that it
is officially sanctioned, or that it will be kept up to date by further
submodule commands. I added Shawn to the CC, maybe he can comment on how
the "branch" setting is used in Gerrit and what he thinks about adding
code to set that with "git submodule add -r <branch> ..." to core git.
^ permalink raw reply
* [PATCH] Documentation: improve the example of overriding LESS via core.pager
From: Patrick Palka @ 2012-10-28 20:12 UTC (permalink / raw)
To: git; +Cc: Patrick Palka
You can override an option set in the LESS variable by simply prefixing
the command line option with `-+`. This is more robust than the previous
example if the default LESS options are to ever change.
Signed-off-by: Patrick Palka <patrick@parcs.ath.cx>
---
Documentation/config.txt | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 11f320b..9a0544c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -538,14 +538,14 @@ core.pager::
`LESS` variable to some other value. Alternately,
these settings can be overridden on a project or
global basis by setting the `core.pager` option.
- Setting `core.pager` has no affect on the `LESS`
+ Setting `core.pager` has no effect on the `LESS`
environment variable behaviour above, so if you want
to override git's default settings this way, you need
to be explicit. For example, to disable the S option
in a backward compatible manner, set `core.pager`
- to `less -+$LESS -FRX`. This will be passed to the
- shell by git, which will translate the final command to
- `LESS=FRSX less -+FRSX -FRX`.
+ to `less -+S`. This will be passed to the shell by
+ git, which will translate the final command to
+ `LESS=FRSX less -+S`.
core.whitespace::
A comma separated list of common whitespace problems to
--
1.7.10.4
^ permalink raw reply related
* Re: git push tags
From: Chris Rorvick @ 2012-10-28 19:59 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Angelo Borsotti, git
In-Reply-To: <508D7628.10509@kdbg.org>
On Sun, Oct 28, 2012 at 1:15 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Tags are refs, just like branches. "Tags don't move" is just a
> convention, and git doesn't even respect it (except possibly in one
> place[1]). You can't reseat tags unless you use -f, which is exactly the
> same with branches, which you can't reseat unless you use -f.
>
> [1] By default, git fetch does not fetch tags that it already has.
Also, git checkout <tag> puts you on a detached HEAD. This seems
pretty significant with regard to Git respecting a "tags don't move"
convention.
Chris Rorvick
^ permalink raw reply
* Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter
From: Peter Oberndorfer @ 2012-10-28 19:56 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20121028120104.GE11434@sigill.intra.peff.net>
On 2012-10-28 13:01, Jeff King wrote:
> On Sat, Oct 27, 2012 at 08:37:24PM +0200, Peter Oberndorfer wrote:
>
>> It seems "git diff-tree -Ganything <tree>" crashes[1] with a null
>> pointer dereference
>> when run on a commit that adds a file (pdf) with a textconv filter.
>>
>> It can be reproduced with vanilla git by having a commit on top that
>> adds a file with a textconv filter and executing git diff-tree
>> -Ganything HEAD
>> But running git log -Ganything still works without a crash.
>> This problem seems to exist since the feature was first added in f506b8e8b5.
> Thanks for a thorough bug report. I didn't reproduce the crash, but I
> can see how it happens (it happens with diff-tree because we will reuse
> the working tree file in that instance; it could also happen if you
> turned on textconv caching).
>
>> While testing I also noticed the -S and -G act on the original file
>> instead of the textconv munged data.
>> Is this intentional or caused by accessing the wrong data?
> Both, perhaps. :)
Hi,
thanks for your patch for this!
>
> -G operates on the munged data; you can see it feed the munged data to
> xdiff in diff_grep. But the optimization for handling added and removed
> files accidentally fed the wrong pointer. Fixing that is a no-brainer,
> since the optimization is inconsistent with the regular code path.
>
> -S, however, predates the invention of textconv and has never used it.
> It is a little less clear that textconv is the right thing here, because
> it is not about grepping the diff, but about counting occurrences of the
> string inside the file content. I tend to think that doing so on the
> textconv'd data would be what people generally want, but it is a
> behavior change.
>
>> Wild guess: should we really access p->one->data and not mf1.ptr?
> Precisely. Thanks for your wild guess; it made finding the bug very
> easy. :)
>
>> Is there some more information i should provide?
> The patch below should fix it. I added tests, but please try your
> real-world test case on it to double-check.
I tested your patch, but now it crashes for another reason :-)
i have a file with exactly 12288(0x3000) bytes in the repository.
When the file is loaded, the data is placed luckily so the data end
falls at a page boundary.
Later diff_grep() calls regexec() which calls strlen() on the loaded buffer
and ends up reading beyond the actual data into the next page
which is not allocated and causes a pagefault.
Or it could possibly (randomly) match the regex on data that is not
actually part of a file...
Different memory allocation rules on Windows probably also have some
influence here.
My guess is that diff_filespec->data is not supposed to be zero terminated
and we should not invoke strlen() on a not zero terminated data.
But this should be decided by somebody who knows the rules.
Greetings Peter
^ permalink raw reply
* Re: git push tags
From: Johannes Sixt @ 2012-10-28 18:15 UTC (permalink / raw)
To: Angelo Borsotti; +Cc: git
In-Reply-To: <CAB9Jk9DMOwhDf3SvMzTmTZiyZg_4pgXx-evrfWkB3U4w-KqtVw@mail.gmail.com>
Am 25.10.2012 08:58, schrieb Angelo Borsotti:
> Hello,
>
> git push tag updates silently the specified tag. E.g.
>
> git init --bare release.git
> git clone release.git integrator
> cd integrator
> git branch -avv
> touch f1; git add f1; git commit -m A
> git tag v1
> git push origin tag v1
> touch f2; git add f2; git commit -m B
> git tag -f v1
> git push origin tag v1
>
> the second git push updates the tag in the remote repository. This is
> somehow counterintuitive because tags normally do not move (unless
> forced to that), and is not documented.
Tags are refs, just like branches. "Tags don't move" is just a
convention, and git doesn't even respect it (except possibly in one
place[1]). You can't reseat tags unless you use -f, which is exactly the
same with branches, which you can't reseat unless you use -f.
[1] By default, git fetch does not fetch tags that it already has.
> This is also harmful because it allows to change silently something
> (tags) that normally must not change.
You asked git push to push a tag, and the tag was pushed. What's wrong?
-- Hannes
^ permalink raw reply
* [PATCH] parse_dirstat_params(): use string_list to split comma-separated string
From: Michael Haggerty @ 2012-10-28 16:50 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Michael Haggerty
Use string_list_split_in_place() to split the comma-separated
parameters string. This simplifies the code and also fixes a bug: the
old code made calls like
memcmp(p, "lines", p_len)
which needn't work if p_len is different than the length of the
constant string (and could illegally access memory if p_len is larger
than the length of the constant string).
When p_len was less than the length of the constant string, the old
code would have allowed some abbreviations to be accepted (e.g., "cha"
for "changes") but this seems to have been a bug rather than a
feature, because (1) it is not documented; (2) no attempt was made to
handle ambiguous abbreviations, like "c" for "changes" vs
"cumulative".
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
diff.c | 40 +++++++++++++++++++++-------------------
1 file changed, 21 insertions(+), 19 deletions(-)
diff --git a/diff.c b/diff.c
index 35d3f07..a9cc8a9 100644
--- a/diff.c
+++ b/diff.c
@@ -15,6 +15,7 @@
#include "sigchain.h"
#include "submodule.h"
#include "ll-merge.h"
+#include "string-list.h"
#ifdef NO_FAST_WORKING_DIRECTORY
#define FAST_WORKING_DIRECTORY 0
@@ -68,26 +69,30 @@ static int parse_diff_color_slot(const char *var, int ofs)
return -1;
}
-static int parse_dirstat_params(struct diff_options *options, const char *params,
+static int parse_dirstat_params(struct diff_options *options, const char *params_string,
struct strbuf *errmsg)
{
- const char *p = params;
- int p_len, ret = 0;
+ char *params_copy = xstrdup(params_string);
+ struct string_list params = STRING_LIST_INIT_NODUP;
+ int ret = 0;
+ int i;
- while (*p) {
- p_len = strchrnul(p, ',') - p;
- if (!memcmp(p, "changes", p_len)) {
+ if (*params_copy)
+ string_list_split_in_place(¶ms, params_copy, ',', -1);
+ for (i = 0; i < params.nr; i++) {
+ const char *p = params.items[i].string;
+ if (!strcmp(p, "changes")) {
DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
- } else if (!memcmp(p, "lines", p_len)) {
+ } else if (!strcmp(p, "lines")) {
DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
- } else if (!memcmp(p, "files", p_len)) {
+ } else if (!strcmp(p, "files")) {
DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
- } else if (!memcmp(p, "noncumulative", p_len)) {
+ } else if (!strcmp(p, "noncumulative")) {
DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
- } else if (!memcmp(p, "cumulative", p_len)) {
+ } else if (!strcmp(p, "cumulative")) {
DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
} else if (isdigit(*p)) {
char *end;
@@ -99,24 +104,21 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
while (isdigit(*++end))
; /* nothing */
}
- if (end - p == p_len)
+ if (!*end)
options->dirstat_permille = permille;
else {
- strbuf_addf(errmsg, _(" Failed to parse dirstat cut-off percentage '%.*s'\n"),
- p_len, p);
+ strbuf_addf(errmsg, _(" Failed to parse dirstat cut-off percentage '%s'\n"),
+ p);
ret++;
}
} else {
- strbuf_addf(errmsg, _(" Unknown dirstat parameter '%.*s'\n"),
- p_len, p);
+ strbuf_addf(errmsg, _(" Unknown dirstat parameter '%s'\n"), p);
ret++;
}
- p += p_len;
-
- if (*p)
- p++; /* more parameters, swallow separator */
}
+ string_list_clear(¶ms, 0);
+ free(params_copy);
return ret;
}
--
1.8.0
^ permalink raw reply related
* [PATCH v4 8/8] string_list_longest_prefix(): remove function
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
In-Reply-To: <1351440987-26636-1-git-send-email-mhagger@alum.mit.edu>
This function was added in f103f95b11d087f07c0c48bf784cd9197e18f203 in
the erroneous expectation that it would be used in the
reimplementation of longest_ancestor_length(). But it turned out to
be easier to use a function specialized for comparing path prefixes
(i.e., one that knows about slashes and root paths) than to prepare
the paths in such a way that a generic string prefix comparison
function can be used. So delete string_list_longest_prefix() and its
documentation and test cases.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Documentation/technical/api-string-list.txt | 8 --------
string-list.c | 20 -------------------
string-list.h | 8 --------
t/t0063-string-list.sh | 30 -----------------------------
test-string-list.c | 20 -------------------
5 files changed, 86 deletions(-)
diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
index 94d7a2b..618400d 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -75,14 +75,6 @@ Functions
to be deleted. Preserve the order of the items that are
retained.
-`string_list_longest_prefix`::
-
- Return the longest string within a string_list that is a
- prefix (in the sense of prefixcmp()) of the specified string,
- or NULL if no such prefix exists. This function does not
- require the string_list to be sorted (it does a linear
- search).
-
`print_string_list`::
Dump a string_list to stdout, useful mainly for debugging purposes. It
diff --git a/string-list.c b/string-list.c
index c54b816..decfa74 100644
--- a/string-list.c
+++ b/string-list.c
@@ -136,26 +136,6 @@ void filter_string_list(struct string_list *list, int free_util,
list->nr = dst;
}
-char *string_list_longest_prefix(const struct string_list *prefixes,
- const char *string)
-{
- int i, max_len = -1;
- char *retval = NULL;
-
- for (i = 0; i < prefixes->nr; i++) {
- char *prefix = prefixes->items[i].string;
- if (!prefixcmp(string, prefix)) {
- int len = strlen(prefix);
- if (len > max_len) {
- retval = prefix;
- max_len = len;
- }
- }
- }
-
- return retval;
-}
-
void string_list_clear(struct string_list *list, int free_util)
{
if (list->items) {
diff --git a/string-list.h b/string-list.h
index 5efd07b..3a6a6dc 100644
--- a/string-list.h
+++ b/string-list.h
@@ -38,14 +38,6 @@ int for_each_string_list(struct string_list *list,
void filter_string_list(struct string_list *list, int free_util,
string_list_each_func_t want, void *cb_data);
-/*
- * Return the longest string in prefixes that is a prefix (in the
- * sense of prefixcmp()) of string, or NULL if no such prefix exists.
- * This function does not require the string_list to be sorted (it
- * does a linear search).
- */
-char *string_list_longest_prefix(const struct string_list *prefixes, const char *string);
-
/* Use these functions only on sorted lists: */
int string_list_has_string(const struct string_list *list, const char *string);
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 41c8826..dbfc05e 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -17,14 +17,6 @@ test_split () {
"
}
-test_longest_prefix () {
- test "$(test-string-list longest_prefix "$1" "$2")" = "$3"
-}
-
-test_no_longest_prefix () {
- test_must_fail test-string-list longest_prefix "$1" "$2"
-}
-
test_split "foo:bar:baz" ":" "-1" <<EOF
3
[0]: "foo"
@@ -96,26 +88,4 @@ test_expect_success "test remove_duplicates" '
test a:b:c = "$(test-string-list remove_duplicates a:a:a:b:b:b:c:c:c)"
'
-test_expect_success "test longest_prefix" '
- test_no_longest_prefix - '' &&
- test_no_longest_prefix - x &&
- test_longest_prefix "" x "" &&
- test_longest_prefix x x x &&
- test_longest_prefix "" foo "" &&
- test_longest_prefix : foo "" &&
- test_longest_prefix f foo f &&
- test_longest_prefix foo foobar foo &&
- test_longest_prefix foo foo foo &&
- test_no_longest_prefix bar foo &&
- test_no_longest_prefix bar:bar foo &&
- test_no_longest_prefix foobar foo &&
- test_longest_prefix foo:bar foo foo &&
- test_longest_prefix foo:bar bar bar &&
- test_longest_prefix foo::bar foo foo &&
- test_longest_prefix foo:foobar foo foo &&
- test_longest_prefix foobar:foo foo foo &&
- test_longest_prefix foo: bar "" &&
- test_longest_prefix :foo bar ""
-'
-
test_done
diff --git a/test-string-list.c b/test-string-list.c
index 4693295..00ce6c9 100644
--- a/test-string-list.c
+++ b/test-string-list.c
@@ -97,26 +97,6 @@ int main(int argc, char **argv)
return 0;
}
- if (argc == 4 && !strcmp(argv[1], "longest_prefix")) {
- /* arguments: <colon-separated-prefixes>|- <string> */
- struct string_list prefixes = STRING_LIST_INIT_DUP;
- int retval;
- const char *prefix_string = argv[2];
- const char *string = argv[3];
- const char *match;
-
- parse_string_list(&prefixes, prefix_string);
- match = string_list_longest_prefix(&prefixes, string);
- if (match) {
- printf("%s\n", match);
- retval = 0;
- }
- else
- retval = 1;
- string_list_clear(&prefixes, 0);
- return retval;
- }
-
fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
argv[1] ? argv[1] : "(there was none)");
return 1;
--
1.8.0
^ permalink raw reply related
* [PATCH v4 7/8] setup_git_directory_gently_1(): resolve symlinks in ceiling paths
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
In-Reply-To: <1351440987-26636-1-git-send-email-mhagger@alum.mit.edu>
longest_ancestor_length() relies on a textual comparison of directory
parts to find the part of path that overlaps with one of the paths in
prefix_list. But this doesn't work if any of the prefixes involves a
symbolic link, because the directories will look different even though
they might logically refer to the same directory. So canonicalize the
paths listed in GIT_CEILING_DIRECTORIES using real_path_if_valid()
before passing them to longest_ancestor_length(). (Also rename
normalize_ceiling_entry() to canonicalize_ceiling_entry() to reflect
the change.)
path is already in canonical form, so doesn't need to be canonicalized
again.
This fixes some problems with using GIT_CEILING_DIRECTORIES that
contains paths involving symlinks, including t4035 if run with --root
set to a path involving symlinks.
Please note that test t0060 is *not* changed analogously, because that
would make the test suite results dependent on the contents of the
local root directory. However, real_path() is already tested
independently, and the "ancestor" tests cover the non-normalization
aspects of longest_ancestor_length(), so coverage remains sufficient.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
setup.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/setup.c b/setup.c
index df97ad3..f108c4b 100644
--- a/setup.c
+++ b/setup.c
@@ -622,24 +622,23 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
}
/*
- * A "string_list_each_func_t" function that normalizes an entry from
- * GIT_CEILING_DIRECTORIES or discards it if unusable.
+ * A "string_list_each_func_t" function that canonicalizes an entry
+ * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
+ * discards it if unusable.
*/
-static int normalize_ceiling_entry(struct string_list_item *item, void *unused)
+static int canonicalize_ceiling_entry(struct string_list_item *item,
+ void *unused)
{
- const char *ceil = item->string;
- int len = strlen(ceil);
- char buf[PATH_MAX+1];
+ char *ceil = item->string;
+ const char *real_path;
- if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
+ if (!*ceil || !is_absolute_path(ceil))
return 0;
- if (normalize_path_copy(buf, ceil) < 0)
+ real_path = real_path_if_valid(ceil);
+ if (!real_path)
return 0;
- len = strlen(buf);
- if (len > 1 && buf[len-1] == '/')
- buf[--len] = '\0';
free(item->string);
- item->string = xstrdup(buf);
+ item->string = xstrdup(real_path);
return 1;
}
@@ -681,7 +680,8 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
if (env_ceiling_dirs) {
string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
- filter_string_list(&ceiling_dirs, 0, normalize_ceiling_entry, NULL);
+ filter_string_list(&ceiling_dirs, 0,
+ canonicalize_ceiling_entry, NULL);
ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0);
}
--
1.8.0
^ permalink raw reply related
* [PATCH v4 6/8] longest_ancestor_length(): require prefix list entries to be normalized
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
In-Reply-To: <1351440987-26636-1-git-send-email-mhagger@alum.mit.edu>
Move the responsibility for normalizing prefixes from
longest_ancestor_length() to its callers. Use slightly different
normalizations at the two callers:
In setup_git_directory_gently_1(), use the old normalization, which
ignores paths that are not usable. In the next commit we will change
this caller to also resolve symlinks in the paths from
GIT_CEILING_DIRECTORIES as part of the normalization.
In "test-path-utils longest_ancestor_length", use the old
normalization, but die() if any paths are unusable. Also change t0060
to only pass normalized paths to the test program (no empty entries or
non-absolute paths, strip trailing slashes from the paths, and remove
tests that thereby become redundant).
The point of this change is to reduce the scope of the ancestor_length
tests in t0060 from testing normalization+longest_prefix to testing
only mostly longest_prefix. This is necessary because when
setup_git_directory_gently_1() starts resolving symlinks as part of
its normalization, it will not be reasonable to do the same in the
test suite, because that would make the test results depend on the
contents of the root directory of the filesystem on which the test is
run. HOWEVER: under Windows, bash mangles arguments that look like
absolute POSIX paths into DOS paths. So we have to retain the level
of normalization done by normalize_path_copy() to convert the
bash-mangled DOS paths (which contain backslashes) into paths that use
forward slashes.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
path.c | 26 +++++++++++---------------
setup.c | 23 +++++++++++++++++++++++
t/t0060-path-utils.sh | 41 +++++++++++++----------------------------
test-path-utils.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 91 insertions(+), 44 deletions(-)
diff --git a/path.c b/path.c
index b80d2e6..d3d3f8b 100644
--- a/path.c
+++ b/path.c
@@ -570,20 +570,20 @@ int normalize_path_copy(char *dst, const char *src)
/*
* path = Canonical absolute path
- * prefixes = string_list containing absolute paths
+ * prefixes = string_list containing normalized, absolute paths without
+ * trailing slashes (except for the root directory, which is denoted by "/").
*
- * Determines, for each path in prefixes, whether the "prefix" really
+ * Determines, for each path in prefixes, whether the "prefix"
* is an ancestor directory of path. Returns the length of the longest
* ancestor directory, excluding any trailing slashes, or -1 if no prefix
* is an ancestor. (Note that this means 0 is returned if prefixes is
* ["/"].) "/foo" is not considered an ancestor of "/foobar". Directories
* are not considered to be their own ancestors. path must be in a
* canonical form: empty components, or "." or ".." components are not
- * allowed. Empty strings in prefixes are ignored.
+ * allowed.
*/
int longest_ancestor_length(const char *path, struct string_list *prefixes)
{
- char buf[PATH_MAX+1];
int i, max_len = -1;
if (!strcmp(path, "/"))
@@ -593,19 +593,15 @@ int longest_ancestor_length(const char *path, struct string_list *prefixes)
const char *ceil = prefixes->items[i].string;
int len = strlen(ceil);
- if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
- continue;
- if (normalize_path_copy(buf, ceil) < 0)
- continue;
- len = strlen(buf);
- if (len > 0 && buf[len-1] == '/')
- buf[--len] = '\0';
+ if (len == 1 && ceil[0] == '/')
+ len = 0; /* root matches anything, with length 0 */
+ else if (!strncmp(path, ceil, len) && path[len] == '/')
+ ; /* match of length len */
+ else
+ continue; /* no match */
- if (!strncmp(path, buf, len) &&
- path[len] == '/' &&
- len > max_len) {
+ if (len > max_len)
max_len = len;
- }
}
return max_len;
diff --git a/setup.c b/setup.c
index b4cd356..df97ad3 100644
--- a/setup.c
+++ b/setup.c
@@ -622,6 +622,28 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
}
/*
+ * A "string_list_each_func_t" function that normalizes an entry from
+ * GIT_CEILING_DIRECTORIES or discards it if unusable.
+ */
+static int normalize_ceiling_entry(struct string_list_item *item, void *unused)
+{
+ const char *ceil = item->string;
+ int len = strlen(ceil);
+ char buf[PATH_MAX+1];
+
+ if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
+ return 0;
+ if (normalize_path_copy(buf, ceil) < 0)
+ return 0;
+ len = strlen(buf);
+ if (len > 1 && buf[len-1] == '/')
+ buf[--len] = '\0';
+ free(item->string);
+ item->string = xstrdup(buf);
+ return 1;
+}
+
+/*
* We cannot decide in this function whether we are in the work tree or
* not, since the config can only be read _after_ this function was called.
*/
@@ -659,6 +681,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
if (env_ceiling_dirs) {
string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
+ filter_string_list(&ceiling_dirs, 0, normalize_ceiling_entry, NULL);
ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0);
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 4ef2345..09a42a4 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -93,47 +93,32 @@ norm_path /d1/s1//../s2/../../d2 /d2 POSIX
norm_path /d1/.../d2 /d1/.../d2 POSIX
norm_path /d1/..././../d2 /d1/d2 POSIX
-ancestor / "" -1
ancestor / / -1
-ancestor /foo "" -1
-ancestor /foo : -1
-ancestor /foo ::. -1
-ancestor /foo ::..:: -1
ancestor /foo / 0
ancestor /foo /fo -1
ancestor /foo /foo -1
-ancestor /foo /foo/ -1
ancestor /foo /bar -1
-ancestor /foo /bar/ -1
ancestor /foo /foo/bar -1
-ancestor /foo /foo:/bar/ -1
-ancestor /foo /foo/:/bar/ -1
-ancestor /foo /foo::/bar/ -1
-ancestor /foo /:/foo:/bar/ 0
-ancestor /foo /foo:/:/bar/ 0
-ancestor /foo /:/bar/:/foo 0
-ancestor /foo/bar "" -1
+ancestor /foo /foo:/bar -1
+ancestor /foo /:/foo:/bar 0
+ancestor /foo /foo:/:/bar 0
+ancestor /foo /:/bar:/foo 0
ancestor /foo/bar / 0
ancestor /foo/bar /fo -1
-ancestor /foo/bar foo -1
ancestor /foo/bar /foo 4
-ancestor /foo/bar /foo/ 4
ancestor /foo/bar /foo/ba -1
ancestor /foo/bar /:/fo 0
ancestor /foo/bar /foo:/foo/ba 4
ancestor /foo/bar /bar -1
-ancestor /foo/bar /bar/ -1
-ancestor /foo/bar /fo: -1
-ancestor /foo/bar :/fo -1
-ancestor /foo/bar /foo:/bar/ 4
-ancestor /foo/bar /:/foo:/bar/ 4
-ancestor /foo/bar /foo:/:/bar/ 4
-ancestor /foo/bar /:/bar/:/fo 0
-ancestor /foo/bar /:/bar/ 0
-ancestor /foo/bar .:/foo/. 4
-ancestor /foo/bar .:/foo/.:.: 4
-ancestor /foo/bar /foo/./:.:/bar 4
-ancestor /foo/bar .:/bar -1
+ancestor /foo/bar /fo -1
+ancestor /foo/bar /foo:/bar 4
+ancestor /foo/bar /:/foo:/bar 4
+ancestor /foo/bar /foo:/:/bar 4
+ancestor /foo/bar /:/bar:/fo 0
+ancestor /foo/bar /:/bar 0
+ancestor /foo/bar /foo 4
+ancestor /foo/bar /foo:/bar 4
+ancestor /foo/bar /bar -1
test_expect_success 'strip_path_suffix' '
test c:/msysgit = $(test-path-utils strip_path_suffix \
diff --git a/test-path-utils.c b/test-path-utils.c
index acb0560..0092cbf 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -1,6 +1,33 @@
#include "cache.h"
#include "string-list.h"
+/*
+ * A "string_list_each_func_t" function that normalizes an entry from
+ * GIT_CEILING_DIRECTORIES. If the path is unusable for some reason,
+ * die with an explanation.
+ */
+static int normalize_ceiling_entry(struct string_list_item *item, void *unused)
+{
+ const char *ceil = item->string;
+ int len = strlen(ceil);
+ char buf[PATH_MAX+1];
+
+ if (len == 0)
+ die("Empty path is not supported");
+ if (len > PATH_MAX)
+ die("Path \"%s\" is too long", ceil);
+ if (!is_absolute_path(ceil))
+ die("Path \"%s\" is not absolute", ceil);
+ if (normalize_path_copy(buf, ceil) < 0)
+ die("Path \"%s\" could not be normalized", ceil);
+ len = strlen(buf);
+ if (len > 1 && buf[len-1] == '/')
+ die("Normalized path \"%s\" ended with slash", buf);
+ free(item->string);
+ item->string = xstrdup(buf);
+ return 1;
+}
+
int main(int argc, char **argv)
{
if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
@@ -33,10 +60,26 @@ int main(int argc, char **argv)
if (argc == 4 && !strcmp(argv[1], "longest_ancestor_length")) {
int len;
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
+ char *path = xstrdup(argv[2]);
+ /*
+ * We have to normalize the arguments because under
+ * Windows, bash mangles arguments that look like
+ * absolute POSIX paths or colon-separate lists of
+ * absolute POSIX paths into DOS paths (e.g.,
+ * "/foo:/foo/bar" might be converted to
+ * "D:\Src\msysgit\foo;D:\Src\msysgit\foo\bar"),
+ * whereas longest_ancestor_length() requires paths
+ * that use forward slashes.
+ */
+ if (normalize_path_copy(path, path))
+ die("Path \"%s\" could not be normalized", argv[2]);
string_list_split(&ceiling_dirs, argv[3], PATH_SEP, -1);
- len = longest_ancestor_length(argv[2], &ceiling_dirs);
+ filter_string_list(&ceiling_dirs, 0,
+ normalize_ceiling_entry, NULL);
+ len = longest_ancestor_length(path, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0);
+ free(path);
printf("%d\n", len);
return 0;
}
--
1.8.0
^ permalink raw reply related
* [PATCH v4 5/8] longest_ancestor_length(): take a string_list argument for prefixes
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
In-Reply-To: <1351440987-26636-1-git-send-email-mhagger@alum.mit.edu>
Change longest_ancestor_length() to take the prefixes argument as a
string_list rather than as a colon-separated string. This will make
it easier for the caller to alter the entries before calling
longest_ancestor_length().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
cache.h | 2 +-
path.c | 22 +++++++++-------------
setup.c | 11 +++++++++--
test-path-utils.c | 8 +++++++-
4 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/cache.h b/cache.h
index b0d75bc..8103385 100644
--- a/cache.h
+++ b/cache.h
@@ -718,7 +718,7 @@ const char *real_path_if_valid(const char *path);
const char *absolute_path(const char *path);
const char *relative_path(const char *abs, const char *base);
int normalize_path_copy(char *dst, const char *src);
-int longest_ancestor_length(const char *path, const char *prefix_list);
+int longest_ancestor_length(const char *path, struct string_list *prefixes);
char *strip_path_suffix(const char *path, const char *suffix);
int daemon_avoid_alias(const char *path);
int offset_1st_component(const char *path);
diff --git a/path.c b/path.c
index f455e8e..b80d2e6 100644
--- a/path.c
+++ b/path.c
@@ -570,30 +570,27 @@ int normalize_path_copy(char *dst, const char *src)
/*
* path = Canonical absolute path
- * prefix_list = Colon-separated list of absolute paths
+ * prefixes = string_list containing absolute paths
*
- * Determines, for each path in prefix_list, whether the "prefix" really
+ * Determines, for each path in prefixes, whether the "prefix" really
* is an ancestor directory of path. Returns the length of the longest
* ancestor directory, excluding any trailing slashes, or -1 if no prefix
- * is an ancestor. (Note that this means 0 is returned if prefix_list is
- * "/".) "/foo" is not considered an ancestor of "/foobar". Directories
+ * is an ancestor. (Note that this means 0 is returned if prefixes is
+ * ["/"].) "/foo" is not considered an ancestor of "/foobar". Directories
* are not considered to be their own ancestors. path must be in a
* canonical form: empty components, or "." or ".." components are not
- * allowed. prefix_list may be null, which is like "".
+ * allowed. Empty strings in prefixes are ignored.
*/
-int longest_ancestor_length(const char *path, const char *prefix_list)
+int longest_ancestor_length(const char *path, struct string_list *prefixes)
{
- struct string_list prefixes = STRING_LIST_INIT_DUP;
char buf[PATH_MAX+1];
int i, max_len = -1;
- if (prefix_list == NULL || !strcmp(path, "/"))
+ if (!strcmp(path, "/"))
return -1;
- string_list_split(&prefixes, prefix_list, PATH_SEP, -1);
-
- for (i = 0; i < prefixes.nr; i++) {
- const char *ceil = prefixes.items[i].string;
+ for (i = 0; i < prefixes->nr; i++) {
+ const char *ceil = prefixes->items[i].string;
int len = strlen(ceil);
if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
@@ -611,7 +608,6 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
}
}
- string_list_clear(&prefixes, 0);
return max_len;
}
diff --git a/setup.c b/setup.c
index 3a1b2fd..b4cd356 100644
--- a/setup.c
+++ b/setup.c
@@ -1,5 +1,6 @@
#include "cache.h"
#include "dir.h"
+#include "string-list.h"
static int inside_git_dir = -1;
static int inside_work_tree = -1;
@@ -627,10 +628,11 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
static const char *setup_git_directory_gently_1(int *nongit_ok)
{
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
+ struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
static char cwd[PATH_MAX+1];
const char *gitdirenv, *ret;
char *gitfile;
- int len, offset, offset_parent, ceil_offset;
+ int len, offset, offset_parent, ceil_offset = -1;
dev_t current_device = 0;
int one_filesystem = 1;
@@ -655,7 +657,12 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
if (gitdirenv)
return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
- ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
+ if (env_ceiling_dirs) {
+ string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
+ ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
+ string_list_clear(&ceiling_dirs, 0);
+ }
+
if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
ceil_offset = 1;
diff --git a/test-path-utils.c b/test-path-utils.c
index 3bc20e9..acb0560 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "string-list.h"
int main(int argc, char **argv)
{
@@ -30,7 +31,12 @@ int main(int argc, char **argv)
}
if (argc == 4 && !strcmp(argv[1], "longest_ancestor_length")) {
- int len = longest_ancestor_length(argv[2], argv[3]);
+ int len;
+ struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
+
+ string_list_split(&ceiling_dirs, argv[3], PATH_SEP, -1);
+ len = longest_ancestor_length(argv[2], &ceiling_dirs);
+ string_list_clear(&ceiling_dirs, 0);
printf("%d\n", len);
return 0;
}
--
1.8.0
^ permalink raw reply related
* [PATCH v4 4/8] longest_ancestor_length(): use string_list_split()
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
In-Reply-To: <1351440987-26636-1-git-send-email-mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
path.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/path.c b/path.c
index cbbdf7d..f455e8e 100644
--- a/path.c
+++ b/path.c
@@ -12,6 +12,7 @@
*/
#include "cache.h"
#include "strbuf.h"
+#include "string-list.h"
static char bad_path[] = "/bad-path/";
@@ -582,20 +583,22 @@ int normalize_path_copy(char *dst, const char *src)
*/
int longest_ancestor_length(const char *path, const char *prefix_list)
{
+ struct string_list prefixes = STRING_LIST_INIT_DUP;
char buf[PATH_MAX+1];
- const char *ceil, *colon;
- int len, max_len = -1;
+ int i, max_len = -1;
if (prefix_list == NULL || !strcmp(path, "/"))
return -1;
- for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
- for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
- len = colon - ceil;
+ string_list_split(&prefixes, prefix_list, PATH_SEP, -1);
+
+ for (i = 0; i < prefixes.nr; i++) {
+ const char *ceil = prefixes.items[i].string;
+ int len = strlen(ceil);
+
if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
continue;
- strlcpy(buf, ceil, len+1);
- if (normalize_path_copy(buf, buf) < 0)
+ if (normalize_path_copy(buf, ceil) < 0)
continue;
len = strlen(buf);
if (len > 0 && buf[len-1] == '/')
@@ -608,6 +611,7 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
}
}
+ string_list_clear(&prefixes, 0);
return max_len;
}
--
1.8.0
^ permalink raw reply related
* [PATCH v4 3/8] Introduce new function real_path_if_valid()
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
In-Reply-To: <1351440987-26636-1-git-send-email-mhagger@alum.mit.edu>
The function is like real_path(), except that it returns NULL on error
instead of dying.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
abspath.c | 5 +++++
cache.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/abspath.c b/abspath.c
index f8a526f..40cdc46 100644
--- a/abspath.c
+++ b/abspath.c
@@ -153,6 +153,11 @@ const char *real_path(const char *path)
return real_path_internal(path, 1);
}
+const char *real_path_if_valid(const char *path)
+{
+ return real_path_internal(path, 0);
+}
+
static const char *get_pwd_cwd(void)
{
static char cwd[PATH_MAX + 1];
diff --git a/cache.h b/cache.h
index a58df84..b0d75bc 100644
--- a/cache.h
+++ b/cache.h
@@ -714,6 +714,7 @@ static inline int is_absolute_path(const char *path)
}
int is_directory(const char *);
const char *real_path(const char *path);
+const char *real_path_if_valid(const char *path);
const char *absolute_path(const char *path);
const char *relative_path(const char *abs, const char *base);
int normalize_path_copy(char *dst, const char *src);
--
1.8.0
^ permalink raw reply related
* [PATCH v4 2/8] real_path_internal(): add comment explaining use of cwd
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
In-Reply-To: <1351440987-26636-1-git-send-email-mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
abspath.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/abspath.c b/abspath.c
index a7ab8e9..f8a526f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -35,7 +35,14 @@ static const char *real_path_internal(const char *path, int die_on_error)
{
static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
char *retval = NULL;
+
+ /*
+ * If we have to temporarily chdir(), store the original CWD
+ * here so that we can chdir() back to it at the end of the
+ * function:
+ */
char cwd[1024] = "";
+
int buf_index = 1;
int depth = MAXDEPTH;
--
1.8.0
^ permalink raw reply related
* [PATCH v4 1/8] Introduce new static function real_path_internal()
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
In-Reply-To: <1351440987-26636-1-git-send-email-mhagger@alum.mit.edu>
It accepts a new parameter, die_on_error. If die_on_error is false,
it simply cleans up after itself and returns NULL rather than dying.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
abspath.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 72 insertions(+), 21 deletions(-)
diff --git a/abspath.c b/abspath.c
index 05f2d79..a7ab8e9 100644
--- a/abspath.c
+++ b/abspath.c
@@ -15,15 +15,26 @@ int is_directory(const char *path)
#define MAXDEPTH 5
/*
- * Use this to get the real path, i.e. resolve links. If you want an
- * absolute path but don't mind links, use absolute_path.
+ * Return the real path (i.e., absolute path, with symlinks resolved
+ * and extra slashes removed) equivalent to the specified path. (If
+ * you want an absolute path but don't mind links, use
+ * absolute_path().) The return value is a pointer to a static
+ * buffer.
+ *
+ * The input and all intermediate paths must be shorter than MAX_PATH.
+ * The directory part of path (i.e., everything up to the last
+ * dir_sep) must denote a valid, existing directory, but the last
+ * component need not exist. If die_on_error is set, then die with an
+ * informative error message if there is a problem. Otherwise, return
+ * NULL on errors (without generating any output).
*
* If path is our buffer, then return path, as it's already what the
* user wants.
*/
-const char *real_path(const char *path)
+static const char *real_path_internal(const char *path, int die_on_error)
{
static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+ char *retval = NULL;
char cwd[1024] = "";
int buf_index = 1;
@@ -35,11 +46,19 @@ const char *real_path(const char *path)
if (path == buf || path == next_buf)
return path;
- if (!*path)
- die("The empty string is not a valid path");
+ if (!*path) {
+ if (die_on_error)
+ die("The empty string is not a valid path");
+ else
+ goto error_out;
+ }
- if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
- die ("Too long path: %.*s", 60, path);
+ if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
+ if (die_on_error)
+ die("Too long path: %.*s", 60, path);
+ else
+ goto error_out;
+ }
while (depth--) {
if (!is_directory(buf)) {
@@ -54,20 +73,36 @@ const char *real_path(const char *path)
}
if (*buf) {
- if (!*cwd && !getcwd(cwd, sizeof(cwd)))
- die_errno ("Could not get current working directory");
+ if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+ if (die_on_error)
+ die_errno("Could not get current working directory");
+ else
+ goto error_out;
+ }
- if (chdir(buf))
- die_errno ("Could not switch to '%s'", buf);
+ if (chdir(buf)) {
+ if (die_on_error)
+ die_errno("Could not switch to '%s'", buf);
+ else
+ goto error_out;
+ }
+ }
+ if (!getcwd(buf, PATH_MAX)) {
+ if (die_on_error)
+ die_errno("Could not get current working directory");
+ else
+ goto error_out;
}
- if (!getcwd(buf, PATH_MAX))
- die_errno ("Could not get current working directory");
if (last_elem) {
size_t len = strlen(buf);
- if (len + strlen(last_elem) + 2 > PATH_MAX)
- die ("Too long path name: '%s/%s'",
- buf, last_elem);
+ if (len + strlen(last_elem) + 2 > PATH_MAX) {
+ if (die_on_error)
+ die("Too long path name: '%s/%s'",
+ buf, last_elem);
+ else
+ goto error_out;
+ }
if (len && !is_dir_sep(buf[len-1]))
buf[len++] = '/';
strcpy(buf + len, last_elem);
@@ -77,10 +112,18 @@ const char *real_path(const char *path)
if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
ssize_t len = readlink(buf, next_buf, PATH_MAX);
- if (len < 0)
- die_errno ("Invalid symlink '%s'", buf);
- if (PATH_MAX <= len)
- die("symbolic link too long: %s", buf);
+ if (len < 0) {
+ if (die_on_error)
+ die_errno("Invalid symlink '%s'", buf);
+ else
+ goto error_out;
+ }
+ if (PATH_MAX <= len) {
+ if (die_on_error)
+ die("symbolic link too long: %s", buf);
+ else
+ goto error_out;
+ }
next_buf[len] = '\0';
buf = next_buf;
buf_index = 1 - buf_index;
@@ -89,10 +132,18 @@ const char *real_path(const char *path)
break;
}
+ retval = buf;
+error_out:
+ free(last_elem);
if (*cwd && chdir(cwd))
die_errno ("Could not change back to '%s'", cwd);
- return buf;
+ return retval;
+}
+
+const char *real_path(const char *path)
+{
+ return real_path_internal(path, 1);
}
static const char *get_pwd_cwd(void)
--
1.8.0
^ permalink raw reply related
* [PATCH v4 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks
From: Michael Haggerty @ 2012-10-28 16:16 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jiang Xin, Lea Wiemann, Johannes Sixt, git,
Michael Haggerty
v4 of the series; see the cover letter for v3 [1] for more editorial
comments.
Changes since v3:
* For "test-path-utils longest_ancestor_length", normalize all of the
paths using normalize_path_copy() to counteract the path mangling
carried out by bash on Windows. (Thanks to Johannes Sixt for his
helpful advice.)
* Rebased onto a more recent master.
[1] http://thread.gmane.org/gmane.comp.version-control.git/208102
Michael Haggerty (8):
Introduce new static function real_path_internal()
real_path_internal(): add comment explaining use of cwd
Introduce new function real_path_if_valid()
longest_ancestor_length(): use string_list_split()
longest_ancestor_length(): take a string_list argument for prefixes
longest_ancestor_length(): require prefix list entries to be
normalized
setup_git_directory_gently_1(): resolve symlinks in ceiling paths
string_list_longest_prefix(): remove function
Documentation/technical/api-string-list.txt | 8 ---
abspath.c | 105 ++++++++++++++++++++++------
cache.h | 3 +-
path.c | 46 ++++++------
setup.c | 34 ++++++++-
string-list.c | 20 ------
string-list.h | 8 ---
t/t0060-path-utils.sh | 41 ++++-------
t/t0063-string-list.sh | 30 --------
test-path-utils.c | 51 +++++++++++++-
test-string-list.c | 20 ------
11 files changed, 202 insertions(+), 164 deletions(-)
--
1.8.0
^ permalink raw reply
* Re: [PATCH] Fix t9200 on case insensitive file systems
From: Torsten Bögershausen @ 2012-10-28 15:28 UTC (permalink / raw)
To: Jeff King
Cc: Torsten Bögershausen, git, bdwalton, bosch, brian,
robin.rosenberg
In-Reply-To: <20121028111053.GC11434@sigill.intra.peff.net>
On 28.10.12 12:10, Jeff King wrote:
> On Fri, Oct 26, 2012 at 06:18:24PM +0200, Torsten Bögershausen wrote:
>
>> t9200 defines $CVSROOT where cvs should init its repository
>> $CVSROOT is set to $PWD/cvsroot.
>> cvs init is supposed to create the repository inside $PWD/cvsroot/CVSROOT
>>
>> "cvs init" (e.g. version 1.11.23) checks if the last element of the path is
>> "CVSROOT", and if a directory with e.g. $PWD/cvsroot/CVSROOT already exists.
>>
>> For such a $CVSROOT cvs refuses to init a repository here:
>> "Cannot initialize repository under existing CVSROOT:
>>
>> On a case insenstive file system cvsroot and CVSROOT are the same directories
>> and t9200 fails.
>>
>> Solution: use $PWD/tmp/cvsroot instead of cvsroot $PWD/cvsroot
>
> Wouldn't tmp/cvsroot have the same problem, since the basename is still
> cvsroot?
>
>> diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
>> index b59be9a..69934b2 100755
>> --- a/t/t9200-git-cvsexportcommit.sh
>> +++ b/t/t9200-git-cvsexportcommit.sh
>> @@ -19,7 +19,7 @@ then
>> test_done
>> fi
>>
>> -CVSROOT=$PWD/cvsroot
>> +CVSROOT=$PWD/tmpcvsroot
>
> Ah, but here you do something different, which makes sense. Should I
> tweak the commit message?
>
Yes, please do so.
Thanks for spotting,
/torsten
^ permalink raw reply
* Re: [PATCH] Fix git p4 sync errors
From: Pete Wyckoff @ 2012-10-28 15:06 UTC (permalink / raw)
To: Matt Arsenault; +Cc: Junio C Hamano, git, Luke Diamand
In-Reply-To: <F0F92B0A-D37F-40D4-A0DF-43EEDA2818B9@gmail.com>
arsenm2@gmail.com wrote on Wed, 24 Oct 2012 19:41 -0700:
>
> On Oct 21, 2012, at 12:06 , Junio C Hamano <gitster@pobox.com> wrote:
> >
> >>
> >> This solves errors in some cases when syncing renamed files.
> >
> > Can you be a bit more descriptive? What are "errors in some case"?
> >
> It might just be when files are renamed. I ran into this after months of using it, and I'm skeptical that in that time no files were ever renamed. I'm not sure what was special about the file that was renamed. (There also might have been deleted files in the same commit, not sure if that matters)
I set up a test case where I did a "p4 move" on a file and tried
syncing it, with and without "-s" to describe. It works in both
cases, for an old (2009.2) and new (2012.1) version of p4.
The output of -s versus no -s does differ, and the differences
are different with server version worse yet. But in no case is
there ever a set of file differences. -G does seem to disable
that.
I'd love to track this down, but can't seem to provoke anything
on my own. Let me know if you have any hints based on what is in
your depot or server/client config. Or if you see it again.
> > In short, what I am getting at are:
> >
> > - What breaks by not passing "-s"? What are the user visible
> > symptoms?
>
> There's a key error on the line
> line 2198: epoch = details["time"]
> The details object is an error different fields set (I don't remember what it is exactly, I'm not at work right now)
This would happen if describe did not return a "time" field, but
there's an explicit check for that:
res = p4CmdList("describe -s %d" % newestRevision)
newestTime = None
for r in res:
if r.has_key('time'):
newestTime = int(r['time'])
if newestTime is None:
die("Output from \"describe -s\" on newest change %d did not give a time" %
newestRevision)
details["time"] = newestTime
so I'm confused how this could happen. Maybe your version is
older/different than what is in the git source?
I'm not against putting in your patch, since it is true we don't
want the file diff, and adding "-s" should be harmless in theory.
And it doesn't cause any existing tests to fail. It just scares
me that there's something else going on we haven't figured out.
-- Pete
^ permalink raw reply
* Re: Mistake in git-reset documentation
From: Krzysztof Mazur @ 2012-10-28 14:13 UTC (permalink / raw)
To: Jeff King; +Cc: Andreas Schwab, Bojan Petrović, git
In-Reply-To: <20121028134635.GA25519@sigill.intra.peff.net>
On Sun, Oct 28, 2012 at 09:46:35AM -0400, Jeff King wrote:
> On Sun, Oct 28, 2012 at 02:39:49PM +0100, Andreas Schwab wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > On Sun, Oct 28, 2012 at 09:36:10AM +0100, Krzysztof Mazur wrote:
> > >
> > >> DESCRIPTION
> > >> -----------
> > >> @@ -43,7 +43,7 @@ This means that `git reset -p` is the opposite of `git add -p`, i.e.
> > >> you can use it to selectively reset hunks. See the ``Interactive Mode''
> > >> section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
> > >>
> > >> -'git reset' --<mode> [<commit>]::
> > >> +'git reset' [--<mode>] [<commit>]::
> > >> This form resets the current branch head to <commit> and
> > >> possibly updates the index (resetting it to the tree of <commit>) and
> > >> the working tree depending on <mode>, which
> > >
> > > Should we say something like "if --<mode> is omitted, defaults to
> > > "--mixed"?
> >
> > Under --mixed it already says "This is the default action", though.
>
> I know, but that is somewhat buried for somebody who is seeing that the
> "--<mode>" bit is optional and wondering what it means to omit it.
>
The --mixed mode is also described as second mode, and saying that --mixed
is default earlier may save some time wasted on reading --soft
description.
There is also small inconsequence in what <mode> is, just "mixed" or
"--mixed".
Krzysiek
-- >8 --
Subject: [PATCH] doc: git-reset: make "<mode>" optional
The git-reset's "<mode>" is an optional argument, however it was
documented as required.
The "<mode>" is documented as one of: --soft, --mixed, --hard, --merge
or --keep, so "<mode>" should be used instead of "--<mode>".
Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
Documentation/git-reset.txt | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 117e374..978d8da 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,7 +10,7 @@ SYNOPSIS
[verse]
'git reset' [-q] [<commit>] [--] <paths>...
'git reset' (--patch | -p) [<commit>] [--] [<paths>...]
-'git reset' (--soft | --mixed | --hard | --merge | --keep) [-q] [<commit>]
+'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [<commit>]
DESCRIPTION
-----------
@@ -43,11 +43,11 @@ This means that `git reset -p` is the opposite of `git add -p`, i.e.
you can use it to selectively reset hunks. See the ``Interactive Mode''
section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
-'git reset' --<mode> [<commit>]::
+'git reset' [<mode>] [<commit>]::
This form resets the current branch head to <commit> and
possibly updates the index (resetting it to the tree of <commit>) and
- the working tree depending on <mode>, which
- must be one of the following:
+ the working tree depending on <mode>. If <mode> is omitted,
+ defaults to "--mixed". The <mode> must be one of the following:
+
--
--soft::
--
1.8.0.47.g5b520ba
^ permalink raw reply related
* Re: Mistake in git-reset documentation
From: Jeff King @ 2012-10-28 13:46 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Krzysztof Mazur, Bojan Petrović, git
In-Reply-To: <m2zk36hg0q.fsf@igel.home>
On Sun, Oct 28, 2012 at 02:39:49PM +0100, Andreas Schwab wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Sun, Oct 28, 2012 at 09:36:10AM +0100, Krzysztof Mazur wrote:
> >
> >> DESCRIPTION
> >> -----------
> >> @@ -43,7 +43,7 @@ This means that `git reset -p` is the opposite of `git add -p`, i.e.
> >> you can use it to selectively reset hunks. See the ``Interactive Mode''
> >> section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
> >>
> >> -'git reset' --<mode> [<commit>]::
> >> +'git reset' [--<mode>] [<commit>]::
> >> This form resets the current branch head to <commit> and
> >> possibly updates the index (resetting it to the tree of <commit>) and
> >> the working tree depending on <mode>, which
> >
> > Should we say something like "if --<mode> is omitted, defaults to
> > "--mixed"?
>
> Under --mixed it already says "This is the default action", though.
I know, but that is somewhat buried for somebody who is seeing that the
"--<mode>" bit is optional and wondering what it means to omit it.
-Peff
^ permalink raw reply
* Re: Mistake in git-reset documentation
From: Andreas Schwab @ 2012-10-28 13:39 UTC (permalink / raw)
To: Jeff King; +Cc: Krzysztof Mazur, Bojan Petrović, git
In-Reply-To: <20121028110715.GB11434@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Oct 28, 2012 at 09:36:10AM +0100, Krzysztof Mazur wrote:
>
>> DESCRIPTION
>> -----------
>> @@ -43,7 +43,7 @@ This means that `git reset -p` is the opposite of `git add -p`, i.e.
>> you can use it to selectively reset hunks. See the ``Interactive Mode''
>> section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
>>
>> -'git reset' --<mode> [<commit>]::
>> +'git reset' [--<mode>] [<commit>]::
>> This form resets the current branch head to <commit> and
>> possibly updates the index (resetting it to the tree of <commit>) and
>> the working tree depending on <mode>, which
>
> Should we say something like "if --<mode> is omitted, defaults to
> "--mixed"?
Under --mixed it already says "This is the default action", though.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH] mailmap: avoid out-of-bounds memory access
From: Romain Francoise @ 2012-10-28 13:21 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20121028110207.GA11434@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So you could also check for "left == nstart" before the loop even
> begins. I think your fix (to just make the loop more robust to that
> precondition) is better, though, as the rest of the code does the right
> thing with such a value of nend.
Yep.
> It looks like t4203 triggers this problem. Curious that valgrind does
> not find it. I guess since it does not have compiler support, it cannot
> find out-of-bound errors on stack buffers. Does the rest of the test
> suite turn up clean with AddressSanitizer?
I tested your 'master' and your 'pu' with expensive tests enabled and both
are clean after fixing t4203.
Thanks!
^ permalink raw reply
* [PATCH 2/2] pickaxe: use textconv for -S counting
From: Jeff King @ 2012-10-28 12:47 UTC (permalink / raw)
To: Peter Oberndorfer; +Cc: git, Junio C Hamano
In-Reply-To: <20121028124540.GF11434@sigill.intra.peff.net>
We currently just look at raw blob data when using "-S" to
pickaxe. This is mostly historical, as pickaxe predates the
textconv feature. If the user has bothered to define a
textconv filter, it is more likely that their search string will be
on the textconv output, as that is what they will see in the
diff (and we do not even provide a mechanism for them to
search for binary needles that contain NUL characters).
This patch teaches "-S" to use textconv, just as we
already do for "-G".
Signed-off-by: Jeff King <peff@peff.net>
---
diffcore-pickaxe.c | 56 +++++++++++++++++++++++++++++++++---------------
t/t4030-diff-textconv.sh | 12 +++++++++++
2 files changed, 51 insertions(+), 17 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 61f628c..b097fa7 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -157,17 +157,15 @@ static unsigned int contains(struct diff_filespec *one, struct diff_options *o,
return;
}
-static unsigned int contains(struct diff_filespec *one, struct diff_options *o,
+static unsigned int contains(mmfile_t *mf, struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
unsigned int cnt;
unsigned long sz;
const char *data;
- if (diff_populate_filespec(one, 0))
- return 0;
- sz = one->size;
- data = one->data;
+ sz = mf->size;
+ data = mf->ptr;
cnt = 0;
if (regexp) {
@@ -197,29 +195,53 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,
cnt++;
}
}
- diff_free_filespec_data(one);
return cnt;
}
static int has_changes(struct diff_filepair *p, struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
+ struct userdiff_driver *textconv_one = get_textconv(p->one);
+ struct userdiff_driver *textconv_two = get_textconv(p->two);
+ mmfile_t mf1, mf2;
+ int ret;
+
if (!o->pickaxe[0])
return 0;
- if (!DIFF_FILE_VALID(p->one)) {
- if (!DIFF_FILE_VALID(p->two))
- return 0; /* ignore unmerged */
+ /*
+ * If we have an unmodified pair, we know that the count will be the
+ * same and don't even have to load the blobs. Unless textconv is in
+ * play, _and_ we are using two different textconv filters (e.g.,
+ * because a pair is an exact rename with different textconv attributes
+ * for each side, which might generate different content).
+ */
+ if (textconv_one == textconv_two && diff_unmodified_pair(p))
+ return 0;
+
+ fill_one(p->one, &mf1, &textconv_one);
+ fill_one(p->two, &mf2, &textconv_two);
+
+ if (!mf1.ptr) {
+ if (!mf2.ptr)
+ ret = 0; /* ignore unmerged */
/* created */
- return contains(p->two, o, regexp, kws) != 0;
- }
- if (!DIFF_FILE_VALID(p->two))
- return contains(p->one, o, regexp, kws) != 0;
- if (!diff_unmodified_pair(p)) {
- return contains(p->one, o, regexp, kws) !=
- contains(p->two, o, regexp, kws);
+ ret = contains(&mf2, o, regexp, kws) != 0;
}
- return 0;
+ else if (!mf2.ptr) /* removed */
+ ret = contains(&mf1, o, regexp, kws) != 0;
+ else
+ ret = contains(&mf1, o, regexp, kws) !=
+ contains(&mf2, o, regexp, kws);
+
+ if (textconv_one)
+ free(mf1.ptr);
+ if (textconv_two)
+ free(mf2.ptr);
+ diff_free_filespec_data(p->one);
+ diff_free_filespec_data(p->two);
+
+ return ret;
}
static void diffcore_pickaxe_count(struct diff_options *o)
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 461d27a..53ec330 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -96,6 +96,18 @@ test_expect_success 'grep-diff (-G) operates on textconv data (modification)' '
test_cmp expect actual
'
+test_expect_success 'pickaxe (-S) operates on textconv data (add)' '
+ echo one >expect &&
+ git log --root --format=%s -S0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'pickaxe (-S) operates on textconv data (modification)' '
+ echo two >expect &&
+ git log --root --format=%s -S1 >actual &&
+ test_cmp expect actual
+'
+
cat >expect.stat <<'EOF'
file | Bin 2 -> 4 bytes
1 file changed, 0 insertions(+), 0 deletions(-)
--
1.8.0.3.g3456896
^ permalink raw reply related
* [PATCH 1/2] pickaxe: hoist empty needle check
From: Jeff King @ 2012-10-28 12:46 UTC (permalink / raw)
To: Peter Oberndorfer; +Cc: git, Junio C Hamano
In-Reply-To: <20121028124540.GF11434@sigill.intra.peff.net>
If we are given an empty pickaxe needle like "git log -S ''",
it is impossible for us to find anything (because no matter
what the content, the count will always be 0). We currently
check this at the lowest level of contains(). Let's hoist
the logic much earlier to has_changes(), so that it is
simpler to return our answer before loading any blob data.
Signed-off-by: Jeff King <peff@peff.net>
---
diffcore-pickaxe.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index a209376..61f628c 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -163,8 +163,6 @@ static unsigned int contains(struct diff_filespec *one, struct diff_options *o,
unsigned int cnt;
unsigned long sz;
const char *data;
- if (!o->pickaxe[0])
- return 0;
if (diff_populate_filespec(one, 0))
return 0;
@@ -206,6 +204,9 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,
static int has_changes(struct diff_filepair *p, struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
+ if (!o->pickaxe[0])
+ return 0;
+
if (!DIFF_FILE_VALID(p->one)) {
if (!DIFF_FILE_VALID(p->two))
return 0; /* ignore unmerged */
--
1.8.0.3.g3456896
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox