* [PATCH v2 05/19] reset.c: extract function for parsing arguments
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
Declutter cmd_reset() a bit by moving out the argument parsing to its
own function.
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
builtin/reset.c | 70 +++++++++++++++++++++++++++++++--------------------------
1 file changed, 38 insertions(+), 32 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 664fad9..58f0f61 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -198,36 +198,11 @@ static void die_if_unmerged_cache(int reset_type)
}
-int cmd_reset(int argc, const char **argv, const char *prefix)
+static const char **parse_args(int argc, const char **argv, const char *prefix, const char **rev_ret)
{
- int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
- int patch_mode = 0;
+ int i = 0;
const char *rev = "HEAD";
- unsigned char sha1[20], *orig = NULL, sha1_orig[20],
- *old_orig = NULL, sha1_old_orig[20];
- const char **pathspec = NULL;
- struct commit *commit;
- struct strbuf msg = STRBUF_INIT;
- const struct option options[] = {
- OPT__QUIET(&quiet, N_("be quiet, only report errors")),
- OPT_SET_INT(0, "mixed", &reset_type,
- N_("reset HEAD and index"), MIXED),
- OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT),
- OPT_SET_INT(0, "hard", &reset_type,
- N_("reset HEAD, index and working tree"), HARD),
- OPT_SET_INT(0, "merge", &reset_type,
- N_("reset HEAD, index and working tree"), MERGE),
- OPT_SET_INT(0, "keep", &reset_type,
- N_("reset HEAD but keep local changes"), KEEP),
- OPT_BOOLEAN('p', "patch", &patch_mode, N_("select hunks interactively")),
- OPT_END()
- };
-
- git_config(git_default_config, NULL);
-
- argc = parse_options(argc, argv, prefix, options, git_reset_usage,
- PARSE_OPT_KEEP_DASHDASH);
-
+ unsigned char unused[20];
/*
* Possible arguments are:
*
@@ -250,7 +225,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
* Otherwise, argv[i] could be either <rev> or <paths> and
* has to be unambiguous.
*/
- else if (!get_sha1_committish(argv[i], sha1)) {
+ else if (!get_sha1_committish(argv[i], unused)) {
/*
* Ok, argv[i] looks like a rev; it should not
* be a filename.
@@ -262,6 +237,40 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
verify_filename(prefix, argv[i], 1);
}
}
+ *rev_ret = rev;
+ return i < argc ? get_pathspec(prefix, argv + i) : NULL;
+}
+
+int cmd_reset(int argc, const char **argv, const char *prefix)
+{
+ int reset_type = NONE, update_ref_status = 0, quiet = 0;
+ int patch_mode = 0;
+ const char *rev;
+ unsigned char sha1[20], *orig = NULL, sha1_orig[20],
+ *old_orig = NULL, sha1_old_orig[20];
+ const char **pathspec = NULL;
+ struct commit *commit;
+ struct strbuf msg = STRBUF_INIT;
+ const struct option options[] = {
+ OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+ OPT_SET_INT(0, "mixed", &reset_type,
+ N_("reset HEAD and index"), MIXED),
+ OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT),
+ OPT_SET_INT(0, "hard", &reset_type,
+ N_("reset HEAD, index and working tree"), HARD),
+ OPT_SET_INT(0, "merge", &reset_type,
+ N_("reset HEAD, index and working tree"), MERGE),
+ OPT_SET_INT(0, "keep", &reset_type,
+ N_("reset HEAD but keep local changes"), KEEP),
+ OPT_BOOLEAN('p', "patch", &patch_mode, N_("select hunks interactively")),
+ OPT_END()
+ };
+
+ git_config(git_default_config, NULL);
+
+ argc = parse_options(argc, argv, prefix, options, git_reset_usage,
+ PARSE_OPT_KEEP_DASHDASH);
+ pathspec = parse_args(argc, argv, prefix, &rev);
if (get_sha1_committish(rev, sha1))
die(_("Failed to resolve '%s' as a valid ref."), rev);
@@ -277,9 +286,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("Could not parse object '%s'."), rev);
hashcpy(sha1, commit->object.sha1);
- if (i < argc)
- pathspec = get_pathspec(prefix, argv + i);
-
if (patch_mode) {
if (reset_type != NONE)
die(_("--patch is incompatible with --{hard,mixed,soft}"));
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* [PATCH v2 13/19] reset.c: move lock, write and commit out of update_index_refresh()
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
In preparation for the/a following patch, move the locking, writing
and committing of the index file out of update_index_refresh(). The
code duplication caused will soon be taken care of. What remains of
update_index_refresh() is just one line, but it is still called from
two places, so let's leave it for now.
In the process, we expose and fix the minor UI bug that makes us print
"Could not refresh index" when we fail to write the index file when
invoked with a pathspec. Copy the error message from the pathspec-less
codepath ("Could not write new index file.").
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
builtin/reset.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 70733c2..c1d6ef2 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -109,19 +109,10 @@ static void print_new_head_line(struct commit *commit)
printf("\n");
}
-static int update_index_refresh(int fd, struct lock_file *index_lock, int flags)
+static void update_index_refresh(int flags)
{
- if (!index_lock) {
- index_lock = xcalloc(1, sizeof(struct lock_file));
- fd = hold_locked_index(index_lock, 1);
- }
-
refresh_index(&the_index, (flags), NULL, NULL,
_("Unstaged changes after reset:"));
- if (write_cache(fd, active_cache, active_nr) ||
- commit_locked_index(index_lock))
- return error ("Could not refresh index");
- return 0;
}
static void update_index_from_diff(struct diff_queue_struct *q,
@@ -321,9 +312,14 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (pathspec) {
struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
int index_fd = hold_locked_index(lock, 1);
- return read_from_tree(pathspec, sha1) ||
- update_index_refresh(index_fd, lock,
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (read_from_tree(pathspec, sha1))
+ return 1;
+ update_index_refresh(
+ quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (write_cache(index_fd, active_cache, active_nr) ||
+ commit_locked_index(lock))
+ return error("Could not write new index file.");
+ return 0;
}
/* Soft reset does not touch the index file nor the working tree
@@ -351,9 +347,15 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type == HARD && !update_ref_status && !quiet)
print_new_head_line(commit);
- else if (reset_type == MIXED) /* Report what has not been updated. */
- update_index_refresh(0, NULL,
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ else if (reset_type == MIXED) { /* Report what has not been updated. */
+ struct lock_file *index_lock = xcalloc(1, sizeof(struct lock_file));
+ int fd = hold_locked_index(index_lock, 1);
+ update_index_refresh(
+ quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (write_cache(fd, active_cache, active_nr) ||
+ commit_locked_index(index_lock))
+ error("Could not refresh index");
+ }
remove_branch_state();
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* [PATCH v2 11/19] reset.c: replace switch by if-else
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
The switch statement towards the end of reset.c is missing case arms
for KEEP and MERGE for no obvious reason, and soon the only non-empty
case arm will be the one for HARD. So let's proactively replace it by
if-else, which will let us move one if statement out without leaving
funny-looking left-overs.
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
builtin/reset.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 97fa9f7..c3eb2eb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -349,18 +349,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
* saving the previous head in ORIG_HEAD before. */
update_ref_status = update_refs(rev, sha1);
- switch (reset_type) {
- case HARD:
- if (!update_ref_status && !quiet)
- print_new_head_line(commit);
- break;
- case SOFT: /* Nothing else to do. */
- break;
- case MIXED: /* Report what has not been updated. */
+ if (reset_type == HARD && !update_ref_status && !quiet)
+ print_new_head_line(commit);
+ else if (reset_type == MIXED) /* Report what has not been updated. */
update_index_refresh(0, NULL,
quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
- break;
- }
remove_branch_state();
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* [PATCH v2 16/19] reset.c: inline update_index_refresh()
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
Now that there is only one caller left to the single-line method
update_index_refresh(), inline it.
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
builtin/reset.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index c316d9b..520c1a5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -109,12 +109,6 @@ static void print_new_head_line(struct commit *commit)
printf("\n");
}
-static void update_index_refresh(int flags)
-{
- refresh_index(&the_index, (flags), NULL, NULL,
- _("Unstaged changes after reset:"));
-}
-
static void update_index_from_diff(struct diff_queue_struct *q,
struct diff_options *opt, void *data)
{
@@ -329,9 +323,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("Could not reset index file to revision '%s'."), rev);
}
- if (reset_type == MIXED) /* Report what has not been updated. */
- update_index_refresh(
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (reset_type == MIXED) { /* Report what has not been updated. */
+ int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
+ refresh_index(&the_index, flags, NULL, NULL,
+ _("Unstaged changes after reset:"));
+ }
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock))
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* [PATCH v2 07/19] reset.c: extract function for updating {ORIG_,}HEAD
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
By extracting the code for updating the HEAD and ORIG_HEAD symbolic
references to a separate function, we declutter cmd_reset() a bit and
we make it clear that e.g. the four variables {,sha1_}{,old_}orig are
only used by this code.
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
builtin/reset.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index d89cf4d..2187d64 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -240,16 +240,35 @@ static const char **parse_args(const char **argv, const char *prefix, const char
return argv[0] ? get_pathspec(prefix, argv) : NULL;
}
+static int update_refs(const char *rev, const unsigned char *sha1)
+{
+ int update_ref_status;
+ struct strbuf msg = STRBUF_INIT;
+ unsigned char *orig = NULL, sha1_orig[20],
+ *old_orig = NULL, sha1_old_orig[20];
+
+ if (!get_sha1("ORIG_HEAD", sha1_old_orig))
+ old_orig = sha1_old_orig;
+ if (!get_sha1("HEAD", sha1_orig)) {
+ orig = sha1_orig;
+ set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
+ update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
+ } else if (old_orig)
+ delete_ref("ORIG_HEAD", old_orig, 0);
+ set_reflog_message(&msg, "updating HEAD", rev);
+ update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+ strbuf_release(&msg);
+ return update_ref_status;
+}
+
int cmd_reset(int argc, const char **argv, const char *prefix)
{
int reset_type = NONE, update_ref_status = 0, quiet = 0;
int patch_mode = 0;
const char *rev;
- unsigned char sha1[20], *orig = NULL, sha1_orig[20],
- *old_orig = NULL, sha1_old_orig[20];
+ unsigned char sha1[20];
const char **pathspec = NULL;
struct commit *commit;
- struct strbuf msg = STRBUF_INIT;
const struct option options[] = {
OPT__QUIET(&quiet, N_("be quiet, only report errors")),
OPT_SET_INT(0, "mixed", &reset_type,
@@ -333,17 +352,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
/* Any resets update HEAD to the head being switched to,
* saving the previous head in ORIG_HEAD before. */
- if (!get_sha1("ORIG_HEAD", sha1_old_orig))
- old_orig = sha1_old_orig;
- if (!get_sha1("HEAD", sha1_orig)) {
- orig = sha1_orig;
- set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
- update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
- }
- else if (old_orig)
- delete_ref("ORIG_HEAD", old_orig, 0);
- set_reflog_message(&msg, "updating HEAD", rev);
- update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+ update_ref_status = update_refs(rev, sha1);
switch (reset_type) {
case HARD:
@@ -360,7 +369,5 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
remove_branch_state();
- strbuf_release(&msg);
-
return update_ref_status;
}
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* [PATCH v2 09/19] reset --keep: only write index file once
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
"git reset --keep" calls reset_index_file() twice, first doing a
two-way merge to the target revision, updating the index and worktree,
and then resetting the index. After each call, we write the index
file.
In the unlikely event that the second call to reset_index_file()
fails, the index will have been merged to the target revision, but
HEAD will not be updated, leaving the user with a dirty index.
By moving the locking, writing and committing out of
reset_index_file() and into the caller, we can avoid writing the index
twice, thereby making the sure we don't end up in the half-way reset
state. As a bonus, we speed up "git reset --keep" a little on the
linux-2.6 repo (best of five, warm cache):
Before After
real 0m0.315s 0m0.296s
user 0m0.290s 0m0.280s
sys 0m0.020s 0m0.010s
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
builtin/reset.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 4e34195..7c440ad 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -38,14 +38,12 @@ static inline int is_merge(void)
return !access(git_path("MERGE_HEAD"), F_OK);
}
-static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet)
+static int reset_index(const unsigned char *sha1, int reset_type, int quiet)
{
int nr = 1;
- int newfd;
struct tree_desc desc[2];
struct tree *tree;
struct unpack_trees_options opts;
- struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
memset(&opts, 0, sizeof(opts));
opts.head_idx = 1;
@@ -67,8 +65,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
opts.reset = 1;
}
- newfd = hold_locked_index(lock, 1);
-
read_cache_unmerged();
if (reset_type == KEEP) {
@@ -91,10 +87,6 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
prime_cache_tree(&active_cache_tree, tree);
}
- if (write_cache(newfd, active_cache, active_nr) ||
- commit_locked_index(lock))
- return error(_("Could not write new index file."));
-
return 0;
}
@@ -341,9 +333,16 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die_if_unmerged_cache(reset_type);
if (reset_type != SOFT) {
- int err = reset_index_file(sha1, reset_type, quiet);
+ struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+ int newfd = hold_locked_index(lock, 1);
+ int err = reset_index(sha1, reset_type, quiet);
if (reset_type == KEEP && !err)
- err = reset_index_file(sha1, MIXED, quiet);
+ err = reset_index(sha1, MIXED, quiet);
+ if (!err &&
+ (write_cache(newfd, active_cache, active_nr) ||
+ commit_locked_index(lock))) {
+ err = error(_("Could not write new index file."));
+ }
if (err)
die(_("Could not reset index file to revision '%s'."), rev);
}
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* [PATCH v2 00/19] reset improvements
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1357719376-16406-1-git-send-email-martinvonz@gmail.com>
Changes since v1:
- Spelling fixes.
- Explained how "git reset -- $pathspec" in bare repo is broken.
- Provided motivation for replacement of switch by if-else
- Fixed argv/argc handling by removing use of argc.
- Replaced "don't refresh index on --quiet" patch by one that just
inlines update_index_refresh()
- Incorporated fixes from Junio's repo
- Provided some motivation for "replace switch by if-else" amd moved
the patch later in the series.
Thanks for reviewing!
Martin von Zweigbergk (19):
reset $pathspec: no need to discard index
reset $pathspec: exit with code 0 if successful
reset.c: pass pathspec around instead of (prefix, argv) pair
reset: don't allow "git reset -- $pathspec" in bare repo
reset.c: extract function for parsing arguments
reset.c: remove unnecessary variable 'i'
reset.c: extract function for updating {ORIG_,}HEAD
reset.c: share call to die_if_unmerged_cache()
reset --keep: only write index file once
reset: avoid redundant error message
reset.c: replace switch by if-else
reset.c: move update_index_refresh() call out of read_from_tree()
reset.c: move lock, write and commit out of update_index_refresh()
reset [--mixed]: only write index file once
reset.c: finish entire cmd_reset() whether or not pathspec is given
reset.c: inline update_index_refresh()
reset $sha1 $pathspec: require $sha1 only to be treeish
reset: allow reset on unborn branch
reset [--mixed]: use diff-based reset whether or not pathspec was
given
builtin/reset.c | 283 +++++++++++++++++++----------------------
t/t2013-checkout-submodule.sh | 2 +-
t/t7102-reset.sh | 26 +++-
t/t7106-reset-unborn-branch.sh | 52 ++++++++
4 files changed, 203 insertions(+), 160 deletions(-)
create mode 100755 t/t7106-reset-unborn-branch.sh
--
1.8.1.1.454.gce43f05
^ permalink raw reply
* [PATCH v2 12/19] reset.c: move update_index_refresh() call out of read_from_tree()
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
The final part of cmd_reset() essentially looks like:
if (pathspec) {
...
read_from_tree(...);
} else {
...
reset_index(...);
update_index_refresh(...);
...
}
where read_from_tree() internally also calls
update_index_refresh(). Move the call to update_index_refresh() out of
read_from_tree for symmetry with the 'else' block, making
read_from_tree() and reset_index() closer in functionality.
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
builtin/reset.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index c3eb2eb..70733c2 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -145,11 +145,8 @@ static void update_index_from_diff(struct diff_queue_struct *q,
}
}
-static int read_from_tree(const char **pathspec, unsigned char *tree_sha1,
- int refresh_flags)
+static int read_from_tree(const char **pathspec, unsigned char *tree_sha1)
{
- struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
- int index_fd;
struct diff_options opt;
memset(&opt, 0, sizeof(opt));
@@ -157,7 +154,6 @@ static int read_from_tree(const char **pathspec, unsigned char *tree_sha1,
opt.output_format = DIFF_FORMAT_CALLBACK;
opt.format_callback = update_index_from_diff;
- index_fd = hold_locked_index(lock, 1);
read_cache();
if (do_diff_cache(tree_sha1, &opt))
return 1;
@@ -165,7 +161,7 @@ static int read_from_tree(const char **pathspec, unsigned char *tree_sha1,
diff_flush(&opt);
diff_tree_release_paths(&opt);
- return update_index_refresh(index_fd, lock, refresh_flags);
+ return 0;
}
static void set_reflog_message(struct strbuf *sb, const char *action,
@@ -322,9 +318,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die(_("%s reset is not allowed in a bare repository"),
_(reset_type_names[reset_type]));
- if (pathspec)
- return read_from_tree(pathspec, sha1,
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (pathspec) {
+ struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+ int index_fd = hold_locked_index(lock, 1);
+ return read_from_tree(pathspec, sha1) ||
+ update_index_refresh(index_fd, lock,
+ quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ }
/* Soft reset does not touch the index file nor the working tree
* at all, but requires them in a good order. Other resets reset
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* [PATCH v2 08/19] reset.c: share call to die_if_unmerged_cache()
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
Use a single condition to guard the call to die_if_unmerged_cache for
both --soft and --keep. This avoids the small distraction of the
precondition check from the logic following it.
Also change an instance of
if (e)
err = err || f();
to the almost as short, but clearer
if (e && !err)
err = f();
(which is equivalent since we only care whether exit code is 0)
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
builtin/reset.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 2187d64..4e34195 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -337,15 +337,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
/* Soft reset does not touch the index file nor the working tree
* at all, but requires them in a good order. Other resets reset
* the index file to the tree object we are switching to. */
- if (reset_type == SOFT)
+ if (reset_type == SOFT || reset_type == KEEP)
die_if_unmerged_cache(reset_type);
- else {
- int err;
- if (reset_type == KEEP)
- die_if_unmerged_cache(reset_type);
- err = reset_index_file(sha1, reset_type, quiet);
- if (reset_type == KEEP)
- err = err || reset_index_file(sha1, MIXED, quiet);
+
+ if (reset_type != SOFT) {
+ int err = reset_index_file(sha1, reset_type, quiet);
+ if (reset_type == KEEP && !err)
+ err = reset_index_file(sha1, MIXED, quiet);
if (err)
die(_("Could not reset index file to revision '%s'."), rev);
}
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* Re: What's cooking in git.git (Jan 2013, #06; Mon, 14)
From: Chris Rorvick @ 2013-01-15 6:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric S. Raymond, git, Michael Haggerty, Jonathan Nieder
In-Reply-To: <7v1udn6tdg.fsf@alter.siamese.dyndns.org>
On Mon, Jan 14, 2013 at 9:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I converted one of Chris's follow-up test tweaks to this to
> illustrate how it can be done without breaking tests for the
> original cvsimport, but didn't do all of them. Chris, is this a
> foundation we can work together on top?
Sure, looks straightforward and makes things easier.
Thanks,
Chris
^ permalink raw reply
* [PATCH v2 06/19] reset.c: remove unnecessary variable 'i'
From: Martin von Zweigbergk @ 2013-01-15 5:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-1-git-send-email-martinvonz@gmail.com>
Throughout most of parse_args(), the variable 'i' remains at 0. Many
references are still made to the variable even when it could only have
the value 0. This made at least me, who has relatively little
experience with C programming styles, think that parts of the function
was meant to be part of a loop. To avoid such confusion, remove the
variable and also the 'argc' parameter and check for NULL trailing
argv instead.
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
---
I explained a bit more why I was confused by the current style, but
I'm also perfectly happy if you just drop the patch (there would be
some minor conflicts in a later patch, though).
builtin/reset.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 58f0f61..d89cf4d 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -198,9 +198,8 @@ static void die_if_unmerged_cache(int reset_type)
}
-static const char **parse_args(int argc, const char **argv, const char *prefix, const char **rev_ret)
+static const char **parse_args(const char **argv, const char *prefix, const char **rev_ret)
{
- int i = 0;
const char *rev = "HEAD";
unsigned char unused[20];
/*
@@ -211,34 +210,34 @@ static const char **parse_args(int argc, const char **argv, const char *prefix,
* git reset [-opts] -- <paths>...
* git reset [-opts] <paths>...
*
- * At this point, argv[i] points immediately after [-opts].
+ * At this point, argv points immediately after [-opts].
*/
- if (i < argc) {
- if (!strcmp(argv[i], "--")) {
- i++; /* reset to HEAD, possibly with paths */
- } else if (i + 1 < argc && !strcmp(argv[i+1], "--")) {
- rev = argv[i];
- i += 2;
+ if (argv[0]) {
+ if (!strcmp(argv[0], "--")) {
+ argv++; /* reset to HEAD, possibly with paths */
+ } else if (argv[1] && !strcmp(argv[1], "--")) {
+ rev = argv[0];
+ argv += 2;
}
/*
- * Otherwise, argv[i] could be either <rev> or <paths> and
+ * Otherwise, argv[0] could be either <rev> or <paths> and
* has to be unambiguous.
*/
- else if (!get_sha1_committish(argv[i], unused)) {
+ else if (!get_sha1_committish(argv[0], unused)) {
/*
- * Ok, argv[i] looks like a rev; it should not
+ * Ok, argv[0] looks like a rev; it should not
* be a filename.
*/
- verify_non_filename(prefix, argv[i]);
- rev = argv[i++];
+ verify_non_filename(prefix, argv[0]);
+ rev = *argv++;
} else {
/* Otherwise we treat this as a filename */
- verify_filename(prefix, argv[i], 1);
+ verify_filename(prefix, argv[0], 1);
}
}
*rev_ret = rev;
- return i < argc ? get_pathspec(prefix, argv + i) : NULL;
+ return argv[0] ? get_pathspec(prefix, argv) : NULL;
}
int cmd_reset(int argc, const char **argv, const char *prefix)
@@ -270,7 +269,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, git_reset_usage,
PARSE_OPT_KEEP_DASHDASH);
- pathspec = parse_args(argc, argv, prefix, &rev);
+ pathspec = parse_args(argv, prefix, &rev);
if (get_sha1_committish(rev, sha1))
die(_("Failed to resolve '%s' as a valid ref."), rev);
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* Re: [PATCH 3/3] cvsimport: start adding cvsps 3.x support
From: Chris Rorvick @ 2013-01-15 6:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jrnieder, mhagger, esr
In-Reply-To: <1358127629-7500-4-git-send-email-gitster@pobox.com>
On Sun, Jan 13, 2013 at 7:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The new cvsps 3.x series lacks support of some options cvsps 2.x
> series had and used by cvsimport-2; add a replacement program from
> the author of cvsps 3.x and allow users to choose it by setting the
> GIT_CVSPS_VERSION environment variable to 3. We would later add
> support to auto-detect the version of installed cvsps to this code
> when the environment variable is not set.
>
> Note that in this step, cvsimport-3 that relies on cvsps 3.x does
> not have any test coverage. As cvsimport-3 supports most of the
> command line options cvsimport-2, we should be able to tweak some of
> t96xx tests and run them with GIT_CVSPS_VERSION set to both 2 and 3,
> but that is a topic of later patches that should come on top.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Makefile | 18 ++-
> git-cvsimport-3.py | 344 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> git-cvsimport.sh | 4 +-
> 3 files changed, 359 insertions(+), 7 deletions(-)
> create mode 100755 git-cvsimport-3.py
>
> diff --git a/Makefile b/Makefile
> index b022db2..060cdc2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -470,8 +470,9 @@ SCRIPT_PERL += git-relink.perl
> SCRIPT_PERL += git-send-email.perl
> SCRIPT_PERL += git-svn.perl
>
> -SCRIPT_PYTHON += git-remote-testpy.py
> +SCRIPT_PYTHON += git-cvsimport-3.py
> SCRIPT_PYTHON += git-p4.py
> +SCRIPT_PYTHON += git-remote-testpy.py
>
> SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
> $(patsubst %.perl,%,$(SCRIPT_PERL)) \
> @@ -575,8 +576,11 @@ endif
> ifndef CVSPS2_PATH
> CVSPS2_PATH = cvsps
> endif
> +ifndef CVSPS3_PATH
> + CVSPS3_PATH = cvsps
> +endif
>
> -export PERL_PATH PYTHON_PATH CVSPS2_PATH
> +export PERL_PATH PYTHON_PATH CVSPS2_PATH CVSPS3_PATH
>
> LIB_FILE = libgit.a
> XDIFF_LIB = xdiff/lib.a
> @@ -1515,6 +1519,7 @@ PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
> PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
> TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
> CVSPS2_PATH_SQ = $(subst ','\'',$(CVSPS2_PATH))
> +CVSPS3_PATH_SQ = $(subst ','\'',$(CVSPS3_PATH))
> DIFF_SQ = $(subst ','\'',$(DIFF))
>
> LIBS = $(GITLIBS) $(EXTLIBS)
> @@ -1757,12 +1762,15 @@ ifndef NO_PYTHON
> $(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
> $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
> $(QUIET_GEN)$(RM) $@ $@+ && \
> - INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
> + INSTLIBDIR_SQ=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
> --no-print-directory prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' \
> - instlibdir` && \
> + instlibdir | \
> + sed -e "s/'/'\''/g"` && \
> + echo "InstLibDir is <$$INSTLIBDIR_SQ>" && \
> sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
> -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"@@INSTLIBDIR@@")|' \
> - -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
> + -e 's|@@CVSPS3_PATH@@|$(CVSPS3_PATH_SQ)|g' \
> + -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR_SQ"'|g' \
> $@.py >$@+ && \
> chmod +x $@+ && \
> mv $@+ $@
> diff --git a/git-cvsimport-3.py b/git-cvsimport-3.py
> new file mode 100755
> index 0000000..57f13b7
> --- /dev/null
> +++ b/git-cvsimport-3.py
> @@ -0,0 +1,344 @@
> +#!/usr/bin/env python
> +#
> +# Import CVS history into git
> +#
> +# Intended to be a near-workalike of Matthias Urlichs's Perl implementation.
> +#
> +# By Eric S. Raymond <esr@thyrsus.com>, December 2012
> +# May be redistributed under the license of the git project.
> +
> +import sys
> +
> +if sys.hexversion < 0x02060000:
> + sys.stderr.write("git cvsimport: requires Python 2.6 or later.\n")
> + sys.exit(1)
> +
> +import os, getopt, subprocess, tempfile, shutil
> +
> +DEBUG_COMMANDS = 1
> +
> +class Fatal(Exception):
> + "Unrecoverable error."
> + def __init__(self, msg):
> + Exception.__init__(self)
> + self.msg = msg
> +
> +def do_or_die(dcmd, legend=""):
> + "Either execute a command or raise a fatal exception."
> + if legend:
> + legend = " " + legend
> + if verbose >= DEBUG_COMMANDS:
> + sys.stdout.write("git cvsimport: executing '%s'%s\n" % (dcmd, legend))
> + try:
> + retcode = subprocess.call(dcmd, shell=True)
> + if retcode < 0:
> + raise Fatal("git cvsimport: child was terminated by signal %d." % -retcode)
> + elif retcode != 0:
> + raise Fatal("git cvsimport: child returned %d." % retcode)
> + except (OSError, IOError) as e:
> + raise Fatal("git cvsimport: execution of %s%s failed: %s" % (dcmd, legend, e))
> +
> +def capture_or_die(dcmd, legend=""):
> + "Either execute a command and capture its output or die."
> + if legend:
> + legend = " " + legend
> + if verbose >= DEBUG_COMMANDS:
> + sys.stdout.write("git cvsimport: executing '%s'%s\n" % (dcmd, legend))
> + try:
> + return subprocess.check_output(dcmd, shell=True)
> + except subprocess.CalledProcessError as e:
> + if e.returncode < 0:
> + sys.stderr.write("git cvsimport: child was terminated by signal %d." % -e.returncode)
> + elif e.returncode != 0:
> + sys.stderr.write("git cvsimport: child returned %d." % e.returncode)
> + sys.exit(1)
> +
> +class cvsps:
> + "Method class for cvsps back end."
> +
> + cvsps = "@@CVSPS3_PATH@@"
> + def __init__(self):
> + self.opts = ""
> + self.revmap = None
> + def set_repo(self, val):
> + "Set the repository root option."
> + if not val.startswith(":"):
> + if not val.startswith(os.sep):
> + val = os.path.abspath(val)
> + val = ":local:" + val
> + self.opts += " --root '%s'" % val
> + def set_authormap(self, val):
> + "Set the author-map file."
> + self.opts += " -A '%s'" % val
> + def set_fuzz(self, val):
> + "Set the commit-similarity window."
> + self.opts += " -z %s" % val
> + def set_nokeywords(self):
> + "Suppress CVS keyword expansion."
> + self.opts += " -k"
> + def add_opts(self, val):
> + "Add options to the engine command line."
> + self.opts += " " + val
> + def set_exclusion(self, val):
> + "Set a file exclusion regexp."
> + self.opts += " -n -f '%s'" % val
> + def set_after(self, val):
> + "Set a date threshold for incremental import."
> + self.opts += " -d '%s'" % val
> + def set_revmap(self, val):
> + "Set the file to which the engine should dump a reference map."
> + self.revmap = val
> + self.opts += " -R '%s'" % self.revmap
> + def set_module(self, val):
> + "Set the module to query."
> + self.opts += " " + val
> + def command(self):
> + "Emit the command implied by all previous options."
> + return self.cvsps + "--fast-export " + self.opts
"--fast-export" string is missing a leading space. With this fix and
the latest cvsps build I'm seeing 6 of 15 failures for t9650 which is
what I was getting out of the patched t9600.
Chris
^ permalink raw reply
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Junio C Hamano @ 2013-01-15 6:22 UTC (permalink / raw)
To: Jardel Weyrich; +Cc: Michael J Gruber, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <1D472234-A0A5-4F02-878D-D05DEE995FCD@gmail.com>
Jardel Weyrich <jweyrich@gmail.com> writes:
> If you allow me, I'd like you to forget about the concepts for a minute, and focus on the user experience.
> Imagine a simple hypothetical scenario in which the user wants to push to 2 distinct repositories. He already has cloned the repo from the 1st repository, thus (theoretically) all he needs to do, is to add a new repository for push. He then uses `remote set-url --add --push <2nd-repo>` (which I personally thought would suffice). However, if he tries to push a new commit to this remote, it would be pushed _only_ to the 2nd-repo.
The primary reason behind push-url was that
(1) usually you push to and fetch from the same, so no pushUrl is
ever needed, just a single Url will do (this is often true for
cvs/svn style shared repository workflow); and
(2) sometimes you want to fetch from one place and push to another
(this is often true for "fetch from upstream, push to my own
and ask upstream to pull from it" workflow), and in that case
you want pushUrl in addition to Url. Most importantly, in this
case, you do *NOT* want to push to Url. You only push to
pushUrl.
Setting *one* pushURL is a way to say "That URL I fetch from is
*not* the place I want to push (I may not even be able to push
there); when I say 'push', push there instead". Your proposed
semantics will make it impossible to arrange such an asymmetric
setting.
^ permalink raw reply
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Junio C Hamano @ 2013-01-15 6:39 UTC (permalink / raw)
To: Jardel Weyrich; +Cc: Michael J Gruber, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <7vpq1755jb.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jardel Weyrich <jweyrich@gmail.com> writes:
>
>> If you allow me, I'd like you to forget about the concepts for a minute, and focus on the user experience.
>> Imagine a simple hypothetical scenario in which the user wants to push to 2 distinct repositories. He already has cloned the repo from the 1st repository, thus (theoretically) all he needs to do, is to add a new repository for push. He then uses `remote set-url --add --push <2nd-repo>` (which I personally thought would suffice). However, if he tries to push a new commit to this remote, it would be pushed _only_ to the 2nd-repo.
>
> The primary reason behind push-url was that
>
> (1) usually you push to and fetch from the same, so no pushUrl is
> ever needed, just a single Url will do (this is often true for
> cvs/svn style shared repository workflow); and
>
> (2) sometimes you want to fetch from one place and push to another
> (this is often true for "fetch from upstream, push to my own
> and ask upstream to pull from it" workflow), and in that case
> you want pushUrl in addition to Url. Most importantly, in this
> case, you do *NOT* want to push to Url. You only push to
> pushUrl.
>
> Setting *one* pushURL is a way to say "That URL I fetch from is
> *not* the place I want to push (I may not even be able to push
> there); when I say 'push', push there instead". Your proposed
> semantics will make it impossible to arrange such an asymmetric
> setting.
Now I think I finally see where that misunderstanding comes from.
It is "remote -v" that is misdesigned.
$ git clone ../there here
$ cd here
$ git remote -v
origin /var/tmp/there (fetch)
origin /var/tmp/there (push)
This is totally bogus. It should report something like this:
$ git remote -v
origin /var/tmp/there (fetch/push)
Then after running "git remote set-url --push origin ../another" we
should see
$ git remote -v
origin /var/tmp/there (fetch)
origin /var/tmp/another (push)
which would make it clear that the original fetch/push came from the
(1) usuall you push and fetch from the same place so there is only
one setting, and the two lines came from the (2) sometimes you need
a separate places to fetch from and push to.
At this point, if you say "set-url --push origin ../third", then
"another" will disappear and gets replaced by "third"; if you
instead say "set-url --add --push origin ../third", then we will see
two (push) lines, in addition to one (fetch), making it clear that
you are still in (2) above, fetching from and pushing to different
places, and having two places to push to.
I misread your response
From: Jardel Weyrich <jweyrich@gmail.com>
Subject: Re: [BUG] Possible bug in `remote set-url --add --push`
Date: Sat, 12 Jan 2013 06:09:35 -0200
Message-ID: <CAN8TAOvP_HX6BEK86aYoX-kVqWDmsbyptxTT2nk+fx+Ut1Tojg@mail.gmail.com>
where you showed that there was only remote.origin.url (and no
pushurl) in the first step, and somehow thought you had a
remote.origin.pushurl in the first place.
^ permalink raw reply
* Re: [PATCH 3/3] cvsimport: start adding cvsps 3.x support
From: Junio C Hamano @ 2013-01-15 6:44 UTC (permalink / raw)
To: Chris Rorvick; +Cc: git, jrnieder, mhagger, esr
In-Reply-To: <CAEUsAPZV6rdFz5R6NN55qYr5se4bFJftE0xGSPAtXLp8jcO0vw@mail.gmail.com>
Chris Rorvick <chris@rorvick.com> writes:
[jc: please elide parts you are not responding to, leaving enough
lines to understand the context]
>> + def command(self):
>> + "Emit the command implied by all previous options."
>> + return self.cvsps + "--fast-export " + self.opts
>
> "--fast-export" string is missing a leading space. With this fix and
> the latest cvsps build I'm seeing 6 of 15 failures for t9650 which is
> what I was getting out of the patched t9600.
Thanks; I'll amend it and push the result out, but I think we will
need to rip this part out and form the command string in a saner way
(e.g. if the path needs to have SP in it, e.g. "/Program Files/cvsps3",
the above would not work correctly anyway).
^ permalink raw reply
* Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields
From: Johannes Sixt @ 2013-01-15 7:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Robin Rosenberg, git
In-Reply-To: <7vy5fv71ad.fsf@alter.siamese.dyndns.org>
Am 1/15/2013 1:11, schrieb Junio C Hamano:
> I'd say a simplistic "ignore if zero is stored" or even "ignore this
> as one of the systems that shares this file writes crap in it" may
> be sufficient, and if this is a jGit specific issue, it might even
> make sense to introduce a single configuration variable with string
> "jgit" somewhere in its name and bypass the stat field comparison
> for known-problematic fields, instead of having the user know and
> list what stat fields need special attention.
It was my suggestion to have a list of names to ignore because the answer
to this question
> Is this "the user edits in eclipse and then runs 'git status' from the
> terminal" problem?
was "It is purely for performance in some situations" back then. But
today, the answer is "Yes". With this new background, your suggestion to
have just a single option that contains the token "jgit" may make more
sense. (core.ignoreCygwinFSTricks may serve as a precedent.) The original
patch was along this way, and the name contained "minimal", which I
objected to.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields
From: Robin Rosenberg @ 2013-01-15 7:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, j sixt, Shawn Pearce
In-Reply-To: <7vy5fv71ad.fsf@alter.siamese.dyndns.org>
----- Ursprungligt meddelande -----
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
>
> > Semantically they're somewhat different. My flags are for ignoring
> > a value when it's not used as indicated by the value zero, while
> > trustctime is for ignoring untrustworthy, non-zero, values.
>
> Yeah, I realized that after writing that message.
>
> > Another thing that I noticed, is that I probably wanto to be able
> > to filter on the precision
> > of timestamps. Again, this i JGit-related. Current JGit has
> > milliseconds precision (max), whereas
> > Git has down to nanosecond precision in timestamps. Newer JGits may
> > get nanoseconds timestamps too,
> > but on current Linux versions JGit gets only integral seconds
> > regardless of file system.
> >
> > Would the names, milli, micro, nano be good for ignoring the tail
> > when zero, or n1..n9 (obviously
> > n2 would be ok too). nN = ignore all but first N nsec digits if
> > they are zero)?
>
> It somehow starts to sound like over-engineering to solve a wrong
> problem.
>
> I'd say a simplistic "ignore if zero is stored" or even "ignore this
> as one of the systems that shares this file writes crap in it" may
> be sufficient, and if this is a jGit specific issue, it might even
> make sense to introduce a single configuration variable with string
> "jgit" somewhere in its name and bypass the stat field comparison
> for known-problematic fields, instead of having the user know and
> list what stat fields need special attention.
My first patch was something like that, just not using the word jgit. As
for what fields to ignore, it's something that can be configured by EGit
and documented on the EGit/JGit wiki.
-- robin
^ permalink raw reply
* Re: git grep performance regression
From: Ross Lagerwall @ 2013-01-15 7:36 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git, Jean-Noël AVILA, Junio C Hamano
In-Reply-To: <CACsJy8AnABCisgSVL7Qh_-uejAJA3x1kzy=4i+uXm3+90m5tuA@mail.gmail.com>
On Tue, Jan 15, 2013 at 11:38:32AM +0700, Duy Nguyen wrote:
> dirlen is not expected to include the trailing slash, but
> find_basename() does that. It messes up with the path filters for
> push/pop in the next code. This brings grep performance closely back
> to before for me. Ross, can you check (patch could be whitespace
> damaged by gmail)?
>
Yes, that did seem to bring back the performance to the old level.
I noticed that before the patch, the strace output was something like
this:
open("tools/vm/.gitattributes", O_RDONLY) = -1 ENOENT
open("tools/vm//.gitattributes", O_RDONLY) = -1 ENOENT
open("tools/vm//.gitattributes", O_RDONLY) = -1 ENOENT
open("tools/vm//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr/.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
(and yes, the whitespace was damaged by Gmail!)
Regards
--
Ross Lagerwall
^ permalink raw reply
* [PATCH v2 00/14] Remove unused code from imap-send.c
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
This is a re-roll, incorporating the feedback of Jonathan Nieder
(thanks!).
Differences from v1:
* Added comments to get_cmd_result() at the place where the
"NAMESPACE" response is skipped over.
* Added some comments to lf_to_crlf(), simplified the code a bit
further, and expanded the commit message.
* Replaced erroneously-deleted space in "APPEND" command in
imap_store_msg().
I also moved the patch rewriting lf_to_crlf() to the end of the
series, because it is not just dead-code elimination like the others.
Michael Haggerty (14):
imap-send.c: remove msg_data::flags, which was always zero
imap-send.c: remove struct msg_data
iamp-send.c: remove unused struct imap_store_conf
imap-send.c: remove struct store_conf
imap-send.c: remove struct message
imap-send.c: remove some unused fields from struct store
imap-send.c: inline imap_parse_list() in imap_list()
imap-send.c: remove struct imap argument to parse_imap_list_l()
imap-send.c: remove namespace fields from struct imap
imap-send.c: remove unused field imap_store::trashnc
imap-send.c: use struct imap_store instead of struct store
imap-send.c: remove unused field imap_store::uidvalidity
imap-send.c: fold struct store into struct imap_store
imap-send.c: simplify logic in lf_to_crlf()
imap-send.c | 308 +++++++++++-------------------------------------------------
1 file changed, 55 insertions(+), 253 deletions(-)
--
1.8.0.3
^ permalink raw reply
* [PATCH v2 03/14] iamp-send.c: remove unused struct imap_store_conf
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 29c10a4..dbe0546 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -130,11 +130,6 @@ static struct imap_server_conf server = {
NULL, /* auth_method */
};
-struct imap_store_conf {
- struct store_conf gen;
- struct imap_server_conf *server;
-};
-
#define NIL (void *)0x1
#define LIST (void *)0x2
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 04/14] imap-send.c: remove struct store_conf
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
It was never used.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index dbe0546..b8a7ff9 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -33,15 +33,6 @@ typedef void *SSL;
#include <openssl/hmac.h>
#endif
-struct store_conf {
- char *name;
- const char *path; /* should this be here? its interpretation is driver-specific */
- char *map_inbox;
- char *trash;
- unsigned max_size; /* off_t is overkill */
- unsigned trash_remote_new:1, trash_only_new:1;
-};
-
/* For message->status */
#define M_RECENT (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */
#define M_DEAD (1<<1) /* expunged */
@@ -55,8 +46,6 @@ struct message {
};
struct store {
- struct store_conf *conf; /* foreign */
-
/* currently open mailbox */
const char *name; /* foreign! maybe preset? */
char *path; /* own */
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 05/14] imap-send.c: remove struct message
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
It was never used.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 26 --------------------------
1 file changed, 26 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index b8a7ff9..9e181e0 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -33,23 +33,10 @@ typedef void *SSL;
#include <openssl/hmac.h>
#endif
-/* For message->status */
-#define M_RECENT (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */
-#define M_DEAD (1<<1) /* expunged */
-#define M_FLAGS (1<<2) /* flags fetched */
-
-struct message {
- struct message *next;
- size_t size; /* zero implies "not fetched" */
- int uid;
- unsigned char flags, status;
-};
-
struct store {
/* currently open mailbox */
const char *name; /* foreign! maybe preset? */
char *path; /* own */
- struct message *msgs; /* own */
int uidvalidity;
unsigned char opts; /* maybe preset? */
/* note that the following do _not_ reflect stats from msgs, but mailbox totals */
@@ -74,8 +61,6 @@ static void imap_warn(const char *, ...);
static char *next_arg(char **);
-static void free_generic_messages(struct message *);
-
__attribute__((format (printf, 3, 4)))
static int nfsnprintf(char *buf, int blen, const char *fmt, ...);
@@ -447,16 +432,6 @@ static char *next_arg(char **s)
return ret;
}
-static void free_generic_messages(struct message *msgs)
-{
- struct message *tmsg;
-
- for (; msgs; msgs = tmsg) {
- tmsg = msgs->next;
- free(msgs);
- }
-}
-
static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
{
int ret;
@@ -914,7 +889,6 @@ static void imap_close_server(struct imap_store *ictx)
static void imap_close_store(struct store *ctx)
{
imap_close_server((struct imap_store *)ctx);
- free_generic_messages(ctx->msgs);
free(ctx);
}
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 9e181e0..f193211 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -36,12 +36,7 @@ typedef void *SSL;
struct store {
/* currently open mailbox */
const char *name; /* foreign! maybe preset? */
- char *path; /* own */
int uidvalidity;
- unsigned char opts; /* maybe preset? */
- /* note that the following do _not_ reflect stats from msgs, but mailbox totals */
- int count; /* # of messages */
- int recent; /* # of recent messages - don't trust this beyond the initial read */
};
static const char imap_send_usage[] = "git imap-send < <mbox>";
@@ -772,13 +767,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
!strcmp("NO", arg) || !strcmp("BYE", arg)) {
if ((resp = parse_response_code(ctx, NULL, cmd)) != RESP_OK)
return resp;
- } else if (!strcmp("CAPABILITY", arg))
+ } else if (!strcmp("CAPABILITY", arg)) {
parse_capability(imap, cmd);
- else if ((arg1 = next_arg(&cmd))) {
- if (!strcmp("EXISTS", arg1))
- ctx->gen.count = atoi(arg);
- else if (!strcmp("RECENT", arg1))
- ctx->gen.recent = atoi(arg);
+ } else if ((arg1 = next_arg(&cmd))) {
+ /* unused */
} else {
fprintf(stderr, "IMAP error: unable to parse untagged response\n");
return RESP_BAD;
@@ -1254,7 +1246,6 @@ static int imap_store_msg(struct store *gctx, struct strbuf *msg)
imap->caps = imap->rcaps;
if (ret != DRV_OK)
return ret;
- gctx->count++;
return DRV_OK;
}
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 08/14] imap-send.c: remove struct imap argument to parse_imap_list_l()
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
It was always set to NULL.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 39 +++------------------------------------
1 file changed, 3 insertions(+), 36 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index cbbf845..29e4037 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -578,11 +578,10 @@ static void free_list(struct imap_list *list)
}
}
-static int parse_imap_list_l(struct imap *imap, char **sp, struct imap_list **curp, int level)
+static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
{
struct imap_list *cur;
char *s = *sp, *p;
- int n, bytes;
for (;;) {
while (isspace((unsigned char)*s))
@@ -598,39 +597,7 @@ static int parse_imap_list_l(struct imap *imap, char **sp, struct imap_list **cu
/* sublist */
s++;
cur->val = LIST;
- if (parse_imap_list_l(imap, &s, &cur->child, level + 1))
- goto bail;
- } else if (imap && *s == '{') {
- /* literal */
- bytes = cur->len = strtol(s + 1, &s, 10);
- if (*s != '}')
- goto bail;
-
- s = cur->val = xmalloc(cur->len);
-
- /* dump whats left over in the input buffer */
- n = imap->buf.bytes - imap->buf.offset;
-
- if (n > bytes)
- /* the entire message fit in the buffer */
- n = bytes;
-
- memcpy(s, imap->buf.buf + imap->buf.offset, n);
- s += n;
- bytes -= n;
-
- /* mark that we used part of the buffer */
- imap->buf.offset += n;
-
- /* now read the rest of the message */
- while (bytes > 0) {
- if ((n = socket_read(&imap->buf.sock, s, bytes)) <= 0)
- goto bail;
- s += n;
- bytes -= n;
- }
-
- if (buffer_gets(&imap->buf, &s))
+ if (parse_imap_list_l(&s, &cur->child, level + 1))
goto bail;
} else if (*s == '"') {
/* quoted string */
@@ -673,7 +640,7 @@ static struct imap_list *parse_list(char **sp)
{
struct imap_list *head;
- if (!parse_imap_list_l(NULL, sp, &head, 0))
+ if (!parse_imap_list_l(sp, &head, 0))
return head;
free_list(head);
return NULL;
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 09/14] imap-send.c: remove namespace fields from struct imap
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
They are unused, and their removal means that a bunch of list-related
infrastructure can be disposed of.
It might be that the "NAMESPACE" response that is now skipped over in
get_cmd_result() should never be sent by the server. But somebody
would have to check the IMAP protocol and how we interact with the
server to be sure. So for now I am leaving that branch of the "if"
statement there.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 75 ++++++++-----------------------------------------------------
1 file changed, 9 insertions(+), 66 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 29e4037..ff44013 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -99,15 +99,6 @@ static struct imap_server_conf server = {
NULL, /* auth_method */
};
-#define NIL (void *)0x1
-#define LIST (void *)0x2
-
-struct imap_list {
- struct imap_list *next, *child;
- char *val;
- int len;
-};
-
struct imap_socket {
int fd[2];
SSL *ssl;
@@ -124,7 +115,6 @@ struct imap_cmd;
struct imap {
int uidnext; /* from SELECT responses */
- struct imap_list *ns_personal, *ns_other, *ns_shared; /* NAMESPACE info */
unsigned caps, rcaps; /* CAPABILITY results */
/* command queue */
int nexttag, num_in_progress, literal_pending;
@@ -554,34 +544,9 @@ static int imap_exec_m(struct imap_store *ctx, struct imap_cmd_cb *cb,
}
}
-static int is_atom(struct imap_list *list)
-{
- return list && list->val && list->val != NIL && list->val != LIST;
-}
-
-static int is_list(struct imap_list *list)
-{
- return list && list->val == LIST;
-}
-
-static void free_list(struct imap_list *list)
-{
- struct imap_list *tmp;
-
- for (; list; list = tmp) {
- tmp = list->next;
- if (is_list(list))
- free_list(list->child);
- else if (is_atom(list))
- free(list->val);
- free(list);
- }
-}
-
-static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
+static int skip_imap_list_l(char **sp, int level)
{
- struct imap_list *cur;
- char *s = *sp, *p;
+ char *s = *sp;
for (;;) {
while (isspace((unsigned char)*s))
@@ -590,36 +555,23 @@ static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
s++;
break;
}
- *curp = cur = xmalloc(sizeof(*cur));
- curp = &cur->next;
- cur->val = NULL; /* for clean bail */
if (*s == '(') {
/* sublist */
s++;
- cur->val = LIST;
- if (parse_imap_list_l(&s, &cur->child, level + 1))
+ if (skip_imap_list_l(&s, level + 1))
goto bail;
} else if (*s == '"') {
/* quoted string */
s++;
- p = s;
for (; *s != '"'; s++)
if (!*s)
goto bail;
- cur->len = s - p;
s++;
- cur->val = xmemdupz(p, cur->len);
} else {
/* atom */
- p = s;
for (; *s && !isspace((unsigned char)*s); s++)
if (level && *s == ')')
break;
- cur->len = s - p;
- if (cur->len == 3 && !memcmp("NIL", p, 3))
- cur->val = NIL;
- else
- cur->val = xmemdupz(p, cur->len);
}
if (!level)
@@ -628,22 +580,15 @@ static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
goto bail;
}
*sp = s;
- *curp = NULL;
return 0;
bail:
- *curp = NULL;
return -1;
}
-static struct imap_list *parse_list(char **sp)
+static void skip_list(char **sp)
{
- struct imap_list *head;
-
- if (!parse_imap_list_l(sp, &head, 0))
- return head;
- free_list(head);
- return NULL;
+ skip_imap_list_l(sp, 0);
}
static void parse_capability(struct imap *imap, char *cmd)
@@ -722,9 +667,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
}
if (!strcmp("NAMESPACE", arg)) {
- imap->ns_personal = parse_list(&cmd);
- imap->ns_other = parse_list(&cmd);
- imap->ns_shared = parse_list(&cmd);
+ /* rfc2342 NAMESPACE response. */
+ skip_list(&cmd); /* Personal mailboxes */
+ skip_list(&cmd); /* Others' mailboxes */
+ skip_list(&cmd); /* Shared mailboxes */
} else if (!strcmp("OK", arg) || !strcmp("BAD", arg) ||
!strcmp("NO", arg) || !strcmp("BYE", arg)) {
if ((resp = parse_response_code(ctx, NULL, cmd)) != RESP_OK)
@@ -834,9 +780,6 @@ static void imap_close_server(struct imap_store *ictx)
imap_exec(ictx, NULL, "LOGOUT");
socket_shutdown(&imap->buf.sock);
}
- free_list(imap->ns_personal);
- free_list(imap->ns_other);
- free_list(imap->ns_shared);
free(imap);
}
--
1.8.0.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox