* [PATCHv7 0/6] submodule absorbgitdirs
From: Stefan Beller @ 2016-12-12 19:04 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
v8:
* Change the worktree implementation: do not have an internal
submodule_get_worktrees, and count the number of worktrees, instead
directly look if there is a worktrees dir and check if there are any files
in there.
* reworded one test title slightly.
* interdiff to v7 (that is queued as origin/sb/submodule-embed-gitdir) below.
v7:
* do not expose submodule_get_worktrees. The values may be wrong internally,
but for our purpose we do not care about the actual values, only the count.
Document the wrong values!
* more strings are _(marked up)
* renamed variables in connect_work_tree_and_git_dir
* unsigned instead of int for options in the submodule helper.
*
v6:
* renamed embedgitdirs to absorbgitdirs embedding may be interpreted as
embedding the git dir into the working directory, whereas absorbing sounds
more like the submodule is absorbed by the superproject, making the
submodule less independent
* Worktrees API offer uses_worktrees(void) and submodule_uses_worktree(path).
* moved the printing to stderr and one layer up (out of the pure
relocate_git_dir function).
* connect_... is in dir.h now.
v5:
* Add another layer of abstraction, i.e. the relocate_git_dir is only about
moving a git dir of one repository. The submodule specific stuff (e.g.
recursion into nested submodules) is in submodule.{c,h}
This was motivated by reviews on the series of checkout aware of submodules
building on top of this series, as we want to directly call the embed-git-dirs
function without the overhead of spawning a child process.
v4:
* rebuilt on top of nd/worktree-list-fixup
* fix and test behavior for un-init submodules (don't crash, rather do nothing)
* incorporated a "static" as pointed out by Ramsay
* use internal functions instead of duplicating code in worktree.c
(use get_common_dir_noenv for the submodule to actually get the common dir)
* fixed a memory leak in relocate_gitdir
v3:
* have a slightly more generic function "relocate_gitdir".
The recursion is strictly related to submodules, though.
* bail out if a submodule is using worktrees.
This also lays the groundwork for later doing the proper thing,
as worktree.h offers a function `get_submodule_worktrees(path)`
* nit by duy: use git_path instead of git_common_dir
v2:
* fixed commit message for patch:
"submodule: use absolute path for computing relative path connecting"
* a new patch "submodule helper: support super prefix"
* redid the final patch with more tests and fixing bugs along the way
* "test-lib-functions.sh: teach test_commit -C <dir>" unchanged
v1:
The discussion of the submodule checkout series revealed to me that a command
is needed to move the git directory from the submodules working tree to be
embedded into the superprojects git directory.
So I wrote the code to intern the submodules git dir into the superproject,
but whilst writing the code I realized this could be valueable for our use
in testing too. So I exposed it via the submodule--helper. But as the
submodule helper ought to be just an internal API, we could also
offer it via the proper submodule command.
The command as it is has little value to the end user for now, but
breaking it out of the submodule checkout series hopefully makes review easier.
Thanks,
Stefan
Stefan Beller (6):
submodule: use absolute path for computing relative path connecting
submodule helper: support super prefix
test-lib-functions.sh: teach test_commit -C <dir>
worktree: have a function to check if worktrees are in use
move connect_work_tree_and_git_dir to dir.h
submodule: add absorb-git-dir function
Documentation/git-submodule.txt | 15 +++++
builtin/submodule--helper.c | 69 ++++++++++++++++----
dir.c | 37 +++++++++++
dir.h | 4 ++
git-submodule.sh | 7 +-
git.c | 2 +-
submodule.c | 127 ++++++++++++++++++++++++++++++-------
submodule.h | 5 +-
t/t7412-submodule-absorbgitdirs.sh | 101 +++++++++++++++++++++++++++++
t/test-lib-functions.sh | 20 ++++--
worktree.c | 68 +++++++++++++++++---
worktree.h | 7 ++
12 files changed, 409 insertions(+), 53 deletions(-)
create mode 100755 t/t7412-submodule-absorbgitdirs.sh
diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
index 7c4e8b8d79..1c47780e2b 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -93,7 +93,7 @@ test_expect_success 'setup a submodule with multiple worktrees' '
git -C sub3 worktree add ../sub3_second_work_tree
'
-test_expect_success 'absorb a submodule with multiple worktrees' '
+test_expect_success 'absorbing fails for a submodule with multiple worktrees' '
test_must_fail git submodule absorbgitdirs sub3 2>error &&
test_i18ngrep "not supported" error
'
diff --git a/worktree.c b/worktree.c
index 1c46fcf25f..d4606aa8cd 100644
--- a/worktree.c
+++ b/worktree.c
@@ -72,7 +72,7 @@ static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
/**
* get the main worktree
*/
-static struct worktree *get_main_worktree(const char *git_common_dir)
+static struct worktree *get_main_worktree(void)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(const char *git_common_dir)
int is_bare = 0;
int is_detached = 0;
- strbuf_add_absolute_path(&worktree_path, git_common_dir);
+ strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
if (is_bare)
strbuf_strip_suffix(&worktree_path, "/.");
- strbuf_addf(&path, "%s/HEAD", git_common_dir);
+ strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -101,8 +101,7 @@ static struct worktree *get_main_worktree(const char *git_common_dir)
return worktree;
}
-static struct worktree *get_linked_worktree(const char *git_common_dir,
- const char *id)
+static struct worktree *get_linked_worktree(const char *id)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -113,7 +112,7 @@ static struct worktree *get_linked_worktree(const char *git_common_dir,
if (!id)
die("Missing linked worktree name");
- strbuf_addf(&path, "%s/worktrees/%s/gitdir", git_common_dir, id);
+ strbuf_git_common_path(&path, "worktrees/%s/gitdir", id);
if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
/* invalid gitdir file */
goto done;
@@ -126,7 +125,7 @@ static struct worktree *get_linked_worktree(const char *git_common_dir,
}
strbuf_reset(&path);
- strbuf_addf(&path, "%s/worktrees/%s/HEAD", git_common_dir, id);
+ strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
goto done;
@@ -168,8 +167,7 @@ static int compare_worktree(const void *a_, const void *b_)
return fspathcmp((*a)->path, (*b)->path);
}
-static struct worktree **get_worktrees_internal(const char *git_common_dir,
- unsigned flags)
+struct worktree **get_worktrees(unsigned flags)
{
struct worktree **list = NULL;
struct strbuf path = STRBUF_INIT;
@@ -179,10 +177,9 @@ static struct worktree **get_worktrees_internal(const char *git_common_dir,
list = xmalloc(alloc * sizeof(struct worktree *));
- if (!(flags & GWT_LINKED_ONLY))
- list[counter++] = get_main_worktree(git_common_dir);
+ list[counter++] = get_main_worktree();
- strbuf_addf(&path, "%s/worktrees", git_common_dir);
+ strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
dir = opendir(path.buf);
strbuf_release(&path);
if (dir) {
@@ -191,7 +188,7 @@ static struct worktree **get_worktrees_internal(const char *git_common_dir,
if (is_dot_or_dotdot(d->d_name))
continue;
- if ((linked = get_linked_worktree(git_common_dir, d->d_name))) {
+ if ((linked = get_linked_worktree(d->d_name))) {
ALLOC_GROW(list, counter + 1, alloc);
list[counter++] = linked;
}
@@ -212,34 +209,6 @@ static struct worktree **get_worktrees_internal(const char *git_common_dir,
return list;
}
-struct worktree **get_worktrees(unsigned flags)
-{
- return get_worktrees_internal(get_git_common_dir(), flags);
-}
-
-/*
- * NEEDSWORK: The values in the returned worktrees are broken, e.g.
- * the refs or path resolution is influenced by the current repository.
- */
-static struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
-{
- char *submodule_gitdir;
- struct strbuf sb = STRBUF_INIT;
- struct worktree **ret;
-
- submodule_gitdir = git_pathdup_submodule(path, "%s", "");
- if (!submodule_gitdir)
- return NULL;
-
- /* the env would be set for the superproject */
- get_common_dir_noenv(&sb, submodule_gitdir);
- ret = get_worktrees_internal(sb.buf, flags);
-
- strbuf_release(&sb);
- free(submodule_gitdir);
- return ret;
-}
-
const char *get_worktree_git_dir(const struct worktree *wt)
{
if (!wt)
@@ -412,19 +381,52 @@ const struct worktree *find_shared_symref(const char *symref,
return existing;
}
-int uses_worktrees(void)
-{
- struct worktree **worktrees = get_worktrees(GWT_LINKED_ONLY);
- int retval = (worktrees != NULL && worktrees[0] != NULL);
- free_worktrees(worktrees);
- return retval;
-}
-
int submodule_uses_worktrees(const char *path)
{
- struct worktree **worktrees =
- get_submodule_worktrees(path, GWT_LINKED_ONLY);
- int retval = (worktrees != NULL && worktrees[0] != NULL);
- free_worktrees(worktrees);
- return retval;
+ char *submodule_gitdir;
+ struct strbuf sb = STRBUF_INIT;
+ DIR *dir;
+ struct dirent *d;
+ int ret;
+ struct repository_format format;
+
+ submodule_gitdir = git_pathdup_submodule(path, "%s", "");
+ if (!submodule_gitdir)
+ return 0;
+
+ /* The env would be set for the superproject. */
+ get_common_dir_noenv(&sb, submodule_gitdir);
+
+ /*
+ * The check below is only known to be good for repository format
+ * version 0 at the time of writing this code.
+ */
+ strbuf_addstr(&sb, "/config");
+ read_repository_format(&format, sb.buf);
+ if (format.version != 0) {
+ strbuf_release(&sb);
+ return 1;
+ }
+
+ /* Replace config by worktrees. */
+ strbuf_setlen(&sb, sb.len - strlen("config"));
+ strbuf_addstr(&sb, "worktrees");
+
+ /* See if there is any file inside the worktrees directory. */
+ dir = opendir(sb.buf);
+ strbuf_release(&sb);
+ free(submodule_gitdir);
+
+ if (!dir)
+ return 0;
+
+ while ((d = readdir(dir)) != NULL) {
+ if (is_dot_or_dotdot(d->d_name))
+ continue;
+
+ ret = 1;
+ break;
+ }
+ closedir(dir);
+ return ret;
}
diff --git a/worktree.h b/worktree.h
index ebe0856330..6bfb985203 100644
--- a/worktree.h
+++ b/worktree.h
@@ -16,7 +16,6 @@ struct worktree {
/* Functions for acting on the information about worktrees. */
#define GWT_SORT_LINKED (1 << 0) /* keeps linked worktrees sorted */
-#define GWT_LINKED_ONLY (1 << 1) /* do not include the main worktree */
/*
* Get the worktrees. The primary worktree will always be the first returned,
@@ -31,7 +30,6 @@ extern struct worktree **get_worktrees(unsigned flags);
/*
* Returns 1 if linked worktrees exist, 0 otherwise.
*/
-extern int uses_worktrees(void);
extern int submodule_uses_worktrees(const char *path);
/*
--
2.11.0.rc2.29.g7c00390.dirty
^ permalink raw reply related
* Re: [PULL] minor git-svn updates (probably for 2.11.x)
From: Junio C Hamano @ 2016-12-12 18:49 UTC (permalink / raw)
To: Eric Wong; +Cc: git
In-Reply-To: <20161212111320.GA25451@starla>
Eric Wong <e@80x24.org> writes:
> The following changes since commit 8d7a455ed52e2a96debc080dfc011b6bb00db5d2:
>
> Start post 2.11 cycle (2016-12-05 11:31:47 -0800)
>
> are available in the git repository at:
>
> git://bogomips.org/git-svn.git master
>
> for you to fetch changes up to 1b7edec78b754a1e901c164a4bf4e94bff96ed7b:
>
> git-svn: document useLogAuthor and addAuthorFrom config keys (2016-12-12 11:09:29 +0000)
>
> ----------------------------------------------------------------
> Eric Wong (2):
> git-svn: allow "0" in SVN path components
> git-svn: document useLogAuthor and addAuthorFrom config keys
>
> Documentation/git-svn.txt | 8 +++++++-
> perl/Git/SVN/Ra.pm | 2 +-
> 2 files changed, 8 insertions(+), 2 deletions(-)
Thanks; basing these two on top of "Start post 2.11 cycle" that is
on 'master' would mean that this won't merge to 'maint' for 2.11.x,
though. I hope you wouldn't mind if I rebased them on top of
'maint' before merging?
^ permalink raw reply
* Re: [PATCH 1/3] update_unicode.sh: update the uniset repo if it exists
From: Junio C Hamano @ 2016-12-12 18:33 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Beat Bolli, git
In-Reply-To: <ca10a51a-0fab-e4a4-8d7d-035673af4c06@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> If I run ./update_unicode.sh on the latest master of
> https://github.com/depp/uniset.git , commit
> a5fac4a091857dd5429cc2d, I get a diff in unicode_width.h like
> this:
>
> -{ 0x0300, 0x036F },
>
> +{ 768, 879 },
>
> IOW, all hex values are printed as decimal values.
> Not a problem for the compiler, but for the human
> to check the unicode tables.
>
> So I think we should "pin" the version of uniset.
Sure, and I'd rather see the update-unicode.sh script moved
somewhere in contrib/ while at it. Those who are interested in
keeping up with the unicode standard are tiny minority of the
developer population, and most of us would treat the built width
table as the source (after all, that is what we ship).
To be bluntly honest, I'd rather not to see "update-unicode.sh"
download and build uniset at all. It's as if po/ hierarchy shipping
with its own script to download and build msgmerge--that's madness.
Needless to say, shipping the sources for uniset embedded in our
project tree (either as a snapshot-fork or as a submodule) is even
worse. Those who want to muck with po/ are expected to have
msgmerge and friends. Why not expect the same for those who want to
update the unicode width table?
I'd rather see a written instruction telling which snapshot to get
and from where to build and place on their $PATH in the README file,
sitting next to the update-unicode.sh script in contrib/uniwidth/
directory, for those who are interested in building the width table
"from the source", and the update-unicode.sh script to assume that
uniset is available.
^ permalink raw reply
* [PATCH v3 3/4] real_path: create real_pathdup
From: Brandon Williams @ 2016-12-12 18:16 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1481566615-75299-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 8c6c76b..79ee310 100644
--- a/abspath.c
+++ b/abspath.c
@@ -205,6 +205,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 v3 2/4] real_path: convert real_path_internal to strbuf_realpath
From: Brandon Williams @ 2016-12-12 18:16 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1481566615-75299-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 cafcae0..8c6c76b 100644
--- a/abspath.c
+++ b/abspath.c
@@ -55,21 +55,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;
@@ -77,10 +73,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");
@@ -88,16 +80,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
@@ -116,21 +108,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;
}
@@ -146,12 +138,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;
}
@@ -159,8 +151,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 {
/*
@@ -168,7 +160,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);
}
/*
@@ -188,24 +180,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
* [PATCH v3 4/4] real_path: have callers use real_pathdup and strbuf_realpath
From: Brandon Williams @ 2016-12-12 18:16 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1481566615-75299-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 | 13 ++++++++-----
sha1_file.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
worktree.c | 2 +-
7 files changed, 16 insertions(+), 13 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..1b534a7 100644
--- a/setup.c
+++ b/setup.c
@@ -256,8 +256,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
strbuf_addbuf(&path, &data);
strbuf_addstr(sb, real_path(path.buf));
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 v3 1/4] real_path: resolve symlinks by hand
From: Brandon Williams @ 2016-12-12 18:16 UTC (permalink / raw)
To: git
Cc: Brandon Williams, sbeller, peff, jacob.keller, gitster, ramsay,
tboegi, j6t, pclouds
In-Reply-To: <1481566615-75299-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 | 190 ++++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 129 insertions(+), 61 deletions(-)
diff --git a/abspath.c b/abspath.c
index 2825de8..cafcae0 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,8 +11,45 @@ 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)
+{
+ size_t offset = offset_1st_component(path->buf);
+ size_t len = path->len;
+
+ /* Find start of the last component */
+ while (offset < len && !is_dir_sep(path->buf[len - 1]))
+ len--;
+ /* Skip sequences of multiple path-separators */
+ while (offset < len && is_dir_sep(path->buf[len - 1]))
+ len--;
+
+ strbuf_setlen(path, len);
+}
+
+/* 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 +58,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 +69,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 +88,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);
+ }
- if (sb.len) {
- if (!cwd.len && strbuf_getcwd(&cwd)) {
+ /* 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;
+ }
+
+ /* 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 v3 0/4] road to reentrant real_path
From: Brandon Williams @ 2016-12-12 18:16 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>
Changes in v3:
* Rewrite of `strip_last_component()` as the v2 verison didn't properly handle
inputs like '/foo'. Thanks to Johannes for pointing this out and suggesting
a solution.
* Small style changes
* Revert the call in `get_common_dir_noenv()` to maintain proper functionality.
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 | 222 ++++++++++++++++++++++++++++++++++++------------------
builtin/init-db.c | 6 +-
cache.h | 3 +
environment.c | 2 +-
setup.c | 13 ++--
sha1_file.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
worktree.c | 2 +-
9 files changed, 169 insertions(+), 85 deletions(-)
--- interdiff from v2
diff --git a/abspath.c b/abspath.c
index df37356..79ee310 100644
--- a/abspath.c
+++ b/abspath.c
@@ -14,10 +14,17 @@ 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 > offset_1st_component(path->buf)) {
- char *last_slash = find_last_dir_sep(path->buf);
- strbuf_setlen(path, last_slash - path->buf);
- }
+ size_t offset = offset_1st_component(path->buf);
+ size_t len = path->len;
+
+ /* Find start of the last component */
+ while (offset < len && !is_dir_sep(path->buf[len - 1]))
+ len--;
+ /* Skip sequences of multiple path-separators */
+ while (offset < len && is_dir_sep(path->buf[len - 1]))
+ len--;
+
+ strbuf_setlen(path, len);
}
/* get (and remove) the next component in 'remaining' and place it in 'next' */
@@ -112,7 +119,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
if (lstat(resolved->buf, &st)) {
/* error out unless this was the last component */
- if (!(errno == ENOENT && !remaining.len)) {
+ if (errno != ENOENT || remaining.len) {
if (die_on_error)
die_errno("Invalid path '%s'",
resolved->buf);
@@ -203,7 +210,7 @@ char *real_pathdup(const char *path)
struct strbuf realpath = STRBUF_INIT;
char *retval = NULL;
- if(strbuf_realpath(&realpath, path, 0))
+ if (strbuf_realpath(&realpath, path, 0))
retval = strbuf_detach(&realpath, NULL);
strbuf_release(&realpath);
diff --git a/setup.c b/setup.c
index 0d9fdd0..1b534a7 100644
--- a/setup.c
+++ b/setup.c
@@ -254,7 +254,7 @@ 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_realpath(sb, path.buf, 1);
+ strbuf_addstr(sb, real_path(path.buf));
ret = 1;
} else {
strbuf_addstr(sb, gitdir);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH 1/3] update_unicode.sh: update the uniset repo if it exists
From: Torsten Bögershausen @ 2016-12-12 18:12 UTC (permalink / raw)
To: Beat Bolli, Torsten Bögershausen; +Cc: git
In-Reply-To: <c96d013c38df7737cfd551a0fce87314@drbeat.li>
>> Minor question, especially to the next commit:
>> Should we make sure to checkout the exact version, which has been tested?
>> In this case cb97792880625e24a9f581412d03659091a0e54f
>>
>> And this is for both a fresh clone and the git pull
>> needs to be replaced by
>> git fetch && git checkout cb97792880625e24a9f581412d03659091a0e54f
>>
>>
>> (Which of course is a shell variable)
>
> I was actually wondering what the policy was for adding submodules to the Git repo,
> but then decided against it. Another option would be to fork uniset on GitHub and
> just let it stay on a working commit.
>
> Junio, what's your stance on this?
>
> Beat
If I run ./update_unicode.sh on the latest master of https://github.com/depp/uniset.git ,
commit a5fac4a091857dd5429cc2d, I get a diff in unicode_width.h like this:
-{ 0x0300, 0x036F },
+{ 768, 879 },
IOW, all hex values are printed as decimal values.
Not a problem for the compiler, but for the human
to check the unicode tables.
So I think we should "pin" the version of uniset.
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Junio C Hamano @ 2016-12-12 18:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: vi0oss, Stefan Beller, git@vger.kernel.org
In-Reply-To: <CAGZ79kYUbsy2TQ1noqS-9zLVUkQaeJbv6vwxykS+A_HHcxGnCw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Sat, Dec 10, 2016 at 5:41 AM, vi0oss <vi0oss@gmail.com> wrote:
>> On 12/08/2016 04:38 AM, vi0oss@gmail.com wrote:
>>>
>>> Third review: missing && in test fixed.
>>>
>>
>> Shall something more be done about this or just wait until the patch gets
>> reviewed and integrated?
>
> I have no further comments and think the most recent version you sent
> to the list
> is fine. However others are invited to comment as well. :)
I'll take that as a reviewed-by from you and queue it.
Thanks, both.
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Karthik Nayak @ 2016-12-12 17:18 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <20161212164020.btrrg2f62haxt7sh@sigill.intra.peff.net>
On Mon, Dec 12, 2016 at 10:10 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 12, 2016 at 09:59:49PM +0530, Karthik Nayak wrote:
>
>> >> > This caller never stores the return value, and it ends up leaking. So I
>> >> > wonder if you wanted "static struct strbuf" in the first place (and that
>> >> > would explain the strbuf_reset() in your function).
>> >>
>> >> Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
>> >> suggestion.
>> >>
>> >> strbuf_detach() is also a better way to go.
>> >
>> > One of the other, though. If it's static, then you _don't_ want to
>> > detach.
>> >
>>
>> Wait. Why not? since it gets added to the strbuf's within
>> build_format() and that
>> value is not needed again, why do we need to re-allocate? we can use the same
>> variable again (i.e by keeping it as static).
>>
>> I'm sorry, but I didn't get why these two should be mutually exclusive?
>
> What is the memory ownership convention for the return value from the
> function?
>
> If the caller should own the memory, then you want to do this:
>
> char *foo(...)
> {
> struct strbuf buf = STRBUF_INIT;
> ... fill up buf ...
> return strbuf_detach(&buf, NULL);
> }
>
> The "detach" disconnects the memory from the strbuf (which is going out
> of scope anyway), and the only pointer left to it is in the caller. It's
> important to use "detach" here and not just return the pointer, because
> that ensures that the returned value is always allocated memory (and
> never strbuf_slopbuf).
>
> If the caller should not own the memory, then the function retains
> ownership. And you want something like this:
>
> char *foo(...)
> {
> static struct strbuf buf = STRBUF_INIT;
> strbuf_reset(&buf);
> ... fill up buf ...
> return buf.buf;
> }
>
> The same buffer is reused over and over. The "reset" call clears any
> leftover bits from the last invocation, and you must _not_ call
> strbuf_detach() in the return, as it disconnects the memory from the
> strbuf (and so next time you'd end up allocating again, and each return
> value becomes a memory leak).
Ah! perfect, makes perfect sense. Sorry you had to spell that out for me.
'strbuf_detach() in the return, as it disconnects the memory from the strbuf'
that was what I was missing, thanks.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH 0/1] Fix a long-standing isatty() problem on Windows
From: Junio C Hamano @ 2016-12-12 16:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Pranit Bauva
In-Reply-To: <alpine.DEB.2.20.1612121052310.23160@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> This is a great find, and a very impactful fix, as redirecting from
>> /dev/null is how we try to force a "go interactive if talking to
>> tty" program to realize that it is not talking to a tty.
>
> Can we fast-track this to maint?
Yes, "fast-track" is a good phrase, as there is not much point
cooking a subsystem specific fix like this one that came directly
from a subsystem maintainer in 'next' for 1/2 to 2 weeks as usual.
Let's make sure that the first maintenance release won't ship
without merging this.
Thanks.
^ permalink raw reply
* [PATCH] date-formats.txt: Typo fix
From: Luis Ressel @ 2016-12-12 16:45 UTC (permalink / raw)
To: git
Last time I checked, I was living in the UTC+01:00 time zone. UTC+02:00
would be Central European _Summer_ Time.
Signed-off-by: Luis Ressel <aranea@aixah.de>
---
Documentation/date-formats.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/date-formats.txt b/Documentation/date-formats.txt
index 35e8da201..6926e0a4c 100644
--- a/Documentation/date-formats.txt
+++ b/Documentation/date-formats.txt
@@ -11,7 +11,7 @@ Git internal format::
It is `<unix timestamp> <time zone offset>`, where `<unix
timestamp>` is the number of seconds since the UNIX epoch.
`<time zone offset>` is a positive or negative offset from UTC.
- For example CET (which is 2 hours ahead UTC) is `+0200`.
+ For example CET (which is 1 hour ahead of UTC) is `+0100`.
RFC 2822::
The standard email format as described by RFC 2822, for example
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Jeff King @ 2016-12-12 16:40 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <CAOLa=ZT1KN_iw7JzB78hPb-LrY_yaDZk0D+TqY2W9hCOftzumA@mail.gmail.com>
On Mon, Dec 12, 2016 at 09:59:49PM +0530, Karthik Nayak wrote:
> >> > This caller never stores the return value, and it ends up leaking. So I
> >> > wonder if you wanted "static struct strbuf" in the first place (and that
> >> > would explain the strbuf_reset() in your function).
> >>
> >> Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
> >> suggestion.
> >>
> >> strbuf_detach() is also a better way to go.
> >
> > One of the other, though. If it's static, then you _don't_ want to
> > detach.
> >
>
> Wait. Why not? since it gets added to the strbuf's within
> build_format() and that
> value is not needed again, why do we need to re-allocate? we can use the same
> variable again (i.e by keeping it as static).
>
> I'm sorry, but I didn't get why these two should be mutually exclusive?
What is the memory ownership convention for the return value from the
function?
If the caller should own the memory, then you want to do this:
char *foo(...)
{
struct strbuf buf = STRBUF_INIT;
... fill up buf ...
return strbuf_detach(&buf, NULL);
}
The "detach" disconnects the memory from the strbuf (which is going out
of scope anyway), and the only pointer left to it is in the caller. It's
important to use "detach" here and not just return the pointer, because
that ensures that the returned value is always allocated memory (and
never strbuf_slopbuf).
If the caller should not own the memory, then the function retains
ownership. And you want something like this:
char *foo(...)
{
static struct strbuf buf = STRBUF_INIT;
strbuf_reset(&buf);
... fill up buf ...
return buf.buf;
}
The same buffer is reused over and over. The "reset" call clears any
leftover bits from the last invocation, and you must _not_ call
strbuf_detach() in the return, as it disconnects the memory from the
strbuf (and so next time you'd end up allocating again, and each return
value becomes a memory leak).
Does that make sense?
-Peff
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Karthik Nayak @ 2016-12-12 16:29 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <20161212121548.aj2ptnmrqt67anlc@sigill.intra.peff.net>
On Mon, Dec 12, 2016 at 5:45 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 12, 2016 at 04:50:20PM +0530, Karthik Nayak wrote:
>
>> > This caller never stores the return value, and it ends up leaking. So I
>> > wonder if you wanted "static struct strbuf" in the first place (and that
>> > would explain the strbuf_reset() in your function).
>>
>> Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
>> suggestion.
>>
>> strbuf_detach() is also a better way to go.
>
> One of the other, though. If it's static, then you _don't_ want to
> detach.
>
Wait. Why not? since it gets added to the strbuf's within
build_format() and that
value is not needed again, why do we need to re-allocate? we can use the same
variable again (i.e by keeping it as static).
I'm sorry, but I didn't get why these two should be mutually exclusive?
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Johannes Schindelin @ 2016-12-12 14:05 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: git
In-Reply-To: <20161212115030.qx2y276bxnzbtxkj@ikki.ethgen.ch>
Hi Klaus,
you probably missed the note about reply-to-all being customary on this
list. The reason is that this list is high-volume, and open to
non-subscribers. You may want to get into the habit of hitting
reply-to-all when responding to mails from this list, in case that you
plan to continue communicating on this forum.
On Mon, 12 Dec 2016, Klaus Ethgen wrote:
> Am Mo den 12. Dez 2016 um 12:03 schrieb Johannes Schindelin:
> > On Sun, 11 Dec 2016, Klaus Ethgen wrote:
> > > Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > > > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > > > Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > > > There are too many systems out there that use a backslash in path names. I
> > > > > > don't think it is wise to use it also as the quoting character.
> > > > > Well, the minority, I believe. And only the minority where the command
> > > > > line git is used anywhere.
> > > > Please provide evidence for such bold assumptions.
> > > How is it "bold" to see that the majority of widows users does not use
> > > or even like command line tools.
> >
> > Second, you still did not back up your claim with anything resembling
> > evidence, instead just reiterating your beliefs. That is not good enough.
>
> Well, my evidence is what I seen with many windows users in the past. I
> have no link or something like that. I just shared that observation.
Thank you for confirming that you just shared a personal observation and
did not plan to back that up by scientific evidence.
In light of that, let's continue according to Johannes Sixt's insightful
suggestions.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Jeff King @ 2016-12-12 12:15 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <CAOLa=ZSPDLwziGEvyixebAkS2M1JMYidQNHfDbnmYarFCjn80A@mail.gmail.com>
On Mon, Dec 12, 2016 at 04:50:20PM +0530, Karthik Nayak wrote:
> > This caller never stores the return value, and it ends up leaking. So I
> > wonder if you wanted "static struct strbuf" in the first place (and that
> > would explain the strbuf_reset() in your function).
>
> Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
> suggestion.
>
> strbuf_detach() is also a better way to go.
One of the other, though. If it's static, then you _don't_ want to
detach.
-Peff
^ permalink raw reply
* Re: [BUG] Colon in remote urls
From: Klaus Ethgen @ 2016-12-12 11:50 UTC (permalink / raw)
To: git
In-Reply-To: <alpine.DEB.2.20.1612121133220.23160@virtualbox>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hi,
Am Mo den 12. Dez 2016 um 12:03 schrieb Johannes Schindelin:
> On Sun, 11 Dec 2016, Klaus Ethgen wrote:
> > Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > > Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > > There are too many systems out there that use a backslash in path names. I
> > > > > don't think it is wise to use it also as the quoting character.
> > > > Well, the minority, I believe. And only the minority where the command
> > > > line git is used anywhere.
> > > Please provide evidence for such bold assumptions.
> > How is it "bold" to see that the majority of widows users does not use
> > or even like command line tools.
>
> First of all, it is "Windows users", not "widows users", unless, of
> course, you want to discuss things that are completely inappropriate for
> this list.
Sorry to have a typo in my response.
> Second, you still did not back up your claim with anything resembling
> evidence, instead just reiterating your beliefs. That is not good enough.
Well, my evidence is what I seen with many windows users in the past. I
have no link or something like that. I just shared that observation.
> Third, my experience contradicts your beliefs rather violently.
So we have two different observations. Good. Have no problem with that.
[...]
> Now let's look at your claim that Windows users do not use the
> command-line. The mere existence of posh-git (Powershell bindings for Git)
> is already a contradiction to that claim.
Is that the same git tools or is it a separate implementation?
[Proof of many downloads and other]
Proofs accepted. They do not match my observations but ok.
> Fourth, even if Windows users were the minority, and even if Windows users
> were not using the command-line, which are claims soundly refuted by the
> evidence I presented above, the fact alone that you are talking about
> putting a group of people at a disadvantage based merely on your belief
> that they are in a minority should not inform us, the Git developers, on
> any kind of policy decision.
>
> We will not intentionally break Git usage, or make Git usage hard, for
> a specific group of Git users, unless there are technical reasons to
> do that. Demographic reasons do not count.
Sorry, but I was posting my answer to the comment of preferring not to
allow ":" in prefer of using "\" for quoting. I never wanted to attack
you. It was just a response... And I do not remember attacking you
personally to style me "bold".
Currently the source of that all was, that push to pathes with ":" did
break in the current version. I did not ask for implementing something
instead of something different. I just ask for fixing of a regression.
Moreover, as you might know, windows users are more affected by that bug
as they have a colon in every absolute path. Nevertheless I had that
problem on linux but with a map to a windows share.
For more, I have to agree with Philip, that this is a bit off topic. So
please stop taking single words on the personal level. Please stay on
the technical level. My english is to bad to go to personal level
discussions. And I don't want to spend time to personal or (as you
tried) feminism discussions.
Regards
Klaus
- --
Klaus Ethgen http://www.ethgen.ch/
pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen <Klaus@Ethgen.ch>
Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Comment: Charset: ISO-8859-1
iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhOjvwACgkQpnwKsYAZ
9qyb2wwAuUFTRrOQcJ6mDLx5bHPNrQy6e4ify4qWcfcp3VJhOLpA56CdJwwA2bcl
gcllUwOn+SSjjYaRmuFQ6aY0/oKDEtF98Zh01pXLnAgY+Dabg9gfBCMWyWv8LxUf
NWHADPpSkY2eongvm6aXOGTqNvU73wriuQKM6BwG49bJUpdR1q8Fe+DjtufrS8eW
ALeGoEFrbm0aIbd121AvWw53hCz3j9ssDpGdFefkhjeJnRcSSDmzTID5KcuWME9P
L67BjGJb3swYIpw3Sdg7LjWnMK0o9GpzfZVNuJFMsRHBQUaWAQvVG79AHe/5QI9w
pq0K5C6frllP5CgcoD2/H+8F8sMD7BrhyN1MxFRdaa4eCEh/hFxZV2nIfbZnx0SS
5/SNMKcCdly+yodZCgohU4uJuqDBkRIEbr6+OCffsupQMCYkh/Ew/RRa8tMN26bN
45/ferqMK1KEenpllXGNHi3/a9dT5CaqEvLjErdatChhQR6i7QbA5B3KhXKRSsRz
YPbYc7rc
=e8K5
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"
From: Philip Oakley @ 2016-12-12 11:44 UTC (permalink / raw)
To: David Aguilar, Johannes Sixt; +Cc: Junio C Hamano, Git ML
In-Reply-To: <20161212071646.5bqnnjpfnmnj6fm4@gmail.com>
From: "David Aguilar" <davvid@gmail.com>
> On Sat, Dec 10, 2016 at 09:15:34AM +0100, Johannes Sixt wrote:
>> Am 10.12.2016 um 04:21 schrieb David Aguilar:
>> > Signed-off-by: David Aguilar <davvid@gmail.com>
>> > ---
>> > This patch builds upon da/mergetool-trust-exit-code
>> >
>> > mergetools/tortoisemerge | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
>> > index d7ab666a59..9067d8a4e5 100644
>> > --- a/mergetools/tortoisemerge
>> > +++ b/mergetools/tortoisemerge
>> > @@ -1,5 +1,5 @@
>> > can_diff () {
>> > - return 1
>> > + false
>> > }
>>
>> Why is this a simplification?
>>
>> My concern is that 'false' is not necessarily a shell built-in. Then this
>> is
>> actually a pessimization.
>
> The "simplification" is semantic only.
>
> Motivation: if someone reads the implementation of can_diff()
> and it says "false" then that communicates intent moreso than
> reading "return 1", which a programmer unfamiliar with shell
> conventions might misinterpret as boolean "true".
>
Is this a case where a short comment would be informative?
+ return 1 /* shell: false */
> I care less about semantics then I do about making things better
> for Windows, so we can forget about these two patches.
> --
> David
>
--
Philip
^ permalink raw reply
* Re: [PATCH v8 18/19] branch: use ref-filter printing APIs
From: Karthik Nayak @ 2016-12-12 11:20 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Jacob Keller, Junio C Hamano, Jakub Narębski
In-Reply-To: <20161209140345.76ybodldmg2lxgbn@sigill.intra.peff.net>
On Fri, Dec 9, 2016 at 7:33 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Dec 07, 2016 at 09:06:26PM +0530, Karthik Nayak wrote:
>
>> +const char *quote_literal_for_format(const char *s)
>> {
>> + struct strbuf buf = STRBUF_INIT;
>>
>> + strbuf_reset(&buf);
>> + while (*s) {
>> + const char *ep = strchrnul(s, '%');
>> + if (s < ep)
>> + strbuf_add(&buf, s, ep - s);
>> + if (*ep == '%') {
>> + strbuf_addstr(&buf, "%%");
>> + s = ep + 1;
>> + } else {
>> + s = ep;
>> + }
>> }
>> + return buf.buf;
>> }
>
> You should use strbuf_detach() to return the buffer from a strbuf.
> Otherwise it is undefined whether the pointer is allocated or not (and
> whether it needs to be freed).
>
> In this case, if "s" is empty, buf.buf would point to a string literal,
> but otherwise to allocated memory. strbuf_detach() normalizes that.
>
> But...
>
>> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth, quote_literal_for_format(remote_prefix),
>
> This caller never stores the return value, and it ends up leaking. So I
> wonder if you wanted "static struct strbuf" in the first place (and that
> would explain the strbuf_reset() in your function).
>
> -Peff
Ah! Yes this should be 'static struct strbuf' indeed, I blindly copied Junio's
suggestion.
strbuf_detach() is also a better way to go.
Thanks.
--
Regards,
Karthik Nayak
^ permalink raw reply
* [PULL] minor git-svn updates (probably for 2.11.x)
From: Eric Wong @ 2016-12-12 11:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The following changes since commit 8d7a455ed52e2a96debc080dfc011b6bb00db5d2:
Start post 2.11 cycle (2016-12-05 11:31:47 -0800)
are available in the git repository at:
git://bogomips.org/git-svn.git master
for you to fetch changes up to 1b7edec78b754a1e901c164a4bf4e94bff96ed7b:
git-svn: document useLogAuthor and addAuthorFrom config keys (2016-12-12 11:09:29 +0000)
----------------------------------------------------------------
Eric Wong (2):
git-svn: allow "0" in SVN path components
git-svn: document useLogAuthor and addAuthorFrom config keys
Documentation/git-svn.txt | 8 +++++++-
perl/Git/SVN/Ra.pm | 2 +-
2 files changed, 8 insertions(+), 2 deletions(-)
^ permalink raw reply
* [PATCH] git-svn: allow "0" in SVN path components
From: Eric Wong @ 2016-12-12 11:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Blindly checking a path component for falsiness is unwise, as
"0" is false to Perl, but a valid pathname component for SVN
(or any filesystem).
Found via random code reading.
Signed-off-by: Eric Wong <e@80x24.org>
---
Junio: this bugfix should go to "maint".
Will push along with a doc fix for Juergen.
perl/Git/SVN/Ra.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index e764696801..56ad9870bc 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -606,7 +606,7 @@ sub minimize_url {
my $latest = $ra->get_latest_revnum;
$ra->get_log("", $latest, 0, 1, 0, 1, sub {});
};
- } while ($@ && ($c = shift @components));
+ } while ($@ && defined($c = shift @components));
return canonicalize_url($url);
}
--
EW
^ permalink raw reply related
* Re: [BUG] Colon in remote urls
From: Johannes Schindelin @ 2016-12-12 11:03 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: git
In-Reply-To: <20161211110208.642unp7c2i653sav@ikki.ethgen.ch>
Hi Klaus,
On Sun, 11 Dec 2016, Klaus Ethgen wrote:
> Am Sa den 10. Dez 2016 um 19:18 schrieb Johannes Schindelin:
> > On Sat, 10 Dec 2016, Klaus Ethgen wrote:
> > > Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt:
> > > > There are too many systems out there that use a backslash in path names. I
> > > > don't think it is wise to use it also as the quoting character.
> > > Well, the minority, I believe. And only the minority where the command
> > > line git is used anywhere.
> >
> > Please provide evidence for such bold assumptions.
>
> How is it "bold" to see that the majority of widows users does not use
> or even like command line tools.
First of all, it is "Windows users", not "widows users", unless, of
course, you want to discuss things that are completely inappropriate for
this list.
Second, you still did not back up your claim with anything resembling
evidence, instead just reiterating your beliefs. That is not good enough.
Third, my experience contradicts your beliefs rather violently.
So let's try some evidence for a change: the 64-bit installer of Git for
Windows v2.10.2 was downloaded over 1.25 million times. That is not a
negligible number. If you want to go back to v2.8.1, it is even 3.8
million downloads. That is a tall number to call a minority.
Now let's look at your claim that Windows users do not use the
command-line. The mere existence of posh-git (Powershell bindings for Git)
is already a contradiction to that claim.
Even if that was not enough, the Git for Windows bug tracker is full of
reports of users who clearly use the command-line.
And there is more evidence: When comparing the download numbers of the
different Git for Windows versions, one thing really sticks out: those
versions were downloaded the most (by a factor of more than 2x over the
other versions) which were made available through Visual Studio's
"Download command-line Git tools" feature, e.g. v2.8.1 and v2.9.0. That is
a rather strong indicator that users wanted to use the command-line.
Fourth, even if Windows users were the minority, and even if Windows users
were not using the command-line, which are claims soundly refuted by the
evidence I presented above, the fact alone that you are talking about
putting a group of people at a disadvantage based merely on your belief
that they are in a minority should not inform us, the Git developers, on
any kind of policy decision.
We will not intentionally break Git usage, or make Git usage hard, for
a specific group of Git users, unless there are technical reasons to
do that. Demographic reasons do not count.
For example, we will not make Git hard to use for female programmers,
on the grounds that they currently constitute a minority.
> I know companies where the "developers" doesn't even know of the
> existent of a git command line use. They look with owe when they see
> that I use a shell to use git.
I must have spoken to hundreds of Git for Windows users, and must have
been in communication with many more via email or bug tracker, and I
cannot recall a single one who used Git without using the command-line.
Note: I do not count my personal experience here as evidence, but the
numbers alone are a strong indicator to me that your argument has a pretty
weak foundation.
Ciao,
Johannes
P.S.: Maybe reply-to-all in the future; it is the custom on this here
mailing list.
^ permalink raw reply
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-12-12 10:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jacob Keller, Jakub Narębski
In-Reply-To: <xmqqpol2rn0z.fsf@gitster.mtv.corp.google.com>
On Fri, Dec 9, 2016 at 5:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.
>
> Will replace, with the attached stylistic fixes squashed in for
> minor issues that were spotted by my mechanical pre-acceptance
> filter.
>
Thanks for this. Will add it to my local branch too if there's a need
for a re-roll.
^ permalink raw reply
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-12-12 10:42 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, Junio C Hamano, Jakub Narębski
In-Reply-To: <CA+P7+xquordVY19dypqNcAuQqoRbFmHhzb0w+HXCaJmm_Ex7zQ@mail.gmail.com>
On Thu, Dec 8, 2016 at 5:31 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index f4ad297..c72baeb 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -92,13 +92,14 @@ refname::
>> The name of the ref (the part after $GIT_DIR/).
>> For a non-ambiguous short name of the ref append `:short`.
>> The option core.warnAmbiguousRefs is used to select the strict
>> - abbreviation mode. If `strip=<N>` is appended, strips `<N>`
>> - slash-separated path components from the front of the refname
>> - (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>> - `<N>` must be a positive integer. If a displayed ref has fewer
>> - components than `<N>`, the command aborts with an error. For the base
>> - directory of the ref (i.e. foo in refs/foo/bar/boz) append
>> - `:base`. For the entire directory path append `:dir`.
>> + abbreviation mode. If `lstrip=<N>` or `rstrip=<N>` option can
>
> Grammar here, drop the If before `lstrip since you're referring to
> multiples and you say "x can be appended to y" rather than "if x is
> added, do y"
>
Will do. Thanks.
>> + be appended to strip `<N>` slash-separated path components
>> + from or end of the refname respectively (e.g.,
>> + `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
>> + `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`). if
>> + `<N>` is a negative number, then only `<N>` path components
>> + are left behind. If a displayed ref has fewer components than
>> + `<N>`, the command aborts with an error.
>>
>
> Would it make more sense to not die and instead just return the empty
> string? On the one hand, if we die() it's obvious that you tried to
> strip too many components. But on the other hand, it's also somewhat
> annoying to have the whole command fail because we happen upon a
> single ref that has fewer components?
>
> So, for positive numbers, we simply strip what we can, which may
> result in the empty string, and for negative numbers, we keep up to
> what we said, while potentially keeping the entire string. I feel
> that's a better alternative than a die() in the middle of a ref
> filter..
>
> What are other people's thoughts on this?
I am _for_ this. Even I think it'd be better to return an empty string rather
than just die in the middle.
--
Regards,
Karthik Nayak
^ 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