* [PATCH v3 15/17] sequencer: lib'ify fast_forward_to()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
The only caller of fast_forward_to(), do_pick_commit() already checks
the return value and passes it on to its callers, so its caller must
be already prepared to handle error returns, and with this step, we
make it notice an error return from this function.
So this is a safe conversion to make fast_forward_to() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 021ddf3..d92a632 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -226,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
read_cache();
if (checkout_fast_forward(from, to, 1))
- exit(128); /* the callee should have complained already */
+ return -1; /* the callee should have complained already */
strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts));
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 13/17] sequencer: lib'ify save_todo()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only caller of save_todo(), pick_commits() can already return
errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.
So this is a safe conversion to make save_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7a1561e..32c53bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -943,24 +943,31 @@ static int sequencer_rollback(struct replay_opts *opts)
return -1;
}
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
{
static struct lock_file todo_lock;
struct strbuf buf = STRBUF_INIT;
int fd;
- fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
- if (format_todo(&buf, todo_list, opts) < 0)
- die(_("Could not format %s."), git_path_todo_file());
+ fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
+ if (fd < 0)
+ return error_errno(_("Could not lock '%s'"),
+ git_path_todo_file());
+ if (format_todo(&buf, todo_list, opts) < 0) {
+ strbuf_release(&buf);
+ return error(_("Could not format %s."), git_path_todo_file());
+ }
if (write_in_full(fd, buf.buf, buf.len) < 0) {
strbuf_release(&buf);
- die_errno(_("Could not write to %s"), git_path_todo_file());
+ return error_errno(_("Could not write to %s"),
+ git_path_todo_file());
}
if (commit_lock_file(&todo_lock) < 0) {
strbuf_release(&buf);
- die(_("Error wrapping up %s."), git_path_todo_file());
+ return error(_("Error wrapping up %s."), git_path_todo_file());
}
strbuf_release(&buf);
+ return 0;
}
static void save_opts(struct replay_opts *opts)
@@ -1009,7 +1016,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
return -1;
for (cur = todo_list; cur; cur = cur->next) {
- save_todo(cur, opts);
+ if (save_todo(cur, opts))
+ return -1;
res = do_pick_commit(cur->item, opts);
if (res)
return res;
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 16/17] lib'ify checkout_fast_forward_to()
From: Johannes Schindelin @ 2016-09-09 14:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
The only callers of checkout_fast_forward_to(), cmd_merge(),
pull_into_void(), cmd_pull() and sequencer's fast_forward_to(),
already check the return value and handle it appropriately. With this
step, we make it notice an error return from this function.
So this is a safe conversion to make checkout_fast_forward_to()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/merge.c b/merge.c
index 5db7d56..23866c9 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,
refresh_cache(REFRESH_QUIET);
- hold_locked_index(lock_file, 1);
+ if (hold_locked_index(lock_file, 0) < 0)
+ return -1;
memset(&trees, 0, sizeof(trees));
memset(&opts, 0, sizeof(opts));
@@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,
}
if (unpack_trees(nr_trees, t, &opts))
return -1;
- if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
- die(_("unable to write new index file"));
+ if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {
+ rollback_lock_file(lock_file);
+ return error(_("unable to write new index file"));
+ }
return 0;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 14/17] sequencer: lib'ify save_opts()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only caller of save_opts(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.
So this is a safe conversion to make save_opts() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 32c53bb..021ddf3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -970,37 +970,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
return 0;
}
-static void save_opts(struct replay_opts *opts)
+static int save_opts(struct replay_opts *opts)
{
const char *opts_file = git_path_opts_file();
+ int res = 0;
if (opts->no_commit)
- git_config_set_in_file(opts_file, "options.no-commit", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.no-commit", "true");
if (opts->edit)
- git_config_set_in_file(opts_file, "options.edit", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.edit", "true");
if (opts->signoff)
- git_config_set_in_file(opts_file, "options.signoff", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true");
if (opts->record_origin)
- git_config_set_in_file(opts_file, "options.record-origin", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true");
if (opts->allow_ff)
- git_config_set_in_file(opts_file, "options.allow-ff", "true");
+ res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true");
if (opts->mainline) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "%d", opts->mainline);
- git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+ res |= git_config_set_in_file_gently(opts_file, "options.mainline", buf.buf);
strbuf_release(&buf);
}
if (opts->strategy)
- git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+ res |= git_config_set_in_file_gently(opts_file, "options.strategy", opts->strategy);
if (opts->gpg_sign)
- git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign);
+ res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign);
if (opts->xopts) {
int i;
for (i = 0; i < opts->xopts_nr; i++)
- git_config_set_multivar_in_file(opts_file,
+ res |= git_config_set_multivar_in_file_gently(opts_file,
"options.strategy-option",
opts->xopts[i], "^$", 0);
}
+ return res;
}
static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
@@ -1147,7 +1149,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return error(_("Can't revert as initial commit"));
if (save_head(sha1_to_hex(sha1)))
return -1;
- save_opts(opts);
+ if (save_opts(opts))
+ return -1;
return pick_commits(todo_list, opts);
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 11/17] sequencer: lib'ify create_seq_dir()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only caller of create_seq_dir(), sequencer_pick_revisions() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an error
return from this function.
So this is a safe conversion to make create_seq_dir() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1614efb..eb9c473 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -847,8 +847,8 @@ static int create_seq_dir(void)
return -1;
}
else if (mkdir(git_path_seq_dir(), 0777) < 0)
- die_errno(_("Could not create sequencer directory %s"),
- git_path_seq_dir());
+ return error_errno(_("Could not create sequencer directory %s"),
+ git_path_seq_dir());
return 0;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 10/17] sequencer: lib'ify read_populate_opts()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only caller of read_populate_opts(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an error
return from this function.
So this is a safe conversion to make read_populate_opts() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.
Note that the function git_config_from_file(), called from
read_populate_opts(), can currently still die() (in git_parse_source(),
because the do_config_from_file() function sets die_on_error = 1). We do
not try to fix that here, as it would have larger ramifications on the
config code, and we also assume that we write the opts file
programmatically, hence any parse errors would be bugs.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index c73cdfd..1614efb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -808,12 +808,20 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
return 0;
}
-static void read_populate_opts(struct replay_opts **opts_ptr)
+static int read_populate_opts(struct replay_opts **opts)
{
if (!file_exists(git_path_opts_file()))
- return;
- if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
- die(_("Malformed options sheet: %s"), git_path_opts_file());
+ return 0;
+ /*
+ * The function git_parse_source(), called from git_config_from_file(),
+ * may die() in case of a syntactically incorrect file. We do not care
+ * about this case, though, because we wrote that file ourselves, so we
+ * are pretty certain that it is syntactically correct.
+ */
+ if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
+ return error(_("Malformed options sheet: %s"),
+ git_path_opts_file());
+ return 0;
}
static int walk_revs_populate_todo(struct commit_list **todo_list,
@@ -1021,8 +1029,8 @@ static int sequencer_continue(struct replay_opts *opts)
if (!file_exists(git_path_todo_file()))
return continue_single_pick();
- read_populate_opts(&opts);
- if (read_populate_todo(&todo_list, opts))
+ if (read_populate_opts(&opts) ||
+ read_populate_todo(&todo_list, opts))
return -1;
/* Verify that the conflict has been resolved */
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 12/17] sequencer: lib'ify save_head()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only caller of save_head(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.
So this is a safe conversion to make save_head() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index eb9c473..7a1561e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -852,18 +852,28 @@ static int create_seq_dir(void)
return 0;
}
-static void save_head(const char *head)
+static int save_head(const char *head)
{
static struct lock_file head_lock;
struct strbuf buf = STRBUF_INIT;
int fd;
- fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);
+ fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
+ if (fd < 0) {
+ rollback_lock_file(&head_lock);
+ return error_errno(_("Could not lock HEAD"));
+ }
strbuf_addf(&buf, "%s\n", head);
- if (write_in_full(fd, buf.buf, buf.len) < 0)
- die_errno(_("Could not write to %s"), git_path_head_file());
- if (commit_lock_file(&head_lock) < 0)
- die(_("Error wrapping up %s."), git_path_head_file());
+ if (write_in_full(fd, buf.buf, buf.len) < 0) {
+ rollback_lock_file(&head_lock);
+ return error_errno(_("Could not write to %s"),
+ git_path_head_file());
+ }
+ if (commit_lock_file(&head_lock) < 0) {
+ rollback_lock_file(&head_lock);
+ return error(_("Error wrapping up %s."), git_path_head_file());
+ }
+ return 0;
}
static int reset_for_rollback(const unsigned char *sha1)
@@ -1127,7 +1137,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
return error(_("Can't revert as initial commit"));
- save_head(sha1_to_hex(sha1));
+ if (save_head(sha1_to_hex(sha1)))
+ return -1;
save_opts(opts);
return pick_commits(todo_list, opts);
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 03/17] sequencer: lib'ify write_message()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
The only caller of write_message(), do_pick_commit() already checks
the return value and passes it on to its callers, so its caller must
be already prepared to handle error returns, and with this step, we
make it notice an error return from this function.
So this is a safe conversion to make write_message() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index baf6b40..ec85fe7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)
}
}
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
{
static struct lock_file msg_file;
- int msg_fd = hold_lock_file_for_update(&msg_file, filename,
- LOCK_DIE_ON_ERROR);
+ int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
+ if (msg_fd < 0)
+ return error_errno(_("Could not lock '%s'"), filename);
if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
- die_errno(_("Could not write to %s"), filename);
+ return error_errno(_("Could not write to %s"), filename);
strbuf_release(msgbuf);
if (commit_lock_file(&msg_file) < 0)
- die(_("Error wrapping up %s."), filename);
+ return error(_("Error wrapping up %s."), filename);
+
+ return 0;
}
static struct tree *empty_tree(void)
@@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
head, &msgbuf, opts);
if (res < 0)
return res;
- write_message(&msgbuf, git_path_merge_msg());
+ res |= write_message(&msgbuf, git_path_merge_msg());
} else {
struct commit_list *common = NULL;
struct commit_list *remotes = NULL;
- write_message(&msgbuf, git_path_merge_msg());
+ res = write_message(&msgbuf, git_path_merge_msg());
commit_list_insert(base, &common);
commit_list_insert(next, &remotes);
- res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+ res |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
common, sha1_to_hex(head), remotes);
free_commit_list(common);
free_commit_list(remotes);
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 08/17] sequencer: lib'ify read_and_refresh_cache()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
There are two call sites of read_and_refresh_cache(), one of which is
pick_commits(), whose callers were already prepared to do the right
thing given an "error" return from it by an earlier patch, so the
conversion is safe.
The other one, sequencer_pick_revisions() was also prepared to relay
an error return back to its caller in all remaining cases in an
earlier patch.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7fd0f99..631b75d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts)
return 0;
}
-static void read_and_refresh_cache(struct replay_opts *opts)
+static int read_and_refresh_cache(struct replay_opts *opts)
{
static struct lock_file index_lock;
int index_fd = hold_locked_index(&index_lock, 0);
if (read_index_preload(&the_index, NULL) < 0)
- die(_("git %s: failed to read the index"), action_name(opts));
+ return error(_("git %s: failed to read the index"),
+ action_name(opts));
refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
if (the_index.cache_changed && index_fd >= 0) {
if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
- die(_("git %s: failed to refresh the index"), action_name(opts));
+ return error(_("git %s: failed to refresh the index"),
+ action_name(opts));
}
rollback_lock_file(&index_lock);
+ return 0;
}
static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
@@ -981,7 +984,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
opts->record_origin || opts->edit));
- read_and_refresh_cache(opts);
+ if (read_and_refresh_cache(opts))
+ return -1;
for (cur = todo_list; cur; cur = cur->next) {
save_todo(cur, opts);
@@ -1045,7 +1049,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
if (opts->subcommand == REPLAY_NONE)
assert(opts->revs);
- read_and_refresh_cache(opts);
+ if (read_and_refresh_cache(opts))
+ return -1;
/*
* Decide what to do depending on the arguments; a fresh
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 09/17] sequencer: lib'ify read_populate_todo()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
The only caller of read_populate_todo(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an
error return from this function.
So this is a safe conversion to make read_populate_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 631b75d..c73cdfd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -748,7 +748,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
return 0;
}
-static void read_populate_todo(struct commit_list **todo_list,
+static int read_populate_todo(struct commit_list **todo_list,
struct replay_opts *opts)
{
struct strbuf buf = STRBUF_INIT;
@@ -756,18 +756,21 @@ static void read_populate_todo(struct commit_list **todo_list,
fd = open(git_path_todo_file(), O_RDONLY);
if (fd < 0)
- die_errno(_("Could not open %s"), git_path_todo_file());
+ return error_errno(_("Could not open %s"),
+ git_path_todo_file());
if (strbuf_read(&buf, fd, 0) < 0) {
close(fd);
strbuf_release(&buf);
- die(_("Could not read %s."), git_path_todo_file());
+ return error(_("Could not read %s."), git_path_todo_file());
}
close(fd);
res = parse_insn_buffer(buf.buf, todo_list, opts);
strbuf_release(&buf);
if (res)
- die(_("Unusable instruction sheet: %s"), git_path_todo_file());
+ return error(_("Unusable instruction sheet: %s"),
+ git_path_todo_file());
+ return 0;
}
static int populate_opts_cb(const char *key, const char *value, void *data)
@@ -1019,7 +1022,8 @@ static int sequencer_continue(struct replay_opts *opts)
if (!file_exists(git_path_todo_file()))
return continue_single_pick();
read_populate_opts(&opts);
- read_populate_todo(&todo_list, opts);
+ if (read_populate_todo(&todo_list, opts))
+ return -1;
/* Verify that the conflict has been resolved */
if (file_exists(git_path_cherry_pick_head()) ||
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 06/17] sequencer: lib'ify walk_revs_populate_todo()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The function sequencer_pick_revisions() is the only caller of
walk_revs_populate_todo(), and it already returns errors
appropriately, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.
So this is a safe conversion to make walk_revs_populate_todo()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 96b9ae1..ab599e0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -809,17 +809,19 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
die(_("Malformed options sheet: %s"), git_path_opts_file());
}
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static int walk_revs_populate_todo(struct commit_list **todo_list,
struct replay_opts *opts)
{
struct commit *commit;
struct commit_list **next;
- prepare_revs(opts);
+ if (prepare_revs(opts))
+ return -1;
next = todo_list;
while ((commit = get_revision(opts->revs)))
next = commit_list_append(commit, next);
+ return 0;
}
static int create_seq_dir(void)
@@ -1102,8 +1104,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
* progress
*/
- walk_revs_populate_todo(&todo_list, opts);
- if (create_seq_dir() < 0)
+ if (walk_revs_populate_todo(&todo_list, opts) ||
+ create_seq_dir() < 0)
return -1;
if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
return error(_("Can't revert as initial commit"));
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 05/17] sequencer: lib'ify do_pick_commit()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The only two callers of do_pick_commit(), pick_commits() and
single_pick() already check the return value and pass it on to their
callers, so their callers must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.
So this is a safe conversion to make do_pick_commit() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.
While at it, remove the superfluous space.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index eb70091..96b9ae1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
* to work on.
*/
if (write_cache_as_tree(head, 0, NULL))
- die (_("Your index file is unmerged."));
+ return error(_("Your index file is unmerged."));
} else {
unborn = get_sha1("HEAD", head);
if (unborn)
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 04/17] sequencer: lib'ify do_recursive_merge()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
The only caller of do_recursive_merge(), do_pick_commit() already
checks the return value and passes it on to its callers, so its caller
must be already prepared to handle error returns, and with this step,
we make it notice an error return from this function.
So this is a safe conversion to make do_recursive_merge() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index ec85fe7..eb70091 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
if (active_cache_changed &&
write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
- die(_("%s: Unable to write new index file"), action_name(opts));
+ return error(_("%s: Unable to write new index file"),
+ action_name(opts));
rollback_lock_file(&index_lock);
if (opts->signoff)
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 02/17] sequencer: do not die() in do_pick_commit()
From: Johannes Schindelin @ 2016-09-09 14:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).
The eventual caller of do_pick_commit() is sequencer_pick_revisions(),
which already relays a reported error from its helper functions
(including this one), and both of its two callers know how to react to
a negative return correctly.
So this makes do_pick_commit() callable from new callers that want it
not to die, without changing the external behaviour of anything
existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 76b1c52..baf6b40 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -585,12 +585,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
* However, if the merge did not even start, then we don't want to
* write it at all.
*/
- if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
- update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
- REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
- if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
- update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
- REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+ if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&
+ update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
+ REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+ res = -1;
+ if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
+ update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
+ REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+ res = -1;
if (res) {
error(opts->action == REPLAY_REVERT
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 01/17] sequencer: lib'ify sequencer_pick_revisions()
From: Johannes Schindelin @ 2016-09-09 14:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1473431645.git.johannes.schindelin@gmx.de>
Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).
The function sequencer_pick_revisions() has only two callers,
cmd_revert() and cmd_cherry_pick(), both of which check the return
value and react appropriately upon errors.
So this is a safe conversion to make sequencer_pick_revisions()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 3804fa9..76b1c52 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1063,10 +1063,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)
if (!get_sha1(name, sha1)) {
if (!lookup_commit_reference_gently(sha1, 1)) {
enum object_type type = sha1_object_info(sha1, NULL);
- die(_("%s: can't cherry-pick a %s"), name, typename(type));
+ return error(_("%s: can't cherry-pick a %s"),
+ name, typename(type));
}
} else
- die(_("%s: bad revision"), name);
+ return error(_("%s: bad revision"), name);
}
/*
@@ -1082,10 +1083,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
!opts->revs->cmdline.rev->flags) {
struct commit *cmit;
if (prepare_revision_walk(opts->revs))
- die(_("revision walk setup failed"));
+ return error(_("revision walk setup failed"));
cmit = get_revision(opts->revs);
if (!cmit || get_revision(opts->revs))
- die("BUG: expected exactly one commit from walk");
+ return error("BUG: expected exactly one commit from walk");
return single_pick(cmit, opts);
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 00/17] Lib'ify quite a few functions in sequencer.c
From: Johannes Schindelin @ 2016-09-09 14:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1472219214.git.johannes.schindelin@gmx.de>
This patch series is one of the half dozen patch series left to move the
bulk of rebase -i into a builtin.
The purpose of this patch series is to switch the functions in
sequencer.c from die()ing to returning errors instead, as proper library
functions should do, to give callers a chance to clean up after an
error.
Changes since v2:
- the commit message of the read_populate_opts() patch now clarifies why
we do not take care of git_config_from_file() possibly die()ing.
- the save_head() and the save_opts() conditionals are now separate, for
improved readability.
- the fast_forward_to() libification now happens in its own commit.
- checkout_fast_forward_to() is now libified, too.
- added a code comment to clarify why we don't care about
git_parse_source() being able to die().
- added rollbacks in case of failure in read_and_refresh_cache() and
save_head().
Johannes Schindelin (17):
sequencer: lib'ify sequencer_pick_revisions()
sequencer: do not die() in do_pick_commit()
sequencer: lib'ify write_message()
sequencer: lib'ify do_recursive_merge()
sequencer: lib'ify do_pick_commit()
sequencer: lib'ify walk_revs_populate_todo()
sequencer: lib'ify prepare_revs()
sequencer: lib'ify read_and_refresh_cache()
sequencer: lib'ify read_populate_todo()
sequencer: lib'ify read_populate_opts()
sequencer: lib'ify create_seq_dir()
sequencer: lib'ify save_head()
sequencer: lib'ify save_todo()
sequencer: lib'ify save_opts()
sequencer: lib'ify fast_forward_to()
lib'ify checkout_fast_forward_to()
sequencer: ensure to release the lock when we could not read the index
merge.c | 9 ++-
sequencer.c | 197 ++++++++++++++++++++++++++++++++++++++----------------------
2 files changed, 131 insertions(+), 75 deletions(-)
Published-As: https://github.com/dscho/git/releases/tag/libify-sequencer-v3
Fetch-It-Via: git fetch https://github.com/dscho/git libify-sequencer-v3
Interdiff vs v2:
diff --git a/merge.c b/merge.c
index 5db7d56..23866c9 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,
refresh_cache(REFRESH_QUIET);
- hold_locked_index(lock_file, 1);
+ if (hold_locked_index(lock_file, 0) < 0)
+ return -1;
memset(&trees, 0, sizeof(trees));
memset(&opts, 0, sizeof(opts));
@@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,
}
if (unpack_trees(nr_trees, t, &opts))
return -1;
- if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
- die(_("unable to write new index file"));
+ if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {
+ rollback_lock_file(lock_file);
+ return error(_("unable to write new index file"));
+ }
return 0;
}
diff --git a/sequencer.c b/sequencer.c
index b6481bb..eec8a60 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -644,14 +644,18 @@ static int read_and_refresh_cache(struct replay_opts *opts)
{
static struct lock_file index_lock;
int index_fd = hold_locked_index(&index_lock, 0);
- if (read_index_preload(&the_index, NULL) < 0)
+ if (read_index_preload(&the_index, NULL) < 0) {
+ rollback_lock_file(&index_lock);
return error(_("git %s: failed to read the index"),
action_name(opts));
+ }
refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
if (the_index.cache_changed && index_fd >= 0) {
- if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
+ if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
+ rollback_lock_file(&index_lock);
return error(_("git %s: failed to refresh the index"),
action_name(opts));
+ }
}
rollback_lock_file(&index_lock);
return 0;
@@ -812,6 +816,12 @@ static int read_populate_opts(struct replay_opts **opts)
{
if (!file_exists(git_path_opts_file()))
return 0;
+ /*
+ * The function git_parse_source(), called from git_config_from_file(),
+ * may die() in case of a syntactically incorrect file. We do not care
+ * about this case, though, because we wrote that file ourselves, so we
+ * are pretty certain that it is syntactically correct.
+ */
if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
return error(_("Malformed options sheet: %s"),
git_path_opts_file());
@@ -853,14 +863,20 @@ static int save_head(const char *head)
int fd;
fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
- if (fd < 0)
+ if (fd < 0) {
+ rollback_lock_file(&head_lock);
return error_errno(_("Could not lock HEAD"));
+ }
strbuf_addf(&buf, "%s\n", head);
- if (write_in_full(fd, buf.buf, buf.len) < 0)
+ if (write_in_full(fd, buf.buf, buf.len) < 0) {
+ rollback_lock_file(&head_lock);
return error_errno(_("Could not write to %s"),
git_path_head_file());
- if (commit_lock_file(&head_lock) < 0)
+ }
+ if (commit_lock_file(&head_lock) < 0) {
+ rollback_lock_file(&head_lock);
return error(_("Error wrapping up %s."), git_path_head_file());
+ }
return 0;
}
@@ -1135,8 +1151,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
return error(_("Can't revert as initial commit"));
- if (save_head(sha1_to_hex(sha1)) ||
- save_opts(opts))
+ if (save_head(sha1_to_hex(sha1)))
+ return -1;
+ if (save_opts(opts))
return -1;
return pick_commits(todo_list, opts);
}
--
2.10.0.windows.1.10.g803177d
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
^ permalink raw reply
* Re: [PATCH 21/22] sequencer: left-trim the lines read from the script
From: Johannes Schindelin @ 2016-09-09 14:31 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, Junio C Hamano
In-Reply-To: <54cc3d11-6384-b666-3103-4e56a399ba6d@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 513 bytes --]
Hi Kuba,
On Fri, 2 Sep 2016, Jakub Narębski wrote:
> Hello Johannes,
>
> W dniu 01.09.2016 o 16:13, Johannes Schindelin pisze:
> > On Thu, 1 Sep 2016, Jakub Narębski wrote:
>
> >> 'bol' is beginning-of-line, isn't it (a complement to eol)?
> >
> > Yep. How did you guess? :-)
>
> Wouldn't 'beg' and 'end' instead of 'bol' and 'eol' be easier
> to understand, thus more readable?
It is just consistency with the code I inherited: sequencer.c used 'bol'
and 'eol' before.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
From: Johannes Schindelin @ 2016-09-09 14:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narębski, git
In-Reply-To: <xmqqvayftefc.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 823 bytes --]
Hi Junio,
On Thu, 1 Sep 2016, Junio C Hamano wrote:
> Jakub Narębski <jnareb@gmail.com> writes:
>
> > I wonder how probable is situation where we save instruction sheet
> > for interactive rebase, with shortened SHA-1, and during rebase
> > shortened SHA-1 stops being unambiguous...
>
> It is my understanding that the shortened ones are only for end-user
> consumption. The insn sheet internally uses fully expanded form for
> this exact reason, and then abbreviated back at each step before the
> updated one is presented to the end-user. Uniqueness guarantee is
> enforced with new objects created during each step taken into
> account by doing it this way.
Indeed, the rebase -i shortens the SHA-1s just before letting the user
edit git-rebase-todo and then expands them back.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 10/22] sequencer: avoid completely different messages for different actions
From: Johannes Schindelin @ 2016-09-09 14:23 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, Junio C Hamano, Jiang Xin
In-Reply-To: <198780d4-dbef-c0cc-fb4c-fc8986a33002@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]
Hi Kuba,
On Fri, 2 Sep 2016, Jakub Narębski wrote:
> W dniu 01.09.2016 o 09:52, Johannes Schindelin pisze:
> > On Wed, 31 Aug 2016, Jakub Narębski wrote:
> >> CC-ed to Jiang Xin, L10N coordinator.
> >> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
>
> [...]
> >>> - /* Different translation strings for cherry-pick and revert */
> >>> - if (opts->action == REPLAY_PICK)
> >>> - error(_("Your local changes would be overwritten by cherry-pick."));
> >>> - else
> >>> - error(_("Your local changes would be overwritten by revert."));
> >>> + error(_("Your local changes would be overwritten by %s."),
> >>> + action_name(opts));
> >>
> >> If I understand it correctly, it would make "revert" or "cherry-pick"
> >> untranslated part of error message. You would need to use translation
> >> on the result with "_(action_name(opts))", you would have to mark
> >> todo_command_strings elements for gettext lexicon with N_(...).
Okay, that is easy enough.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-09-09 14:12 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, Junio C Hamano
In-Reply-To: <d8112b27-8de3-0860-e902-fb271b80a0e7@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3209 bytes --]
Hi Kuba,
On Fri, 2 Sep 2016, Jakub Narębski wrote:
> W dniu 01.09.2016 o 09:49, Johannes Schindelin pisze:
> > On Wed, 31 Aug 2016, Jakub Narębski wrote:
>
> >> Here todo_list uses growable array implementation of list. Which is
> >> I guess better on current CPU architecture, with slow memory,
> >> limited-size caches, and adjacency prefetching.
> >
> > That is not the reason that an array is used here. The array allows us
> > much more flexibility.
>
> It would be nice if this reasoning (behind the change from linked list
> to growable array) was mentioned in appropriate commit message, and
> perhaps also in the cover letter for the series. It is IMVHO quite
> important information (that you thought obvious).
Amended.
> >>> +struct todo_item *append_todo(struct todo_list *todo_list)
> >>
> >> Errr... I don't quite understand the name of this function.
> >> What are you appending here to the todo_list?
> >
> > A new item.
> >
> >> Compare string_list_append() and string_list_append_nodup(),
> >> where the second parameter is item to append.
> >
> > Yes, that is correct. In the case of a todo_item, things are a lot more
> > complicated, though. Some of the values have to be determined tediously
> > (such as the offset and length of the oneline after the "pick <oid>"
> > command). I just put those values directly into the newly allocated item,
> > is all.
>
> I would expect sth_append command to take a list (or other collection),
> an element, and return [modified] collection with the new element added.
> Such API would require temporary variable in caller and memcopy in the
> sth_append() function.
>
> This is not it. It creates a new element, expanding a list (a collection),
> and then expose this element. Which spares us memcopy... on non-critical
> path.
>
> I don't know how to name operation "grow list and return new element".
> But "append" it is not.
I renamed it to append_new_todo().
> >>> - end_of_object_name = bol + strcspn(bol, " \t\n");
> >>> + end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> >>
> >> Why is this cast needed?
> >
> > Because bol is a "const char *" and we need to put "NUL" temporarily to
> > *end_of_object_name:
>
> Would compiler complain without this const'ness-stripping cast?
Yes. I would not have added it otherwise.
Please note that this is only necessary because I changed the parameter
from "char *" to "const char *" (which was The Right Thing To Do).
> >>> saved = *end_of_object_name;
> >>> *end_of_object_name = '\0';
> >>> status = get_sha1(bol, commit_sha1);
> >>> *end_of_object_name = saved;
> >
> > Technically, this would have made a fine excuse to teach get_sha1() a
> > mode where it expects a length parameter instead of relying on a
> > NUL-terminated string.
> >
> > Practically, such fine excuses cost me months in this rebase--helper
> > project already, and I need to protect my time better.
>
> Put it in TODO list (and perhaps add a TODO comment) ;-).
I am also a realist: I won't be able to do anything about this. If you
care enough, please go right to town.
Thanks again,
Dscho
^ permalink raw reply
* RE: [PATCH] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-09 13:29 UTC (permalink / raw)
To: 'Jeff King', 'Junio C Hamano'
Cc: git, pclouds, peartben, 'Ben Peart'
In-Reply-To: <20160908213738.zgwgfy3nybkam3hk@sigill.intra.peff.net>
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Thursday, September 8, 2016 5:38 PM
> To: Junio C Hamano <gitster@pobox.com>
> Cc: Ben Peart <peartben@gmail.com>; git@vger.kernel.org;
> pclouds@gmail.com; =peartben@gmail.com; Ben Peart
> <benpeart@microsoft.com>
> Subject: Re: [PATCH] checkout: eliminate unnecessary merge for trivial
> checkout
>
> On Thu, Sep 08, 2016 at 02:22:16PM -0700, Junio C Hamano wrote:
>
> > > + /*
> > > + * Optimize the performance of checkout when the current and
> > > + * new branch have the same OID and avoid the trivial merge.
> > > + * For example, a "git checkout -b foo" just needs to create
> > > + * the new ref and report the stats.
> > > + */
> > > + if (!old.commit || !new->commit
> > > + || oidcmp(&old.commit->object.oid, &new->commit-
> >object.oid)
> > > + || !opts->new_branch || opts->new_branch_force || opts-
> >new_orphan_branch
> > > + || opts->patch_mode || opts->merge || opts->force || opts-
> >force_detach
> > > + || opts->writeout_stage || !opts->overwrite_ignore
> > > + || opts->ignore_skipworktree || opts-
> >ignore_other_worktrees
> > > + || opts->new_branch_log || opts->branch_exists || opts-
> >prefix
> > > + || opts->source_tree) {
> >
> > ... this is a maintenance nightmare in that any new option we will add
> > later will need to consider what this "optimization" is trying
> > (not) to skip. The first two lines (i.e. we need a real checkout if
> > we cannot positively say that old and new commits are the same
> > object) are clear, but no explanation was given for all the other
> > random conditions this if condition checks. What if opts->something
> > was not listed (or "listed" for that matter) in the list above--it is
> > totally unclear if it was missed by mistake (or "added by
> > mistake") or deliberately excluded (or "deliberately added").
> >
> > For example, why is opts->prefix there? If
> >
> > git checkout -b new-branch HEAD
> >
> > should be able to omit the two-way merge, shouldn't
> >
> > cd t && git checkout -b new-branch HEAD
> >
> > also be able to?
Because this induces a behavior change (the optimized path will no
longer do a "soft reset" and regenerate the index for example) I was
attempting to make it as restrictive as possible but still enable the
fast path in the most common case. If everyone is OK with the behavior
change, I can make the optimization more inclusive by removing those
tests that are not absolutely required (like opts->prefix).
To help ensure the optimization is updated when new checkout options are
added I could add a comment into the checkout_opts structure and/or put
a pseudo version check into the code so if the size of the structure
changes, the fast path fails. That feels a little hacky and I haven't
seen that in other areas so I'd rather stick with splitting it out into
a helper function and add comments.
>
> I was just writing another reply, but I think our complaints may have
> dovetailed.
>
> My issue is that the condition above is an unreadable mass. It would be
> really nice to pull it out into a helper function, and then all of the items could
> be split out and commented independently, like:
>
> static int needs_working_tree_merge(const struct checkout_opts *opts,
> const struct branch_info *old,
> const struct branch_info *new)
> {
> /*
> * We must do the merge if we are actually moving to a new
> * commit.
> */
> if (!old->commit || !new->commit ||
> oidcmp(&old.commit->object.oid, &new->commit->object.oid))
> return 1;
>
> /* Option "foo" is not compatible because of... */
> if (opts->foo)
> return 1;
>
> ... etc ...
> }
That is a great suggestion. Splitting this out into a helper function
with comments will definitely make this more readable/maintainable and
provide more information on why each test is there. I'll do that and
reroll the patch.
>
> That still leaves your "what if opts->something is not listed" question open,
> but at least it makes it easier to comment on it in the code.
>
> -Peff
>
> PS I didn't think hard on whether the conditions above make _sense_. My
> first goal would be to get more communication about them individually,
> and then we can evaluate them.
^ permalink raw reply
* Re: Announcing Git User's Survey 2016 [was: Working with public-inbox.org]
From: Johannes Schindelin @ 2016-09-09 13:06 UTC (permalink / raw)
To: Jakub Narębski
Cc: Duy Nguyen, Jeff King, Stefan Beller, meta, git@vger.kernel.org,
Eric Wong
In-Reply-To: <bb1ee52f-c4b6-ded3-19af-ddb7b65b376d@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]
Hi Kuba,
On Sun, 28 Aug 2016, Jakub Narębski wrote:
> W dniu 28.08.2016 o 10:38, Johannes Schindelin pisze:
>
> > I would like to strongly caution against putting too much stock into
> > this users' survey. It is the best we have, granted. Yet I have not
> > heard from anybody that they participated in the survey, unless they
> > were also subscribed to the Git mailing list.
>
> I tried in past and will try for this year Git User's Survey to be
> announced more widely than just Git mailing list (git@vger.kernel.org).
I did not mean to criticise you. I think you are doing the best you can,
and it is valuable.
My point is: many professional developers use Git not necessarily because
they want to, but because their day job requires them to do so.
I *highly* doubt that we reach a notable fraction of those developers,
even if they are arguably power users of any development tool, including
Git.
The question is not so much how to advertise the survey. I skip almost all
surveys I am asked to participate in, because I am just a little bit busy
all the time. I feel that my colleagues do the same. Unless forced to take
a survey, they skip it.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result
From: Johannes Schindelin @ 2016-09-09 12:58 UTC (permalink / raw)
To: Jeff King
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye, Josh Triplett
In-Reply-To: <20160909104007.pwki2ir6et3vvk55@sigill.intra.peff.net>
Hi Peff,
On Fri, 9 Sep 2016, Jeff King wrote:
> On Fri, Sep 09, 2016 at 12:28:38PM +0200, Johannes Schindelin wrote:
>
> > I like the simplification, but I *hate* the fact that the calling code has
> > *no way* to inform the user about the proper next steps.
> >
> > You are touching code that is really quite at the bottom of a lot of call
> > chains. For example in the one of `git pull --rebase`. I just spent an
> > insane amount of time trying to make sure that this command will not
> > simply die() somewhere deep in the code, leaving the user puzzled.
> >
> > Please see 3be18b4 (t5520: verify that `pull --rebase` shows the helpful
> > advice when failing, 2016-07-26) for more details.
>
> Yes, I agree that this is the opposite direction of libification. And I
> agree that the current message is not very helpful.
>
> But I am not sure that returning the error up the stack will actually
> help somebody move forward. The reason these are all die() calls in the
> rest of the diff code is that they are generally indicative of
> unrecoverable repository corruption. So any advice does not really
> depend on what operation you are performing; it is always "stop what you
> are doing immediately, run fsck, and try to get the broken objects from
> somebody else".
>
> So IMHO, on balance this is not hurting anything.
Well, you make such a situation even worse than it already is.
It would be one thing to change the code to actually say "stop what you
are doing immediately, run `git fsck` and try to get the broken objects
from somewhere else", *before* saying how to proceed after that.
But that is not what your patch does.
What your patch does is to remove *even the possibility* of saying how to
proceed after getting the repository corruption fixed. And instead of
saying how the corruption could be fixed, it outputs a terse "cannot read
files to diff".
I do not think that is a wise direction.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] git-gui: respect commit.gpgsign again
From: Johannes Schindelin @ 2016-09-09 12:28 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pat Thoyts
As of v2.9.0, `git commit-tree` no longer heeds the `commit.gpgsign`
config setting. This broke committing in Git GUI.
This fixes https://github.com/git-for-windows/git/issues/850
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/git-gui-gpgsign-v1
Fetch-It-Via: git fetch https://github.com/dscho/git git-gui-gpgsign-v1
git-gui/lib/commit.tcl | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 864b687..01d2cc2 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -369,6 +369,9 @@ A rescan will be automatically started now.
# -- Create the commit.
#
set cmd [list commit-tree $tree_id]
+ if {[is_config_true commit.gpgsign]} {
+ lappend cmd -S
+ }
foreach p [concat $PARENT $MERGE_HEAD] {
lappend cmd -p $p
}
--
2.10.0.windows.1.10.g803177d
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
^ permalink raw reply related
* Re: [PATCH v2] rebase -i: improve advice on bad instruction lines
From: Johannes Schindelin @ 2016-09-09 12:21 UTC (permalink / raw)
To: Ralf Thielow; +Cc: Dennis Kaarsemaker, git, Junio C Hamano
In-Reply-To: <CAN0XMO+diDLXUrxjNGnUi6oUEwkcGavEX8vyGWy2+Wju-N4WJQ@mail.gmail.com>
Hi Ralf,
On Wed, 7 Sep 2016, Ralf Thielow wrote:
> 2016-09-07 11:28 GMT+02:00 Dennis Kaarsemaker <dennis@kaarsemaker.net>:
> > Hi Ralf,
> >
> > There are quite a few patch series in flight these days around
> > interactive rebase. Have you checked for conflicts with those?
> >
>
> Thanks. I did not check against 'pu' when I created this patch but I'm able
> to apply without any conflicts.
Yeah, rewrites in C won't conflict with your patches. If you rebase
interactively, the worst that will happen is that you update code that was
moved into contrib/...
Ciao,
Johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox