Any interest in 'git merge --continue' as a command
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?
git bash error
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
Re: [BUG] regarding `git add -p` and files containing wildcard characters
On Fri, Dec 09, 2016 at 04:46:49AM +0300, unixway.dr...@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
[PATCH] commit: remove 'Clever' message for --only --amend
That behavior is now documented, and we don't
need a reward afterwards.
Signed-off-by: Andreas Krey
---
> 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
Re: [PATCH v2 1/4] real_path: resolve symlinks by hand
On Thu, Dec 8, 2016 at 3:58 PM, Brandon Williams 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
[BUG] regarding `git add -p` and files containing wildcard characters
`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 … … MRandom *
:100644 100644 … … MRandom 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.)
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
Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
On Thu, Dec 8, 2016 at 10:58 AM, Junio C Hamano 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
Re: [PATCH v2 14/16] pathspec: create strip submodule slash helpers
Brandon Williams 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)?
Re: [PATCH v2 00/16] pathspec cleanup
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;
Re: [PATCH v4] diff: handle --no-abbrev in no-index case
On 08/12/16 03:53 PM, Junio C Hamano wrote: Jack Bates 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[=] 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!
[PATCH v2 3/4] real_path: create real_pathdup
Create real_pathdup which returns a caller owned string of the resolved
realpath based on the provide path.
Signed-off-by: Brandon Williams
---
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
[PATCH v2 4/4] real_path: have callers use real_pathdup and strbuf_realpath
Migrate callers of real_path() who duplicate the retern value to use
real_pathdup or strbuf_realpath.
Signed-off-by: Brandon Williams
---
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 */
st
Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
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
[PATCH v2 2/4] real_path: convert real_path_internal to strbuf_realpath
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
---
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)
[PATCH v2 1/4] real_path: resolve symlinks by hand
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
---
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
[PATCH v2 0/4] road to reentrant real_path
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");
+
Re: [PATCH v4] diff: handle --no-abbrev in no-index case
Jack Bates 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[=] 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.
[PATCHv7 3/6] test-lib-functions.sh: teach test_commit -C
Specifically when setting up submodule tests, it comes in handy if
we can create commits in repositories that are not at the root of
the tested trash dir. Add "-C " similar to gits -C parameter
that will perform the operation in the given directory.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
t/test-lib-functions.sh | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..579e812506 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -157,16 +157,21 @@ debug () {
GIT_TEST_GDB=1 "$@"
}
-# Call test_commit with the arguments " [ [ []]]"
+# Call test_commit with the arguments
+# [-C ] [ [ []]]"
#
# This will commit a file with the given contents and the given commit
# message, and tag the resulting commit with the given tag name.
#
# , , and all default to .
+#
+# If the first argument is "-C", the second argument is used as a path for
+# the git invocations.
test_commit () {
notick= &&
signoff= &&
+ indir= &&
while test $# != 0
do
case "$1" in
@@ -176,21 +181,26 @@ test_commit () {
--signoff)
signoff="$1"
;;
+ -C)
+ indir="$2"
+ shift
+ ;;
*)
break
;;
esac
shift
done &&
+ indir=${indir:+"$indir"/} &&
file=${2:-"$1.t"} &&
- echo "${3-$1}" > "$file" &&
- git add "$file" &&
+ echo "${3-$1}" > "$indir$file" &&
+ git ${indir:+ -C "$indir"} add "$file" &&
if test -z "$notick"
then
test_tick
fi &&
- git commit $signoff -m "$1" &&
- git tag "${4:-$1}"
+ git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
+ git ${indir:+ -C "$indir"} tag "${4:-$1}"
}
# Call test_merge with the arguments " ", where
--
2.11.0.rc2.29.g7c00390.dirty
[PATCHv7 4/6] worktree: have a function to check if worktrees are in use
In a later patch we want to move around the the git directory of
a submodule. Both submodules as well as worktrees are involved in
placing git directories at unusual places, so their functionality
may collide. To react appropriately to situations where worktrees
in submodules are in use, offer a new function to query the
a submodule if it uses the worktree feature.
This can be done cheaply (both in new code to write as well as run time)
by obtaining the list of worktrees based off that submodules git
directory. However as we have loaded the variables for the current
repository, the values in the submodule worktree
can be wrong, e.g.
* core.ignorecase may differ between these two repositories
* the ref resolution is broken (refs/heads/branch in the submodule
resolves to the sha1 value of the `branch` in the current repository
that may not exist or have another sha1)
Signed-off-by: Stefan Beller
---
worktree.c | 68 +-
worktree.h | 7 +++
2 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/worktree.c b/worktree.c
index eb6121263b..1c46fcf25f 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(void)
+static struct worktree *get_main_worktree(const char *git_common_dir)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(void)
int is_bare = 0;
int is_detached = 0;
- strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+ strbuf_add_absolute_path(&worktree_path, git_common_dir);
is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
if (is_bare)
strbuf_strip_suffix(&worktree_path, "/.");
- strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+ strbuf_addf(&path, "%s/HEAD", git_common_dir);
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
return worktree;
}
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(const char *git_common_dir,
+ const char *id)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -112,7 +113,7 @@ static struct worktree *get_linked_worktree(const char *id)
if (!id)
die("Missing linked worktree name");
- strbuf_git_common_path(&path, "worktrees/%s/gitdir", id);
+ strbuf_addf(&path, "%s/worktrees/%s/gitdir", git_common_dir, id);
if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
/* invalid gitdir file */
goto done;
@@ -125,7 +126,7 @@ static struct worktree *get_linked_worktree(const char *id)
}
strbuf_reset(&path);
- strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+ strbuf_addf(&path, "%s/worktrees/%s/HEAD", git_common_dir, id);
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
goto done;
@@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)
return fspathcmp((*a)->path, (*b)->path);
}
-struct worktree **get_worktrees(unsigned flags)
+static struct worktree **get_worktrees_internal(const char *git_common_dir,
+ unsigned flags)
{
struct worktree **list = NULL;
struct strbuf path = STRBUF_INIT;
@@ -177,9 +179,10 @@ struct worktree **get_worktrees(unsigned flags)
list = xmalloc(alloc * sizeof(struct worktree *));
- list[counter++] = get_main_worktree();
+ if (!(flags & GWT_LINKED_ONLY))
+ list[counter++] = get_main_worktree(git_common_dir);
- strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+ strbuf_addf(&path, "%s/worktrees", git_common_dir);
dir = opendir(path.buf);
strbuf_release(&path);
if (dir) {
@@ -188,7 +191,7 @@ struct worktree **get_worktrees(unsigned flags)
if (is_dot_or_dotdot(d->d_name))
continue;
- if ((linked = get_linked_worktree(d->d_name))) {
+ if ((linked = get_linked_worktree(git_common_dir,
d->d_name))) {
ALLOC_GROW(list, counter + 1, alloc);
list[counter++] = linked;
}
@@ -209,6 +212,34 @@ struct worktree **get_worktrees(unsigned flags)
return list;
}
+struct worktree **get_worktrees(unsigned flags)
+{
+ return get_worktrees_internal(get_git_common_dir(), flags);
+}
+
+/*
+ * NEEDSWORK: The values in the returned worktrees a
[PATCHv7 5/6] move connect_work_tree_and_git_dir to dir.h
That function was primarily used by submodule code, but the function
itself is not inherently about submodules. In the next patch we'll
introduce relocate_git_dir, which can be used by worktrees as well,
so find a neutral middle ground in dir.h.
Signed-off-by: Stefan Beller
---
dir.c | 25 +
dir.h | 1 +
submodule.c | 25 -
submodule.h | 1 -
4 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/dir.c b/dir.c
index bfa8c8a9a5..e0efd3c2c3 100644
--- a/dir.c
+++ b/dir.c
@@ -2748,3 +2748,28 @@ void untracked_cache_add_to_index(struct index_state
*istate,
{
untracked_cache_invalidate_path(istate, path);
}
+
+/* Update gitfile and core.worktree setting to connect work tree and git dir */
+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;
+ char *git_dir = xstrdup(real_path(git_dir_));
+ char *work_tree = xstrdup(real_path(work_tree_));
+
+ /* Update gitfile */
+ strbuf_addf(&file_name, "%s/.git", work_tree);
+ write_file(file_name.buf, "gitdir: %s",
+ relative_path(git_dir, work_tree, &rel_path));
+
+ /* Update core.worktree setting */
+ strbuf_reset(&file_name);
+ strbuf_addf(&file_name, "%s/config", git_dir);
+ git_config_set_in_file(file_name.buf, "core.worktree",
+ relative_path(work_tree, git_dir, &rel_path));
+
+ strbuf_release(&file_name);
+ strbuf_release(&rel_path);
+ free(work_tree);
+ free(git_dir);
+}
diff --git a/dir.h b/dir.h
index 97c83bb383..051674a431 100644
--- a/dir.h
+++ b/dir.h
@@ -335,4 +335,5 @@ struct untracked_cache *read_untracked_extension(const void
*data, unsigned long
void write_untracked_extension(struct strbuf *out, struct untracked_cache
*untracked);
void add_untracked_cache(struct index_state *istate);
void remove_untracked_cache(struct index_state *istate);
+extern void connect_work_tree_and_git_dir(const char *work_tree, const char
*git_dir);
#endif
diff --git a/submodule.c b/submodule.c
index d4f7afe2f1..0bb50b4b62 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1222,31 +1222,6 @@ int merge_submodule(unsigned char result[20], const char
*path,
return 0;
}
-/* Update gitfile and core.worktree setting to connect work tree and git dir */
-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;
- char *git_dir = xstrdup(real_path(git_dir_));
- char *work_tree = xstrdup(real_path(work_tree_));
-
- /* Update gitfile */
- strbuf_addf(&file_name, "%s/.git", work_tree);
- write_file(file_name.buf, "gitdir: %s",
- relative_path(git_dir, work_tree, &rel_path));
-
- /* Update core.worktree setting */
- strbuf_reset(&file_name);
- strbuf_addf(&file_name, "%s/config", git_dir);
- git_config_set_in_file(file_name.buf, "core.worktree",
- relative_path(work_tree, git_dir, &rel_path));
-
- strbuf_release(&file_name);
- strbuf_release(&rel_path);
- free(work_tree);
- free(git_dir);
-}
-
int parallel_submodules(void)
{
return parallel_jobs;
diff --git a/submodule.h b/submodule.h
index d9e197a948..4e3bf469b4 100644
--- a/submodule.h
+++ b/submodule.h
@@ -65,7 +65,6 @@ int merge_submodule(unsigned char result[20], const char
*path, const unsigned c
int find_unpushed_submodules(unsigned char new_sha1[20], const char
*remotes_name,
struct string_list *needs_pushing);
int push_unpushed_submodules(unsigned char new_sha1[20], const char
*remotes_name);
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
/*
--
2.11.0.rc2.29.g7c00390.dirty
[PATCHv7 6/6] submodule: add absorb-git-dir function
When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.
Add functionality to migrate the git directory to be absorbed
into the superprojects git directory.
The newly added code in this patch is structured such that other areas of
Git can also make use of it. The code in the submodule--helper is a mere
wrapper and option parser for the function
`absorb_git_dir_into_superproject`, that takes care of embedding the
submodules git directory into the superprojects git dir. That function
makes use of the more abstract function for this use case
`relocate_gitdir`, which can be used by e.g. the worktree code eventually
to move around a git directory.
Signed-off-by: Stefan Beller
---
Documentation/git-submodule.txt| 15 ++
builtin/submodule--helper.c| 38 ++
dir.c | 12 +
dir.h | 3 ++
git-submodule.sh | 7 ++-
submodule.c| 103 +
submodule.h| 4 ++
t/t7412-submodule-absorbgitdirs.sh | 101
8 files changed, 282 insertions(+), 1 deletion(-)
create mode 100755 t/t7412-submodule-absorbgitdirs.sh
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..918bd1d1bd 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,6 +22,7 @@ SYNOPSIS
[commit] [--] [...]
'git submodule' [--quiet] foreach [--recursive]
'git submodule' [--quiet] sync [--recursive] [--] [...]
+'git submodule' [--quiet] absorbgitdirs [--] [...]
DESCRIPTION
@@ -245,6 +246,20 @@ sync::
If `--recursive` is specified, this command will recurse into the
registered submodules, and sync any nested submodules within.
+absorbgitdirs::
+ If a git directory of a submodule is inside the submodule,
+ move the git directory of the submodule into its superprojects
+ `$GIT_DIR/modules` path and then connect the git directory and
+ its working directory by setting the `core.worktree` and adding
+ a .git file pointing to the git directory embedded in the
+ superprojects git directory.
++
+A repository that was cloned independently and later added as a submodule or
+old setups have the submodules git directory inside the submodule instead of
+embedded into the superprojects git directory.
++
+This command is recursive by default.
+
OPTIONS
---
-q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5643848667..242d9911a6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,43 @@ static int resolve_remote_submodule_branch(int argc,
const char **argv,
return 0;
}
+static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
+{
+ int i;
+ struct pathspec pathspec;
+ struct module_list list = MODULE_LIST_INIT;
+ unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;
+
+ struct option embed_gitdir_options[] = {
+ OPT_STRING(0, "prefix", &prefix,
+ N_("path"),
+ N_("path into the working tree")),
+ OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"),
+ ABSORB_GITDIR_RECURSE_SUBMODULES),
+ OPT_END()
+ };
+
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule--helper embed-git-dir [...]"),
+ NULL
+ };
+
+ argc = parse_options(argc, argv, prefix, embed_gitdir_options,
+git_submodule_helper_usage, 0);
+
+ gitmodules_config();
+ git_config(submodule_config, NULL);
+
+ if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+ return 1;
+
+ for (i = 0; i < list.nr; i++)
+ absorb_git_dir_into_superproject(prefix,
+ list.entries[i]->name, flags);
+
+ return 0;
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -1094,6 +1131,7 @@ static struct cmd_struct commands[] = {
{"resolve-relative-url-test", resolve_relative_url_test, 0},
{"init", module_init, 0},
{"remote-branch", resolve_remote_submodule_branch, 0},
+ {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/dir.c b/dir.c
index e0efd3c2c3..d872cc1570 100644
--- a/dir.c
+++ b/dir.c
@@ -2773,3 +2773,15 @@ void connect_work_tree_and_git_dir(const char
*work_tree_, const char *git_dir_)
free(work_tree);
free(git_dir);
}
+
+/*
+ * Migrate the git directory of the given path from old_git_dir to new_git_dir.
+ */
+void relocate_gitdir(const char *path, const char *old_git_dir, con
[PATCHv7 0/6] submodule absorbgitdirs
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 " 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
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
--
2.11.0.rc2.29.g7c00390.dirty
[PATCHv7 2/6] submodule helper: support super prefix
Just like main commands in Git, the submodule helper needs
access to the superproject prefix. Enable this in the git.c
but have its own fuse in the helper code by having a flag to
turn on the super prefix.
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 31 ---
git.c | 2 +-
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..5643848667 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc,
const char **argv,
return 0;
}
+#define SUPPORT_SUPER_PREFIX (1<<0)
+
struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
+ unsigned option;
};
static struct cmd_struct commands[] = {
- {"list", module_list},
- {"name", module_name},
- {"clone", module_clone},
- {"update-clone", update_clone},
- {"relative-path", resolve_relative_path},
- {"resolve-relative-url", resolve_relative_url},
- {"resolve-relative-url-test", resolve_relative_url_test},
- {"init", module_init},
- {"remote-branch", resolve_remote_submodule_branch}
+ {"list", module_list, 0},
+ {"name", module_name, 0},
+ {"clone", module_clone, 0},
+ {"update-clone", update_clone, 0},
+ {"relative-path", resolve_relative_path, 0},
+ {"resolve-relative-url", resolve_relative_url, 0},
+ {"resolve-relative-url-test", resolve_relative_url_test, 0},
+ {"init", module_init, 0},
+ {"remote-branch", resolve_remote_submodule_branch, 0},
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
@@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv,
const char *prefix)
die(_("submodule--helper subcommand must be "
"called with a subcommand"));
- for (i = 0; i < ARRAY_SIZE(commands); i++)
- if (!strcmp(argv[1], commands[i].cmd))
+ for (i = 0; i < ARRAY_SIZE(commands); i++) {
+ if (!strcmp(argv[1], commands[i].cmd)) {
+ if (get_super_prefix() &&
+ !(commands[i].option & SUPPORT_SUPER_PREFIX))
+ die(_("%s doesn't support --super-prefix"),
+ commands[i].cmd);
return commands[i].fn(argc - 1, argv + 1, prefix);
+ }
+ }
die(_("'%s' is not a valid submodule--helper "
"subcommand"), argv[1]);
diff --git a/git.c b/git.c
index efa1059fe0..98dcf6c518 100644
--- a/git.c
+++ b/git.c
@@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
{ "stripspace", cmd_stripspace },
- { "submodule--helper", cmd_submodule__helper, RUN_SETUP },
+ { "submodule--helper", cmd_submodule__helper, RUN_SETUP |
SUPPORT_SUPER_PREFIX},
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
{ "tag", cmd_tag, RUN_SETUP },
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
--
2.11.0.rc2.29.g7c00390.dirty
[PATCHv7 1/6] submodule: use absolute path for computing relative path connecting
The current caller of connect_work_tree_and_git_dir passes
an absolute path for the `git_dir` parameter. In the future patch
we will also pass in relative path for `git_dir`. Extend the functionality
of connect_work_tree_and_git_dir to take relative paths for parameters.
We could work around this in the future patch by computing the absolute
path for the git_dir in the calling site, however accepting relative
paths for either parameter makes the API for this function much harder
to misuse.
Signed-off-by: Stefan Beller
---
submodule.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/submodule.c b/submodule.c
index 6f7d883de9..d4f7afe2f1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1223,27 +1223,28 @@ int merge_submodule(unsigned char result[20], const
char *path,
}
/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
+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));
+ char *git_dir = xstrdup(real_path(git_dir_));
+ char *work_tree = xstrdup(real_path(work_tree_));
/* Update gitfile */
strbuf_addf(&file_name, "%s/.git", work_tree);
write_file(file_name.buf, "gitdir: %s",
- relative_path(git_dir, real_work_tree, &rel_path));
+ relative_path(git_dir, work_tree, &rel_path));
/* Update core.worktree setting */
strbuf_reset(&file_name);
strbuf_addf(&file_name, "%s/config", git_dir);
git_config_set_in_file(file_name.buf, "core.worktree",
- relative_path(real_work_tree, git_dir,
-&rel_path));
+ relative_path(work_tree, git_dir, &rel_path));
strbuf_release(&file_name);
strbuf_release(&rel_path);
- free((void *)real_work_tree);
+ free(work_tree);
+ free(git_dir);
}
int parallel_submodules(void)
--
2.11.0.rc2.29.g7c00390.dirty
Re: [PATCHv6 2/7] submodule helper: support super prefix
>
> unsigned int is probably safer for variables that are used as bit-flags.
done
> If it's meant for users to see, please _() the string.
done
>> + { "submodule--helper", cmd_submodule__helper, RUN_SETUP |
>> SUPPORT_SUPER_PREFIX},
>
> The same macro defined twice in two separate .c files? Hmm.. it
> confused me a bit because i thought there was a connection.. I guess
> it's ok.
Its effect is exactly the same. just plain code duplication. I assume that
is okay here as it really is only one constant for now. Maybe we can
refactor that to
be defined in a header file (but which? There is no such thing as a git.h, where
I'd expect to put it.)
Thanks,
Stefan
Feature request: read git config from parent directory
Hello I use the same machine for work and open-source contribution. In both cases, I deal with a lot of repositories. Depending on whether I commit for work or open-source activities, I must use a different mail address. I used to setup work address for each work repo in git local config, but this is no longer practical. Since I use different directories for work and open-source, would it be possible for git to read irs config also from parent directories ? I.e. setup git to read config from ./.git/config then ../.gitconfig, ../../gitconfig until global ~/.gitconfig. All the best -- https://github.com/dod38fr/ -o- http://search.cpan.org/~ddumont/ http://ddumont.wordpress.com/ -o- irc: dod at irc.debian.org
Re: [PATCHv2 7/7] versioncmp: generalize version sort suffix reordering
SZEDER Gábor writes: > The 'versionsort.prereleaseSuffix' configuration variable, as its name > suggests, is supposed to only deal with tagnames with prerelease > suffixes, and allows sorting those prerelease tags in a user-defined > order before the suffixless main release tag, instead of sorting them > simply lexicographically. > > However, the previous changes in this series resulted in an > interesting and useful property of version sort: > > - The empty string as a configured suffix matches all tagnames, > including tagnames without any suffix, but > > - tagnames containing a "real" configured suffix are still ordered > according to that real suffix, because any longer suffix takes > precedence over the empty string. > > Exploiting this property we can easily generalize suffix reordering > and specify the order of tags with given suffixes not only before but > even after a main release tag by using the empty suffix to denote the > position of the main release tag, without any algorithm changes: > > $ git -c versionsort.prereleaseSuffix=-alpha \ > -c versionsort.prereleaseSuffix=-beta \ > -c versionsort.prereleaseSuffix="" \ > -c versionsort.prereleaseSuffix=-gamma \ > -c versionsort.prereleaseSuffix=-delta \ > tag -l --sort=version:refname 'v3.0*' > v3.0-alpha1 > v3.0-beta1 > v3.0 > v3.0-gamma1 > v3.0-delta1 > > Since 'versionsort.prereleaseSuffix' is not a fitting name for a > configuration variable to control this more general suffix reordering, > introduce the new variable 'versionsort.suffix'. Still keep the old > configuration variable name as a deprecated alias, though, to avoid > suddenly breaking setups already using it. Ignore the old variable if > both old and new configuration variables are set, but emit a warning > so users will be aware of it and can fix their configuration. Extend > the documentation to describe and add a test to check this more > general behavior. > > Note: since the empty suffix matches all tagnames, tagnames with > suffixes not included in the configuration are listed together with > the suffixless main release tag, ordered lexicographically right after > that, i.e. before tags with suffixes listed in the configuration > following the empty suffix. Thanks. Will comment on the individual patches later, but the end result looks very nice.
Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use
On Thu, Dec 8, 2016 at 2:51 AM, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 5:40 PM, Duy Nguyen wrote:
>> Alternatively, we could add a new flag to get_worktrees() to tell it
>> to return all worktrees if there is a least one linked worktree, or
>> return NULL if there's only main worktree. I'm not sure if this is
>> clever or very stupid.
>
> No, this may be better. Add a flag to say "returns linked worktrees
> _only_". Which means when you're in a "normal" repo, get_worktrees()
> with this flag returns NULL. When you're in a multiple-worktree repo,
> it returns all linked worktrees (no main worktree). I think I might
> have a use for this flag in addition to this uses_worktrees() here.
> uses_worktrees() look quite simple with that flag
>
> int uses_worktrees(void)
> {
> struct worktree **worktrees = get_worktrees(WT_LINKED_ONLY);
> int retval = worktrees != NULL;
I am interested in the submodule case however, where we already return NULL
e.g. when the submodule git dir cannot be found. Actually that would
work out fine
as well:
/* NOTE on accuracy of result, hence not exposed. */
static worktree **submodule_get_worktrees(const char *path, unsigned flags)
..
int submodule_uses_worktrees(const char *path)
{
struct worktree **worktrees = submodule_get_worktrees(path,
WT_LINKED_ONLY);
int retval = worktrees != NULL;
free_worktrees(worktrees);
return retval;
}
Thanks for that inspiration!
Re: [PATCH 4/5] Make sequencer abort safer
Hi, I'm a little afraid of feeding Parkinson's law of triviality here, but... ;) On 12/08/2016 06:27 PM, Junio C Hamano wrote: > Johannes Schindelin writes: > >> On Wed, 7 Dec 2016, Stephan Beyer wrote: >> >>> diff --git a/sequencer.c b/sequencer.c >>> index 30b10ba14..c9b560ac1 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") >>> static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") >>> static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") >>> static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") >>> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current") >> >> Is it required by law to have a four-letter infix, or can we have a nicer >> variable name (e.g. git_path_current_file)? > > I agree with you that, as other git_path_*_file variables match the > actual name on the filesystem, this one should too, together with > the update_curr_file() function. I totally agree with that (and I don't know why I used "curr", probably just because it looked consistent and good...). However: > -static void update_curr_file() > +static void update_current_file(void) This function name could lead to the impression that there is some current file (defined by a global state or whatever) that is updated. So I'd rather rename the *file* to one of * sequencer/abort-safety (consistent to am, describes its purpose) * sequencer/safety (shorter, still describes the purpose) * sequencer/current-head (describes what it contains) * sequencer/last (a four-letter word, not totally unambiguous though) > By the way, this step seems to be a fix to an existing problem, and > the new test added in 3/5 seems to be a demonstration of the issue. > If that is the case, shouldn't the new test initially expect failure > and updated by this step to expect success? That's usually a matter of taste that I sometimes also discuss with colleagues in other projects... However, for the git test suite with its "known breakage" behavior, your recommendation is surely the best way to do it (aside from introducing the test and the fix in one commit... but that does not show in the history that there actually was that breakage) ~Stephan
[PATCH v2 07/16] pathspec: remove unused variable from unsupported_magic
Removed unused variable 'n' from the 'unsupported_magic()' function.
Signed-off-by: Brandon Williams
---
pathspec.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 8f367f0..ec0d590 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -333,8 +333,8 @@ static void NORETURN unsupported_magic(const char *pattern,
unsigned short_magic)
{
struct strbuf sb = STRBUF_INIT;
- int i, n;
- for (n = i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
const struct pathspec_magic *m = pathspec_magic + i;
if (!(magic & m->bit))
continue;
@@ -344,7 +344,6 @@ static void NORETURN unsupported_magic(const char *pattern,
strbuf_addf(&sb, "'%c'", m->mnemonic);
else
strbuf_addf(&sb, "'%s'", m->name);
- n++;
}
/*
* We may want to substitute "this command" with a command
--
2.8.0.rc3.226.g39d4020
Re: [PATCH] real_path: make real_path thread-safe
On 12/08, Johannes Sixt wrote:
> Am 08.12.2016 um 08:55 schrieb Torsten Bögershausen:
> >Some conversion may be done in mingw.c:
> >https://github.com/github/git-msysgit/blob/master/compat/mingw.c
> >So what I understand, '/' in Git are already converted into '\' if needed ?
>
> Only if needed, and there are not many places where this is the
> case. (Actually, I can't find one place where we do.) In particular,
> typical file accesses does not require the conversion.
That's convenient, that way I don't have to worry about using '\' as a
directory separator instead of '/'.
>
> >It seams that we may wnat a function get_start_of_path(uncpath),
> >which returns:
> >
> >get_start_of_path_win("//?/D:/very-long-path") "/very-long-path"
>
> We have offset_1st_component().
Thanks for letting me know this function exists, that makes my job a bit
easier.
--
Brandon Williams
[PATCH v2 05/16] pathspec: remove the deprecated get_pathspec function
Now that all callers of the old 'get_pathspec' interface have been
migrated to use the new pathspec struct interface it can be removed
from the codebase.
Since there are no more users of the '_raw' field in the pathspec struct
it can also be removed. This patch also removes the old functionality
of modifying the const char **argv array that was passed into
parse_pathspec. Instead the constructed 'match' string (which is a
pathspec element with the prefix prepended) is only stored in its
corresponding pathspec_item entry.
Signed-off-by: Brandon Williams
---
Documentation/technical/api-setup.txt | 2 --
cache.h | 1 -
pathspec.c| 42 +++
pathspec.h| 1 -
4 files changed, 3 insertions(+), 43 deletions(-)
diff --git a/Documentation/technical/api-setup.txt
b/Documentation/technical/api-setup.txt
index 540e455..eb1fa98 100644
--- a/Documentation/technical/api-setup.txt
+++ b/Documentation/technical/api-setup.txt
@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments:
- prefix and args come from cmd_* functions
-get_pathspec() is obsolete and should never be used in new code.
-
parse_pathspec() helps catch unsupported features and reject them
politely. At a lower level, different pathspec-related functions may
not support the same set of features. Such pathspec-sensitive
diff --git a/cache.h b/cache.h
index a50a61a..0f80e01 100644
--- a/cache.h
+++ b/cache.h
@@ -514,7 +514,6 @@ extern void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
-extern const char **get_pathspec(const char *prefix, const char **pathspec);
extern void setup_work_tree(void);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
diff --git a/pathspec.c b/pathspec.c
index 22ca74a..1f918cb 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -103,7 +103,7 @@ static void prefix_short_magic(struct strbuf *sb, int
prefixlen,
*/
static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned *p_short_magic,
- const char **raw, unsigned flags,
+ unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
@@ -240,7 +240,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
- *raw = item->match = match;
+ item->match = match;
/*
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
@@ -381,8 +381,6 @@ void parse_pathspec(struct pathspec *pathspec,
/* No arguments with prefix -> prefix pathspec */
if (!entry) {
- static const char *raw[2];
-
if (flags & PATHSPEC_PREFER_FULL)
return;
@@ -394,10 +392,7 @@ void parse_pathspec(struct pathspec *pathspec,
item->original = prefix;
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
- raw[0] = prefix;
- raw[1] = NULL;
pathspec->nr = 1;
- pathspec->_raw = raw;
return;
}
@@ -415,7 +410,6 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec->nr = n;
ALLOC_ARRAY(pathspec->items, n);
item = pathspec->items;
- pathspec->_raw = argv;
prefixlen = prefix ? strlen(prefix) : 0;
for (i = 0; i < n; i++) {
@@ -423,7 +417,7 @@ void parse_pathspec(struct pathspec *pathspec,
entry = argv[i];
item[i].magic = prefix_pathspec(item + i, &short_magic,
- argv + i, flags,
+ flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -457,36 +451,6 @@ void parse_pathspec(struct pathspec *pathspec,
}
}
-/*
- * N.B. get_pathspec() is deprecated in favor of the "struct pathspec"
- * based interface - see pathspec.c:parse_pathspec().
- *
- * Arguments:
- * - prefix - a path relative to the root of the working tree
- * - pathspec - a list of paths underneath the prefix path
- *
- * Iterates over pathspec, prepending each path with prefix,
- * and return the resulting list.
- *
- * If pathspec is empty, return a singleton list containing prefix.
- *
- * If pathspec and prefix are both empty, return an empty list.
- *
- * This is typically used by built-in commands such as add.c, in order
- * to normalize argv arguments provided to the built-in into a li
[PATCH v2 15/16] pathspec: small readability changes
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, and
adding additional comments.
Signed-off-by: Brandon Williams
---
pathspec.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index cf88390..4686298 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -336,6 +336,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
die(_("%s: 'literal' and 'glob' are incompatible"), elt);
+ /* Create match string which will be used for pathspec matching */
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
@@ -343,11 +344,16 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
- match = prefix_path_gently(prefix, prefixlen, &prefixlen,
copyfrom);
+ match = prefix_path_gently(prefix, prefixlen,
+ &prefixlen, copyfrom);
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
+
item->match = match;
+ item->len = strlen(item->match);
+ item->prefix = prefixlen;
+
/*
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
@@ -364,8 +370,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
} else {
item->original = xstrdup(elt);
}
- item->len = strlen(item->match);
- item->prefix = prefixlen;
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
strip_submodule_slash_cheap(item);
@@ -373,13 +377,14 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
strip_submodule_slash_expensive(item);
- if (magic & PATHSPEC_LITERAL)
+ if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
- else {
+ } else {
item->nowildcard_len = simple_length(item->match);
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
}
+
item->flags = 0;
if (magic & PATHSPEC_GLOB) {
/*
--
2.8.0.rc3.226.g39d4020
[PATCH v2 04/16] ls-tree: convert show_recursive to use the pathspec struct interface
Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams
---
builtin/ls-tree.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 0e30d86..d7ebeb4 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -31,21 +31,18 @@ static const char * const ls_tree_usage[] = {
static int show_recursive(const char *base, int baselen, const char *pathname)
{
- const char **s;
+ int i;
if (ls_options & LS_RECURSIVE)
return 1;
- s = pathspec._raw;
- if (!s)
+ if (!pathspec.nr)
return 0;
- for (;;) {
- const char *spec = *s++;
+ for (i = 0; i < pathspec.nr; i++) {
+ const char *spec = pathspec.items[i].match;
int len, speclen;
- if (!spec)
- return 0;
if (strncmp(base, spec, baselen))
continue;
len = strlen(pathname);
@@ -59,6 +56,7 @@ static int show_recursive(const char *base, int baselen,
const char *pathname)
continue;
return 1;
}
+ return 0;
}
static int show_tree(const unsigned char *sha1, struct strbuf *base,
@@ -175,8 +173,8 @@ int cmd_ls_tree(int argc, const char **argv, const char
*prefix)
* cannot be lifted until it is converted to use
* match_pathspec() or tree_entry_interesting()
*/
- parse_pathspec(&pathspec, PATHSPEC_GLOB | PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE,
+ parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
PATHSPEC_PREFER_CWD,
prefix, argv + 1);
for (i = 0; i < pathspec.nr; i++)
--
2.8.0.rc3.226.g39d4020
[PATCH v2 08/16] pathspec: always show mnemonic and name in unsupported_magic
For better clarity, always show the mnemonic and name of the unsupported
magic being used. This lets users have a more clear understanding of
what magic feature isn't supported. And if they supplied a mnemonic,
the user will be told what its corresponding name is which will allow
them to more easily search the man pages for that magic type.
This also avoids passing an extra parameter around the pathspec
initialization code.
Signed-off-by: Brandon Williams
---
pathspec.c | 23 ---
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index ec0d590..a360193 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" },
@@ -101,9 +101,7 @@ static void prefix_short_magic(struct strbuf *sb, int
prefixlen,
* the prefix part must always match literally, and a single stupid
* string cannot express such a case.
*/
-static unsigned prefix_pathspec(struct pathspec_item *item,
- unsigned *p_short_magic,
- unsigned flags,
+static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
@@ -210,7 +208,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
magic |= short_magic;
- *p_short_magic = short_magic;
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -329,8 +326,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
}
static void NORETURN unsupported_magic(const char *pattern,
- unsigned magic,
- unsigned short_magic)
+ unsigned magic)
{
struct strbuf sb = STRBUF_INIT;
int i;
@@ -340,8 +336,9 @@ static void NORETURN unsupported_magic(const char *pattern,
continue;
if (sb.len)
strbuf_addch(&sb, ' ');
- if (short_magic & m->bit)
- strbuf_addf(&sb, "'%c'", m->mnemonic);
+
+ if (m->mnemonic)
+ strbuf_addf(&sb, "'(%c)%s'", m->mnemonic, m->name);
else
strbuf_addf(&sb, "'%s'", m->name);
}
@@ -413,11 +410,9 @@ void parse_pathspec(struct pathspec *pathspec,
prefixlen = prefix ? strlen(prefix) : 0;
for (i = 0; i < n; i++) {
- unsigned short_magic;
entry = argv[i];
- item[i].magic = prefix_pathspec(item + i, &short_magic,
- flags,
+ item[i].magic = prefix_pathspec(item + i, flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -425,9 +420,7 @@ void parse_pathspec(struct pathspec *pathspec,
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
if (item[i].magic & magic_mask)
- unsupported_magic(entry,
- item[i].magic & magic_mask,
- short_magic);
+ unsupported_magic(entry, item[i].magic & magic_mask);
if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
has_symlink_leading_path(item[i].match, item[i].len)) {
--
2.8.0.rc3.226.g39d4020
[PATCH v2 14/16] pathspec: create strip submodule slash helpers
Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.
Signed-off-by: Brandon Williams
---
pathspec.c | 68 ++
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index f37f887..cf88390 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic,
int *prefix_len,
return parse_short_magic(magic, elem);
}
+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';
+ }
+}
+
+static void strip_submodule_slash_expensive(struct pathspec_item *item)
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int ce_len = ce_namelen(ce);
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+ if (item->len <= ce_len || item->match[ce_len] != '/' ||
+ memcmp(ce->name, item->match, ce_len))
+ continue;
+
+ if (item->len == ce_len + 1) {
+ /* strip trailing slash */
+ item->len--;
+ item->match[item->len] = '\0';
+ } else {
+ die (_("Pathspec '%s' is in submodule '%.*s'"),
+item->original, ce_len, ce->name);
+ }
+ }
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
- int i, pathspec_prefix = -1;
+ int pathspec_prefix = -1;
/* PATHSPEC_LITERAL_PATH ignores magic */
if (flags & PATHSPEC_LITERAL_PATH) {
@@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
item->len = strlen(item->match);
item->prefix = prefixlen;
- if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
- (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--;
- match[item->len] = '\0';
- }
+ if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
+ strip_submodule_slash_cheap(item);
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
- for (i = 0; i < active_nr; i++) {
- struct cache_entry *ce = active_cache[i];
- int ce_len = ce_namelen(ce);
-
- if (!S_ISGITLINK(ce->ce_mode))
- continue;
-
- if (item->len <= ce_len || match[ce_len] != '/' ||
- memcmp(ce->name, match, ce_len))
- continue;
- if (item->len == ce_len + 1) {
- /* strip trailing slash */
- item->len--;
- match[item->len] = '\0';
- } else
- die (_("Pathspec '%s' is in submodule '%.*s'"),
-elt, ce_len, ce->name);
- }
+ strip_submodule_slash_expensive(item);
if (magic & PATHSPEC_LITERAL)
item->nowildcard_len = item->len;
--
2.8.0.rc3.226.g39d4020
[PATCH v2 13/16] pathspec: create parse_element_magic helper
Factor out the logic responsible for the magic in a pathspec element
into its own function.
Also avoid calling into the parsing functions when
`PATHSPEC_LITERAL_PATH` is specified since it causes magic to be
ignored and all paths to be treated as literals.
Signed-off-by: Brandon Williams
---
pathspec.c | 37 -
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 6e9555e..f37f887 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -245,6 +245,19 @@ static const char *parse_short_magic(unsigned *magic,
const char *elem)
return pos;
}
+static const char *parse_element_magic(unsigned *magic, int *prefix_len,
+ const char *elem)
+{
+ if (elem[0] != ':' || get_literal_global())
+ return elem; /* nothing to do */
+ else if (elem[1] == '(')
+ /* longhand */
+ return parse_long_magic(magic, prefix_len, elem);
+ else
+ /* shorthand */
+ return parse_short_magic(magic, elem);
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -267,26 +280,16 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
char *match;
int i, pathspec_prefix = -1;
- if (elt[0] != ':' || get_literal_global() ||
- (flags & PATHSPEC_LITERAL_PATH)) {
- ; /* nothing to do */
- } else if (elt[1] == '(') {
- /* longhand */
- copyfrom = parse_long_magic(&element_magic,
- &pathspec_prefix,
- elt);
- } else {
- /* shorthand */
- copyfrom = parse_short_magic(&element_magic, elt);
- }
-
- magic |= element_magic;
-
/* PATHSPEC_LITERAL_PATH ignores magic */
- if (flags & PATHSPEC_LITERAL_PATH)
+ if (flags & PATHSPEC_LITERAL_PATH) {
magic = PATHSPEC_LITERAL;
- else
+ } else {
+ copyfrom = parse_element_magic(&element_magic,
+ &pathspec_prefix,
+ elt);
+ magic |= element_magic;
magic |= get_global_magic(element_magic);
+ }
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
--
2.8.0.rc3.226.g39d4020
[PATCH v2 12/16] pathspec: create parse_long_magic function
Factor out the logic responsible for parsing long magic into its own
function. As well as hoist the prefix check logic outside of the inner
loop as there isn't anything that needs to be done after matching
"prefix:".
Signed-off-by: Brandon Williams
---
pathspec.c | 92 ++
1 file changed, 57 insertions(+), 35 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 29054d2..6e9555e 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -157,6 +157,60 @@ static int get_global_magic(int element_magic)
}
/*
+ * Parse the pathspec element looking for long magic
+ *
+ * saves all magic in 'magic'
+ * if prefix magic is used, save the prefix length in 'prefix_len'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_long_magic(unsigned *magic, int *prefix_len,
+ const char *elem)
+{
+ const char *pos;
+ const char *nextat;
+
+ for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
+ size_t len = strcspn(pos, ",)");
+ int i;
+
+ if (pos[len] == ',')
+ nextat = pos + len + 1; /* handle ',' */
+ else
+ nextat = pos + len; /* handle ')' and '\0' */
+
+ if (!len)
+ continue;
+
+ if (starts_with(pos, "prefix:")) {
+ char *endptr;
+ *prefix_len = strtol(pos + 7, &endptr, 10);
+ if (endptr - pos != len)
+ die(_("invalid parameter for pathspec magic
'prefix'"));
+ continue;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if (strlen(pathspec_magic[i].name) == len &&
+ !strncmp(pathspec_magic[i].name, pos, len)) {
+ *magic |= pathspec_magic[i].bit;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(pathspec_magic) <= i)
+ die(_("Invalid pathspec magic '%.*s' in '%s'"),
+ (int) len, pos, elem);
+ }
+
+ if (*pos != ')')
+ die(_("Missing ')' at the end of pathspec magic in '%s'"),
+ elem);
+ pos++;
+
+ return pos;
+}
+
+/*
* Parse the pathspec element looking for short magic
*
* saves all magic in 'magic'
@@ -218,41 +272,9 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
; /* nothing to do */
} else if (elt[1] == '(') {
/* longhand */
- const char *nextat;
- for (copyfrom = elt + 2;
-*copyfrom && *copyfrom != ')';
-copyfrom = nextat) {
- size_t len = strcspn(copyfrom, ",)");
- if (copyfrom[len] == ',')
- nextat = copyfrom + len + 1;
- else
- /* handle ')' and '\0' */
- nextat = copyfrom + len;
- if (!len)
- continue;
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
- if (strlen(pathspec_magic[i].name) == len &&
- !strncmp(pathspec_magic[i].name, copyfrom,
len)) {
- element_magic |= pathspec_magic[i].bit;
- break;
- }
- if (starts_with(copyfrom, "prefix:")) {
- char *endptr;
- pathspec_prefix = strtol(copyfrom + 7,
-&endptr, 10);
- if (endptr - copyfrom != len)
- die(_("invalid parameter for
pathspec magic 'prefix'"));
- /* "i" would be wrong, but it does not
matter */
- break;
- }
- }
- if (ARRAY_SIZE(pathspec_magic) <= i)
- die(_("Invalid pathspec magic '%.*s' in '%s'"),
- (int) len, copyfrom, elt);
- }
- if (*copyfrom != ')')
- die(_("Missing ')' at the end of pathspec magic in
'%s'"), elt);
- copyfrom++;
+ copyfrom = parse_long_magic(&element_magic,
+ &pathspec_prefix,
+ elt);
} else {
/* shorthand */
copyfrom = parse_short_
[PATCH v2 16/16] pathspec: rename prefix_pathspec to init_pathspec_item
Give a more relevant name to the prefix_pathspec function as it does
more than just prefix a pathspec element.
Signed-off-by: Brandon Williams
---
pathspec.c | 24 +++-
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 4686298..08abdd3 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -297,21 +297,11 @@ static void strip_submodule_slash_expensive(struct
pathspec_item *item)
}
/*
- * Take an element of a pathspec and check for magic signatures.
- * Append the result to the prefix. Return the magic bitmap.
- *
- * For now, we only parse the syntax and throw out anything other than
- * "top" magic.
- *
- * NEEDSWORK: This needs to be rewritten when we start migrating
- * get_pathspec() users to use the "struct pathspec" interface. For
- * example, a pathspec element may be marked as case-insensitive, but
- * the prefix part must always match literally, and a single stupid
- * string cannot express such a case.
+ * Perform the initialization of a pathspec_item based on a pathspec element.
*/
-static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
- const char *prefix, int prefixlen,
- const char *elt)
+static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
+ const char *prefix, int prefixlen,
+ const char *elt)
{
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
@@ -329,6 +319,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
magic |= get_global_magic(element_magic);
}
+ item->magic = magic;
+
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
die("BUG: 'prefix' magic is supposed to be used at worktree's
root");
@@ -401,7 +393,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
/* sanity checks, pathspec matchers assume these are sane */
assert(item->nowildcard_len <= item->len &&
item->prefix <= item->len);
- return magic;
}
static int pathspec_item_cmp(const void *a_, const void *b_)
@@ -500,8 +491,7 @@ void parse_pathspec(struct pathspec *pathspec,
for (i = 0; i < n; i++) {
entry = argv[i];
- item[i].magic = prefix_pathspec(item + i, flags,
- prefix, prefixlen, entry);
+ init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
--
2.8.0.rc3.226.g39d4020
[PATCH v2 06/16] pathspec: copy and free owned memory
The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.
Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').
Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.
Signed-off-by: Brandon Williams
---
pathspec.c | 22 ++
pathspec.h | 4 ++--
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 1f918cb..8f367f0 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -259,8 +259,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL);
- } else
- item->original = elt;
+ } else {
+ item->original = xstrdup(elt);
+ }
item->len = strlen(item->match);
item->prefix = prefixlen;
@@ -388,8 +389,8 @@ void parse_pathspec(struct pathspec *pathspec,
die("BUG: PATHSPEC_PREFER_CWD requires arguments");
pathspec->items = item = xcalloc(1, sizeof(*item));
- item->match = prefix;
- item->original = prefix;
+ item->match = xstrdup(prefix);
+ item->original = xstrdup(prefix);
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
pathspec->nr = 1;
@@ -453,13 +454,26 @@ void parse_pathspec(struct pathspec *pathspec,
void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
+ int i;
+
*dst = *src;
ALLOC_ARRAY(dst->items, dst->nr);
COPY_ARRAY(dst->items, src->items, dst->nr);
+
+ for (i = 0; i < dst->nr; i++) {
+ dst->items[i].match = xstrdup(src->items[i].match);
+ dst->items[i].original = xstrdup(src->items[i].original);
+ }
}
void clear_pathspec(struct pathspec *pathspec)
{
+ int i;
+
+ for (i = 0; i < pathspec->nr; i++) {
+ free(pathspec->items[i].match);
+ free(pathspec->items[i].original);
+ }
free(pathspec->items);
pathspec->items = NULL;
}
diff --git a/pathspec.h b/pathspec.h
index 70a592e..49fd823 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -25,8 +25,8 @@ struct pathspec {
unsigned magic;
int max_depth;
struct pathspec_item {
- const char *match;
- const char *original;
+ char *match;
+ char *original;
unsigned magic;
int len, prefix;
int nowildcard_len;
--
2.8.0.rc3.226.g39d4020
[PATCH v2 11/16] pathspec: create parse_short_magic function
Factor out the logic responsible for parsing short magic into its own
function.
Signed-off-by: Brandon Williams
---
pathspec.c | 54 --
1 file changed, 36 insertions(+), 18 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 523d7bf..29054d2 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -157,6 +157,41 @@ static int get_global_magic(int element_magic)
}
/*
+ * Parse the pathspec element looking for short magic
+ *
+ * saves all magic in 'magic'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_short_magic(unsigned *magic, const char *elem)
+{
+ const char *pos;
+
+ for (pos = elem + 1; *pos && *pos != ':'; pos++) {
+ char ch = *pos;
+ int i;
+
+ if (!is_pathspec_magic(ch))
+ break;
+
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if (pathspec_magic[i].mnemonic == ch) {
+ *magic |= pathspec_magic[i].bit;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(pathspec_magic) <= i)
+ die(_("Unimplemented pathspec magic '%c' in '%s'"),
+ ch, elem);
+ }
+
+ if (*pos == ':')
+ pos++;
+
+ return pos;
+}
+
+/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
*
@@ -220,24 +255,7 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
copyfrom++;
} else {
/* shorthand */
- for (copyfrom = elt + 1;
-*copyfrom && *copyfrom != ':';
-copyfrom++) {
- char ch = *copyfrom;
-
- if (!is_pathspec_magic(ch))
- break;
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (pathspec_magic[i].mnemonic == ch) {
- element_magic |= pathspec_magic[i].bit;
- break;
- }
- if (ARRAY_SIZE(pathspec_magic) <= i)
- die(_("Unimplemented pathspec magic '%c' in
'%s'"),
- ch, elt);
- }
- if (*copyfrom == ':')
- copyfrom++;
+ copyfrom = parse_short_magic(&element_magic, elt);
}
magic |= element_magic;
--
2.8.0.rc3.226.g39d4020
[PATCH v2 02/16] dir: convert create_simplify to use the pathspec struct interface
Convert 'create_simplify()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec.
Signed-off-by: Brandon Williams
---
dir.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/dir.c b/dir.c
index bfa8c8a..7df292b 100644
--- a/dir.c
+++ b/dir.c
@@ -1787,25 +1787,24 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
}
-static struct path_simplify *create_simplify(const char **pathspec)
+static struct path_simplify *create_simplify(const struct pathspec *pathspec)
{
- int nr, alloc = 0;
+ int i;
struct path_simplify *simplify = NULL;
- if (!pathspec)
+ if (!pathspec || !pathspec->nr)
return NULL;
- for (nr = 0 ; ; nr++) {
+ ALLOC_ARRAY(simplify, pathspec->nr + 1);
+ for (i = 0; i < pathspec->nr; i++) {
const char *match;
- ALLOC_GROW(simplify, nr + 1, alloc);
- match = *pathspec++;
- if (!match)
- break;
- simplify[nr].path = match;
- simplify[nr].len = simple_length(match);
+ match = pathspec->items[i].match;
+ simplify[i].path = match;
+ simplify[i].len = pathspec->items[i].nowildcard_len;
}
- simplify[nr].path = NULL;
- simplify[nr].len = 0;
+ simplify[i].path = NULL;
+ simplify[i].len = 0;
+
return simplify;
}
@@ -2036,7 +2035,7 @@ int read_directory(struct dir_struct *dir, const char
*path, int len, const stru
* subset of positive ones, which has no impacts on
* create_simplify().
*/
- simplify = create_simplify(pathspec ? pathspec->_raw : NULL);
+ simplify = create_simplify(pathspec);
untracked = validate_untracked_cache(dir, len, pathspec);
if (!untracked)
/*
--
2.8.0.rc3.226.g39d4020
[PATCH v2 10/16] pathspec: factor global magic into its own function
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams
---
pathspec.c | 127 +
1 file changed, 78 insertions(+), 49 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 49adea4..523d7bf 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -87,6 +87,75 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
unsigned magic)
strbuf_addf(sb, ",prefix:%d)", prefixlen);
}
+static inline int get_literal_global(void)
+{
+ static int literal = -1;
+
+ if (literal < 0)
+ literal = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
+
+ return literal;
+}
+
+static inline int get_glob_global(void)
+{
+ static int glob = -1;
+
+ if (glob < 0)
+ glob = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
+
+ return glob;
+}
+
+static inline int get_noglob_global(void)
+{
+ static int noglob = -1;
+
+ if (noglob < 0)
+ noglob = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT, 0);
+
+ return noglob;
+}
+
+static inline int get_icase_global(void)
+{
+ static int icase = -1;
+
+ if (icase < 0)
+ icase = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
+
+ return icase;
+}
+
+static int get_global_magic(int element_magic)
+{
+ int global_magic = 0;
+
+ if (get_literal_global())
+ global_magic |= PATHSPEC_LITERAL;
+
+ /* --glob-pathspec is overridden by :(literal) */
+ if (get_glob_global() && !(element_magic & PATHSPEC_LITERAL))
+ global_magic |= PATHSPEC_GLOB;
+
+ if (get_glob_global() && get_noglob_global())
+ die(_("global 'glob' and 'noglob' pathspec settings are
incompatible"));
+
+ if (get_icase_global())
+ global_magic |= PATHSPEC_ICASE;
+
+ if ((global_magic & PATHSPEC_LITERAL) &&
+ (global_magic & ~PATHSPEC_LITERAL))
+ die(_("global 'literal' pathspec setting is incompatible "
+ "with all other global pathspec settings"));
+
+ /* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
+ if (get_noglob_global() && !(element_magic & PATHSPEC_GLOB))
+ global_magic |= PATHSPEC_LITERAL;
+
+ return global_magic;
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -104,46 +173,12 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
- static int literal_global = -1;
- static int glob_global = -1;
- static int noglob_global = -1;
- static int icase_global = -1;
- unsigned magic = 0, element_magic = 0, global_magic = 0;
+ unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
- if (literal_global < 0)
- literal_global =
git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
- if (literal_global)
- global_magic |= PATHSPEC_LITERAL;
-
- if (glob_global < 0)
- glob_global = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
- if (glob_global)
- global_magic |= PATHSPEC_GLOB;
-
- if (noglob_global < 0)
- noglob_global = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT,
0);
-
- if (glob_global && noglob_global)
- die(_("global 'glob' and 'noglob' pathspec settings are
incompatible"));
-
-
- if (icase_global < 0)
- icase_global = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
- if (icase_global)
- global_magic |= PATHSPEC_ICASE;
-
- if ((global_magic & PATHSPEC_LITERAL) &&
- (global_magic & ~PATHSPEC_LITERAL))
- die(_("global 'literal' pathspec setting is incompatible "
- "with all other global pathspec settings"));
-
- if (flags & PATHSPEC_LITERAL_PATH)
- global_magic = 0;
-
- if (elt[0] != ':' || literal_global ||
+ if (elt[0] != ':' || get_literal_global() ||
(flags & PATHSPEC_LITERAL_PATH)) {
; /* nothing to do */
} else if (elt[1] == '(') {
@@ -207,15 +242,11 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
magic |= element_magic;
- /* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
- if (noglob_global && !(magic & PATHSPEC_GLOB))
- global_magic |= PATHSPEC_LITERAL;
-
- /* --glob-pathspec is overridden by :(literal) */
- if ((global_magic & PATHSPEC_GLOB) && (magic & PATHSPEC_LITERAL))
- globa
[PATCH v2 03/16] dir: convert fill_directory to use the pathspec struct interface
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams
---
dir.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index 7df292b..a50b6f0 100644
--- a/dir.c
+++ b/dir.c
@@ -179,17 +179,21 @@ char *common_prefix(const struct pathspec *pathspec)
int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
{
- size_t len;
+ char *prefix;
+ size_t prefix_len;
/*
* Calculate common prefix for the pathspec, and
* use that to optimize the directory walk
*/
- len = common_prefix_len(pathspec);
+ prefix = common_prefix(pathspec);
+ prefix_len = prefix ? strlen(prefix) : 0;
/* Read the directory and prune it */
- read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len,
pathspec);
- return len;
+ read_directory(dir, prefix, prefix_len, pathspec);
+
+ free(prefix);
+ return prefix_len;
}
int within_depth(const char *name, int namelen,
--
2.8.0.rc3.226.g39d4020
[PATCH v2 09/16] pathspec: simpler logic to prefix original pathspec elements
The logic used to prefix an original pathspec element with 'prefix'
magic is more general purpose and can be used for more than just short
magic. Remove the extra code paths and rename 'prefix_short_magic' to
'prefix_magic' to better indicate that it can be used in more general
situations.
Also, slightly change the logic which decides when to prefix the
original element in order to prevent a pathspec of "." from getting
converted to "" (empty string).
Signed-off-by: Brandon Williams
---
pathspec.c | 33 +
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index a360193..49adea4 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -74,13 +74,12 @@ static struct pathspec_magic {
{ PATHSPEC_EXCLUDE, '!', "exclude" },
};
-static void prefix_short_magic(struct strbuf *sb, int prefixlen,
- unsigned short_magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
{
int i;
strbuf_addstr(sb, ":(");
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (short_magic & pathspec_magic[i].bit) {
+ if (magic & pathspec_magic[i].bit) {
if (sb->buf[sb->len - 1] != '(')
strbuf_addch(sb, ',');
strbuf_addstr(sb, pathspec_magic[i].name);
@@ -109,8 +108,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
static int glob_global = -1;
static int noglob_global = -1;
static int icase_global = -1;
- unsigned magic = 0, short_magic = 0, global_magic = 0;
- const char *copyfrom = elt, *long_magic_end = NULL;
+ unsigned magic = 0, element_magic = 0, global_magic = 0;
+ const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
@@ -164,7 +163,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
if (strlen(pathspec_magic[i].name) == len &&
!strncmp(pathspec_magic[i].name, copyfrom,
len)) {
- magic |= pathspec_magic[i].bit;
+ element_magic |= pathspec_magic[i].bit;
break;
}
if (starts_with(copyfrom, "prefix:")) {
@@ -183,7 +182,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
}
if (*copyfrom != ')')
die(_("Missing ')' at the end of pathspec magic in
'%s'"), elt);
- long_magic_end = copyfrom;
copyfrom++;
} else {
/* shorthand */
@@ -196,7 +194,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
break;
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
if (pathspec_magic[i].mnemonic == ch) {
- short_magic |= pathspec_magic[i].bit;
+ element_magic |= pathspec_magic[i].bit;
break;
}
if (ARRAY_SIZE(pathspec_magic) <= i)
@@ -207,7 +205,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
copyfrom++;
}
- magic |= short_magic;
+ magic |= element_magic;
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -242,18 +240,13 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
*/
- if (flags & PATHSPEC_PREFIX_ORIGIN) {
+ if ((flags & PATHSPEC_PREFIX_ORIGIN) &&
+ prefixlen && !literal_global) {
struct strbuf sb = STRBUF_INIT;
- if (prefixlen && !literal_global) {
- /* Preserve the actual prefix length of each pattern */
- if (short_magic)
- prefix_short_magic(&sb, prefixlen, short_magic);
- else if (long_magic_end) {
- strbuf_add(&sb, elt, long_magic_end - elt);
- strbuf_addf(&sb, ",prefix:%d)", prefixlen);
- } else
- strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
- }
+
+ /* Preserve the actual prefix length of each pattern */
+ prefix_magic(&sb, prefixlen, element_magic);
+
strbuf_addstr(&sb, match
[PATCH v2 00/16] pathspec cleanup
v2 of this series addresses the comments brought up in v1, most of which were
small cosmetic changes (since this is mostly a cosmetic series to begin with).
Brandon Williams (16):
mv: remove use of deprecated 'get_pathspec()'
dir: convert create_simplify to use the pathspec struct interface
dir: convert fill_directory to use the pathspec struct interface
ls-tree: convert show_recursive to use the pathspec struct interface
pathspec: remove the deprecated get_pathspec function
pathspec: copy and free owned memory
pathspec: remove unused variable from unsupported_magic
pathspec: always show mnemonic and name in unsupported_magic
pathspec: simpler logic to prefix original pathspec elements
pathspec: factor global magic into its own function
pathspec: create parse_short_magic function
pathspec: create parse_long_magic function
pathspec: create parse_element_magic helper
pathspec: create strip submodule slash helpers
pathspec: small readability changes
pathspec: rename prefix_pathspec to init_pathspec_item
Documentation/technical/api-setup.txt | 2 -
builtin/ls-tree.c | 16 +-
builtin/mv.c | 50 ++--
cache.h | 1 -
dir.c | 37 +--
pathspec.c| 468 +++---
pathspec.h| 5 +-
7 files changed, 317 insertions(+), 262 deletions(-)
--- interdiff from v1
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index e0f4307..d7ebeb4 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -173,8 +173,8 @@ int cmd_ls_tree(int argc, const char **argv, const char
*prefix)
* cannot be lifted until it is converted to use
* match_pathspec() or tree_entry_interesting()
*/
- parse_pathspec(&pathspec, PATHSPEC_GLOB | PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE,
+ parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
PATHSPEC_PREFER_CWD,
prefix, argv + 1);
for (i = 0; i < pathspec.nr; i++)
diff --git a/builtin/mv.c b/builtin/mv.c
index b7cceb6..4e86dc5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -20,13 +20,13 @@ static const char * const builtin_mv_usage[] = {
#define DUP_BASENAME 1
#define KEEP_TRAILING_SLASH 2
-static const char **internal_copy_pathspec(const char *prefix,
- const char **pathspec,
- int count, unsigned flags)
+static const char **internal_prefix_pathspec(const char *prefix,
+const char **pathspec,
+int count, unsigned flags)
{
int i;
const char **result;
- struct pathspec ps;
+ int prefixlen = prefix ? strlen(prefix) : 0;
ALLOC_ARRAY(result, count + 1);
/* Create an intermediate copy of the pathspec based on the flags */
@@ -42,25 +42,19 @@ static const char **internal_copy_pathspec(const char
*prefix,
if (flags & DUP_BASENAME) {
result[i] = xstrdup(basename(it));
free(it);
- } else
+ } else {
result[i] = it;
+ }
}
result[count] = NULL;
- parse_pathspec(&ps,
- PATHSPEC_ALL_MAGIC &
- ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
- PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
- prefix, result);
- assert(count == ps.nr);
-
- /* Copy the pathspec and free the old intermediate strings */
+ /* Prefix the pathspec and free the old intermediate strings */
for (i = 0; i < count; i++) {
+ const char *match = prefix_path(prefix, prefixlen, result[i]);
free((char *) result[i]);
- result[i] = xstrdup(ps.items[i].match);
+ result[i] = match;
}
- clear_pathspec(&ps);
return result;
}
@@ -148,7 +142,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (read_cache() < 0)
die(_("index file corrupt"));
- source = internal_copy_pathspec(prefix, argv, argc, 0);
+ source = internal_prefix_pathspec(prefix, argv, argc, 0);
modes = xcalloc(argc, sizeof(enum update_mode));
/*
* Keep trailing slash, needed to let
@@ -158,16 +152,16 @@ int cmd_mv(int argc, const char **argv, const char
*prefix)
flags = KEEP_TRAILING_SLASH;
if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
flags = 0;
- dest_path = internal_copy_pathspec(prefix, argv + argc, 1, flags);
+ dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
submodule_
[PATCH v2 01/16] mv: remove use of deprecated 'get_pathspec()'
Convert the 'internal_copy_pathspec()' function to 'prefix_path()'
instead of using the deprecated 'get_pathspec()' interface. Also,
rename 'internal_copy_pathspec()' to 'internal_prefix_pathspec()' to be
more descriptive of what the funciton is actually doing.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed after getting the result from 'prefix_path()'.
Signed-off-by: Brandon Williams
---
builtin/mv.c | 50 +++---
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index 2f43877..4e86dc5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -4,6 +4,7 @@
* Copyright (C) 2006 Johannes Schindelin
*/
#include "builtin.h"
+#include "pathspec.h"
#include "lockfile.h"
#include "dir.h"
#include "cache-tree.h"
@@ -19,31 +20,42 @@ static const char * const builtin_mv_usage[] = {
#define DUP_BASENAME 1
#define KEEP_TRAILING_SLASH 2
-static const char **internal_copy_pathspec(const char *prefix,
- const char **pathspec,
- int count, unsigned flags)
+static const char **internal_prefix_pathspec(const char *prefix,
+const char **pathspec,
+int count, unsigned flags)
{
int i;
const char **result;
+ int prefixlen = prefix ? strlen(prefix) : 0;
ALLOC_ARRAY(result, count + 1);
- COPY_ARRAY(result, pathspec, count);
- result[count] = NULL;
+
+ /* Create an intermediate copy of the pathspec based on the flags */
for (i = 0; i < count; i++) {
- int length = strlen(result[i]);
+ int length = strlen(pathspec[i]);
int to_copy = length;
+ char *it;
while (!(flags & KEEP_TRAILING_SLASH) &&
- to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
+ to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
to_copy--;
- if (to_copy != length || flags & DUP_BASENAME) {
- char *it = xmemdupz(result[i], to_copy);
- if (flags & DUP_BASENAME) {
- result[i] = xstrdup(basename(it));
- free(it);
- } else
- result[i] = it;
+
+ it = xmemdupz(pathspec[i], to_copy);
+ if (flags & DUP_BASENAME) {
+ result[i] = xstrdup(basename(it));
+ free(it);
+ } else {
+ result[i] = it;
}
}
- return get_pathspec(prefix, result);
+ result[count] = NULL;
+
+ /* Prefix the pathspec and free the old intermediate strings */
+ for (i = 0; i < count; i++) {
+ const char *match = prefix_path(prefix, prefixlen, result[i]);
+ free((char *) result[i]);
+ result[i] = match;
+ }
+
+ return result;
}
static const char *add_slash(const char *path)
@@ -130,7 +142,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (read_cache() < 0)
die(_("index file corrupt"));
- source = internal_copy_pathspec(prefix, argv, argc, 0);
+ source = internal_prefix_pathspec(prefix, argv, argc, 0);
modes = xcalloc(argc, sizeof(enum update_mode));
/*
* Keep trailing slash, needed to let
@@ -140,16 +152,16 @@ int cmd_mv(int argc, const char **argv, const char
*prefix)
flags = KEEP_TRAILING_SLASH;
if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
flags = 0;
- dest_path = internal_copy_pathspec(prefix, argv + argc, 1, flags);
+ dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
submodule_gitfile = xcalloc(argc, sizeof(char *));
if (dest_path[0][0] == '\0')
/* special case: "." was normalized to "" */
- destination = internal_copy_pathspec(dest_path[0], argv, argc,
DUP_BASENAME);
+ destination = internal_prefix_pathspec(dest_path[0], argv,
argc, DUP_BASENAME);
else if (!lstat(dest_path[0], &st) &&
S_ISDIR(st.st_mode)) {
dest_path[0] = add_slash(dest_path[0]);
- destination = internal_copy_pathspec(dest_path[0], argv, argc,
DUP_BASENAME);
+ destination = internal_prefix_pathspec(dest_path[0], argv,
argc, DUP_BASENAME);
} else {
if (argc != 1)
die(_("destination '%s' is not a directory"),
dest_path[0]);
--
Re: [PATCH 2/2] ref-filter: add function to parse atoms from a nul-terminated string
On Wed, Dec 7, 2016 at 5:09 PM, SZEDER Gábor wrote: > ref-filter's parse_ref_filter_atom() function parses an atom between > the start and end pointers it gets as arguments. This is fine for two > of its callers, which process '%(atom)' format specifiers and the end > pointer comes directly from strchr() looking for the closing ')'. > However, it's not quite so straightforward for its other two callers, > which process sort specifiers given as plain nul-terminated strings. > Especially not for ref_default_sorting(), which has the default > hard-coded as a string literal, but can't use it directly, because a > pointer to the end of that string literal is needed as well. > The next patch will add yet another caller using a string literal. Oops, that last sentence should be deleted, there is no third patch, sorry. Gábor
Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
Jacob Keller writes: >> + are left behind. If a displayed ref has fewer components than >> + ``, 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? There probably are three ways to handle a formatting request that cannot be satisfied for some refs but not others [*1*]. I agree with you that dying the whole thing is probably the least useful one. We already format "%(taggername)" into an empty string when the ref points at an object that is not an annotated tag, and substituting an unsatisifiable request "%(refname:lstrip=N)" with an empty string for a ref whose name does not have enough number of components is in line with that existing practice. The other possibility is to omit refs that cannot be formatted according to the format specifier. We do not currently have provision for doing so, but it may not be bad if we can say: $ git for-each-ref \ --require="%(taggername)" \ --require="%(refname:lstrip=4)" \ --format="%(refname:short)" \ refs/tags/ to list _only_ annotated tags that has at least 4 components from refs/tags/ hierarchy. The "--require" thing obviously is an orthogonal feature, that nobody has asked for, and does not have to exist. For the purpose of this review thread, I think it is OK to just conclude: "--format" should replace "%(any unsatisifiable atom)" with an empty string. Thanks. [Footnote] *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.
Re: [PATCHv6 4/7] worktree: get worktrees from submodules
On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:
>>
>> worktree = xcalloc(1, sizeof(*worktree));
>> worktree->path = strbuf_detach(&worktree_path, NULL);
>> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
>
> All the good stuff is outside context lines again.. Somewhere between
> here we call add_head_info() which calls resolve_ref_unsafe(), which
> always uses data from current repo, not the submodule we want it to
> look at.
Unrelated side question: What would you think of "variable context line
configuration" ? e.g. you could configure it to include anything from
up that line
that is currently shown after the @@ which is the function signature line.
As to the add_head_info/resolve_ref_unsafe what impact does that have?
It produces a wrong head info but AFAICT it will never die(), such that for the
purposes of this series (which only wants to know if a submodule uses the
worktree feature) it should be fine.
It is highly misleading though for others to build upon this.
So maybe I'll only add the functionality internally in worktree.c
and document why the values are wrong, and only expose the
"int submodule_uses_worktrees(const char *path)" ?
>> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
>> return list;
>
> Right before this line is mark_current_worktree(), which in turn calls
> get_git_dir() and not suitable for peeking into another repository the
> way submodule code does. get_worktree_git_dir() called within that
> function shares the same problem.
It actually works correctly: "No submodule is the current worktree".
>
>> }
>>
>> +struct worktree **get_worktrees(unsigned flags)
>> +{
>> + return get_worktrees_internal(get_git_common_dir(), flags);
>> +}
>> +
>> +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);
>
> Technically we need to read submodule_gitdir/.config and see if we can
> understand core.repositoryformatversion, or find any unrecognized
> extensions. But the problem is not specific to this code. And fixing
> it is no small task. But perhaps we could call a dummy
> validate_submodule_gitdir() here? Then when we implement that function
> for real, we don't have to search the entire code base to see where to
> put it.
>
> Kinda off-topic though. Feel free to ignore the above comment.
ok I'll add a TODO/emptyfunction for that.
Thanks for the review!
Stefan
Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
On Thu, Dec 08, 2016 at 09:04:46PM +0300, vi0oss wrote:
> Why Git test use &&-chains instead of proper "set -e"?
Because "set -e" comes with all kinds of confusing corner cases. Using
&& chains is annoying, but rarely surprising.
One of my favorite examples is:
set -e
(
false
echo 1
) || {
echo outcome=$?
false
}
echo 2
which prints both "1" and "2".
Inside the subshell, "set -e" has no effect, and you cannot re-enable it
by setting "-e" (it's suppressed entirely because we are on the
left-hand side of an || conditional).
So you could write a function like this:
foo() {
do_one
do_two
}
that relies on catching the failure from do_one. And it works here:
set -e
foo
but not here:
set -e
if foo then
do_something
fi
And there's no way to make it work without adding back in the
&&-chaining.
-Peff
Re: [PATCH] real_path: make real_path thread-safe
Am 08.12.2016 um 08:55 schrieb Torsten Bögershausen:
Some conversion may be done in mingw.c:
https://github.com/github/git-msysgit/blob/master/compat/mingw.c
So what I understand, '/' in Git are already converted into '\' if needed ?
Only if needed, and there are not many places where this is the case.
(Actually, I can't find one place where we do.) In particular, typical
file accesses does not require the conversion.
It seams that we may wnat a function get_start_of_path(uncpath),
which returns:
get_start_of_path_win("//?/D:/very-long-path") "/very-long-path"
We have offset_1st_component().
-- Hannes
Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
On 12/08, Duy Nguyen wrote: > On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams wrote: > > On 12/07, Duy Nguyen wrote: > >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams wrote: > >> > Convert 'create_simplify()' to use the pathspec struct interface from > >> > using the '_raw' entry in the pathspec. > >> > >> It would be even better to kill this create_simplify() and let > >> simplify_away() handle struct pathspec directly. > >> > >> There is a bug in this code, that might have been found if we > >> simpify_away() handled pathspec directly: the memcmp() in > >> simplify_away() will not play well with :(icase) magic. My bad. If > >> :(icase) is used, the easiest/safe way is simplify nothing. Later on > >> maybe we can teach simplify_away() to do strncasecmp instead. We could > >> ignore exclude patterns there too (although not excluding is not a > >> bug). > > > > So are you implying that the simplify struct needs to be killed? That > > way the pathspec struct itself is being passed around instead? > > Yes. simplify struct was a thing when pathspec was an array of char *. > At this point I think it can retire (when we have time to retire it) Alright, then for now I can leave this change as is and have a follow up series that kills the simplify struct. -- Brandon Williams
Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
Johannes Schindelin writes:
> Sorry for the breakage.
Apologies from me, too. Once a topic is merged, the credit still
remains with the contributor, but the blame is shared by the project
as a whole, with those who missed breakages during their reviews,
and those who didn't review or test and let breakages pass.
> When libifying the code, I tried to be careful to retain the error
> messages when not dying,...
Quite honestly, I do not think either of us cared about preserving
the exact error message the end-user was getting from each failure
sites that the series changed a call with die-on-error=1 to a call
with die-on-error=0 that is followed by a negative return while
reviewing this series. As I wrote in the proposed log message for
3/3, this one was noticed as a end-user breaking change because it
was the only one that has become totally silent. For example, this
bit from sequencer.c::write_message() we can see in the output from
"git show --first-parent 2a4062a4a8" does not preserve the message
at all:
diff --git a/sequencer.c b/sequencer.c
index 3804fa931d..eec8a60d6b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -180,17 +180,20 @@
...
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
{
static struct lock_file msg_file;
- int msg_fd = hold_lock_file_for_update(&msg_file, filename,
- LOCK_DIE_ON_ERROR);
+ int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
+ if (msg_fd < 0)
+ return error_errno(_("Could not lock '%s'"), filename);
And I do not think it is necessarily bad that the error message
changed with this conversion. In other words, I do not think it
should have been the goal to preserve the exact error message.
hold_lock*() can afford to give a detailed message that strongly
sounds as being the final decision when called with die-on-error=1
because it knows it is dying. However, the message from the updated
write_message(), "could not lock", cannot be final---the caller may
want to add something else after it to describe what failed in a
larger picture.
Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
On Thu, Dec 8, 2016 at 10:04 AM, vi0oss wrote: > On 12/08/2016 08:46 PM, Jeff King wrote: >> >> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote: >> >>> On Wed, Dec 7, 2016 at 4:39 PM, wrote: >>> Previously test contained errorneous test_must_fail, which was masked by missing &&. >>> >>> I wonder if we could make either >>> the test_must_fail intelligent to detect such a broken && call chain >>> or the test_expect_success macro to see for those broken chains. >>> >>> >>> I wish we could improve that, but I spend a lot of brain cycles on it at >>> one point and couldn't come up with a workable solution. >>> >>> -Peff >>> > Why Git test use &&-chains instead of proper "set -e"? > "Because set -e kills the shell and we would want to keep going until the test suite is finished and display a summary what failed" would be my first reaction, but let's dig into history: bb79af9d09 might be helpful on that, but it doesn't explain why we use && chains. I could not find any commit explaining the use of && chains. e1970ce43abf might be interesting (the introduction of the test suite), as that did not contain && chains. I guess it would be hard(er) to implement e.g. test_must_fail in an environment where -e is set.
Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
Thanks all for taking a look at this, I didn't expect such a quick response :). No hard feelings re: breakage; I know how gnarly these sorts of refactors can be (and more libification is always better :)). Robbie On Thu, Dec 8, 2016 at 3:53 AM, Johannes Schindelin wrote: > Hi Junio, > > On Wed, 7 Dec 2016, Junio C Hamano wrote: > >> The "libify sequencer" topic stopped passing the die_on_error option >> to hold_locked_index(), and this lost an error message from "git >> merge --ff-only $commit" when there are competing updates in >> progress. > > Sorry for the breakage. > > When libifying the code, I tried to be careful to retain the error > messages when not dying, and mistakenly assumed that hold_locked_index() > would do the same. > > Ciao, > Dscho
Re: [PATCH 01/17] mv: convert to using pathspec struct interface
On 12/08, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams wrote:
> >> > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const
> >> > char *prefix,
> >> > {
> >> > int i;
> >> > const char **result;
> >> > + struct pathspec ps;
> >> > ALLOC_ARRAY(result, count + 1);
> >> > - COPY_ARRAY(result, pathspec, count);
> >> > - result[count] = NULL;
> >> > +
> >> > + /* Create an intermediate copy of the pathspec based on the
> >> > flags */
> >> > for (i = 0; i < count; i++) {
> >> > - int length = strlen(result[i]);
> >> > + int length = strlen(pathspec[i]);
> >> > int to_copy = length;
> >> > + char *it;
> >> > while (!(flags & KEEP_TRAILING_SLASH) &&
> >> > - to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
> >> > + to_copy > 0 && is_dir_sep(pathspec[i][to_copy -
> >> > 1]))
> >> > to_copy--;
> >> > - if (to_copy != length || flags & DUP_BASENAME) {
> >> > - char *it = xmemdupz(result[i], to_copy);
> >> > - if (flags & DUP_BASENAME) {
> >> > - result[i] = xstrdup(basename(it));
> >> > - free(it);
> >> > - } else
> >> > - result[i] = it;
> >> > - }
> >> > +
> >> > + it = xmemdupz(pathspec[i], to_copy);
> >> > + if (flags & DUP_BASENAME) {
> >> > + result[i] = xstrdup(basename(it));
> >> > + free(it);
> >> > + } else
> >> > + result[i] = it;
> >> > + }
> >> > + result[count] = NULL;
> >> > +
> >> > + parse_pathspec(&ps,
> >> > + PATHSPEC_ALL_MAGIC &
> >> > + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> >> > + PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
> >> > + prefix, result);
> >> > + assert(count == ps.nr);
> >> > +
> >> > + /* Copy the pathspec and free the old intermediate strings */
> >> > + for (i = 0; i < count; i++) {
> >> > + const char *match = xstrdup(ps.items[i].match);
> >> > + free((char *) result[i]);
> >> > + result[i] = match;
> >>
> >> Sigh.. it looks so weird that we do all the parsing (in a _copy_
> >> pathspec function) then remove struct pathspec and return the plain
> >> string. I guess we can't do anything more until we rework cmd_mv code
> >> to handle pathspec natively.
> >>
> >> At the least I think we should rename this function to something else.
> >> But if you have time I really wish we could kill this function. I
> >> haven't stared at cmd_mv() long and hard, but it looks to me that we
> >> combining two separate functionalities in the same function here.
> >>
> >> If "mv" takes n arguments, then the first arguments may be
> >> pathspec, the last one is always a plain path. The "dest_path =
> >> internal_copy_pathspec..." could be as simple as "dest_path =
> >> prefix_path(argv[argc - 1])". the special treatment for this last
> >> argument [1] can live here. Then, we can do parse_pathspec for the
> >> arguments in cmd_mv(). It's still far from perfect, because
> >> cmd_mv can't handle pathspec properly, but it reduces the messy mess
> >> in internal_copy_pathspec a bit, I hope.
> >>
> >
> > Actually, after looking at this a bit more it seems like we could
> > technically use prefix_path for both source and dest (based on how the
> > current code is structured) since the source's provied must all exist (as
> > in no wildcards are allowed). We could drop using the pathspec struct
> > completely in addition to renaming the function (to what I'm still
> > unsure).
>
> Yeah that sounds good too (with a caveat: I'm not a heavy user of
> git-mv nor touching this code a lot, I might be missing something).
> It'll take some looong time before somebody starts converting it to
> use pathspec properly, I guess. prefix_path() would keep the code
> clean meanwhile.
K for now I'll switch to using prefix_path() and rename the function
`internal_prefix_pathspec()` as that is a bit more descriptive.
--
Brandon Williams
Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
On 12/08/2016 08:46 PM, Jeff King wrote: On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote: On Wed, Dec 7, 2016 at 4:39 PM, wrote: Previously test contained errorneous test_must_fail, which was masked by missing &&. I wonder if we could make either the test_must_fail intelligent to detect such a broken && call chain or the test_expect_success macro to see for those broken chains. I wish we could improve that, but I spend a lot of brain cycles on it at one point and couldn't come up with a workable solution. -Peff Why Git test use &&-chains instead of proper "set -e"?
Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
On Thu, Dec 8, 2016 at 9:46 AM, Jeff King wrote: > > will both trigger on the &&-chain linter, because it uses a magic exit > code to detect the breakage. I think the problem is just that the > &&-chain linter cannot peek inside subshells, and that's where the bug > was in this case. Uh, yeah in the subshell, but the patch v2 did have it not in subshells, I'll take another look. > > I wish we could improve that, but I spend a lot of brain cycles on it at > one point and couldn't come up with a workable solution. Is it possible to overwrite what happens when you open a subshell with ( ) ? i.e. I imagine in the global test-setup we'd setup that ( ) not just opens /bin/sh but a shell with benefits such that we can execute just one && chain.
Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
Paul Tan writes: > Hmm, to add on, looking at the three other call sites of this > function, two of them (builtin/commit.c and builtin/describe.c) > basically do: > > if (0 <= fd) > update_index_if_able(...) > > with that 0 <= fd conditional. With this patch it becomes three out of > four. The other one is diff.c::refresh_index_quietly() that you are not counting, I think, but if you look at it again, it also is not called after hold_locked_index() fails to acquire the lock, so with this fix everybody refrains from calling it when it does not hold the lock. > Perhaps the repeated use of this conditional is a sign that the > 0 <= fd check could be built into update_index_if_able()? That condition is "do we have the lock? Otherwise we are not even allowed to update it", so in that sense it may make sense. > I think there is precedent for building in these kind of checks -- > rollback_lock_file() also does not fail if the lock file was not > successfully opened. > > That said, the number of call sites is quite low so it's probably not > worth doing this. Yeah, I can go either way. At least with the change things are consistent.
Re: [PATCH] real_path: make real_path thread-safe
On 12/08, Duy Nguyen wrote:
> On Tue, Dec 6, 2016 at 3:16 AM, Brandon Williams wrote:
> > On 12/05, Stefan Beller wrote:
> >> > 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;
> >>
> >> Also by having this static here, it is not quite thread safe, yet.
> >>
> >> By removing the static here we cannot do the early cheap check as:
> >>
> >> > /* We've already done it */
> >> > - if (path == sb.buf)
> >> > + if (path == resolved.buf)
> >> > return path;
> >>
> >> I wonder how often we run into this case; are there some callers explicitly
> >> relying on real_path_internal being cheap for repeated calls?
> >> (Maybe run the test suite with this early return instrumented? Not sure how
> >> to assess the impact of removing the cheap out return optimization)
> >>
> >> The long tail (i.e. the actual functionality) should actually be
> >> faster, I'd imagine
> >> as we do less than with using chdir.
> >
> > Depends on how expensive the chdir calls were. And I'm working to get
> > rid of the static buffer. Just need have the callers own the memory
> > first.
>
> I suggest you turn this real_path_internal into strbuf_real_path. In
> other words, it takes a strbuf and writes the result there, allocating
> memory if needed.
>
> This function can replace the two strbuf_addstr(..., real_path(..));
> we have in setup.c and sha1_file.c. real_path() can own a static
> strbuf buffer to retain current behavior. We could also have a new
> wrapper real_pathdup() around strbuf_real_path(), which can replace 9
> instances of xstrdup(real_path(...)) (and Stefan is adding a few more;
> that's what led me back to these mails)
Sounds like a plan, thanks for the advice.
--
Brandon Williams
Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote: > On Wed, Dec 7, 2016 at 4:39 PM, wrote: > > > > > Previously test contained errorneous > > test_must_fail, which was masked by > > missing &&. > > I wonder if we could make either > the test_must_fail intelligent to detect such a broken && call chain > or the test_expect_success macro to see for those broken chains. I don't think test_must_fail is relevant for &&-chains. Even something like: test_must_fail foo bar or: bar test_must_fail foo will both trigger on the &&-chain linter, because it uses a magic exit code to detect the breakage. I think the problem is just that the &&-chain linter cannot peek inside subshells, and that's where the bug was in this case. I wish we could improve that, but I spend a lot of brain cycles on it at one point and couldn't come up with a workable solution. -Peff
Re: [PATCH 4/5] Make sequencer abort safer
Johannes Schindelin writes:
> On Wed, 7 Dec 2016, Stephan Beyer wrote:
>
>> diff --git a/sequencer.c b/sequencer.c
>> index 30b10ba14..c9b560ac1 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
>> static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
>> static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
>> static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
>
> Is it required by law to have a four-letter infix, or can we have a nicer
> variable name (e.g. git_path_current_file)?
I agree with you that, as other git_path_*_file variables match the
actual name on the filesystem, this one should too, together with
the update_curr_file() function.
By the way, this step seems to be a fix to an existing problem, and
the new test added in 3/5 seems to be a demonstration of the issue.
If that is the case, shouldn't the new test initially expect failure
and updated by this step to expect success?
I'll queue this on top of step 4/5 as "SQUASH???" as usual. The
other SQUASH??? that must come after 3/5 for t3510 should be trivial
(the reverse of what appears here).
Thanks.
sequencer.c | 22 +++---
t/t3510-cherry-pick-sequence.sh | 2 +-
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index c9b560ac15..ce04377f8e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
-static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
+static GIT_PATH_FUNC(git_path_current_file, "sequencer/current")
/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -311,7 +311,7 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
}
-static void update_curr_file()
+static void update_current_file(void)
{
struct object_id head;
@@ -320,9 +320,9 @@ static void update_curr_file()
return;
if (!get_oid("HEAD", &head))
- write_file(git_path_curr_file(), "%s", oid_to_hex(&head));
+ write_file(git_path_current_file(), "%s", oid_to_hex(&head));
else
- write_file(git_path_curr_file(), "%s", "");
+ write_file(git_path_current_file(), "%s", "");
}
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -354,7 +354,7 @@ static int fast_forward_to(const unsigned char *to, const
unsigned char *from,
strbuf_release(&sb);
strbuf_release(&err);
ref_transaction_free(transaction);
- update_curr_file();
+ update_current_file();
return 0;
}
@@ -829,7 +829,7 @@ static int do_pick_commit(enum todo_command command, struct
commit *commit,
leave:
free_message(commit, &msg);
- update_curr_file();
+ update_current_file();
return res;
}
@@ -1149,23 +1149,23 @@ static int save_head(const char *head)
return 0;
}
-static int rollback_is_safe()
+static int rollback_is_safe(void)
{
struct strbuf sb = STRBUF_INIT;
struct object_id expected_head, actual_head;
- if (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) {
+ if (strbuf_read_file(&sb, git_path_current_file(), 0) >= 0) {
strbuf_trim(&sb);
if (get_oid_hex(sb.buf, &expected_head)) {
strbuf_release(&sb);
- die(_("could not parse %s"), git_path_curr_file());
+ die(_("could not parse %s"), git_path_current_file());
}
strbuf_release(&sb);
}
else if (errno == ENOENT)
oidclr(&expected_head);
else
- die_errno(_("could not read '%s'"), git_path_curr_file());
+ die_errno(_("could not read '%s'"), git_path_current_file());
if (get_oid("HEAD", &actual_head))
oidclr(&actual_head);
@@ -1441,7 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
- update_curr_file();
+ update_current_file();
res = pick_commits(&todo_list, opts);
todo_list_release(&todo_list);
return res;
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index efcd4fc485..372307c21b 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
git diff-index --exit-code HEAD
'
-test_expect_failure '--abort does not unsafely change HEAD' '
+test_expect_success '--abort does not unsafely change HEAD' '
pri
Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
On Thu, Dec 08, 2016 at 04:35:56PM +0100, Johannes Schindelin wrote: > The idea here is to discover the .git/ directory gently (i.e. without > changing the current working directory), and to use it to read the > .git/config file early, before we actually called setup_git_directory() > (if we ever do that). Great. Thanks for taking a stab at this. > Notes: > > - I find the diff pretty ugly: I wish there was a more elegant way to > *disable* discovery of .git/ *just* for `init` and `clone`. I > considered a function `about_to_create_git_dir()` that is called in a > hard-coded manner *only* for `init` and `clone`, but that would > introduce another magic side effect, when all I want is to reduce those. It looks like a lot of that ugliness comes from passing around the "are we OK to peek at git-dir config" flag through the various pager-related calls. I don't think it would be bad to use a global for "we do not want a repo". After all, it's just modifying the _existing_ global for "are we in a repo". And I do not see that global going away anytime soon. Sometimes it's good to make incremental steps towards an end goal, but in this case it seems like we just pay the cost of the step without any real plan for reaping the benefit in the long term. As an alternative, I also think it would be OK to just always have the pager config read from local repo, even for init/clone. In other words, to loosen the idea that git-init can _never_ look in the current git-dir, and declare that there is a stage before the command is initiated, and during which git may read local-repo config. Aliases would fall into this, too, so: git config --local alias.foo init git foo /some/other/dir would work (as it must, because we cannot know that "foo" is "init" until we read the config!). I have a feeling you may declare that too magical, but I think it's consistent and practical. > - For the moment, I do not handle dashed invocations of `init` and > `clone` correctly. The real problem is the continued existence of > the dashed git-init and git-clone, of course. I assume you mean setting the CREATES_GIT_DIR flag here? I think it would apply to the dashed invocations, too. They strip off the "git-" and end up in handle_builtin() just like "git clone" does. I didn't test it, though. > - There is still duplicated code. For the sake of this RFC, I did not > address that yet. Yeah, I did not read your discover function very carefully. Because I think the one thing we really don't want to do here is introduce a separate lookup process that is not shared by setup_git_directory(). The only sane path, I think, is to refactor setup_git_directory() to build on top of discover_git_directory(), which implies that the latter handles all of the cases. > - The read_early_config() function is called multiple times, re-reading > all the config files and re-discovering the .git/ directory multiple > times, which is quite wasteful. For the sake of this RFC, I did not > address that yet. We already have a config-caching system. If we went with a global "config_discover_refs", then I think the sequence for something like git-init would become: 1. When git.c starts, config_discover_refs is set to "true". Pager and alias lookup may look in .git/config if it's available, even if they go through the configset cache. 2. As soon as git-init starts, it disables config_discover_refs, and it flushes the config cache. Any configset lookups will now examine the reduced config. 3. When git-init has set up the real repo it is operating on, it can reenable config_discover_refs (though it may not even need to; that flag probably wouldn't have any effect once we've entered the repository and have_git_dir() returns true). > - t7006 fails and the error message is a bit cryptic (not to mention the > involved function trace, which is mind-boggling for what is supposed > to be a simply, shell script-based test suite). I did not have time to > look into that yet. Running t7006 I see a lot of old failures turned into successes, which is good (because running from a subdirectory now actually respects local pager config). The one failure looks like it is testing the wrong thing. It is checking that we _don't_ respect a local core.pager in some cases, as a proxy for whether or not we are breaking things by doing setup too early. But the whole point of your series is to fix that, and respect the config without causing the setup breakage. After your patches, the proxy behavior and the failure case are disconnected. The test should be flipped, and ideally another one added that confirms we didn't actually run setup_git_directory(), but I'm not sure how to test that directly. > - after discover_git_directory_gently() did its work, the code happily > uses its result *only* for the current read_early_config() run, and > lets setup_git_dir_gently() do the whole work *again*. For the sake of >
Re: [PATCH] real_path: make real_path thread-safe
Johannes Sixt writes: > Am 07.12.2016 um 23:29 schrieb Brandon Williams: >> Instead of assuming root is "/" >> I'll need to extract what root is from an absolute path. Aside from >> what root looks like, do most other path constructs behave similarly in >> unix and windows? (like ".." and "." as examples) > > Yes, .. and . work the same way, except that they cannot appear in the > \\server\share part. I also think that .. does not cancel these parts. > > As long as you use is_absolute_path() and do not simplify path > components before offset_1st_component(), you should be on the safe > side. > >> Since I don't really have a windows machine to test things it might be >> slightly difficult to get everything correct quickly but hopefully we can >> get this working :) > > I'll lend a hand, of course, as time permits. Thanks, as always, for working well together ;-).
Re: [PATCH v2] commit: make --only --allow-empty work without paths
Andreas Krey writes: > Ok, I've removed the clever message, as Junio suggested. > I don't know what else to do to make it acceptable. :-) > We're going to deploy it internally anyway, but I think > it belongs in git.git as well (aka 'Can I has "will queue"?'). Oh, sorry for being unclear. Before I started saying "Slightly related topic.", after quoting "The patch itself looks good to me." by Peff, I meant to say "Yeah, this looks good; thanks.", but apparently I forgot. Removal of "Clever" is a separate issue and it may make sense to do so, but it deserves its own commit with its own justification. 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. Thanks.
[PATCH/RFC 7/7] WIP: read_early_config(): add tests
Signed-off-by: Johannes Schindelin
---
t/helper/test-config.c | 15 +++
t/t1309-early-config.sh | 50 +
2 files changed, 65 insertions(+)
create mode 100755 t/t1309-early-config.sh
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 83a4f2ab86..5105069587 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -66,6 +66,16 @@ static int iterate_cb(const char *var, const char *value,
void *data)
return 0;
}
+static int early_config_cb(const char *var, const char *value, void *vdata)
+{
+ const char *key = vdata;
+
+ if (!strcmp(key, var))
+ printf("%s\n", value);
+
+ return 0;
+}
+
int cmd_main(int argc, const char **argv)
{
int i, val;
@@ -73,6 +83,11 @@ int cmd_main(int argc, const char **argv)
const struct string_list *strptr;
struct config_set cs;
+ if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
+ read_early_config(early_config_cb, (void *)argv[2], 1);
+ return 0;
+ }
+
setup_git_directory();
git_configset_init(&cs);
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
new file mode 100755
index 00..0c55dee514
--- /dev/null
+++ b/t/t1309-early-config.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='Test read_early_config()'
+
+. ./test-lib.sh
+
+test_expect_success 'read early config' '
+ test_config early.config correct &&
+ test-config read_early_config early.config >output &&
+ test correct = "$(cat output)"
+'
+
+test_expect_success 'in a sub-directory' '
+ test_config early.config sub &&
+ mkdir -p sub &&
+ (
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test sub = "$(cat output)"
+'
+
+test_expect_success 'ceiling' '
+ test_config early.config ceiling &&
+ mkdir -p sub &&
+ (
+ GIT_CEILING_DIRECTORIES="$PWD" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test -z "$(cat output)"
+'
+
+test_expect_success 'ceiling #2' '
+ mkdir -p xdg/git &&
+ git config -f xdg/git/config early.config xdg &&
+ test_config early.config ceiling &&
+ mkdir -p sub &&
+ (
+ XDG_CONFIG_HOME="$PWD"/xdg &&
+ GIT_CEILING_DIRECTORIES="$PWD" &&
+ export GIT_CEILING_DIRECTORIES XDG_CONFIG_HOME &&
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test xdg = "$(cat output)"
+'
+
+test_done
--
2.11.0.rc3.windows.1
Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
On 8 December 2016 at 14:18, Lars Schneider wrote: > >> On 08 Dec 2016, at 15:00, Nick Warr wrote: >> >> That looks pretty much like the error we're dealing with, any reason >> why going back a point version on Git (not git-lfs) would resolve the >> issue however? > > Going back to GitLFS 1.4.* would make the error disappear. However, I think > you should fix your repository. Try this: > > git lfs clone > cd > git rm --cached "workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi > Effects/Effects/Debris/Meshes/debris_junk.FBX" > git add --force "workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi > Effects/Effects/Debris/Meshes/debris_junk.FBX" > git commit -m "Move files properly to GitLFS" > git push > > Afterwards you should be able to use the latest version of Git and GitLFS > without trouble. > > Cheers, > Lars > > PS: Top posting is not that popular in the Git community ;-) > > >> >> On 8 December 2016 at 13:57, Lars Schneider wrote: >>> On 08 Dec 2016, at 12:46, Nick Warr wrote: Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS. When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the first time the clone will finish the checkout of the git part, then when it starts downloading the LFS parts it will reliably finish with a smudge filter error. This leaves the repo in an unstable condition, you can then fetch the lfs part without issue, but checking out the lfs files or trying a git reset --hard will continue to spit out the same error. As you can see, the actual error is not particularly useful. fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs failed Unknown command "" Possibly it's due to the file extension being all capital letters, we did manage to change the error by recommitting the file with a lowercase extension, but it failed on the next file (which also had a capital letter extension). This has happened on multiple fresh windows 10 64 bit installs, different machines and target directories (to hopefully remove the possibility of file permissions) where cloning is taking place. The solution is to back level to Git 2.10.2 and the error disappears. More than willing to provide any further information, >>> >>> Hi Nick, >>> >>> debris_junk.FBX is not stored properly in Git LFS. >>> I explained the problem in detail here: >>> https://github.com/git-lfs/git-lfs/issues/1729 >>> >>> You should add the file properly to GitLFS to fix the problem. >>> However, I think this is a regression in GitLFS and I hope it will >>> be fixed in the next version. >>> >>> No change/fix in Git is required. >>> >>> Cheers, >>> Lars > Thank gmail for the top posting, hard enough getting it to send non html mail :( Just to add a bit more to the discussion, we went and had a look at that directory which was freshly cloned with 2.10.2 and 1.5.3 and the files were there (400 KB + FBX files, not 2KB pointers) after the git reset --hard, we're actually looking at renaming all those files, as that isn't the only one unfortunately..
[PATCH/RFC 6/7] WIP read_config_early(): respect ceiling directories
This ports the part from setup_git_directory_gently_1() where the
GIT_CEILING_DIRECTORIES environment variable is handled.
TODO: DRY up code again (exporting canonicalize_ceiling_directories()?)
Signed-off-by: Johannes Schindelin
---
config.c | 56 +---
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/config.c b/config.c
index 286d3cad66..eda3546491 100644
--- a/config.c
+++ b/config.c
@@ -1386,6 +1386,37 @@ static void configset_iter(struct config_set *cs,
config_fn_t fn, void *data)
}
/*
+ * A "string_list_each_func_t" function that canonicalizes an entry
+ * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
+ * discards it if unusable. The presence of an empty entry in
+ * GIT_CEILING_DIRECTORIES turns off canonicalization for all
+ * subsequent entries.
+ */
+static int canonicalize_ceiling_entry(struct string_list_item *item,
+ void *cb_data)
+{
+ int *empty_entry_found = cb_data;
+ char *ceil = item->string;
+
+ if (!*ceil) {
+ *empty_entry_found = 1;
+ return 0;
+ } else if (!is_absolute_path(ceil)) {
+ return 0;
+ } else if (*empty_entry_found) {
+ /* Keep entry but do not canonicalize it */
+ return 1;
+ } else {
+ const char *real_path = real_path_if_valid(ceil);
+ if (!real_path)
+ return 0;
+ free(item->string);
+ item->string = xstrdup(real_path);
+ return 1;
+ }
+}
+
+/*
* Note that this is a really dirty hack that replicates what the
* setup_git_directory() function does, without changing the current
* working directory. The crux of the problem is that we cannot run
@@ -1394,6 +1425,8 @@ static void configset_iter(struct config_set *cs,
config_fn_t fn, void *data)
*/
static int discover_git_directory_gently(struct strbuf *result)
{
+ const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
+ int ceiling_offset = -1;
const char *p;
if (strbuf_getcwd(result) < 0)
@@ -1403,6 +1436,23 @@ static int discover_git_directory_gently(struct strbuf
*result)
return -1;
strbuf_reset(result);
strbuf_addstr(result, p);
+
+ if (env_ceiling_dirs) {
+ struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
+ int empty_entry_found = 0;
+
+ string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP,
+ -1);
+ filter_string_list(&ceiling_dirs, 0, canonicalize_ceiling_entry,
+ &empty_entry_found);
+ ceiling_offset = longest_ancestor_length(result->buf,
+&ceiling_dirs);
+ string_list_clear(&ceiling_dirs, 0);
+ }
+
+ if (ceiling_offset < 0 && has_dos_drive_prefix(result->buf))
+ ceiling_offset = 1;
+
for (;;) {
int len = result->len, i;
@@ -1418,10 +1468,10 @@ static int discover_git_directory_gently(struct strbuf
*result)
strbuf_setlen(result, len);
if (is_git_directory(result->buf))
return 0;
- for (i = len; i > 0; )
- if (is_dir_sep(result->buf[--i]))
+ for (i = len; --i > ceiling_offset; )
+ if (is_dir_sep(result->buf[i]))
break;
- if (!i)
+ if (i <= ceiling_offset)
return -1;
strbuf_setlen(result, i);
}
--
2.11.0.rc3.windows.1
[PATCH/RFC 2/7] read_early_config(): avoid .git/config hack when unneeded
So far, we only look whether the startup_info claims to have seen a
git_dir.
However, do_git_config_sequence() (and consequently the
git_config_with_options() call used by read_early_config() asks the
have_git_dir() function whether we have a .git/ directory, which in turn
also looks at git_dir and at the environment variable GIT_DIR.
Let's just use the same function, have_git_dir(), to determine whether we
have to look for .git/config specifically.
Signed-off-by: Johannes Schindelin
---
config.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 7dcd8d8622..104c3c3dcd 100644
--- a/config.c
+++ b/config.c
@@ -1407,7 +1407,7 @@ void read_early_config(config_fn_t cb, void *data)
* valid repository), and would rarely make things worse (i.e., you do
* not generally have a .git/config file sitting around).
*/
- if (!startup_info->have_repository) {
+ if (!have_git_dir()) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
--
2.11.0.rc3.windows.1
[PATCH/RFC 4/7] read_early_config(): special-case `init` and `clone`
The `init` and `clone` commands create their own .git/ directory,
therefore we must be careful not to read any repository config when
determining the pager settings.
Signed-off-by: Johannes Schindelin
---
builtin/am.c| 2 +-
builtin/blame.c | 2 +-
builtin/grep.c | 4 ++--
builtin/log.c | 4 ++--
builtin/var.c | 2 +-
cache.h | 9 +
config.c| 4 ++--
diff.c | 4 ++--
git.c | 16
pager.c | 13 +++--
10 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce9..e6c2ee01bc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1791,7 +1791,7 @@ static int do_interactive(struct am_state *state)
}
strbuf_release(&msg);
} else if (*reply == 'v' || *reply == 'V') {
- const char *pager = git_pager(1);
+ const char *pager = git_pager(1, 1);
struct child_process cp = CHILD_PROCESS_INIT;
if (!pager)
diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..5b7daa3686 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2913,7 +2913,7 @@ int cmd_blame(int argc, const char **argv, const char
*prefix)
assign_blame(&sb, opt);
if (!incremental)
- setup_pager();
+ setup_pager(1);
free(final_commit_name);
diff --git a/builtin/grep.c b/builtin/grep.c
index 8887b6addb..363a753369 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -800,7 +800,7 @@ int cmd_grep(int argc, const char **argv, const char
*prefix)
}
if (show_in_pager == default_pager)
- show_in_pager = git_pager(1);
+ show_in_pager = git_pager(1, 1);
if (show_in_pager) {
opt.color = 0;
opt.name_only = 1;
@@ -896,7 +896,7 @@ int cmd_grep(int argc, const char **argv, const char
*prefix)
}
if (!show_in_pager && !opt.status_only)
- setup_pager();
+ setup_pager(1);
if (!use_index && (untracked || cached))
die(_("--cached or --untracked cannot be used with
--no-index."));
diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc2d8..96618d38cb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const char
**argv, const char *prefix,
if (rev->line_level_traverse)
line_log_init(rev, line_cb.prefix, &line_cb.args);
- setup_pager();
+ setup_pager(1);
}
static void cmd_log_init(int argc, const char **argv, const char *prefix,
@@ -1600,7 +1600,7 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
if (!use_stdout)
output_directory = set_outdir(prefix, output_directory);
else
- setup_pager();
+ setup_pager(1);
if (output_directory) {
if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53a2d..879867b842 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -19,7 +19,7 @@ static const char *editor(int flag)
static const char *pager(int flag)
{
- const char *pgm = git_pager(1);
+ const char *pgm = git_pager(1, 1);
if (!pgm)
pgm = "cat";
diff --git a/cache.h b/cache.h
index 96e0cb2523..6627239de8 100644
--- a/cache.h
+++ b/cache.h
@@ -1325,7 +1325,7 @@ extern const char *fmt_name(const char *name, const char
*email);
extern const char *ident_default_name(void);
extern const char *ident_default_email(void);
extern const char *git_editor(void);
-extern const char *git_pager(int stdout_is_tty);
+extern const char *git_pager(int stdout_is_tty, int discover_git_dir);
extern int git_ident_config(const char *, const char *, void *);
extern void reset_ident_date(void);
@@ -1692,7 +1692,8 @@ extern int git_config_from_mem(config_fn_t fn, const enum
config_origin_type,
const char *name, const char *buf,
size_t len, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern void read_early_config(config_fn_t cb, void *data);
+extern void read_early_config(config_fn_t cb, void *data,
+ int discover_git_dir);
extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
@@ -1888,12 +1889,12 @@ __attribute__((format (printf, 2, 3)))
extern void write_file(const char *path, const char *fmt, ...);
/* pager.c */
-extern void setup_pager(void);
+extern void setup_pager(int discover_git_dir);
extern int pager_in_use(void);
extern int pager_use_color;
extern int term_columns(void);
extern int decimal_width
[PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
Hopefully these patches will lead to something that we can integrate, and that eventually will make Git's startup sequence much less surprising. The idea here is to discover the .git/ directory gently (i.e. without changing the current working directory), and to use it to read the .git/config file early, before we actually called setup_git_directory() (if we ever do that). Notes: - I find the diff pretty ugly: I wish there was a more elegant way to *disable* discovery of .git/ *just* for `init` and `clone`. I considered a function `about_to_create_git_dir()` that is called in a hard-coded manner *only* for `init` and `clone`, but that would introduce another magic side effect, when all I want is to reduce those. - For the moment, I do not handle dashed invocations of `init` and `clone` correctly. The real problem is the continued existence of the dashed git-init and git-clone, of course. - There is still duplicated code. For the sake of this RFC, I did not address that yet. - The read_early_config() function is called multiple times, re-reading all the config files and re-discovering the .git/ directory multiple times, which is quite wasteful. For the sake of this RFC, I did not address that yet. - t7006 fails and the error message is a bit cryptic (not to mention the involved function trace, which is mind-boggling for what is supposed to be a simply, shell script-based test suite). I did not have time to look into that yet. - after discover_git_directory_gently() did its work, the code happily uses its result *only* for the current read_early_config() run, and lets setup_git_dir_gently() do the whole work *again*. For the sake of this RFC, I did not address that yet. Johannes Schindelin (7): Make read_early_config() reusable read_early_config(): avoid .git/config hack when unneeded Mark builtins that create .git/ directories read_early_config(): special-case `init` and `clone` read_early_config(): really discover .git/ WIP read_config_early(): respect ceiling directories WIP: read_early_config(): add tests builtin/am.c| 2 +- builtin/blame.c | 2 +- builtin/grep.c | 4 +- builtin/log.c | 4 +- builtin/var.c | 2 +- cache.h | 8 ++-- config.c| 110 diff.c | 4 +- git.c | 25 +-- pager.c | 44 +++ t/helper/test-config.c | 15 +++ t/t1309-early-config.sh | 50 ++ 12 files changed, 209 insertions(+), 61 deletions(-) create mode 100755 t/t1309-early-config.sh base-commit: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2 Published-As: https://github.com/dscho/git/releases/tag/early-config-v1 Fetch-It-Via: git fetch https://github.com/dscho/git early-config-v1 -- 2.11.0.rc3.windows.1
[PATCH/RFC 1/7] Make read_early_config() reusable
The pager configuration needs to be read early, possibly before
discovering any .git/ directory.
Let's not hide this function in pager.c, but make it available to other
callers.
Signed-off-by: Johannes Schindelin
---
cache.h | 1 +
config.c | 31 +++
pager.c | 31 ---
3 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/cache.h b/cache.h
index a50a61a197..96e0cb2523 100644
--- a/cache.h
+++ b/cache.h
@@ -1692,6 +1692,7 @@ extern int git_config_from_mem(config_fn_t fn, const enum
config_origin_type,
const char *name, const char *buf,
size_t len, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
+extern void read_early_config(config_fn_t cb, void *data);
extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
diff --git a/config.c b/config.c
index 83fdecb1bc..7dcd8d8622 100644
--- a/config.c
+++ b/config.c
@@ -1385,6 +1385,37 @@ static void configset_iter(struct config_set *cs,
config_fn_t fn, void *data)
}
}
+void read_early_config(config_fn_t cb, void *data)
+{
+ git_config_with_options(cb, data, NULL, 1);
+
+ /*
+* Note that this is a really dirty hack that does the wrong thing in
+* many cases. The crux of the problem is that we cannot run
+* setup_git_directory() early on in git's setup, so we have no idea if
+* we are in a repository or not, and therefore are not sure whether
+* and how to read repository-local config.
+*
+* So if we _aren't_ in a repository (or we are but we would reject its
+* core.repositoryformatversion), we'll read whatever is in .git/config
+* blindly. Similarly, if we _are_ in a repository, but not at the
+* root, we'll fail to find .git/config (because it's really
+* ../.git/config, etc). See t7006 for a complete set of failures.
+*
+* However, we have historically provided this hack because it does
+* work some of the time (namely when you are at the top-level of a
+* valid repository), and would rarely make things worse (i.e., you do
+* not generally have a .git/config file sitting around).
+*/
+ if (!startup_info->have_repository) {
+ struct git_config_source repo_config;
+
+ memset(&repo_config, 0, sizeof(repo_config));
+ repo_config.file = ".git/config";
+ git_config_with_options(cb, data, &repo_config, 1);
+ }
+}
+
static void git_config_check_init(void);
void git_config(config_fn_t fn, void *data)
diff --git a/pager.c b/pager.c
index ae79643363..73ca8bc3b1 100644
--- a/pager.c
+++ b/pager.c
@@ -43,37 +43,6 @@ static int core_pager_config(const char *var, const char
*value, void *data)
return 0;
}
-static void read_early_config(config_fn_t cb, void *data)
-{
- git_config_with_options(cb, data, NULL, 1);
-
- /*
-* Note that this is a really dirty hack that does the wrong thing in
-* many cases. The crux of the problem is that we cannot run
-* setup_git_directory() early on in git's setup, so we have no idea if
-* we are in a repository or not, and therefore are not sure whether
-* and how to read repository-local config.
-*
-* So if we _aren't_ in a repository (or we are but we would reject its
-* core.repositoryformatversion), we'll read whatever is in .git/config
-* blindly. Similarly, if we _are_ in a repository, but not at the
-* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc). See t7006 for a complete set of failures.
-*
-* However, we have historically provided this hack because it does
-* work some of the time (namely when you are at the top-level of a
-* valid repository), and would rarely make things worse (i.e., you do
-* not generally have a .git/config file sitting around).
-*/
- if (!startup_info->have_repository) {
- struct git_config_source repo_config;
-
- memset(&repo_config, 0, sizeof(repo_config));
- repo_config.file = ".git/config";
- git_config_with_options(cb, data, &repo_config, 1);
- }
-}
-
const char *git_pager(int stdout_is_tty)
{
const char *pager;
--
2.11.0.rc3.windows.1
[PATCH/RFC 5/7] read_early_config(): really discover .git/
Earlier, we punted and simply assumed that we are in the top-level
directory of the project, and that there is no .git file but a .git/
directory so that we can read directly from .git/config.
Let's discover the .git/ directory correctly.
Signed-off-by: Johannes Schindelin
---
config.c | 69 +---
1 file changed, 49 insertions(+), 20 deletions(-)
diff --git a/config.c b/config.c
index 4c756edca1..286d3cad66 100644
--- a/config.c
+++ b/config.c
@@ -1385,35 +1385,64 @@ static void configset_iter(struct config_set *cs,
config_fn_t fn, void *data)
}
}
+/*
+ * Note that this is a really dirty hack that replicates what the
+ * setup_git_directory() function does, without changing the current
+ * working directory. The crux of the problem is that we cannot run
+ * setup_git_directory() early on in git's setup, so we have to
+ * duplicate the work that setup_git_directory() would otherwise do.
+ */
+static int discover_git_directory_gently(struct strbuf *result)
+{
+ const char *p;
+
+ if (strbuf_getcwd(result) < 0)
+ return -1;
+ p = real_path_if_valid(result->buf);
+ if (!p)
+ return -1;
+ strbuf_reset(result);
+ strbuf_addstr(result, p);
+ for (;;) {
+ int len = result->len, i;
+
+ strbuf_addstr(result, "/" DEFAULT_GIT_DIR_ENVIRONMENT);
+ p = read_gitfile_gently(result->buf, &i);
+ if (p) {
+ strbuf_reset(result);
+ strbuf_addstr(result, p);
+ return 0;
+ }
+ if (is_git_directory(result->buf))
+ return 0;
+ strbuf_setlen(result, len);
+ if (is_git_directory(result->buf))
+ return 0;
+ for (i = len; i > 0; )
+ if (is_dir_sep(result->buf[--i]))
+ break;
+ if (!i)
+ return -1;
+ strbuf_setlen(result, i);
+ }
+}
+
void read_early_config(config_fn_t cb, void *data, int discover_git_dir)
{
+ struct strbuf buf = STRBUF_INIT;
+
git_config_with_options(cb, data, NULL, 1);
- /*
-* Note that this is a really dirty hack that does the wrong thing in
-* many cases. The crux of the problem is that we cannot run
-* setup_git_directory() early on in git's setup, so we have no idea if
-* we are in a repository or not, and therefore are not sure whether
-* and how to read repository-local config.
-*
-* So if we _aren't_ in a repository (or we are but we would reject its
-* core.repositoryformatversion), we'll read whatever is in .git/config
-* blindly. Similarly, if we _are_ in a repository, but not at the
-* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc). See t7006 for a complete set of failures.
-*
-* However, we have historically provided this hack because it does
-* work some of the time (namely when you are at the top-level of a
-* valid repository), and would rarely make things worse (i.e., you do
-* not generally have a .git/config file sitting around).
-*/
- if (discover_git_dir && !have_git_dir()) {
+ if (discover_git_dir && !have_git_dir() &&
+ !discover_git_directory_gently(&buf)) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
- repo_config.file = ".git/config";
+ strbuf_addstr(&buf, "/config");
+ repo_config.file = buf.buf;
git_config_with_options(cb, data, &repo_config, 1);
}
+ strbuf_release(&buf);
}
static void git_config_check_init(void);
--
2.11.0.rc3.windows.1
[PATCH/RFC 3/7] Mark builtins that create .git/ directories
To refactor read_early_config() so that it discovers .git/config
properly, we have to make certain that commands such as `git init` (i.e.
commands that create their own .git/ and therefore do *not* want to
be affected by any other .git/ directory) skip this discovery.
Let's introduce a flag that states for every builtin whether it creates
its own .git/ directory or not.
Signed-off-by: Johannes Schindelin
---
git.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/git.c b/git.c
index dce529fcbf..61df78afc8 100644
--- a/git.c
+++ b/git.c
@@ -318,12 +318,13 @@ static int handle_alias(int *argcp, const char ***argv)
#define RUN_SETUP (1<<0)
#define RUN_SETUP_GENTLY (1<<1)
#define USE_PAGER (1<<2)
+#define CREATES_GIT_DIR (1<<3)
/*
* require working tree to be present -- anything uses this needs
* RUN_SETUP for reading from the configuration file.
*/
-#define NEED_WORK_TREE (1<<3)
-#define SUPPORT_SUPER_PREFIX (1<<4)
+#define NEED_WORK_TREE (1<<4)
+#define SUPPORT_SUPER_PREFIX (1<<5)
struct cmd_struct {
const char *cmd;
@@ -412,7 +413,7 @@ static struct cmd_struct commands[] = {
{ "cherry", cmd_cherry, RUN_SETUP },
{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
- { "clone", cmd_clone },
+ { "clone", cmd_clone, CREATES_GIT_DIR },
{ "column", cmd_column, RUN_SETUP_GENTLY },
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
@@ -438,7 +439,7 @@ static struct cmd_struct commands[] = {
{ "hash-object", cmd_hash_object },
{ "help", cmd_help },
{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
- { "init", cmd_init_db },
+ { "init", cmd_init_db, CREATES_GIT_DIR },
{ "init-db", cmd_init_db },
{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
{ "log", cmd_log, RUN_SETUP },
--
2.11.0.rc3.windows.1
Re: [PATCH 4/5] Make sequencer abort safer
Hi, On Wed, 7 Dec 2016, Stephan Beyer wrote: > diff --git a/sequencer.c b/sequencer.c > index 30b10ba14..c9b560ac1 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer") > static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") > static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts") > static GIT_PATH_FUNC(git_path_head_file, "sequencer/head") > +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current") Is it required by law to have a four-letter infix, or can we have a nicer variable name (e.g. git_path_current_file)? Ciao, Dscho
[PATCHv2 6.5/7] squash! versioncmp: use earliest-longest contained suffix to determine sorting order
As the number of identical steps to be done for both tagnames grows,
extract them into a helper function, with the additional benefit that
the conditionals near the end of swap_prereleases() will use more
meaningful variable names.
Signed-off-by: SZEDER Gábor
---
I was not particularly happy with the amount of code duplication this
series added to swap_prereleases() in patches 5 and 6, hence this bit
of refactoring. Which I'm not particularly happy with either,
because, well, look at that diffstat.
I don't have strong feelings either way, but here it is, take it or
leave it.
versioncmp.c | 62
1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/versioncmp.c b/versioncmp.c
index 62c8d69dc..601ceddef 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -24,6 +24,29 @@
static const struct string_list *prereleases;
static int initialized;
+struct suffix_match {
+ int conf_pos;
+ int start;
+ int len;
+};
+
+static void find_better_matching_suffix(const char *tagname, const char
*suffix,
+ int suffix_len, int start, int conf_pos,
+ struct suffix_match *match)
+{
+ /* A better match either starts earlier or starts at the same offset
+* but is longer. */
+ int end = match->len < suffix_len ? match->start : match->start-1;
+ int i;
+ for (i = start; i <= end; i++)
+ if (starts_with(tagname + i, suffix)) {
+ match->conf_pos = conf_pos;
+ match->start = i;
+ match->len = suffix_len;
+ break;
+ }
+}
+
/*
* off is the offset of the first different character in the two strings
* s1 and s2. If either s1 or s2 contains a prerelease suffix containing
@@ -47,46 +70,35 @@ static int swap_prereleases(const char *s1,
int off,
int *diff)
{
- int i, i1 = -1, i2 = -1;
- int start_at1 = off, start_at2 = off, match_len1 = -1, match_len2 = -1;
+ int i;
+ struct suffix_match match1 = { -1, off, -1 };
+ struct suffix_match match2 = { -1, off, -1 };
for (i = 0; i < prereleases->nr; i++) {
const char *suffix = prereleases->items[i].string;
- int j, start, end, suffix_len = strlen(suffix);
+ int start, suffix_len = strlen(suffix);
if (suffix_len < off)
start = off - suffix_len;
else
start = 0;
- end = match_len1 < suffix_len ? start_at1 : start_at1-1;
- for (j = start; j <= end; j++)
- if (starts_with(s1 + j, suffix)) {
- i1 = i;
- start_at1 = j;
- match_len1 = suffix_len;
- break;
- }
- end = match_len2 < suffix_len ? start_at2 : start_at2-1;
- for (j = start; j <= end; j++)
- if (starts_with(s2 + j, suffix)) {
- i2 = i;
- start_at2 = j;
- match_len2 = suffix_len;
- break;
- }
+ find_better_matching_suffix(s1, suffix, suffix_len, start,
+ i, &match1);
+ find_better_matching_suffix(s2, suffix, suffix_len, start,
+ i, &match2);
}
- if (i1 == -1 && i2 == -1)
+ if (match1.conf_pos == -1 && match2.conf_pos == -1)
return 0;
- if (i1 == i2)
+ if (match1.conf_pos == match2.conf_pos)
/* Found the same suffix in both, e.g. "-rc" in "v1.0-rcX"
* and "v1.0-rcY": the caller should decide based on "X"
* and "Y". */
return 0;
- if (i1 >= 0 && i2 >= 0)
- *diff = i1 - i2;
- else if (i1 >= 0)
+ if (match1.conf_pos >= 0 && match2.conf_pos >= 0)
+ *diff = match1.conf_pos - match2.conf_pos;
+ else if (match1.conf_pos >= 0)
*diff = -1;
- else /* if (i2 >= 0) */
+ else /* if (match2.conf_pos >= 0) */
*diff = 1;
return 1;
}
--
2.11.0.78.g5a2d011
[REGRESSION 2.10.2] problematic "empty auth" changes
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? Ciao, Dscho
Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
> On 08 Dec 2016, at 15:00, Nick Warr wrote: > > That looks pretty much like the error we're dealing with, any reason > why going back a point version on Git (not git-lfs) would resolve the > issue however? Going back to GitLFS 1.4.* would make the error disappear. However, I think you should fix your repository. Try this: git lfs clone cd git rm --cached "workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi Effects/Effects/Debris/Meshes/debris_junk.FBX" git add --force "workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi Effects/Effects/Debris/Meshes/debris_junk.FBX" git commit -m "Move files properly to GitLFS" git push Afterwards you should be able to use the latest version of Git and GitLFS without trouble. Cheers, Lars PS: Top posting is not that popular in the Git community ;-) > > On 8 December 2016 at 13:57, Lars Schneider wrote: >> >>> On 08 Dec 2016, at 12:46, Nick Warr wrote: >>> >>> Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with >>> 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS. >>> >>> When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the >>> first time the clone will finish the checkout of the git part, then >>> when it starts downloading the LFS parts it will reliably finish with >>> a smudge filter error. >>> >>> This leaves the repo in an unstable condition, you can then fetch the >>> lfs part without issue, but checking out the lfs files or trying a git >>> reset --hard will continue to spit out the same error. As you can see, >>> the actual error is not particularly useful. >>> >>> fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi >>> Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs >>> failed >>> Unknown command "" >>> >>> Possibly it's due to the file extension being all capital letters, we >>> did manage to change the error by recommitting the file with a >>> lowercase extension, but it failed on the next file (which also had a >>> capital letter extension). >>> >>> This has happened on multiple fresh windows 10 64 bit installs, >>> different machines and target directories (to hopefully remove the >>> possibility of file permissions) where cloning is taking place. >>> >>> The solution is to back level to Git 2.10.2 and the error disappears. >>> >>> More than willing to provide any further information, >> >> Hi Nick, >> >> debris_junk.FBX is not stored properly in Git LFS. >> I explained the problem in detail here: >> https://github.com/git-lfs/git-lfs/issues/1729 >> >> You should add the file properly to GitLFS to fix the problem. >> However, I think this is a regression in GitLFS and I hope it will >> be fixed in the next version. >> >> No change/fix in Git is required. >> >> Cheers, >> Lars
[PATCHv2 5/7] versioncmp: cope with common part overlapping with prerelease suffix
Version sort with prerelease reordering sometimes puts tagnames in the
wrong order, when the common part of two compared tagnames overlaps
with the leading character(s) of one or more configured prerelease
suffixes. Note the position of "v2.1.0-beta-1":
$ git -c versionsort.prereleaseSuffix=-beta \
tag -l --sort=version:refname v2.1.*
v2.1.0-beta-2
v2.1.0-beta-3
v2.1.0
v2.1.0-RC1
v2.1.0-RC2
v2.1.0-beta-1
v2.1.1
v2.1.2
The reason is that when comparing a pair of tagnames, first
versioncmp() looks for the first different character in a pair of
tagnames, and then the swap_prereleases() helper function looks for a
configured prerelease suffix _starting at_ that character. Thus, when
in the above example the sorting algorithm happens to compare the
tagnames "v2.1.0-beta-1" and "v2.1.0-RC2", swap_prereleases() tries to
match the suffix "-beta" against "beta-1" to no avail, and the two
tagnames erroneously end up being ordered lexicographically.
To fix this issue change swap_prereleases() to look for configured
prerelease suffixes _containing_ the position of that first different
character.
Care must be taken, when a configured suffix is longer than the
tagnames' common part up to the first different character, to avoid
reading memory before the beginning of the tagnames. Add a test that
uses an exceptionally long prerelease suffix to check for this, in the
hope that in case of a regression the illegal memory access causes a
segfault in 'git tag' on one of the commonly used platforms (the test
happens to pass successfully on my Linux system with the safety check
removed), or at least makes valgrind complain.
Under some circumstances it's possible that more than one prerelease
suffixes can be found in the same tagname around that first different
character. With this simple bugfix patch such a tagname is sorted
according to the contained suffix that comes first in the
configuration for now. This is less than ideal in some cases, and the
following patch will take care of those.
Reported-by: Leho Kraav
Signed-off-by: SZEDER Gábor
---
Documentation/config.txt | 8 ++--
t/t7004-tag.sh | 9 +++--
versioncmp.c | 28 +---
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb6790d..c1a9616e9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3008,8 +3008,12 @@ versionsort.prereleaseSuffix::
This variable can be specified multiple times, once per suffix. The
order of suffixes in the config file determines the sorting order
(e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX
-is sorted before 1.0-rcXX). The sorting order between different
-suffixes is undefined if they are in multiple config files.
+is sorted before 1.0-rcXX).
+If more than one suffixes match the same tagname, then that tagname will
+be sorted according to the matching suffix which comes first in the
+configuration.
+The sorting order between different suffixes is undefined if they are
+in multiple config files.
web.browser::
Specify a web browser that may be used by some commands.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 6445aae29..c7aaace8c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1538,7 +1538,7 @@ test_expect_success 'reverse version sort with prerelease
reordering' '
test_cmp expect actual
'
-test_expect_failure 'version sort with prerelease reordering and common
leading character' '
+test_expect_success 'version sort with prerelease reordering and common
leading character' '
test_config versionsort.prereleaseSuffix -before &&
git tag foo1.7-before1 &&
git tag foo1.7 &&
@@ -1552,7 +1552,7 @@ test_expect_failure 'version sort with prerelease
reordering and common leading
test_cmp expect actual
'
-test_expect_failure 'version sort with prerelease reordering, multiple
suffixes and common leading character' '
+test_expect_success 'version sort with prerelease reordering, multiple
suffixes and common leading character' '
test_config versionsort.prereleaseSuffix -before &&
git config --add versionsort.prereleaseSuffix -after &&
git tag -l --sort=version:refname "foo1.7*" >actual &&
@@ -1564,6 +1564,11 @@ test_expect_failure 'version sort with prerelease
reordering, multiple suffixes
test_cmp expect actual
'
+test_expect_success 'version sort with very long prerelease suffix' '
+ test_config versionsort.prereleaseSuffix
-very-long-prerelease-suffix &&
+ git tag -l --sort=version:refname
+'
+
run_with_limited_stack () {
(ulimit -s 128 && "$@")
}
diff --git a/versioncmp.c b/versioncmp.c
index a55c23ad5..f86ac562e 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -26,12 +26,15 @@ static int initialized;
/*
* off is the offset of the first different character in the two strings
- * s1 an
[PATCHv2 2/7] t7004-tag: use test_config helper
... instead of setting and then manually unsetting configuration variables, on one occasion even outside the test_expect_success block. Signed-off-by: SZEDER Gábor --- t/t7004-tag.sh | 23 ++- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 396cffeeb..920a1b4b2 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -297,11 +297,9 @@ EOF ' test_expect_success 'listing tags in column with column.*' ' - git config column.tag row && - git config column.ui dense && + test_config column.tag row && + test_config column.ui dense && COLUMNS=40 git tag -l >actual && - git config --unset column.ui && - git config --unset column.tag && cat >expected <<\EOF && a1 aa1 cba t210t211 v0.2.1 v1.0 v1.0.1 v1.1.3 @@ -314,9 +312,8 @@ test_expect_success 'listing tag with -n --column should fail' ' ' test_expect_success 'listing tags -n in column with column.ui ignored' ' - git config column.ui "row dense" && + test_config column.ui "row dense" && COLUMNS=40 git tag -l -n >actual && - git config --unset column.ui && cat >expected <<\EOF && a1 Foo aa1 Foo @@ -1200,11 +1197,10 @@ test_expect_success GPG,RFC1991 \ ' # try to sign with bad user.signingkey -git config user.signingkey BobTheMouse test_expect_success GPG \ 'git tag -s fails if gpg is misconfigured (bad key)' \ - 'test_must_fail git tag -s -m tail tag-gpg-failure' -git config --unset user.signingkey + 'test_config user.signingkey BobTheMouse && + test_must_fail git tag -s -m tail tag-gpg-failure' # try to produce invalid signature test_expect_success GPG \ @@ -1484,7 +1480,7 @@ test_expect_success 'reverse lexical sort' ' ' test_expect_success 'configured lexical sort' ' - git config tag.sort "v:refname" && + test_config tag.sort "v:refname" && git tag -l "foo*" >actual && cat >expect <<-\EOF && foo1.3 @@ -1495,6 +1491,7 @@ test_expect_success 'configured lexical sort' ' ' test_expect_success 'option override configured sort' ' + test_config tag.sort "v:refname" && git tag -l --sort=-refname "foo*" >actual && cat >expect <<-\EOF && foo1.6 @@ -1509,13 +1506,12 @@ test_expect_success 'invalid sort parameter on command line' ' ' test_expect_success 'invalid sort parameter in configuratoin' ' - git config tag.sort "v:notvalid" && + test_config tag.sort "v:notvalid" && test_must_fail git tag -l "foo*" ' test_expect_success 'version sort with prerelease reordering' ' - git config --unset tag.sort && - git config versionsort.prereleaseSuffix -rc && + test_config versionsort.prereleaseSuffix -rc && git tag foo1.6-rc1 && git tag foo1.6-rc2 && git tag -l --sort=version:refname "foo*" >actual && @@ -1530,6 +1526,7 @@ test_expect_success 'version sort with prerelease reordering' ' ' test_expect_success 'reverse version sort with prerelease reordering' ' + test_config versionsort.prereleaseSuffix -rc && git tag -l --sort=-version:refname "foo*" >actual && cat >expect <<-\EOF && foo1.10 -- 2.11.0.78.g5a2d011
[PATCHv2 7/7] versioncmp: generalize version sort suffix reordering
The 'versionsort.prereleaseSuffix' configuration variable, as its name suggests, is supposed to only deal with tagnames with prerelease suffixes, and allows sorting those prerelease tags in a user-defined order before the suffixless main release tag, instead of sorting them simply lexicographically. However, the previous changes in this series resulted in an interesting and useful property of version sort: - The empty string as a configured suffix matches all tagnames, including tagnames without any suffix, but - tagnames containing a "real" configured suffix are still ordered according to that real suffix, because any longer suffix takes precedence over the empty string. Exploiting this property we can easily generalize suffix reordering and specify the order of tags with given suffixes not only before but even after a main release tag by using the empty suffix to denote the position of the main release tag, without any algorithm changes: $ git -c versionsort.prereleaseSuffix=-alpha \ -c versionsort.prereleaseSuffix=-beta \ -c versionsort.prereleaseSuffix="" \ -c versionsort.prereleaseSuffix=-gamma \ -c versionsort.prereleaseSuffix=-delta \ tag -l --sort=version:refname 'v3.0*' v3.0-alpha1 v3.0-beta1 v3.0 v3.0-gamma1 v3.0-delta1 Since 'versionsort.prereleaseSuffix' is not a fitting name for a configuration variable to control this more general suffix reordering, introduce the new variable 'versionsort.suffix'. Still keep the old configuration variable name as a deprecated alias, though, to avoid suddenly breaking setups already using it. Ignore the old variable if both old and new configuration variables are set, but emit a warning so users will be aware of it and can fix their configuration. Extend the documentation to describe and add a test to check this more general behavior. Note: since the empty suffix matches all tagnames, tagnames with suffixes not included in the configuration are listed together with the suffixless main release tag, ordered lexicographically right after that, i.e. before tags with suffixes listed in the configuration following the empty suffix. Signed-off-by: SZEDER Gábor --- Documentation/config.txt | 36 ++-- Documentation/git-tag.txt | 4 ++-- t/t7004-tag.sh| 35 +++ versioncmp.c | 9 - 4 files changed, 71 insertions(+), 13 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 58009c70c..ae85d4b9a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2999,16 +2999,32 @@ user.signingKey:: This option is passed unchanged to gpg's --local-user parameter, so you may specify a key using any method that gpg supports. -versionsort.prereleaseSuffix:: - When version sort is used in linkgit:git-tag[1], prerelease - tags (e.g. "1.0-rc1") may appear after the main release - "1.0". By specifying the suffix "-rc" in this variable, - "1.0-rc1" will appear before "1.0". -+ -This variable can be specified multiple times, once per suffix. The -order of suffixes in the config file determines the sorting order -(e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX -is sorted before 1.0-rcXX). +versionsort.prereleaseSuffix (deprecated):: + Deprecated alias for `versionsort.suffix`. Ignored if + `versionsort.suffix` is set. + +versionsort.suffix:: + Even when version sort is used in linkgit:git-tag[1], tagnames + with the same base version but different suffixes are still sorted + lexicographically, resulting e.g. in prerelease tags appearing + after the main release (e.g. "1.0-rc1" after "1.0"). This + variable can be specified to determine the sorting order of tags + with different suffixes. ++ +By specifying a single suffix in this variable, any tagname containing +that suffix will appear before the corresponding main release. E.g. if +the variable is set to "-rc", then all "1.0-rcX" tags will appear before +"1.0". If specified multiple times, once per suffix, then the order of +suffixes in the configuration will determine the sorting order of tagnames +with those suffixes. E.g. if "-pre" appears before "-rc" in the +configuration, then all "1.0-preX" tags will be listed before any +"1.0-rcX" tags. The placement of the main release tag relative to tags +with various suffixes can be determined by specifying the empty suffix +among those other suffixes. E.g. if the suffixes "-rc", "", "-ck" and +"-bfs" appear in the configuration in this order, then all "v4.8-rcX" tags +are listed first, followed by "v4.8", then "v4.8-ckX" and finally +"v4.8-bfsX". ++ If more than one suffixes match the same tagname, then that tagname will be sorted according to the suffix which starts at the earliest position in the tagname. If more than one different matching suffixes
[PATCHv2 4/7] versioncmp: pass full tagnames to swap_prereleases()
The swap_prereleases() helper function is responsible for finding
configured prerelease suffixes in a pair of tagnames to be compared,
but this function currently gets to see only the parts of those two
tagnames starting at the first different character. To fix some
issues related to the common part of two tagnames overlapping with
leading part of a prerelease suffix, this helper function must see
both full tagnames.
In preparation for the fix in the following patch, refactor
swap_prereleases() and its caller to pass two full tagnames and an
additional offset indicating the position of the first different
character.
While updating the comment describing that function, remove the
sentence about not dealing with both tagnames having the same suffix.
Currently it doesn't add much value (we know that there is a different
character, so it's obvious that it can't possibly be the same suffix
in both), and at the end of this patch series it won't even be true
anymore.
Signed-off-by: SZEDER Gábor
---
versioncmp.c | 29 +
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/versioncmp.c b/versioncmp.c
index 80bfd109f..a55c23ad5 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -25,32 +25,28 @@ static const struct string_list *prereleases;
static int initialized;
/*
- * p1 and p2 point to the first different character in two strings. If
- * either p1 or p2 starts with a prerelease suffix, it will be forced
- * to be on top.
+ * off is the offset of the first different character in the two strings
+ * s1 and s2. If either s1 or s2 contains a prerelease suffix starting
+ * at that offset, it will be forced to be on top.
*
- * If both p1 and p2 start with (different) suffix, the order is
- * determined by config file.
- *
- * Note that we don't have to deal with the situation when both p1 and
- * p2 start with the same suffix because the common part is already
- * consumed by the caller.
+ * If both s1 and s2 contain a (different) suffix at that position,
+ * their order is determined by the order of those two suffixes in the
+ * configuration.
*
* Return non-zero if *diff contains the return value for versioncmp()
*/
-static int swap_prereleases(const void *p1_,
- const void *p2_,
+static int swap_prereleases(const char *s1,
+ const char *s2,
+ int off,
int *diff)
{
- const char *p1 = p1_;
- const char *p2 = p2_;
int i, i1 = -1, i2 = -1;
for (i = 0; i < prereleases->nr; i++) {
const char *suffix = prereleases->items[i].string;
- if (i1 == -1 && starts_with(p1, suffix))
+ if (i1 == -1 && starts_with(s1 + off, suffix))
i1 = i;
- if (i2 == -1 && starts_with(p2, suffix))
+ if (i2 == -1 && starts_with(s2 + off, suffix))
i2 = i;
}
if (i1 == -1 && i2 == -1)
@@ -121,7 +117,8 @@ int versioncmp(const char *s1, const char *s2)
initialized = 1;
prereleases =
git_config_get_value_multi("versionsort.prereleasesuffix");
}
- if (prereleases && swap_prereleases(p1 - 1, p2 - 1, &diff))
+ if (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1,
+ &diff))
return diff;
state = result_type[state * 3 + (((c2 == '0') + (isdigit (c2) != 0)))];
--
2.11.0.78.g5a2d011
[PATCHv2 6/7] versioncmp: use earliest-longest contained suffix to determine sorting order
When comparing tagnames, it is possible that a tagname contains more than one of the configured prerelease suffixes around the first different character. After fixing a bug in the previous commit such a tagname is sorted according to the contained suffix which comes first in the configuration. This is, however, not quite the right thing to do in the following corner cases: 1. $ git -c versionsort.suffix=-bar -c versionsort.suffix=-foo-baz -c versionsort.suffix=-foo-bar tag -l --sort=version:refname 'v1*' v1.0-foo-bar v1.0-foo-baz The suffix of the tagname 'v1.0-foo-bar' is clearly '-foo-bar', so it should be listed last. However, as it also contains '-bar' around the first different character, it is listed first instead, because that '-bar' suffix comes first the configuration. 2. One of the configured suffixes starts with the other: $ git -c versionsort.prereleasesuffix=-pre \ -c versionsort.prereleasesuffix=-prerelease \ tag -l --sort=version:refname 'v2*' v2.0-prerelease1 v2.0-pre1 v2.0-pre2 Here the tagname 'v2.0-prerelease1' should be the last. When comparing 'v2.0-pre1' and 'v2.0-prerelease1' the first different characters are '1' and 'r', respectively. Since this first different character must be part of the configured suffix, the '-pre' suffix is not recognized in the first tagname. OTOH, the '-prerelease' suffix is properly recognized in 'v2.0-prerelease1', thus it is listed first. Improve version sort in these corner cases, and - look for a configured prerelease suffix containing the first different character or ending right before it, so the '-pre' suffixes are recognized in case (2). This also means that when comparing tagnames 'v2.0-pre1' and 'v2.0-pre2', swap_prereleases() would find the '-pre' suffix in both, but then it will return "undecided" and the caller will do the right thing by sorting based in '1' and '2'. - If the tagname contains more than one suffix, then give precedence to the contained suffix that starts at the earliest offset in the tagname to address (1). - If there are more than one suffixes starting at that earliest position, then give precedence to the longest of those suffixes, thus ensuring that in (2) the tagname 'v2.0-prerelease1' won't be sorted based on the '-pre' suffix. Add tests for these corner cases and adjust the documentation accordingly. Signed-off-by: SZEDER Gábor --- Documentation/config.txt | 6 -- t/t7004-tag.sh | 31 +++ versioncmp.c | 33 + 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index c1a9616e9..58009c70c 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -3010,8 +3010,10 @@ order of suffixes in the config file determines the sorting order (e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX is sorted before 1.0-rcXX). If more than one suffixes match the same tagname, then that tagname will -be sorted according to the matching suffix which comes first in the -configuration. +be sorted according to the suffix which starts at the earliest position in +the tagname. If more than one different matching suffixes start at +that earliest position, then that tagname will be sorted according to the +longest of those suffixes. The sorting order between different suffixes is undefined if they are in multiple config files. diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index c7aaace8c..e2efe312d 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1564,6 +1564,37 @@ test_expect_success 'version sort with prerelease reordering, multiple suffixes test_cmp expect actual ' +test_expect_success 'version sort with prerelease reordering, multiple suffixes match the same tag' ' + test_config versionsort.prereleaseSuffix -bar && + git config --add versionsort.prereleaseSuffix -foo-baz && + git config --add versionsort.prereleaseSuffix -foo-bar && + git tag foo1.8-foo-bar && + git tag foo1.8-foo-baz && + git tag foo1.8 && + git tag -l --sort=version:refname "foo1.8*" >actual && + cat >expect <<-\EOF && + foo1.8-foo-baz + foo1.8-foo-bar + foo1.8 + EOF + test_cmp expect actual +' + +test_expect_success 'version sort with prerelease reordering, multiple suffixes match starting at the same position' ' + test_config versionsort.prereleaseSuffix -pre && + git config --add versionsort.prereleaseSuffix -prerelease && + git tag foo1.9-pre1 && + git tag foo1.9-pre2 && + git tag foo1.9-prerelease1 && + git tag -l --sort=version:refname "foo1.9*" >actual && + cat >expect <<-\EOF && + foo1.9-pre1 + foo1.9
[PATCHv2 0/7] Fix and generalize version sort reordering
> After having slept on this a couple of times
After having slept on it a few (erm...) more times...
> I think the longest
> matching prerelease suffix should determine the sorting order.
>
> A release tag usually consists of an optional prefix, e.g. 'v' or
> 'snapshot-', followed by the actual version number, followed by an
> optional suffix. In the contrived example quoted above this suffix
> is '-foo-bar', thus it feels wrong to match '-bar' against it, when
> the user explicitly configured '-foo-bar' as prerelease suffix as
> well.
At the risk of overthinking it: I think it would be more correct to
say that the earliest starting matching prerelease suffix should
determine the sorting order in the first place.
> Then here is a more realistic case for sorting based on the longest
> matching suffix, where
>
>- a longer suffix starts with the shorter one,
>- and the longer suffix comes after the shorter one in the
> configuration.
>
> With my patches it looks like this:
>
> $ git -c versionsort.prereleasesuffix=-pre \
> -c versionsort.prereleasesuffix=-prerelease \
> tag -l --sort=version:refname
> v1.0.0-prerelease1
> v1.0.0-pre1
> v1.0.0-pre2
> v1.0.0
And when there happen to be more than one matching suffixes starting
at the same earliest position, then we should pick the longest of
them. The new patch 6/7 implements this behavior.
Changes since v1:
- Documentation, in-code comment and commit message updates.
- Use different tagnames and suffixes in the tests, keeping the new
tests simpler and changes to existing tests smaller.
- Added a test of questionable value to try to check that we don't
read memory before the tagname strings in case of an unusually long
suffix.
- Removed unnecessary {}.
- Added two patches on top to address the corner cases and to
introduce 'versionsort.suffix' for the resulting more general
suffix reordering behavior.
The interdiff at the bottom is between v1 and only the first five
patches of v2, i.e. not including the two new patches. I think it's
easier to judge the changes this way.
SZEDER Gábor (7):
t7004-tag: delete unnecessary tags with test_when_finished
t7004-tag: use test_config helper
t7004-tag: add version sort tests to show prerelease reordering issues
versioncmp: pass full tagnames to swap_prereleases()
versioncmp: cope with common part overlapping with prerelease suffix
versioncmp: use earliest-longest contained suffix to determine sorting
order
versioncmp: generalize version sort suffix reordering
Documentation/config.txt | 44
Documentation/git-tag.txt | 4 +-
t/t7004-tag.sh| 132 +++---
versioncmp.c | 68 +---
4 files changed, 196 insertions(+), 52 deletions(-)
--
2.11.0.78.g5a2d011
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb6790d..c1a9616e9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3008,8 +3008,12 @@ versionsort.prereleaseSuffix::
This variable can be specified multiple times, once per suffix. The
order of suffixes in the config file determines the sorting order
(e.g. if "-pre" appears before "-rc" in the config file then 1.0-preXX
-is sorted before 1.0-rcXX). The sorting order between different
-suffixes is undefined if they are in multiple config files.
+is sorted before 1.0-rcXX).
+If more than one suffixes match the same tagname, then that tagname will
+be sorted according to the matching suffix which comes first in the
+configuration.
+The sorting order between different suffixes is undefined if they are
+in multiple config files.
web.browser::
Specify a web browser that may be used by some commands.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f0cfe1fa3..c7aaace8c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1511,14 +1511,14 @@ test_expect_success 'invalid sort parameter in
configuratoin' '
'
test_expect_success 'version sort with prerelease reordering' '
- test_config versionsort.prereleaseSuffix -beta &&
- git tag foo1.6-beta1 &&
- git tag foo1.6-beta2 &&
+ test_config versionsort.prereleaseSuffix -rc &&
+ git tag foo1.6-rc1 &&
+ git tag foo1.6-rc2 &&
git tag -l --sort=version:refname "foo*" >actual &&
cat >expect <<-\EOF &&
foo1.3
- foo1.6-beta1
- foo1.6-beta2
+ foo1.6-rc1
+ foo1.6-rc2
foo1.6
foo1.10
EOF
@@ -1526,54 +1526,49 @@ test_expect_success 'version sort with prerelease
reordering' '
'
test_expect_success 'reverse version sort with prerelease reordering' '
- test_config versionsort.prereleaseSuffix -beta &&
+ test_config versionsort.prereleaseSuffix -rc &&
git tag -l --sort=-version:refname "foo*" >actual &&
cat >expect <<-\EOF &&
foo1.10
foo1.6
- foo1.6-beta2
- foo1.6-
[PATCHv2 3/7] t7004-tag: add version sort tests to show prerelease reordering issues
Version sort with prerelease reordering sometimes puts tagnames in the
wrong order, when the common part of two compared tagnames ends with
the leading character(s) of one or more configured prerelease
suffixes. Add tests that demonstrate these issues.
The unrelated '--format should list tags as per format given' test
later uses tags matching the same prefix as the version sort tests,
thus was affected by the new tags added for the new tests in this
patch. Change that test to perform its checks on a different set of
tags.
Signed-off-by: SZEDER Gábor
---
t/t7004-tag.sh | 36 ++--
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 920a1b4b2..6445aae29 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1538,6 +1538,32 @@ test_expect_success 'reverse version sort with
prerelease reordering' '
test_cmp expect actual
'
+test_expect_failure 'version sort with prerelease reordering and common
leading character' '
+ test_config versionsort.prereleaseSuffix -before &&
+ git tag foo1.7-before1 &&
+ git tag foo1.7 &&
+ git tag foo1.7-after1 &&
+ git tag -l --sort=version:refname "foo1.7*" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.7-before1
+ foo1.7
+ foo1.7-after1
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_failure 'version sort with prerelease reordering, multiple
suffixes and common leading character' '
+ test_config versionsort.prereleaseSuffix -before &&
+ git config --add versionsort.prereleaseSuffix -after &&
+ git tag -l --sort=version:refname "foo1.7*" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.7-before1
+ foo1.7-after1
+ foo1.7
+ EOF
+ test_cmp expect actual
+'
+
run_with_limited_stack () {
(ulimit -s 128 && "$@")
}
@@ -1566,13 +1592,11 @@ EOF"
test_expect_success '--format should list tags as per format given' '
cat >expect <<-\EOF &&
- refname : refs/tags/foo1.10
- refname : refs/tags/foo1.3
- refname : refs/tags/foo1.6
- refname : refs/tags/foo1.6-rc1
- refname : refs/tags/foo1.6-rc2
+ refname : refs/tags/v1.0
+ refname : refs/tags/v1.0.1
+ refname : refs/tags/v1.1.3
EOF
- git tag -l --format="refname : %(refname)" "foo*" >actual &&
+ git tag -l --format="refname : %(refname)" "v1*" >actual &&
test_cmp expect actual
'
--
2.11.0.78.g5a2d011
[PATCHv2 1/7] t7004-tag: delete unnecessary tags with test_when_finished
The '--force is moot with a non-existing tag name' test creates two new tags, which are then deleted right after the test is finished, outside the test_expect_success block, allowing 'git tag -d's output to pollute the test output. Use test_when_finished to delete those tags. Signed-off-by: SZEDER Gábor --- t/t7004-tag.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 8b0f71a2a..396cffeeb 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -122,11 +122,11 @@ test_expect_success '--force can create a tag with the name of one existing' ' tag_exists mytag' test_expect_success '--force is moot with a non-existing tag name' ' + test_when_finished git tag -d newtag forcetag && git tag newtag >expect && git tag --force forcetag >actual && test_cmp expect actual ' -git tag -d newtag forcetag # deleting tags: -- 2.11.0.78.g5a2d011
Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
That looks pretty much like the error we're dealing with, any reason why going back a point version on Git (not git-lfs) would resolve the issue however? On 8 December 2016 at 13:57, Lars Schneider wrote: > >> On 08 Dec 2016, at 12:46, Nick Warr wrote: >> >> Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with >> 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS. >> >> When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the >> first time the clone will finish the checkout of the git part, then >> when it starts downloading the LFS parts it will reliably finish with >> a smudge filter error. >> >> This leaves the repo in an unstable condition, you can then fetch the >> lfs part without issue, but checking out the lfs files or trying a git >> reset --hard will continue to spit out the same error. As you can see, >> the actual error is not particularly useful. >> >> fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi >> Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs >> failed >> Unknown command "" >> >> Possibly it's due to the file extension being all capital letters, we >> did manage to change the error by recommitting the file with a >> lowercase extension, but it failed on the next file (which also had a >> capital letter extension). >> >> This has happened on multiple fresh windows 10 64 bit installs, >> different machines and target directories (to hopefully remove the >> possibility of file permissions) where cloning is taking place. >> >> The solution is to back level to Git 2.10.2 and the error disappears. >> >> More than willing to provide any further information, > > Hi Nick, > > debris_junk.FBX is not stored properly in Git LFS. > I explained the problem in detail here: > https://github.com/git-lfs/git-lfs/issues/1729 > > You should add the file properly to GitLFS to fix the problem. > However, I think this is a regression in GitLFS and I hope it will > be fixed in the next version. > > No change/fix in Git is required. > > Cheers, > Lars
Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
> On 08 Dec 2016, at 12:46, Nick Warr wrote: > > Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with > 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS. > > When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the > first time the clone will finish the checkout of the git part, then > when it starts downloading the LFS parts it will reliably finish with > a smudge filter error. > > This leaves the repo in an unstable condition, you can then fetch the > lfs part without issue, but checking out the lfs files or trying a git > reset --hard will continue to spit out the same error. As you can see, > the actual error is not particularly useful. > > fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi > Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs > failed > Unknown command "" > > Possibly it's due to the file extension being all capital letters, we > did manage to change the error by recommitting the file with a > lowercase extension, but it failed on the next file (which also had a > capital letter extension). > > This has happened on multiple fresh windows 10 64 bit installs, > different machines and target directories (to hopefully remove the > possibility of file permissions) where cloning is taking place. > > The solution is to back level to Git 2.10.2 and the error disappears. > > More than willing to provide any further information, Hi Nick, debris_junk.FBX is not stored properly in Git LFS. I explained the problem in detail here: https://github.com/git-lfs/git-lfs/issues/1729 You should add the file properly to GitLFS to fix the problem. However, I think this is a regression in GitLFS and I hope it will be fixed in the next version. No change/fix in Git is required. Cheers, Lars
[PATCH v2] commit: make --only --allow-empty work without paths
--only is implied when paths are present, and required
them unless --amend. But with --allow-empty it should
be allowed as well - it is the only way to create an
empty commit in the presence of staged changes.
Also remove the post-fact cleverness indication;
it's in the man page anyway.
Signed-off-by: Andreas Krey
---
Ok, I've removed the clever message, as Junio suggested.
I don't know what else to do to make it acceptable. :-)
We're going to deploy it internally anyway, but I think
it belongs in git.git as well (aka 'Can I has "will queue"?').
Documentation/git-commit.txt | 3 ++-
builtin/commit.c | 4 +---
t/t7501-commit.sh| 9 +
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f2ab0ee2e..4f8f20a36 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
If this option is specified together with `--amend`, then
no paths need to be specified, which can be used to amend
the last commit without committing changes that have
- already been staged.
+ already been staged. If used together with `--allow-empty`
+ paths are also not required, and an empty commit will be created.
-u[]::
--untracked-files[=]::
diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d29..276c74034 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1206,10 +1206,8 @@ static int parse_and_validate_options(int argc, const
char *argv[],
if (also + only + all + interactive > 1)
die(_("Only one of --include/--only/--all/--interactive/--patch
can be used."));
- if (argc == 0 && (also || (only && !amend)))
+ 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"))
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a67..0d8d89309 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -155,6 +155,15 @@ test_expect_success 'amend --only ignores staged contents'
'
git diff --exit-code
'
+test_expect_success 'allow-empty --only ignores staged contents' '
+ echo changed-again >file &&
+ git add file &&
+ git commit --allow-empty --only -m "empty" &&
+ git cat-file blob HEAD:file >file.actual &&
+ test_cmp file.expect file.actual &&
+ git diff --exit-code
+'
+
test_expect_success 'set up editor' '
cat >editor <<-\EOF &&
#!/bin/sh
--
2.11.0.10.g1e1b186.dirty
Re:Christmas Is Here!
Hi, My name is Julinda, i'm single and looking.
Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
Hi Junio, On Wed, 7 Dec 2016, Junio C Hamano wrote: > The "libify sequencer" topic stopped passing the die_on_error option > to hold_locked_index(), and this lost an error message from "git > merge --ff-only $commit" when there are competing updates in > progress. Sorry for the breakage. When libifying the code, I tried to be careful to retain the error messages when not dying, and mistakenly assumed that hold_locked_index() would do the same. Ciao, Dscho
Serious bug with Git-2.11.0-64-bit and Git-LFS
Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS. When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the first time the clone will finish the checkout of the git part, then when it starts downloading the LFS parts it will reliably finish with a smudge filter error. This leaves the repo in an unstable condition, you can then fetch the lfs part without issue, but checking out the lfs files or trying a git reset --hard will continue to spit out the same error. As you can see, the actual error is not particularly useful. fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs failed Unknown command "" Possibly it's due to the file extension being all capital letters, we did manage to change the error by recommitting the file with a lowercase extension, but it failed on the next file (which also had a capital letter extension). This has happened on multiple fresh windows 10 64 bit installs, different machines and target directories (to hopefully remove the possibility of file permissions) where cloning is taking place. The solution is to back level to Git 2.10.2 and the error disappears. More than willing to provide any further information, Nick Warr
Re: [PATCH] real_path: make real_path thread-safe
Am 07.12.2016 um 23:29 schrieb Brandon Williams: Instead of assuming root is "/" I'll need to extract what root is from an absolute path. Aside from what root looks like, do most other path constructs behave similarly in unix and windows? (like ".." and "." as examples) Yes, .. and . work the same way, except that they cannot appear in the \\server\share part. I also think that .. does not cancel these parts. As long as you use is_absolute_path() and do not simplify path components before offset_1st_component(), you should be on the safe side. Since I don't really have a windows machine to test things it might be slightly difficult to get everything correct quickly but hopefully we can get this working :) I'll lend a hand, of course, as time permits. -- Hannes
Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams wrote: > On 12/07, Duy Nguyen wrote: >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams wrote: >> > Convert 'create_simplify()' to use the pathspec struct interface from >> > using the '_raw' entry in the pathspec. >> >> It would be even better to kill this create_simplify() and let >> simplify_away() handle struct pathspec directly. >> >> There is a bug in this code, that might have been found if we >> simpify_away() handled pathspec directly: the memcmp() in >> simplify_away() will not play well with :(icase) magic. My bad. If >> :(icase) is used, the easiest/safe way is simplify nothing. Later on >> maybe we can teach simplify_away() to do strncasecmp instead. We could >> ignore exclude patterns there too (although not excluding is not a >> bug). > > So are you implying that the simplify struct needs to be killed? That > way the pathspec struct itself is being passed around instead? Yes. simplify struct was a thing when pathspec was an array of char *. At this point I think it can retire (when we have time to retire it) -- Duy
Re: [PATCH 01/17] mv: convert to using pathspec struct interface
On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams wrote:
>> > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char
>> > *prefix,
>> > {
>> > int i;
>> > const char **result;
>> > + struct pathspec ps;
>> > ALLOC_ARRAY(result, count + 1);
>> > - COPY_ARRAY(result, pathspec, count);
>> > - result[count] = NULL;
>> > +
>> > + /* Create an intermediate copy of the pathspec based on the flags
>> > */
>> > for (i = 0; i < count; i++) {
>> > - int length = strlen(result[i]);
>> > + int length = strlen(pathspec[i]);
>> > int to_copy = length;
>> > + char *it;
>> > while (!(flags & KEEP_TRAILING_SLASH) &&
>> > - to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
>> > + to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
>> > to_copy--;
>> > - if (to_copy != length || flags & DUP_BASENAME) {
>> > - char *it = xmemdupz(result[i], to_copy);
>> > - if (flags & DUP_BASENAME) {
>> > - result[i] = xstrdup(basename(it));
>> > - free(it);
>> > - } else
>> > - result[i] = it;
>> > - }
>> > +
>> > + it = xmemdupz(pathspec[i], to_copy);
>> > + if (flags & DUP_BASENAME) {
>> > + result[i] = xstrdup(basename(it));
>> > + free(it);
>> > + } else
>> > + result[i] = it;
>> > + }
>> > + result[count] = NULL;
>> > +
>> > + parse_pathspec(&ps,
>> > + PATHSPEC_ALL_MAGIC &
>> > + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
>> > + PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
>> > + prefix, result);
>> > + assert(count == ps.nr);
>> > +
>> > + /* Copy the pathspec and free the old intermediate strings */
>> > + for (i = 0; i < count; i++) {
>> > + const char *match = xstrdup(ps.items[i].match);
>> > + free((char *) result[i]);
>> > + result[i] = match;
>>
>> Sigh.. it looks so weird that we do all the parsing (in a _copy_
>> pathspec function) then remove struct pathspec and return the plain
>> string. I guess we can't do anything more until we rework cmd_mv code
>> to handle pathspec natively.
>>
>> At the least I think we should rename this function to something else.
>> But if you have time I really wish we could kill this function. I
>> haven't stared at cmd_mv() long and hard, but it looks to me that we
>> combining two separate functionalities in the same function here.
>>
>> If "mv" takes n arguments, then the first arguments may be
>> pathspec, the last one is always a plain path. The "dest_path =
>> internal_copy_pathspec..." could be as simple as "dest_path =
>> prefix_path(argv[argc - 1])". the special treatment for this last
>> argument [1] can live here. Then, we can do parse_pathspec for the
>> arguments in cmd_mv(). It's still far from perfect, because
>> cmd_mv can't handle pathspec properly, but it reduces the messy mess
>> in internal_copy_pathspec a bit, I hope.
>>
>
> Actually, after looking at this a bit more it seems like we could
> technically use prefix_path for both source and dest (based on how the
> current code is structured) since the source's provied must all exist (as
> in no wildcards are allowed). We could drop using the pathspec struct
> completely in addition to renaming the function (to what I'm still
> unsure).
Yeah that sounds good too (with a caveat: I'm not a heavy user of
git-mv nor touching this code a lot, I might be missing something).
It'll take some looong time before somebody starts converting it to
use pathspec properly, I guess. prefix_path() would keep the code
clean meanwhile.
> I agree that this code should probably be rewritten and made a
> bit cleaner, I don't know if that fits in the scope of this series or
> should be done as a followup patch. If you think it fits here then I
> can try and find some time to do the rework.
--
Duy
Re: [PATCHv5 5/5] submodule: add embed-git-dir function
On Thu, Dec 8, 2016 at 4:01 AM, Stefan Beller wrote:
> +/*
> + * Migrate the git directory of the given `path` from `old_git_dir` to
> + * `new_git_dir`. If an error occurs, append it to `err` and return the
> + * error code.
> + */
> +int relocate_gitdir(const char *path, const char *old_git_dir,
> + const char *new_git_dir, const char *displaypath,
> + struct strbuf *err)
> +{
> + int ret = 0;
> +
> + printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
> + displaypath, old_git_dir, new_git_dir);
I'm going to nag you about _() until your fingers automatically type
"_(" after "(" :-D
--
Duy
Re: [PATCHv6 7/7] submodule: add absorb-git-dir function
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:
> diff --git a/dir.c b/dir.c
> index 8b74997c66..cc5729f733 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2774,3 +2774,15 @@ void connect_work_tree_and_git_dir(const char
> *work_tree, const char *git_dir)
> free(real_work_tree);
> free(real_git_dir);
> }
> +
> +/*
> + * Migrate the git directory of the given path from old_git_dir to
> new_git_dir.
> + */
> +void relocate_gitdir(const char *path, const char *old_git_dir, const char
> *new_git_dir)
> +{
> + if (rename(old_git_dir, new_git_dir) < 0)
> + die_errno(_("could not migrate git directory from '%s' to
> '%s'"),
> + old_git_dir, new_git_dir);
> +
> + connect_work_tree_and_git_dir(path, new_git_dir);
> +}
Thank you!
--
Duy
Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use
On Thu, Dec 8, 2016 at 5:40 PM, Duy Nguyen wrote:
> Alternatively, we could add a new flag to get_worktrees() to tell it
> to return all worktrees if there is a least one linked worktree, or
> return NULL if there's only main worktree. I'm not sure if this is
> clever or very stupid.
No, this may be better. Add a flag to say "returns linked worktrees
_only_". Which means when you're in a "normal" repo, get_worktrees()
with this flag returns NULL. When you're in a multiple-worktree repo,
it returns all linked worktrees (no main worktree). I think I might
have a use for this flag in addition to this uses_worktrees() here.
uses_worktrees() look quite simple with that flag
int uses_worktrees(void)
{
struct worktree **worktrees = get_worktrees(WT_LINKED_ONLY);
int retval = worktrees != NULL;
free_worktrees(worktrees);
return retval;
}
--
Duy
Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use
On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
> worktree.c | 24
> worktree.h | 7 +++
> 2 files changed, 31 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index 75db689672..2559f33846 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -406,3 +406,27 @@ const struct worktree *find_shared_symref(const char
> *symref,
>
> return existing;
> }
> +
> +static int uses_worktree_internal(struct worktree **worktrees)
> +{
> + int i;
> + for (i = 0; worktrees[i]; i++)
> + ; /* nothing */
> +
> + free_worktrees(worktrees);
Ayy.. caller allocates, callee frees. This might become a new
maintenance nightmare. Elsewhere I believe we (Junio and me) discussed
the possibility of returning the number of worktrees from
get_worktrees() too. get_worktrees() would take an "int *", if not
NULL, we return the number of worktrees in that pointer.
It's probably a better approach, although I'm afraid it'll add a bit
more work on you.
Alternatively, we could add a new flag to get_worktrees() to tell it
to return all worktrees if there is a least one linked worktree, or
return NULL if there's only main worktree. I'm not sure if this is
clever or very stupid.
> + return i > 1;
> +}
> +
> +int uses_worktrees(void)
"has" may be a better verb than "uses". maybe "has_linked_worktrees"
since we always have and use the main worktree.
> +{
> + return uses_worktree_internal(get_worktrees(0));
> +}
> +
> +int submodule_uses_worktrees(const char *path)
> +{
> + struct worktree **worktrees = get_submodule_worktrees(path, 0);
> + if (!worktrees)
> + return 0;
> +
> + return uses_worktree_internal(worktrees);
> +}
> diff --git a/worktree.h b/worktree.h
> index 157fbc4a66..76027b1fd2 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -33,6 +33,13 @@ extern struct worktree **get_worktrees(unsigned flags);
> extern struct worktree **get_submodule_worktrees(const char *path,
> unsigned flags);
>
> +/*
> + * Returns 1 if more than one worktree exists.
> + * Returns 0 if only the main worktree exists.
> + */
> +extern int uses_worktrees(void);
> +extern int submodule_uses_worktrees(const char *path);
> +
> /*
> * Return git dir of the worktree. Note that the path may be relative.
> * If wt is NULL, git dir of current worktree is returned.
> --
> 2.11.0.rc2.30.gc512cbd.dirty
>
--
Duy
