* Re: [PATCH v4] diff: handle --no-abbrev in no-index case
From: Junio C Hamano @ 2016-12-08 22:53 UTC (permalink / raw)
To: Jack Bates; +Cc: git, Jeff King, Jack Bates
In-Reply-To: <20161206165614.22921-1-jack@nottheoilrig.com>
Jack Bates <bk874k@nottheoilrig.com> writes:
> There are two different places where the --no-abbrev option is parsed,
> and two different places where SHA-1s are abbreviated. We normally parse
> --no-abbrev with setup_revisions(), but in the no-index case, "git diff"
> calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
> --no-abbrev until now. (It did handle --abbrev, however.) We normally
> abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
> handle sha1 abbreviations outside of repository, 2016-10-20) recently
> introduced a special case when you run "git diff" outside of a
> repository.
>
> setup_revisions() does also call diff_opt_parse(), but not for --abbrev
> or --no-abbrev, which it handles itself. setup_revisions() sets
> rev_info->abbrev, and later copies that to diff_options->abbrev. It
> handles --no-abbrev by setting abbrev to zero. (This change doesn't
> touch that.)
>
> Setting abbrev to zero was broken in the outside-of-a-repository special
> case, which until now resulted in a truly zero-length SHA-1, rather than
> taking zero to mean do not abbreviate. The only way to trigger this bug,
> however, was by running "git diff --raw" without either the --abbrev or
> --no-abbrev options, because 1) without --raw it doesn't respect abbrev
> (which is bizarre, but has been that way forever), 2) we silently clamp
> --abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
> now.
>
> The outside-of-a-repository case is one of three no-index cases. The
> other two are when one of the files you're comparing is outside of the
> repository you're in, and the --no-index option.
Nicely described.
> diff --git a/diff.c b/diff.c
> index ec87283..84dba60 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
> abbrev = FALLBACK_DEFAULT_ABBREV;
> if (abbrev > GIT_SHA1_HEXSZ)
> die("BUG: oid abbreviation out of range: %d", abbrev);
> - hex[abbrev] = '\0';
> + if (abbrev)
> + hex[abbrev] = '\0';
> return hex;
> }
> }
This is the same since your earlier round and it is correct. The
code before this part clamps abbrev to be between 0 and 40.
> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
>
> options->file = stdout;
>
> + options->abbrev = DEFAULT_ABBREV;
This is a new change relative to your earlier one.
I looked at all the callers of diff_setup() and noticed that many of
them were initializing "struct diff_options" that is on-stack that
is totally uninitialized, which means they were using a completely
random value that happened to be on the stack.
Which was surprising and made me wonder how the entire "diff" code
could have ever worked correctly for the past 10 years, as it's not
like all the users always passed --[no-]abbrev[=<value>] from the
command line.
In any case, this cannot possibly be introducing a regression; these
callsites of diff_setup() were starting from a random garbage---now
they start with -1 in this field. If they were doing the right
thing by assigning their own abbrev to the field after diff_setup()
returned, they will continue to do the same, and otherwise they will
keep doing whatever random things they have been doing when the
uninitialized field happened to contain -1 the same way.
I didn't look carefully at the additional tests, but the code change
looks good.
Thanks.
^ permalink raw reply
* [PATCH v2 0/4] road to reentrant real_path
From: Brandon Williams @ 2016-12-08 23:58 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1480964316-99305-1-git-send-email-bmwill@google.com>
Thanks for all of the comments on v1 of the series. Hopefully this series
addresses the issues with windows and actually passes the first test :)
Some changes in v2:
* the 1st component of a path should now be handled correctly on windows as well
as unix.
* Pushed the static strbuf to the callers of real_path_internal
* renamed real_path_internal to strbuf_realpath
* added real_pathdup
* migrated some callers of real_path to real_pathdup and strbuf_realpath
Brandon Williams (4):
real_path: resolve symlinks by hand
real_path: convert real_path_internal to strbuf_realpath
real_path: create real_pathdup
real_path: have callers use real_pathdup and strbuf_realpath
abspath.c | 215 ++++++++++++++++++++++++++++++++++++------------------
builtin/init-db.c | 6 +-
cache.h | 3 +
environment.c | 2 +-
setup.c | 15 ++--
sha1_file.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
worktree.c | 2 +-
9 files changed, 163 insertions(+), 86 deletions(-)
--- interdiff from v1
diff --git a/abspath.c b/abspath.c
index 6f546e0..df37356 100644
--- a/abspath.c
+++ b/abspath.c
@@ -14,13 +14,13 @@ int is_directory(const char *path)
/* removes the last path component from 'path' except if 'path' is root */
static void strip_last_component(struct strbuf *path)
{
- if (path->len > 1) {
+ if (path->len > offset_1st_component(path->buf)) {
char *last_slash = find_last_dir_sep(path->buf);
strbuf_setlen(path, last_slash - path->buf);
}
}
-/* gets the next component in 'remaining' and places it in 'next' */
+/* get (and remove) the next component in 'remaining' and place it in 'next' */
static void get_next_component(struct strbuf *next, struct strbuf *remaining)
{
char *start = NULL;
@@ -31,10 +31,10 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
/* look for the next component */
/* Skip sequences of multiple path-separators */
for (start = remaining->buf; is_dir_sep(*start); start++)
- /* nothing */;
+ ; /* nothing */
/* Find end of the path component */
for (end = start; *end && !is_dir_sep(*end); end++)
- /* nothing */;
+ ; /* nothing */
strbuf_add(next, start, end - start);
/* remove the component from 'remaining' */
@@ -48,21 +48,17 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
* 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.
+ * absolute_path().) Places the resolved realpath in the provided strbuf.
*
* 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.
*/
-static const char *real_path_internal(const char *path, int die_on_error)
+char *strbuf_realpath(struct strbuf *resolved, const char *path,
+ int die_on_error)
{
- static struct strbuf resolved = STRBUF_INIT;
struct strbuf remaining = STRBUF_INIT;
struct strbuf next = STRBUF_INIT;
struct strbuf symlink = STRBUF_INIT;
@@ -70,10 +66,6 @@ static const char *real_path_internal(const char *path, int die_on_error)
int num_symlinks = 0;
struct stat st;
- /* We've already done it */
- if (path == resolved.buf)
- return path;
-
if (!*path) {
if (die_on_error)
die("The empty string is not a valid path");
@@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- strbuf_reset(&resolved);
+ strbuf_reset(resolved);
if (is_absolute_path(path)) {
/* absolute path; start with only root as being resolved */
- strbuf_addch(&resolved, '/');
- strbuf_addstr(&remaining, path + 1);
+ int offset = offset_1st_component(path);
+ strbuf_add(resolved, path, offset);
+ strbuf_addstr(&remaining, path + offset);
} else {
/* relative path; can use CWD as the initial resolved path */
- if (strbuf_getcwd(&resolved)) {
+ if (strbuf_getcwd(resolved)) {
if (die_on_error)
- die_errno("Could not get current working directory");
+ die_errno("unable to get current working directory");
else
goto error_out;
}
@@ -108,21 +101,21 @@ static const char *real_path_internal(const char *path, int die_on_error)
continue; /* '.' component */
} else if (next.len == 2 && !strcmp(next.buf, "..")) {
/* '..' component; strip the last path component */
- strip_last_component(&resolved);
+ strip_last_component(resolved);
continue;
}
/* append the next component and resolve resultant path */
- if (resolved.buf[resolved.len - 1] != '/')
- strbuf_addch(&resolved, '/');
- strbuf_addbuf(&resolved, &next);
+ if (!is_dir_sep(resolved->buf[resolved->len - 1]))
+ strbuf_addch(resolved, '/');
+ strbuf_addbuf(resolved, &next);
- if (lstat(resolved.buf, &st)) {
+ if (lstat(resolved->buf, &st)) {
/* error out unless this was the last component */
if (!(errno == ENOENT && !remaining.len)) {
if (die_on_error)
die_errno("Invalid path '%s'",
- resolved.buf);
+ resolved->buf);
else
goto error_out;
}
@@ -138,29 +131,29 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- len = strbuf_readlink(&symlink, resolved.buf,
+ len = strbuf_readlink(&symlink, resolved->buf,
st.st_size);
if (len < 0) {
if (die_on_error)
die_errno("Invalid symlink '%s'",
- resolved.buf);
+ resolved->buf);
else
goto error_out;
}
if (is_absolute_path(symlink.buf)) {
- /*
- * absolute symlink
- * reset resolved path to root
- */
- strbuf_setlen(&resolved, 1);
+ /* absolute symlink; set resolved to root */
+ int offset = offset_1st_component(symlink.buf);
+ strbuf_reset(resolved);
+ strbuf_add(resolved, symlink.buf, offset);
+ strbuf_remove(&symlink, 0, offset);
} else {
/*
* relative symlink
* strip off the last component since it will
* be replaced with the contents of the symlink
*/
- strip_last_component(&resolved);
+ strip_last_component(resolved);
}
/*
@@ -180,25 +173,42 @@ static const char *real_path_internal(const char *path, int die_on_error)
}
}
- retval = resolved.buf;
+ retval = resolved->buf;
error_out:
- //strbuf_release(&resolved);
strbuf_release(&remaining);
strbuf_release(&next);
strbuf_release(&symlink);
+ if (!retval)
+ strbuf_reset(resolved);
+
return retval;
}
const char *real_path(const char *path)
{
- return real_path_internal(path, 1);
+ static struct strbuf realpath = STRBUF_INIT;
+ return strbuf_realpath(&realpath, path, 1);
}
const char *real_path_if_valid(const char *path)
{
- return real_path_internal(path, 0);
+ static struct strbuf realpath = STRBUF_INIT;
+ return strbuf_realpath(&realpath, path, 0);
+}
+
+char *real_pathdup(const char *path)
+{
+ struct strbuf realpath = STRBUF_INIT;
+ char *retval = NULL;
+
+ if(strbuf_realpath(&realpath, path, 0))
+ retval = strbuf_detach(&realpath, NULL);
+
+ strbuf_release(&realpath);
+
+ return retval;
}
/*
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2399b97..76d68fa 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
{
int reinit;
int exist_ok = flags & INIT_DB_EXIST_OK;
- char *original_git_dir = xstrdup(real_path(git_dir));
+ char *original_git_dir = real_pathdup(git_dir);
if (real_git_dir) {
struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
if (real_git_dir && !is_absolute_path(real_git_dir))
- real_git_dir = xstrdup(real_path(real_git_dir));
+ real_git_dir = real_pathdup(real_git_dir);
if (argc == 1) {
int mkdir_tried = 0;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
const char *git_dir_parent = strrchr(git_dir, '/');
if (git_dir_parent) {
char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
- git_work_tree_cfg = xstrdup(real_path(rel));
+ git_work_tree_cfg = real_pathdup(rel);
free(rel);
}
if (!git_work_tree_cfg)
diff --git a/cache.h b/cache.h
index a50a61a..e12a5d9 100644
--- a/cache.h
+++ b/cache.h
@@ -1064,8 +1064,11 @@ static inline int is_absolute_path(const char *path)
return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
}
int is_directory(const char *);
+char *strbuf_realpath(struct strbuf *resolved, const char *path,
+ int die_on_error);
const char *real_path(const char *path);
const char *real_path_if_valid(const char *path);
+char *real_pathdup(const char *path);
const char *absolute_path(const char *path);
const char *remove_leading_path(const char *in, const char *prefix);
const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
diff --git a/environment.c b/environment.c
index 0935ec6..9b943d2 100644
--- a/environment.c
+++ b/environment.c
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)
return;
}
git_work_tree_initialized = 1;
- work_tree = xstrdup(real_path(new_work_tree));
+ work_tree = real_pathdup(new_work_tree);
}
const char *get_git_work_tree(void)
diff --git a/setup.c b/setup.c
index fe572b8..0d9fdd0 100644
--- a/setup.c
+++ b/setup.c
@@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
if (!is_absolute_path(data.buf))
strbuf_addf(&path, "%s/", gitdir);
strbuf_addbuf(&path, &data);
- strbuf_addstr(sb, real_path(path.buf));
+ strbuf_realpath(sb, path.buf, 1);
ret = 1;
- } else
+ } else {
strbuf_addstr(sb, gitdir);
+ }
+
strbuf_release(&data);
strbuf_release(&path);
return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
if (offset != cwd->len && !is_absolute_path(gitdir))
- gitdir = xstrdup(real_path(gitdir));
+ gitdir = real_pathdup(gitdir);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
/* Keep entry but do not canonicalize it */
return 1;
} else {
- const char *real_path = real_path_if_valid(ceil);
- if (!real_path)
+ char *real_path = real_pathdup(ceil);
+ if (!real_path) {
return 0;
+ }
free(item->string);
- item->string = xstrdup(real_path);
+ item->string = real_path;
return 1;
}
}
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d19..6a03cc3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
struct strbuf pathbuf = STRBUF_INIT;
if (!is_absolute_path(entry) && relative_base) {
- strbuf_addstr(&pathbuf, real_path(relative_base));
+ strbuf_realpath(&pathbuf, relative_base, 1);
strbuf_addch(&pathbuf, '/');
}
strbuf_addstr(&pathbuf, entry);
diff --git a/submodule.c b/submodule.c
index 6f7d883..c85ba50 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1227,7 +1227,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
{
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
- const char *real_work_tree = xstrdup(real_path(work_tree));
+ const char *real_work_tree = real_pathdup(work_tree);
/* Update gitfile */
strbuf_addf(&file_name, "%s/.git", work_tree);
diff --git a/transport.c b/transport.c
index d57e8de..236c6f6 100644
--- a/transport.c
+++ b/transport.c
@@ -1130,7 +1130,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,
const struct ref *extra;
struct alternate_refs_data *cb = data;
- other = xstrdup(real_path(e->path));
+ other = real_pathdup(e->path);
len = strlen(other);
while (other[len-1] == '/')
diff --git a/worktree.c b/worktree.c
index f7869f8..c90e013 100644
--- a/worktree.c
+++ b/worktree.c
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,
return wt;
arg = prefix_filename(prefix, strlen(prefix), arg);
- path = xstrdup(real_path(arg));
+ path = real_pathdup(arg);
for (; *list; list++)
if (!fspathcmp(path, real_path((*list)->path)))
break;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 1/4] real_path: resolve symlinks by hand
From: Brandon Williams @ 2016-12-08 23:58 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1481241494-6861-1-git-send-email-bmwill@google.com>
The current implementation of real_path uses chdir() in order to resolve
symlinks. Unfortunately this isn't thread-safe as chdir() affects a
process as a whole and not just an individual thread. Instead perform
the symlink resolution by hand so that the calls to chdir() can be
removed, making real_path one step closer to being reentrant.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 122 insertions(+), 61 deletions(-)
diff --git a/abspath.c b/abspath.c
index 2825de8..92f2a29 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,8 +11,38 @@ int is_directory(const char *path)
return (!stat(path, &st) && S_ISDIR(st.st_mode));
}
+/* removes the last path component from 'path' except if 'path' is root */
+static void strip_last_component(struct strbuf *path)
+{
+ if (path->len > offset_1st_component(path->buf)) {
+ char *last_slash = find_last_dir_sep(path->buf);
+ strbuf_setlen(path, last_slash - path->buf);
+ }
+}
+
+/* get (and remove) the next component in 'remaining' and place it in 'next' */
+static void get_next_component(struct strbuf *next, struct strbuf *remaining)
+{
+ char *start = NULL;
+ char *end = NULL;
+
+ strbuf_reset(next);
+
+ /* look for the next component */
+ /* Skip sequences of multiple path-separators */
+ for (start = remaining->buf; is_dir_sep(*start); start++)
+ ; /* nothing */
+ /* Find end of the path component */
+ for (end = start; *end && !is_dir_sep(*end); end++)
+ ; /* nothing */
+
+ strbuf_add(next, start, end - start);
+ /* remove the component from 'remaining' */
+ strbuf_remove(remaining, 0, end - remaining->buf);
+}
+
/* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXDEPTH 5
+#define MAXSYMLINKS 5
/*
* Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +51,6 @@ int is_directory(const char *path)
* 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
@@ -33,22 +62,16 @@ int is_directory(const char *path)
*/
static const char *real_path_internal(const char *path, int die_on_error)
{
- static struct strbuf sb = STRBUF_INIT;
+ static struct strbuf resolved = STRBUF_INIT;
+ struct strbuf remaining = STRBUF_INIT;
+ struct strbuf next = STRBUF_INIT;
+ struct strbuf symlink = STRBUF_INIT;
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:
- */
- struct strbuf cwd = STRBUF_INIT;
-
- int depth = MAXDEPTH;
- char *last_elem = NULL;
+ int num_symlinks = 0;
struct stat st;
/* We've already done it */
- if (path == sb.buf)
+ if (path == resolved.buf)
return path;
if (!*path) {
@@ -58,74 +81,112 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- strbuf_reset(&sb);
- strbuf_addstr(&sb, path);
-
- while (depth--) {
- if (!is_directory(sb.buf)) {
- char *last_slash = find_last_dir_sep(sb.buf);
- if (last_slash) {
- last_elem = xstrdup(last_slash + 1);
- strbuf_setlen(&sb, last_slash - sb.buf + 1);
- } else {
- last_elem = xmemdupz(sb.buf, sb.len);
- strbuf_reset(&sb);
- }
+ strbuf_reset(&resolved);
+
+ if (is_absolute_path(path)) {
+ /* absolute path; start with only root as being resolved */
+ int offset = offset_1st_component(path);
+ strbuf_add(&resolved, path, offset);
+ strbuf_addstr(&remaining, path + offset);
+ } else {
+ /* relative path; can use CWD as the initial resolved path */
+ if (strbuf_getcwd(&resolved)) {
+ if (die_on_error)
+ die_errno("unable to get current working directory");
+ else
+ goto error_out;
+ }
+ strbuf_addstr(&remaining, path);
+ }
+
+ /* Iterate over the remaining path components */
+ while (remaining.len > 0) {
+ get_next_component(&next, &remaining);
+
+ if (next.len == 0) {
+ continue; /* empty component */
+ } else if (next.len == 1 && !strcmp(next.buf, ".")) {
+ continue; /* '.' component */
+ } else if (next.len == 2 && !strcmp(next.buf, "..")) {
+ /* '..' component; strip the last path component */
+ strip_last_component(&resolved);
+ continue;
}
- if (sb.len) {
- if (!cwd.len && strbuf_getcwd(&cwd)) {
+ /* append the next component and resolve resultant path */
+ if (!is_dir_sep(resolved.buf[resolved.len - 1]))
+ strbuf_addch(&resolved, '/');
+ strbuf_addbuf(&resolved, &next);
+
+ if (lstat(resolved.buf, &st)) {
+ /* error out unless this was the last component */
+ if (!(errno == ENOENT && !remaining.len)) {
if (die_on_error)
- die_errno("Could not get current working directory");
+ die_errno("Invalid path '%s'",
+ resolved.buf);
else
goto error_out;
}
+ } else if (S_ISLNK(st.st_mode)) {
+ ssize_t len;
+ strbuf_reset(&symlink);
- if (chdir(sb.buf)) {
+ if (num_symlinks++ > MAXSYMLINKS) {
if (die_on_error)
- die_errno("Could not switch to '%s'",
- sb.buf);
+ die("More than %d nested symlinks "
+ "on path '%s'", MAXSYMLINKS, path);
else
goto error_out;
}
- }
- if (strbuf_getcwd(&sb)) {
- if (die_on_error)
- die_errno("Could not get current working directory");
- else
- goto error_out;
- }
-
- if (last_elem) {
- if (sb.len && !is_dir_sep(sb.buf[sb.len - 1]))
- strbuf_addch(&sb, '/');
- strbuf_addstr(&sb, last_elem);
- free(last_elem);
- last_elem = NULL;
- }
- if (!lstat(sb.buf, &st) && S_ISLNK(st.st_mode)) {
- struct strbuf next_sb = STRBUF_INIT;
- ssize_t len = strbuf_readlink(&next_sb, sb.buf, 0);
+ len = strbuf_readlink(&symlink, resolved.buf,
+ st.st_size);
if (len < 0) {
if (die_on_error)
die_errno("Invalid symlink '%s'",
- sb.buf);
+ resolved.buf);
else
goto error_out;
}
- strbuf_swap(&sb, &next_sb);
- strbuf_release(&next_sb);
- } else
- break;
+
+ if (is_absolute_path(symlink.buf)) {
+ /* absolute symlink; set resolved to root */
+ int offset = offset_1st_component(symlink.buf);
+ strbuf_reset(&resolved);
+ strbuf_add(&resolved, symlink.buf, offset);
+ strbuf_remove(&symlink, 0, offset);
+ } else {
+ /*
+ * relative symlink
+ * strip off the last component since it will
+ * be replaced with the contents of the symlink
+ */
+ strip_last_component(&resolved);
+ }
+
+ /*
+ * if there are still remaining components to resolve
+ * then append them to symlink
+ */
+ if (remaining.len) {
+ strbuf_addch(&symlink, '/');
+ strbuf_addbuf(&symlink, &remaining);
+ }
+
+ /*
+ * use the symlink as the remaining components that
+ * need to be resloved
+ */
+ strbuf_swap(&symlink, &remaining);
+ }
}
- retval = sb.buf;
+ retval = resolved.buf;
+
error_out:
- free(last_elem);
- if (cwd.len && chdir(cwd.buf))
- die_errno("Could not change back to '%s'", cwd.buf);
- strbuf_release(&cwd);
+ strbuf_release(&remaining);
+ strbuf_release(&next);
+ strbuf_release(&symlink);
return retval;
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 2/4] real_path: convert real_path_internal to strbuf_realpath
From: Brandon Williams @ 2016-12-08 23:58 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1481241494-6861-1-git-send-email-bmwill@google.com>
Change the name of real_path_internal to strbuf_realpath. In addition
push the static strbuf up to its callers and instead take as a
parameter a pointer to a strbuf to use for the final result.
This change makes strbuf_realpath reentrant.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 53 +++++++++++++++++++++++++----------------------------
cache.h | 2 ++
2 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/abspath.c b/abspath.c
index 92f2a29..b0d4c1b 100644
--- a/abspath.c
+++ b/abspath.c
@@ -48,21 +48,17 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
* 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.
+ * absolute_path().) Places the resolved realpath in the provided strbuf.
*
* 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.
*/
-static const char *real_path_internal(const char *path, int die_on_error)
+char *strbuf_realpath(struct strbuf *resolved, const char *path,
+ int die_on_error)
{
- static struct strbuf resolved = STRBUF_INIT;
struct strbuf remaining = STRBUF_INIT;
struct strbuf next = STRBUF_INIT;
struct strbuf symlink = STRBUF_INIT;
@@ -70,10 +66,6 @@ static const char *real_path_internal(const char *path, int die_on_error)
int num_symlinks = 0;
struct stat st;
- /* We've already done it */
- if (path == resolved.buf)
- return path;
-
if (!*path) {
if (die_on_error)
die("The empty string is not a valid path");
@@ -81,16 +73,16 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- strbuf_reset(&resolved);
+ strbuf_reset(resolved);
if (is_absolute_path(path)) {
/* absolute path; start with only root as being resolved */
int offset = offset_1st_component(path);
- strbuf_add(&resolved, path, offset);
+ strbuf_add(resolved, path, offset);
strbuf_addstr(&remaining, path + offset);
} else {
/* relative path; can use CWD as the initial resolved path */
- if (strbuf_getcwd(&resolved)) {
+ if (strbuf_getcwd(resolved)) {
if (die_on_error)
die_errno("unable to get current working directory");
else
@@ -109,21 +101,21 @@ static const char *real_path_internal(const char *path, int die_on_error)
continue; /* '.' component */
} else if (next.len == 2 && !strcmp(next.buf, "..")) {
/* '..' component; strip the last path component */
- strip_last_component(&resolved);
+ strip_last_component(resolved);
continue;
}
/* append the next component and resolve resultant path */
- if (!is_dir_sep(resolved.buf[resolved.len - 1]))
- strbuf_addch(&resolved, '/');
- strbuf_addbuf(&resolved, &next);
+ if (!is_dir_sep(resolved->buf[resolved->len - 1]))
+ strbuf_addch(resolved, '/');
+ strbuf_addbuf(resolved, &next);
- if (lstat(resolved.buf, &st)) {
+ if (lstat(resolved->buf, &st)) {
/* error out unless this was the last component */
if (!(errno == ENOENT && !remaining.len)) {
if (die_on_error)
die_errno("Invalid path '%s'",
- resolved.buf);
+ resolved->buf);
else
goto error_out;
}
@@ -139,12 +131,12 @@ static const char *real_path_internal(const char *path, int die_on_error)
goto error_out;
}
- len = strbuf_readlink(&symlink, resolved.buf,
+ len = strbuf_readlink(&symlink, resolved->buf,
st.st_size);
if (len < 0) {
if (die_on_error)
die_errno("Invalid symlink '%s'",
- resolved.buf);
+ resolved->buf);
else
goto error_out;
}
@@ -152,8 +144,8 @@ static const char *real_path_internal(const char *path, int die_on_error)
if (is_absolute_path(symlink.buf)) {
/* absolute symlink; set resolved to root */
int offset = offset_1st_component(symlink.buf);
- strbuf_reset(&resolved);
- strbuf_add(&resolved, symlink.buf, offset);
+ strbuf_reset(resolved);
+ strbuf_add(resolved, symlink.buf, offset);
strbuf_remove(&symlink, 0, offset);
} else {
/*
@@ -161,7 +153,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
* strip off the last component since it will
* be replaced with the contents of the symlink
*/
- strip_last_component(&resolved);
+ strip_last_component(resolved);
}
/*
@@ -181,24 +173,29 @@ static const char *real_path_internal(const char *path, int die_on_error)
}
}
- retval = resolved.buf;
+ retval = resolved->buf;
error_out:
strbuf_release(&remaining);
strbuf_release(&next);
strbuf_release(&symlink);
+ if (!retval)
+ strbuf_reset(resolved);
+
return retval;
}
const char *real_path(const char *path)
{
- return real_path_internal(path, 1);
+ static struct strbuf realpath = STRBUF_INIT;
+ return strbuf_realpath(&realpath, path, 1);
}
const char *real_path_if_valid(const char *path)
{
- return real_path_internal(path, 0);
+ static struct strbuf realpath = STRBUF_INIT;
+ return strbuf_realpath(&realpath, path, 0);
}
/*
diff --git a/cache.h b/cache.h
index a50a61a..7a81294 100644
--- a/cache.h
+++ b/cache.h
@@ -1064,6 +1064,8 @@ static inline int is_absolute_path(const char *path)
return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
}
int is_directory(const char *);
+char *strbuf_realpath(struct strbuf *resolved, const char *path,
+ int die_on_error);
const char *real_path(const char *path);
const char *real_path_if_valid(const char *path);
const char *absolute_path(const char *path);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Junio C Hamano @ 2016-12-08 23:58 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller, jnareb
In-Reply-To: <20161207153627.1468-1-Karthik.188@gmail.com>
Thanks.
Will replace, with the attached stylistic fixes squashed in for
minor issues that were spotted by my mechanical pre-acceptance
filter.
ref-filter.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git b/ref-filter.c a/ref-filter.c
index a68ed7b147..a9d2c6a89d 100644
--- b/ref-filter.c
+++ a/ref-filter.c
@@ -76,7 +76,7 @@ static struct used_atom {
struct {
enum { RR_REF, RR_TRACK, RR_TRACKSHORT } option;
struct refname_atom refname;
- unsigned int nobracket: 1;
+ unsigned int nobracket : 1;
} remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
@@ -559,7 +559,7 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
if (if_then_else->cmp_status == COMPARE_EQUAL) {
if (!strcmp(if_then_else->str, cur->output.buf))
if_then_else->condition_satisfied = 1;
- } else if (if_then_else->cmp_status == COMPARE_UNEQUAL) {
+ } else if (if_then_else->cmp_status == COMPARE_UNEQUAL) {
if (strcmp(if_then_else->str, cur->output.buf))
if_then_else->condition_satisfied = 1;
} else if (cur->output.len && !is_empty(cur->output.buf))
@@ -1106,7 +1106,8 @@ static const char *lstrip_ref_components(const char *refname, int len)
const char *p = refname;
/* Find total no of '/' separated path-components */
- for (i = 0; p[i]; p[i] == '/' ? i++ : *p++);
+ for (i = 0; p[i]; p[i] == '/' ? i++ : *p++)
+ ;
/*
* The number of components we need to strip is now
* the total minus the components to be left (Plus one
@@ -1140,7 +1141,8 @@ static const char *rstrip_ref_components(const char *refname, int len)
const char *p = refname;
/* Find total no of '/' separated path-components */
- for (i = 0; p[i]; p[i] == '/' ? i++ : *p++);
+ for (i = 0; p[i]; p[i] == '/' ? i++ : *p++)
+ ;
/*
* The number of components we need to strip is now
* the total minus the components to be left (Plus one
^ permalink raw reply related
* [PATCH v2 3/4] real_path: create real_pathdup
From: Brandon Williams @ 2016-12-08 23:58 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1481241494-6861-1-git-send-email-bmwill@google.com>
Create real_pathdup which returns a caller owned string of the resolved
realpath based on the provide path.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
abspath.c | 13 +++++++++++++
cache.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/abspath.c b/abspath.c
index b0d4c1b..df37356 100644
--- a/abspath.c
+++ b/abspath.c
@@ -198,6 +198,19 @@ const char *real_path_if_valid(const char *path)
return strbuf_realpath(&realpath, path, 0);
}
+char *real_pathdup(const char *path)
+{
+ struct strbuf realpath = STRBUF_INIT;
+ char *retval = NULL;
+
+ if(strbuf_realpath(&realpath, path, 0))
+ retval = strbuf_detach(&realpath, NULL);
+
+ strbuf_release(&realpath);
+
+ return retval;
+}
+
/*
* Use this to get an absolute path from a relative one. If you want
* to resolve links, you should use real_path.
diff --git a/cache.h b/cache.h
index 7a81294..e12a5d9 100644
--- a/cache.h
+++ b/cache.h
@@ -1068,6 +1068,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
int die_on_error);
const char *real_path(const char *path);
const char *real_path_if_valid(const char *path);
+char *real_pathdup(const char *path);
const char *absolute_path(const char *path);
const char *remove_leading_path(const char *in, const char *prefix);
const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 4/4] real_path: have callers use real_pathdup and strbuf_realpath
From: Brandon Williams @ 2016-12-08 23:58 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1481241494-6861-1-git-send-email-bmwill@google.com>
Migrate callers of real_path() who duplicate the retern value to use
real_pathdup or strbuf_realpath.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/init-db.c | 6 +++---
environment.c | 2 +-
setup.c | 15 +++++++++------
sha1_file.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
worktree.c | 2 +-
7 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2399b97..76d68fa 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
{
int reinit;
int exist_ok = flags & INIT_DB_EXIST_OK;
- char *original_git_dir = xstrdup(real_path(git_dir));
+ char *original_git_dir = real_pathdup(git_dir);
if (real_git_dir) {
struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
if (real_git_dir && !is_absolute_path(real_git_dir))
- real_git_dir = xstrdup(real_path(real_git_dir));
+ real_git_dir = real_pathdup(real_git_dir);
if (argc == 1) {
int mkdir_tried = 0;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
const char *git_dir_parent = strrchr(git_dir, '/');
if (git_dir_parent) {
char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
- git_work_tree_cfg = xstrdup(real_path(rel));
+ git_work_tree_cfg = real_pathdup(rel);
free(rel);
}
if (!git_work_tree_cfg)
diff --git a/environment.c b/environment.c
index 0935ec6..9b943d2 100644
--- a/environment.c
+++ b/environment.c
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)
return;
}
git_work_tree_initialized = 1;
- work_tree = xstrdup(real_path(new_work_tree));
+ work_tree = real_pathdup(new_work_tree);
}
const char *get_git_work_tree(void)
diff --git a/setup.c b/setup.c
index fe572b8..0d9fdd0 100644
--- a/setup.c
+++ b/setup.c
@@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
if (!is_absolute_path(data.buf))
strbuf_addf(&path, "%s/", gitdir);
strbuf_addbuf(&path, &data);
- strbuf_addstr(sb, real_path(path.buf));
+ strbuf_realpath(sb, path.buf, 1);
ret = 1;
- } else
+ } else {
strbuf_addstr(sb, gitdir);
+ }
+
strbuf_release(&data);
strbuf_release(&path);
return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
if (offset != cwd->len && !is_absolute_path(gitdir))
- gitdir = xstrdup(real_path(gitdir));
+ gitdir = real_pathdup(gitdir);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
/* Keep entry but do not canonicalize it */
return 1;
} else {
- const char *real_path = real_path_if_valid(ceil);
- if (!real_path)
+ char *real_path = real_pathdup(ceil);
+ if (!real_path) {
return 0;
+ }
free(item->string);
- item->string = xstrdup(real_path);
+ item->string = real_path;
return 1;
}
}
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d19..6a03cc3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
struct strbuf pathbuf = STRBUF_INIT;
if (!is_absolute_path(entry) && relative_base) {
- strbuf_addstr(&pathbuf, real_path(relative_base));
+ strbuf_realpath(&pathbuf, relative_base, 1);
strbuf_addch(&pathbuf, '/');
}
strbuf_addstr(&pathbuf, entry);
diff --git a/submodule.c b/submodule.c
index 6f7d883..c85ba50 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1227,7 +1227,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
{
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
- const char *real_work_tree = xstrdup(real_path(work_tree));
+ const char *real_work_tree = real_pathdup(work_tree);
/* Update gitfile */
strbuf_addf(&file_name, "%s/.git", work_tree);
diff --git a/transport.c b/transport.c
index d57e8de..236c6f6 100644
--- a/transport.c
+++ b/transport.c
@@ -1130,7 +1130,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,
const struct ref *extra;
struct alternate_refs_data *cb = data;
- other = xstrdup(real_path(e->path));
+ other = real_pathdup(e->path);
len = strlen(other);
while (other[len-1] == '/')
diff --git a/worktree.c b/worktree.c
index f7869f8..c90e013 100644
--- a/worktree.c
+++ b/worktree.c
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,
return wt;
arg = prefix_filename(prefix, strlen(prefix), arg);
- path = xstrdup(real_path(arg));
+ path = real_pathdup(arg);
for (; *list; list++)
if (!fspathcmp(path, real_path((*list)->path)))
break;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v4] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-09 0:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <xmqqtwaerq1x.fsf@gitster.mtv.corp.google.com>
On 08/12/16 03:53 PM, Junio C Hamano wrote:
> Jack Bates <bk874k@nottheoilrig.com> writes:
>> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
>>
>> options->file = stdout;
>>
>> + options->abbrev = DEFAULT_ABBREV;
>
> This is a new change relative to your earlier one.
>
> I looked at all the callers of diff_setup() and noticed that many of
> them were initializing "struct diff_options" that is on-stack that
> is totally uninitialized, which means they were using a completely
> random value that happened to be on the stack.
>
> Which was surprising and made me wonder how the entire "diff" code
> could have ever worked correctly for the past 10 years, as it's not
> like all the users always passed --[no-]abbrev[=<value>] from the
> command line.
>
> In any case, this cannot possibly be introducing a regression; these
> callsites of diff_setup() were starting from a random garbage---now
> they start with -1 in this field. If they were doing the right
> thing by assigning their own abbrev to the field after diff_setup()
> returned, they will continue to do the same, and otherwise they will
> keep doing whatever random things they have been doing when the
> uninitialized field happened to contain -1 the same way.
>
> I didn't look carefully at the additional tests, but the code change
> looks good.
>
> Thanks.
Great, thanks for reviewing it!
^ permalink raw reply
* Re: [PATCH v2 00/16] pathspec cleanup
From: Junio C Hamano @ 2016-12-09 0:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, pclouds
In-Reply-To: <1481223550-65277-1-git-send-email-bmwill@google.com>
Will queue, but with fixes on issues spotted by my pre-acceptance
mechanical filter squashed in, to fix style issues in the
destination of code movements.
pathspec.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 08abdd3922..cabc02e79b 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -68,7 +68,7 @@ static struct pathspec_magic {
const char *name;
} pathspec_magic[] = {
{ PATHSPEC_FROMTOP, '/', "top" },
- { PATHSPEC_LITERAL,'\0', "literal" },
+ { PATHSPEC_LITERAL, '\0', "literal" },
{ PATHSPEC_GLOB, '\0', "glob" },
{ PATHSPEC_ICASE, '\0', "icase" },
{ PATHSPEC_EXCLUDE, '!', "exclude" },
@@ -290,8 +290,8 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
item->len--;
item->match[item->len] = '\0';
} else {
- die (_("Pathspec '%s' is in submodule '%.*s'"),
- item->original, ce_len, ce->name);
+ die(_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_len, ce->name);
}
}
}
@@ -364,10 +364,10 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
}
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
- strip_submodule_slash_cheap(item);
+ strip_submodule_slash_cheap(item);
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
- strip_submodule_slash_expensive(item);
+ strip_submodule_slash_expensive(item);
if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
^ permalink raw reply related
* Re: [PATCH v2 14/16] pathspec: create strip submodule slash helpers
From: Junio C Hamano @ 2016-12-09 0:28 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, pclouds
In-Reply-To: <1481223550-65277-15-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> +static void strip_submodule_slash_cheap(struct pathspec_item *item)
> +{
> + int i;
> +
> + if ((item->len >= 1 && item->match[item->len - 1] == '/') &&
> + (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
> + S_ISGITLINK(active_cache[i]->ce_mode)) {
> + item->len--;
> + item->match[item->len] = '\0';
> + }
> +}
I know that this is merely a moved code, but while I was reading
this, it triggered "Do not make an assignment inside if condition"
check. But more importantly, is the code even correct? If the path
for the submodule is unmerged, we would get a negative i that points
at the conflicting entry; don't we want to do something about it, at
least when we have a submodule entry at stage #2 (i.e. ours)?
^ permalink raw reply
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Jacob Keller @ 2016-12-09 1:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Git mailing list, Jakub Narębski
In-Reply-To: <xmqqa8c6tfhc.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 8, 2016 at 10:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> *1* A malformed formatting request (e.g. %(if) that is not closed)
> cannot be satisified but that is true for all refs and is
> outside of the scope of this discussion. The command should die
> and I think it already does.
Agreed. I was only making the case that if it could "possibly" be
satisfied, then we shouldn't die.
Thanks,
Jake
^ permalink raw reply
* [BUG] regarding `git add -p` and files containing wildcard characters
From: unixway.drive @ 2016-12-09 1:46 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2937 bytes --]
`git add -p` behaves incorrectly if modified file contains any wildcard
character. Consider 'random.diff' (attached). For this, interactive
'add' would first ask to add hunk with two diff headers (or with some
"random" header at the end):
$ git add -p
diff --git a/Random * b/Random *
index 01e79c3..01f03ff 100644
--- a/Random *
+++ b/Random *
@@ -1,3 +1,5 @@
1
+ a
2
+ b
3
diff --git a/Random Crap b/Random Crap
index 01e79c3..b4373d5 100644
--- a/Random Crap
+++ b/Random Crap
Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]? y
Then it will ask for some irrelevant hunk:
@@ -1,3 +1,5 @@
1
+ whoa
2
+ dude
3
Stage this hunk [y,n,q,a,d,/,K,g,s,e,?]? y
…and, if confirmed, apply it to the wildcard-named file instead of what
was confirmed for staging before:
$ git diff --cached
diff --git a/Random * b/Random *
index 01e79c3..283ac91 100644
--- a/Random *
+++ b/Random *
@@ -1,3 +1,5 @@
1
+ whoa
2
+ dude
3
Then one can just repeat this over and over again (unless safely named
files are resolved first):
$ git add -p
diff --git a/Random * b/Random *
index 283ac91..04c92f1 100644
--- a/Random *
+++ b/Random *
@@ -1,5 +1,5 @@
1
- whoa
+ a
2
- dude
+ b
3
diff --git a/Random Crap b/Random Crap
index 01e79c3..283ac91 100644
--- a/Random Crap
+++ b/Random Crap
Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,?]?
The problem is that `git-diff-files` does some globbing on the 'path'
arguments on its own and has no option to disable that (and
`git-add--interactive`'s `run_cmd_pipe` already handles all other sorts
of unsafe characters like spaces and backticks well).
$ strace -f -e trace=execve git add -p
…
[pid 1713] execve("/usr/lib/git-core/git",
["git", "diff-files", "-p", "--", "Random *"],
[/* 36 vars */]) = 0
…
$ git diff-files -- 'Random *'
:100644 100644 … … M Random *
:100644 100644 … … M Random Crap
For all the eager people (although I doubt if anybody else is lazy
enough to not bother with file names like these), this could be easily
worked around like this:
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d812..358d877 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,3 +2,3 @@
-use 5.008;
+use 5.014;
use strict;
@@ -761,3 +761,5 @@ sub parse_diff {
}
- my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
+ my @diff = run_cmd_pipe("git", @diff_cmd, "--", (
+ $path =~ s#[\[*?]#\\$&#gr
+ ));
my @colored = ();
(Although a `git-diff-files` option is clearly a better solution that
would cover tools other than `git-add--interactive` as well.)
[-- Attachment #2: random.diff --]
[-- Type: text/x-patch, Size: 278 bytes --]
diff --git a/Random * b/Random *
index 01e79c3..04c92f1 100644
--- a/Random *
+++ b/Random *
@@ -1,3 +1,5 @@
1
+ a
2
+ b
3
diff --git a/Random Crap b/Random Crap
index 01e79c3..283ac91 100644
--- a/Random Crap
+++ b/Random Crap
@@ -1,3 +1,5 @@
1
+ whoa
2
+ dude
3
^ permalink raw reply related
* Re: [PATCH v2 1/4] real_path: resolve symlinks by hand
From: Jacob Keller @ 2016-12-09 1:49 UTC (permalink / raw)
To: Brandon Williams
Cc: Git mailing list, Stefan Beller, Jeff King, Junio C Hamano,
Ramsay Jones, Torsten Bögershausen, Johannes Sixt,
Duy Nguyen
In-Reply-To: <1481241494-6861-2-git-send-email-bmwill@google.com>
On Thu, Dec 8, 2016 at 3:58 PM, Brandon Williams <bmwill@google.com> wrote:
> The current implementation of real_path uses chdir() in order to resolve
> symlinks. Unfortunately this isn't thread-safe as chdir() affects a
> process as a whole and not just an individual thread. Instead perform
> the symlink resolution by hand so that the calls to chdir() can be
> removed, making real_path one step closer to being reentrant.
>
Better description for this part of the change. I like the
improvement, as it clearly indicates what this particular patch is
about, and why, but doesn't over-state what we gain here.
The rest of this seems reasonable, though I'm not super familiar with
the code, so I didn't have any comments.
Thanks,
Jake
^ permalink raw reply
* [PATCH] commit: remove 'Clever' message for --only --amend
From: Andreas Krey @ 2016-12-09 4:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <xmqq1sxiv051.fsf@gitster.mtv.corp.google.com>
That behavior is now documented, and we don't
need a reward afterwards.
Signed-off-by: Andreas Krey <a.krey@gmx.de>
---
> Sorry for making you send an extra round; let's queue the original,
> and if you still are interested, have the "Clever" removal as its
> own patch.
Here you go.
builtin/commit.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 89b66816f..276c74034 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1208,8 +1208,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
if (argc == 0 && (also || (only && !amend && !allow_empty)))
die(_("No paths with --include/--only does not make sense."));
- if (argc == 0 && only && amend)
- only_include_assumed = _("Clever... amending the last one with dirty index.");
if (argc > 0 && !also && !only)
only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths...");
if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
--
2.11.0.10.g1e1b186.dirty
^ permalink raw reply related
* Re: [BUG] regarding `git add -p` and files containing wildcard characters
From: Jeff King @ 2016-12-09 5:37 UTC (permalink / raw)
To: unixway.drive; +Cc: git
In-Reply-To: <c9876671-6252-5dfa-18df-a6719dc6834c@gmail.com>
On Fri, Dec 09, 2016 at 04:46:49AM +0300, unixway.drive@gmail.com wrote:
> The problem is that `git-diff-files` does some globbing on the 'path'
> arguments on its own and has no option to disable that (and
> `git-add--interactive`'s `run_cmd_pipe` already handles all other sorts of
> unsafe characters like spaces and backticks well).
I think the option you're looking for is:
git --literal-pathspecs diff-files ... -- 'Random *'
I don't know if there are other commands run by add--interactive that
would want the same treatment. It might actually make sense to set
GIT_LITERAL_PATHSPECS=1 in the environment right after it expands the
list via ls-files.
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index ee3d812..358d877 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -2,3 +2,3 @@
>
> -use 5.008;
> +use 5.014;
> use strict;
> @@ -761,3 +761,5 @@ sub parse_diff {
> }
> - my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
> + my @diff = run_cmd_pipe("git", @diff_cmd, "--", (
> + $path =~ s#[\[*?]#\\$&#gr
> + ));
This callsite covers "-p". It looks like list_modified() probably needs
similar treatment. Maybe others; I didn't look exhaustively.
-Peff
^ permalink raw reply
* git bash error
From: Karamjeet Singh @ 2016-12-09 6:08 UTC (permalink / raw)
To: git
Dear git support,
My app is crashing whenever i launch the git bash tool. I am attaching the
error log file from the event viewer. Can you please let me know what the
issue is with it.
https://www.dropbox.com/sh/mhkmjn8bmh3x1oh/AABUKmhnn-HW2Kv5UVxdckN6a?dl=0
Thanks
Karamjeet
^ permalink raw reply
* Any interest in 'git merge --continue' as a command
From: Chris Packham @ 2016-12-09 7:57 UTC (permalink / raw)
To: GIT
I hit this at $dayjob recently.
A developer had got themselves into a confused state when needing to
resolve a merge conflict.
They knew about git rebase --continue (and git am and git cherry-pick)
but they were unsure how to "continue" a merge (it didn't help that
the advice saying to use 'git commit' was scrolling off the top of the
terminal). I know that using 'git commit' has been the standard way to
complete a merge but given other commands have a --continue should
merge have it as well?
^ permalink raw reply
* Re: git bash error
From: Konstantin Khomoutov @ 2016-12-09 8:25 UTC (permalink / raw)
To: git; +Cc: Karamjeet Singh
In-Reply-To: <58D2713C848141E88F0156A1BF3A8B19@Karamjeet>
On Fri, 9 Dec 2016 11:38:55 +0530
"Karamjeet Singh" <karamjeet.singh@netsutra.com> wrote:
> Dear git support,
> My app is crashing whenever i launch the git bash tool. I am
> attaching the error log file from the event viewer. Can you please
> let me know what the issue is with it.
> https://www.dropbox.com/sh/mhkmjn8bmh3x1oh/AABUKmhnn-HW2Kv5UVxdckN6a?dl=0
Hi!
Your report misses lots of information required to even approach it:
- What Git version are you using (the fact is: in each next version of
a software package some bugs get fixed and others might creep in;
so knowing an exact version is paramount).
- What OS? Version, flavor, architecture (32/64 bit).
- What software package (i.e. where did you get your Git install from)?
From the term "git bash", I gather you're talking about Git for Windows.
If so, that project has its own bug tracker on Github [1] -- because
it's still a project sort-of separate from the "vanilla" Git due to
the fact it maintains a set of changes not yet in the Git proper, and
they do packaging work, too.
Please use that issue tracker in two steps:
1) Search it for your issue. Say, remove the "is:open" modifier from
the search box in the tracker's web interface, put there the words
"git", "bash" and "crash" and search. I'm sure you'll get a hefty
amount of reports. Please see whether your issue is already
reported.
2) If yes, and if (and only if) you have additional details about it,
please summarise them in a comment. Please try to write that in
plain English (plain bad non-native English is okay :-)); try not to
post links to pictures or videos. They aren't indexed by search
engines and require the maintainers to switch their context when
reading your report/comment. On some platforms (say, w/o proper
full-blown web browser) they can even be plain hard to even see.
3) If no, write your report there -- by filling the offerred template.
Thanks.
1. https://github.com/git-for-windows/git/issues/
^ permalink raw reply
* [PATCH 1/3] difftool: sanitize $workdir as early as possible
From: David Aguilar @ 2016-12-09 8:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML
The double-slash fixup on the $workdir variable was being
performed just-in-time to avoid double-slashes in symlink
targets, but the rest of the code was silently using paths with
embedded "//" in them.
A recent user-reported error message contained double-slashes.
Eliminate the issue by sanitizing inputs as soon as they arrive.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool.perl | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 959822d5f3..17c336321f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -224,9 +224,7 @@ EOF
delete($ENV{GIT_INDEX_FILE});
# Changes in the working tree need special treatment since they are
- # not part of the index. Remove any trailing slash from $workdir
- # before starting to avoid double slashes in symlink targets.
- $workdir =~ s|/$||;
+ # not part of the index.
for my $file (@working_tree) {
my $dir = dirname($file);
unless (-d "$rdir/$dir") {
@@ -389,6 +387,7 @@ sub dir_diff
my $repo = Git->repository();
my $repo_path = $repo->repo_path();
my $workdir = $repo->wc_path();
+ $workdir =~ s|/$||; # Avoid double slashes in symlink targets
my ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks);
if (defined($extcmd)) {
--
2.11.0.26.gb65c994
^ permalink raw reply related
* [PATCH 3/3] difftool: rename variables for consistency
From: David Aguilar @ 2016-12-09 8:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML
In-Reply-To: <20161209085848.10929-1-davvid@gmail.com>
Always call the list of files @files.
Always call the worktree $worktree.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool.perl | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 99b03949bf..4e4f5d8138 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -100,7 +100,7 @@ sub changed_files
sub setup_dir_diff
{
- my ($workdir, $symlinks) = @_;
+ my ($worktree, $symlinks) = @_;
my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
my $diffrtn = Git::command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
@@ -109,7 +109,7 @@ sub setup_dir_diff
# changed files. The paths returned by diff --raw are relative to the
# top-level of the repository, but we defer changing directories so
# that @ARGV can perform pathspec limiting in the current directory.
- chdir($workdir);
+ chdir($worktree);
# Build index info for left and right sides of the diff
my $submodule_mode = '160000';
@@ -121,7 +121,7 @@ sub setup_dir_diff
my $wtindex = '';
my %submodule;
my %symlink;
- my @working_tree = ();
+ my @files = ();
my %working_tree_dups = ();
my @rawdiff = split('\0', $diffrtn);
@@ -173,14 +173,14 @@ EOF
}
if ($rmode ne $null_mode) {
- # Avoid duplicate working_tree entries
+ # Avoid duplicate entries
if ($working_tree_dups{$dst_path}++) {
next;
}
my ($use, $wt_sha1) =
use_wt_file($dst_path, $rsha1);
if ($use) {
- push @working_tree, $dst_path;
+ push @files, $dst_path;
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
} else {
$rindex .= "$rmode $rsha1\t$dst_path\0";
@@ -227,14 +227,14 @@ EOF
# Changes in the working tree need special treatment since they are
# not part of the index.
- for my $file (@working_tree) {
+ for my $file (@files) {
my $dir = dirname($file);
unless (-d "$rdir/$dir") {
mkpath("$rdir/$dir") or
exit_cleanup($tmpdir, 1);
}
if ($symlinks) {
- symlink("$workdir/$file", "$rdir/$file") or
+ symlink("$worktree/$file", "$rdir/$file") or
exit_cleanup($tmpdir, 1);
} else {
copy($file, "$rdir/$file") or
@@ -278,7 +278,7 @@ EOF
exit_cleanup($tmpdir, 1) if not $ok;
}
- return ($ldir, $rdir, $tmpdir, @working_tree);
+ return ($ldir, $rdir, $tmpdir, @files);
}
sub write_to_file
@@ -388,9 +388,9 @@ sub dir_diff
my $error = 0;
my $repo = Git->repository();
my $repo_path = $repo->repo_path();
- my $workdir = $repo->wc_path();
- $workdir =~ s|/$||; # Avoid double slashes in symlink targets
- my ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks);
+ my $worktree = $repo->wc_path();
+ $worktree =~ s|/$||; # Avoid double slashes in symlink targets
+ my ($a, $b, $tmpdir, @files) = setup_dir_diff($worktree, $symlinks);
if (defined($extcmd)) {
$rc = system($extcmd, $a, $b);
@@ -411,13 +411,13 @@ sub dir_diff
my %tmp_modified;
my $indices_loaded = 0;
- for my $file (@worktree) {
+ for my $file (@files) {
next if $symlinks && -l "$b/$file";
next if ! -f "$b/$file";
if (!$indices_loaded) {
%wt_modified = changed_files(
- $repo_path, "$tmpdir/wtindex", $workdir);
+ $repo_path, "$tmpdir/wtindex", $worktree);
%tmp_modified = changed_files(
$repo_path, "$tmpdir/wtindex", $b);
$indices_loaded = 1;
@@ -425,7 +425,7 @@ sub dir_diff
if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
my $errmsg = "warning: Both files modified: ";
- $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
+ $errmsg .= "'$worktree/$file' and '$b/$file'.\n";
$errmsg .= "warning: Working tree file has been left.\n";
$errmsg .= "warning:\n";
warn $errmsg;
--
2.11.0.26.gb65c994
^ permalink raw reply related
* [PATCH 2/3] difftool: chdir as early as possible
From: David Aguilar @ 2016-12-09 8:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML
In-Reply-To: <20161209085848.10929-1-davvid@gmail.com>
Make difftool chdir to the top-level of the repository as soon as it can
so that we can simplify how paths are handled. Replace construction of
absolute paths via string concatenation with relative paths wherever
possible. The bulk of the code no longer needs to use absolute paths.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool.perl | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 17c336321f..99b03949bf 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -59,14 +59,14 @@ sub exit_cleanup
sub use_wt_file
{
- my ($workdir, $file, $sha1) = @_;
+ my ($file, $sha1) = @_;
my $null_sha1 = '0' x 40;
- if (-l "$workdir/$file" || ! -e _) {
+ if (-l $file || ! -e _) {
return (0, $null_sha1);
}
- my $wt_sha1 = Git::command_oneline('hash-object', "$workdir/$file");
+ my $wt_sha1 = Git::command_oneline('hash-object', $file);
my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
return ($use, $wt_sha1);
}
@@ -105,6 +105,12 @@ sub setup_dir_diff
my $diffrtn = Git::command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
+ # Go to the root of the worktree now that we've captured the list of
+ # changed files. The paths returned by diff --raw are relative to the
+ # top-level of the repository, but we defer changing directories so
+ # that @ARGV can perform pathspec limiting in the current directory.
+ chdir($workdir);
+
# Build index info for left and right sides of the diff
my $submodule_mode = '160000';
my $symlink_mode = '120000';
@@ -172,7 +178,7 @@ EOF
next;
}
my ($use, $wt_sha1) =
- use_wt_file($workdir, $dst_path, $rsha1);
+ use_wt_file($dst_path, $rsha1);
if ($use) {
push @working_tree, $dst_path;
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
@@ -182,10 +188,6 @@ EOF
}
}
- # Go to the root of the worktree so that the left index files
- # are properly setup -- the index is toplevel-relative.
- chdir($workdir);
-
# Setup temp directories
my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
@@ -235,10 +237,10 @@ EOF
symlink("$workdir/$file", "$rdir/$file") or
exit_cleanup($tmpdir, 1);
} else {
- copy("$workdir/$file", "$rdir/$file") or
+ copy($file, "$rdir/$file") or
exit_cleanup($tmpdir, 1);
- my $mode = stat("$workdir/$file")->mode;
+ my $mode = stat($file)->mode;
chmod($mode, "$rdir/$file") or
exit_cleanup($tmpdir, 1);
}
@@ -430,10 +432,10 @@ sub dir_diff
$error = 1;
} elsif (exists $tmp_modified{$file}) {
my $mode = stat("$b/$file")->mode;
- copy("$b/$file", "$workdir/$file") or
+ copy("$b/$file", $file) or
exit_cleanup($tmpdir, 1);
- chmod($mode, "$workdir/$file") or
+ chmod($mode, $file) or
exit_cleanup($tmpdir, 1);
}
}
--
2.11.0.26.gb65c994
^ permalink raw reply related
* Re: Any interest in 'git merge --continue' as a command
From: Jeff King @ 2016-12-09 9:11 UTC (permalink / raw)
To: Chris Packham; +Cc: GIT
In-Reply-To: <CAFOYHZDs5rBt5+4D_ViMYfV04foq3h_UrsSMA3FfyMzLh9QdwA@mail.gmail.com>
On Fri, Dec 09, 2016 at 08:57:58PM +1300, Chris Packham wrote:
> I hit this at $dayjob recently.
>
> A developer had got themselves into a confused state when needing to
> resolve a merge conflict.
>
> They knew about git rebase --continue (and git am and git cherry-pick)
> but they were unsure how to "continue" a merge (it didn't help that
> the advice saying to use 'git commit' was scrolling off the top of the
> terminal). I know that using 'git commit' has been the standard way to
> complete a merge but given other commands have a --continue should
> merge have it as well?
It seems like that would be in line with 35d2fffdb (Provide 'git merge
--abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated
goal was providing consistency with other multi-command operations.
I assume it would _just_ run a vanilla "git commit", and not try to do
any trickery with updating the index (which could be disastrous).
-Peff
^ permalink raw reply
* Re: [REGRESSION 2.10.2] problematic "empty auth" changes
From: David Turner @ 2016-12-08 21:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1612081538260.23160@virtualbox>
On Thu, 2016-12-08 at 15:47 +0100, Johannes Schindelin wrote:
> Hi Dave,
>
> I got a couple of bug reports that claim that 2.10.2 regressed on using
> network credentials. That is, users regularly hit Enter twice when being
> asked for user name and password while fetching via https://, and cURL
> automatically used to fall back to using the login credentials (i.e.
> authenticating via the Domain controller).
>
> Turns out those claims are correct: hitting Enter twice (or using URLs
> with empty user name/password such as https://:tfs:8080/) work in 2.10.1
> and yield "Authentication failed" in 2.10.2.
>
> I tracked this down to 5275c3081c (http: http.emptyauth should allow empty
> (not just NULL) usernames, 2016-10-04) which all of a sudden disallowed
> empty user names (and now only handles things correctly when
> http.emptyAuth is set to true specifically).
>
> This smells like a real bad regression to me, certainly given the time I
> had to spend to figure this out (starting from not exactly helpful bug
> reports, due to being very specific to their setups being private).
>
> I am *really* tempted to change the default of http.emptyAuth to true, *at
> least* for Windows (where it is quite common to use your login credentials
> to authenticate to corporate servers).
>
> Before I do anything rash, though: Do you see any downside to that?
I know of no reason that shouldn't work. Indeed, it's what we use do
internally. So far, nobody has reported problems. That said, we have
exactly three sets of git servers that most users talk to (two different
internal; and occasionally github.com for external stuff). So our
coverage is not very broad.
If you're going to do it, tho, don't just do it for Windows users -- do
it for everyone. Plenty of Unix clients connect to Windows-based auth
systems.
That said, I could imagine that there are cases where it would cause
failures for a different set of users. I don't know of any off the top
of my head, but this is not my area of expertise.
We could move closer to the old behavior with something like:
if (!http_auth.username || !*http_auth.username) {
if (curl_empty_auth)
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
if (!http_auth.username)
return;
}
This is very ad-hoc, in that I have not examined exactly when
http_auth.username would be null vs empty; it merely attempts to get as
close as possible to the old behavior.
[Side note: I was curious if 26a7b23429 would have been a better fix for
our problem. It appears that the answer is no; using that patch but
minus my 5275c3081c does not work for us.]
^ permalink raw reply
* Re: Error after calling git difftool -d with
From: David Aguilar @ 2016-12-09 9:47 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: P. Duijst, git
In-Reply-To: <alpine.DEB.2.20.1612051142550.117539@virtualbox>
On Mon, Dec 05, 2016 at 11:56:31AM +0100, Johannes Schindelin wrote:
> Hi Peter,
>
> On Mon, 5 Dec 2016, P. Duijst wrote:
>
> > On 12/5/2016 06:15, David Aguilar wrote:
> > > On Fri, Dec 02, 2016 at 05:05:06PM +0100, Johannes Schindelin wrote:
> > > >
> > > > On Fri, 2 Dec 2016, P. Duijst wrote:
> > > >
> > > > > Incase filenames are used with a quote ' or a bracket [ (and
> > > > > maybe some more characters), git "diff" and "difftool -y" works
> > > > > fine, but git *difftool **-d* gives the next error message:
> > > > >
> > > > > peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> > > > > $ git diff
> > > > > diff --git a/Test ''inch.txt b/Test ''inch.txt
> > > > > index dbff793..41f3257 100644
> > > > > --- a/Test ''inch.txt
> > > > > +++ b/Test ''inch.txt
> > > > > @@ -1 +1,3 @@
> > > > > +
> > > > > +ddd
> > > > > Test error in simple repository
> > > > > warning: LF will be replaced by CRLF in Test ''inch.txt.
> > > > > The file will have its original line endings in your working
> > > > > directory.
> > > > >
> > > > > peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> > > > > *$ git difftool -d*
> > > > > *fatal: Cannot open '/d/Dev/test//Test ''inch.txt': No such file or
> > > > > directory*
> > > > > *hash-object /d/Dev/test//Test ''inch.txt: command returned error:
> > > > > 128*
> > > > >
> > > > > peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> > > > > $
> > > > >
> > > > >
> > > > > This issue is inside V2.10.x and V2.11.0.
> > > > > V2.9.0 is working correctly...
> > > > You say v2.11.0, but did you also try the new, experimental builtin
> > > > difftool? You can test without reinstalling:
> > > >
> > > > git -c difftool.useBuiltin=true difftool -d ...
> > >
> > > FWIW, I verified that this problem does not manifest itself on Linux,
> > > using the current scripted difftool.
> > >
> > > Peter, what actual diff tool are you using?
> > >
> > > Since these filenames work fine with "difftool -d" on Linux, it
> > > suggests that this is either a tool-specific issue, or an issue
> > > related to unix-to-windows path translation.
> >
> > @Johannes: "git -c difftool.useBuiltin=true difftool -d" works OK :-), beyond
> > compare is launching with the diff's displayed
>
> Perfect.
>
> In that case, I think it is not worth fixing the scripted tool but focus
> on getting rid of it in favor of the builtin version.
>
> It's not like it is the only problem with having difftool implemented
> as a script...
I just sent some patches[1] that makes it so that difftool always
operates from the top-level of the repo, particularly when
calling hash-object. They also eliminate using paths with
embedded "//" in them, both of which may have caused this issue
Though we can side-step this specific issue with the new builtin
difftool, if our use of hash-object with double-slashed absolute
paths was not the problem reported above, then another
possibility is that there's a problem in the Git.pm Perl module,
which affects more than just difftool.
I'm curious to understand the root cause of the problem.
Does Git.pm go through a shell on Windows?
Why was hash-object complaining about the correct path,
but reported that it didn't exist?
Did having "//" in the path cause the problem?
Enlightenment from the GFW internals perspective is much
appreciated.
Since this reportedly worked in older versions, I'm led to
believe that 32b8c581ec (difftool: use Git::* functions instead
of passing around state), which first introduced the use of
paths with embedded "//", was the root cause. If this is true
then the patches should fix the scripted difftool on Windows.
[1] http://public-inbox.org/git/20161209085848.10929-1-davvid@gmail.com/T/#t
cheers,
--
David
^ permalink raw reply
* Re: Any interest in 'git merge --continue' as a command
From: Jacob Keller @ 2016-12-09 10:37 UTC (permalink / raw)
To: Jeff King, Chris Packham; +Cc: GIT
In-Reply-To: <20161209091127.sxxczhfslrqsqs3m@sigill.intra.peff.net>
On December 9, 2016 1:11:27 AM PST, Jeff King <peff@peff.net> wrote:
>On Fri, Dec 09, 2016 at 08:57:58PM +1300, Chris Packham wrote:
>
>> I hit this at $dayjob recently.
>>
>> A developer had got themselves into a confused state when needing to
>> resolve a merge conflict.
>>
>> They knew about git rebase --continue (and git am and git
>cherry-pick)
>> but they were unsure how to "continue" a merge (it didn't help that
>> the advice saying to use 'git commit' was scrolling off the top of
>the
>> terminal). I know that using 'git commit' has been the standard way
>to
>> complete a merge but given other commands have a --continue should
>> merge have it as well?
>
>It seems like that would be in line with 35d2fffdb (Provide 'git merge
>--abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated
>goal was providing consistency with other multi-command operations.
>
>I assume it would _just_ run a vanilla "git commit", and not try to do
>any trickery with updating the index (which could be disastrous).
>
>-Peff
This makes sense to me.
Thanks,
Jake
^ 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