* [PATCH v2 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
From: Johannes Schindelin @ 2016-12-13 15:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
This is a huge patch, and at the same time a huge step forward to
execute the performance-critical parts of the interactive rebase in a
builtin command.
Since 'fixup' and 'squash' are not only similar, but also need to know
about each other (we want to reduce a series of fixups/squashes into a
single, final commit message edit, from the user's point of view), we
really have to implement them both at the same time.
Most of the actual work is done by the existing code path that already
handles the "pick" and the "edit" commands; We added support for other
features (e.g. to amend the commit message) in the patches leading up to
this one, yet there are still quite a few bits in this patch that simply
would not make sense as individual patches (such as: determining whether
there was anything to "fix up" in the "todo" script, etc).
In theory, it would be possible to reuse the fast-forward code path also
for the fixup and the squash code paths, but in practice this would make
the code less readable. The end result cannot be fast-forwarded anyway,
therefore let's just extend the cherry-picking code path for now.
Since the sequencer parses the entire `git-rebase-todo` script in one go,
fixup or squash commands without a preceding pick can be reported early
(in git-rebase--interactive, we could only report such errors just before
executing the fixup/squash).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 232 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 222 insertions(+), 10 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index f6e20b142a..271c21581d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -45,6 +45,35 @@ static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
*/
static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
/*
+ * The commit message that is planned to be used for any changes that
+ * need to be committed following a user interaction.
+ */
+static GIT_PATH_FUNC(rebase_path_message, "rebase-merge/message")
+/*
+ * The file into which is accumulated the suggested commit message for
+ * squash/fixup commands. When the first of a series of squash/fixups
+ * is seen, the file is created and the commit message from the
+ * previous commit and from the first squash/fixup commit are written
+ * to it. The commit message for each subsequent squash/fixup commit
+ * is appended to the file as it is processed.
+ *
+ * The first line of the file is of the form
+ * # This is a combination of $count commits.
+ * where $count is the number of commits whose messages have been
+ * written to the file so far (including the initial "pick" commit).
+ * Each time that a commit message is processed, this line is read and
+ * updated. It is deleted just before the combined commit is made.
+ */
+static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash")
+/*
+ * If the current series of squash/fixups has not yet included a squash
+ * command, then this file exists and holds the commit message of the
+ * original "pick" commit. (If the series ends without a "squash"
+ * command, then this can be used as the commit message of the combined
+ * commit without opening the editor.)
+ */
+static GIT_PATH_FUNC(rebase_path_fixup_msg, "rebase-merge/message-fixup")
+/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
* GIT_AUTHOR_DATE that will be used for the commit that is currently
* being rebased.
@@ -673,6 +702,8 @@ enum todo_command {
TODO_PICK = 0,
TODO_REVERT,
TODO_EDIT,
+ TODO_FIXUP,
+ TODO_SQUASH,
/* commands that do something else than handling a single commit */
TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
@@ -683,6 +714,8 @@ static const char *todo_command_strings[] = {
"pick",
"revert",
"edit",
+ "fixup",
+ "squash",
"exec",
"noop"
};
@@ -694,16 +727,119 @@ static const char *command_to_string(const enum todo_command command)
die("Unknown command: %d", command);
}
+static int is_fixup(enum todo_command command)
+{
+ return command == TODO_FIXUP || command == TODO_SQUASH;
+}
+
+static int update_squash_messages(enum todo_command command,
+ struct commit *commit, struct replay_opts *opts)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int count, res;
+ const char *message, *body;
+
+ if (file_exists(rebase_path_squash_msg())) {
+ char *p, *p2;
+
+ if (strbuf_read_file(&buf, rebase_path_squash_msg(), 2048) <= 0)
+ return error(_("could not read '%s'"),
+ rebase_path_squash_msg());
+
+ if (buf.buf[0] != comment_line_char ||
+ !skip_prefix(buf.buf + 1, " This is a combination of ",
+ (const char **)&p))
+ return error(_("unexpected 1st line of squash message:"
+ "\n\n\t%.*s"),
+ (int)(strchrnul(buf.buf, '\n') - buf.buf),
+ buf.buf);
+ count = strtol(p, &p2, 10);
+
+ if (count < 1 || *p2 != ' ')
+ return error(_("invalid 1st line of squash message:\n"
+ "\n\t%.*s"),
+ (int)(strchrnul(buf.buf, '\n') - buf.buf),
+ buf.buf);
+
+ sprintf((char *)p, "%d", ++count);
+ if (!*p2)
+ *p2 = ' ';
+ else {
+ *(++p2) = 'c';
+ strbuf_insert(&buf, p2 - buf.buf, " ", 1);
+ }
+ }
+ else {
+ unsigned char head[20];
+ struct commit *head_commit;
+ const char *head_message, *body;
+
+ if (get_sha1("HEAD", head))
+ return error(_("need a HEAD to fixup"));
+ if (!(head_commit = lookup_commit_reference(head)))
+ return error(_("could not read HEAD"));
+ if (!(head_message = get_commit_buffer(head_commit, NULL)))
+ return error(_("could not read HEAD's commit message"));
+
+ body = strstr(head_message, "\n\n");
+ if (!body)
+ body = "";
+ else
+ body = skip_blank_lines(body + 2);
+ if (write_message(body, strlen(body),
+ rebase_path_fixup_msg(), 0))
+ return error(_("cannot write '%s'"),
+ rebase_path_fixup_msg());
+
+ count = 2;
+ strbuf_addf(&buf, _("%c This is a combination of 2 commits.\n"
+ "%c The first commit's message is:\n\n%s"),
+ comment_line_char, comment_line_char, body);
+
+ unuse_commit_buffer(head_commit, head_message);
+ }
+
+ if (!(message = get_commit_buffer(commit, NULL)))
+ return error(_("could not read commit message of %s"),
+ oid_to_hex(&commit->object.oid));
+ body = strstr(message, "\n\n");
+ if (!body)
+ body = "";
+ else
+ body = skip_blank_lines(body + 2);
+
+ if (command == TODO_SQUASH) {
+ unlink(rebase_path_fixup_msg());
+ strbuf_addf(&buf, _("\n%c This is the commit message #%d:\n"
+ "\n%s"),
+ comment_line_char, count, body);
+ }
+ else if (command == TODO_FIXUP) {
+ strbuf_addf(&buf, _("\n%c The commit message #%d "
+ "will be skipped:\n\n"),
+ comment_line_char, count);
+ strbuf_add_commented_lines(&buf, body, strlen(body));
+ }
+ else
+ return error(_("unknown command: %d"), command);
+ unuse_commit_buffer(commit, message);
+
+ res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0);
+ strbuf_release(&buf);
+ return res;
+}
static int do_pick_commit(enum todo_command command, struct commit *commit,
- struct replay_opts *opts)
+ struct replay_opts *opts, int final_fixup)
{
+ int edit = opts->edit, cleanup_commit_message = 0;
+ const char *msg_file = edit ? NULL : git_path_merge_msg();
unsigned char head[20];
struct commit *base, *next, *parent;
const char *base_label, *next_label;
struct commit_message msg = { NULL, NULL, NULL, NULL };
struct strbuf msgbuf = STRBUF_INIT;
- int res, unborn = 0, allow;
+ int res, unborn = 0, amend = 0, allow;
if (opts->no_commit) {
/*
@@ -749,7 +885,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
else
parent = commit->parents->item;
- if (opts->allow_ff &&
+ if (opts->allow_ff && !is_fixup(command) &&
((parent && !hashcmp(parent->object.oid.hash, head)) ||
(!parent && unborn)))
return fast_forward_to(commit->object.oid.hash, head, unborn, opts);
@@ -813,6 +949,28 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
}
}
+ if (is_fixup(command)) {
+ if (update_squash_messages(command, commit, opts))
+ return -1;
+ amend = 1;
+ if (!final_fixup)
+ msg_file = rebase_path_squash_msg();
+ else if (file_exists(rebase_path_fixup_msg())) {
+ cleanup_commit_message = 1;
+ msg_file = rebase_path_fixup_msg();
+ }
+ else {
+ const char *dest = git_path("SQUASH_MSG");
+ unlink(dest);
+ if (copy_file(dest, rebase_path_squash_msg(), 0666))
+ return error(_("could not rename '%s' to '%s'"),
+ rebase_path_squash_msg(), dest);
+ unlink(git_path("MERGE_MSG"));
+ msg_file = dest;
+ edit = 1;
+ }
+ }
+
if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
head, &msgbuf, opts);
@@ -868,8 +1026,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
goto leave;
}
if (!opts->no_commit)
- res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
- opts, allow, opts->edit, 0, 0);
+ res = run_git_commit(msg_file, opts, allow, edit, amend,
+ cleanup_commit_message);
+
+ if (!res && final_fixup) {
+ unlink(rebase_path_fixup_msg());
+ unlink(rebase_path_squash_msg());
+ }
leave:
free_message(commit, &msg);
@@ -1009,7 +1172,7 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
{
struct todo_item *item;
char *p = buf, *next_p;
- int i, res = 0;
+ int i, res = 0, fixup_okay = file_exists(rebase_path_done());
for (i = 1; *p; i++, p = next_p) {
char *eol = strchrnul(p, '\n');
@@ -1024,8 +1187,16 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
if (parse_insn_line(item, p, eol)) {
res = error(_("invalid line %d: %.*s"),
i, (int)(eol - p), p);
- item->command = -1;
+ item->command = TODO_NOOP;
}
+
+ if (fixup_okay)
+ ; /* do nothing */
+ else if (is_fixup(item->command))
+ return error(_("cannot '%s' without a previous commit"),
+ command_to_string(item->command));
+ else if (item->command != TODO_NOOP)
+ fixup_okay = 1;
}
if (!todo_list->nr)
return error(_("no commits parsed."));
@@ -1434,6 +1605,20 @@ static int error_with_patch(struct commit *commit,
return exit_code;
}
+static int error_failed_squash(struct commit *commit,
+ struct replay_opts *opts, int subject_len, const char *subject)
+{
+ if (rename(rebase_path_squash_msg(), rebase_path_message()))
+ return error(_("could not rename '%s' to '%s'"),
+ rebase_path_squash_msg(), rebase_path_message());
+ unlink(rebase_path_fixup_msg());
+ unlink(git_path("MERGE_MSG"));
+ if (copy_file(git_path("MERGE_MSG"), rebase_path_message(), 0666))
+ return error(_("could not copy '%s' to '%s'"),
+ rebase_path_message(), git_path("MERGE_MSG"));
+ return error_with_patch(commit, subject, subject_len, opts, 1, 0);
+}
+
static int do_exec(const char *command_line)
{
const char *child_argv[] = { NULL, NULL };
@@ -1475,6 +1660,21 @@ static int do_exec(const char *command_line)
return status;
}
+static int is_final_fixup(struct todo_list *todo_list)
+{
+ int i = todo_list->current;
+
+ if (!is_fixup(todo_list->items[i].command))
+ return 0;
+
+ while (++i < todo_list->nr)
+ if (is_fixup(todo_list->items[i].command))
+ return 0;
+ else if (todo_list->items[i].command < TODO_NOOP)
+ break;
+ return 1;
+}
+
static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
{
int res = 0;
@@ -1490,9 +1690,15 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
struct todo_item *item = todo_list->items + todo_list->current;
if (save_todo(todo_list, opts))
return -1;
- if (item->command <= TODO_EDIT) {
+ if (is_rebase_i(opts)) {
+ unlink(rebase_path_message());
+ unlink(rebase_path_author_script());
+ unlink(rebase_path_stopped_sha());
+ unlink(rebase_path_amend());
+ }
+ if (item->command <= TODO_SQUASH) {
res = do_pick_commit(item->command, item->commit,
- opts);
+ opts, is_final_fixup(todo_list));
if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
if (!res)
@@ -1503,6 +1709,12 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
item->arg, item->arg_len, opts, res,
!res);
}
+ if (res && is_fixup(item->command)) {
+ if (res == 1)
+ intend_to_amend();
+ return error_failed_squash(item->commit, opts,
+ item->arg_len, item->arg);
+ }
}
else if (item->command == TODO_EXEC) {
char *end_of_arg = (char *)(item->arg + item->arg_len);
@@ -1597,7 +1809,7 @@ static int single_pick(struct commit *cmit, struct replay_opts *opts)
{
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
return do_pick_commit(opts->action == REPLAY_PICK ?
- TODO_PICK : TODO_REVERT, cmit, opts);
+ TODO_PICK : TODO_REVERT, cmit, opts, 0);
}
int sequencer_pick_revisions(struct replay_opts *opts)
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode
From: Johannes Schindelin @ 2016-12-13 15:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
When calling `git rebase -i -v`, the user wants to see some statistics
after the commits were rebased. Let's show some.
The strbuf we use to perform that task will be used for other things
in subsequent commits, hence it is declared and initialized in a wider
scope than strictly needed here.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 22 ++++++++++++++++++++++
sequencer.h | 1 +
2 files changed, 23 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 700b7575ed..1ab50884bd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -63,6 +63,8 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
* command-line (and are only consumed, not modified, by the sequencer).
*/
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
+static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
+static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
static inline int is_rebase_i(const struct replay_opts *opts)
{
@@ -1121,6 +1123,9 @@ static int read_populate_opts(struct replay_opts *opts)
}
strbuf_release(&buf);
+ if (file_exists(rebase_path_verbose()))
+ opts->verbose = 1;
+
return 0;
}
@@ -1493,9 +1498,26 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
}
if (is_rebase_i(opts)) {
+ struct strbuf buf = STRBUF_INIT;
+
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
return 0;
+
+ if (opts->verbose) {
+ const char *argv[] = {
+ "diff-tree", "--stat", NULL, NULL
+ };
+
+ if (!read_oneliner(&buf, rebase_path_orig_head(), 0))
+ return error(_("could not read '%s'"),
+ rebase_path_orig_head());
+ strbuf_addstr(&buf, "..HEAD");
+ argv[2] = buf.buf;
+ run_command_v_opt(argv, RUN_GIT_CMD);
+ strbuf_reset(&buf);
+ }
+ strbuf_release(&buf);
}
/*
diff --git a/sequencer.h b/sequencer.h
index cb21cfddee..f885b68395 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -24,6 +24,7 @@ struct replay_opts {
int allow_empty;
int allow_empty_message;
int keep_redundant_commits;
+ int verbose;
int mainline;
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 06/34] sequencer (rebase -i): write the 'done' file
From: Johannes Schindelin @ 2016-12-13 15:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
In the interactive rebase, commands that were successfully processed are
not simply discarded, but appended to the 'done' file instead. This is
used e.g. to display the current state to the user in the output of
`git status` or the progress.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 1ab50884bd..f6e20b142a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -39,6 +39,12 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
*/
static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
/*
+ * The rebase command lines that have already been processed. A line
+ * is moved here when it is first handled, before any associated user
+ * actions.
+ */
+static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
+/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
* GIT_AUTHOR_DATE that will be used for the commit that is currently
* being rebased.
@@ -1295,6 +1301,23 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
return error_errno(_("could not write to '%s'"), todo_path);
if (commit_lock_file(&todo_lock) < 0)
return error(_("failed to finalize '%s'."), todo_path);
+
+ if (is_rebase_i(opts)) {
+ const char *done_path = rebase_path_done();
+ int fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
+ int prev_offset = !next ? 0 :
+ todo_list->items[next - 1].offset_in_buf;
+
+ if (fd >= 0 && offset > prev_offset &&
+ write_in_full(fd, todo_list->buf.buf + prev_offset,
+ offset - prev_offset) < 0) {
+ close(fd);
+ return error_errno(_("could not write to '%s'"),
+ done_path);
+ }
+ if (fd >= 0)
+ close(fd);
+ }
return 0;
}
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 27/34] sequencer (rebase -i): differentiate between comments and 'noop'
From: Johannes Schindelin @ 2016-12-13 15:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
In the upcoming patch, we will support rebase -i's progress
reporting. The progress skips comments but counts 'noop's.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1f314b2743..63f6f25ced 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -770,7 +770,9 @@ enum todo_command {
TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP,
- TODO_DROP
+ TODO_DROP,
+ /* comments (not counted for reporting progress) */
+ TODO_COMMENT
};
static struct {
@@ -785,12 +787,13 @@ static struct {
{ 's', "squash" },
{ 'x', "exec" },
{ 0, "noop" },
- { 'd', "drop" }
+ { 'd', "drop" },
+ { 0, NULL }
};
static const char *command_to_string(const enum todo_command command)
{
- if ((size_t)command < ARRAY_SIZE(todo_command_info))
+ if (command < TODO_COMMENT)
return todo_command_info[command].str;
die("Unknown command: %d", command);
}
@@ -1237,14 +1240,14 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
bol += strspn(bol, " \t");
if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
- item->command = TODO_NOOP;
+ item->command = TODO_COMMENT;
item->commit = NULL;
item->arg = bol;
item->arg_len = eol - bol;
return 0;
}
- for (i = 0; i < ARRAY_SIZE(todo_command_info); i++)
+ for (i = 0; i < TODO_COMMENT; i++)
if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
item->command = i;
break;
@@ -1254,7 +1257,7 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
item->command = i;
break;
}
- if (i >= ARRAY_SIZE(todo_command_info))
+ if (i >= TODO_COMMENT)
return -1;
if (item->command == TODO_NOOP) {
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 14/34] sequencer (rebase -i): update refs after a successful rebase
From: Johannes Schindelin @ 2016-12-13 15:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
An interactive rebase operates on a detached HEAD (to keep the reflog
of the original branch relatively clean), and updates the branch only
at the end.
Now that the sequencer learns to perform interactive rebases, it also
needs to learn the trick to update the branch before removing the
directory containing the state of the interactive rebase.
We introduce a new head_ref variable in a wider scope than necessary at
the moment, to allow for a later patch that prints out "Successfully
rebased and updated <ref>".
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index a6625e765d..a4e9b326ba 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -100,6 +100,8 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
+static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
+static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
static inline int is_rebase_i(const struct replay_opts *opts)
{
@@ -1793,12 +1795,39 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
}
if (is_rebase_i(opts)) {
- struct strbuf buf = STRBUF_INIT;
+ struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
return 0;
+ if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
+ starts_with(head_ref.buf, "refs/")) {
+ unsigned char head[20], orig[20];
+
+ if (get_sha1("HEAD", head))
+ return error(_("cannot read HEAD"));
+ if (!read_oneliner(&buf, rebase_path_orig_head(), 0) ||
+ get_sha1_hex(buf.buf, orig))
+ return error(_("could not read orig-head"));
+ strbuf_addf(&buf, "rebase -i (finish): %s onto ",
+ head_ref.buf);
+ if (!read_oneliner(&buf, rebase_path_onto(), 0))
+ return error(_("could not read 'onto'"));
+ if (update_ref(buf.buf, head_ref.buf, head, orig,
+ REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+ return error(_("could not update %s"),
+ head_ref.buf);
+ strbuf_reset(&buf);
+ strbuf_addf(&buf,
+ "rebase -i (finish): returning to %s",
+ head_ref.buf);
+ if (create_symref("HEAD", head_ref.buf, buf.buf))
+ return error(_("could not update HEAD to %s"),
+ head_ref.buf);
+ strbuf_reset(&buf);
+ }
+
if (opts->verbose) {
const char *argv[] = {
"diff-tree", "--stat", NULL, NULL
@@ -1813,6 +1842,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
strbuf_reset(&buf);
}
strbuf_release(&buf);
+ strbuf_release(&head_ref);
}
/*
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 12/34] sequencer (rebase -i): skip some revert/cherry-pick specific code path
From: Johannes Schindelin @ 2016-12-13 15:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
When a cherry-pick continues without a "todo script", the intention is
simply to pick a single commit.
However, when an interactive rebase is continued without a "todo
script", it means that the last command has been completed and that we
now need to clean up.
This commit guards the revert/cherry-pick specific steps so that they
are not executed in rebase -i mode.
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 abffaf3b40..4ceb6f3ac5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1882,25 +1882,28 @@ int sequencer_continue(struct replay_opts *opts)
if (commit_staged_changes(opts))
return -1;
}
- if (!file_exists(get_todo_path(opts)))
+ else if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts))
return -1;
if ((res = read_populate_todo(&todo_list, opts)))
goto release_todo_list;
- /* Verify that the conflict has been resolved */
- if (file_exists(git_path_cherry_pick_head()) ||
- file_exists(git_path_revert_head())) {
- res = continue_single_pick();
- if (res)
+ if (!is_rebase_i(opts)) {
+ /* Verify that the conflict has been resolved */
+ if (file_exists(git_path_cherry_pick_head()) ||
+ file_exists(git_path_revert_head())) {
+ res = continue_single_pick();
+ if (res)
+ goto release_todo_list;
+ }
+ if (index_differs_from("HEAD", 0, 0)) {
+ res = error_dirty_index(opts);
goto release_todo_list;
+ }
+ todo_list.current++;
}
- if (index_differs_from("HEAD", 0, 0)) {
- res = error_dirty_index(opts);
- goto release_todo_list;
- }
- todo_list.current++;
+
res = pick_commits(&todo_list, opts);
release_todo_list:
todo_list_release(&todo_list);
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 03/34] sequencer (rebase -i): implement the 'edit' command
From: Johannes Schindelin @ 2016-12-13 15:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
This patch is a straight-forward reimplementation of the `edit`
operation of the interactive rebase command.
Well, not *quite* straight-forward: when stopping, the `edit`
command wants to write the `patch` file (which is not only the
patch, but includes the commit message and author information). To
that end, this patch requires the earlier work that taught the
log-tree machinery to respect the `file` setting of
rev_info->diffopt to write to a file stream different than stdout.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 114 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1224799286..68e2c84803 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -16,6 +16,7 @@
#include "refs.h"
#include "argv-array.h"
#include "quote.h"
+#include "log-tree.h"
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -43,6 +44,20 @@ static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
*/
static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
/*
+ * When an "edit" rebase command is being processed, the SHA1 of the
+ * commit to be edited is recorded in this file. When "git rebase
+ * --continue" is executed, if there are any staged changes then they
+ * will be amended to the HEAD commit, but only provided the HEAD
+ * commit is still the commit to be edited. When any other rebase
+ * command is processed, this file is deleted.
+ */
+static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
+/*
+ * When we stop at a given patch via the "edit" command, this file contains
+ * the long commit name of the corresponding patch.
+ */
+static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
+/*
* The following files are written by git-rebase just after parsing the
* command-line (and are only consumed, not modified, by the sequencer).
*/
@@ -648,6 +663,7 @@ enum todo_command {
/* commands that handle commits */
TODO_PICK = 0,
TODO_REVERT,
+ TODO_EDIT,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP
};
@@ -655,6 +671,7 @@ enum todo_command {
static const char *todo_command_strings[] = {
"pick",
"revert",
+ "edit",
"noop"
};
@@ -1301,9 +1318,87 @@ static int save_opts(struct replay_opts *opts)
return res;
}
+static int make_patch(struct commit *commit, struct replay_opts *opts)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct rev_info log_tree_opt;
+ const char *commit_buffer = get_commit_buffer(commit, NULL), *subject, *p;
+ int res = 0;
+
+ p = short_commit_name(commit);
+ if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
+ return -1;
+
+ strbuf_addf(&buf, "%s/patch", get_dir(opts));
+ memset(&log_tree_opt, 0, sizeof(log_tree_opt));
+ init_revisions(&log_tree_opt, NULL);
+ log_tree_opt.abbrev = 0;
+ log_tree_opt.diff = 1;
+ log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
+ log_tree_opt.disable_stdin = 1;
+ log_tree_opt.no_commit_id = 1;
+ log_tree_opt.diffopt.file = fopen(buf.buf, "w");
+ log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
+ if (!log_tree_opt.diffopt.file)
+ res |= error_errno(_("could not open '%s'"), buf.buf);
+ else {
+ res |= log_tree_commit(&log_tree_opt, commit);
+ fclose(log_tree_opt.diffopt.file);
+ }
+ strbuf_reset(&buf);
+
+ strbuf_addf(&buf, "%s/message", get_dir(opts));
+ if (!file_exists(buf.buf)) {
+ find_commit_subject(commit_buffer, &subject);
+ res |= write_message(subject, strlen(subject), buf.buf, 1);
+ unuse_commit_buffer(commit, commit_buffer);
+ }
+ strbuf_release(&buf);
+
+ return res;
+}
+
+static int intend_to_amend(void)
+{
+ unsigned char head[20];
+ char *p;
+
+ if (get_sha1("HEAD", head))
+ return error(_("cannot read HEAD"));
+
+ p = sha1_to_hex(head);
+ return write_message(p, strlen(p), rebase_path_amend(), 1);
+}
+
+static int error_with_patch(struct commit *commit,
+ const char *subject, int subject_len,
+ struct replay_opts *opts, int exit_code, int to_amend)
+{
+ if (make_patch(commit, opts))
+ return -1;
+
+ if (to_amend) {
+ if (intend_to_amend())
+ return -1;
+
+ fprintf(stderr, "You can amend the commit now, with\n"
+ "\n"
+ " git commit --amend %s\n"
+ "\n"
+ "Once you are satisfied with your changes, run\n"
+ "\n"
+ " git rebase --continue\n", gpg_sign_opt_quoted(opts));
+ }
+ else if (exit_code)
+ fprintf(stderr, "Could not apply %s... %.*s\n",
+ short_commit_name(commit), subject_len, subject);
+
+ return exit_code;
+}
+
static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
{
- int res;
+ int res = 0;
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
if (opts->allow_ff)
@@ -1316,9 +1411,20 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
struct todo_item *item = todo_list->items + todo_list->current;
if (save_todo(todo_list, opts))
return -1;
- if (item->command <= TODO_REVERT)
+ if (item->command <= TODO_EDIT) {
res = do_pick_commit(item->command, item->commit,
opts);
+ if (item->command == TODO_EDIT) {
+ struct commit *commit = item->commit;
+ if (!res)
+ warning(_("stopped at %s... %.*s"),
+ short_commit_name(commit),
+ item->arg_len, item->arg);
+ return error_with_patch(commit,
+ item->arg, item->arg_len, opts, res,
+ !res);
+ }
+ }
else if (item->command != TODO_NOOP)
return error(_("unknown command %d"), item->command);
@@ -1327,6 +1433,12 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
return res;
}
+ if (is_rebase_i(opts)) {
+ /* Stopped in the middle, as planned? */
+ if (todo_list->current < todo_list->nr)
+ return 0;
+ }
+
/*
* Sequence of picks finished successfully; cleanup by
* removing the .git/sequencer directory
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 04/34] sequencer (rebase -i): implement the 'exec' command
From: Johannes Schindelin @ 2016-12-13 15:29 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <cover.1481642927.git.johannes.schindelin@gmx.de>
The 'exec' command is a little special among rebase -i's commands, as it
does *not* have a SHA-1 as first parameter. Instead, everything after the
`exec` command is treated as command-line to execute.
Let's reuse the arg/arg_len fields of the todo_item structure (which hold
the oneline for pick/edit commands) to point to the command-line.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 68e2c84803..700b7575ed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -17,6 +17,7 @@
#include "argv-array.h"
#include "quote.h"
#include "log-tree.h"
+#include "wt-status.h"
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -664,6 +665,8 @@ enum todo_command {
TODO_PICK = 0,
TODO_REVERT,
TODO_EDIT,
+ /* commands that do something else than handling a single commit */
+ TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP
};
@@ -672,6 +675,7 @@ static const char *todo_command_strings[] = {
"pick",
"revert",
"edit",
+ "exec",
"noop"
};
@@ -971,6 +975,12 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
return -1;
bol += padding;
+ if (item->command == TODO_EXEC) {
+ item->arg = bol;
+ item->arg_len = (int)(eol - bol);
+ return 0;
+ }
+
end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
saved = *end_of_object_name;
*end_of_object_name = '\0';
@@ -1396,6 +1406,47 @@ static int error_with_patch(struct commit *commit,
return exit_code;
}
+static int do_exec(const char *command_line)
+{
+ const char *child_argv[] = { NULL, NULL };
+ int dirty, status;
+
+ fprintf(stderr, "Executing: %s\n", command_line);
+ child_argv[0] = command_line;
+ status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+
+ /* force re-reading of the cache */
+ if (discard_cache() < 0 || read_cache() < 0)
+ return error(_("could not read index"));
+
+ dirty = require_clean_work_tree("rebase", NULL, 1, 1);
+
+ if (status) {
+ warning(_("execution failed: %s\n%s"
+ "You can fix the problem, and then run\n"
+ "\n"
+ " git rebase --continue\n"
+ "\n"),
+ command_line,
+ dirty ? N_("and made changes to the index and/or the "
+ "working tree\n") : "");
+ if (status == 127)
+ /* command not found */
+ status = 1;
+ }
+ else if (dirty) {
+ warning(_("execution succeeded: %s\nbut "
+ "left changes to the index and/or the working tree\n"
+ "Commit or stash your changes, and then run\n"
+ "\n"
+ " git rebase --continue\n"
+ "\n"), command_line);
+ status = 1;
+ }
+
+ return status;
+}
+
static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
{
int res = 0;
@@ -1425,6 +1476,14 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
!res);
}
}
+ else if (item->command == TODO_EXEC) {
+ char *end_of_arg = (char *)(item->arg + item->arg_len);
+ int saved = *end_of_arg;
+
+ *end_of_arg = '\0';
+ res = do_exec(item->arg);
+ *end_of_arg = saved;
+ }
else if (item->command != TODO_NOOP)
return error(_("unknown command %d"), item->command);
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf
From: Jeff King @ 2016-12-13 13:55 UTC (permalink / raw)
To: Elia Pinto; +Cc: git
In-Reply-To: <20161213132717.42965-1-gitter.spiros@gmail.com>
On Tue, Dec 13, 2016 at 01:27:17PM +0000, Elia Pinto wrote:
> With the commits f2f02675 and 5096d490 we have been converted in some files the call from snprintf/sprintf/strcpy to xsnprintf. This patch supersedes the
> snprintf with several calls that make use of the heap rather than fixed
> length buffers (eg. PATH_MAX) that may be incorrect on some systems.
I had trouble parsing this, because it leads with mention of commits
that _aren't_ doing the same thing we are doing here. I think the
argument you want to make is basically:
1. snprintf is bad because it may silently truncate results if we're
wrong.
2. In cases where we use PATH_MAX, we'd want to handle larger paths
anyway, so we should switch to dynamic allocation.
3. In other cases where we know the input is bounded to a certain
length we could use xsnprintf(), which asserts that we don't
truncate. But by switching to dynamic allocation, we can avoid
dealing with magic numbers in the code.
I'd actually consider splitting cases (2) and (3) into separate patches.
Even though the end result is the same, the reasoning is quite
different.
As far as the patch itself goes, they all look like improvements to me.
The extra allocations shouldn't matter because these are all heavy
operations already.
A few minor nits:
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0ed634b26..37228330c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> return 0;
>
> if (use_editor) {
> - char index[PATH_MAX];
> - const char *env[2] = { NULL };
> - env[0] = index;
> - snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> - if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
> + struct argv_array env = ARGV_ARRAY_INIT;
> +
> + argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
> + if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
> fprintf(stderr,
> _("Please supply the message using either -m or -F option.\n"));
> + argv_array_clear(&env);
> exit(1);
> }
> + argv_array_clear(&env);
I'd generally skip the clear() right before exiting, though I saw
somebody disagree with me recently on that. I wonder if we should
decide as a project on whether it is worth doing or not.
> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> static int run_rewrite_hook(const unsigned char *oldsha1,
> const unsigned char *newsha1)
> {
> - /* oldsha1 SP newsha1 LF NUL */
> - static char buf[2*40 + 3];
> + char *buf;
> struct child_process proc = CHILD_PROCESS_INIT;
> const char *argv[3];
> int code;
> - size_t n;
>
> argv[0] = find_hook("post-rewrite");
> if (!argv[0])
> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
> code = start_command(&proc);
> if (code)
> return code;
> - n = snprintf(buf, sizeof(buf), "%s %s\n",
> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> sigchain_push(SIGPIPE, SIG_IGN);
> - write_in_full(proc.in, buf, n);
> + write_in_full(proc.in, buf, strlen(buf));
> close(proc.in);
> + free(buf);
Any time you care about the length of the result, I'd generally use an
actual strbuf instead of xstrfmt. The extra strlen isn't a big deal
here, but it somehow seems simpler to me. It probably doesn't matter
much either way, though.
> int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
> {
> - const char *hook_env[3] = { NULL };
> - char index[PATH_MAX];
> + struct argv_array hook_env = ARGV_ARRAY_INIT;
> va_list args;
> int ret;
>
> - snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> - hook_env[0] = index;
> + argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
>
> /*
> * Let the hook know that no editor will be launched.
> */
> if (!editor_is_used)
> - hook_env[1] = "GIT_EDITOR=:";
> + argv_array_push(&hook_env, "GIT_EDITOR=:");
>
> va_start(args, name);
> - ret = run_hook_ve(hook_env, name, args);
> + ret = run_hook_ve(hook_env.argv,name, args);
> va_end(args);
> + argv_array_clear(&hook_env);
Missing whitespace between arguments in the run_hook_ve() call.
-Peff
^ permalink raw reply
* [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf
From: Elia Pinto @ 2016-12-13 13:27 UTC (permalink / raw)
To: git; +Cc: Elia Pinto
With the commits f2f02675 and 5096d490 we have been converted in some files the call from snprintf/sprintf/strcpy to xsnprintf. This patch supersedes the
snprintf with several calls that make use of the heap rather than fixed
length buffers (eg. PATH_MAX) that may be incorrect on some systems.
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
builtin/commit.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 0ed634b26..37228330c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
return 0;
if (use_editor) {
- char index[PATH_MAX];
- const char *env[2] = { NULL };
- env[0] = index;
- snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
- if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
+ struct argv_array env = ARGV_ARRAY_INIT;
+
+ argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
+ if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
fprintf(stderr,
_("Please supply the message using either -m or -F option.\n"));
+ argv_array_clear(&env);
exit(1);
}
+ argv_array_clear(&env);
}
if (!no_verify &&
@@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
static int run_rewrite_hook(const unsigned char *oldsha1,
const unsigned char *newsha1)
{
- /* oldsha1 SP newsha1 LF NUL */
- static char buf[2*40 + 3];
+ char *buf;
struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
- size_t n;
argv[0] = find_hook("post-rewrite");
if (!argv[0])
@@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
code = start_command(&proc);
if (code)
return code;
- n = snprintf(buf, sizeof(buf), "%s %s\n",
- sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+ buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
sigchain_push(SIGPIPE, SIG_IGN);
- write_in_full(proc.in, buf, n);
+ write_in_full(proc.in, buf, strlen(buf));
close(proc.in);
+ free(buf);
sigchain_pop(SIGPIPE);
return finish_command(&proc);
}
int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
{
- const char *hook_env[3] = { NULL };
- char index[PATH_MAX];
+ struct argv_array hook_env = ARGV_ARRAY_INIT;
va_list args;
int ret;
- snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
- hook_env[0] = index;
+ argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
/*
* Let the hook know that no editor will be launched.
*/
if (!editor_is_used)
- hook_env[1] = "GIT_EDITOR=:";
+ argv_array_push(&hook_env, "GIT_EDITOR=:");
va_start(args, name);
- ret = run_hook_ve(hook_env, name, args);
+ ret = run_hook_ve(hook_env.argv,name, args);
va_end(args);
+ argv_array_clear(&hook_env);
return ret;
}
--
2.11.0.153.gacc9cba
^ permalink raw reply related
* Re: [PATCH 3/4] doc: make the intent of sentence clearer
From: Kristoffer Haugsbakk @ 2016-12-13 13:23 UTC (permalink / raw)
To: Philip Oakley; +Cc: git, Kristoffer Haugsbakk
In-Reply-To: <34DFA15DE05248069167FE613B66CD70@PhilipOakley>
Philip Oakley writes:
> Looks like a reasonable emphasis to me
I'll add this header:
Acked-by: Philip Oakley <philipoakley@iee.org>
To the commit message of this patch in the next review round (version 2
of the patch series).
Let me know if I should add another header, or do something different
than this.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 1/4] doc: add articles (grammar)
From: Kristoffer Haugsbakk @ 2016-12-13 13:05 UTC (permalink / raw)
To: Philip Oakley; +Cc: git, Kristoffer Haugsbakk
In-Reply-To: <88A192B34B3D4DDDA47628687AE458D3@PhilipOakley>
Thank you for reviewing this series, Philip.
Philip Oakley writes:
> This looks good to me.
I'll add this header:
Acked-by: Philip Oakley <philipoakley@iee.org>
To the commit message of this patch in the next review round (version 2
of the patch series).
Let me know if I should add another header, or do something different
than this.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [RFC/PATCH] Makefile: add cppcheck target
From: Jeff King @ 2016-12-13 12:28 UTC (permalink / raw)
To: Chris Packham; +Cc: git, gitter.spiros
In-Reply-To: <20161213121510.5o5axuwzztbxcvfd@sigill.intra.peff.net>
On Tue, Dec 13, 2016 at 07:15:10AM -0500, Jeff King wrote:
> I think these last two are a good sign that we need to be feeding the
> list of source files to cppcheck. I tried your patch and it also started
> looking in t/perf/build, which are old versions of git built to serve
> the performance-testing suite.
>
> See the way that the "tags" target is handled for a possible approach.
Maybe something like this:
diff --git a/Makefile b/Makefile
index 8b5976d88..e7684ae63 100644
--- a/Makefile
+++ b/Makefile
@@ -2638,4 +2638,6 @@ cover_db_html: cover_db
.PHONY: cppcheck
cppcheck:
- cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
+ $(FIND_SOURCE_FILES) |\
+ grep -v ^t/t |\
+ xargs cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD)
> My main complaint with any static checker is how we can handle false
> positives. [...]
Here's what that generates on my machine, with annotations:
[builtin/am.c:766]: (error) Resource leak: in
Correct.
[builtin/notes.c:260]: (error) Memory pointed to by 'buf' is freed twice.
[builtin/notes.c:264]: (error) Memory pointed to by 'buf' is freed twice.
Nope. It appears not to understand that die() does not return.
[builtin/rev-list.c:391]: (error) Uninitialized variable: reaches
[builtin/rev-list.c:391]: (error) Uninitialized variable: all
These are "int foo = foo" (which is ugly, and maybe we can get rid of
if compilers have gotten smart enough to figure it out).
[compat/nedmalloc/malloc.c.h:4646]: (error) Memory leak: mem
Hard to tell, but I think this is wrong and is confused by pointer
arithmetic on the malloc chunks.
[compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
Nope, this return is hit only when sbcset is NULL. The tool is
presumably confused by a conditional hidden inside a macro.
[compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
[compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
[compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
I didn't look at these, but presumably similar.
[compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
[compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
Not sure, but it looks like this function declares another inline
function inside its scope, and that confuses the tool.
[compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
Nope. Tool seems confused by hiding free() in a macro.
[contrib/examples/builtin-fetch--tool.c:420]: (error) Uninitialized variable: lrr_count
[contrib/examples/builtin-fetch--tool.c:427]: (error) Uninitialized variable: lrr_list
More "int foo = foo". Might be worth omitting contrib/examples (or
possibly contrib/ entirely) from the check.
[t/helper/test-hashmap.c:125]: (error) Memory leak: entries
[t/helper/test-hashmap.c:125]: (error) Memory leak: hashes
Correct (but unimportant).
So I think it is capable of finding real problems, but I think we'd need
some way of squelching false positives, preferably in a way that carries
forward as the code changes (so not just saying "foo.c:1234 is a false
positive", which will break when it becomes "foo.c:1235").
-Peff
^ permalink raw reply related
* Re: [RFC/PATCH] Makefile: add cppcheck target
From: Jeff King @ 2016-12-13 12:15 UTC (permalink / raw)
To: Chris Packham; +Cc: git, gitter.spiros
In-Reply-To: <20161213092225.15299-1-judge.packham@gmail.com>
On Tue, Dec 13, 2016 at 10:22:25PM +1300, Chris Packham wrote:
> $ make cppcheck
> cppcheck --force --quiet --inline-suppr .
> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.
>
> The last 2 are just false positives from test data. I haven't looked
> into any of the others.
I think these last two are a good sign that we need to be feeding the
list of source files to cppcheck. I tried your patch and it also started
looking in t/perf/build, which are old versions of git built to serve
the performance-testing suite.
See the way that the "tags" target is handled for a possible approach.
My main complaint with any static checker is how we can handle false
positives. I think our use of "-Wall -Werror" is successful because it's
not too hard to keep the normal state to zero warnings. Looking at the
output of cppcheck on my system (which is different than on yours!), I
do see a few real problems, but many false positives, too.
Unfortunately, one of the false positives is:
int foo = foo;
to silence -Wuninitialized, which causes cppcheck to complain that "foo"
is uninitialized. I'm worried we will end up with two static checkers
fighting each other, and no good way to please both.
-Peff
^ permalink raw reply
* Re: [PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'
From: Jeff King @ 2016-12-13 11:59 UTC (permalink / raw)
To: Chris Packham; +Cc: git, mah, jacob.keller, gitster
In-Reply-To: <20161213084859.13426-1-judge.packham@gmail.com>
On Tue, Dec 13, 2016 at 09:48:58PM +1300, Chris Packham wrote:
> + if (continue_current_merge) {
> + int nargc = 1;
> + const char *nargv[] = {"commit", NULL};
> +
> + if (argc)
> + usage_msg_opt("--continue expects no arguments",
> + builtin_merge_usage, builtin_merge_options);
This checks that we don't have:
git merge --continue foobar
but still allows:
git merge --continue --some-option
because parse_options() decrements argc.
It would be insane to check individually which options might have been
set. But I wonder if we could do something like:
int orig_argc = argc;
...
argc = parse_options(argc, argv, ...);
if (continue_current_merge) {
if (orig_argc != 1) /* maybe 2, to account for argv[0] ? */
usage_msg_opt("--continue expects no arguments", ...);
}
That gets trickier if there ever is an option that's OK to use with
--continue. We might want to forward along "--quiet", for example. On
the other hand, we silently ignore it now, so maybe it is better to
complain and then let --quiet get added later if somebody cares.
Whatever we do here, I think "--abort" should get the same treatment
(probably as a separate patch).
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 85248a14b..44b34ef3a 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -154,6 +154,7 @@ test_expect_success 'test option parsing' '
> test_must_fail git merge -s foobar c1 &&
> test_must_fail git merge -s=foobar c1 &&
> test_must_fail git merge -m &&
> + test_must_fail git merge --continue foobar &&
> test_must_fail git merge
> '
Your tests look good, though obviously if you check for options above,
that should be covered in this test.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] alternates: accept double-quoted paths
From: Jeff King @ 2016-12-13 11:52 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Johannes Sixt, Klaus Ethgen, Git Mailing List
In-Reply-To: <CACsJy8B52ZDRTUjGLqub_1wELtugv99xbDnBg1PX1LUTb6nVMQ@mail.gmail.com>
On Tue, Dec 13, 2016 at 06:30:15PM +0700, Duy Nguyen wrote:
> On Tue, Dec 13, 2016 at 2:52 AM, Jeff King <peff@peff.net> wrote:
> > Instead, let's treat names as unquoted unless they begin
> > with a double-quote, in which case they are interpreted via
> > our usual C-stylke quoting rules. This also breaks
> > backwards-compatibility, but in a smaller way: it only
> > matters if your file has a double-quote as the very _first_
> > character in the path (whereas an escape character is a
> > problem anywhere in the path). It's also consistent with
> > many other parts of git, which accept either a bare pathname
> > or a double-quoted one, and the sender can choose to quote
> > or not as required.
>
> At least attr has the same problem and is going the same direction
> [1]. Cool. (I actually thought the patch was in and evidence that this
> kind of backward compatibility breaking was ok, turns out the patch
> has stayed around for years)
>
> [1] http://public-inbox.org/git/%3C20161110203428.30512-18-sbeller@google.com%3E/
Thanks for digging that up. As soon as I came up with the idea[1], I
wanted to use the attr code as an example of a similar problem and
solution, but I couldn't find it in the code. Which makes sense if it
wasn't merged.
I do think it's a pretty reasonable approach in general, and would be OK
for the attributes code.
-Peff
[1] One could argue that I did not come up with the idea at all, but
rather just remembered that somebody else had done so. :)
^ permalink raw reply
* Re: [PATCH 0/2] handling alternates paths with colons
From: Jeff King @ 2016-12-13 11:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <xmqqvauo7p0r.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 12, 2016 at 02:37:08PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So here are patches that do that. It kicks in only when the first
> > character of a path is a double-quote, and then expects the usual
> > C-style quoting.
>
> The quote being per delimited component is what makes this fifth
> approach a huge win.
>
> All sane components on a list-valued environment are still honored
> and an insane component that has either a colon in the middle or
> begins with a double-quote gets quoted. As long as nobody used a
> path that begins with a double-quote as an element in such a
> list-valued environment (and they cannot be, as using a non-absolute
> path as an element does not make much sense), this will be safe, and
> a path with a colon didn't work with Git unaware of the new quoting
> rule anyway. Nice.
We do support non-absolute paths, both in alternates files and
environment variables. It's a nice feature if you want to have a
relocatable family of shared repositories. I'd imagine that most cases
start with "../", though.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
From: Jeff King @ 2016-12-13 11:44 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Klaus Ethgen, git
In-Reply-To: <d83cd58f-9b52-cbc5-04dd-5aafe2822533@kdbg.org>
On Mon, Dec 12, 2016 at 09:53:11PM +0100, Johannes Sixt wrote:
> > The appropriate check there would be ";" anyway, but I am not
> > sure _that_ is allowed in paths, either.
>
> That was also my line of thought. I tested earlier today whether a file name
> can have ";", and the OS did not reject it bluntly. Let me see whether I can
> adjust the test case for Windows.
The naive conversion is just:
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 6275ec807..e195e168b 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -33,8 +33,13 @@ test_expect_success 'rejected objects are removed' '
test_cmp expect actual
'
-# MINGW does not allow colons in pathnames in the first place
-test_expect_success !MINGW 'push to repo path with colon' '
+if test_have_prereq MINGW
+then
+ path_sep=';'
+else
+ path_sep=':'
+fi
+test_expect_success 'push to repo path with (semi-)colon separator' '
# The interesting failure case here is when the
# receiving end cannot access its original object directory,
# so make it likely for us to generate a delta by having
@@ -43,13 +48,13 @@ test_expect_success !MINGW 'push to repo path with colon' '
test-genrandom foo 4096 >file.bin &&
git add file.bin &&
git commit -m bin &&
- git clone --bare . xxx:yyy.git &&
+ git clone --bare . xxx${path_sep}yyy.git &&
echo change >>file.bin &&
git commit -am change &&
# Note that we have to use the full path here, or it gets confused
# with the ssh host:path syntax.
- git push "$PWD/xxx:yyy.git" HEAD
+ git push "$PWD/xxx${path_sep}yyy.git" HEAD
'
test_done
Does that work?
-Peff
^ permalink raw reply related
* Re: [PATCH 1/2] alternates: accept double-quoted paths
From: Duy Nguyen @ 2016-12-13 11:30 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Sixt, Klaus Ethgen, Git Mailing List
In-Reply-To: <20161212195222.rxnabok6amklt2zf@sigill.intra.peff.net>
On Tue, Dec 13, 2016 at 2:52 AM, Jeff King <peff@peff.net> wrote:
> Instead, let's treat names as unquoted unless they begin
> with a double-quote, in which case they are interpreted via
> our usual C-stylke quoting rules. This also breaks
> backwards-compatibility, but in a smaller way: it only
> matters if your file has a double-quote as the very _first_
> character in the path (whereas an escape character is a
> problem anywhere in the path). It's also consistent with
> many other parts of git, which accept either a bare pathname
> or a double-quoted one, and the sender can choose to quote
> or not as required.
At least attr has the same problem and is going the same direction
[1]. Cool. (I actually thought the patch was in and evidence that this
kind of backward compatibility breaking was ok, turns out the patch
has stayed around for years)
[1] http://public-inbox.org/git/%3C20161110203428.30512-18-sbeller@google.com%3E/
--
Duy
^ permalink raw reply
* Re: [PATCHv2] git-p4: support git worktrees
From: Duy Nguyen @ 2016-12-13 11:17 UTC (permalink / raw)
To: Luke Diamand
Cc: Git Users, Vinicius Kursancew, Lars Schneider, Junio C Hamano
In-Reply-To: <CAE5ih7_6Ap_dY3mRb3Hk2yzDRMkZ3HnnQOaikF=ybx_XNdVWhQ@mail.gmail.com>
On Sun, Dec 11, 2016 at 2:19 PM, Luke Diamand <luke@diamand.org> wrote:
> On 10 December 2016 at 21:57, Luke Diamand <luke@diamand.org> wrote:
>> git-p4 would attempt to find the git directory using
>> its own specific code, which did not know about git
>> worktrees. This caused git operations to fail needlessly.
>>
>> Rework it to use "git rev-parse --git-dir" instead, which
>> knows about worktrees.
>
> Actually this doesn't work as well as the original version. "git
> rev-parse --git-dir" won't go and find the ".git" subdirectory. The
> previous version would go looking for it, so this would introduce a
> regression.
Maybe git rev-parse --git-path HEAD, then strip out the /HEAD part?
--
Duy
^ permalink raw reply
* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Vasco Almeida @ 2016-12-13 11:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Jiang Xin,
Ævar Arnfjörð Bjarmason, Jean-Noël AVILA,
Jakub Narębski, David Aguilar
In-Reply-To: <xmqqy3zno2qv.fsf@gitster.mtv.corp.google.com>
A Sáb, 10-12-2016 às 14:09 -0800, Junio C Hamano escreveu:
> We only update comment_line_char from the default "#" when the
> configured value is a single-byte character and we ignore incorrect
> values in the configuration file. So I think the patch you sent is
> correct after all.
I am still not sure what version do we prefer.
Check whether core.commentchar is a single character. If not, use '#'
as the $comment_line_char.
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ $comment_line_char = '#' if (length($comment_line_char) != 1);
+ return $comment_line_char;
+}
Check whether core.commentchar is a single character. If not, use the
first character of the core.commentchar value, mirroring the "rebase
-i" behavior introduced recently.
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ if (length($comment_line_char) != 1) {
+ # use first character
+ $comment_line_char = substr($comment_line_char, 0, 1);
+ }
+ return $comment_line_char;
+}
Or akin to what I had in the first patch related to handling 'auto'
value of core.commentchar configuration variable:
+sub get_comment_line_char {
+ my $comment_line_char = config("core.commentchar") || '#';
+ $comment_line_char = '#' if ($comment_line_char eq 'auto');
+ return $comment_line_char;
+}
Which assumes that the value of core.commentchar configuration variable
is either 'auto' or one single character, or the variable is not
defined.
^ permalink raw reply
* Re: [RFC/PATCH] Makefile: add cppcheck target
From: stefan.naewe @ 2016-12-13 9:37 UTC (permalink / raw)
To: judge.packham, git; +Cc: gitter.spiros
In-Reply-To: <CAFOYHZAZAH9Rt1o73cx2uFvtr4weL00J+Yktei3h2GN1JgbY=A@mail.gmail.com>
Am 13.12.2016 um 10:32 schrieb Chris Packham:
> On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham <judge.packham@gmail.com> wrote:
>> Add cppcheck target to Makefile. Cppcheck is a static
>> analysis tool for C/C++ code. Cppcheck primarily detects
>> the types of bugs that the compilers normally do not detect.
>> It is an useful target for doing QA analysis.
>>
>> Based-on-patch-by: Elia Pinto <gitter.spiros@gmail.com>
>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>> ---
>> I had been playing with cppcheck for some other projects and happened to
>> notice [1] in the archives. This is my attempt to resolve the feedback
>> that Junio made at the time.
>>
>> In terms of errors that are actually reported there are only a few
>>
>> $ make cppcheck
>> cppcheck --force --quiet --inline-suppr .
>> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
>> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
>> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
>> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
>> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
>> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
>> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
>> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
>> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
>> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
>> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
>> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.
>>
>> The last 2 are just false positives from test data. I haven't looked
>> into any of the others.
>>
>> I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
>> in the make invocation.
>>
>> $ make cppcheck CPPCHECK_ADD=--enable=all
>> ... lots of output
>>
>> [1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t
>>
>> Makefile | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index f53fcc90d..8b5976d88 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2635,3 +2635,7 @@ cover_db: coverage-report
>> cover_db_html: cover_db
>> cover -report html -outputdir cover_db_html cover_db
>>
>> +.PHONY: cppcheck
>> +
>> +cppcheck:
>> + cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
>
> If I'm permitted a little GNU make-ism the following might make
> CPPCHECK_ADD a bit more usable
>
> + cppcheck --force --quiet --inline-suppr $(if
> $(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) .
>
> Which would take us from
>
> $ make cppcheck CPPCHECK_ADD=--enable=all
>
> to
>
> $ make cppcheck CPPCHECK_ADD=all
Hhhmmm....but this allows for only specifying options to '--enable'.
The other version is much more flexible (i.e. allows for other complete options as well).
Just my 0,02€
Stefan
--
----------------------------------------------------------------
/dev/random says: I'd love to, but I prefer to remain an enigma.
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
^ permalink raw reply
* Re: [RFC/PATCH] Makefile: add cppcheck target
From: Chris Packham @ 2016-12-13 9:32 UTC (permalink / raw)
To: GIT; +Cc: Elia Pinto, Chris Packham
In-Reply-To: <20161213092225.15299-1-judge.packham@gmail.com>
On Tue, Dec 13, 2016 at 10:22 PM, Chris Packham <judge.packham@gmail.com> wrote:
> Add cppcheck target to Makefile. Cppcheck is a static
> analysis tool for C/C++ code. Cppcheck primarily detects
> the types of bugs that the compilers normally do not detect.
> It is an useful target for doing QA analysis.
>
> Based-on-patch-by: Elia Pinto <gitter.spiros@gmail.com>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---
> I had been playing with cppcheck for some other projects and happened to
> notice [1] in the archives. This is my attempt to resolve the feedback
> that Junio made at the time.
>
> In terms of errors that are actually reported there are only a few
>
> $ make cppcheck
> cppcheck --force --quiet --inline-suppr .
> [compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
> [compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
> [compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
> [compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
> [compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
> [compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
> [compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
> [t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
> [t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.
>
> The last 2 are just false positives from test data. I haven't looked
> into any of the others.
>
> I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
> in the make invocation.
>
> $ make cppcheck CPPCHECK_ADD=--enable=all
> ... lots of output
>
> [1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t
>
> Makefile | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index f53fcc90d..8b5976d88 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2635,3 +2635,7 @@ cover_db: coverage-report
> cover_db_html: cover_db
> cover -report html -outputdir cover_db_html cover_db
>
> +.PHONY: cppcheck
> +
> +cppcheck:
> + cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
If I'm permitted a little GNU make-ism the following might make
CPPCHECK_ADD a bit more usable
+ cppcheck --force --quiet --inline-suppr $(if
$(CPPCHECK_ADD),--enable=$(CPPCHECK_ADD)) .
Which would take us from
$ make cppcheck CPPCHECK_ADD=--enable=all
to
$ make cppcheck CPPCHECK_ADD=all
^ permalink raw reply
* [RFC/PATCH] Makefile: add cppcheck target
From: Chris Packham @ 2016-12-13 9:22 UTC (permalink / raw)
To: git; +Cc: gitter.spiros, Chris Packham
Add cppcheck target to Makefile. Cppcheck is a static
analysis tool for C/C++ code. Cppcheck primarily detects
the types of bugs that the compilers normally do not detect.
It is an useful target for doing QA analysis.
Based-on-patch-by: Elia Pinto <gitter.spiros@gmail.com>
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
I had been playing with cppcheck for some other projects and happened to
notice [1] in the archives. This is my attempt to resolve the feedback
that Junio made at the time.
In terms of errors that are actually reported there are only a few
$ make cppcheck
cppcheck --force --quiet --inline-suppr .
[compat/nedmalloc/malloc.c.h:4093]: (error) Possible null pointer dereference: sp
[compat/nedmalloc/malloc.c.h:4106]: (error) Possible null pointer dereference: sp
[compat/nedmalloc/nedmalloc.c:551]: (error) Expression '*(&p.mycache)=TlsAlloc(),TLS_OUT_OF_INDEXES==*(&p.mycache)' depends on order of evaluation of side effects
[compat/regex/regcomp.c:3086]: (error) Memory leak: sbcset
[compat/regex/regcomp.c:3634]: (error) Memory leak: sbcset
[compat/regex/regcomp.c:3086]: (error) Memory leak: mbcset
[compat/regex/regcomp.c:3634]: (error) Memory leak: mbcset
[compat/regex/regcomp.c:2802]: (error) Uninitialized variable: table_size
[compat/regex/regcomp.c:2805]: (error) Uninitialized variable: table_size
[compat/regex/regcomp.c:532]: (error) Memory leak: fastmap
[t/t4051/appended1.c:3]: (error) Invalid number of character '{' when these macros are defined: ''.
[t/t4051/appended2.c:35]: (error) Invalid number of character '{' when these macros are defined: ''.
The last 2 are just false positives from test data. I haven't looked
into any of the others.
I've also provisioned for enabling extra checks by passing CPPCHECK_ADD
in the make invocation.
$ make cppcheck CPPCHECK_ADD=--enable=all
... lots of output
[1] - http://public-inbox.org/git/1390993371-2431-1-git-send-email-gitter.spiros@gmail.com/#t
Makefile | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Makefile b/Makefile
index f53fcc90d..8b5976d88 100644
--- a/Makefile
+++ b/Makefile
@@ -2635,3 +2635,7 @@ cover_db: coverage-report
cover_db_html: cover_db
cover -report html -outputdir cover_db_html cover_db
+.PHONY: cppcheck
+
+cppcheck:
+ cppcheck --force --quiet --inline-suppr $(CPPCHECK_ADD) .
--
2.11.0.24.ge6920cf
^ permalink raw reply related
* [PATCHv2 2/2] completion: add --continue option for merge
From: Chris Packham @ 2016-12-13 8:48 UTC (permalink / raw)
To: git; +Cc: mah, peff, jacob.keller, gitster, Chris Packham
In-Reply-To: <20161213084859.13426-1-judge.packham@gmail.com>
Add 'git merge --continue' option when completing.
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
Notes:
Changes in v2:
- new.
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 21016bf8d..1f97ffae1 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1552,7 +1552,7 @@ _git_merge ()
case "$cur" in
--*)
__gitcomp "$__git_merge_options
- --rerere-autoupdate --no-rerere-autoupdate --abort"
+ --rerere-autoupdate --no-rerere-autoupdate --abort --continue"
return
esac
__gitcomp_nl "$(__git_refs)"
--
2.11.0.24.ge6920cf
^ 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