[WIP/PATCH 0/5] git checkout --recurse-submodules
Hi, This patch series comes from https://github.com/jlehmann/git-submod-enhancements branch recursive_submodule_checkout. It needed some tiny tweaks to apply to current master and build without warnings, but nothing major, and I haven't sanity checked it much beyond that and letting the kind folks that use Debian experimental play with it. I'm sending it out now to get review and ideas for what needs to happen next to get this series in shape to be included in git.git. Thoughts of all kinds welcome. Thanks, Jonathan Jens Lehmann (5): submodule: prepare for recursive checkout of submodules submodule: teach unpack_trees() to remove submodule contents submodule: teach unpack_trees() to repopulate submodules submodule: teach unpack_trees() to update submodules Teach checkout to recursively checkout submodules Documentation/git-checkout.txt | 8 ++ builtin/checkout.c | 14 +++ entry.c| 19 +++- submodule.c| 217 - submodule.h| 11 +++ t/t2013-checkout-submodule.sh | 215 +++- unpack-trees.c | 94 ++ unpack-trees.h | 1 + wrapper.c | 3 + 9 files changed, 556 insertions(+), 26 deletions(-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] submodule: teach unpack_trees() to remove submodule contents
From: Jens Lehmann jens.lehm...@web.de
Date: Tue, 19 Jun 2012 20:55:45 +0200
Implement the functionality needed to enable work tree manipulating
commands to that a deleted submodule should not only affect the index
(leaving all the files of the submodule in the work tree) but also to
remove the work tree of the superproject (including any untracked
files).
That will only work properly when the submodule uses a gitfile instead of
a .git directory and no untracked files are present. Otherwise the removal
will fail with a warning (which is just what happened until now).
Extend rmdir_or_warn() to remove the directories of those submodules which
are scheduled for removal. Also teach verify_clean_submodule() to check
that a submodule configured to be removed is not modified before scheduling
it for removal.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Should this share some code (or just the error message) with the 'git
rm' code that checks whether a submodule is safe to remove?
rmdir_or_warn is a pretty low-level function --- it feels odd to be
relying on the git repository layout here. On the other hand, that
function only has two callers, so it is possible to check quickly
whether it is safe.
I assume this is mostly for the sake of the caller in unpack-trees?
In builtin/apply.c, remove_file is used for deletion and rename
patches. Do we want this logic take effect there, too?
submodule.c| 37 +
submodule.h| 1 +
unpack-trees.c | 6 +++---
wrapper.c | 3 +++
4 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/submodule.c b/submodule.c
index 3f18d4d..a25db46 100644
--- a/submodule.c
+++ b/submodule.c
@@ -412,6 +412,43 @@ int submodule_needs_update(const char *path)
return config_update_recurse_submodules != RECURSE_SUBMODULES_OFF;
}
+int depopulate_submodule(const char *path)
+{
+ struct strbuf dot_git = STRBUF_INIT;
+ struct child_process cp;
+ const char *argv[] = {rm, -rf, path, NULL};
+
+ /* Is it populated? */
+ strbuf_addf(dot_git, %s/.git, path);
+ if (!resolve_gitdir(dot_git.buf)) {
+ strbuf_release(dot_git);
+ return 0;
+ }
+ strbuf_release(dot_git);
+
+ /* Does it have a .git directory? */
+ if (!submodule_uses_gitfile(path)) {
+ warning(_(cannot remove submodule '%s' because it (or one of
+ its nested submodules) uses a .git directory),
+ path);
+ return -1;
+ }
+
+ /* Remove the whole submodule directory */
+ memset(cp, 0, sizeof(cp));
+ cp.argv = argv;
+ cp.env = local_repo_env;
+ cp.git_cmd = 0;
+ cp.no_stdin = 1;
+ if (run_command(cp)) {
+ warning(Could not remove submodule %s, path);
+ strbuf_release(dot_git);
+ return -1;
+ }
+
+ return 0;
+}
+
void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
unsigned char one[20], unsigned char two[20],
diff --git a/submodule.h b/submodule.h
index 055918c..df291cf 100644
--- a/submodule.h
+++ b/submodule.h
@@ -24,6 +24,7 @@ void handle_ignore_submodules_arg(struct diff_options
*diffopt, const char *);
int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
int submodule_needs_update(const char *path);
+int depopulate_submodule(const char *path);
void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
unsigned char one[20], unsigned char two[20],
diff --git a/unpack-trees.c b/unpack-trees.c
index ad3e9a0..89b506a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -8,6 +8,7 @@
#include progress.h
#include refs.h
#include attr.h
+#include submodule.h
/*
* Error messages expected by scripts out of plumbing commands such as
@@ -1263,14 +1264,13 @@ static void invalidate_ce_path(const struct cache_entry
*ce,
/*
* Check that checking out ce-sha1 in subdir ce-name is not
* going to overwrite any working files.
- *
- * Currently, git does not checkout subprojects during a superproject
- * checkout, so it is not going to overwrite anything.
*/
static int verify_clean_submodule(const struct cache_entry *ce,
enum unpack_trees_error_types error_type,
struct unpack_trees_options *o)
{
+ if (submodule_needs_update(ce-name) is_submodule_modified(ce-name,
0))
+ return 1;
return 0;
}
diff --git a/wrapper.c b/wrapper.c
index 0cc5636..425a3fd 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -2,6 +2,7 @@
* Various trivial helper wrappers around standard functions
*/
#include cache.h
+#include submodule.h
static void do_nothing(size_t size
[PATCH 3/5] submodule: teach unpack_trees() to repopulate submodules
From: Jens Lehmann jens.lehm...@web.de
Date: Mon, 18 Jun 2012 22:11:45 +0200
Signed-off-by: Jens Lehmann jens.lehm...@web.de
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Neat. Would probably be clearer with some of the corresponding tests
squashed in to illustrate the intended behavior.
entry.c| 4
submodule.c| 44 +---
submodule.h| 1 +
unpack-trees.c | 19 +++
4 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/entry.c b/entry.c
index 7b7aa81..d1bf6ec 100644
--- a/entry.c
+++ b/entry.c
@@ -2,6 +2,7 @@
#include blob.h
#include dir.h
#include streaming.h
+#include submodule.h
static void create_directories(const char *path, int path_len,
const struct checkout *state)
@@ -203,6 +204,9 @@ static int write_entry(struct cache_entry *ce,
return error(cannot create temporary submodule %s,
path);
if (mkdir(path, 0777) 0)
return error(cannot create submodule directory %s,
path);
+ if (submodule_needs_update(path)
+ populate_submodule(path, ce-sha1, state-force))
+ return error(cannot checkout submodule %s, path);
break;
default:
return error(unknown file mode for %s in index, path);
diff --git a/submodule.c b/submodule.c
index a25db46..06df5ae 100644
--- a/submodule.c
+++ b/submodule.c
@@ -412,6 +412,42 @@ int submodule_needs_update(const char *path)
return config_update_recurse_submodules != RECURSE_SUBMODULES_OFF;
}
+int populate_submodule(const char *path, unsigned char sha1[20], int force)
+{
+ struct string_list_item *path_option;
+ const char *name, *real_git_dir;
+ struct strbuf buf = STRBUF_INIT;
+ struct child_process cp;
+ const char *argv[] = {read-tree, force ? --reset : -m, -u,
NULL, NULL};
+
+ path_option = unsorted_string_list_lookup(config_name_for_path, path);
+ if (!path_option)
+ return 0;
+
+ name = path_option-util;
+
+ strbuf_addf(buf, %s/modules/%s, resolve_gitdir(get_git_dir()), name);
+ real_git_dir = resolve_gitdir(buf.buf);
+ if (!real_git_dir)
+ goto out;
+ connect_work_tree_and_git_dir(path, real_git_dir);
+
+ /* Run read-tree --reset sha1 */
+ memset(cp, 0, sizeof(cp));
+ cp.argv = argv;
+ cp.env = local_repo_env;
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.dir = path;
+ argv[3] = sha1_to_hex(sha1);
+ if (run_command(cp))
+ warning(_(Checking out submodule %s failed), path);
+
+out:
+ strbuf_release(buf);
+ return 0;
+}
+
int depopulate_submodule(const char *path)
{
struct strbuf dot_git = STRBUF_INIT;
@@ -1207,6 +1243,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_git_dir = xstrdup(real_path(git_dir));
const char *real_work_tree = xstrdup(real_path(work_tree));
FILE *fp;
@@ -1215,15 +1252,15 @@ void connect_work_tree_and_git_dir(const char
*work_tree, const char *git_dir)
fp = fopen(file_name.buf, w);
if (!fp)
die(_(Could not create git link %s), file_name.buf);
- fprintf(fp, gitdir: %s\n, relative_path(git_dir, real_work_tree,
+ fprintf(fp, gitdir: %s\n, relative_path(real_git_dir, real_work_tree,
rel_path));
fclose(fp);
/* Update core.worktree setting */
strbuf_reset(file_name);
- strbuf_addf(file_name, %s/config, git_dir);
+ strbuf_addf(file_name, %s/config, real_git_dir);
if (git_config_set_in_file(file_name.buf, core.worktree,
- relative_path(real_work_tree, git_dir,
+ relative_path(real_work_tree, real_git_dir,
rel_path)))
die(_(Could not set core.worktree in %s),
file_name.buf);
@@ -1231,4 +1268,5 @@ void connect_work_tree_and_git_dir(const char *work_tree,
const char *git_dir)
strbuf_release(file_name);
strbuf_release(rel_path);
free((void *)real_work_tree);
+ free((void *)real_git_dir);
}
diff --git a/submodule.h b/submodule.h
index df291cf..3657ca8 100644
--- a/submodule.h
+++ b/submodule.h
@@ -24,6 +24,7 @@ void handle_ignore_submodules_arg(struct diff_options
*diffopt, const char *);
int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
int submodule_needs_update(const char *path);
+int populate_submodule(const char *path, unsigned char sha1[20], int force);
int depopulate_submodule(const
[PATCH 4/5] submodule: teach unpack_trees() to update submodules
From: Jens Lehmann jens.lehm...@web.de
Date: Fri, 5 Apr 2013 18:35:27 +0200
Signed-off-by: Jens Lehmann jens.lehm...@web.de
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Also neat, also would benefit from documentation or tests.
entry.c| 15 --
submodule.c| 86 ++
submodule.h| 3 ++
unpack-trees.c | 69 --
unpack-trees.h | 1 +
5 files changed, 157 insertions(+), 17 deletions(-)
diff --git a/entry.c b/entry.c
index d1bf6ec..61a2767 100644
--- a/entry.c
+++ b/entry.c
@@ -265,7 +265,7 @@ int checkout_entry(struct cache_entry *ce,
if (!check_path(path, len, st, state-base_dir_len)) {
unsigned changed = ce_match_stat(ce, st,
CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
- if (!changed)
+ if (!changed (!S_ISDIR(st.st_mode) ||
!S_ISGITLINK(ce-ce_mode)))
return 0;
if (!state-force) {
if (!state-quiet)
@@ -280,9 +280,18 @@ int checkout_entry(struct cache_entry *ce,
* just do the right thing)
*/
if (S_ISDIR(st.st_mode)) {
- /* If it is a gitlink, leave it alone! */
- if (S_ISGITLINK(ce-ce_mode))
+ if (S_ISGITLINK(ce-ce_mode)) {
+ if (submodule_needs_update(ce-name)) {
+ if (is_submodule_populated(ce-name)) {
+ if (update_submodule(ce-name,
ce-sha1, state-force))
+ return error(cannot
checkout submodule %s, path);
+ } else {
+ if (populate_submodule(path,
ce-sha1, state-force))
+ return error(cannot
populate submodule %s, path);
+ }
+ }
return 0;
+ }
if (!state-force)
return error(%s is a directory, path);
remove_subtree(path);
diff --git a/submodule.c b/submodule.c
index 06df5ae..3365987 100644
--- a/submodule.c
+++ b/submodule.c
@@ -485,6 +485,42 @@ int depopulate_submodule(const char *path)
return 0;
}
+int update_submodule(const char *path, const unsigned char sha1[20], int force)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct child_process cp;
+ const char *hex_sha1 = sha1_to_hex(sha1);
+ const char *argv[] = {
+ checkout,
+ force ? -fq : -q,
+ hex_sha1,
+ NULL,
+ };
+ const char *git_dir;
+
+ strbuf_addf(buf, %s/.git, path);
+ git_dir = read_gitfile(buf.buf);
+ if (!git_dir)
+ git_dir = buf.buf;
+ if (!is_directory(git_dir)) {
+ strbuf_release(buf);
+ /* The submodule is not populated, so we can't check it out */
+ return 0;
+ }
+ strbuf_release(buf);
+
+ memset(cp, 0, sizeof(cp));
+ cp.argv = argv;
+ cp.env = local_repo_env;
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.dir = path; /* GIT_WORK_TREE doesn't work for git checkout */
+ if (run_command(cp))
+ return error(Could not checkout submodule %s, path);
+
+ return 0;
+}
+
void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
unsigned char one[20], unsigned char two[20],
@@ -926,6 +962,17 @@ out:
return result;
}
+int is_submodule_populated(const char *path)
+{
+ int retval = 0;
+ struct strbuf gitdir = STRBUF_INIT;
+ strbuf_addf(gitdir, %s/.git, path);
+ if (resolve_gitdir(gitdir.buf))
+ retval = 1;
+ strbuf_release(gitdir);
+ return retval;
+}
+
unsigned is_submodule_modified(const char *path, int ignore_untracked)
{
ssize_t len;
@@ -1075,6 +1122,45 @@ int ok_to_remove_submodule(const char *path)
return ok_to_remove;
}
+unsigned is_submodule_checkout_safe(const char *path, const unsigned char
sha1[20])
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct child_process cp;
+ const char *hex_sha1 = sha1_to_hex(sha1);
+ const char *argv[] = {
+ read-tree,
+ -n,
+ -m,
+ HEAD,
+ hex_sha1,
+ NULL,
+ };
+ const char *git_dir;
+
+ strbuf_addf(buf, %s/.git, path);
+ git_dir = read_gitfile(buf.buf);
+ if (!git_dir)
+ git_dir = buf.buf;
+ if (!is_directory(git_dir)) {
+ strbuf_release(buf);
+ /* The submodule
[PATCH 5/5] Teach checkout to recursively checkout submodules
From: Jens Lehmann jens.lehm...@web.de
Date: Wed, 13 Jun 2012 18:50:10 +0200
Signed-off-by: Jens Lehmann jens.lehm...@web.de
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
This is the patch that actually introduces the --recurse-submodules
option, which makes the rest work.
The tests indicate some future directions for improving this, but
it works reasonably well already. I think I'd be most comfortable
applying these if they were rearranged a little to the following
order:
1. First, introducing the --recurse-submodules option, perhaps
with no actual effect, with tests that it is parsed correctly,
the default works, etc.
2. Then, adding the desired behaviors one by one with corresponding
tests (handling submodule modifications, removals, additions).
3. Finally, any needed tweaks on top.
That way, it is easy to play around with early patches without
worrying about the later ones at first, and the patches are easy
to review to confirm that they at least don't break anything in
the --no-recurse-submodules case.
That said, Debian experimental has these applied as is already,
and there haven't been any complaints yet.
Thoughts?
Jonathan
Documentation/git-checkout.txt | 8 ++
builtin/checkout.c | 14 +++
submodule.c| 14 +++
submodule.h| 3 +
t/t2013-checkout-submodule.sh | 215 -
5 files changed, 251 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 91294f8..aabcc65 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -225,6 +225,14 @@ This means that you can use `git checkout -p` to
selectively discard
edits from your current working tree. See the ``Interactive Mode''
section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
+--[no-]recurse-submodules::
+ Using --recurse-submodules will update the content of all initialized
+ submodules according to the commit recorded in the superproject.If
+ local modifications in a submodule would be overwritten the checkout
+ will fail until `-f` is used. If nothing (or --no-recurse-submodules)
+ is used, the work trees of submodules will not be updated, only the
+ hash recorded in the superproject will be changed.
+
branch::
Branch to checkout; if it refers to a branch (i.e., a name that,
when prepended with refs/heads/, is a valid ref), then that
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..ac2f8d8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -21,6 +21,9 @@
#include submodule.h
#include argv-array.h
+static const char *recurse_submodules_default = off;
+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+
static const char * const checkout_usage[] = {
N_(git checkout [options] branch),
N_(git checkout [options] [branch] -- file...),
@@ -,6 +1114,12 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
N_(do not limit pathspecs to sparse entries only)),
OPT_HIDDEN_BOOL(0, guess, dwim_new_local_branch,
N_(second guess 'git checkout
no-such-branch')),
+ { OPTION_CALLBACK, 0, recurse-submodules, recurse_submodules,
+ checkout, control recursive updating of
submodules,
+ PARSE_OPT_OPTARG, option_parse_update_submodules },
+ { OPTION_STRING, 0, recurse-submodules-default,
+ recurse_submodules_default, NULL,
+ default mode for recursion, PARSE_OPT_HIDDEN },
OPT_END(),
};
@@ -1132,6 +1141,11 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
git_xmerge_config(merge.conflictstyle, conflict_style, NULL);
}
+ set_config_update_recurse_submodules(
+
parse_fetch_recurse_submodules_arg(--recurse-submodules-default,
+ recurse_submodules_default),
+ recurse_submodules);
+
if ((!!opts.new_branch + !!opts.new_branch_force +
!!opts.new_orphan_branch) 1)
die(_(-b, -B and --orphan are mutually exclusive));
diff --git a/submodule.c b/submodule.c
index 3365987..bdce1b2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -398,6 +398,20 @@ int parse_update_recurse_submodules_arg(const char *opt,
const char *arg)
}
}
+int option_parse_update_submodules(const struct option *opt,
+ const char *arg, int unset)
+{
+ if (unset) {
+ *(int *)opt-value = RECURSE_SUBMODULES_OFF;
+ } else {
+ if (arg)
+ *(int *)opt-value =
parse_update_recurse_submodules_arg(opt-long_name, arg);
+ else
+ *(int *)opt-value
Re: [PATCH] add: don't complain when adding empty project root
Hi,
Nguyễn Thái Ngọc Duy wrote:
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Thanks.
[...]
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -544,7 +544,7 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
for (i = 0; i pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
- if (!seen[i]
+ if (!seen[i] pathspec.items[i].match[0]
((pathspec.items[i].magic
(PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
!file_exists(path))) {
Nit: in this loop there's already the synonym 'path' for item.match,
so perhaps
if (!seen[i] path[0] ...)
would be clearer.
Should git add --refresh . get the same treatment?
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -307,4 +307,8 @@ test_expect_success 'git add --dry-run --ignore-missing
of non-existing file out
test_i18ncmp expect.err actual.err
'
+test_expect_success 'git add -A on empty repo does not error out' '
+ git init empty ( cd empty git add -A . )
+'
Adding a test at the end like this means the tests come in chronological
order instead of logical order and simultaneous patches to the same
test script become more likely to conflict.
How about something like the following, for squashing in?
With or without the tweaks below,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/builtin/add.c w/builtin/add.c
index fbd3f3a..d7e3e44 100644
--- i/builtin/add.c
+++ w/builtin/add.c
@@ -544,7 +544,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
for (i = 0; i pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
- if (!seen[i] pathspec.items[i].match[0]
+ if (!seen[i] path[0]
((pathspec.items[i].magic
(PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
!file_exists(path))) {
diff --git i/t/t3700-add.sh w/t/t3700-add.sh
index 1535d8f..fe274e2 100755
--- i/t/t3700-add.sh
+++ w/t/t3700-add.sh
@@ -272,6 +272,25 @@ test_expect_success 'add non-existent should fail' '
! (git ls-files | grep non-existent)
'
+test_expect_success 'git add -A on empty repo does not error out' '
+ rm -fr empty
+ git init empty
+ (
+ cd empty
+ git add -A .
+ git add -A
+ )
+'
+
+test_expect_success 'git add . in empty repo' '
+ rm -fr empty
+ git init empty
+ (
+ cd empty
+ git add .
+ )
+'
+
test_expect_success 'git add --dry-run of existing changed file'
echo new track-this
git add --dry-run track-this actual 21
@@ -307,8 +326,4 @@ test_expect_success 'git add --dry-run --ignore-missing of
non-existing file out
test_i18ncmp expect.err actual.err
'
-test_expect_success 'git add -A on empty repo does not error out' '
- git init empty ( cd empty git add -A . )
-'
-
test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] cat-file --batch-check='%(deltabase)'
Jeff King wrote: I needed this recently to write tests for another (not yet published) series. But I think it stands on its own as a debugging / introspection tool. [1/2]: sha1_object_info_extended: provide delta base sha1s [2/2]: cat-file: provide %(deltabase) batch format Neat. The error handling looks right. Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: workaround for a bug in svn serf backend
Roman Kagan wrote: Subversion serf backend in versions 1.8.5 and below has a bug that the function creating the descriptor of a file change -- add_file() -- doesn't make a copy of its 3d argument when storing it on the returned 3d makes me think of 3-dimensional. ;-) I think you mean third (or the abbreviation 3rd). descriptor. As a result, by the time this field is used (in transactions of file copying or renaming) it may well be released. Please describe the symptom so this patch is easy to find when other people run into it. Do I remember correctly that ... released and scribbled over with a new value, causing such-and-such assertion to fire was what happened? This patch works around this bug, by storing the value to be passed as the 3d argument to add_file() in a local variable with the same scope as the file change descriptor, making sure their lifetime is the same. Could this be reproduced with a test script to make sure we don't reintroduce the bug again later? (It's okay if the test only fails on machines with the problematic svn version.) Modulo the confusing 3-dimensional arguments in comments, the code change looks good. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] safe_create_leading_directories(): modernize format of if chaining
Michael Haggerty wrote: [Subject: safe_create_leading_directories(): modernize format of if chaining] Trivia: it's not so much modernizing as following KR style, which git more or less followed since day 1. Linux's Documentation/CodingStyle explains: Note that the closing brace is empty on a line of its own, _except_ in the cases where it is followed by a continuation of the same statement, ie a while in a do-statement or an else in an if-statement, like this: [...] Rationale: KR. Also, note that this brace-placement also minimizes the number of empty (or almost empty) lines, without any loss of readability. Thus, as the supply of new-lines on your screen is not a renewable resource (think 25-line terminal screens here), you have more empty lines to put comments on. Here it's especially jarring since the function uses a mix of styles. Thanks for cleaning it up. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] safe_create_leading_directories(): reduce scope of local variable
Michael Haggerty wrote:
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
sha1_file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sha1_file.c b/sha1_file.c
index c9245a6..cc9957e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -108,9 +108,10 @@ int mkdir_in_gitdir(const char *path)
int safe_create_leading_directories(char *path)
{
char *pos = path + offset_1st_component(path);
- struct stat st;
while (pos) {
+ struct stat st;
Is this to make it easier to reason about whether 'st' has been
properly initialized at any given moment, or is there a more subtle
reason?
Curious,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] safe_create_leading_directories(): add slash pointer
Michael Haggerty wrote:
[Subject: safe_create_leading_directories(): add slash pointer]
Is this a cleanup or improving the (internal) functionality of the
function somehow? The above one-liner doesn't sum up for me in an
obvious way why this is a good change.
Keep track of the position of the slash character separately, and
Separately from what?
restore the slash character at a single place, at the end of the while
loop. This makes the next change easier to implement.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
Ah, do I understand correctly that this is about cleaning up
after the code that scribbles over 'path' in one place, to make
it harder to forget to do that cleanup as new code paths are
introduced?
It's too bad there's no variant of 'stat' and 'mkdir' that takes
a (buf, len) pair which would avoid the scribbling altogether.
---
sha1_file.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index cc9957e..dcfd35a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -107,40 +107,40 @@ int mkdir_in_gitdir(const char *path)
int safe_create_leading_directories(char *path)
{
- char *pos = path + offset_1st_component(path);
+ char *next_component = path + offset_1st_component(path);
This name change is probably worth also mentioning in the commit
message (or lifting into a separate patch) so the reader doesn't get
distracted.
+ int retval = 0;
- while (pos) {
+ while (!retval next_component) {
A more usual style would be
int ... = 0;
while (pos) {
...
if (!stat(path, st)) {
/* path exists */
if (!S_ISDIR(st.st_mode)) {
... = -3;
goto out;
}
} else if (...) {
...
}
...
}
out:
*slash = '/';
return ...;
}
which makes it more explicit that the slash needs to be written back.
In this example, that would look like:
char *slash = NULL;
int ret;
while (pos) {
...
if (!slash)
break;
...
if (!*pos)
break;
*slash = '\0';
if (!stat(path, st)) {
if (!S_ISDIR(st.st_mode)) {
ret = -3;
goto out;
}
} else if (mkdir(...)) {
if (errno == EEXIST ...) {
; /* ok */
} else {
ret = -1;
goto out;
}
} else if (adjust_shared_perm(...)) {
ret = -2;
goto out;
}
*slash = '/';
}
ret = 0;
out:
if (slash)
*slash = '/';
return ret;
Using retval for control flow instead makes it eight lines more
concise, which is probably worth it.
[...]
if (!S_ISDIR(st.st_mode)) {
- *pos = '/';
- return -3;
+ retval = -3;
}
Now the 'if' body is one line, so we can drop the braces and save
another line. :)
One more nit: elsewhere in this file, a variable keeping track of the
return value is named 'ret', so it probably makes sense to also use
that name here.
That would mean the following changes to be potentially squashed in
(keeping 'pos' name to make the patch easier to read, s/retval/ret/,
removing unnecessary braces). None of these tweaks are particularly
important. Feel free to skip them --- the only comment I've made that
matters is about the commit message.
Thanks for a nice cleanup,
Jonathan
diff --git i/sha1_file.c w/sha1_file.c
index dcfd35a..18cb50a 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -107,40 +107,38 @@ int mkdir_in_gitdir(const char *path)
int safe_create_leading_directories(char *path)
{
- char *next_component = path + offset_1st_component(path);
- int retval = 0;
+ char *pos = path + offset_1st_component(path);
+ int ret = 0;
- while (!retval next_component) {
+ while (!ret pos) {
struct stat st;
- char *slash = strchr(next_component, '/');
+ char *slash = strchr(pos, '/');
if (!slash)
return 0;
while (*(slash + 1) == '/')
slash++;
- next_component = slash + 1;
- if (!*next_component)
+ pos = slash + 1;
+ if (!*pos)
return
Re: [PATCH 4/5] safe_create_leading_directories(): fix a mkdir/rmdir race
Hi, Michael Haggerty wrote: It could be that some other process is trying to clean up empty directories at the same time that safe_create_leading_directories() is attempting to create them. In this case, it could happen that directory a/b was present at the end of one iteration of the loop (either it was already present or we just created it ourselves), but by the time we try to create directory a/b/c, directory a/b has been deleted. In fact, directory a might also have been deleted. When does this happen in practice? Is this about git racing with itself or with some other program? I fear that the aggressive directory creator fighting the aggressive directory remover might be waging a losing battle. Is this about a push that creates a ref racing against a push that deletes a ref from the same hierarchy? So, if a call to mkdir() fails with ENOENT, then try checking/making all directories again from the beginning. Attempt up to three times before giving up. If we are racing against a ref deletion, then we can retry while our rival keeps walking up the directory tree and deleting parent directories. As soon as we successfully create a directory, we have won the race. But what happens if the entire safe_create_leading_directories operation succeeds and *then* our racing partner deletes the directory? No one is putting in a file to reserve the directory for the directory creator. If we care enough to retry more than once, I fear this is retrying at the wrong level. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- sha1_file.c | 11 +++ 1 file changed, 11 insertions(+) Tests? The code is clear and implements the design correctly. Thanks for some food for thought, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] rename_ref(): fix a mkdir()/rmdir() race
Michael Haggerty wrote:
refs.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
A test or example reproduction recipe would be nice. (But I can
understand not having one --- races are hard to test.)
[...]
--- a/refs.c
+++ b/refs.c
[...]
@@ -2574,6 +2575,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
goto retry;
} else {
+ if (errno == ENOENT --attempts)
+ /*
+ * Perhaps somebody just pruned the empty
+ * directory into which we wanted to move the
+ * file.
+ */
+ goto retry;
Style nit: it's easier to read a test of errno when the 'else's
cascade (i.e., using 'else if' here).
This patch doesn't depend on any of the others from the series. For
what it's worth, with or without the following squashed in,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Thanks.
diff --git i/refs.c w/refs.c
index 3ab1491..ea62395 100644
--- i/refs.c
+++ w/refs.c
@@ -2574,14 +2574,14 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
goto retry;
+ } else if (errno == ENOENT --attempts)
+ /*
+* Perhaps somebody just pruned the empty
+* directory into which we wanted to move the
+* file.
+*/
+ goto retry;
} else {
- if (errno == ENOENT --attempts)
- /*
-* Perhaps somebody just pruned the empty
-* directory into which we wanted to move the
-* file.
-*/
- goto retry;
error(unable to move logfile TMP_RENAMED_LOG to
logs/%s: %s,
newrefname, strerror(errno));
goto rollback;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-svn: workaround for a bug in svn serf backend
Roman Kagan wrote: Subversion serf backend in versions 1.8.5 and below has a bug that the function creating the descriptor of a file change -- add_file() -- doesn't make a copy of its third argument when storing it on the returned descriptor. As a result, by the time this field is used (in transactions of file copying or renaming) it may well be released, and the memory reused. One of its possible manifestations is the svn assertion triggering on an invalid path, with a message svn_fspath__skip_ancestor: Assertion `svn_fspath__is_canonical(child_fspath)' failed. [...] Makes sense. Perhaps also worth mentioning that this is fixed by r1553376, but no need to reroll just for that. Cc: Benjamin Pabst benjamin.pabs...@gmail.com Cc: Eric Wong normalper...@yhbt.net Cc: Jonathan Nieder jrnie...@gmail.com No need for these lines --- the mail header already keeps track of who is being cc-ed. Signed-off-by: Roman Kagan rka...@mail.ru Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove the line length limit for graft files
Hi,
Johannes Schindelin wrote:
While regular commit histories hardly win comprehensibility in general
if they merge more than twenty-two branches in one go, it is not Git's
business to limit grafts in such a way.
Fun. :) Makes sense.
[...]
---
builtin/blame.c | 8
commit.c| 10 +-
2 files changed, 9 insertions(+), 9 deletions(-)
Is this easy to reproduce so some interested but lazy person could
write a test?
[...]
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1804,17 +1804,17 @@ static int prepare_lines(struct scoreboard *sb)
static int read_ancestry(const char *graft_file)
{
FILE *fp = fopen(graft_file, r);
- char buf[1024];
+ struct strbuf buf = STRBUF_INIT;
if (!fp)
return -1;
- while (fgets(buf, sizeof(buf), fp)) {
+ while (!strbuf_getwholeline(buf, fp, '\n')) {
If there is no newline at EOF, this will skip the last line, while the
old behavior was to pay attention to it. I haven't thought through
whether that's a good or bad change. Maybe it should just be
documented?
[...]
--- a/commit.c
+++ b/commit.c
@@ -196,19 +196,19 @@ static int read_graft_file(const char *graft_file)
[...]
- while (fgets(buf, sizeof(buf), fp)) {
+ while (!strbuf_getwholeline(buf, fp, '\n')) {
Likewise.
The rest of the patch looks good.
Merry christmas,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove the line length limit for graft files
Johannes Schindelin wrote: it returns EOF only if ch == EOF *and* sb-len == 0, i.e. if no characters have been read before hitting EOF. Yep. api-strbuf.txt even says so. Sorry for the nonsense. For what it's worth, Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Remove the line length limit for graft files
Johannes Schindelin wrote: On Fri, 27 Dec 2013, Jonathan Nieder wrote: Is this easy to reproduce so some interested but lazy person could write a test? Yep. Make 25 orphan commits, add a graft line to make the first a merge of the rest. Thanks. Here's a pair of tests doing that. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- t/annotate-tests.sh | 21 + t/t6101-rev-parse-parents.sh | 16 +++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index c9d105d..304c7b7 100644 --- a/t/annotate-tests.sh +++ b/t/annotate-tests.sh @@ -116,6 +116,27 @@ test_expect_success 'blame evil merge' ' check_count A 2 B 1 B1 2 B2 1 A U Thor 1 ' +test_expect_success 'blame huge graft' ' + test_when_finished git checkout branch2 + test_when_finished rm -f .git/info/grafts + graft= + for i in 0 1 2 + do + for j in 0 1 2 3 4 5 6 7 8 9 + do + git checkout --orphan $i$j + printf %s\n $i $j file + test_tick + GIT_AUTHOR_NAME=$i$j GIT_AUTHOR_EMAIL=$i$j...@test.git \ + git commit -a -m $i$j + commit=$(git rev-parse --verify HEAD) + graft=$graft$commit + done + done + printf %s $graft .git/info/grafts + check_count -h 00 01 1 10 1 +' + test_expect_success 'setup incomplete line' ' echo incomplete | tr -d \\012 file GIT_AUTHOR_NAME=C GIT_AUTHOR_EMAIL=c...@test.git \ diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh index 7ea14ce..10b1452 100755 --- a/t/t6101-rev-parse-parents.sh +++ b/t/t6101-rev-parse-parents.sh @@ -20,7 +20,17 @@ test_expect_success 'setup' ' test_commit start2 git checkout master git merge -m next start2 - test_commit final + test_commit final + + test_seq 40 | + while read i + do + git checkout --orphan b$i + test_tick + git commit --allow-empty -m $i + commit=$(git rev-parse --verify HEAD) + printf $commit .git/info/grafts + done ' test_expect_success 'start is valid' ' @@ -79,6 +89,10 @@ test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' ' test_cmp expect actual ' +test_expect_success 'large graft octopus' ' + test_cmp_rev_output b31 git rev-parse --verify b1^30 +' + test_expect_success 'repack for next test' ' git repack -a -d ' -- 1.8.5.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] t0000: drop known breakage test
Jeff King wrote: I am not _that_ bothered by the known breakage, but AFAICT there is zero benefit to keeping this redundant test. Devil's advocate: it ensures that anyone wrapping git's tests (like the old smoketest infrastructure experiment) is able to handle an expected failure. But in practice I don't mind the behavior before or after this patch. If the test harness is that broken, we'll know. And people writing code that wraps git's tests can write their own custom sanity-checks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests
Hi, Jeff King wrote: Once upon a time, the test-lib library would create trash directories in the current working directory, unless we were explicitly told to put it elsewhere via --root. As a result, t created the sub-test trash directories inside its own trash directory. However, we noticed that this did not cover all cases, since we would need to respect $TEST_OUTPUT_DIRECTORY even if --root is not given (or is relative). Commit 38b074d fixed this to consistently use the full path. So the idea if I am reading correctly is Instead of relying on the implicit output directory chosen with chdir, which doesn't even work any more, set TEST_OUTPUT_DIRECTORY to decide where output for the sub-tests used by t's sanity checks for the test harness go. I'm not sure I completely understand the regression caused by 38b074d. Is the idea that before that commit, TEST_OUTPUT_DIRECTORY was only used for the test-results/ directory so the only harm done was some mixing of test results? What is the symptom this patch alleviates? As a result, t's sub-tests are now created in git's original test output directory rather than in our trash directory. This might be the source of my confusion. Is sub-tests an abbreviation for sub-test trash directories here? Furthermore, since some of the sub-tests simulate failures, the trash directories do not get cleaned up, and the cruft is left in the t/ directory. We could fix this by passing a new --root=$TRASH_DIRECTORY option to the sub-test. However, we do not want the sub-tests to write anything at all to git's directory (e.g., they should not be writing to t/test-results, either, although this is already handled by separate code). Ah, HARNESS_ACTIVE prevents output of test-results. Does the git test harness write something else to TEST_OUTPUT_DIRECTORY? Is the idea that using --root would be functionally equivalent but (1) more confusing and (2) less futureproof? So the best solution is to simply reset $TEST_OUTPUT_DIRECTORY entirely in the sub-test, which covers this case, as well as any future ones. So, to sum up: if I understand correctly - git used to only use TEST_OUTPUT_DIRECTORY to decide where test results go. You'd have to use --root to set a custom location for trash directories. - in that old setup, t leaves around extra trash directories with --root, since the sub-tests inherit the parent test's $root and put trash directories there. - after 38b074d, that old problem still exists and furthermore t leaves around extra trash directories even when --root is not in use, since the sub-tests inherit the value of TEST_OUTPUT_DIRECTORY from the parent test. - this patch fixes the TEST_OUTPUT_DIRECTORY problem (but not the $root problem) by setting TEST_OUTPUT_DIRECTORY explicitly Does that sound right? If so, should sub-tests unset $root, too? Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] t0000: simplify HARNESS_ACTIVE hack
Jeff King wrote:
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -50,11 +50,11 @@ run_sub_test_lib_test () {
shift 2
mkdir $name
(
- # Pretend we're a test harness. This prevents
- # test-lib from writing the counts to a file that will
- # later be summarized, showing spurious failed tests
- HARNESS_ACTIVE=t
- export HARNESS_ACTIVE
+ # Pretend we're not running under a test harness, whether we
+ # are or not. The test-lib output depends on the setting of
+ # this variable, so we need a stable setting under which to run
+ # the sub-test.
+ sane_unset HARNESS_ACTIVE
Makes sense.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] t0000: set TEST_OUTPUT_DIRECTORY for sub-tests
Jonathan Nieder wrote: - git used to only use TEST_OUTPUT_DIRECTORY to decide where test results go. You'd have to use --root to set a custom location for trash directories. - in that old setup, t leaves around extra trash directories with --root, since the sub-tests inherit the parent test's $root and put trash directories there. Nope, since sub-tests are run with fork + exec, which loses $root... - after 38b074d, that old problem still exists and furthermore t leaves around extra trash directories even when --root is not in use, since the sub-tests inherit the value of TEST_OUTPUT_DIRECTORY from the parent test. ... meaning the TEST_OUTPUT_DIRECTORY problem is the only problem - this patch fixes the TEST_OUTPUT_DIRECTORY problem (but not the $root problem) by setting TEST_OUTPUT_DIRECTORY explicitly Does that sound right? If so, should sub-tests unset $root, too? ... and there's no need to 'unset root'. So the patch itself looks right. I think describing the symptoms up front would probably be enough to make the commit message less confusing to read. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Jeff King wrote: When I want to debug a failing test, I often end up doing: cd t ./t4107-tab -v -i cd tratab The test names are long, so tab-completing on the trash directory is very helpful. Lately I've noticed that there are a bunch of crufty trash directories in my t/ directory, which makes my tab-completion more annoying. Ah, and if I'd read this then I wouldn't have had to be confused at all. Would it work to replace the commit message with something like this? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Jeff King wrote: When I want to debug a failing test, I often end up doing: cd t ./t4107-tab -v -i cd tratab The test names are long, so tab-completing on the trash directory is very helpful. Lately I've noticed that there are a bunch of crufty trash directories in my t/ directory, which makes my tab-completion more annoying. Ah, and if I'd read this then I wouldn't have had to be confused at all. [...] The third paragraph of 1/3 sufficiently covers it, no? We could add It makes it less convenient to use tab completion 'cd t/traTAB' to go to the trash directory of the failed test to inspect the situation after ... left in the t/ directory., though. [4 paragraphs snipped] I think it can be better, since the commit message left me scratching my head while the patch itself seems pretty simple. How about something like the following? First, describing the problem: Running t produces more trash directories than expected and does not clean up after itself: $ ./t-basic.sh [...] $ ls -d trash\ directory.* trash directory.failing-cleanup trash directory.mixed-results1 trash directory.mixed-results2 trash directory.partial-pass trash directory.test-verbose trash directory.test-verbose-only-2 Analysis and fix: These scratch areas for sub-tests should be under the t trash directory, but because the TEST_OUTPUT_DIRECTORY setting from the toplevel test leaks into the environment they are created under the toplevel output directory (typically t/) instead. Because some of the sub-tests simulate failures, their trash directories are kept around. Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately for sub-tests. And then, optionally, describing rejected alternatives: An alternative fix would be to pass the --root parameter that only specifies where to put the trash directories, which would also work. However, using TEST_OUTPUT_DIRECTORY is more futureproof in case tests want to write more output in addition to the test-results/ (which are already suppressed in sub-tests using the HARNESS_ACTIVE setting) and trash directories. And more analysis of why this wasn't caught in the first place: This fixes a regression introduced by 38b074d (t/test-lib.sh: fix TRASH_DIRECTORY handling, 2013-04-14). Before then, the TEST_OUTPUT_DIRECTORY setting was not respected consistently so most tests did their work in a trash subdirectory of the current directory instead of the output dir. Does that make sense? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] Consistently use the term builtin instead of internal command
Hi,
Sebastian Schuberth wrote:
[...]
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -14,7 +14,7 @@ Git:
. Add the external declaration for the function to `builtin.h`.
-. Add the command to `commands[]` table in `handle_internal_command()`,
+. Add the command to `commands[]` table in `handle_builtin()`,
Makes sense. Using consistent jargon makes for easier reading.
[...]
+++ b/git.c
[...]
@@ -563,14 +563,14 @@ int main(int argc, char **av)
[...]
if (starts_with(cmd, git-)) {
cmd += 4;
argv[0] = cmd;
- handle_internal_command(argc, argv);
+ handle_builtin(argc, argv);
- die(cannot handle %s internally, cmd);
+ die(cannot handle %s as a builtin, cmd);
I think this makes the user-visible message less clear.
Before when the user had a stale git-whatever link lingering in
gitexecdir, git would say
fatal: cannot handle whatever internally
which tells me git was asked to handle the whatever command internally
and was unable to. Afterward, it becomes
fatal: cannot handle whatever as a builtin
which requires that I learn the jargon use of builtin as a noun.
busybox's analogous message is applet not found. It's less likely
to come up when using git because it requires having a stray link to
git. A message like
$ git whatever
fatal: whatever: no such built-in command
would just leave me wondering I never claimed it was built-in; what's
going on? I think it would be simplest to keep it as
$ git whatever
fatal: cannot handle whatever internally
which at least makes it clear that this is a low-level error.
The rest of the patch looks good.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] t0000 cleanups
Jeff King wrote: On Mon, Dec 30, 2013 at 10:51:25AM -0800, Jonathan Nieder wrote: These scratch areas for sub-tests should be under the t trash directory, but because the TEST_OUTPUT_DIRECTORY setting from the toplevel test leaks [...] This is not exactly true. The TEST_OUTPUT_DIRECTORY setting does not leak. t sets $TEST_DIRECTORY (which it must, so the sub-scripts can find test-lib.sh and friends), and then TEST_OUTPUT_DIRECTORY uses that as a default if it is not explicitly set. So I should have said something like the following instead: These scratch areas for sub-tests should be under the t trash directory, but because TEST_OUTPUT_DIRECTORY defaults to TEST_DIRECTORY which is exported to help sub-tests find test-lib.sh, the sub-test trash directories are created under the toplevel t/ directory instead. Because some of the sub-tests simulate failures, their trash directories are kept around. Fix it by explicitly setting TEST_OUTPUT_DIRECTORY appropriately for sub-tests. Thanks for catching it. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: [Bug report] 'git status' always says Your branch is up-to-date with 'origin/master'
Hi, Thomas Ackermann wrote: In repo_b your ref for origin/master has not moved. It has remotely (meaning refs/heads/master in repo_a has moved), but git status is not hitting the remote to find out; it only looks at the local state. [...] But for the simple use case where you only have a master branch I consider it not really helpful and - at least for me - misleading. I see what you mean, and you're not the only one. Git follows a rule of never contact another machine unless explicitly asked to using a command such as 'git pull' or 'git fetch'. To support this, it makes a distinction between (1) the remote-tracking ref origin/master and (2) the actual branch master in the remote repository. The former is what is updated by 'git fetch', and the latter is something git does not know about without talking to the remote server. What documentation did you use when first starting to learn git? Perhaps it can be fixed to emphasize the distinction between (1) and (2) earlier. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] format-patch: introduce format.defaultTo
Hi,
Ramkumar Ramachandra wrote:
a plain
$ git format-patch -o outgoing
is a no-op on a topic branch, and the user has to remember to specify
'master' explicitly everytime. Save the user the extra keystrokes by
introducing format.defaultTo
Not excited. Two reasons:
1. Most config settings are in noun form: e.g.,
[remote] pushDefault = foo. That makes their names easy to guess
and makes them easy to talk about: I set the default remote for
pushing by changing the remote.pushdefault setting.
'[url foo] insteadOf' is an exception to that and a bit of an
aberration.
This new '[format] defaultTo' repeats the same end-with-a-preposition
mistake, while I think it would be better to learn from it.
2. Wouldn't a more natural default be @{u}..HEAD instead of relying on
the user to do the make-work of keeping a local branch that tracks
master up to date?
Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] format-patch: introduce format.defaultTo
John Szakmeister wrote:
I think in a
typical, feature branch-based workflow @{u} would be nearly useless.
I thought the idea of @{u} was that it represents which ref one
typically wants to compare the current branch to. It is used by
'git branch -v' to show how far ahead or behind a branch is and
used by 'git pull --rebase' to forward-port a branch, for example.
So a topic branch with @{u} pointing to 'master' or 'origin/master'
seems pretty normal and hopefully the shortcuts it allows can make
life more convenient.
It is *not* primarily about where the branch gets pushed. After all,
in both the 'matching' and the 'simple' mode, git push does not push
the current branch to its upstream @{u} unless @{u} happens to have
the same name.
Hoping that clarifies,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pager: set LV=-c alongside LESS=FRSX
On systems with lv configured as the preferred pager (i.e.,
DEFAULT_PAGER=lv at build time, or PAGER=lv exported in the
environment) git commands that use color show control codes instead of
color in the pager:
$ git diff
^[[1mdiff --git a/.mailfilter b/.mailfilter^[[m
^[[1mindex aa4f0b2..17e113e 100644^[[m
^[[1m--- a/.mailfilter^[[m
^[[1m+++ b/.mailfilter^[[m
^[[36m@@ -1,11 +1,58 @@^[[m
less avoids this problem because git uses the LESS environment
variable to pass the -R option ('output ANSI color escapes in raw
form') by default. Use the LV environment variable to pass 'lv' the
-c option ('allow ANSI escape sequences for text decoration / color')
to fix it for lv, too.
Noticed when the default value for color.ui flipped to 'auto' in
v1.8.4-rc0~36^2~1 (2013-06-10).
Reported-by: Olaf Meeuwissen olaf.meeuwis...@avasys.jp
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Olaf Meeuwissen wrote[1]:
Yes, it's called LV and documented in the lv(1) manual page. Simply
search for 'env' ;-)
Ah, thanks. How about this patch?
[1] http://bugs.debian.org/730527
Documentation/config.txt | 4
git-sh-setup.sh | 3 ++-
pager.c | 11 +--
perl/Git/SVN/Log.pm | 1 +
t/t7006-pager.sh | 12
5 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a405806..ed59853 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -567,6 +567,10 @@ be passed to the shell by Git, which will translate the
final
command to `LESS=FRSX less -+S`. The environment tells the command
to set the `S` option to chop long lines but the command line
resets it to the default to fold long lines.
++
+Likewise, when the `LV` environment variable is unset, Git sets it
+to `-c`. You can override this setting by exporting `LV` with
+another value or setting `core.pager` to `lv +c`.
core.whitespace::
A comma separated list of common whitespace problems to
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 190a539..fffa3c7 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -159,7 +159,8 @@ git_pager() {
GIT_PAGER=cat
fi
: ${LESS=-FRSX}
- export LESS
+ : ${LV=-c}
+ export LESS LV
eval $GIT_PAGER '$@'
}
diff --git a/pager.c b/pager.c
index 345b0bc..0cc75a8 100644
--- a/pager.c
+++ b/pager.c
@@ -80,8 +80,15 @@ void setup_pager(void)
pager_process.use_shell = 1;
pager_process.argv = pager_argv;
pager_process.in = -1;
- if (!getenv(LESS)) {
- static const char *env[] = { LESS=FRSX, NULL };
+ if (!getenv(LESS) || !getenv(LV)) {
+ static const char *env[3];
+ int i = 0;
+
+ if (!getenv(LESS))
+ env[i++] = LESS=FRSX;
+ if (!getenv(LV))
+ env[i++] = LV=-c;
+ env[i] = NULL;
pager_process.env = env;
}
if (start_command(pager_process))
diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
index 3f8350a..34f2869 100644
--- a/perl/Git/SVN/Log.pm
+++ b/perl/Git/SVN/Log.pm
@@ -117,6 +117,7 @@ sub run_pager {
}
open STDIN, '', $rfd or fatal Can't redirect stdin: $!;
$ENV{LESS} ||= 'FRSX';
+ $ENV{LV} ||= '-c';
exec $pager or fatal Can't run pager: $! ($pager);
}
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index ff25908..7fe3367 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -37,6 +37,18 @@ test_expect_failure TTY 'pager runs from subdir' '
test_cmp expected actual
'
+test_expect_success TTY 'LESS and LV envvars are set for pagination' '
+ (
+ sane_unset LESS LV
+ PAGER=env pager-env.out
+ export PAGER
+
+ test_terminal git log
+ )
+ grep ^LESS= pager-env.out
+ grep ^LV= pager-env.out
+'
+
test_expect_success TTY 'some commands do not use a pager' '
rm -f paginated.out
test_terminal git rev-list HEAD
--
1.8.5.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
Hi, Ryan Biesemeyer wrote: In this case it was not immediately clear to me how to add cleanup to an existing test that dirtied the state of the test repository by leaving behind an in-progress merge. I see `test_cleanup` defined in the test lib related functions, but see no examples of its use in the test suites. Could you advise? test_when_finished pushes a command to be run unconditionally when the current test assertion finishes. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
Matthieu Moy wrote: Jonathan's answer is an option. Another one is [...] So if the cleanup goes wrong, one can notice. test_when_finished also makes the test fail if the cleanup failed. Another common strategy is test_expect_success 'my exciting test' ' # this test will rely on these files being absent rm -f a b c etc ... rest of the test goes here ... ' which can be a handy way for an especially picky test to protect itself (for example with 'git clean -fdx') regardless of the state other test assertions create for it. This particular example (merge --abort) seems like a good use for test_when_finished because it is about a specific test having made a mess and needing to clean up after itself to restore sanity. Hoping that clarifies, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A question about the error: svn_fspath__is_canonical
Hi Dan, Dan Kaplan wrote: My environment is probably different from most. I'm using cygwin. This makes it very difficult to use different versions of git/svn/git-svn, but I'm interested in learning git more so I'm willing to try whatever it takes. $ git version git version 1.8.3.4 $ svn --version svn, version 1.8.5 (r1542147) compiled Nov 25 2013, 10:45:07 on x86_64-unknown-cygwin You have three choices: A) upgrade git to latest master B) upgrade subversion to latest trunk C) downgrade subversion to a version before that bug was introduced (A) is probably simplest. E.g., something like the following should work: git clone https://kernel.googlesource.com/pub/scm/git/git.git cd git make -j8 make test; # optional, to verify that the git you built works ok export PATH=$(pwd)/bin-wrappers:$PATH Now the updated git is in your $PATH and you can use it. See INSTALL in the git source tree for more details. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A question about the error: svn_fspath__is_canonical
Dan Kaplan wrote: Do you think it'll still work? Yes, that's why I suggested it. ;-) You might need to install the gcc-core, libcurl-devel, openssl-devel, and subversion-perl packages first. Regards, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH sb/diff-orderfile-config] diff test: reading a directory as a file need not error out
There is no guarantee that strbuf_read_file must error out for directories. On some operating systems (e.g., Debian GNU/kFreeBSD wheezy), reading a directory gives its raw content: $ head -c5 / | cat -A ^AM-|^_^@^L$ As a result, 'git diff -O/' succeeds instead of erroring out on these systems, causing t4056.5 orderfile is a directory to fail. On some weird OS it might even make sense to pass a directory to the -O option and this is not a common user mistake that needs catching. Remove the test. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Hi, t4056 is failing on systems using glibc with the kernel of FreeBSD[1]: | expecting success: | test_must_fail git diff -O/ --name-only HEAD^..HEAD | | a.h | b.c | c/Makefile | d.txt | test_must_fail: command succeeded: git diff -O/ --name-only HEAD^..HEAD | not ok 5 - orderfile is a directory How about this patch? Thanks, Jonathan [1] https://buildd.debian.org/status/fetch.php?pkg=gitarch=kfreebsd-amd64ver=1%3A2.0~next.20140107-1stamp=1389379274 t/t4056-diff-order.sh | 4 1 file changed, 4 deletions(-) diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh index 1ddd226..9e2b29e 100755 --- a/t/t4056-diff-order.sh +++ b/t/t4056-diff-order.sh @@ -68,10 +68,6 @@ test_expect_success POSIXPERM,SANITY 'unreadable orderfile' ' test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD ' -test_expect_success 'orderfile is a directory' ' - test_must_fail git diff -O/ --name-only HEAD^..HEAD -' - for i in 1 2 do test_expect_success orderfile using option ($i) ' -- 1.8.5.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Make 'git help everyday' work
Hi, Philip Oakley wrote: The Everyday GIT With 20 Commands Or So guide is not accessible via the git help system. Fix that. Neat. :) Junio covered everything I'd want to say about patch 1/6. After fixing that, I'd suggest squashing all 6 patches into a single patch. They all are part of accomplishing the same task, they are not too hard to read together, and the intermediate state after applying a few but not the rest doesn't make much sense. The details of patches 2-6/6 look good to me. Alternatively, this could be two patches: 1 - modify everyday.txt in place to be a suitable manpage 2 - rename it, add a placeholder for the old name, and modify the build rules to treat it as an actual manpage Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] read-cache: new extension to mark what file is watched
Hi,
Nguyễn Thái Ngọc Duy wrote:
If an entry is watched, git lets an external program decide if the
entry is modified or not. It's more like --assume-unchanged, but
designed to be controlled by machine.
We are running out of on-disk ce_flags, so instead of extending
on-disk entry format again, watched flags are in-core only and
stored as extension instead.
Makes sense.
Care to add a brief description of the on-disk format for
Documetnation/technical/index-format.txt as well?
[...]
--- a/cache.h
+++ b/cache.h
@@ -168,6 +168,7 @@ struct cache_entry {
/* used to temporarily mark paths matched by pathspecs */
#define CE_MATCHED (1 26)
+#define CE_WATCHED (1 27)
Nit: I'd add a blank line before the definition of CE_WATCHED to make
it clear that the comment doesn't apply to it.
Maybe it belongs with one of the groups before (e.g., UNPACKED +
NEW_SKIP_WORKTREE). I dunno.
--- a/read-cache.c
+++ b/read-cache.c
[...]
@@ -1289,6 +1290,19 @@ static int verify_hdr(struct cache_header *hdr,
return 0;
}
+static void read_watch_extension(struct index_state *istate, uint8_t *data,
+ unsigned long sz)
+{
+ int i;
+ if ((istate-cache_nr + 7) / 8 != sz) {
+ error(invalid 'WATC' extension);
+ return;
+ }
+ for (i = 0; i istate-cache_nr; i++)
+ if (data[i / 8] (1 (i % 8)))
+ istate-cache[i]-ce_flags |= CE_WATCHED;
+}
So the WATC section has one bit per index entry, encoding whether that
entry is WATCHED. Makes sense.
Do I understand correctly that this patch just takes care of the
bookkeeping for the CE_WATCHED bit and the actual semantics will
come in a later patch?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Submodule relative URL problems
Hi, Lianheng Tong wrote: git clone W1:path to A on W1/.git path to A on W2 Interesting. Thoughts: * More typical usage is to clone from a bare repository (A.git), which wouldn't have this problem. But I think your case is worth supporting, too. * What would you think of putting symlinks in A's .git directory? cd A/.git ln -s ../B ../C ../D . * Perhaps as a special case when the superproject is foo/.git, git should treat relative submodule paths as relative to foo/ instead of relative to foo/.git/. I think that would take care of your case without breaking existing normal practices, though after the patch is made it still wouldn't take care of people using old versions of git without that patch. What do you think? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git gui crashes ( v 1.8.5.2)
(just cc-ing some area experts)
Hi Benoît,
Benoît Bourbié wrote:
git gui crashes on my Linux machin since I updated it to 1.8.5.2.
I assume you mean master and not 1.8.5.2, since v1.8.5.2 doesn't
include the change 918dbf58 (git-gui: right half window is paned,
2013-08-21).
I had the message
Error in startup script: unknown option -stretch
while executing
.vpane.lower paneconfigure .vpane.lower.diff -stretch always
invoked from within
if {$use_ttk} {
.vpane.lower pane .vpane.lower.diff -weight 1
.vpane.lower pane .vpane.lower.commarea -weight 0
} else {
.vpane.lower paneconfigure...
(file git/libexec/git-core/git-gui line 3233)
So, I reverted the change that has been made in git-gui/git-gui.sh
(Diff and Commit Area)
I replaced
${NS}::panedwindow .vpane.lower -orient vertical
${NS}::frame .vpane.lower.commarea
${NS}::frame .vpane.lower.diff -relief sunken -borderwidth 1 -height 500
.vpane.lower add .vpane.lower.diff
.vpane.lower add .vpane.lower.commarea
.vpane add .vpane.lower
by
${NS}::frame .vpane.lower -height 300 -width 400
${NS}::frame .vpane.lower.commarea
${NS}::frame .vpane.lower.diff -relief sunken -borderwidth 1
pack .vpane.lower.diff -fill both -expand 1
pack .vpane.lower.commarea -side bottom -fill x
.vpane add .vpane.lower
if {!$use_ttk} {.vpane paneconfigure .vpane.lower -sticky nsew}
and now, git gui works as expected.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] send-email: If the ca path is not specified, use the defaults
Junio C Hamano wrote: Ruben Kerkhof ru...@rubenkerkhof.com writes: As a last check, I set smtpsslcertpath = /etc/pki/tls/cert.pem in ~/.gitconfig and git-send-email works fine now. Which would mean that the existing code, by blindly defaulting to /etc/ssl/certs/ and misdiagnosing that the directory is meant to be used as SSL_ca_path, breaks a set-up that otherwise _should_ work. [...] Ram (who did 35035bbf), with the patch from Ruben in the thread, do you get either the warning or SSL failure? Conceptually, the resulting code is much better, I think, without blindly defaulting /etc/ssl/certs and instead of relying on the underlying platform just working out of the box with its own default, FWIW this should help on Mac OS X, too. Folks using git on mac at $DAYJOB have been using the workaround described at http://mercurial.selenic.com/wiki/CACertificates#Mac_OS_X_10.6_and_higher so I forgot to report it. :/ Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] revision: mark contents of an uninteresting tree uninteresting
Junio C Hamano wrote:
we see the top-level tree marked as uninteresting (i.e. ^A^{tree} in
the above example) and call mark_tree_uninteresting() on it; this
unfortunately prevents us from recursing into the tree and marking
the objects in the tree as uninteresting.
So the tree is marked uninteresting twice --- once by setting in the
UNINTERESTING flag in handle_revision_arg() and a second attempted
time in mark_tree_uninteresting()? Makes sense.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] .gitignore: Ignore editor backup and swap files
Junio C Hamano wrote: These paths that depend on your choice of the editor and other tools can still be managed in your personal .git/info/exclude in the meantime. Or $HOME/.config/git/ignore to not have to make the same setting in every repository. :) Maybe it would make sense to add a hint about that somewhere to user-manual.txt. Even better would be to automatically include some common exclude patterns globally without requiring any manual configuration, but that would take some care to make sure the patterns and how to disable them are documented clearly. -- 8 -- Subject: gitignore doc: add global gitignore to synopsis The gitignore(5) manpage already documents $XDG_CONFIG_HOME/git/ignore but it is easy to forget that it exists. Add a reminder to the synopsis. Noticed while looking for a place to put a list of scratch filenames in the cwd used by one's editor of choice. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Documentation/gitignore.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt index f971960..37c9470 100644 --- a/Documentation/gitignore.txt +++ b/Documentation/gitignore.txt @@ -7,7 +7,7 @@ gitignore - Specifies intentionally untracked files to ignore SYNOPSIS -$GIT_DIR/info/exclude, .gitignore +$HOME/.config/git/ignore, $GIT_DIR/info/exclude, .gitignore DESCRIPTION --- -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] prefer xwrite instead of write
Hi,
Erik Faye-Lund wrote:
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct
commit_list *remotehead
sha1_to_hex(commit-object.sha1));
pretty_print_commit(ctx, commit, out);
}
- if (write(fd, out.buf, out.len) 0)
+ if (xwrite(fd, out.buf, out.len) 0)
die_errno(_(Writing SQUASH_MSG));
Shouldn't this use write_in_full() to avoid a silently truncated result? (*)
[...]
--- a/streaming.c
+++ b/streaming.c
@@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1,
struct stream_filter *f
goto close_and_exit;
}
if (kept (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
- write(fd, , 1) != 1))
+ xwrite(fd, , 1) != 1))
Yeah, if we get EINTR then it's worth retrying.
[...]
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1129,9 +1129,8 @@ static int udt_do_write(struct unidirectional_transfer
*t)
return 0; /* Nothing to write. */
transfer_debug(%s is writable, t-dest_name);
- bytes = write(t-dest, t-buf, t-bufuse);
- if (bytes 0 errno != EWOULDBLOCK errno != EAGAIN
- errno != EINTR) {
+ bytes = xwrite(t-dest, t-buf, t-bufuse);
+ if (bytes 0 errno != EWOULDBLOCK) {
Here the write is limited by BUFFERSIZE, and returning to the outer
loop to try another read when the write returns EAGAIN, like the
original code does, seems philosophically like the right thing to do.
Luckily we don't use O_NONBLOCK anywhere, so the change shouldn't
matter in practice. So although it doesn't do any good, using xwrite
here for consistency should be fine.
So my only worry is the (*) above. With that change,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] prefer xwrite instead of write
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Shouldn't this use write_in_full() to avoid a silently truncated result? (*) Meaning this? If so, I think it makes sense. [...] - if (xwrite(fd, out.buf, out.len) 0) + if (write_in_full(fd, out.buf, out.len) != out.len) Yes. Either ' 0' or '!= out.len' would work fine here, since write_in_full is defined to always either write the full 'count' bytes or return an error. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG: [Cosmetic] Commiting a gerrit ChangeId before the commit hook was installed
Hi, Strainu wrote: strainu@emily:~/core git review -f Creating a git remote called gerrit that maps to: ssh://stra...@gerrit.wikimedia.org:29418/pywikibot/core.git Your change was committed before the commit hook was installed. Amending the commit to add a gerrit change id. At this point I ended the transaction, as I was confused by the last message: I was afraid the ChangeId would have changed, causing the patch to be attached to another review. I think git should not show this message if the change description already has a change id This message doesn't come from git. It comes from the git-review tool (in git_review/cmd.py), so cc-ing the authors in case they have thoughts on that. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Date format in 'git log' should be in local timezone
Hi, Yuri wrote: Timezone here doesn't help the log reader at all. It doesn't even reflect the actual location of the submitter. Instead, it should be converted to the local TZ of the client. This will make it easier to read and understand the time. Does git log --date=local or git log --date=relative do what you're looking for? If so, you can set that permanently by setting 'date = local' or 'date = relative' in the [log] section of your ~/.gitconfig. See log.date in the git-config(1) manpage for details. I wonder if 'date = relative' would make a better default. Even further, timezone shouldn't even be stored by the git server. I've found it very useful and would consider that a regression, at least. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c
David Kastrup wrote: Now I might have sent at an unopportune time: blame.c is mostly attributed to Junio who seems to have been a few days absent now. I also have seen quite a few mails and patch submissions on the list go basically unanswered in the last few days. In the U.S., yesterday was a federal holiday (Martin Luther King, Jr. day) and the two days before were the weekend. [...] maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. Now the file involved (builtin/blame.c) itself does not state _any_ license. Most of git is GPLv2-only. (As an aside, if there's interest then I'd be happy to see most of it change to GPLv2-or-later since that makes it possible to link to code under the Apache License. But I'm also happy with the status quo.) [...] As far as I am concerned, I am willing to license my work under the GPLv2 or any later version at the discretion of whoever wants to work with it. I think that should be compatible with the project goals. Now the above passage states you might note so in your copyright message, but my patches do not even contain a copyright message and it is not clear to me that they should, or that there is a sensible place to place such copyright messages. Yeah, since these patches aren't adding a large new chunk of code there's no need for a new copyright notice and so no place to put that kind of thing unless Junio wants to relicense blame (which would be orthogonal to these patches). Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c
David Kastrup wrote: So my understanding is that when we are talking about _significant_ additions to builtin/blame.c (the current patches don't qualify as such really) that a) builtin/blame.c is licensed under GPLv2 b) significant contributions to it will not be relicensed under different licenses without the respective contributors' explicit consent. Yep, that's how it works. [...] The combination of the SubmittingPatches text with the file notices in builtin/blame.c is not really painting a full picture of the situation. Any idea how this could be made more clear? E.g., maybe we should bite the bullet and add a line to all source files that don't already state a license: /* * License: GPLv2. See COPYING for details. */ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c
David Kastrup wrote: Jonathan Nieder jrnie...@gmail.com writes: Any idea how this could be made more clear? E.g., maybe we should bite the bullet and add a line to all source files that don't already state a license: /* * License: GPLv2. See COPYING for details. */ Probably somewhat more verbose like This file may be distributed under the conditions of the GPLv2. See the file COPYING for details. I think there are boilerplate texts for that. All else being equal, longer is worse. Whatever the exact wording, that would be the cleanest way I think. The respective Documentation/SubmittingPatches text looks like it is quoted from somewhere else, so adapting it to the realities of files without clear copyright statement seems less straightforward. Hm, the wording comes from the Linux kernel project, where it's also pretty normal not to have a license notice in every file (and where the default license is also GPLv2). Is the problem the phrase indicated in the file, or is the problem e.g. the lack of a pointer to https://github.com/libgit2/libgit2/blob/development/git.git-authors? Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c
David Kastrup wrote: The combination of the SubmittingPatches text with the file notices in builtin/blame.c is not really painting a full picture of the situation. BTW, thanks for bringing this up. It last came up at [1]. Perhaps we can do better by adding a note to README or some similar file explaining that git is under the GPLv2 and files use that license when not otherwise specified. [1] http://thread.gmane.org/gmane.comp.version-control.git/234705/focus=234709 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] Two janitorial patches for builtin/blame.c
David Kastrup wrote: and contrib. The README file states Git is an Open Source project covered by the GNU General Public License version 2 (some parts of it are under different licenses, compatible with the GPLv2). It was originally written by Linus Torvalds with help of a group of hackers around the net. without mentioning _which_ parts are under different licenses. Okay, how about this patch? diff --git i/README w/README index 15a8e23..6745db5 100644 --- i/README +++ w/README @@ -21,8 +21,9 @@ and full access to internals. Git is an Open Source project covered by the GNU General Public License version 2 (some parts of it are under different licenses, -compatible with the GPLv2). It was originally written by Linus -Torvalds with help of a group of hackers around the net. +compatible with the GPLv2, and have notices to that effect). It was +originally written by Linus Torvalds with help of a group of hackers +around the net. Please read the file INSTALL for installation instructions. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libz and RHEL 5.9 compile of Git
Hi, salmansheikh wrote: I downloaded and installed the latest libz (1.2.8) but i installed it under a local directory under my user name (i.e. /home/ssheikh/local). The problem is that git only looks in the locations below. I even have that directory in my $LD_LIBRARY_PATH. Confusingly, LD_LIBRARY_PATH is only used a run-time. The build time library path is just called LIBRARY_PATH. You may also need to add your libz's include/ dir to CPATH. See http://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html for more details. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 08/23] ewah: compressed bitmap implementation
Hi,
Jeff King wrote:
EWAH is a word-aligned compressed variant of a bitset (i.e. a data
structure that acts as a 0-indexed boolean array for many entries).
I suspect that for some callers it's not word-aligned.
Without the following squashed in, commits 212f2ffb and later fail t5310
on some machines[1].
On ARMv5:
expecting success:
git rev-list --test-bitmap HEAD
*** Error in `/«PKGBUILDDIR»/git': realloc(): invalid pointer:
0x008728b0 ***
Aborted
not ok 3 - rev-list --test-bitmap verifies bitmaps
On sparc:
expecting success:
git rev-list --test-bitmap HEAD
Bus error
not ok 3 - rev-list --test-bitmap verifies bitmaps
Hopefully it's possible to get the alignment right in the caller
and tweak the signature to require that instead of using unaligned
reads like this. There's still something wrong after this patch ---
the new result is a NULL pointer dereference in t5310.7 enumerate
--objects (full bitmap).
(gdb) run
Starting program: /home/jrnieder/src/git/git rev-list --objects
--use-bitmap-index HEAD
[Thread debugging using libthread_db enabled]
Using host libthread_db library /lib/sparc-linux-gnu/libthread_db.so.1.
537ea4d3eb79c95f602873b1167c480006d2ac2d
[...]
ec635144f60048986bc560c5576355344005e6e7
Program received signal SIGSEGV, Segmentation fault.
0x001321c0 in sha1_to_hex (sha1=0x0) at hex.c:68
68 unsigned int val = *sha1++;
(gdb) bt
#0 0x001321c0 in sha1_to_hex (sha1=0x0) at hex.c:68
#1 0x000b839c in show_object_fast (sha1=0x0, type=OBJ_TREE, exclude=0,
name_hash=0, found_pack=0x2b8480, found_offset=4338) at builtin/rev-list.c:270
#2 0x00158abc in show_objects_for_type (objects=0x2b2498,
type_filter=0x2b0fb0, object_type=OBJ_TREE, show_reach=0xb834c
show_object_fast) at pack-bitmap.c:640
#3 0x001592d0 in traverse_bitmap_commit_list (show_reachable=0xb834c
show_object_fast) at pack-bitmap.c:818
#4 0x000b894c in cmd_rev_list (argc=2, argv=0xd688, prefix=0x0) at
builtin/rev-list.c:369
#5 0x00014024 in run_builtin (p=0x256e38 commands+1020, argc=4,
argv=0xd688) at git.c:314
#6 0x00014330 in handle_builtin (argc=4, argv=0xd688) at git.c:487
#7 0x000144a8 in run_argv (argcp=0xd5ec, argv=0xd5a0) at git.c:533
#8 0x000146fc in main (argc=4, av=0xd684) at git.c:616
(gdb) frame 2
#2 0x00158abc in show_objects_for_type (objects=0x2b2498,
type_filter=0x2b0fb0, object_type=OBJ_TREE, show_reach=0xb834c
show_object_fast) at pack-bitmap.c:640
640 show_reach(sha1, object_type, 0, hash,
bitmap_git.pack, entry-offset);
(gdb) p entry-nr
$1 = 4294967295
Line numbers are in the context of 8e6341d9. Ideas?
[1] ARMv5 and sparc:
https://buildd.debian.org/status/logs.php?pkg=gitsuite=experimental
diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
index aed0da6..696a8ec 100644
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -110,25 +110,38 @@ int ewah_serialize(struct ewah_bitmap *self, int fd)
return ewah_serialize_to(self, write_helper, (void *)(intptr_t)fd);
}
+#define get_be32(p) ( \
+ (*((unsigned char *)(p) + 0) 24) | \
+ (*((unsigned char *)(p) + 1) 16) | \
+ (*((unsigned char *)(p) + 2) 8) | \
+ (*((unsigned char *)(p) + 3) 0) )
+
+#define get_be64(p) ( \
+ ((uint64_t) get_be32(p) 32) | \
+ get_be32((unsigned char *)(p) + 4) )
+
int ewah_read_mmap(struct ewah_bitmap *self, void *map, size_t len)
{
- uint32_t *read32 = map;
- eword_t *read64;
+ unsigned char *p = map;
size_t i;
- self-bit_size = ntohl(*read32++);
- self-buffer_size = self-alloc_size = ntohl(*read32++);
+ self-bit_size = get_be32(p);
+ p += 4;
+ self-buffer_size = self-alloc_size = get_be32(p);
+ p += 4;
self-buffer = ewah_realloc(self-buffer,
self-alloc_size * sizeof(eword_t));
if (!self-buffer)
return -1;
- for (i = 0, read64 = (void *)read32; i self-buffer_size; ++i)
- self-buffer[i] = ntohll(*read64++);
+ for (i = 0; i self-buffer_size; ++i) {
+ self-buffer[i] = get_be64(p);
+ p += 8;
+ }
- read32 = (void *)read64;
- self-rlw = self-buffer + ntohl(*read32++);
+ self-rlw = self-buffer + get_be32(p);
+ p += 4;
return (3 * 4) + (self-buffer_size * 8);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] compat: move unaligned helpers to bswap.h
Jeff King wrote: Commit d60c49c (read-cache.c: allow unaligned mapping of the index file, 2012-04-03) introduced helpers to access unaligned data. Let's factor them out to make them more widely available. While we're at it, we'll give the helpers more readable names, add a helper for the ntohll form, and add the appropriate Makefile knob. Weird. Why wasn't git broken on the relevant platforms before (given that no one has been setting NEEDS_ALIGNED_ACCESS for them)? Puzzled, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 08/23] ewah: compressed bitmap implementation
Jeff King wrote: Here's a patch series (on top of jk/pack-bitmap, naturally) that lets t5310 pass there. I assume the ARM problem is the same, though seeing the failure in realloc() is unexpected. Can you try it on both your platforms with these patches? Thanks. Trying it out now. [...] Hopefully it's possible to get the alignment right in the caller and tweak the signature to require that instead of using unaligned reads like this. There's still something wrong after this patch --- the new result is a NULL pointer dereference in t5310.7 enumerate --objects (full bitmap). After my patches, t5310 runs fine for me. I didn't try your patch, but mine are similar. Let me know if you still see the problem (there may simply be a bug in yours, but I didn't see it). I had left out a cast to unsigned, producing an overflow. My main worry about the patches is that they will probably run into an analagous problem to the one that v1.7.12-rc0~1^2~2 (block-sha1: avoid pointer conversion that violates alignment constraints, 2012-07-22) solved. By casting the pointer to (uint32_t *) we are telling the compiler it is 32-bit aligned (C99 section 6.3.2.3). Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] compat: move unaligned helpers to bswap.h
Jeff King wrote: I think it was a bug waiting to surface if index v4 ever got wide use. Ah, ok. In that case I think git-compat-util.h should include something like what block-sha1/sha1.c has: #if !defined(__i386__) !defined(__x86_64__) \ !defined(_M_IX86) !defined(_M_X64) \ !defined(__ppc__) !defined(__ppc64__) \ !defined(__powerpc__) !defined(__powerpc64__) \ !defined(__s390__) !defined(__s390x__) #define NEEDS_ALIGNED_ACCESS #endif Otherwise we are relying on the person building to know their own architecture intimately, which shouldn't be necessary. Meanwhile, as mentioned in the other message, I suspect the NEEDS_ALIGNED_ACCESS code path is broken for aggressive compilers anyway. Looking more. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] compat: move unaligned helpers to bswap.h
Jeff King wrote: On Thu, Jan 23, 2014 at 11:56:43AM -0800, Jonathan Nieder wrote: In that case I think git-compat-util.h should include something like what block-sha1/sha1.c has: #if !defined(__i386__) !defined(__x86_64__) \ !defined(_M_IX86) !defined(_M_X64) \ !defined(__ppc__) !defined(__ppc64__) \ !defined(__powerpc__) !defined(__powerpc64__) \ !defined(__s390__) !defined(__s390x__) #define NEEDS_ALIGNED_ACCESS #endif Otherwise we are relying on the person building to know their own architecture intimately, which shouldn't be necessary. Yeah, I agree it would be nice to autodetect. The nice thing is that false positives are harmless, modulo slowing down git a little if the compiler doesn't figure out how to optimize the NEEDS_ALIGNED_ACCESS codepath when on an unlisted platform that doesn't, in fact, need aligned access. In other words, it would work out of the box for everybody. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 08/23] ewah: compressed bitmap implementation
Jeff King wrote: On Thu, Jan 23, 2014 at 11:52:06AM -0800, Jonathan Nieder wrote: My main worry about the patches is that they will probably run into an analagous problem to the one that v1.7.12-rc0~1^2~2 [...] I think this probably works in practice because align_ntohl is inlined, and any sane compiler will never actually load the variable. I don't think that's safe to rely on. The example named above didn't pose any problems except on one platform. All the relevant functions were static and easy to inline. GCC just followed the standard literally and chose to break by reading one word at a time, just like in this case it could break e.g. by copying one word at a time in __builtin_memcpy (which seems perfectly reasonable to me --- optimization involves a lot of constraint solving, and if you can't trust your constraints then there's not much you can do). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 08/23] ewah: compressed bitmap implementation
Jeff King wrote: [1/2]: compat: move unaligned helpers to bswap.h [2/2]: ewah: support platforms that require aligned reads After setting NEEDS_ALIGNED_ACCESS, Tested-by: Jonathan Nieder jrnie...@gmail.com # ARMv5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 08/23] ewah: compressed bitmap implementation
Jeff King wrote:
If we
change the signature of align_ntohl, we can do this:
uint32_t align_ntohl(void *ptr)
{
uint32_t x;
memcpy(x, ptr, sizeof(x));
return ntohl(x);
}
...
foo = align_ntohl(ptr);
The memcpy solution is taken from read-cache.c, but as we noted, it
probably hasn't been used a lot. The blk_sha1 get_be may be faster, as
it converts as it reads.
I doubt there's much difference either way, especially after an
optimizer gets its hands on it. According to [1] ARM has no fast
byte swap instruction so with -O0 the byte-at-a-time implementation is
probably faster there. I can try a performance test if you like.
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/125737
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 08/23] ewah: compressed bitmap implementation
Jeff King wrote: On Thu, Jan 23, 2014 at 09:53:26PM +, brian m. carlson wrote: Yes, it will. SPARC requires all loads be naturally aligned (4-byte to an address that's a multiple of 4, 8-byte to a multiple of 8, and so on). In general, architectures that do not support unaligned access require natural alignment for all quantities. In that case, I think we cannot even blame Shawn. The ewah serialization format itself (which JGit inherited from the javaewah library) has 8 bytes of header and 4 bytes of trailer. So packed serialized ewahs wouldn't be 8-byte aligned I don't think that's a big issue. A pair of 4-byte reads would not be too slow. Even on x86, aligned reads are supposed to be faster than unaligned reads (though I haven't looked at benchmarks recently). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 08/23] ewah: compressed bitmap implementation
Jeff King wrote: On Thu, Jan 23, 2014 at 02:17:55PM -0800, Jonathan Nieder wrote: I don't think that's a big issue. A pair of 4-byte reads would not be too slow. The header is actually two separate 4-byte values, so that's fine. But between the header and trailer are a series of 8-byte data values, and that is what we need the 8-byte alignment for. Sorry for the lack of clarity. What I meant is that a 4-byte aligned 8-byte value can be read using a pair of 4-byte reads, which is less of a performance issue than a completely unaligned value. [...] Anyway, this is all academic until we are designing bitmap v2, which I do not plan on doing anytime soon. Sure, fair enough. :) Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] unaligned reads from .bitmap files
Jeff King wrote: Here it is again, fixing the issues we've discussed. Thanks! Passes all tests. Tested-by: Jonathan Nieder jrnie...@gmail.com # ARMv5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] block-sha1: factor out get_be and put_be wrappers
Jeff King wrote: These short names might not be descriptive enough now that they are globals. However, they make sense to me. Yeah, I think they're clear. And they match the Linux kernel's get_unaligned_be32() / put_unaligned_be32(). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] read-cache: use get_be32 instead of hand-rolled ntoh_l
Jeff King wrote: This _might_ still suffer from the issue fixed in 5f6a112 (block-sha1: avoid pointer conversion that violates alignment constraints, 2012-07-22), as we are taking the pointer of a uint32 in a struct. No conversion, so no issue there. Line 1484 looks more problematic: disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset); In v4 indexes, src_offset doesn't have any particular alignment so this conversion has undefined behavior. Do you know if any tests exercise this code with paths that don't have convenient length? [...] I'm inclined to leave it for now, as we haven't made anything worse, and nobody has reported a problem. Yeah, agreed. Probably the simplest fix would be to take a char *, memcpy into a new (aligned) buffer and then byteswap in place, but that's orthogonal to this series. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] ewah: support platforms that require aligned reads
Jeff King wrote:
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -112,23 +112,38 @@ int ewah_serialize(struct ewah_bitmap *self, int fd)
[...]
+#if __BYTE_ORDER != __BIG_ENDIAN
Is this portable?
On a platform without __BYTE_ORDER or __BIG_ENDIAN defined,
it is interpreted as
#if 0 != 0
which means that such platforms are assumed to be big endian.
Does Mingw define __BYTE_ORDER, for example?
+ {
+ size_t i;
+ for (i = 0; i self-buffer_size; ++i)
+ self-buffer[i] = ntohll(self-buffer[i]);
+ }
+#endif
It's tempting to guard with something like
if (ntohl(1) != 1) {
...
}
The optimizer can tell if this is true or false at compile time, so
it shouldn't slow anything down.
With that change,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Thanks for the quick fix.
diff --git i/ewah/ewah_io.c w/ewah/ewah_io.c
index 4a7fae6..5a527a4 100644
--- i/ewah/ewah_io.c
+++ w/ewah/ewah_io.c
@@ -135,13 +135,11 @@ int ewah_read_mmap(struct ewah_bitmap *self, void *map,
size_t len)
memcpy(self-buffer, ptr, self-buffer_size * sizeof(uint64_t));
ptr += self-buffer_size * sizeof(uint64_t);
-#if __BYTE_ORDER != __BIG_ENDIAN
- {
+ if (ntohl(1) != 1) {
size_t i;
for (i = 0; i self-buffer_size; ++i)
self-buffer[i] = ntohll(self-buffer[i]);
}
-#endif
self-rlw = self-buffer + get_be32(ptr);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] ewah: support platforms that require aligned reads
Vicent Martí wrote: On Fri, Jan 24, 2014 at 12:44 AM, Jonathan Nieder jrnie...@gmail.com wrote: +#if __BYTE_ORDER != __BIG_ENDIAN Is this portable? We explicitly set the __BYTE_ORDER macros in `compat/bswap.h`. In fact, this preprocessor conditional is the same one that we use when choosing what version of the `ntohl` macro to define, so that's why I decided to use it here. Ah, thanks. Sorry I missed that. So feel free to add my reviewed-by to the patch without my tweak, too. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 1/3] t3030-merge-recursive: Test known breakage with empty work tree
Hi, Brad King wrote: Add test cases that use 'merge-recursive' plumbing with a temporary index and empty work tree. Populate the index using 'read-tree' and 'update-index --ignore-missing --refresh' to prepare for merge without actually checking all files out to disk. Verify that each merge produces its expected tree while displaying no error diagnostics. Following my usual review practice of lazy reading for the sake of readers in the future who might be in a hurry, it's not clear what problem these tests are solving or trying to detect. Could you start with a quick summary of the symptoms and when it came up? The commit message doesn't need to paraphrase the actual code, since anyone curious about the details can always look at the code. It's more important to explain the motivation and intended effect so people can understand what went wrong if something ends up being broken by a later patch. This approach can be used to compute tree merges while checking out only conflicting files to disk (which is useful for server-side scripts). Prior to commit 5b448b85 (merge-recursive: When we detect we can skip an update, actually skip it, 2011-08-11) this worked cleanly in all cases. Do you mean something like the following? Sometimes when working with a large repository it can be useful to try out a merge and only check out conflicting files to disk (for example as a speed optimization on a server). Until v1.7.7-rc1~28^2~20 (merge-recursive: When we detect we can skip an update, actually skip it, 2011-08-11), it was possible to do so with the following idiom: ... summary of commands here ... Nowadays, that still works and the exit status is the same, but merge-recursive produces a diagnostic if our side renamed a file: error: addinfo_cache failed for path 'dst' Add a test to document this regression. [...] +++ b/t/t3030-merge-recursive.sh [...] @@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' ' ' +test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' ' + ( + GIT_WORK_TREE=$PWD/ours-has-rename-work Elsewhere in the test, commands in a subshell are indented by another tab, so these new tests should probably follow suit. As a side effect, that makes the indentation easier to see. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: remote author/documentation sections from more pages
Michael Haggerty wrote: We decided at 48bb914e (doc: drop author/documentation sections from most pages, 2011-03-11) to remove author and documentation sections from our documentation. Remove a few stragglers. Thanks. This puts two blank lines where there was previously one in some cases in the source above the GIT (part of the git suite) section. I don't think that matters. Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: How to get notified of new releases?
Matthieu Moy wrote: Robert Dailey wrote: Are there any dedicated mailing lists for git releases, or RSS feeds? Not sure if there's a Windows-dedicated list, but there's this: http://gitrss.q42.co.uk/ It filters the mailing-list posts starting with eg. [ANNOUNCE] and turns it into an RSS feed. There's also https://github.com/msysgit/msysgit/commits/master.atom, though that might be more activity than you're looking for (it would be the feed to follow if you want to build your own snapshots of git on Windows and try every change). Perhaps https://github.com/msysgit/msysgit/tags.atom would do the trick. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v1.9-rc0
Hi, Kacper Kornet wrote: The change in release numbering also breaks down gitolite v2 setups. One of the gitolite commands, gl-compile-conf, expects the output of git --version to match /git version (\d+)\.(\d+)\.(\d+)/. I have no idea how big problem it is, as I don't know how many people hasn't migrate to gitolite v3 yet. http://qa.debian.org/popcon.php?package=gitolite says there are some. I guess soon we'll see if there are complaints. http://gitolite.com/gitolite/migr.html says gitolite v2 is still maintained. Hopefully the patch to gitolite v2 to fix this would not be too invasive --- e.g., how about this patch (untested)? Thanks, Jonathan diff --git i/src/gl-compile-conf w/src/gl-compile-conf index f497ae5..8508313 100755 --- i/src/gl-compile-conf +++ w/src/gl-compile-conf @@ -394,8 +394,9 @@ die the server. If it is not, please edit ~/.gitolite.rc on the server and set the \$GIT_PATH variable to the correct value\n unless $git_version; -my ($gv_maj, $gv_min, $gv_patchrel) = ($git_version =~ m/git version (\d+)\.(\d+)\.(\d+)/); +my ($gv_maj, $gv_min, $gv_patchrel) = ($git_version =~ m/git version (\d+)\.(\d+)\.([^.-]*)/); die $ABRT I can't understand $git_version\n unless ($gv_maj = 1); +$gv_patchrel = 0 unless ($gv_patchrel =~ m/^\d+$/); $git_version = $gv_maj*1 + $gv_min*100 + $gv_patchrel; # now it's normalised die \n\t\t* AAARGH! *\n . -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH jn/pager-lv-default-env] pager test: make fake pager consume all its input
Otherwise there is a race: if 'git log' finishes writing before the pager terminates and closes the pipe, all is well, and if the pager finishes quickly enough then 'git log' terminates with SIGPIPE. died of signal 13 at /build/buildd/git-1.9~rc1/t/test-terminal.perl line 33. not ok 6 - LESS and LV envvars are set for pagination Noticed on Ubuntu PPA builders, where the race was lost about half the time. Compare v1.7.0.2~6^2 (tests: Fix race condition in t7006-pager, 2010-02-22). Reported-by: Anders Kaseorg ande...@mit.edu Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Anders Kaseorg wrote: On 01/06/2014 09:14 PM, Jonathan Nieder wrote: +PAGER=env pager-env.out +export PAGER + +test_terminal git log [...] On the Ubuntu PPA builders, I’m seeing this new test fail with SIGPIPE about half the time: died of signal 13 at /build/buildd/git-1.9~rc1/t/test-terminal.perl line 33. not ok 6 - LESS and LV envvars are set for pagination Good catch. Sorry for the trouble. t/t7006-pager.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index 7fe3367..b9365b4 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -40,7 +40,7 @@ test_expect_failure TTY 'pager runs from subdir' ' test_expect_success TTY 'LESS and LV envvars are set for pagination' ' ( sane_unset LESS LV - PAGER=env pager-env.out + PAGER=env pager-env.out; wc export PAGER test_terminal git log -- 1.9.rc1.175.g0b1dcb5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Running make rpm fails on a CentOS 6.3 machine
Hi, Erez Zilber wrote: Writing perl.mak for Git Writing perl.mak for Git rename MakeMaker.tmp = perl.mak: No such file or directory at /usr/share/perl5/ExtUtils/MakeMaker.pm line 1024. make[3]: perl.mak: No such file or directory make[3]: perl.mak: No such file or directory make[3]: *** No rule to make target `perl.mak'. Stop. Looks like MakeMaker is racing against itself. Alas, (on a fairly current Debian system, with perl 5.14.2) I'm not able to reproduce that. Instead, I get this: | $ make -j8 rpm [...] | make[2]: Leaving directory `$HOME/rpmbuild/BUILD/git-1.8.5.3/Documentation' | make[1]: Leaving directory `$HOME/rpmbuild/BUILD/git-1.8.5.3' | + exit 0 | Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.WqNYnx | + umask 022 | + cd $HOME/rpmbuild/BUILD | + cd git-1.8.5.3 | + rm -rf $HOME/rpmbuild/BUILDROOT/git-1.8.5.3-1.x86_64 | + make -j12 'CFLAGS=-O2 -g' \ DESTDIR=$HOME/rpmbuild/BUILDROOT/git-1.8.5.3-1.x86_64 \ ETC_GITCONFIG=/etc/gitconfig prefix=/usr \ mandir=/usr/share/man htmldir=/usr/share/doc/git-1.8.5.3 \ INSTALLDIRS=vendor install install-doc | make[1]: Entering directory `$HOME/rpmbuild/BUILD/git-1.8.5.3' | make[1]: warning: -jN forced in submake: disabling jobserver mode. | make[1]: *** write jobserver: Bad file descriptor. Stop. | make[1]: *** Waiting for unfinished jobs | make[1]: *** write jobserver: Bad file descriptor. Stop. | error: Bad exit status from /var/tmp/rpm-tmp.WqNYnx (%install) | | | RPM build errors: | Bad exit status from /var/tmp/rpm-tmp.WqNYnx (%install) | make: *** [rpm] Error 1 Known problem? A build without -j8 gets further. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Ensure __BYTE_ORDER is always set
Hi,
Brian Gernhardt wrote:
a201c20 (ewah: support platforms that require aligned reads) added a
reliance on the existence of __BYTE_ORDER and __BIG_ENDIAN. However,
these macros are spelled without the leading __ on some platforms (OS
X at least). In this case, the endian-swapping code was added even
when unnecessary, which caused assertion failures in
t5310-pack-bitmaps.sh as the code that used the bitmap would read past
the end.
We already had code to handle this case in compat/bswap.h, but it was
only used if we couldn't already find a reasonable version of bswap64.
Makes sense. Sorry I missed this.
In an ideal world I would prefer to just rely on ntohll when it's
decent (meaning that the '#if __BYTE_ORDER != __BIG_ENDIAN' block
could be written as
if (ntohll(1) != 1) {
...
}
or
if (ntohll(1) == 1)
; /* Big endian. Nothing to do.
else {
...
}
). But compat/bswap.h already relies on knowing the endianness at
preprocessing time so that wouldn't buy anything.
Another in an ideal world option: make the loop unconditional after
checking that optimizers on big-endian systems realize it's a noop.
In any event, in the real world your patch looks like the right thing
to do.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Ensure __BYTE_ORDER is always set
Hi, Jeff King wrote: I do find the failure mode interesting. The endian-swapping code kicked in when it did not Odd --- wouldn't the #if condition expand to '0 != 0'? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A few contributor's questions
Hi, David Kastrup wrote: builtin/blame.c merely states /* * Blame * * Copyright (c) 2006, Junio C Hamano */ I think you planned to make substantial changes, so /* * Blame * * Copyright (c) 2006--2014, Junio C Hamano and others * Licensed under GPLv2. See Git's COPYING file for details. */ towards the end of the series (or squashed into some patch that makes significant changes) looks fine to me. Also keep in mind that you don't need a copyright notice to own copyright, that it would be crazy for someone to claim you've assigned copyright on your changes without an explicit reassignment, and that libgit2's git.git-authors file that keeps coming up includes a comment with a heuristic for delving into the history to find the authors of some code. [...] Permissable-Licenses: GPL Version 2 or later Wouldn't a signed message on your website or some other public place (e.g., the mailing list) do the trick? Or a sentence in a commit message saying I'd be happy to have these changes relicensed under the GPL version 2 or later. sounds fine to me, at least. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A few contributor's questions
Hi, David Kastrup wrote: Also whether or not this implies an assignment of copyright, it is a reasonable assumption for [...] Since I think we've completely gone off the rails: I assume the problem you're trying to solve is that files don't have clear enough notices of their licensing. That could be a real problem for people using the code, since if you no one gave you a license then you don't have a license at all. It's also a problem in that it makes it harder to interpret the phrase under the same open source license (though I have no idea how that could be read as I give up my copyright completely). The way git currently works in that area is the same as the Linux kernel: * the code is copyright by the authors and we try not to waste fuss on maintaining a comprehensive list in notices. If you want to find the authors to negotiate special licensing, you get to do the work. * license is GPLv2-only where not otherwise specified * relicensing, when needed, happens by contacting all the copyright holders and getting their consent I don't see anything weird about that. But people using the code might like clearer notices, so I personally would not mind an extra line in most files stating the license. (More than that and it becomes absurd.) That's all just my opinion --- Junio might think differently, etc. [...] It's verbose and cumbersome enough that I would not have been surprised if there'd be an established way of getting this information on record, preferably per-project rather than per-commit. For relicensing the existing practice is to just contact people. That has the advantage that I can make a decision about whether to allow relicensing code I've written in the context of how I expect it to be used. I expect that if you had a stance on GPLv2+ licensing of contributions to git published in some place easily found by search engines (for example a message on the mailing list), interested people would not have too much trouble finding it when the time comes. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [WIP/PATCH 1/9] submodule: prepare for recursive checkout of submodules
Jens Lehmann wrote:
This commit adds the functions and files needed for configuration,
documentation, setting the default behavior and determining if a
submodule path should be updated automatically.
Yay!
[...]
Documentation/recurse-submodules-update.txt | 8 +
submodule.c | 50
+
submodule.h | 6
3 files changed, 64 insertions(+)
create mode 100644 Documentation/recurse-submodules-update.txt
I like the shared documentation snippet.
Ok, naive questions and overly pedantic nitpicking follow. Patch with
a couple of suggested changes at the end.
[...]
--- /dev/null
+++ b/Documentation/recurse-submodules-update.txt
@@ -0,0 +1,8 @@
+--[no-]recurse-submodules::
+ Using --recurse-submodules will update the work tree of all
+ initialized submodules according to the commit recorded in the
+ superproject if their update configuration is set to checkout'. If
+ local modifications in a submodule would be overwritten the checkout
+ will fail unless forced. Without this option or with
+ --no-recurse-submodules is, the work trees of submodules will not be
+ updated, only the hash recorded in the superproject will be updated.
Tweaks:
* Spelling out --no-recurse-submodules, --recurse-submodules (imitating
e.g. --decorate in git-log(1))
* Shortening, using imperative mood
* Skipping description of safety check, since it matches how checkout
works in general
That would make
--no-recurse-submodules::
--recurse-submodules::
Perform the checkout in submodules, too. This only affects
submodules with update strategy `checkout` (which is the
default update strategy; see `submodule.name.update` in
link:gitmodules[5]).
+
The default behavior is to update submodule entries in the superproject
index and to leave the inside of submodules alone. That behavior can
also
be requested explicitly with --no-recurse-submodules.
Ideas for further work:
* The safety check probably deserves a new section where it could be
described in detail alongside a description of the corresponding check
for plain checkout. Then the description of the -f option could
point to that section.
* What happens when update = merge, rebase, or !command? I think
skipping them for now like suggested above is fine, but:
- It would be even better to error out when there are changes to carry
over with update = merge or rebase
- Better still to perform the rebase when update = rebase
- I have no idea what update = merge should do for non-fast-forward
moves
--- a/submodule.c
+++ b/submodule.c
@@ -16,6 +16,8 @@ static struct string_list config_name_for_path;
static struct string_list config_fetch_recurse_submodules_for_name;
static struct string_list config_ignore_for_name;
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
+static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
+static int option_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
Confusingly, config_update_recurse_submodules is set using the
--recurse-submodules-default option, not configuration. There's
precedent for that in fetch.recurseSubmodules handling, but perhaps
a comment would help --- something like
/*
* When no --recurse-submodules option was passed, should git fetch
* from submodules where submodule.name.fetchRecurseSubmodules
* doesn't indicate what to do?
*
* Controlled by fetch.recurseSubmodules. The default is determined by
* the --recurse-submodules-default option, which propagates
* --recurse-submodules from the parent git process when recursing.
*/
static int config_fetch_recurse_submodules =
RECURSE_SUBMODULES_ON_DEMAND;
/*
* When no --recurse-submodules option was passed, should git update
* the index and worktree within submodules (and in turn their
* submodules, etc)?
*
* Controlled by the --recurse-submodules-default option, which
* propagates --recurse-submodules from the parent git process
* when recursing.
*/
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
[...]
@@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt,
const char *arg)
}
}
+int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
+{
+ switch (git_config_maybe_bool(opt, arg)) {
+ case 1:
+ return RECURSE_SUBMODULES_ON;
+ case 0:
+ return RECURSE_SUBMODULES_OFF;
+ default:
+ if (!strcmp(arg, checkout))
+ return RECURSE_SUBMODULES_ON;
Hm, is this arg == checkout case futureproofing for when
Re: [PATCH 1/2] t7101, t7014: rename test files to indicate what that file is for
Nguyễn Thái Ngọc Duy wrote: Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com Makes sense. [...] t/t7101-reset-empty-subdirs.sh (new +x) | 63 + t/t7101-reset.sh (gone) | 63 - t/t7104-reset-hard.sh (new +x) | 46 t/t7104-reset.sh (gone) | 46 Hm, summary incorporated in the diffstat. Neat. :) format-patch -M tells me that this indeed just renamed the files, so fwiw Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
Hi, Miklos Vajna wrote: git clone git://anongit.freedesktop.org/libreoffice/core cd core git log --full-history -p -S'mnTitleBarHeight =' sd/source/ui/dlg/PaneDockingWindow.cxx Here the first output I get from git-log is b390fae1706b9c511158a03e4fd61f263be4e511, where you can see that the commit *added* that string. So it should be there on master, I would assume. df76bfb0695d19d201936df80192108e7ce51b8c (a merge) removed it. Plain 'git log' doesn't notice because in the default mode it skips merges. Since the culprit commit is not in the first-parent history of HEAD, my usual approach doesn't help, either: $ git log -m --first-parent -S'mnTitleBarHeight =' \ -- sd/source/ui/dlg/PaneDockingWindow.cxx $ Using -c or --cc produces too many hits. Luckily '-m -p' without --first-parent worked and the first commit it showed was the right one. It produces more hits than I'd like, too, though. The -L option doesn't interact well enough with --reverse to handle this case: $ git grep -p -e'mnTitleBarHeight =' b390fae1 -- sd/source/ui/dlg/PaneDockingWindow.cxx b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx=void PaneDockingWindow::Layout (void) b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx:mnTitleBarHeight = GetSettings().GetStyleSettings().GetTitleHeight(); b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx: mnTitleBarHeight = aToolBoxSize.Height(); b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx: mnTitleBarHeight = aToolBoxSize.Height(); $ git log --reverse b390fae1..HEAD \ -L:Layout:sd/source/ui/dlg/PaneDockingWindow.cxx fatal: -L parameter 'Layout' starting at line 1: no match Thanks for a useful example. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t0003: do not chdir the whole test process
Junio C Hamano wrote: Moving to some other directory and letting the remainder of the test pieces to expect that they start there is a bad practice. I agree with the above, and I like the patch... The test that contains chdir itself may fail (or by mistake skipped via the GIT_SKIP_TESTS mechanism) in which case the remainder may operate on files in unexpected places. ... but this logic seems wrong. I don't think we've ever supported setup tests failing or being skipped in the past. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] check-attr: move to the top of working tree when in non-bare repository
Hi,
Junio C Hamano wrote:
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -94,6 +94,9 @@ int cmd_check_attr(int argc, const char **argv, const char
*prefix)
struct git_attr_check *check;
int cnt, i, doubledash, filei;
+ if (!is_bare_repository())
+ setup_work_tree();
Hm. Shouldn't check-attr error out when run without a worktree and
without --cached?
That would mean something like
diff --git i/builtin/check-attr.c w/builtin/check-attr.c
index e9af7b2..c34b6ee 100644
--- i/builtin/check-attr.c
+++ w/builtin/check-attr.c
@@ -107,6 +107,9 @@ int cmd_check_attr(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, check_attr_options,
check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
+ if (!cached_attrs)
+ setup_work_tree();
+
if (read_cache() 0) {
die(invalid cache);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] check-attr: move to the top of working tree when in non-bare repository
Hi again, Jonathan Nieder wrote: Junio C Hamano wrote: +if (!is_bare_repository()) +setup_work_tree(); Hm. Shouldn't check-attr error out when run without a worktree and without --cached? That would mean something like diff --git i/builtin/check-attr.c w/builtin/check-attr.c index e9af7b2..c34b6ee 100644 --- i/builtin/check-attr.c +++ w/builtin/check-attr.c @@ -107,6 +107,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, check_attr_options, check_attr_usage, PARSE_OPT_KEEP_DASHDASH); + if (!cached_attrs) + setup_work_tree(); Someone asked in a private reply how this interacts with t0003. t0003 tries check-attr in a bare repository. The question is, is that a desirable feature, and are people relying on it? If people are relying on it, perhaps the intuitive behavior would be to make check-attr use an only-look-at-HEAD mode by default when running in a bare repo. How do I use the only-look-at-HEAD mode from a non-bare repo? If I want attributes with respect to some other commit instead of HEAD, is there a syntax for that? The command doesn't seem to have been well thought out. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t0003: do not chdir the whole test process
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: The test that contains chdir itself may fail (or by mistake skipped via the GIT_SKIP_TESTS mechanism) in which case the remainder may operate on files in unexpected places. ... but this logic seems wrong. I don't think we've ever supported setup tests failing or being skipped in the past. The first set-up test, yes, but something in the middle added as an afterthought? Even set-up in the middle added as an afterthought, yes. For a while I've been wanting to teach GIT_SKIP_TESTS not to skip tests with 'setup' or 'set up' in their name, but I never got around to it. If I try to skip the setup test this patch touches, then there is no bare.git and lots of later tests fail. Perhaps it would be better for each test to do rm -fr bare.git git clone --bare . bare.git ( cd bare.git ... ) for itself to make the state easier to think about. On the other hand I agree that the 'cd' here is a bad practice. I just don't think it's about skipping setup --- instead, it's about it being hard to remember the cwd in general. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Fix the shallow deepen bug with no-done
Duy Nguyen wrote: Don't take it the wrong way. I was just summarizing the last round. It surprised me though that this went under my radar. Perhaps a bug tracker is not a bad idea after all (if Jeff went missing, this bug could fall under the crack) I'm happy to plug - http://bugs.debian.org/cgi-bin/pkgreport.cgi?src=git;include=tags:upstream - http://packages.qa.debian.org/common/index.html (email subscription link: source package = git; under Advanced it's possible to subscribe to bug-tracking system emails and skip e.g. the automated build stuff) - https://www.debian.org/Bugs/Reporting (bug reporting interface - unfortunately the important part is buried under Sending the bug report via e-mail) again. :) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git gc --aggressive led to about 40 times slower git log --raw
David Kastrup wrote: Duy Nguyen pclo...@gmail.com writes: Likely because --aggressive passes --depth=250 to pack-objects. Long delta chains could reduce pack size and increase I/O as well as zlib processing signficantly. [...] Compression should reduce rather than increase the total amount of reads. --depth=250 means to allow chains of To get this object, first inflate this object, then apply this delta of length 250. That's absurdly long, and doesn't even help compression much in practice (many short chains referring to the same objects tends to work fine). We probably shouldn't make --aggressive do that. Something like --depth=10 would make more sense. Hoping that clarifies, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] revert.c: Allow to specify -x via git-config
Hi, Guido Günther wrote: Without this when maintaining stable branches it's easy to forget to use -x to track where a patch was cherry-picked from. [...] --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -215,6 +215,14 @@ the working tree. spending extra time to avoid mistakes based on incorrectly matching context lines. +CONFIGURATION +- + +See linkgit:git-config[1] for core variables. + +cherrypick.record-origin:: + Default for the `-x` option. Defaults to `false`. I'm not convinced this is a good idea. Even if I always want -x when cherry-picking to stable, isn't this going to add the extra clutter line when I cherry-pick on other branches? It's especially worrying because there would be no way to override the configuration with a flag on the command line. (-r which used to do that is now a no-op.) I would be more easily convinced by a '[branch foo] recordcherrypickorigins' option that makes cherry-pick default to '-x' when and only when on the foo branch. Can you say more about the context? Why is it important to record the original commit id? Is it a matter of keeping a reminder of the commits' similarity (which cherry-pick without '-x' does ok by reusing the same message) or are people reviewing the change downstream going to be judging the change based on the recorded upstream commit id? (Like linux's stable-version branches --- but those have other requirements so I don't think this configuration would work as is there.) [...] +++ b/builtin/revert.c @@ -196,6 +196,15 @@ int cmd_revert(int argc, const char **argv, const char *prefix) [...] + if (!strcmp(var, cherrypick.record-origin)) + opts-record_origin = git_config_bool (var, value); More nitpicky: git uses camelCase, not dash-delimited, for multiword configuration items. The config parsing machinery normalizes to lowercase, so this would then be cherrypick.recordorigin. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git am and mangled subject lines
Hi Phillip, Phillip Susi wrote: git am already ignores the [PATCH X/Y] prefix that format-patch adds. Is it possible to get it to ignore any additional prefix that a bug tracker mangles into the subject line? i.e. bug #:? builtin/mailinfo.c is the place to start (see git-mailinfo(1)). This is a little tricky because some people *like* the bug #: in the subject line for a commit. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c
Hi, Michael Haggerty wrote: No, this hasn't changed. I've been documenting public functions in the header files above the declaration, and private ones where they are defined. So I moved the documentation for this function to cache.h: +/* + * Return the name of the file in the local object database that would + * be used to store a loose object with the specified sha1. The + * return value is a pointer to a statically allocated buffer that is + * overwritten each time the function is called. + */ extern const char *sha1_file_name(const unsigned char *sha1); I also rewrite the comment, as you can see. The NOTE! seemed a bit overboard to me, given that there are a lot of functions in our codebase that behave similarly. So I toned the warning down, and tightened up the comment overall. Let me know if you think I've made it less helpful. In the present state of the codebase, where many important functions have no documentation or out-of-date documentation, the first place I look to understand a function is its point of definition. So I do think that moving docs to the header file makes it less helpful. I'd prefer a mass move in the opposite direction (from header files to the point of definition). On the other hand I don't feel strongly about it. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git am and mangled subject lines
Hi, Phillip Susi wrote: On 2/24/2014 3:19 PM, Junio C Hamano wrote: Phillip Susi ps...@ubuntu.com writes: git am already ignores the [PATCH X/Y] prefix that format-patch adds. Is it possible to get it to ignore any additional prefix that a bug tracker mangles into the subject line? i.e. bug #:? I think applypatch-msg hook is your friend in a case like this. Can you point me in the direction of some documentation on this? I don't see it mentioned in the man pages for git am or mailinfo ( I would think that would be the place to have it ). Gladly. Thanks for noticing. -- 8 -- Subject: am doc: add a pointer to relevant hooks It is not obvious when looking at a new command what hooks will affect it. Add a HOOKS section to the git-am(1) page, imitating git-commit(1), to make it easier for people to discover e.g. the applypatch-msg hook that can implement a custom subject-mangling strategy (e.g., removing a bug #: prefix introduced by a bug tracker). Reported-by: Phillip Susi ps...@ubuntu.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Documentation/git-am.txt | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 54d8461..abcffb6 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -189,6 +189,11 @@ commits, like running 'git am' on the wrong branch or an error in the commits that is more easily fixed by changing the mailbox (e.g. errors in the From: lines). +HOOKS +- +This command can run `applypatch-msg`, `pre-applypatch`, +and `post-applypatch` hooks. See linkgit:githooks[5] for more +information. SEE ALSO -- 1.9.0.rc1.175.g0b1dcb5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/3] Make git more user-friendly during a merge conflict
Hi, Andrew Wong wrote: The first two patches are just about rewording a message, and adding messages to tell users to use git merge --abort to abort a merge. Sounds like a good idea. I look forward to reading the patches. We could stop here and hope that the users would read the messages, but I think git could be a bit more user-friendly. The last patch might be a bit more controversial. It changes the default behavior of git reset to default to git reset --merge during a merge conflict. I imagine that's what the user would want most of the time, and not git reset --mixed. I don't think that's a good idea. I'm not sure what new users would expect; in any case, making the command context-dependent just makes the learning process harder imho. And for experienced users, this would be a bad regression. An 'advice' message pointing the user to 'git merge --abort' might make sense, though. What do you think? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/3] wt-status: Make conflict hint message more consistent with other hints
Hi, Andrew Wong wrote: [Subject: wt-status: Make conflict hint message more consistent with other hints] Thanks for working on this. Could you include a little more detail? What other hints is this making the message more consistent with? Ideally the commit message would include a quick sample interaction, so the reviewer could see the user going Wha? and then look at the patch to see how it resolves the confusion. [...] --- a/wt-status.c +++ b/wt-status.c @@ -899,7 +899,7 @@ static void show_merge_in_progress(struct wt_status *s, status_printf_ln(s, color, _(You have unmerged paths.)); if (s-hints) status_printf_ln(s, color, - _( (fix conflicts and run \git commit\))); + _( (fix conflicts, and use \git commit\ to conclude the merge))); Quick thoughts: - The comma just moves the message closer to the right margin. I think it makes the message less readable. - What else would git commit do other than concluding the merge? What confusion is this meant to prevent? - Would introducing a new git merge --continue command help? Advantages: (1) the name of the command makes it obvious what it does; (2) the command could check that there is actually a merge in progress, helping the user when the state is not what they think; (3) consistency with git cherry-pick --abort / git cherry-pick --continue. Disadvantage: redundancy (but see (2) above). Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 3/3] reset: Change the default behavior to use --merge during a merge
Andrew Wong wrote: Yeah, this breaks compatibility, but like I said, during a merge, I don't see a good reason to do git reset --mixed, and not git reset --merge. Yeah, in principle if it had a different behavior, then plain git reset could be useful during a merge, but as is, I tend to use the form with a path (git reset -- .) to avoid losing MERGE_HEAD. I really don't like the idea of making git reset modal, though. I'd rather that reset --mixed print some advice about how to recover from the mistake, which would also have the advantage of allowing scripts that for whatever reason used git reset in this situation to continue to work. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i18n: proposed command missing leading dash
Hi,
Sandy Carter wrote:
Add missing leading dash to proposed commands in french output when
using the command:
Thanks!
[...]
--- a/po/fr.po
+++ b/po/fr.po
@@ -3266,7 +3266,7 @@ msgstr git branch -d %s\n
#: builtin/branch.c:1027
#, c-format
msgid git branch --set-upstream-to %s\n
-msgstr git branch -set-upstream-to %s\n
+msgstr git branch --set-upstream-to %s\n
To make life saner for translators, this should be either
untranslatable or a single multi-line string, I suspect:
diff --git i/builtin/branch.c w/builtin/branch.c
index b4d7716..972040c 100644
--- i/builtin/branch.c
+++ w/builtin/branch.c
@@ -1022,11 +1022,13 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
*/
if (argc == 1 track == BRANCH_TRACK_OVERRIDE
!branch_existed remote_tracking) {
- fprintf(stderr, _(\nIf you wanted to make '%s' track
'%s', do this:\n\n), head, branch-name);
- fprintf(stderr, _(git branch -d %s\n),
branch-name);
- fprintf(stderr, _(git branch --set-upstream-to
%s\n), branch-name);
+ fprintf(stderr, \n);
+ fprintf(stderr, _(If you wanted to make '%s' track
'%s', do this:\n\n
+ git branch -d %s\n
+ git branch --set-upstream-to
%s),
+ head, branch-name, branch-name,
branch-name);
+ fprintf(stderr, \n);
}
-
} else
usage_with_options(builtin_branch_usage, options);
What do you think?
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] implement submodule config cache for lookup of submodule names
Hi,
Some quick thoughts.
Heiko Voigt wrote:
This submodule configuration cache allows us to lazily read .gitmodules
configurations by commit into a runtime cache which can then be used to
easily lookup values from it. Currently only the values for path or name
are stored but it can be extended for any value needed.
Makes sense.
[...]
Next I am planning to extend this so configuration cache so it will
return the local configuration (with the usual .gitmodules,
/etc/gitconfig, .git/config, ... overlay of variables) when you pass it
null_sha1 for gitmodule or commit sha1. That way we can give this
configuration cache some use and implement its usage for the current
configuration variables in submodule.c.
Yay!
I wonder whether local configuration should also override information
from commits at times.
[...]
--- /dev/null
+++ b/Documentation/technical/api-submodule-config-cache.txt
@@ -0,0 +1,39 @@
+submodule config cache API
+==
Most API documents in Documentation/technical have a section explaining
general usage --- e.g. (from api-revision-walking),
Calling sequence
The walking API has a given calling sequence: first you need to
initialize a rev_info structure, then add revisions to control what kind
of revision list do you want to get, finally you can iterate over the
revision list.
Or (from api-string-list):
The caller:
. Allocates and clears a `struct string_list` variable.
. Initializes the members. You might want to set the flag
`strdup_strings`
if the strings should be strdup()ed. For example, this is necessary
[...]
. Can remove items not matching a criterion from a sorted or unsorted
list using `filter_string_list`, or remove empty strings using
`string_list_remove_empty_items`.
. Finally it should free the list using `string_list_clear`.
This makes it easier to get started by looking at the documentation alone
without having to mimic an example.
Another thought: it's tempting to call this the 'submodule config API' and
treat the (optional?) memoization as an implementation detail instead of
reminding the caller of it too much. But I haven't thought that through
completely.
[...]
+`struct submodule_config *get_submodule_config_from_path(
+ struct submodule_config_cache *cache,
+ const unsigned char *commit_sha1,
+ const char *path)::
+
+ Lookup a submodules configuration by its commit_sha1 and path.
The cache just always grows until it's freed, right? Would it make
sense to allow cached configurations to be explicitly evicted when the
caller is done with them some day? (Just curious, not a complaint.
Might be worth mentioning in the overview how the cache is expected to
be used, though.)
[...]
--- /dev/null
+++ b/submodule-config-cache.h
@@ -0,0 +1,26 @@
+#ifndef SUBMODULE_CONFIG_CACHE_H
+#define SUBMODULE_CONFIG_CACHE_H
+
+#include hashmap.h
+#include strbuf.h
+
+struct submodule_config_cache {
+ struct hashmap for_path;
+ struct hashmap for_name;
+};
Could use comments (e.g., saying what keys each maps to what values).
Would callers ever look at the hashmaps directly or are they only
supposed to be accessed using helper functions that take a whole
struct?
[...]
+
+/* one submodule_config_cache entry */
+struct submodule_config {
+ struct strbuf path;
+ struct strbuf name;
+ unsigned char gitmodule_sha1[20];
Could also use comments. What does the gitmodule_sha1 field represent?
[...]
--- /dev/null
+++ b/submodule-config-cache.c
@@ -0,0 +1,256 @@
+#include cache.h
+#include submodule-config-cache.h
+#include strbuf.h
+
+struct submodule_config_entry {
+ struct hashmap_entry ent;
+ struct submodule_config *config;
+};
+
+static int submodule_config_path_cmp(const struct submodule_config_entry *a,
+ const struct submodule_config_entry *b,
+ const void *unused)
+{
+ int ret;
+ if ((ret = strcmp(a-config-path.buf, b-config-path.buf)))
+ return ret;
Style: avoid assignments in conditionals:
ret = strcmp(...);
if (ret)
return ret;
return hashcmp(...);
The actual return value from strcmp isn't important since
hashmap_cmp_fn is only used to check for equality. Perhaps as a
separate change it would make sense to make it a hashmap_equality_fn
some day:
return !strcmp(...) !hashcmp(...);
This checks both the path and gitmodule_sha1, so I guess it's for
looking up submodule names.
[...]
+static int submodule_config_name_cmp(const struct submodule_config_entry *a,
+ const struct submodule_config_entry *b,
+ const void *unused)
Likewise.
[...]
+void
Re: Corner case bug caused by shell dependent behavior
Hi, Uwe Storbeck wrote: To reproduce the behavior (with dash as /bin/sh): mkdir test cd test git init echo 1 foo git add foo git commit -mthis commit message ends with '\n' echo 2 foo git commit -a --fixup HEAD git rebase -i --autosquash --root Now the editor opens with garbage in line 3 which has to be removed or the rebase fails. Would it make sense to add this as a test to e.g. t/t3404-rebase-interactive.sh? The attached one-line patch fixes the bug. May we have your sign-off? (See Documentation/SubmittingPatches section Sign your work for what this means. Looks obviously correct, so for what it's worth, Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] simplifying branch.c:install_branch_config() if()
Hi, Nemina Amarasinghe wrote: Signed-off-by: Nemina Amarasinghe nemi...@gmail.com The above is a place to explain why this is a good change. For example: When it prints a message indicating what it has done, install_branch_config() treats the (!remote_is_branch origin) and (!remote_is_branch !origin) cases separately. But they are the same, and it uses the same message for both. Simplify by just checking for !remote_is_branch. Noticed using the magic-identical-branch-checker tool. Signed-off-by: ... (That's just a first random hypothetical guess --- of course please do not use it but put your own rationale for the change there.) [...] --- a/branch.c +++ b/branch.c @@ -87,16 +87,11 @@ void install_branch_config(int flag, const char *local, const char *origin, cons _(Branch %s set up to track local branch %s by rebasing.) : _(Branch %s set up to track local branch %s.), local, shortname); - else if (!remote_is_branch origin) + else if (!remote_is_branch) printf_ln(rebasing ? _(Branch %s set up to track remote ref %s by rebasing.) : _(Branch %s set up to track remote ref %s.), local, remote); - else if (!remote_is_branch !origin) - printf_ln(rebasing ? - _(Branch %s set up to track local ref %s by rebasing.) : - _(Branch %s set up to track local ref %s.), - local, remote); Is this safe? Today branch.c::create_branch checks that the argument to e.g. --set-upstream-to is either in refs/heads/* or the image of some remote, but what is making sure that remains true tomorrow? So if changing this, I would be happier if the if condition were still (!remote_is_branch origin) so the impossible case could emit a BUG. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Uwe Storbeck wrote:
Backslash sequences are interpreted as control characters
by the echo command of some shells (e.g. dash).
This has bothered me for a while but never enough to do anything about
it. Thanks for fixing it.
Signed-off-by: Uwe Storbeck u...@ibr.ch
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
(patch left unsnipped for reference)
---
t/test-lib.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..8209204 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -277,7 +277,7 @@ error Test script did not set test_description.
if test $help = t
then
- echo $test_description
+ printf '%s\n' $test_description
exit 0
fi
@@ -328,7 +328,7 @@ test_failure_ () {
test_failure=$(($test_failure + 1))
say_color error not ok $test_count - $1
shift
- echo $@ | sed -e 's/^/# /'
+ printf '%s\n' $@ | sed -e 's/^/# /'
test $immediate = || { GIT_EXIT_OK=t; exit 1; }
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Junio C Hamano wrote:
Uwe Storbeck wrote:
+ printf '%s\n' $@ | sed -e 's/^/# /'
This is wrong, isn't it? Why do we want one line per item here?
Yes, Hannes caught the same, too. Sorry for the sloppiness.
We currently use echo all over the place (e.g., 'echo $path' in
git-sh-setup), and every time we fix it there is a chance of making
mistakes. I wonder if it would make sense to add a helper to make the
echo calls easier to replace:
-- 8 --
Subject: git-sh-setup: introduce sane_echo helper
Using 'echo' with arguments that might contain backslashes or -e or
-n can produce confusing results that vary from platform to platform
(e.g., dash always interprets \ escape sequences and echoes -e
verbatim, whereas bash does not interpret \ escapes unless -e is
passed as an argument to echo and suppresses the -e from its
output).
Instead, we should use printf, which is more predictable:
printf '%s\n' $foo; # Just prints $foo, on all platforms.
Blindly replacing echo with printf '%s\n' would not be good enough
because that printf prints each argument on its own line. Provide a
sane_echo helper that prints its arguments, space-delimited, on a
single line, to make this easier to remember, and tweak 'say'
and 'die_with_status' to illustrate how it is used.
No functional change intended.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
git-sh-setup.sh | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git i/git-sh-setup.sh w/git-sh-setup.sh
index 256c89a..f35b5b9 100644
--- i/git-sh-setup.sh
+++ w/git-sh-setup.sh
@@ -43,6 +43,10 @@ git_broken_path_fix () {
# @@BROKEN_PATH_FIX@@
+sane_echo () {
+ printf '%s\n' $*
+}
+
die () {
die_with_status 1 $@
}
@@ -50,7 +54,7 @@ die () {
die_with_status () {
status=$1
shift
- printf 2 '%s\n' $*
+ sane_echo 2 $*
exit $status
}
@@ -59,7 +63,7 @@ GIT_QUIET=
say () {
if test -z $GIT_QUIET
then
- printf '%s\n' $*
+ sane_echo $*
fi
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: We currently use echo all over the place (e.g., 'echo $path' in git-sh-setup), and every time we fix it there is a chance of making mistakes. I wonder if it would make sense to add a helper to make the echo calls easier to replace: I agree that we would benefit from having a helper to print a single line, which we very often do, without having to worry about the boilerplate '%s\n' of printf or the portability gotcha of echo. I am a bit reluctant to name the helper sane_echo to declare echo that interprets backslashes in the string is insane, though. For these print a single line uses, we are only interested in using a subset of the features offered by 'echo', but that does not mean the other features we do not want to trigger in our use is of no use to any sane person. In a portable script, uncareful use of 'echo' is always insane. In a script tailored to an environment where echo behaves consistently it is perfectly reasonable to use 'echo', but that's a different story. In the context of git, saying Here is the thing you should always use instead of echo is a good thing, in my opinion. Hoping that clarifies, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Found a bug in git 1.9.0 but can't reproduce it without copyrighted source code.
Hi, yun sheng wrote: these two files have the same timestamp, the same size, bug slightly different contents. How did they get the same timestamp? [...] Git I'm using is msysgit 1.9.0 on windows 7 Unixy operating systems have other fields like inode number and ctime that make it possible to notice that a file might have been changed without actually rereading it. Unfortunately Git for Windows is limited to what's in the WIN32_FILE_ATTRIBUTE_DATA which means the size, mtime, and mode are basically all it has to go by. Do you know of some other Windows API call that could help? Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
